From 8b0e8b8192c2b2fd5ef26ff4e61996c13fd797ea Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 5 Aug 2026 11:05:28 +0800 Subject: [PATCH] fix: add onCompromised handlers to proper-lockfile calls to prevent daemon crash (#8442) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add onCompromised handlers to proper-lockfile calls to prevent daemon crash When a lock.lock directory is deleted while held (e.g. by another process cleaning stale locks), proper-lockfile's updateLock timer gets ENOENT on stat and calls the default onCompromised handler, which throws and crashes the process. Add onCompromised handlers that log instead of throw, consistent with existing handlers in tasks.ts and mailbox.ts. * fix: guard lock release after compromise and cover trusted folders (#8442) * fix: guard mailbox/task lock release and cover compromise handlers with tests (#8442) * test(core): add lock-compromise regression tests for mailbox and task guards (#8442) * test: share lock-compromise simulation via test-utils helpers (#8442) --------- Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: qwen-code-dev-bot --- .../cli/src/config/trustedFolders.test.ts | 24 ++++++++++++ packages/cli/src/config/trustedFolders.ts | 6 +++ .../src/test-utils/mock-compromised-lock.ts | 39 +++++++++++++++++++ .../cli/src/utils/managed-npm-update.test.ts | 31 +++++++++++++++ packages/cli/src/utils/managed-npm-update.ts | 12 +++++- packages/core/src/agents/team/mailbox.test.ts | 19 +++++++++ packages/core/src/agents/team/mailbox.ts | 8 +++- packages/core/src/agents/team/tasks.test.ts | 22 +++++++++++ packages/core/src/agents/team/tasks.ts | 8 +++- .../src/extension/extension-store.test.ts | 16 ++++++++ .../core/src/extension/extension-store.ts | 3 ++ .../core/src/memory/channel-memory.test.ts | 30 ++++++++++++++ packages/core/src/memory/channel-memory.ts | 6 +++ .../core/src/skills/skill-curator.test.ts | 18 +++++++++ packages/core/src/skills/skill-curator.ts | 6 +++ .../src/test-utils/mock-compromised-lock.ts | 39 +++++++++++++++++++ 16 files changed, 281 insertions(+), 6 deletions(-) create mode 100644 packages/cli/src/test-utils/mock-compromised-lock.ts create mode 100644 packages/core/src/test-utils/mock-compromised-lock.ts diff --git a/packages/cli/src/config/trustedFolders.test.ts b/packages/cli/src/config/trustedFolders.test.ts index 025f15d050..ac2895694f 100644 --- a/packages/cli/src/config/trustedFolders.test.ts +++ b/packages/cli/src/config/trustedFolders.test.ts @@ -23,6 +23,7 @@ import { import * as fs from 'node:fs'; import stripJsonComments from 'strip-json-comments'; import * as path from 'node:path'; +import lockfile from 'proper-lockfile'; import * as jsoncEditor from '../utils/jsonc-editor.js'; import { loadTrustedFolders, @@ -335,6 +336,29 @@ describe('Trusted Folders Loading', () => { expect(writtenContent).toContain('"/new/path": "TRUST_FOLDER"'); }); + it('saveTrustedFolders registers a lock-compromised handler', () => { + const userPath = getTrustedFoldersPath(); + const dirPath = path.dirname(userPath); + + (mockFsExistsSync as Mock).mockImplementation( + (p) => p === userPath || p === dirPath, + ); + (fs.readFileSync as Mock).mockReturnValue('{}'); + + saveTrustedFolders({ + path: userPath, + config: { + '/new/path': TrustLevel.TRUST_FOLDER, + }, + }); + + const options = vi.mocked(lockfile.lockSync).mock.calls.at(-1)?.[1]; + expect(options?.onCompromised).toBeTypeOf('function'); + expect(() => + options?.onCompromised?.(new Error('lock lost')), + ).not.toThrow(); + }); + it('saveTrustedFolders should reject malformed input without overwriting it', () => { const userPath = getTrustedFoldersPath(); const dirPath = path.dirname(userPath); diff --git a/packages/cli/src/config/trustedFolders.ts b/packages/cli/src/config/trustedFolders.ts index 3825926a0a..57f2ae803b 100644 --- a/packages/cli/src/config/trustedFolders.ts +++ b/packages/cli/src/config/trustedFolders.ts @@ -9,6 +9,7 @@ import * as path from 'node:path'; import lockfile from 'proper-lockfile'; import { atomicWriteFileSync, + createDebugLogger, FatalConfigError, getErrorMessage, ideContextStore, @@ -23,6 +24,8 @@ import { isWithinRoot, } from './path-comparison.js'; +const debugLogger = createDebugLogger('TRUSTED_FOLDERS'); + export const TRUSTED_FOLDERS_FILENAME = 'trustedFolders.json'; export function getTrustedFoldersPath(): string { @@ -255,6 +258,9 @@ function writeTrustedFolders( const release = lockfile.lockSync(filePath, { realpath: false, stale: 10_000, + onCompromised: (err) => { + debugLogger.warn('trusted folders lock compromised:', err); + }, }); try { let originalContent = '{}'; diff --git a/packages/cli/src/test-utils/mock-compromised-lock.ts b/packages/cli/src/test-utils/mock-compromised-lock.ts new file mode 100644 index 0000000000..325bf71e49 --- /dev/null +++ b/packages/cli/src/test-utils/mock-compromised-lock.ts @@ -0,0 +1,39 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import lockfile from 'proper-lockfile'; +import { vi } from 'vitest'; + +/** + * Simulates a proper-lockfile compromise on the next `lockfile.lock` call. + * After a real compromise, proper-lockfile marks the lock released before + * invoking `onCompromised`, so the later `release()` rejects with + * `ERELEASED`; the mocked release reproduces that exact state. + */ +export function mockCompromisedLock() { + let onCompromised: ((error: Error) => void) | undefined; + let lockedFile: string | undefined; + const lockSpy = vi + .spyOn(lockfile, 'lock') + .mockImplementationOnce(async (file, options) => { + lockedFile = file; + onCompromised = options?.onCompromised; + onCompromised?.( + Object.assign(new Error('lock lost'), { code: 'ECOMPROMISED' }), + ); + return () => + Promise.reject( + Object.assign(new Error('Lock is already released'), { + code: 'ERELEASED', + }), + ); + }); + return { + lockSpy, + getLockedFile: () => lockedFile, + getOnCompromised: () => onCompromised, + }; +} diff --git a/packages/cli/src/utils/managed-npm-update.test.ts b/packages/cli/src/utils/managed-npm-update.test.ts index fb4f2a54ba..eef3726fe0 100644 --- a/packages/cli/src/utils/managed-npm-update.test.ts +++ b/packages/cli/src/utils/managed-npm-update.test.ts @@ -17,6 +17,7 @@ import { import { EventEmitter } from 'node:events'; import type { spawn } from 'node:child_process'; import { createHash } from 'node:crypto'; +import { mockCompromisedLock } from '../test-utils/mock-compromised-lock.js'; const temporaryDirectories: string[] = []; @@ -130,6 +131,36 @@ describe('managed npm update', () => { expect(fs.readFileSync(runningChunk, 'utf8')).toBe('old chunk'); }); + it('still activates when the lock is compromised during activation', async () => { + const root = makeTemporaryDirectory(); + const bootstrap = writeBaseInstallation(root); + const update = prepareManagedNpmUpdate( + '2.0.0', + bootstrap, + path.join(root, 'updates'), + ); + writeInstallation(update.stagingDir, '2.0.0'); + + const { lockSpy, getOnCompromised } = mockCompromisedLock(); + + try { + await expect( + activateManagedNpmUpdate(update, '2.0.0', bootstrap), + ).resolves.toBeUndefined(); + expect(getOnCompromised()).toBeTypeOf('function'); + expect( + JSON.parse( + fs.readFileSync( + path.join(update.launcherRoot, 'active.json'), + 'utf8', + ), + ), + ).toMatchObject({ version: '2.0.0' }); + } finally { + lockSpy.mockRestore(); + } + }); + it('installs and activates inside the managed worker', async () => { const root = makeTemporaryDirectory(); const bootstrap = writeBaseInstallation(root); diff --git a/packages/cli/src/utils/managed-npm-update.ts b/packages/cli/src/utils/managed-npm-update.ts index c154bdf3f6..52443c76a2 100644 --- a/packages/cli/src/utils/managed-npm-update.ts +++ b/packages/cli/src/utils/managed-npm-update.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { Storage } from '@qwen-code/qwen-code-core'; +import { Storage, createDebugLogger } from '@qwen-code/qwen-code-core'; import { execFile, execFileSync, spawn } from 'node:child_process'; import { createHash } from 'node:crypto'; import * as fs from 'node:fs'; @@ -16,6 +16,7 @@ import semver from 'semver'; import { getNpmCliPath } from './installationInfo.js'; const PACKAGE_NAME = '@qwen-code/qwen-code'; +const debugLogger = createDebugLogger('MANAGED_NPM_UPDATE'); const execFileAsync = promisify(execFile); interface ManagedNpmUpdate { @@ -310,6 +311,9 @@ export async function activateManagedNpmUpdate( realpath: false, stale: 30_000, retries: { retries: 50, minTimeout: 20, maxTimeout: 100 }, + onCompromised: (err) => { + debugLogger.warn('managed npm update lock compromised:', err); + }, }); try { const base = readBaseInstallation(resolvedBootstrapPath); @@ -365,7 +369,11 @@ export async function activateManagedNpmUpdate( await fsPromises.rm(temporaryActivePath, { force: true }); } } finally { - await release(); + try { + await release(); + } catch (error) { + debugLogger.warn('Failed to release managed npm update lock:', error); + } } } diff --git a/packages/core/src/agents/team/mailbox.test.ts b/packages/core/src/agents/team/mailbox.test.ts index aaa847ebab..4c9798511e 100644 --- a/packages/core/src/agents/team/mailbox.test.ts +++ b/packages/core/src/agents/team/mailbox.test.ts @@ -19,6 +19,7 @@ import { sendStructuredMessage, disposeInboxLocks, } from './mailbox.js'; +import { mockCompromisedLock } from '../../test-utils/mock-compromised-lock.js'; vi.mock('../../config/storage.js', async (importOriginal) => { const original = @@ -139,6 +140,24 @@ describe('mailbox', () => { expect(texts).toContain('fresh'); }); + // ─── Lock compromise ───────────────────────────────────── + + it('still writes the message when the lock is compromised', async () => { + const { lockSpy, getOnCompromised } = mockCompromisedLock(); + + try { + await expect( + writeMessage('team', 'worker', makeMessage({ text: 'compromised' })), + ).resolves.toBeUndefined(); + expect(getOnCompromised()).toBeTypeOf('function'); + } finally { + lockSpy.mockRestore(); + } + + const messages = await readInbox('team', 'worker'); + expect(messages.map((m) => m.text)).toEqual(['compromised']); + }); + // ─── consumeUnread ───────────────────────────────────────── it('returns unread messages and marks them read', async () => { diff --git a/packages/core/src/agents/team/mailbox.ts b/packages/core/src/agents/team/mailbox.ts index bcfbd225f5..ab1e70bb91 100644 --- a/packages/core/src/agents/team/mailbox.ts +++ b/packages/core/src/agents/team/mailbox.ts @@ -83,7 +83,7 @@ const LOCK_OPTIONS: lockfile.LockOptions = { // Stale locks from crashed processes are expected in multi-agent // scenarios; log at debug level for traceability without noise. onCompromised: (err) => { - debug.debug('mailbox lock compromised:', err?.message ?? err); + debug.debug('mailbox lock compromised:', err); }, }; @@ -150,7 +150,11 @@ async function withInboxLock( try { return await fn(); } finally { - await release(); + try { + await release(); + } catch (error) { + debug.debug('Failed to release mailbox lock:', error); + } } }); } diff --git a/packages/core/src/agents/team/tasks.test.ts b/packages/core/src/agents/team/tasks.test.ts index 7a15dd8522..5fa71f4fb3 100644 --- a/packages/core/src/agents/team/tasks.test.ts +++ b/packages/core/src/agents/team/tasks.test.ts @@ -25,6 +25,7 @@ import { TaskOwnershipError, RECIPROCAL_CALLER, } from './tasks.js'; +import { mockCompromisedLock } from '../../test-utils/mock-compromised-lock.js'; vi.mock('../../config/storage.js', async (importOriginal) => { const original = @@ -153,6 +154,27 @@ describe('tasks', () => { expect(updated!.owner).toBe('worker@team'); }); + it('still updates the task when the lock is compromised', async () => { + const task = await createTask('team', { + subject: 'Test', + description: 'Desc', + }); + const { lockSpy, getOnCompromised } = mockCompromisedLock(); + + try { + await expect( + updateTask('team', task.id, { status: 'in_progress' }), + ).resolves.toMatchObject({ id: task.id, status: 'in_progress' }); + expect(getOnCompromised()).toBeTypeOf('function'); + } finally { + lockSpy.mockRestore(); + } + + expect(await getTask('team', task.id)).toMatchObject({ + status: 'in_progress', + }); + }); + it('clears owner with null', async () => { const task = await createTask('team', { subject: 'Test', diff --git a/packages/core/src/agents/team/tasks.ts b/packages/core/src/agents/team/tasks.ts index 54a7cd21bd..3bae460941 100644 --- a/packages/core/src/agents/team/tasks.ts +++ b/packages/core/src/agents/team/tasks.ts @@ -79,7 +79,7 @@ const LOCK_OPTIONS: lockfile.LockOptions = { }, stale: 5000, onCompromised: (err) => { - debug.warn('task lock compromised:', err?.message ?? err); + debug.warn('task lock compromised:', err); }, }; @@ -132,7 +132,11 @@ async function withTaskFileLock( try { return await fn(); } finally { - await release(); + try { + await release(); + } catch (error) { + debug.warn('Failed to release task lock:', error); + } } }); } diff --git a/packages/core/src/extension/extension-store.test.ts b/packages/core/src/extension/extension-store.test.ts index d299162845..b9db82e2c1 100644 --- a/packages/core/src/extension/extension-store.test.ts +++ b/packages/core/src/extension/extension-store.test.ts @@ -16,6 +16,7 @@ import { ExtensionStore, ExtensionStoreCorruptError, } from './extension-store.js'; +import { mockCompromisedLock } from '../test-utils/mock-compromised-lock.js'; describe('ExtensionStore', () => { let root: string; @@ -269,6 +270,21 @@ describe('ExtensionStore', () => { }); }); + it('registers a lock-compromised handler and completes when the store lock is compromised', async () => { + const store = makeStore(); + const identity = { id: 'd4'.repeat(32), name: 'demo' }; + const { lockSpy, getOnCompromised } = mockCompromisedLock(); + + try { + await expect(store.ensureInitialized([identity])).resolves.toMatchObject({ + generation: 0, + }); + expect(getOnCompromised()).toBeTypeOf('function'); + } finally { + lockSpy.mockRestore(); + } + }); + it('serializes mutations from two Node processes sharing QWEN_HOME', async () => { const id = 'd2'.repeat(32); const store = makeStore(); diff --git a/packages/core/src/extension/extension-store.ts b/packages/core/src/extension/extension-store.ts index 6acf159ec7..e58c19de09 100644 --- a/packages/core/src/extension/extension-store.ts +++ b/packages/core/src/extension/extension-store.ts @@ -1014,6 +1014,9 @@ export class ExtensionStore { maxTimeout: 500, randomize: true, }, + onCompromised: (err) => { + debugLogger.warn('extension store lock compromised:', err); + }, }); } catch (error) { throw new ExtensionStoreBusyError(this.storeDir, { cause: error }); diff --git a/packages/core/src/memory/channel-memory.test.ts b/packages/core/src/memory/channel-memory.test.ts index 6839786ca3..c14a51b8c6 100644 --- a/packages/core/src/memory/channel-memory.test.ts +++ b/packages/core/src/memory/channel-memory.test.ts @@ -55,6 +55,8 @@ const fsFailure = vi.hoisted(() => ({ const lockObservation = vi.hoisted(() => ({ path: undefined as string | undefined, attempted: undefined as (() => void) | undefined, + options: undefined as lockfile.LockOptions | undefined, + simulateCompromise: false, })); vi.mock('proper-lockfile', async (importOriginal) => { @@ -65,7 +67,17 @@ vi.mock('proper-lockfile', async (importOriginal) => { ...actual, async lock(...args: Parameters) { if (String(args[0]) === lockObservation.path) { + lockObservation.options = args[1]; lockObservation.attempted?.(); + const release = await actual.lock(...args); + if (lockObservation.simulateCompromise) { + // A real compromise marks the lock released in the registry + // before invoking onCompromised, so release() later rejects + // with ERELEASED. Reproduce that state exactly. + lockObservation.options?.onCompromised?.(new Error('lock lost')); + await release(); + } + return release; } return actual.lock(...args); }, @@ -164,6 +176,8 @@ describe('channel memory', () => { fsFailure.legacyAppendAfterRename = undefined; lockObservation.path = undefined; lockObservation.attempted = undefined; + lockObservation.options = undefined; + lockObservation.simulateCompromise = false; vi.restoreAllMocks(); if (originalQwenHome === undefined) { delete process.env['QWEN_HOME']; @@ -437,6 +451,22 @@ describe('channel memory', () => { } }); + it('completes mutations when the channel memory lock is compromised', async () => { + writeJson(serializeChannelMemoryDocument({ version: 1, entries: [] })); + lockObservation.path = path.join( + path.dirname(getChannelMemoryFilePath(target)), + '.channel-memory.lock', + ); + lockObservation.simulateCompromise = true; + + await expect( + addChannelMemoryEntries(target, ['Run tests']), + ).resolves.toMatchObject({ changed: true }); + + expect(lockObservation.options?.onCompromised).toBeTypeOf('function'); + await expect(readChannelMemory(target)).resolves.toBe('Run tests\n'); + }); + it('does not delete legacy bytes changed after canonical commit', async () => { const legacyPath = writeLegacy('Use staging\n'); fsFailure.legacyAppendAfterRename = { diff --git a/packages/core/src/memory/channel-memory.ts b/packages/core/src/memory/channel-memory.ts index acf7a1072c..963c79c32e 100644 --- a/packages/core/src/memory/channel-memory.ts +++ b/packages/core/src/memory/channel-memory.ts @@ -9,6 +9,7 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import lockfile from 'proper-lockfile'; import { Storage } from '../config/storage.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; import { createChannelMemoryEntry, MAX_CHANNEL_MEMORY_ENTRIES_PER_REQUEST, @@ -52,6 +53,8 @@ export const CHANNEL_MEMORY_FILE_NAME = 'CHANNEL.json'; export const LEGACY_CHANNEL_MEMORY_FILE_NAME = 'CHANNEL.md'; export const MAX_CHANNEL_MEMORY_BYTES = 1024 * 1024; +const debugLogger = createDebugLogger('CHANNEL_MEMORY'); + const pendingMutations = new Map>(); const LOCK_OPTIONS: lockfile.LockOptions = { realpath: false, @@ -63,6 +66,9 @@ const LOCK_OPTIONS: lockfile.LockOptions = { randomize: true, }, stale: 5000, + onCompromised: (err) => { + debugLogger.warn('channel memory lock compromised:', err); + }, }; interface LoadedChannelMemory { diff --git a/packages/core/src/skills/skill-curator.test.ts b/packages/core/src/skills/skill-curator.test.ts index ab8557809d..8a0c75a9c6 100644 --- a/packages/core/src/skills/skill-curator.test.ts +++ b/packages/core/src/skills/skill-curator.test.ts @@ -17,6 +17,7 @@ import { runAutoSkillCurator, setAutoSkillPinned, } from './skill-curator.js'; +import { mockCompromisedLock } from '../test-utils/mock-compromised-lock.js'; const DAY_MS = 24 * 60 * 60 * 1000; @@ -91,6 +92,23 @@ describe('auto-skill curator', () => { ).rejects.toMatchObject({ code: 'ENOENT' }); }); + it('registers a lock-compromised handler and completes when the curator lock is compromised', async () => { + const now = new Date('2026-07-27T00:00:00.000Z'); + const { lockSpy, getLockedFile, getOnCompromised } = mockCompromisedLock(); + + try { + await expect( + runAutoSkillCurator(projectRoot, { now }), + ).resolves.toMatchObject({ dryRun: false }); + expect(getOnCompromised()).toBeTypeOf('function'); + expect(getLockedFile()).toBe( + path.join(projectRoot, '.qwen', 'skill-curator.lock'), + ); + } finally { + lockSpy.mockRestore(); + } + }); + it('ignores auto-skill directories whose names carry control/ANSI bytes', async () => { const now = new Date('2026-07-27T00:00:00.000Z'); const old = new Date(now.getTime() - 100 * DAY_MS); diff --git a/packages/core/src/skills/skill-curator.ts b/packages/core/src/skills/skill-curator.ts index be48785e68..de8ce0dc94 100644 --- a/packages/core/src/skills/skill-curator.ts +++ b/packages/core/src/skills/skill-curator.ts @@ -10,6 +10,7 @@ import * as path from 'node:path'; import lockfile from 'proper-lockfile'; import { QWEN_DIR } from '../config/storage.js'; import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; import { parse as parseYaml } from '../utils/yaml-parser.js'; import { getArchivedSkillsRoot, @@ -23,6 +24,8 @@ export const AUTO_SKILL_CURATOR_INTERVAL_MS = 7 * 24 * 60 * 60 * 1000; export const AUTO_SKILL_STALE_AFTER_MS = 30 * 24 * 60 * 60 * 1000; export const AUTO_SKILL_ARCHIVE_AFTER_MS = 90 * 24 * 60 * 60 * 1000; +const debugLogger = createDebugLogger('SKILL_CURATOR'); + const AUTO_SKILL_PREFIX = 'auto-skill-'; const CURATOR_STATE_VERSION = 1; const CURATOR_STATE_FILE = 'skill-curator.json'; @@ -156,6 +159,9 @@ const LOCK_OPTIONS: lockfile.LockOptions = { randomize: true, }, stale: 10_000, + onCompromised: (err) => { + debugLogger.warn('skill curator lock compromised:', err); + }, }; function getCuratorPaths(projectRoot: string): CuratorPaths { diff --git a/packages/core/src/test-utils/mock-compromised-lock.ts b/packages/core/src/test-utils/mock-compromised-lock.ts new file mode 100644 index 0000000000..325bf71e49 --- /dev/null +++ b/packages/core/src/test-utils/mock-compromised-lock.ts @@ -0,0 +1,39 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import lockfile from 'proper-lockfile'; +import { vi } from 'vitest'; + +/** + * Simulates a proper-lockfile compromise on the next `lockfile.lock` call. + * After a real compromise, proper-lockfile marks the lock released before + * invoking `onCompromised`, so the later `release()` rejects with + * `ERELEASED`; the mocked release reproduces that exact state. + */ +export function mockCompromisedLock() { + let onCompromised: ((error: Error) => void) | undefined; + let lockedFile: string | undefined; + const lockSpy = vi + .spyOn(lockfile, 'lock') + .mockImplementationOnce(async (file, options) => { + lockedFile = file; + onCompromised = options?.onCompromised; + onCompromised?.( + Object.assign(new Error('lock lost'), { code: 'ECOMPROMISED' }), + ); + return () => + Promise.reject( + Object.assign(new Error('Lock is already released'), { + code: 'ERELEASED', + }), + ); + }); + return { + lockSpy, + getLockedFile: () => lockedFile, + getOnCompromised: () => onCompromised, + }; +}