From b92fde137b94f2eec32715a4a556cea5fe1705b6 Mon Sep 17 00:00:00 2001 From: qqqys Date: Sat, 8 Aug 2026 14:46:07 +0800 Subject: [PATCH 01/17] feat(core): add a live-session registry and `qwen sessions ps` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records each interactive session at `~/.qwen/sessions/.json` while it runs, so "which Qwen Code sessions are on this machine right now" is one readdir instead of a walk over every project's transcript directory. This is the discovery surface that cross-session messaging needs (QwenLM/qwen-code#8724), landed on its own because it is useful by itself and changes nothing about how a session behaves. Why not extend the existing runtime.json sidecar: it lives under `/chats/.runtime.json`, so enumeration costs a read per *historical* session and grows with transcript history; and it is deliberately never deleted, so its presence carries no liveness signal. The two now coexist — runtime.json stays the stable, kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start-time token read from /proc, so a recycled PID cannot resurrect a dead session's record. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out: the directory is chmod 0700 on every register (mkdir's mode is umask-masked and does nothing for an existing directory), records are 0600, and only `.json` is ever considered a record — a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. It sits next to `qwen sessions list`, which walks saved transcripts and answers the other question. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/sessions.test.ts | 14 +- packages/cli/src/commands/sessions.ts | 2 + packages/cli/src/commands/sessions/ps.test.ts | 132 ++++++ packages/cli/src/commands/sessions/ps.ts | 152 +++++++ packages/cli/src/ui/startInteractiveUI.tsx | 21 + packages/core/src/agents/team/teamHelpers.ts | 15 +- packages/core/src/config/config.ts | 14 + packages/core/src/index.ts | 2 + .../src/services/session-registry.test.ts | 357 +++++++++++++++++ .../core/src/services/session-registry.ts | 379 ++++++++++++++++++ .../core/src/utils/process-liveness.test.ts | 75 ++++ packages/core/src/utils/process-liveness.ts | 98 +++++ 12 files changed, 1245 insertions(+), 16 deletions(-) create mode 100644 packages/cli/src/commands/sessions/ps.test.ts create mode 100644 packages/cli/src/commands/sessions/ps.ts create mode 100644 packages/core/src/services/session-registry.test.ts create mode 100644 packages/core/src/services/session-registry.ts create mode 100644 packages/core/src/utils/process-liveness.test.ts create mode 100644 packages/core/src/utils/process-liveness.ts diff --git a/packages/cli/src/commands/sessions.test.ts b/packages/cli/src/commands/sessions.test.ts index 0c96002579b..9fd4bdb306d 100644 --- a/packages/cli/src/commands/sessions.test.ts +++ b/packages/cli/src/commands/sessions.test.ts @@ -6,6 +6,8 @@ import { describe, it, expect, vi } from 'vitest'; +// Subcommand modules are stubbed so this file tests wiring only — loading +// the real ones would pull the whole core barrel in behind them. vi.mock('./sessions/list.js', () => ({ listCommand: { command: 'list', @@ -13,6 +15,13 @@ vi.mock('./sessions/list.js', () => ({ }, })); +vi.mock('./sessions/ps.js', () => ({ + psCommand: { + command: 'ps', + describe: 'List Qwen Code sessions running right now', + }, +})); + import { sessionsCommand } from './sessions.js'; import { type Argv } from 'yargs'; import yargs from 'yargs'; @@ -42,7 +51,7 @@ describe('sessions command', () => { expect(options.key).toHaveProperty('help'); }); - it('should register list subcommand', () => { + it('should register list and ps subcommands', () => { const mockYargs = { command: vi.fn().mockReturnThis(), demandCommand: vi.fn().mockReturnThis(), @@ -55,12 +64,13 @@ describe('sessions command', () => { } builder(mockYargs as unknown as Argv); - expect(mockYargs.command).toHaveBeenCalledTimes(1); + expect(mockYargs.command).toHaveBeenCalledTimes(2); const commandCalls = mockYargs.command.mock.calls; const commandNames = commandCalls.map((call) => call[0].command); expect(commandNames).toContain('list'); + expect(commandNames).toContain('ps'); expect(mockYargs.demandCommand).toHaveBeenCalledWith( 1, diff --git a/packages/cli/src/commands/sessions.ts b/packages/cli/src/commands/sessions.ts index 513c0a40b3b..884dd3d97bb 100644 --- a/packages/cli/src/commands/sessions.ts +++ b/packages/cli/src/commands/sessions.ts @@ -6,6 +6,7 @@ import type { CommandModule, Argv } from 'yargs'; import { listCommand } from './sessions/list.js'; +import { psCommand } from './sessions/ps.js'; export const sessionsCommand: CommandModule = { command: 'sessions', @@ -13,6 +14,7 @@ export const sessionsCommand: CommandModule = { builder: (yargs: Argv) => yargs .command(listCommand) + .command(psCommand) .demandCommand(1, 'You need at least one command before continuing.') .version(false), // demandCommand(1) ensures a subcommand is always required; diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts new file mode 100644 index 00000000000..ddee9e1a0c2 --- /dev/null +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -0,0 +1,132 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import type { SessionRegistryRecord } from '@qwen-code/qwen-code-core'; + +const listLiveSessions = vi.fn(); + +vi.mock('@qwen-code/qwen-code-core', () => ({ + listLiveSessions: (...args: unknown[]) => listLiveSessions(...args), +})); + +const stdout: string[] = []; +const stderr: string[] = []; + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: (line: string) => stdout.push(line), + writeStderrLine: (line: string) => stderr.push(line), +})); + +const { psCommand, formatAge } = await import('./ps.js'); + +function record( + over: Partial = {}, +): SessionRegistryRecord { + return { + schemaVersion: 1, + pid: 4242, + procStart: '123', + sessionId: 'sess-1', + cwd: '/w/app', + name: 'app-ab', + kind: 'interactive', + startedAt: Date.now() - 90_000, + qwenVersion: '1.0.0', + peerProtocol: 1, + ...over, + }; +} + +async function run(argv: Record): Promise { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (psCommand.handler as any)(argv); +} + +beforeEach(() => { + stdout.length = 0; + stderr.length = 0; + listLiveSessions.mockReset(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('formatAge', () => { + it('scales the unit with the magnitude', () => { + expect(formatAge(5_000)).toBe('5s'); + expect(formatAge(90_000)).toBe('1m'); + expect(formatAge(3 * 3600_000)).toBe('3h'); + expect(formatAge(50 * 3600_000)).toBe('2d'); + }); + + it('clamps a record from the future to zero rather than showing a negative age', () => { + expect(formatAge(-10_000)).toBe('0s'); + }); +}); + +describe('qwen sessions ps', () => { + it('prints a table of live sessions', async () => { + listLiveSessions.mockResolvedValue([record()]); + await run({ json: false, all: false }); + + expect(stdout[0]).toMatch(/^NAME\s+PID\s+AGE\s+DIRECTORY$/); + expect(stdout[1]).toContain('app-ab'); + expect(stdout[1]).toContain('4242'); + expect(stdout[1]).toContain('/w/app'); + }); + + it('says so plainly when nothing else is running', async () => { + listLiveSessions.mockResolvedValue([]); + await run({ json: false, all: false }); + expect(stdout).toEqual(['No other Qwen Code sessions are running.']); + }); + + it('emits one JSON object per line with no header', async () => { + listLiveSessions.mockResolvedValue([record(), record({ pid: 7 })]); + await run({ json: true, all: false }); + + expect(stdout).toHaveLength(2); + expect(JSON.parse(stdout[0]).pid).toBe(4242); + expect(JSON.parse(stdout[1]).pid).toBe(7); + }); + + it('prints nothing on stdout for an empty JSON listing', async () => { + listLiveSessions.mockResolvedValue([]); + await run({ json: true, all: false }); + expect(stdout).toEqual([]); + }); + + it('excludes this process unless --all is passed', async () => { + listLiveSessions.mockResolvedValue([]); + + await run({ json: true, all: false }); + expect(listLiveSessions).toHaveBeenLastCalledWith({ includeSelf: false }); + + await run({ json: true, all: true }); + expect(listLiveSessions).toHaveBeenLastCalledWith({ includeSelf: true }); + }); + + it('neutralizes control sequences coming from another process record', async () => { + listLiveSessions.mockResolvedValue([ + record({ name: 'evil\r', cwd: '/w/a\nb' }), + ]); + await run({ json: false, all: false }); + + const row = stdout[1]; + expect(row).not.toContain(''); + expect(row).not.toContain('\r'); + expect(row).not.toContain('\n'); + }); + + it('truncates an over-long name instead of breaking the columns', async () => { + listLiveSessions.mockResolvedValue([record({ name: 'x'.repeat(80) })]); + await run({ json: false, all: false }); + expect(stdout[1]).toContain('...'); + expect(stdout[1]).toContain('4242'); + }); +}); diff --git a/packages/cli/src/commands/sessions/ps.ts b/packages/cli/src/commands/sessions/ps.ts new file mode 100644 index 00000000000..2d34ebc9b78 --- /dev/null +++ b/packages/cli/src/commands/sessions/ps.ts @@ -0,0 +1,152 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * `qwen sessions ps` — list the Qwen Code sessions running right now. + * + * The sibling `qwen sessions list` walks saved transcripts; this walks the + * live-process registry, so the two answer different questions: "what have + * I worked on" versus "what is running on this machine at this moment". + */ + +import type { CommandModule, Argv } from 'yargs'; +import { + listLiveSessions, + type SessionRegistryRecord, +} from '@qwen-code/qwen-code-core'; +import stringWidth from 'string-width'; +import { escapeAnsiCtrlCodes } from '../../ui/utils/textUtils.js'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; + +/** Fixed column widths for the human-readable table (exported for tests). */ +export const NAME_COL = 22; +export const PID_COL = 9; +export const AGE_COL = 10; + +interface PsArgs { + json?: boolean; + all?: boolean; +} + +/** + * Sanitize a value for terminal output. + * + * `cwd` and `name` originate from another process's on-disk record, so + * they are attacker-influenced in exactly the way a log line is: a record + * containing an ANSI sequence or a stray `\r` could otherwise repaint or + * misalign this table. Mirrors `sessions list`. + */ +function sanitize(value: string): string { + const stripped = value.replace(/[\r\n\t]/g, ''); + const escaped = escapeAnsiCtrlCodes(stripped); + // eslint-disable-next-line no-control-regex + return escaped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, ''); +} + +function padDisplay(str: string, width: number): string { + const currentWidth = stringWidth(str); + if (currentWidth >= width) return str; + return str + ' '.repeat(width - currentWidth); +} + +function truncate(str: string, maxLen: number): string { + if (stringWidth(str) <= maxLen) return str; + const suffix = maxLen > 3 ? '...' : ''; + const target = maxLen - stringWidth(suffix); + let result = ''; + let w = 0; + for (const char of str) { + const cw = stringWidth(char); + if (w + cw > target) break; + result += char; + w += cw; + } + return result + suffix; +} + +/** + * Render an age as a short, human-scannable string. + * + * A negative delta means the record's clock ran ahead of ours (a paused + * VM, a corrected clock). Showing "-3m" reads as a bug, so clamp to 0. + */ +export function formatAge(ms: number): string { + const seconds = Math.max(0, Math.floor(ms / 1000)); + if (seconds < 60) return `${seconds}s`; + const minutes = Math.floor(seconds / 60); + if (minutes < 60) return `${minutes}m`; + const hours = Math.floor(minutes / 60); + if (hours < 24) return `${hours}h`; + return `${Math.floor(hours / 24)}d`; +} + +function outputHuman(records: SessionRegistryRecord[], now: number): void { + writeStdoutLine( + padDisplay('NAME', NAME_COL) + + padDisplay('PID', PID_COL) + + padDisplay('AGE', AGE_COL) + + 'DIRECTORY', + ); + for (const record of records) { + writeStdoutLine( + padDisplay(truncate(sanitize(record.name), NAME_COL - 2), NAME_COL) + + padDisplay(String(record.pid), PID_COL) + + padDisplay(formatAge(now - record.startedAt), AGE_COL) + + sanitize(record.cwd), + ); + } +} + +async function handlePs(argv: PsArgs): Promise { + let records: SessionRegistryRecord[]; + try { + records = await listLiveSessions({ includeSelf: argv.all ?? false }); + } catch (err) { + writeStderrLine( + `Error: failed to read the session registry: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + process.exit(1); + return; + } + + const now = Date.now(); + + if (argv.json) { + for (const record of records) { + writeStdoutLine(JSON.stringify(record)); + } + return; + } + + if (records.length === 0) { + writeStdoutLine('No other Qwen Code sessions are running.'); + return; + } + + outputHuman(records, now); +} + +export const psCommand: CommandModule = { + command: 'ps', + describe: 'List Qwen Code sessions running right now', + builder: (yargs: Argv) => + yargs + .option('json', { + type: 'boolean', + describe: 'Output as JSON Lines', + default: false, + }) + .option('all', { + type: 'boolean', + describe: 'Include this process, if it is itself a registered session', + default: false, + }), + handler: async (argv) => { + await handlePs(argv); + }, +}; diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index d27b93374bd..9f1125f9ca5 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -10,7 +10,9 @@ import React from 'react'; import { createDebugLogger, isDebugLogFileEnabled, + registerSession, type Config, + unregisterSession, writeRuntimeStatus, } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../config/settings.js'; @@ -98,6 +100,25 @@ export async function startInteractiveUI( // ignored: best-effort, never block UI startup. } + // Announce this session in the machine-wide registry so sibling + // sessions can discover it (`qwen sessions ps`). Unlike the runtime.json + // sidecar above, this record is unlinked on exit — the registry's whole + // value is that presence means "running right now". + // + // registerSession swallows its own I/O errors, so a failure here is + // silent by design: discovery is a convenience, not a precondition for + // running Qwen Code. + if ( + await registerSession({ + sessionId: config.getSessionId(), + cwd: config.getTargetDir(), + kind: 'interactive', + qwenVersion: version, + }) + ) { + registerCleanup(() => unregisterSession()); + } + const restoreTerminalRedrawOptimizer = process.stdout.isTTY && !config.getScreenReader() ? installTerminalRedrawOptimizer(process.stdout) diff --git a/packages/core/src/agents/team/teamHelpers.ts b/packages/core/src/agents/team/teamHelpers.ts index 1b1db9d8587..94eddd343b7 100644 --- a/packages/core/src/agents/team/teamHelpers.ts +++ b/packages/core/src/agents/team/teamHelpers.ts @@ -18,6 +18,7 @@ import * as path from 'node:path'; import { Storage } from '../../config/storage.js'; import { isNodeError } from '../../utils/errors.js'; import { atomicWriteJSON } from '../../utils/atomicFileWrite.js'; +import { isPidAlive } from '../../utils/process-liveness.js'; import type { TeamFile, TeamMember } from './types.js'; import { TEAMS_DIR, @@ -294,20 +295,6 @@ export async function createTeamFile( }); } -/** - * Returns true when the given PID belongs to a live process. - * EPERM means the process exists but is owned by another user — - * treat as alive. - */ -function isPidAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (err) { - return isNodeError(err) && err.code === 'EPERM'; - } -} - /** * Reclaim a stale team so its name can be reused. * diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index ad186d82428..4141574ef77 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -210,6 +210,10 @@ import { clearRuntimeStatus, writeRuntimeStatus, } from '../utils/runtimeStatus.js'; +import { + deriveSessionName, + patchSessionRecord, +} from '../services/session-registry.js'; import { SessionService, type ResumedSessionData, @@ -3869,6 +3873,16 @@ export class Config { workDir, qwenVersion: cliVersion, }); + // Keep the machine-wide session registry in step for the same + // reason and under the same ownership rule: this PID's record + // would otherwise point discovery at the previous transcript. + // The record is keyed by PID, so a swap is a patch, not a + // delete-and-rewrite. + await patchSessionRecord({ + sessionId: newSessionId, + cwd: workDir, + name: deriveSessionName(workDir, newSessionId), + }); }); } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 53774b760f7..4d393a7a544 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -283,6 +283,7 @@ export * from './services/visionBridge/image-capability.js'; export * from './services/sessionRecap.js'; export * from './services/session-artifact-persistence.js'; export * from './services/session-reference-service.js'; +export * from './services/session-registry.js'; export * from './services/sessionService.js'; export * from './services/session-writer-lease.js'; export { @@ -571,6 +572,7 @@ export { preloadRuntimeFetchModule, redactProxyCredentials, } from './utils/runtimeFetchOptions.js'; +export * from './utils/process-liveness.js'; export * from './utils/runtimeStatus.js'; export * from './utils/schemaValidator.js'; export * from './utils/sessionIdContext.js'; diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts new file mode 100644 index 00000000000..5c76e0e3a33 --- /dev/null +++ b/packages/core/src/services/session-registry.test.ts @@ -0,0 +1,357 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { + deriveSessionName, + getSessionRecordPath, + getSessionRegistryDir, + listLiveSessions, + patchSessionRecord, + registerSession, + unregisterSession, + SESSION_REGISTRY_SCHEMA_VERSION, +} from './session-registry.js'; + +vi.mock('../config/storage.js', () => { + let mockDir = '/tmp/session-registry-test'; + return { + Storage: { + getGlobalQwenDir: () => mockDir, + }, + __setMockGlobalDir: (d: string) => { + mockDir = d; + }, + }; +}); + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const { __setMockGlobalDir } = (await import('../config/storage.js')) as any; + +let tmpDir: string; + +/** A PID that is essentially certain not to be running. */ +const DEAD_PID = 0x7ffffffe; + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'session-registry-')); + __setMockGlobalDir(tmpDir); +}); + +afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +async function writeRaw(fileName: string, body: unknown): Promise { + const dir = getSessionRegistryDir(); + await fs.mkdir(dir, { recursive: true }); + const filePath = path.join(dir, fileName); + await fs.writeFile( + filePath, + typeof body === 'string' ? body : JSON.stringify(body), + ); + return filePath; +} + +describe('deriveSessionName', () => { + it('combines the cwd basename with a session-derived suffix', () => { + const name = deriveSessionName('/home/u/projects/qwen-code', 'abc-123'); + expect(name).toMatch(/^qwen-code-[0-9a-f]{2}$/); + }); + + it('separates two sessions in the same directory', () => { + const a = deriveSessionName('/w/app', 'session-a'); + const b = deriveSessionName('/w/app', 'session-b'); + expect(a).not.toBe(b); + }); + + it('is stable for the same inputs', () => { + expect(deriveSessionName('/w/app', 's1')).toBe( + deriveSessionName('/w/app', 's1'), + ); + }); + + it('strips characters that would not survive a shell or a table', () => { + const name = deriveSessionName('/w/my project (v2)', 's1'); + expect(name).toMatch(/^[\w.-]+$/); + }); + + it('falls back to a placeholder when the basename is empty', () => { + expect(deriveSessionName('/', 's1')).toMatch(/^session-[0-9a-f]{2}$/); + }); +}); + +describe('registerSession', () => { + it('writes a record for this process and lists it back', async () => { + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + qwenVersion: '1.2.3', + }), + ).toBe(true); + + const live = await listLiveSessions({ includeSelf: true }); + expect(live).toHaveLength(1); + expect(live[0]).toMatchObject({ + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + qwenVersion: '1.2.3', + }); + expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); + }); + + it('creates the registry directory as 0700', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }); + + it('tightens a pre-existing loose registry directory', async () => { + await fs.mkdir(getSessionRegistryDir(), { recursive: true, mode: 0o755 }); + await fs.chmod(getSessionRegistryDir(), 0o755); + + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }); + + it('writes the record as 0600', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + const stat = await fs.stat(getSessionRecordPath()); + expect(stat.mode & 0o777).toBe(0o600); + }); + + it('reports failure instead of throwing when the home dir is unwritable', async () => { + __setMockGlobalDir(path.join(tmpDir, 'nope', '\0invalid')); + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(false); + }); +}); + +describe('patchSessionRecord', () => { + it('updates a field without dropping the others', async () => { + await registerSession({ + sessionId: 'old', + cwd: '/w/app', + kind: 'interactive', + qwenVersion: '1.2.3', + }); + + await patchSessionRecord({ sessionId: 'new', name: 'renamed' }); + + const [record] = await listLiveSessions({ includeSelf: true }); + expect(record).toMatchObject({ + sessionId: 'new', + name: 'renamed', + cwd: '/w/app', + qwenVersion: '1.2.3', + }); + }); + + it('does not create a record for a session that never registered', async () => { + await patchSessionRecord({ sessionId: 'new' }); + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + }); +}); + +describe('unregisterSession', () => { + it('removes the record', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + await unregisterSession(); + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + }); + + it('is a no-op when nothing was registered', async () => { + await expect(unregisterSession()).resolves.toBeUndefined(); + }); +}); + +describe('listLiveSessions', () => { + it('excludes the calling session by default', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + expect(await listLiveSessions()).toEqual([]); + }); + + it('returns an empty list when the registry does not exist', async () => { + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + }); + + it('sweeps a record whose process is gone', async () => { + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: null, + sessionId: 's-dead', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + qwenVersion: null, + peerProtocol: 1, + }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).rejects.toThrow(); + }); + + it('leaves a dead record in place when sweeping is disabled', async () => { + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + sessionId: 's-dead', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + expect(await listLiveSessions({ sweepStale: false })).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('treats a recycled PID as stale', async () => { + // Our own PID is alive, but the recorded start token belongs to a + // different process — so the record describes a session that is gone. + if (process.platform !== 'linux') return; + await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: '1', + sessionId: 's-recycled', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + // selfPid is set elsewhere so this record goes through the liveness + // path rather than the trust-our-own-record shortcut. + expect(await listLiveSessions({ selfPid: DEAD_PID })).toEqual([]); + }); + + it('ignores files that are not .json', async () => { + await writeRaw('2026-planning-notes.json', { hello: 'world' }); + await writeRaw('notes.txt', 'nope'); + await writeRaw('007.json', { + schemaVersion: 1, + pid: 7, + sessionId: 's', + cwd: '/w', + name: 'n', + kind: 'interactive', + startedAt: 1, + }); + + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + // Critically, none of them were deleted. + const remaining = await fs.readdir(getSessionRegistryDir()); + expect(remaining.sort()).toEqual([ + '007.json', + '2026-planning-notes.json', + 'notes.txt', + ]); + }); + + it('skips a record whose pid disagrees with its filename', async () => { + await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid + 1, + sessionId: 's', + cwd: '/w', + name: 'n', + kind: 'interactive', + startedAt: 1, + }); + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + await expect( + fs.stat(path.join(getSessionRegistryDir(), `${process.pid}.json`)), + ).resolves.toBeDefined(); + }); + + it('skips malformed and future-schema records without deleting them', async () => { + await writeRaw('11.json', 'not json at all'); + await writeRaw('12.json', { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + pid: 12, + sessionId: 's', + cwd: '/w', + name: 'n', + kind: 'interactive', + startedAt: 1, + }); + await writeRaw('13.json', { + schemaVersion: 1, + pid: 13, + sessionId: 's', + cwd: '/w', + name: 'n', + kind: 'not-a-kind', + startedAt: 1, + }); + + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + expect((await fs.readdir(getSessionRegistryDir())).sort()).toEqual([ + '11.json', + '12.json', + '13.json', + ]); + }); + + it('sorts newest first', async () => { + await registerSession({ + sessionId: 's-self', + cwd: '/w/app', + kind: 'interactive', + }); + await patchSessionRecord({ startedAt: 1000 }); + await writeRaw(`${process.ppid}.json`, { + schemaVersion: 1, + pid: process.ppid, + sessionId: 's-parent', + cwd: '/w/other', + name: 'other-bb', + kind: 'interactive', + startedAt: 2000, + }); + + const live = await listLiveSessions({ includeSelf: true }); + expect(live.map((r) => r.sessionId)).toEqual(['s-parent', 's-self']); + }); +}); diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts new file mode 100644 index 00000000000..36781e78098 --- /dev/null +++ b/packages/core/src/services/session-registry.ts @@ -0,0 +1,379 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * A machine-wide index of the Qwen Code sessions that are running right + * now. + * + * Each top-level session writes `~/.qwen/sessions/.json` at startup + * and unlinks it on exit. The directory is flat and keyed by PID so that + * "who else is running on this box" is one `readdir` plus a handful of + * small reads. + * + * ## Why this is not `runtime.json` + * + * {@link ../utils/runtimeStatus.ts} already writes a per-session sidecar, + * but it answers a different question and cannot serve this one: + * + * - It lives at `/chats/.runtime.json`, so finding + * every live session means walking every project directory and reading + * a file per *historical* session, not per live one. That cost grows + * with transcript history and would be paid on every lookup. + * - It is deliberately never deleted — not on clean quit, not on crash — + * so presence carries no liveness signal at all. + * + * The two coexist: `runtime.json` stays the stable, kimi-compatible + * "which session is PID X serving" sidecar for external observers, and + * this registry is the discovery index for the CLI's own features. + * + * ## Staleness + * + * A record is live when its PID is running *and* the recorded process + * start token still matches (see `isSameProcess`) — a recycled PID must + * not resurrect a dead session. Records that fail that check are swept + * during enumeration; anything we cannot positively prove dead is left + * alone. + */ + +import { createHash } from 'node:crypto'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import { Storage } from '../config/storage.js'; +import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; +import { + isSameProcess, + readProcStartToken, +} from '../utils/process-liveness.js'; + +const debugLogger = createDebugLogger('SESSION_REGISTRY'); + +export const SESSION_REGISTRY_SCHEMA_VERSION = 1; + +/** + * Version of the peer messaging contract this session speaks. Recorded so + * a future sender can skip sessions that predate a protocol change + * instead of writing frames they cannot parse. Bump on a breaking change + * to the on-the-wire message shape. + */ +export const PEER_PROTOCOL_VERSION = 1; + +/** Directory mode: the registry names live sessions and their work dirs. */ +const REGISTRY_DIR_MODE = 0o700; +const REGISTRY_FILE_MODE = 0o600; + +/** Refuse to parse anything larger; a record is a few hundred bytes. */ +const MAX_RECORD_BYTES = 64 * 1024; + +/** + * Only `.json` is a candidate record. + * + * This is deliberately strict. A lenient `parseInt` prefix match would + * read `2026-planning-notes.json` as PID 2026, fail its liveness check, + * and delete a file this code never wrote. + */ +const RECORD_FILENAME = /^\d+\.json$/; + +export type SessionKind = 'interactive' | 'headless'; + +/** One live session, as recorded on disk. */ +export interface SessionRegistryRecord { + schemaVersion: number; + pid: number; + /** Start-time token guarding against PID reuse; null where unavailable. */ + procStart: string | null; + sessionId: string; + cwd: string; + /** Short human-facing label, unique-ish per session. */ + name: string; + kind: SessionKind; + /** Epoch milliseconds. */ + startedAt: number; + qwenVersion: string | null; + peerProtocol: number; +} + +export interface RegisterSessionFields { + sessionId: string; + cwd: string; + kind: SessionKind; + qwenVersion?: string | null; + /** Defaults to `process.pid`. Tests pass an explicit value. */ + pid?: number; + /** Overrides the derived name. */ + name?: string; +} + +export function getSessionRegistryDir(): string { + return path.join(Storage.getGlobalQwenDir(), 'sessions'); +} + +export function getSessionRecordPath(pid: number = process.pid): string { + return path.join(getSessionRegistryDir(), `${pid}.json`); +} + +/** + * A short, stable, human-readable label: the working directory's basename + * plus two hex characters derived from the session id. + * + * The suffix exists because two sessions in the same directory is the + * common case, not the exception — bare `qwen-code` would collide + * immediately. Two hex characters keep it typeable while making a + * same-directory collision unlikely rather than certain; callers that + * need a guaranteed-unique handle should use the session id. + */ +export function deriveSessionName(cwd: string, sessionId: string): string { + const base = path + .basename(cwd) + .replace(/[^\w.-]+/g, '-') + .slice(0, 32); + const suffix = createHash('sha256') + .update(sessionId) + .digest('hex') + .slice(0, 2); + return `${base || 'session'}-${suffix}`; +} + +/** + * Write this process's record. Best-effort: a read-only or full home + * directory must not stop a session from starting, so failures are logged + * and reported, never thrown. + * + * Returns true when the record was written. + */ +export async function registerSession( + fields: RegisterSessionFields, +): Promise { + const pid = fields.pid ?? process.pid; + const record: SessionRegistryRecord = { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid, + procStart: readProcStartToken(pid), + sessionId: fields.sessionId, + cwd: fields.cwd, + name: fields.name ?? deriveSessionName(fields.cwd, fields.sessionId), + kind: fields.kind, + startedAt: Date.now(), + qwenVersion: fields.qwenVersion ?? null, + peerProtocol: PEER_PROTOCOL_VERSION, + }; + + try { + const dir = getSessionRegistryDir(); + await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); + // mkdir's mode is masked by the umask, and does nothing at all when + // the directory already exists — chmod is what actually guarantees + // 0700 on an upgrade from a build that created it more loosely. + await fs.chmod(dir, REGISTRY_DIR_MODE); + await atomicWriteJSON(getSessionRecordPath(pid), record, { + mode: REGISTRY_FILE_MODE, + forceMode: true, + }); + return true; + } catch (error) { + debugLogger.debug(`registerSession failed: ${describe(error)}`); + return false; + } +} + +/** + * Merge `patch` into this process's record. + * + * Used when a field changes mid-session — `/clear`, `/resume` and friends + * swap the session id under a stable PID, and a record still advertising + * the old id points readers at the wrong transcript. + * + * No-ops when the record is missing: a session that failed to register + * should not be resurrected by a later patch, because the resurrected + * record would be missing whatever else registration would have set. + */ +export async function patchSessionRecord( + patch: Partial>, + pid: number = process.pid, +): Promise { + const filePath = getSessionRecordPath(pid); + try { + const existing = await readRecord(filePath); + if (existing === null) return; + await atomicWriteJSON( + filePath, + { ...existing, ...patch }, + { mode: REGISTRY_FILE_MODE, forceMode: true }, + ); + } catch (error) { + debugLogger.debug(`patchSessionRecord failed: ${describe(error)}`); + } +} + +/** Remove this process's record. Safe to call when none was written. */ +export async function unregisterSession( + pid: number = process.pid, +): Promise { + try { + await fs.unlink(getSessionRecordPath(pid)); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; + debugLogger.debug(`unregisterSession failed: ${describe(error)}`); + } +} + +export interface ListLiveSessionsOptions { + /** Include the calling process's own record. Defaults to false. */ + includeSelf?: boolean; + /** Overrides `process.pid` when deciding what "self" means. */ + selfPid?: number; + /** + * Delete records proven to belong to a dead process. Defaults to true; + * read-only callers can turn it off. + */ + sweepStale?: boolean; +} + +/** + * Enumerate live sessions, newest first, sweeping records whose process + * is provably gone. + * + * Returns an empty list rather than throwing when the registry directory + * is missing or unreadable — "no peers" and "cannot look" are the same + * outcome for every caller, and this sits on interactive paths. + */ +export async function listLiveSessions( + options: ListLiveSessionsOptions = {}, +): Promise { + const { + includeSelf = false, + selfPid = process.pid, + sweepStale = true, + } = options; + + const dir = getSessionRegistryDir(); + let entries: string[]; + try { + entries = await fs.readdir(dir); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== 'ENOENT') { + debugLogger.debug(`listLiveSessions readdir failed: ${describe(error)}`); + } + return []; + } + + const live: SessionRegistryRecord[] = []; + await Promise.all( + entries + .filter((name) => RECORD_FILENAME.test(name)) + .map(async (name) => { + const filePath = path.join(dir, name); + const record = await readRecord(filePath); + if (record === null) return; + + // A record whose filename disagrees with its contents was not + // written by this code (or was renamed by hand). Skip it, and + // never sweep it — we cannot reason about which PID it describes. + if (`${record.pid}.json` !== name) return; + + if (record.pid === selfPid) { + // Trust our own record without probing: `isSameProcess` on self + // is always true, and the token read is pure overhead. + if (includeSelf) live.push(record); + return; + } + + if (isSameProcess(record.pid, record.procStart)) { + live.push(record); + return; + } + + if (sweepStale) { + try { + await fs.unlink(filePath); + } catch { + // Raced with another session's sweep, or not ours to delete. + } + } + }), + ); + + return live.sort((a, b) => b.startedAt - a.startedAt); +} + +/** Read and validate one record. Returns null for anything unusable. */ +async function readRecord( + filePath: string, +): Promise { + let raw: string; + try { + const stat = await fs.stat(filePath); + if (!stat.isFile() || stat.size > MAX_RECORD_BYTES) return null; + raw = await fs.readFile(filePath, 'utf8'); + } catch { + return null; + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + return null; + } + const value = parsed as Record; + + // Forward compatibility runs one way: a newer schema may add fields, so + // an unknown *higher* version is skipped rather than guessed at. + const schemaVersion = value['schemaVersion']; + if ( + typeof schemaVersion !== 'number' || + schemaVersion > SESSION_REGISTRY_SCHEMA_VERSION + ) { + return null; + } + + const pid = value['pid']; + const sessionId = value['sessionId']; + const cwd = value['cwd']; + const name = value['name']; + const kind = value['kind']; + const startedAt = value['startedAt']; + if ( + typeof pid !== 'number' || + !Number.isInteger(pid) || + pid <= 0 || + typeof sessionId !== 'string' || + typeof cwd !== 'string' || + typeof name !== 'string' || + (kind !== 'interactive' && kind !== 'headless') || + typeof startedAt !== 'number' || + !Number.isFinite(startedAt) + ) { + return null; + } + + const procStart = value['procStart']; + const qwenVersion = value['qwenVersion']; + const peerProtocol = value['peerProtocol']; + + return { + schemaVersion, + pid, + procStart: typeof procStart === 'string' ? procStart : null, + sessionId, + cwd, + name, + kind, + startedAt, + qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, + peerProtocol: typeof peerProtocol === 'number' ? peerProtocol : 0, + }; +} + +function describe(error: unknown): string { + return error instanceof Error + ? `${error.name}: ${error.message}` + : String(error); +} diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts new file mode 100644 index 00000000000..89eb0f63a9e --- /dev/null +++ b/packages/core/src/utils/process-liveness.test.ts @@ -0,0 +1,75 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { + isPidAlive, + isSameProcess, + readProcStartToken, +} from './process-liveness.js'; + +/** A PID that is essentially certain not to be running. */ +const DEAD_PID = 0x7ffffffe; + +describe('isPidAlive', () => { + it('reports the current process as alive', () => { + expect(isPidAlive(process.pid)).toBe(true); + }); + + it('reports an unused pid as dead', () => { + expect(isPidAlive(DEAD_PID)).toBe(false); + }); + + it('rejects nonsense pids without throwing', () => { + expect(isPidAlive(0)).toBe(false); + expect(isPidAlive(-1)).toBe(false); + expect(isPidAlive(1.5)).toBe(false); + expect(isPidAlive(NaN)).toBe(false); + }); +}); + +describe('readProcStartToken', () => { + it('returns a numeric token for a live process on Linux', () => { + const token = readProcStartToken(process.pid); + if (process.platform !== 'linux') { + expect(token).toBeNull(); + return; + } + expect(token).toMatch(/^\d+$/); + }); + + it('is stable across calls', () => { + expect(readProcStartToken(process.pid)).toBe( + readProcStartToken(process.pid), + ); + }); + + it('returns null for a dead pid', () => { + expect(readProcStartToken(DEAD_PID)).toBeNull(); + }); +}); + +describe('isSameProcess', () => { + it('is false for a dead pid regardless of token', () => { + expect(isSameProcess(DEAD_PID, null)).toBe(false); + expect(isSameProcess(DEAD_PID, '123')).toBe(false); + }); + + it('accepts a live pid recorded without a token', () => { + expect(isSameProcess(process.pid, null)).toBe(true); + expect(isSameProcess(process.pid, undefined)).toBe(true); + }); + + it('accepts a live pid whose token still matches', () => { + const token = readProcStartToken(process.pid); + expect(isSameProcess(process.pid, token)).toBe(true); + }); + + it('rejects a live pid whose token has changed', () => { + if (process.platform !== 'linux') return; + expect(isSameProcess(process.pid, 'definitely-not-the-token')).toBe(false); + }); +}); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts new file mode 100644 index 00000000000..6119b955de9 --- /dev/null +++ b/packages/core/src/utils/process-liveness.ts @@ -0,0 +1,98 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Process liveness helpers shared by anything that records a PID on disk + * and later has to decide whether that record still describes a running + * process. + * + * A bare PID is not enough on its own: PIDs are recycled, so a record + * written by a process that has since exited can be "confirmed alive" by + * an unrelated process that happens to inherit the number. Pair + * {@link isPidAlive} with {@link readProcStartToken} to close that gap + * wherever the platform provides a start-time token. + */ + +import * as fs from 'node:fs'; +import { isNodeError } from './errors.js'; + +/** + * True when the given PID belongs to a live process. + * + * `EPERM` means the process exists but is owned by another user — that is + * still alive, and reporting it as dead would let one user's session sweep + * another's record out of a shared registry directory. + */ +export function isPidAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (err) { + return isNodeError(err) && err.code === 'EPERM'; + } +} + +/** + * An opaque token that changes when a PID is recycled, or `null` when the + * platform does not expose one cheaply. + * + * Backed by the `starttime` field of `/proc//stat` on Linux — the + * process start time in clock ticks since boot. Two processes sharing a + * PID across a recycle will not share this value. + * + * Returns `null` on every non-Linux platform rather than shelling out to + * `ps`: callers must already tolerate a missing token (the registry falls + * back to a plain liveness check), and a subprocess per record would make + * enumeration far more expensive than the problem it solves. + */ +export function readProcStartToken(pid: number): string | null { + if (process.platform !== 'linux') return null; + if (!Number.isInteger(pid) || pid <= 0) return null; + + let raw: string; + try { + raw = fs.readFileSync(`/proc/${pid}/stat`, 'utf8'); + } catch { + return null; + } + + // Field 2 (`comm`) is parenthesized and may itself contain spaces and + // ')' — a process named "my ) proc" is legal. Splitting the whole line + // on whitespace therefore misaligns every later field, so anchor on the + // LAST ')' and count from there. + const commEnd = raw.lastIndexOf(')'); + if (commEnd === -1) return null; + + // After " () " the next token is field 3 (state), so field N + // lands at index N - 3. `starttime` is field 22. + const fields = raw + .slice(commEnd + 1) + .trim() + .split(/\s+/); + const startTime = fields[19]; + return startTime !== undefined && /^\d+$/.test(startTime) ? startTime : null; +} + +/** + * True when `pid` is alive AND is the same process that recorded + * `procStart`. + * + * A `null` recorded token (written on a platform without one) or a `null` + * current token (the process died between the two reads, or `/proc` is not + * readable) degrades to a plain liveness check rather than declaring the + * record stale — deleting a live session's record is the worse failure. + */ +export function isSameProcess( + pid: number, + procStart: string | null | undefined, +): boolean { + if (!isPidAlive(pid)) return false; + if (procStart == null) return true; + const current = readProcStartToken(pid); + if (current === null) return true; + return current === procStart; +} From a9e9cee390b08c5eb2104abe25ce06ad28ade88f Mon Sep 17 00:00:00 2001 From: qqqys Date: Sat, 8 Aug 2026 15:36:52 +0800 Subject: [PATCH 02/17] test(cli): restore gemini.test.tsx mocks for the session registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit startInteractiveUI now calls registerSession({ sessionId: config.getSessionId(), cwd: config.getTargetDir(), ... }), but the mock Config objects in gemini.test.tsx predate it and expose neither getter, so all 32 tests that reach startInteractiveUI died with "config.getTargetDir is not a function". Add the two getters to the mocks that feed those tests, and stub registerSession/unregisterSession so the suite does not write a real record into the global Qwen dir on every run — the registry has its own coverage in packages/core/src/services/session-registry.test.ts. Tests: packages/cli src/gemini.test.tsx 69 passed (was 32 failed | 37 passed); npm run build, npm run typecheck, eslint and prettier on the changed file all clean. --- packages/cli/src/gemini.test.tsx | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 421cfc8337b..456b39666a4 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -89,6 +89,19 @@ class MockProcessExitError extends Error { } // Mock dependencies +// startInteractiveUI announces the session in the machine-wide registry, which +// writes under the real global Qwen dir. Stub it so the suite leaves no record +// behind; the registry has its own tests in core. +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + registerSession: vi.fn().mockResolvedValue(true), + unregisterSession: vi.fn().mockResolvedValue(undefined), + }; +}); + vi.mock('./config/settings.js', async (importOriginal) => { const actual = await importOriginal(); return { @@ -1420,6 +1433,7 @@ describe('gemini.tsx main function kitty protocol', () => { getModelsConfig: () => ({ getCurrentAuthType: () => null }), getUsageStatisticsEnabled: () => true, getSessionId: () => 'test-session-id', + getTargetDir: () => '/test/dir', isTelemetryInitializationDeferred: () => true, } as unknown as Config); vi.mocked(loadSettings).mockReturnValue({ @@ -1545,6 +1559,7 @@ describe('gemini.tsx main function kitty protocol', () => { getModelsConfig: () => ({ getCurrentAuthType: () => null }), getUsageStatisticsEnabled: () => true, getSessionId: () => 'test-session-id', + getTargetDir: () => '/test/dir', isTelemetryInitializationDeferred: () => false, } as unknown as Config); vi.mocked(loadSettings).mockReturnValue({ @@ -1669,6 +1684,7 @@ describe('gemini.tsx main function kitty protocol', () => { getModelsConfig: () => ({ getCurrentAuthType: () => null }), getUsageStatisticsEnabled: () => true, getSessionId: () => 'test-session-id', + getTargetDir: () => '/test/dir', isTelemetryInitializationDeferred: () => true, } as unknown as Config); vi.mocked(loadSettings).mockReturnValue({ @@ -1920,6 +1936,7 @@ describe('gemini.tsx main function kitty protocol', () => { getProxy: () => undefined, getUsageStatisticsEnabled: () => true, getSessionId: () => 'test-session-id', + getTargetDir: () => '/test/dir', isTelemetryInitializationDeferred: () => true, } as unknown as Config); vi.mocked( @@ -2312,6 +2329,8 @@ describe('startInteractiveUI', () => { // Mock dependencies const mockConfig = { getProjectRoot: () => '/root', + getSessionId: () => 'test-session-id', + getTargetDir: () => '/root', getScreenReader: () => false, isTelemetryInitializationDeferred: () => true, getChatRecordingService: () => undefined, From 0f0af418382408839f4ba4ad4954c2bc26eda70a Mon Sep 17 00:00:00 2001 From: qqqys Date: Sat, 8 Aug 2026 21:32:03 +0800 Subject: [PATCH 03/17] fix(core): keep the session-registry swap independent of the sidecar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 4888791383 on #8728 found the /clear-and-/resume swap wired so that the machine-wide registry patch could be skipped in two ways that nothing detected. Both halves of the swap ran in one queued closure whose rejection is swallowed, so a sidecar write that failed mid-swap (unwritable chats/, full disk) took patchSessionRecord down with it and the record kept advertising the pre-swap session id. The two sidecar awaits now sit in their own try/catch, so the registry patch always runs. The patch was also gated on runtimeStatusEnabled, which registerSession never consults — a startup where the sidecar write failed but registration succeeded left the record stranded on the old session id for the life of the process. Unlike clearRuntimeStatus, the patch is keyed by this PID and no-ops without a record, so it cannot trample a sibling and needs no ownership gate; the gate now covers only the sidecar half it was written for. Tests for the gaps the review probed, each flip-verified against the mutant that motivated it: - runtimeStatus.config.test.ts: the registry half of the swap had no coverage at all — deleting the call kept the suite green. Four cases now pin it, including the two failure modes fixed above. - session-registry.test.ts: enumerate as a foreign PID so the record goes through isSameProcess rather than the self-pid shortcut, and assert the written procStart. `procStart: null` previously stayed green while silently voiding the recycled-PID guard. - process-liveness.test.ts: cover the degrade branch where the current token cannot be read. `return true` -> `return false` there sweeps live records on every platform without /proc, and passed 33/33. - ps.test.ts: assert the exact truncated cell for a full-width name; `contains '...'` survived a loop guard replaced with `if (true)`. Also cover the registry-read failure branch (stderr text + exit 1). - ps.test.ts carried raw ESC bytes, so the ANSI assertion rendered as `not.toContain('')` and a byte-stripping tool would have voided the test silently. Now `\x1b`, matching repo convention. Also drops the "Tests pass an explicit value" claim from the `pid` option's doc comment: no caller in production or tests passes one. Verified: npm run build, npm run typecheck, eslint and prettier on the touched files, plus core config/runtimeStatus/session-registry/ process-liveness (562 passed) and cli commands/sessions (39 passed). Co-Authored-By: Claude Opus 5 --- packages/cli/src/commands/sessions/ps.test.ts | 38 +++++- packages/core/src/config/config.ts | 40 ++++-- .../src/services/session-registry.test.ts | 27 ++++ .../core/src/services/session-registry.ts | 2 +- .../core/src/utils/process-liveness.test.ts | 41 +++++- .../src/utils/runtimeStatus.config.test.ts | 119 +++++++++++++++++- 6 files changed, 249 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index ddee9e1a0c2..d32bb0439fb 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -21,7 +21,7 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ writeStderrLine: (line: string) => stderr.push(line), })); -const { psCommand, formatAge } = await import('./ps.js'); +const { psCommand, formatAge, NAME_COL } = await import('./ps.js'); function record( over: Partial = {}, @@ -113,12 +113,12 @@ describe('qwen sessions ps', () => { it('neutralizes control sequences coming from another process record', async () => { listLiveSessions.mockResolvedValue([ - record({ name: 'evil\r', cwd: '/w/a\nb' }), + record({ name: 'ev\x1b[31mil\r', cwd: '/w/a\nb' }), ]); await run({ json: false, all: false }); const row = stdout[1]; - expect(row).not.toContain(''); + expect(row).not.toContain('\x1b'); expect(row).not.toContain('\r'); expect(row).not.toContain('\n'); }); @@ -129,4 +129,36 @@ describe('qwen sessions ps', () => { expect(stdout[1]).toContain('...'); expect(stdout[1]).toContain('4242'); }); + + it('cuts a multi-width name on a character boundary, not a column one', async () => { + // 15 full-width characters is 30 display columns against a 20-column + // budget. Subtracting the three columns "..." costs leaves 17: the + // eighth character ends at column 16, and a ninth would straddle the + // limit, so the cut lands at eight characters for a 19-column cell. + // Asserting the cell exactly is what pins the accumulation loop — + // "contains ..." survives a loop that copies nothing at all. + listLiveSessions.mockResolvedValue([record({ name: '中'.repeat(15) })]); + await run({ json: false, all: false }); + + const cell = '中'.repeat(8) + '...'; + expect(stdout[1]).toBe( + cell + ' '.repeat(NAME_COL - 19) + '4242 ' + '1m ' + '/w/app', + ); + }); + + it('reports a registry read failure on stderr and exits non-zero', async () => { + listLiveSessions.mockRejectedValue(new Error('registry on fire')); + const exit = vi + .spyOn(process, 'exit') + .mockImplementation((() => undefined) as never); + + await run({ json: false, all: false }); + + expect(stderr).toEqual([ + 'Error: failed to read the session registry: registry on fire', + ]); + expect(exit).toHaveBeenCalledWith(1); + // Nothing is printed once the listing failed — not even the header. + expect(stdout).toEqual([]); + }); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 4141574ef77..ca0108d8d7d 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -3854,30 +3854,46 @@ export class Config { // so handling the swap centrally covers every same-PID session // transition. Best-effort: must never block /clear or /resume. // - // Only refresh when THIS process established its own sidecar at + // Only refresh the sidecar when THIS process established its own at // startup (interactive UI). A non-interactive `/clear` (e.g. // qwen --prompt-interactive) must not delete a sibling shell's // sidecar that happens to share the outgoing session id // mirrors the kimi-cli "write only when a session is // established for this process" rule. - if (this.runtimeStatusEnabled && previousSessionId !== this.sessionId) { + if (previousSessionId !== this.sessionId) { + const refreshSidecar = this.runtimeStatusEnabled; const oldPath = this.storage.getRuntimeStatusPath(previousSessionId); const newPath = this.storage.getRuntimeStatusPath(this.sessionId); const cliVersion = this.cliVersion ?? null; const workDir = this.targetDir; const newSessionId = this.sessionId; this.queueRuntimeStatusWrite(async () => { - await clearRuntimeStatus(oldPath); - await writeRuntimeStatus(newPath, { - sessionId: newSessionId, - workDir, - qwenVersion: cliVersion, - }); + if (refreshSidecar) { + try { + await clearRuntimeStatus(oldPath); + await writeRuntimeStatus(newPath, { + sessionId: newSessionId, + workDir, + qwenVersion: cliVersion, + }); + } catch { + // A sidecar that could not be rewritten (unwritable chats + // dir, full disk) must not take the registry patch below + // down with it: the two records fail independently, and a + // registry entry left pointing at the previous transcript + // is the more visible of the two failures. + } + } // Keep the machine-wide session registry in step for the same - // reason and under the same ownership rule: this PID's record - // would otherwise point discovery at the previous transcript. - // The record is keyed by PID, so a swap is a patch, not a - // delete-and-rewrite. + // reason: this PID's record would otherwise point discovery at + // the previous transcript. The record is keyed by PID, so a + // swap is a patch, not a delete-and-rewrite — and it no-ops + // when this process never registered. That makes the sidecar + // ownership rule above irrelevant here: unlike + // clearRuntimeStatus, this cannot trample a sibling's record, + // so gating it on runtimeStatusEnabled would only let the two + // ownership signals drift apart when the startup sidecar write + // failed but registration succeeded. await patchSessionRecord({ sessionId: newSessionId, cwd: workDir, diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 5c76e0e3a33..991344d12be 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -18,6 +18,7 @@ import { unregisterSession, SESSION_REGISTRY_SCHEMA_VERSION, } from './session-registry.js'; +import { readProcStartToken } from '../utils/process-liveness.js'; vi.mock('../config/storage.js', () => { let mockDir = '/tmp/session-registry-test'; @@ -111,6 +112,32 @@ describe('registerSession', () => { expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); }); + it('records a start-time token so a recycled pid cannot resurrect it', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + + // Enumerate as a *different* process: the self-pid shortcut never + // calls isSameProcess, so it is the only path on which the recorded + // token is read at all. Without this, a regression writing + // `procStart: null` stays green here and, in production, degrades + // every sibling's check to bare liveness — the exact recycled-pid + // hole the token exists to close. + const live = await listLiveSessions({ + selfPid: DEAD_PID, + sweepStale: false, + }); + expect(live).toHaveLength(1); + if (process.platform === 'linux') { + expect(live[0].procStart).toBe(readProcStartToken(process.pid)); + expect(live[0].procStart).toMatch(/^\d+$/); + } else { + expect(live[0].procStart).toBeNull(); + } + }); + it('creates the registry directory as 0700', async () => { await registerSession({ sessionId: 's1', diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 36781e78098..06bc7f2de7d 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -101,7 +101,7 @@ export interface RegisterSessionFields { cwd: string; kind: SessionKind; qwenVersion?: string | null; - /** Defaults to `process.pid`. Tests pass an explicit value. */ + /** Defaults to `process.pid`. */ pid?: number; /** Overrides the derived name. */ name?: string; diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 89eb0f63a9e..62aad1fd690 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -4,13 +4,36 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { isPidAlive, isSameProcess, readProcStartToken, } from './process-liveness.js'; +/** + * Lets a single test make the `/proc//stat` read fail. Everything + * else passes straight through to the real `node:fs`. + */ +const procReadFails = vi.hoisted(() => ({ value: false })); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: actual, + readFileSync: ((...args: unknown[]) => { + if (procReadFails.value) { + throw Object.assign(new Error('EACCES: /proc unreadable'), { + code: 'EACCES', + }); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (actual.readFileSync as any)(...args); + }) as typeof actual.readFileSync, + }; +}); + /** A PID that is essentially certain not to be running. */ const DEAD_PID = 0x7ffffffe; @@ -72,4 +95,20 @@ describe('isSameProcess', () => { if (process.platform !== 'linux') return; expect(isSameProcess(process.pid, 'definitely-not-the-token')).toBe(false); }); + + it('keeps a live pid when the current token cannot be read', () => { + // The degrade branch: /proc went unreadable between the two reads, or + // the platform never exposed a token at all. Liveness alone has to + // win here — inverting this sweeps a live session's record on every + // platform without /proc, which is the failure the module exists to + // avoid. Nothing else covers it: the recorded-token tests are + // Linux-guarded, and on Linux a live pid always reads back a token. + procReadFails.value = true; + try { + expect(readProcStartToken(process.pid)).toBeNull(); + expect(isSameProcess(process.pid, '123')).toBe(true); + } finally { + procReadFails.value = false; + } + }); }); diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index 9865844aad9..db135bfb42d 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -14,11 +14,52 @@ import { mkdtemp, readdir, rm } from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { SessionRegistryRecord } from '../services/session-registry.js'; import { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; import { readRuntimeStatus, writeRuntimeStatus } from './runtimeStatus.js'; +/** Lets one test make the sidecar half of the swap fail. */ +const failSidecarWrite = vi.hoisted(() => ({ value: false })); + +/** + * Records what the swap asked the machine-wide registry to do. Stubbed + * rather than exercised for real: `patchSessionRecord` writes under the + * developer's actual `~/.qwen/sessions`, and it no-ops when this PID has + * no record — so a real call would both pollute the home directory and + * silently pass no matter what the swap did. + */ +const patchCalls = vi.hoisted( + () => [] as Array>, +); + +vi.mock('./runtimeStatus.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeRuntimeStatus: async ( + ...args: Parameters + ) => { + if (failSidecarWrite.value) { + throw new Error('chats dir is read-only'); + } + return actual.writeRuntimeStatus(...args); + }, + }; +}); + +vi.mock('../services/session-registry.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + patchSessionRecord: async (patch: Partial) => { + patchCalls.push(patch); + }, + }; +}); + let tmpDir: string; let runtimeDir: string; let prevRuntimeEnv: string | undefined; @@ -28,6 +69,8 @@ beforeEach(async () => { runtimeDir = path.join(tmpDir, 'runtime'); prevRuntimeEnv = process.env['QWEN_RUNTIME_DIR']; process.env['QWEN_RUNTIME_DIR'] = runtimeDir; + patchCalls.length = 0; + failSidecarWrite.value = false; }); afterEach(async () => { @@ -150,6 +193,80 @@ describe('Config.startNewSession runtime.json swap', () => { }); }); +describe('Config.startNewSession session-registry swap', () => { + const sessionA = 'aaaaaaaa-1111-2222-3333-aaaaaaaaaaaa'; + const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; + + /** Resolves once the fire-and-forget swap has reached the registry. */ + const waitForPatch = () => + waitFor(async () => (patchCalls.length > 0 ? patchCalls[0] : null)); + + it('repoints the record for this PID at the new session', async () => { + const config = makeConfig(sessionA); + const aPath = config.storage.getRuntimeStatusPath(sessionA); + await writeRuntimeStatus(aPath, { + sessionId: sessionA, + workDir: tmpDir, + qwenVersion: '0.0.0-test', + }); + config.markRuntimeStatusEnabled(); + + config.startNewSession(sessionB); + + const patch = await waitForPatch(); + expect(patch).not.toBeNull(); + expect(patch).toMatchObject({ sessionId: sessionB, cwd: tmpDir }); + expect(patch!.name).toMatch(/^[\w.-]+$/); + }); + + it('patches the record even when this process never bootstrapped a sidecar', async () => { + // Registration and the sidecar are separate signals: a startup where + // the sidecar write failed but registerSession() succeeded leaves + // runtimeStatusEnabled off with a live record still on disk. Gating + // the patch on the sidecar flag would strand that record on the old + // session id for the rest of the process's life. Unlike + // clearRuntimeStatus, the patch is keyed by PID and cannot touch a + // sibling's record, so it needs no ownership gate of its own. + const config = makeConfig(sessionA); + + config.startNewSession(sessionB); + + expect(await waitForPatch()).toMatchObject({ sessionId: sessionB }); + // ...and the sibling sidecar rule above still holds: nothing written. + const bPath = config.storage.getRuntimeStatusPath(sessionB); + expect(await readRuntimeStatus(bPath)).toBeNull(); + }); + + it('patches the record even when the sidecar half of the swap throws', async () => { + const config = makeConfig(sessionA); + const aPath = config.storage.getRuntimeStatusPath(sessionA); + await writeRuntimeStatus(aPath, { + sessionId: sessionA, + workDir: tmpDir, + qwenVersion: '0.0.0-test', + }); + config.markRuntimeStatusEnabled(); + + // Full disk, or a chats/ directory that went read-only after startup. + failSidecarWrite.value = true; + config.startNewSession(sessionB); + + // The queue swallows the rejection, so without isolating the two + // halves the patch is simply never reached and nothing reports it. + expect(await waitForPatch()).toMatchObject({ sessionId: sessionB }); + }); + + it('does not touch the record when the session id does not change', async () => { + const config = makeConfig(sessionA); + config.markRuntimeStatusEnabled(); + + config.startNewSession(sessionA); + await new Promise((r) => setTimeout(r, 100)); + + expect(patchCalls).toEqual([]); + }); +}); + describe('Storage.getRuntimeStatusPath', () => { it('co-locates the sidecar under /chats/', () => { const storage = new Storage(tmpDir); From dc42240bbc2c005e90aa4aca8b8ee3d8e1cd4df9 Mon Sep 17 00:00:00 2001 From: qqqys Date: Sat, 8 Aug 2026 22:28:02 +0800 Subject: [PATCH 04/17] test: pin the session-registry wiring and the /proc starttime field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two review comments left open on #8728 after 0f0af4183. - gemini.test.tsx: `startInteractiveUI`'s register-on-start / unregister-on-exit block was the only production call site of `registerSession`/`unregisterSession` and was asserted nowhere, so deleting it — or registering the cleanup even when registration returned false — kept every test green. Two cases now pin the announced fields and that exactly one registered cleanup unlinks the record, and none does when registration failed. - process-liveness.test.ts: every existing assertion survived an off-by-one on the `/proc//stat` field index, which silently voids the PID-recycle guard. Two children started 80ms apart must now read strictly increasing tokens — a constant neighbour field cannot. Verified: both mutants (unconditional `registerCleanup`, and the whole block deleted) fail exactly the new CLI tests; `fields[19]` -> `[18]` and -> `[20]` each fail exactly the new core test. npm run build, npm run typecheck, eslint and prettier --check clean; packages/core process-liveness + session-registry + runtimeStatus.config = 44 passed; packages/cli gemini.test.tsx + commands/sessions = 110 passed. --- packages/cli/src/gemini.test.tsx | 78 +++++++++++++++++++ .../core/src/utils/process-liveness.test.ts | 54 +++++++++++++ 2 files changed, 132 insertions(+) diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 456b39666a4..b9efe72f2b1 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -3088,4 +3088,82 @@ describe('startInteractiveUI', () => { expect(performCheck.mock.calls.length).toBe(afterCleanup); }); }); + + describe('session registry announcement', () => { + const mockInitializationResult = { + authError: null, + themeError: null, + shouldOpenAuthDialog: false, + geminiMdFileCount: 0, + }; + + /** + * Invokes every callback handed to `registerCleanup`. The teardown + * chain also unmounts Ink, pops the kitty protocol and stats the + * transcript — none of which is under test here — so per-callback + * failures are swallowed. The only question these tests ask is whether + * one of the registered cleanups unregisters the session. + */ + async function runRegisteredCleanups() { + const { registerCleanup } = await import('./utils/cleanup.js'); + for (const [callback] of vi.mocked(registerCleanup).mock.calls) { + try { + await (callback as () => Promise | void)(); + } catch { + // ignored: an unrelated teardown step, not what is asserted. + } + } + } + + it('announces the session and unregisters it on exit', async () => { + const { registerSession, unregisterSession } = await import( + '@qwen-code/qwen-code-core' + ); + vi.mocked(registerSession).mockResolvedValueOnce(true); + + await startInteractiveUI( + mockConfig, + mockSettings, + mockStartupWarnings, + mockWorkspaceRoot, + mockInitializationResult, + ); + + expect(registerSession).toHaveBeenCalledTimes(1); + expect(registerSession).toHaveBeenCalledWith({ + sessionId: 'test-session-id', + cwd: '/root', + kind: 'interactive', + qwenVersion: '1.0.0', + }); + + // Presence is what the registry sells, so the record must not + // outlive the process: exactly one cleanup has to unlink it. + expect(unregisterSession).not.toHaveBeenCalled(); + await runRegisteredCleanups(); + expect(unregisterSession).toHaveBeenCalledTimes(1); + }); + + it('skips the unregister cleanup when registration failed', async () => { + const { registerSession, unregisterSession } = await import( + '@qwen-code/qwen-code-core' + ); + vi.mocked(registerSession).mockResolvedValueOnce(false); + + await startInteractiveUI( + mockConfig, + mockSettings, + mockStartupWarnings, + mockWorkspaceRoot, + mockInitializationResult, + ); + + expect(registerSession).toHaveBeenCalledTimes(1); + // `unregisterSession()` defaults to this PID, so unlinking on behalf + // of a registration that never happened would delete whichever + // record a sibling tool wrote for the same PID. + await runRegisteredCleanups(); + expect(unregisterSession).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 62aad1fd690..27749aff129 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi } from 'vitest'; +import { spawn, type ChildProcess } from 'node:child_process'; import { isPidAlive, isSameProcess, @@ -37,6 +38,29 @@ vi.mock('node:fs', async (importOriginal) => { /** A PID that is essentially certain not to be running. */ const DEAD_PID = 0x7ffffffe; +const delay = (ms: number) => + new Promise((resolve) => { + setTimeout(resolve, ms); + }); + +/** + * Starts a child that idles until it is killed, resolving once it is + * actually spawned so `/proc//stat` is readable. + */ +function spawnSleeper(): Promise { + const child = spawn( + process.execPath, + ['-e', 'setTimeout(() => {}, 60_000)'], + { + stdio: 'ignore', + }, + ); + return new Promise((resolve, reject) => { + child.once('spawn', () => resolve(child)); + child.once('error', reject); + }); +} + describe('isPidAlive', () => { it('reports the current process as alive', () => { expect(isPidAlive(process.pid)).toBe(true); @@ -73,6 +97,36 @@ describe('readProcStartToken', () => { it('returns null for a dead pid', () => { expect(readProcStartToken(DEAD_PID)).toBeNull(); }); + + it('grows with start order, so a later process reads a larger token', async () => { + if (process.platform !== 'linux') return; + // Pins the *field index*, which every other assertion here tolerates + // being wrong: `starttime`'s neighbours in /proc//stat are + // `itrealvalue` (hardcoded 0 on modern kernels) and `vsize` (equal for + // two copies of the same binary), and a constant cannot be strictly + // increasing. Without this, an off-by-one edit hands every process the + // same token, `isSameProcess` stops detecting PID recycling, and dead + // sessions resurrect under a reused PID. + // + // /proc reports `starttime` in clock ticks at a fixed USER_HZ of 100, + // i.e. 10ms per tick, so the gap below is several ticks wide. + const first = await spawnSleeper(); + try { + await delay(80); + const second = await spawnSleeper(); + try { + const firstToken = readProcStartToken(first.pid!); + const secondToken = readProcStartToken(second.pid!); + expect(firstToken).toMatch(/^\d+$/); + expect(secondToken).toMatch(/^\d+$/); + expect(Number(secondToken)).toBeGreaterThan(Number(firstToken)); + } finally { + second.kill('SIGKILL'); + } + } finally { + first.kill('SIGKILL'); + } + }); }); describe('isSameProcess', () => { From 552dfbbcd0ff353e07e5e24161478043a0c95a9c Mon Sep 17 00:00:00 2001 From: qqqys Date: Sun, 9 Aug 2026 04:32:00 +0800 Subject: [PATCH 05/17] fix(core): do not sweep session records from another PID namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three Critical findings in review 4889504524 on #8728. **The sweep deleted live sessions' records across a namespace boundary** (comment 3741404815). `listLiveSessions` read a PID that is invisible (ESRCH) or token-mismatched as proof of death, but `sandbox.ts` mounts the host's global qwen dir — `sessions/` included — into a container that gets its own PID namespace, so both sides read each other's records while neither can see the other's processes. Registration is startup-only and `patchSessionRecord` no-ops on a missing record, so a sweep from the wrong side hid a live session for the rest of its life. Each record now carries the namespace its PID was allocated in, read from `/proc/self/ns/pid` by the new `readPidNamespaceId()`, and enumeration ignores — never lists, never sweeps — any record it cannot attribute to its own namespace. Two nulls is the no-namespaces case (every non-Linux platform) and stays on the original path. The field is additive under schema version 1, so an older reader still parses these records. **Three POSIX mode assertions had no win32 guard** (comment 3741404812): Windows synthesizes `st_mode` from file attributes, so `0o700`/`0o600` cannot hold and `test_windows` was deterministically red. Same `it.skipIf` as `atomicFileWrite.test.ts` and `session-writer-lease.test.ts`. **`gemini.test.tsx`'s registerSession stub was order-dependent** (comment 3741404813): the earlier describes' `vi.restoreAllMocks()` wiped its implementation, so every `startInteractiveUI` test silently took the registration-failed branch. Re-armed in the describe's `beforeEach`, and the stale `registerCleanup` count updated to the 2 that production registers; that test now reads the last cleanup, since the first is the registry's. Verification: npm run build, npm run typecheck, eslint and prettier --check on all six files — all clean. core process-liveness + session-registry + runtimeStatus.config = 50 passed; cli ps.test.ts = 11 passed; cli gemini.test.tsx = 71 passed, and the previously order- dependent case now passes on its own too (it failed 1-vs-2 before). Two-way probes: dropping the namespace guard turns exactly the two new tests red (a live foreign record gets listed, an invisible one gets unlinked); pointing `readPidNamespaceId` at `/proc/self/ns/mnt` turns both of its tests red; flipping the skipIf predicate to 'linux' skips exactly those three mode cases and no others. --- packages/cli/src/commands/sessions/ps.test.ts | 1 + packages/cli/src/gemini.test.tsx | 22 ++- .../src/services/session-registry.test.ts | 158 +++++++++++++++--- .../core/src/services/session-registry.ts | 35 ++++ .../core/src/utils/process-liveness.test.ts | 50 ++++++ packages/core/src/utils/process-liveness.ts | 28 ++++ 6 files changed, 265 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index d32bb0439fb..1310f42008d 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -30,6 +30,7 @@ function record( schemaVersion: 1, pid: 4242, procStart: '123', + pidNamespace: '4026531836', sessionId: 'sess-1', cwd: '/w/app', name: 'app-ab', diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index b9efe72f2b1..5bb2a601b6d 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -2382,8 +2382,18 @@ describe('startInteractiveUI', () => { let originalStdoutIsTTY: boolean | undefined; let restoreCiEnv = () => {}; - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks(); + // The `main function` and kitty-protocol describes above tear down + // with vi.restoreAllMocks(), which wipes the suite-level + // registerSession stub's implementation. Left wiped it resolves + // undefined, so every test here would silently take + // startInteractiveUI's registration-failed branch and register one + // cleanup fewer than production does — green in a full-file run, red + // the moment a single test is run on its own. Re-arm it so the + // default path through this describe is the one users get. + const { registerSession } = await import('@qwen-code/qwen-code-core'); + vi.mocked(registerSession).mockResolvedValue(true); restoreCiEnv = clearCiEnv(); vi.stubEnv('TERM', 'xterm-256color'); originalStdoutIsTTY = process.stdout.isTTY; @@ -2646,12 +2656,14 @@ describe('startInteractiveUI', () => { mockInitializationResult, ); - // Verify all startup tasks were called + // Verify all startup tasks were called. Two cleanups: the session + // registry's unregister hook, then the unmount hook. expect(getCliVersion).toHaveBeenCalledTimes(1); - expect(registerCleanup).toHaveBeenCalledTimes(1); + expect(registerCleanup).toHaveBeenCalledTimes(2); - // Verify cleanup handler is registered with unmount function - const cleanupFn = vi.mocked(registerCleanup).mock.calls[0][0]; + // Verify cleanup handler is registered with unmount function. Read + // the last call, not the first — the first is now the registry's. + const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0]; expect(typeof cleanupFn).toBe('function'); expect(mockStartPostRenderPrefetches).toHaveBeenCalledWith( diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 991344d12be..dd9f1885073 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -18,7 +18,10 @@ import { unregisterSession, SESSION_REGISTRY_SCHEMA_VERSION, } from './session-registry.js'; -import { readProcStartToken } from '../utils/process-liveness.js'; +import { + readPidNamespaceId, + readProcStartToken, +} from '../utils/process-liveness.js'; vi.mock('../config/storage.js', () => { let mockDir = '/tmp/session-registry-test'; @@ -138,39 +141,75 @@ describe('registerSession', () => { } }); - it('creates the registry directory as 0700', async () => { + it('records the PID namespace the pid was allocated in', async () => { await registerSession({ sessionId: 's1', cwd: '/w/app', kind: 'interactive', }); - const stat = await fs.stat(getSessionRegistryDir()); - expect(stat.mode & 0o777).toBe(0o700); + + // Same reason as the token above: the self-pid shortcut skips every + // check, so read it back as a different process. A regression writing + // `pidNamespace: null` would let a sandboxed sibling sweep this + // record while the session is still running. + const [record] = await listLiveSessions({ + selfPid: DEAD_PID, + sweepStale: false, + }); + expect(record.pidNamespace).toBe(readPidNamespaceId()); + if (process.platform === 'linux') { + expect(record.pidNamespace).toMatch(/^\d+$/); + } else { + expect(record.pidNamespace).toBeNull(); + } }); - it('tightens a pre-existing loose registry directory', async () => { - await fs.mkdir(getSessionRegistryDir(), { recursive: true, mode: 0o755 }); - await fs.chmod(getSessionRegistryDir(), 0o755); + // Windows synthesizes st_mode from file attributes (a writable dir reads + // 0o777, a file 0o666) and chmod there can only toggle the read-only bit, + // so POSIX permission bits are not assertable on the test_windows gate. + // Same guard as atomicFileWrite.test.ts and session-writer-lease.test.ts. + it.skipIf(process.platform === 'win32')( + 'creates the registry directory as 0700', + async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }, + ); - await registerSession({ - sessionId: 's1', - cwd: '/w/app', - kind: 'interactive', - }); + it.skipIf(process.platform === 'win32')( + 'tightens a pre-existing loose registry directory', + async () => { + await fs.mkdir(getSessionRegistryDir(), { recursive: true, mode: 0o755 }); + await fs.chmod(getSessionRegistryDir(), 0o755); - const stat = await fs.stat(getSessionRegistryDir()); - expect(stat.mode & 0o777).toBe(0o700); - }); + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); - it('writes the record as 0600', async () => { - await registerSession({ - sessionId: 's1', - cwd: '/w/app', - kind: 'interactive', - }); - const stat = await fs.stat(getSessionRecordPath()); - expect(stat.mode & 0o777).toBe(0o600); - }); + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'writes the record as 0600', + async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + const stat = await fs.stat(getSessionRecordPath()); + expect(stat.mode & 0o777).toBe(0o600); + }, + ); it('reports failure instead of throwing when the home dir is unwritable', async () => { __setMockGlobalDir(path.join(tmpDir, 'nope', '\0invalid')); @@ -245,6 +284,7 @@ describe('listLiveSessions', () => { schemaVersion: 1, pid: DEAD_PID, procStart: null, + pidNamespace: readPidNamespaceId(), sessionId: 's-dead', cwd: '/w/app', name: 'app-aa', @@ -262,6 +302,7 @@ describe('listLiveSessions', () => { const filePath = await writeRaw(`${DEAD_PID}.json`, { schemaVersion: 1, pid: DEAD_PID, + pidNamespace: readPidNamespaceId(), sessionId: 's-dead', cwd: '/w/app', name: 'app-aa', @@ -281,6 +322,7 @@ describe('listLiveSessions', () => { schemaVersion: 1, pid: process.pid, procStart: '1', + pidNamespace: readPidNamespaceId(), sessionId: 's-recycled', cwd: '/w/app', name: 'app-aa', @@ -361,6 +403,73 @@ describe('listLiveSessions', () => { ]); }); + // The sandbox mounts the host's global qwen dir — `sessions/` included — + // into a container that gets its own PID namespace, so both sides read + // each other's records while neither can see the other's processes. A + // PID that is invisible here is not therefore dead there. + it('neither lists nor sweeps a record from another PID namespace', async () => { + // Same PID as this very process, so the local liveness probe would + // say "alive"; the record still must not be reported, because that + // number names an unrelated process on the other side of the border. + const livePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: readProcStartToken(process.pid), + pidNamespace: 'not-our-namespace', + sessionId: 's-foreign-live', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + // And a PID that is invisible here, which is what the sweep would + // otherwise read as proof of death. + const deadPath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: null, + pidNamespace: 'not-our-namespace', + sessionId: 's-foreign-invisible', + cwd: '/w/app', + name: 'app-bb', + kind: 'interactive', + startedAt: Date.now(), + }); + + expect(await listLiveSessions({ selfPid: process.ppid })).toEqual([]); + // Registration is startup-only and patchSessionRecord no-ops on a + // missing record, so an unlink here would hide a live session for the + // rest of its life. Both files must survive. + await expect(fs.stat(livePath)).resolves.toBeDefined(); + await expect(fs.stat(deadPath)).resolves.toBeDefined(); + }); + + it('leaves a record alone when its namespace is unrecorded', async () => { + // Written by a build that predates the namespace field. We cannot + // prove it is ours, so we cannot prove its PID is dead either. + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: null, + sessionId: 's-unknown-ns', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + const live = await listLiveSessions(); + if (readPidNamespaceId() === null) { + // No namespaces on this platform: nothing to disagree about, so the + // record is ours and the sweep proceeds as it always did. + expect(live).toEqual([]); + await expect(fs.stat(filePath)).rejects.toThrow(); + } else { + expect(live).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + } + }); + it('sorts newest first', async () => { await registerSession({ sessionId: 's-self', @@ -371,6 +480,7 @@ describe('listLiveSessions', () => { await writeRaw(`${process.ppid}.json`, { schemaVersion: 1, pid: process.ppid, + pidNamespace: readPidNamespaceId(), sessionId: 's-parent', cwd: '/w/other', name: 'other-bb', diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 06bc7f2de7d..2cf5e804900 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -36,6 +36,15 @@ * not resurrect a dead session. Records that fail that check are swept * during enumeration; anything we cannot positively prove dead is left * alone. + * + * That liveness probe is only meaningful inside one PID namespace, and + * this directory can span several: the sandbox mounts the host's global + * qwen dir into a container that gets its own PID namespace, so both + * sides read each other's records while neither can see the other's + * processes. Each record therefore carries the namespace it was written + * in (see `readPidNamespaceId`), and enumeration ignores — never sweeps, + * never lists — any record it cannot attribute to its own namespace. A + * namespace-local `ESRCH` is not proof of death for a shared directory. */ import { createHash } from 'node:crypto'; @@ -46,6 +55,7 @@ import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSameProcess, + readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; @@ -85,6 +95,12 @@ export interface SessionRegistryRecord { pid: number; /** Start-time token guarding against PID reuse; null where unavailable. */ procStart: string | null; + /** + * The PID namespace `pid` was allocated in; null where the platform has + * no such concept. Readers that cannot match it against their own must + * treat the PID as unreadable rather than dead. + */ + pidNamespace: string | null; sessionId: string; cwd: string; /** Short human-facing label, unique-ish per session. */ @@ -152,6 +168,7 @@ export async function registerSession( schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, pid, procStart: readProcStartToken(pid), + pidNamespace: readPidNamespaceId(), sessionId: fields.sessionId, cwd: fields.cwd, name: fields.name ?? deriveSessionName(fields.cwd, fields.sessionId), @@ -260,6 +277,10 @@ export async function listLiveSessions( return []; } + // Read once per enumeration, not once per record: a process cannot + // change PID namespace under itself, and this is a syscall. + const selfNamespace = readPidNamespaceId(); + const live: SessionRegistryRecord[] = []; await Promise.all( entries @@ -281,6 +302,18 @@ export async function listLiveSessions( return; } + // Every check below reads `record.pid` as a number in *our* PID + // namespace. When the record came from another one — or from a + // writer whose namespace we cannot pin down — that reading is + // meaningless, so the record is neither reported (the PID would + // name some unrelated local process) nor swept (a local ESRCH + // says nothing about a process in another namespace, and + // registration is startup-only, so an unlink here would hide a + // live session for the rest of its life). Two nulls is the + // no-namespaces case — every non-Linux platform — and stays on + // the original path. + if (selfNamespace !== record.pidNamespace) return; + if (isSameProcess(record.pid, record.procStart)) { live.push(record); return; @@ -355,6 +388,7 @@ async function readRecord( } const procStart = value['procStart']; + const pidNamespace = value['pidNamespace']; const qwenVersion = value['qwenVersion']; const peerProtocol = value['peerProtocol']; @@ -362,6 +396,7 @@ async function readRecord( schemaVersion, pid, procStart: typeof procStart === 'string' ? procStart : null, + pidNamespace: typeof pidNamespace === 'string' ? pidNamespace : null, sessionId, cwd, name, diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 27749aff129..4953705ce6f 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -6,9 +6,11 @@ import { describe, it, expect, vi } from 'vitest'; import { spawn, type ChildProcess } from 'node:child_process'; +import { readlinkSync } from 'node:fs'; import { isPidAlive, isSameProcess, + readPidNamespaceId, readProcStartToken, } from './process-liveness.js'; @@ -18,6 +20,9 @@ import { */ const procReadFails = vi.hoisted(() => ({ value: false })); +/** The same, for the `/proc/self/ns/pid` readlink. */ +const nsReadFails = vi.hoisted(() => ({ value: false })); + vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); return { @@ -32,6 +37,15 @@ vi.mock('node:fs', async (importOriginal) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any return (actual.readFileSync as any)(...args); }) as typeof actual.readFileSync, + readlinkSync: ((...args: unknown[]) => { + if (nsReadFails.value) { + throw Object.assign(new Error('EACCES: /proc unreadable'), { + code: 'EACCES', + }); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (actual.readlinkSync as any)(...args); + }) as typeof actual.readlinkSync, }; }); @@ -166,3 +180,39 @@ describe('isSameProcess', () => { } }); }); + +describe('readPidNamespaceId', () => { + it('reads a stable inode on Linux and null everywhere else', () => { + const id = readPidNamespaceId(); + if (process.platform === 'linux') { + // `/proc/self/ns/pid` reads `pid:[]`; only the inode is + // returned, and a process cannot change namespace under itself, so + // two reads in one process must agree. + expect(id).toMatch(/^\d+$/); + expect(readPidNamespaceId()).toBe(id); + } else { + expect(id).toBeNull(); + } + }); + + it('agrees with the namespace this process actually reports', () => { + if (process.platform !== 'linux') return; + // Anchored against the real symlink rather than a second call to the + // function under test: a regression that returned a constant, or read + // the wrong ns entry, would otherwise stay green above. + const target = readlinkSync('/proc/self/ns/pid'); + expect(target).toBe(`pid:[${readPidNamespaceId()}]`); + // The mount namespace is a different entry with the same shape, so a + // typo'd path is a live failure mode worth pinning against. + expect(target).not.toBe(readlinkSync('/proc/self/ns/mnt')); + }); + + it('returns null instead of throwing when the link cannot be read', () => { + nsReadFails.value = true; + try { + expect(readPidNamespaceId()).toBeNull(); + } finally { + nsReadFails.value = false; + } + }); +}); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index 6119b955de9..b21fca5374f 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -77,6 +77,34 @@ export function readProcStartToken(pid: number): string | null { return startTime !== undefined && /^\d+$/.test(startTime) ? startTime : null; } +/** + * An opaque identifier for the PID namespace this process lives in, or + * `null` where the platform does not expose one. + * + * A PID only means something inside one namespace. Anything that writes a + * PID into a directory another namespace can also read — a container and + * its host sharing a mounted home dir, say — has to record *which* + * namespace the number came from, or a namespace-local "no such process" + * reads as proof of death for a process that is very much alive. + * + * Backed by `/proc/self/ns/pid`, a symlink whose target is + * `pid:[]`; the inode is stable for the namespace's lifetime and + * identical for two processes exactly when a PID means the same thing to + * both of them. Returns `null` off Linux, where the concept does not + * exist — callers must treat a `null` on both sides as "no namespace + * boundary to worry about", and a mismatch of any kind as unprovable. + */ +export function readPidNamespaceId(): string | null { + if (process.platform !== 'linux') return null; + try { + const target = fs.readlinkSync('/proc/self/ns/pid'); + const match = /^pid:\[(\d+)\]$/.exec(target); + return match?.[1] ?? null; + } catch { + return null; + } +} + /** * True when `pid` is alive AND is the same process that recorded * `procStart`. From d62d196359492e4b188bd5347d930512c328e087 Mon Sep 17 00:00:00 2001 From: qqqys Date: Sun, 9 Aug 2026 09:34:50 +0800 Subject: [PATCH 06/17] fix(core): key session-registry trust on the record's origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review found five ways `.json` is treated as proof of ownership when it is only proof of a number. R3-1: the self-PID shortcut ran before the namespace gate, so a record written in another PID namespace whose PID happens to equal ours was adopted as this session — the gate added last round never ran for the one collision that matters. The gate now runs first. R3-10: that gate was also machine-blind. The initial PID namespace inode is the same constant on every non-containerized Linux host, so two machines sharing one registry directory (NFS home, `QWEN_HOME` on a shared volume) pass it, and each sweeps the other's records on a purely local ESRCH — no PID collision required. Records now carry a machineId and the gate is (machine, namespace). `/etc/machine-id` rather than the suggested `boot_id`: a boot id would make every pre-reboot record permanently unattributable, hence unsweepable and — with the write guard below — able to block registration at that PID forever. Reboot-recycled PIDs are already `procStart`'s job, and its token is boot-relative. R3-2: only the read/sweep path was guarded. `registerSession` overwrote, `patchSessionRecord` merged into, and `unregisterSession` unlinked whatever record sat at the path. All three now check the origin first; register refuses rather than clobbering, which leaves the loser of a bare-PID collision absent from discovery instead of destroying the winner's record. R3-14: both write sites called `atomicWriteJSON` without `noFollow`, so a symlink planted at `.json` — in a directory the sandbox shares across a trust boundary — redirected the write, and its forced 0600, to a file outside the mounts. R3-7: `relocateWorkingDirectory` refreshed the runtime.json sidecar on `/cd` but never the registry, leaving `cwd` and the derived name advertising the directory the session had left. Patched on the same write queue, and deliberately not gated on runtimeStatusEnabled for the reason startNewSession already documents. --- packages/cli/src/commands/sessions/ps.test.ts | 1 + packages/core/src/config/config.ts | 16 ++ .../src/services/session-registry.test.ts | 222 +++++++++++++++++- .../core/src/services/session-registry.ts | 147 +++++++++--- packages/core/src/utils/process-liveness.ts | 48 ++++ .../src/utils/runtimeStatus.config.test.ts | 45 +++- 6 files changed, 446 insertions(+), 33 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index 1310f42008d..11bbb27fc29 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -31,6 +31,7 @@ function record( pid: 4242, procStart: '123', pidNamespace: '4026531836', + machineId: 'test-machine', sessionId: 'sess-1', cwd: '/w/app', name: 'app-ab', diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index ca0108d8d7d..35252647182 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -4840,6 +4840,22 @@ export class Config { this.cwd = expected; resetPreloadedContentGenerator(this.contentGenerator); await this.refreshCurrentRuntimeStatus(expected); + // The registry advertises this session's working directory and the + // name derived from it, so refreshing only the sidecar would leave + // `qwen sessions ps` naming the directory the session just left — + // until the next session swap, and possibly never. Queued on the same + // chain as the sidecar write so it cannot interleave with a + // concurrent swap, and deliberately not gated on runtimeStatusEnabled + // for the reason startNewSession gives: this patch cannot trample a + // sibling's record, so tying it to the sidecar's ownership signal + // would only let the two drift apart. + this.queueRuntimeStatusWrite(async () => { + await patchSessionRecord({ + cwd: expected, + name: deriveSessionName(expected, this.sessionId), + }); + }); + await this.flushRuntimeStatusWrites(); this.workspaceContext.applyRootDirectories(workspaceDirectories); this.fileDiscoveryService = null; this.sessionService = undefined; diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index dd9f1885073..bc707a93e44 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -19,6 +19,7 @@ import { SESSION_REGISTRY_SCHEMA_VERSION, } from './session-registry.js'; import { + readMachineId, readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; @@ -52,14 +53,23 @@ afterEach(async () => { await fs.rm(tmpDir, { recursive: true, force: true }); }); +/** + * Plant a file in the registry directory verbatim. + * + * Object bodies get this machine's `machineId` unless they name one + * themselves: a record without it reads as "written somewhere else", + * which is what the origin tests below assert deliberately and every + * other test would then hit by accident. + */ async function writeRaw(fileName: string, body: unknown): Promise { const dir = getSessionRegistryDir(); await fs.mkdir(dir, { recursive: true }); const filePath = path.join(dir, fileName); - await fs.writeFile( - filePath, - typeof body === 'string' ? body : JSON.stringify(body), - ); + const content = + typeof body === 'string' + ? body + : JSON.stringify({ machineId: readMachineId(), ...(body as object) }); + await fs.writeFile(filePath, content); return filePath; } @@ -164,6 +174,85 @@ describe('registerSession', () => { } }); + it('records the machine the pid was allocated on', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }); + + // The namespace id alone cannot carry this: every non-containerized + // Linux host reports the same initial-namespace inode, so a record + // without a machine claim is sweepable by any other machine sharing + // the directory. + const [record] = await listLiveSessions({ + selfPid: DEAD_PID, + sweepStale: false, + }); + expect(record.machineId).toBe(readMachineId()); + expect(record.machineId).not.toBeNull(); + }); + + it('refuses to overwrite a record from another origin', async () => { + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + + // Their PID number, not ours to reuse: the session behind it is still + // running over there, registration is startup-only, so clobbering the + // file removes it from discovery for the rest of its life. + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + }), + ).toBe(false); + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-theirs', + cwd: '/w/theirs', + }); + }); + + // Symlinks are not creatable without elevation on stock Windows. + it.skipIf(process.platform === 'win32')( + 'does not write through a symlink planted at its record path', + async () => { + const victim = path.join(tmpDir, 'victim.txt'); + await fs.writeFile(victim, 'do not clobber me'); + const dir = getSessionRegistryDir(); + await fs.mkdir(dir, { recursive: true }); + await fs.symlink(victim, getSessionRecordPath()); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + // The sandbox mounts this directory across a trust boundary, so a + // planted `.json -> ` is a write primitive pointed + // at a file the attacker chooses. Replacing the link is the correct + // outcome, not refusing to register. + expect(await fs.readFile(victim, 'utf8')).toBe('do not clobber me'); + expect((await fs.lstat(getSessionRecordPath())).isSymbolicLink()).toBe( + false, + ); + expect(await listLiveSessions({ includeSelf: true })).toHaveLength(1); + }, + ); + // Windows synthesizes st_mode from file attributes (a writable dir reads // 0o777, a file 0o666) and chmod there can only toggle the read-only bit, // so POSIX permission bits are not assertable on the test_windows gate. @@ -247,6 +336,30 @@ describe('patchSessionRecord', () => { await patchSessionRecord({ sessionId: 'new' }); expect(await listLiveSessions({ includeSelf: true })).toEqual([]); }); + + it('leaves a record from another origin unmerged', async () => { + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: null, + pidNamespace: 'not-our-namespace', + machineId: readMachineId(), + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + + // A merge here would rewrite their sessionId/cwd/name in place, + // pointing every reader of that record at our transcript. + await patchSessionRecord({ sessionId: 'ours', cwd: '/w/ours' }); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-theirs', + cwd: '/w/theirs', + }); + }); }); describe('unregisterSession', () => { @@ -263,6 +376,27 @@ describe('unregisterSession', () => { it('is a no-op when nothing was registered', async () => { await expect(unregisterSession()).resolves.toBeUndefined(); }); + + it('leaves a record from another origin in place', async () => { + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + + // Our exit says nothing about their session, and they will not + // re-register: registration happens once, at startup. + await unregisterSession(); + + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); }); describe('listLiveSessions', () => { @@ -444,6 +578,86 @@ describe('listLiveSessions', () => { await expect(fs.stat(deadPath)).resolves.toBeDefined(); }); + it('does not adopt a foreign record sitting at our own PID number', async () => { + // The test above routes around the self-pid shortcut by enumerating + // as another process. This one does not: container PIDs are small and + // host PIDs recycle, so `.json` written on the other side of + // the border is the collision that actually happens. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: readProcStartToken(process.pid), + pidNamespace: 'not-our-namespace', + sessionId: 's-foreign-self', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + // Reported as our own session, `qwen sessions ps` would print their + // sessionId, cwd and name as this process's — and ours not at all. + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + // `QWEN_HOME` on a shared volume, or an NFS home: one registry + // directory, two machines. The initial PID namespace inode is the same + // constant on both, so the namespace gate alone waves them through. + it('neither lists nor sweeps a record from another machine', async () => { + const deadPath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-remote-invisible', + cwd: '/w/app', + name: 'app-bb', + kind: 'interactive', + startedAt: Date.now(), + }); + // No PID collision needed for the sweep to fire: it is enough that + // this machine has no process with that number, which is the normal + // case. + const livePath = await writeRaw(`${process.ppid}.json`, { + schemaVersion: 1, + pid: process.ppid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-remote-live', + cwd: '/w/app', + name: 'app-cc', + kind: 'interactive', + startedAt: Date.now(), + }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(deadPath)).resolves.toBeDefined(); + await expect(fs.stat(livePath)).resolves.toBeDefined(); + }); + + it('leaves a record alone when its machine is unrecorded', async () => { + // Written by a build that predates the machine field: no claim we can + // check, so no proof its PID is ours to read. + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: null, + sessionId: 's-unknown-machine', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + it('leaves a record alone when its namespace is unrecorded', async () => { // Written by a build that predates the namespace field. We cannot // prove it is ours, so we cannot prove its PID is dead either. diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 2cf5e804900..9caaa518393 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -37,14 +37,23 @@ * during enumeration; anything we cannot positively prove dead is left * alone. * - * That liveness probe is only meaningful inside one PID namespace, and - * this directory can span several: the sandbox mounts the host's global - * qwen dir into a container that gets its own PID namespace, so both - * sides read each other's records while neither can see the other's - * processes. Each record therefore carries the namespace it was written - * in (see `readPidNamespaceId`), and enumeration ignores — never sweeps, - * never lists — any record it cannot attribute to its own namespace. A - * namespace-local `ESRCH` is not proof of death for a shared directory. + * That liveness probe is only meaningful inside one PID namespace on one + * machine, and this directory can span both: the sandbox mounts the + * host's global qwen dir into a container that gets its own PID + * namespace, and `QWEN_HOME` on a shared volume points two machines at + * one directory. Either way both sides read each other's records while + * neither can see the other's processes. Each record therefore carries + * its *origin* — the machine and the PID namespace it was written in (see + * `readMachineId` and `readPidNamespaceId`) — and everything here ignores + * records from another origin: enumeration neither lists nor sweeps them, + * and the write paths neither overwrite, patch nor unlink them. A + * namespace-local `ESRCH` is not proof of death for a shared directory, + * and `.json` is not proof of ownership. + * + * Two origins can still *want* the same `.json`, because the key is + * a bare PID. The loser of that collision is simply absent from + * discovery, which is the safe half of the trade: a durable fix needs + * origin-disambiguated keying, not a wider guard here. */ import { createHash } from 'node:crypto'; @@ -55,6 +64,7 @@ import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSameProcess, + readMachineId, readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; @@ -101,6 +111,13 @@ export interface SessionRegistryRecord { * treat the PID as unreadable rather than dead. */ pidNamespace: string | null; + /** + * The machine `pid` was allocated on; null where none could be read. + * Same rule as `pidNamespace`, and needed alongside it: the initial PID + * namespace id is identical on every non-containerized Linux host, so + * that field alone lets one machine read another's PIDs as its own. + */ + machineId: string | null; sessionId: string; cwd: string; /** Short human-facing label, unique-ish per session. */ @@ -153,12 +170,37 @@ export function deriveSessionName(cwd: string, sessionId: string): string { return `${base || 'session'}-${suffix}`; } +/** + * True when `record` was written from this machine and this PID + * namespace — the only case in which `record.pid` is a number this + * process can probe, and the only case in which `.json` is this + * process's to write. + * + * Compared strictly, nulls included: two nulls is the no-identity case + * (a platform that exposes neither) and stays on the original + * trust-the-path behaviour, while a null on one side only means the + * writer made no claim we can check. + */ +function isSameOrigin( + record: Pick, + selfMachine: string | null, + selfNamespace: string | null, +): boolean { + return ( + record.machineId === selfMachine && record.pidNamespace === selfNamespace + ); +} + /** * Write this process's record. Best-effort: a read-only or full home * directory must not stop a session from starting, so failures are logged * and reported, never thrown. * - * Returns true when the record was written. + * Returns true when the record was written. Returns false — without + * touching the file — when `.json` already holds a record from + * another origin: that PID number belongs to someone else's namespace or + * machine, and overwriting it would both destroy a live session's + * discovery entry and point readers at the wrong transcript. */ export async function registerSession( fields: RegisterSessionFields, @@ -169,6 +211,7 @@ export async function registerSession( pid, procStart: readProcStartToken(pid), pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), sessionId: fields.sessionId, cwd: fields.cwd, name: fields.name ?? deriveSessionName(fields.cwd, fields.sessionId), @@ -185,9 +228,31 @@ export async function registerSession( // the directory already exists — chmod is what actually guarantees // 0700 on an upgrade from a build that created it more loosely. await fs.chmod(dir, REGISTRY_DIR_MODE); - await atomicWriteJSON(getSessionRecordPath(pid), record, { + + const filePath = getSessionRecordPath(pid); + // Registration is the one write with nothing to merge into, so it is + // also the one that would happily clobber a stranger. A record from + // another origin at our PID number is not stale, it is not ours, and + // it cannot be proven dead from here. + const existing = await readRecord(filePath); + if ( + existing !== null && + !isSameOrigin(existing, record.machineId, record.pidNamespace) + ) { + debugLogger.debug( + `registerSession skipped: ${filePath} holds a record from another origin`, + ); + return false; + } + + // `noFollow` keeps a pre-planted `.json` symlink from redirecting + // this write (and its forced 0600) to a file outside the registry: + // the sandbox shares this directory across a trust boundary, so the + // planting side is not hypothetical. + await atomicWriteJSON(filePath, record, { mode: REGISTRY_FILE_MODE, forceMode: true, + noFollow: true, }); return true; } catch (error) { @@ -206,6 +271,9 @@ export async function registerSession( * No-ops when the record is missing: a session that failed to register * should not be resurrected by a later patch, because the resurrected * record would be missing whatever else registration would have set. + * No-ops too when the record present at this PID came from another + * origin — merging into it would rewrite a stranger's sessionId, cwd and + * name, sending discovery to the wrong transcript. */ export async function patchSessionRecord( patch: Partial>, @@ -215,22 +283,35 @@ export async function patchSessionRecord( try { const existing = await readRecord(filePath); if (existing === null) return; + if (!isSameOrigin(existing, readMachineId(), readPidNamespaceId())) return; await atomicWriteJSON( filePath, { ...existing, ...patch }, - { mode: REGISTRY_FILE_MODE, forceMode: true }, + { mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true }, ); } catch (error) { debugLogger.debug(`patchSessionRecord failed: ${describe(error)}`); } } -/** Remove this process's record. Safe to call when none was written. */ +/** + * Remove this process's record. Safe to call when none was written. + * + * Unlinks only what it can read back as its own: a record from another + * origin at this PID number belongs to a session that is still running + * somewhere, and one that will not re-register (registration is + * startup-only). Anything unparseable is left too — it is not a record + * this code wrote, so it is not this code's to delete. + */ export async function unregisterSession( pid: number = process.pid, ): Promise { + const filePath = getSessionRecordPath(pid); try { - await fs.unlink(getSessionRecordPath(pid)); + const existing = await readRecord(filePath); + if (existing === null || existing.pid !== pid) return; + if (!isSameOrigin(existing, readMachineId(), readPidNamespaceId())) return; + await fs.unlink(filePath); } catch (error) { if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; debugLogger.debug(`unregisterSession failed: ${describe(error)}`); @@ -278,8 +359,9 @@ export async function listLiveSessions( } // Read once per enumeration, not once per record: a process cannot - // change PID namespace under itself, and this is a syscall. + // change PID namespace or machine under itself, and both are syscalls. const selfNamespace = readPidNamespaceId(); + const selfMachine = readMachineId(); const live: SessionRegistryRecord[] = []; await Promise.all( @@ -295,25 +377,32 @@ export async function listLiveSessions( // never sweep it — we cannot reason about which PID it describes. if (`${record.pid}.json` !== name) return; + // Every check below — the self-PID comparison included — reads + // `record.pid` as a number on *our* machine in *our* PID + // namespace. When the record came from another origin, or from a + // writer whose origin we cannot pin down, that reading is + // meaningless: the record is neither reported (the PID would name + // some unrelated local process, up to and including this one) nor + // swept (a local ESRCH says nothing about a process elsewhere, + // and registration is startup-only, so an unlink here would hide + // a live session for the rest of its life). This has to run + // before the self-PID shortcut below, or a foreign record sitting + // at our own PID number is adopted as our session without ever + // reaching the gate. + if (!isSameOrigin(record, selfMachine, selfNamespace)) return; + if (record.pid === selfPid) { - // Trust our own record without probing: `isSameProcess` on self - // is always true, and the token read is pure overhead. + // Report the record at our own PID without probing: the origin + // gate above has established it describes this machine and this + // namespace, and the PID is ours, so it is alive by + // construction. (It is not necessarily *this session's* record + // — a same-origin predecessor that died on this PID leaves one + // behind — but that is a liveness-of-content question, not one + // this shortcut answers.) if (includeSelf) live.push(record); return; } - // Every check below reads `record.pid` as a number in *our* PID - // namespace. When the record came from another one — or from a - // writer whose namespace we cannot pin down — that reading is - // meaningless, so the record is neither reported (the PID would - // name some unrelated local process) nor swept (a local ESRCH - // says nothing about a process in another namespace, and - // registration is startup-only, so an unlink here would hide a - // live session for the rest of its life). Two nulls is the - // no-namespaces case — every non-Linux platform — and stays on - // the original path. - if (selfNamespace !== record.pidNamespace) return; - if (isSameProcess(record.pid, record.procStart)) { live.push(record); return; @@ -389,6 +478,7 @@ async function readRecord( const procStart = value['procStart']; const pidNamespace = value['pidNamespace']; + const machineId = value['machineId']; const qwenVersion = value['qwenVersion']; const peerProtocol = value['peerProtocol']; @@ -397,6 +487,7 @@ async function readRecord( pid, procStart: typeof procStart === 'string' ? procStart : null, pidNamespace: typeof pidNamespace === 'string' ? pidNamespace : null, + machineId: typeof machineId === 'string' ? machineId : null, sessionId, cwd, name, diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index b21fca5374f..b371c7768e7 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -17,6 +17,7 @@ */ import * as fs from 'node:fs'; +import * as os from 'node:os'; import { isNodeError } from './errors.js'; /** @@ -105,6 +106,53 @@ export function readPidNamespaceId(): string | null { } } +/** + * Candidate sources for a machine identity, most specific first. + * + * Both are the standard 32-hex-character host id; the dbus copy is the + * fallback for distributions that populate it but not `/etc/machine-id`. + */ +const MACHINE_ID_FILES = ['/etc/machine-id', '/var/lib/dbus/machine-id']; + +/** + * An opaque identifier for the machine this process is running on, or + * `null` when none could be read. + * + * A PID namespace id does not identify a machine: the initial namespace's + * inode is the same constant on every non-containerized Linux box, so two + * machines sharing one registry directory — an NFS home, a `QWEN_HOME` on + * a shared volume — agree on it and each reads the other's PIDs as its + * own. Pair this with {@link readPidNamespaceId}: together they say + * whether a recorded PID is a number this process can probe at all. + * + * Backed by `/etc/machine-id`, which is *stable across reboots* on + * purpose. `/proc/sys/kernel/random/boot_id` would additionally invalidate + * every pre-reboot record, but it would also make a record written before + * the last reboot permanently unattributable — unsweepable, and (for + * writers that refuse to overwrite another origin's record) able to block + * registration at that PID forever. Reboot-recycled PIDs are already the + * job of {@link readProcStartToken}, whose token is boot-relative. + * + * Falls back to the hostname where no machine id file is readable, which + * covers every non-Linux platform. A hostname is weaker — it can change + * under a running session, leaving that session's own record + * unattributable to it — but the alternative, `null`, silently restores + * the cross-machine hole for exactly the platforms with no other + * discriminator. + */ +export function readMachineId(): string | null { + for (const file of MACHINE_ID_FILES) { + try { + const id = fs.readFileSync(file, 'utf8').trim(); + if (id !== '') return id; + } catch { + // Not this one; try the next source. + } + } + const hostname = os.hostname().trim(); + return hostname === '' ? null : hostname; +} + /** * True when `pid` is alive AND is the same process that recorded * `procStart`. diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index db135bfb42d..877ffb83a86 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -11,7 +11,7 @@ * the interactive UI bootstrap has flipped runtimeStatusEnabled on. */ -import { mkdtemp, readdir, rm } from 'node:fs/promises'; +import { mkdir, mkdtemp, readdir, realpath, rm } from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -267,6 +267,49 @@ describe('Config.startNewSession session-registry swap', () => { }); }); +describe('Config.relocateWorkingDirectory session-registry patch', () => { + const sessionA = 'aaaaaaaa-1111-2222-3333-aaaaaaaaaaaa'; + + it('repoints the record at the directory the session moved to', async () => { + const config = makeConfig(sessionA); + const target = path.join(tmpDir, 'project-b'); + await mkdir(target); + const expected = await realpath(target); + + // `/cd` refreshes the sidecar; the registry is the other half of the + // same claim, and a reader of `qwen sessions ps` sees only the + // registry. Left unpatched, `cwd` and the name derived from it + // advertise the directory the session left until the next session + // swap — possibly never. + await config.relocateWorkingDirectory(target, expected, { + skipProcessChdir: true, + skipArtifactMigration: true, + }); + + expect(patchCalls).toHaveLength(1); + expect(patchCalls[0]).toMatchObject({ cwd: expected }); + expect(patchCalls[0].name).toMatch(/^project-b-[0-9a-f]{2}$/); + }); + + it('patches the record even when this process never bootstrapped a sidecar', async () => { + // Same asymmetry as the swap above: refreshCurrentRuntimeStatus + // returns early when the sidecar was never claimed, and the registry + // patch must not inherit that gate. + const config = makeConfig(sessionA); + const target = path.join(tmpDir, 'project-c'); + await mkdir(target); + const expected = await realpath(target); + + await config.relocateWorkingDirectory(target, expected, { + skipProcessChdir: true, + skipArtifactMigration: true, + }); + + expect(patchCalls).toHaveLength(1); + expect(patchCalls[0]).toMatchObject({ cwd: expected }); + }); +}); + describe('Storage.getRuntimeStatusPath', () => { it('co-locates the sidecar under /chats/', () => { const storage = new Storage(tmpDir); From c1f78132d7ae0e7f314e03870af8b3fcb6e4b871 Mon Sep 17 00:00:00 2001 From: qqqys Date: Sun, 9 Aug 2026 16:28:15 +0800 Subject: [PATCH 07/17] fix(core): reject the uninitialized machine-id sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Critical R4-24 from review round 4. `readMachineId` rejected only the empty string, so a `/etc/machine-id` holding systemd's literal `uninitialized` — the state `machine-id(5)` reserves for a provisioned-but-uncommitted host, which is where every OSTree-style image (Fedora CoreOS, rpm-ostree) and every host between `systemd-firstboot` and `machine-id-setup --commit` sits — was returned as the machine identity. The file exists and is readable there, so the hostname fallback never fires and each such host reports the same id. Paired with the kernel-constant initial PID-namespace inode, two of them sharing a registry directory (an NFS home, a `QWEN_HOME` on a shared volume) pass each other's origin gate: one host's `qwen sessions ps` probes the other's PID locally, finds nothing, and unlinks a live session's record. Registration is startup-only, so that session stays invisible to discovery for the rest of its life. Secondary mode: once a host commits its real id, its own earlier records become foreign to it — never listed, never swept, and able to block re-registration at a recycled PID. The sentinel is now skipped like an unreadable source, so the lookup falls through to `/var/lib/dbus/machine-id` and then to the hostname. `readMachineId` had no direct coverage at all (review R4-7); it now has five cases, including both fall-through paths. Reverting the one-line guard turns exactly the two sentinel cases red. --- .../core/src/utils/process-liveness.test.ts | 65 ++++++++++++++++++- packages/core/src/utils/process-liveness.ts | 17 ++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 4953705ce6f..4272e42d4e0 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -4,12 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { spawn, type ChildProcess } from 'node:child_process'; import { readlinkSync } from 'node:fs'; +import { hostname } from 'node:os'; import { isPidAlive, isSameProcess, + readMachineId, readPidNamespaceId, readProcStartToken, } from './process-liveness.js'; @@ -23,12 +25,30 @@ const procReadFails = vi.hoisted(() => ({ value: false })); /** The same, for the `/proc/self/ns/pid` readlink. */ const nsReadFails = vi.hoisted(() => ({ value: false })); +/** + * Stands in for the machine-id sources, which cannot be arranged on the + * real filesystem: a path present here is served from the map (a string + * is the file's contents, `null` means ENOENT), anything absent falls + * through to the real `node:fs`. + */ +const machineIdFiles = vi.hoisted(() => new Map()); + vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, default: actual, readFileSync: ((...args: unknown[]) => { + if (typeof args[0] === 'string' && machineIdFiles.has(args[0])) { + const contents = machineIdFiles.get(args[0]); + if (contents === null) { + throw Object.assign( + new Error(`ENOENT: no such file, open '${args[0]}'`), + { code: 'ENOENT' }, + ); + } + return contents; + } if (procReadFails.value) { throw Object.assign(new Error('EACCES: /proc unreadable'), { code: 'EACCES', @@ -216,3 +236,46 @@ describe('readPidNamespaceId', () => { } }); }); + +describe('readMachineId', () => { + const ETC = '/etc/machine-id'; + const DBUS = '/var/lib/dbus/machine-id'; + + afterEach(() => { + machineIdFiles.clear(); + }); + + it('returns the committed id from /etc/machine-id', () => { + machineIdFiles.set(ETC, 'd2f0e4b1c3a54e6f8a9b0c1d2e3f4a5b\n'); + expect(readMachineId()).toBe('d2f0e4b1c3a54e6f8a9b0c1d2e3f4a5b'); + }); + + it("does not accept systemd's 'uninitialized' sentinel as an identity", () => { + // OSTree/CoreOS images and any host before `machine-id-setup --commit` + // ship this readable, non-empty file. Accepting it makes every such + // host report one machineId, so their origin gates open to each other + // and one host's sweep unlinks the other's live session records. + machineIdFiles.set(ETC, 'uninitialized\n'); + machineIdFiles.set(DBUS, null); + expect(readMachineId()).not.toBe('uninitialized'); + expect(readMachineId()).toBe(hostname().trim()); + }); + + it('falls through the sentinel to the dbus copy when that one is committed', () => { + machineIdFiles.set(ETC, 'uninitialized\n'); + machineIdFiles.set(DBUS, 'ab12cd34ef56ab78cd90ef12ab34cd56\n'); + expect(readMachineId()).toBe('ab12cd34ef56ab78cd90ef12ab34cd56'); + }); + + it('falls back to the hostname when no source is readable', () => { + machineIdFiles.set(ETC, null); + machineIdFiles.set(DBUS, null); + expect(readMachineId()).toBe(hostname().trim()); + }); + + it('treats an empty file the same as a missing one', () => { + machineIdFiles.set(ETC, '\n'); + machineIdFiles.set(DBUS, null); + expect(readMachineId()).toBe(hostname().trim()); + }); +}); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index b371c7768e7..f9008816348 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -114,6 +114,21 @@ export function readPidNamespaceId(): string | null { */ const MACHINE_ID_FILES = ['/etc/machine-id', '/var/lib/dbus/machine-id']; +/** + * The literal systemd writes into `/etc/machine-id` when the file is + * provisioned but no id has been committed yet — `machine-id(5)` reserves + * it for exactly that state, and OSTree-style images (Fedora CoreOS, + * rpm-ostree) plus any host between `systemd-firstboot` and + * `machine-id-setup --commit` sit in it. + * + * It has to be rejected explicitly rather than left to the empty-string + * check: the file *exists* and is readable, so nothing else would fall + * through to the next source. Treating it as an identity would hand every + * such host the same `machineId`, which is precisely the "one machine" + * verdict {@link readMachineId} exists to withhold. + */ +const UNINITIALIZED_MACHINE_ID = 'uninitialized'; + /** * An opaque identifier for the machine this process is running on, or * `null` when none could be read. @@ -144,7 +159,7 @@ export function readMachineId(): string | null { for (const file of MACHINE_ID_FILES) { try { const id = fs.readFileSync(file, 'utf8').trim(); - if (id !== '') return id; + if (id !== '' && id !== UNINITIALIZED_MACHINE_ID) return id; } catch { // Not this one; try the next source. } From 421774f44076006296df0150f5dbcd46e97b0697 Mon Sep 17 00:00:00 2001 From: qqqys Date: Sun, 9 Aug 2026 22:31:11 +0800 Subject: [PATCH 08/17] fix(core): stop noFollow writes and unprovable records from being trusted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review round 5 on #8728. R5-2 (Critical): atomicWriteFile's ownership-preservation fallback ran before any rename and stat'd the target through a planted symlink, so `fs.writeFile` + `chmod` landed the payload and the forced 0600 on the link's target — the clobber `noFollow` exists to prevent, and the one the EXDEV fallback already guards with unlink + O_EXCL. Under `noFollow` the target is now `lstat`ed and a symlink is discarded outright, so it is neither a mode-preservation source nor an ownership-fallback trigger and the replacing rename takes it. Applied the same lstat to the sync twin, which has no ownership fallback but would still copy a link target's mode onto the replacement. R5-7 (Critical): a same-origin record with no start token degraded `isSameProcess` to a bare liveness check, so any locally-live PID could be dressed in attacker-chosen sessionId/cwd/name — the origin fields needed to pass the gate are plaintext in every sibling record. Where a token is readable this build always writes one, so such a record is now withheld from callers. It is not swept: the PID is live and the writer may be a future version, and registration is startup-only, so a wrong unlink hides a session for good. Dead-PID records still sweep exactly as before. R5-7 also flagged the collision refusal as silent. Nothing removes an ownerless foreign record and registration happens once, so the session — and every later one drawing that PID — is missing from `qwen sessions ps` indefinitely. registerSession now reports it through an optional `onOriginConflict` callback (a bare `false` cannot be told apart from a transient I/O error) and the CLI turns it into a startup warning. R5-1: reject the all-zero machine id, which `machine-id(5)` reserves as invalid — the legacy form of the uncommitted state the `uninitialized` sentinel already covers, and the same cross-machine sweep hole. R5-12: the reboot-recycled-PID docstring claimed a boot-relative token bounds cross-reboot collisions; boot-relativeness is what allows them. Corrected to name what actually bounds them. R5-9 / R5-10: cover the origin guard's overwrite branch (a same-origin record left at this PID by an unclean exit — the blanket-refuse mutation the review measured as undetected now fails), and pin the "leave what this code did not write" contract for unparseable and future-schema records in unregisterSession and patchSessionRecord. Verification: packages/core session-registry (44), process-liveness (22), atomicFileWrite (84) and the five other noFollow consumers (162) pass; packages/cli gemini.test.tsx (72) passes; tsc --noEmit clean for both packages; core builds; eslint clean on all changed files. --- packages/cli/src/gemini.test.tsx | 30 +++ packages/cli/src/ui/startInteractiveUI.tsx | 14 ++ .../src/services/session-registry.test.ts | 184 ++++++++++++++++++ .../core/src/services/session-registry.ts | 34 ++++ .../core/src/utils/atomicFileWrite.test.ts | 64 ++++++ packages/core/src/utils/atomicFileWrite.ts | 25 ++- .../core/src/utils/process-liveness.test.ts | 15 ++ packages/core/src/utils/process-liveness.ts | 42 +++- 8 files changed, 402 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 5bb2a601b6d..79e499df548 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -3147,6 +3147,7 @@ describe('startInteractiveUI', () => { cwd: '/root', kind: 'interactive', qwenVersion: '1.0.0', + onOriginConflict: expect.any(Function), }); // Presence is what the registry sells, so the record must not @@ -3177,5 +3178,34 @@ describe('startInteractiveUI', () => { await runRegisteredCleanups(); expect(unregisterSession).not.toHaveBeenCalled(); }); + + it('warns at startup when another origin holds this PID', async () => { + const { registerSession } = await import('@qwen-code/qwen-code-core'); + // Not the shared fixture: this one gets written to. + const startupWarnings: string[] = []; + vi.mocked(registerSession).mockImplementationOnce(async (fields) => { + fields.onOriginConflict?.({ + pid: 4242, + filePath: '/home/u/.qwen/sessions/4242.json', + }); + return false; + }); + + await startInteractiveUI( + mockConfig, + mockSettings, + startupWarnings, + mockWorkspaceRoot, + mockInitializationResult, + ); + + // Every other registration failure is transient and stays quiet. + // This one never resolves itself — nothing sweeps the record in the + // way and registration only happens at startup — so the session is + // missing from `qwen sessions ps` for good unless someone is told. + expect(startupWarnings).toHaveLength(1); + expect(startupWarnings[0]).toContain('/home/u/.qwen/sessions/4242.json'); + expect(startupWarnings[0]).toContain('qwen sessions ps'); + }); }); }); diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index 2772f74ed87..41690c13b24 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -111,12 +111,26 @@ export async function startInteractiveUI( // registerSession swallows its own I/O errors, so a failure here is // silent by design: discovery is a convenience, not a precondition for // running Qwen Code. + // + // An origin conflict is the exception. It is not transient — nothing + // ever removes the record standing in the way, and registration only + // happens at startup — so this session, and every later one landing on + // the same PID, stays missing from `qwen sessions ps` for good. Silent + // is the wrong failure mode for a blackout that never lifts. if ( await registerSession({ sessionId: config.getSessionId(), cwd: config.getTargetDir(), kind: 'interactive', qwenVersion: version, + onOriginConflict: ({ filePath }) => { + startupWarnings.push( + `This session will not appear in \`qwen sessions ps\`: ${filePath} ` + + `holds a session record from another machine or PID namespace, ` + + `which Qwen Code will not overwrite. Remove that file if the ` + + `session it describes is gone.`, + ); + }, }) ) { registerCleanup(() => unregisterSession()); diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index bc707a93e44..06d2ebce310 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -223,6 +223,112 @@ describe('registerSession', () => { }); }); + it('overwrites a same-origin record left at its own pid', async () => { + // The origin guard's other branch, and the only recovery path there + // is: a predecessor that died without unlinking leaves its record at + // this PID, registration happens once at startup, and the sole sweep + // trigger is `qwen sessions ps`. Widening the guard to refuse on any + // existing record leaves the rest of this suite green while making + // every session on a recycled PID invisible for its whole life. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: '1', + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + sessionId: 's-predecessor', + cwd: '/w/before', + name: 'before-aa', + kind: 'interactive', + startedAt: 1000, + }); + + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + }), + ).toBe(true); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-ours', + cwd: '/w/ours', + }); + }); + + it('reports an origin conflict to the caller', async () => { + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + + // Nothing removes an ownerless foreign record and registration is + // startup-only, so this blackout never lifts on its own — a bare + // `false`, indistinguishable from a transient I/O failure, is not + // enough for the caller to say so. + const conflicts: Array<{ pid: number; filePath: string }> = []; + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + onOriginConflict: (info) => conflicts.push(info), + }), + ).toBe(false); + + expect(conflicts).toEqual([{ pid: process.pid, filePath }]); + }); + + it('does not report a conflict when registration succeeds', async () => { + const onOriginConflict = vi.fn(); + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + onOriginConflict, + }), + ).toBe(true); + expect(onOriginConflict).not.toHaveBeenCalled(); + }); + + it('still reports failure when the conflict callback throws', async () => { + await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + + // Reporting is a courtesy; it must not escalate a discovery miss into + // a failed startup. + await expect( + registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + onOriginConflict: () => { + throw new Error('reporting blew up'); + }, + }), + ).resolves.toBe(false); + }); + // Symlinks are not creatable without elevation on stock Windows. it.skipIf(process.platform === 'win32')( 'does not write through a symlink planted at its record path', @@ -360,6 +466,29 @@ describe('patchSessionRecord', () => { cwd: '/w/theirs', }); }); + + it.each([ + ['unparseable', 'not json at all'], + [ + 'a future schema version', + { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + pid: process.pid, + sessionId: 's-future', + cwd: '/w/future', + }, + ], + ])('leaves %s record at its own pid untouched', async (_label, body) => { + const filePath = await writeRaw(`${process.pid}.json`, body); + const before = await fs.readFile(filePath, 'utf8'); + + // `readRecord` returns null for both, and a resurrect-on-null patch + // would overwrite a record this code did not write — a newer build's, + // once the schema bumps. + await patchSessionRecord({ sessionId: 'ours', cwd: '/w/ours' }); + + expect(await fs.readFile(filePath, 'utf8')).toBe(before); + }); }); describe('unregisterSession', () => { @@ -397,6 +526,30 @@ describe('unregisterSession', () => { await expect(fs.stat(filePath)).resolves.toBeDefined(); }); + + it.each([ + ['unparseable', 'not json at all'], + [ + 'a future schema version', + { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + pid: process.pid, + sessionId: 's-future', + cwd: '/w/future', + }, + ], + ])('leaves %s record at its own pid in place', async (_label, body) => { + const filePath = await writeRaw(`${process.pid}.json`, body); + const before = await fs.readFile(filePath, 'utf8'); + + // "Not a record this code wrote, so not this code's to delete." An + // exit-time cleanup that unlinked whenever `readRecord` returned null + // would swallow its own ENOENT and pass every other test here, while + // deleting a newer build's record at a recycled PID. + await unregisterSession(); + + await expect(fs.readFile(filePath, 'utf8')).resolves.toBe(before); + }); }); describe('listLiveSessions', () => { @@ -469,6 +622,33 @@ describe('listLiveSessions', () => { expect(await listLiveSessions({ selfPid: DEAD_PID })).toEqual([]); }); + it('neither lists nor sweeps a same-origin record with no start token', async () => { + // Where a token is readable this build always records one, so a + // same-origin record without one did not come from this code. Trusting + // it collapses the check to bare liveness, and every field a reader + // shows — sessionId, cwd, name — is then whoever wrote the file's to + // choose; the origin fields it has to match are plaintext in every + // sibling record. + if (process.platform !== 'linux') return; + const filePath = await writeRaw(`${process.ppid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.ppid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + sessionId: 's-forged', + cwd: '/w/forged', + name: 'forged-aa', + kind: 'interactive', + startedAt: Date.now(), + }); + + expect(await listLiveSessions()).toEqual([]); + // Not swept: it may be a future version's record, and registration is + // startup-only, so a wrong unlink hides that session permanently. + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + it('ignores files that are not .json', async () => { await writeRaw('2026-planning-notes.json', { hello: 'world' }); await writeRaw('notes.txt', 'nope'); @@ -694,6 +874,10 @@ describe('listLiveSessions', () => { await writeRaw(`${process.ppid}.json`, { schemaVersion: 1, pid: process.ppid, + // A real token: a live record without one is unprovable and is + // withheld from callers, which would empty this list before it + // could be sorted. + procStart: readProcStartToken(process.ppid), pidNamespace: readPidNamespaceId(), sessionId: 's-parent', cwd: '/w/other', diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 9caaa518393..2151d46a60e 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -67,6 +67,7 @@ import { readMachineId, readPidNamespaceId, readProcStartToken, + supportsProcStartToken, } from '../utils/process-liveness.js'; const debugLogger = createDebugLogger('SESSION_REGISTRY'); @@ -138,6 +139,20 @@ export interface RegisterSessionFields { pid?: number; /** Overrides the derived name. */ name?: string; + /** + * Called when registration is refused because `.json` already + * holds another origin's record. + * + * A plain `false` cannot carry this: every other failure is an I/O + * error that is silent by design, whereas this one is indefinite. + * Nothing sweeps an ownerless foreign record — sweep, unregister and + * patch all skip foreign origins — and registration is startup-only, so + * this session stays invisible to discovery for its entire lifetime, + * and so does every later session that draws the same PID. Core has no + * business picking a presentation channel, so the caller is handed the + * fact and decides. + */ + onOriginConflict?: (info: { pid: number; filePath: string }) => void; } export function getSessionRegistryDir(): string { @@ -242,6 +257,13 @@ export async function registerSession( debugLogger.debug( `registerSession skipped: ${filePath} holds a record from another origin`, ); + try { + fields.onOriginConflict?.({ pid, filePath }); + } catch (error) { + // A reporting callback must not turn a discovery miss into a + // failed startup; registration is already best-effort. + debugLogger.debug(`onOriginConflict threw: ${describe(error)}`); + } return false; } @@ -362,6 +384,7 @@ export async function listLiveSessions( // change PID namespace or machine under itself, and both are syscalls. const selfNamespace = readPidNamespaceId(); const selfMachine = readMachineId(); + const tokensAvailable = supportsProcStartToken(); const live: SessionRegistryRecord[] = []; await Promise.all( @@ -404,6 +427,17 @@ export async function listLiveSessions( } if (isSameProcess(record.pid, record.procStart)) { + // Alive — but on a platform that has start tokens, a record + // without one was not written by this build, which always + // records one. `isSameProcess` has just degraded to a bare + // liveness check, so all this record proves is that *some* + // process holds that PID; the session it describes — + // sessionId, cwd, name — is whoever wrote the file's to choose, + // and the origin fields needed to get this far are plaintext in + // every sibling record. Withhold it from callers, but do not + // sweep it: the PID is live, and it may equally be a future + // version's record, which an unlink would erase for good. + if (tokensAvailable && record.procStart == null) return; live.push(record); return; } diff --git a/packages/core/src/utils/atomicFileWrite.test.ts b/packages/core/src/utils/atomicFileWrite.test.ts index c073b0a15a1..a1c52962a62 100644 --- a/packages/core/src/utils/atomicFileWrite.test.ts +++ b/packages/core/src/utils/atomicFileWrite.test.ts @@ -395,6 +395,70 @@ describe('atomicWriteFile', () => { }, ); + it.skipIf( + process.platform === 'win32' || typeof process.geteuid !== 'function', + )( + 'should not let the ownership fallback write through a symlink under noFollow', + async () => { + // The ownership-preservation fallback runs before any rename, so it + // is reached even under noFollow. If the target stat followed the + // link, the fallback's writeFile+chmod would land the payload and + // the forced mode on the victim — defeating noFollow exactly as the + // naive EXDEV fallback once did. The victim's uid is what makes the + // existing same-uid symlink test blind to this branch. + const victim = path.join(tmpDir, 'victim.txt'); + const planted = path.join(tmpDir, 'planted.json'); + await fs.writeFile(victim, 'PRECIOUS', { mode: 0o644 }); + await fs.symlink(victim, planted); + + const realGeteuid = process.geteuid!; + const victimStat = await fs.stat(victim); + const inoBefore = victimStat.ino; + process.geteuid = () => victimStat.uid + 1; + + try { + await atomicWriteFile(planted, 'payload', { + mode: 0o600, + forceMode: true, + noFollow: true, + }); + } finally { + process.geteuid = realGeteuid; + } + + // The victim is untouched — same content, same mode, same inode. + expect(await fs.readFile(victim, 'utf-8')).toBe('PRECIOUS'); + expect((await fs.stat(victim)).mode & 0o7777).toBe(0o644); + expect((await fs.stat(victim)).ino).toBe(inoBefore); + + // The symlink was replaced by a regular file holding the payload. + expect((await fs.lstat(planted)).isSymbolicLink()).toBe(false); + expect(await fs.readFile(planted, 'utf-8')).toBe('payload'); + expect((await fs.stat(planted)).mode & 0o7777).toBe(0o600); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'should not copy a symlink mode onto the replacement under noFollow (sync)', + () => { + // atomicWriteFileSync has no ownership fallback, so a link is only ever + // a mode-preservation source here — but statSync would read it through + // the link and stamp the target's mode onto the replacement file. + const victim = path.join(tmpDir, 'victim-sync.txt'); + const planted = path.join(tmpDir, 'planted-sync.json'); + fsSync.writeFileSync(victim, 'PRECIOUS', { mode: 0o666 }); + fsSync.symlinkSync(victim, planted); + + atomicWriteFileSync(planted, 'payload', { mode: 0o600, noFollow: true }); + + // Victim untouched; the replacement took the requested mode, not 0666. + expect(fsSync.readFileSync(victim, 'utf-8')).toBe('PRECIOUS'); + expect(fsSync.lstatSync(planted).isSymbolicLink()).toBe(false); + expect(fsSync.readFileSync(planted, 'utf-8')).toBe('payload'); + expect(fsSync.statSync(planted).mode & 0o7777).toBe(0o600); + }, + ); + it.skipIf( process.platform === 'win32' || typeof process.geteuid !== 'function' || diff --git a/packages/core/src/utils/atomicFileWrite.ts b/packages/core/src/utils/atomicFileWrite.ts index b6961cca6fb..b05f5dc0468 100644 --- a/packages/core/src/utils/atomicFileWrite.ts +++ b/packages/core/src/utils/atomicFileWrite.ts @@ -183,14 +183,27 @@ export async function atomicWriteFile( // Stat the target to preserve existing permissions and detect // ownership-changing renames (see the ownership-preservation note in // the function doc). + // + // Under noFollow this must not traverse the final component: `fs.stat` + // follows a planted symlink, and the ownership-preservation fallback + // below would then writeFile+chmod straight through it, landing the + // payload and the forced mode on the link's target — the exact clobber + // noFollow exists to prevent, and the one the EXDEV fallback already + // guards with unlink + O_EXCL. `lstat` describes the link itself; a link + // is then discarded outright so it is neither a mode-preservation source + // (a symlink's 0777 would be copied onto the replacement) nor an + // ownership-fallback trigger, leaving the replacing rename to take it. let existingStat: Stats | undefined; try { - existingStat = await fs.stat(targetPath); + existingStat = options?.noFollow + ? await fs.lstat(targetPath) + : await fs.stat(targetPath); } catch (err) { if (!isNodeError(err) || err.code !== 'ENOENT') { throw err; } } + if (existingStat?.isSymbolicLink()) existingStat = undefined; // forceMode skips permission preservation only when an explicit mode is // supplied — otherwise we'd silently downgrade an existing file's perms @@ -536,8 +549,14 @@ export function atomicWriteFileSync( let existingMode: number | undefined; if (!options?.forceMode || options?.mode === undefined) { try { - const stat = fsSync.statSync(targetPath); - existingMode = stat.mode & 0o7777; + // lstat under noFollow for the same reason as atomicWriteFile: statSync + // follows a planted symlink, which would copy the link target's mode + // onto the replacement file. This path has no ownership fallback, so a + // link is only a mode-preservation source here, never a write target. + const stat = options?.noFollow + ? fsSync.lstatSync(targetPath) + : fsSync.statSync(targetPath); + if (!stat.isSymbolicLink()) existingMode = stat.mode & 0o7777; } catch (err) { if (!isNodeError(err) || err.code !== 'ENOENT') { throw err; diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 4272e42d4e0..1bc63263276 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -267,6 +267,21 @@ describe('readMachineId', () => { expect(readMachineId()).toBe('ab12cd34ef56ab78cd90ef12ab34cd56'); }); + it('does not accept the all-zero id as an identity', () => { + // `machine-id(5)`: "This ID may not be all zeros." It is the legacy, + // pre-sentinel form of the same uncommitted state — same consequence, + // one machineId shared by every host in it. + machineIdFiles.set(ETC, `${'0'.repeat(32)}\n`); + machineIdFiles.set(DBUS, null); + expect(readMachineId()).toBe(hostname().trim()); + }); + + it('falls through the all-zero id to the dbus copy when that one is committed', () => { + machineIdFiles.set(ETC, `${'0'.repeat(32)}\n`); + machineIdFiles.set(DBUS, 'ab12cd34ef56ab78cd90ef12ab34cd56\n'); + expect(readMachineId()).toBe('ab12cd34ef56ab78cd90ef12ab34cd56'); + }); + it('falls back to the hostname when no source is readable', () => { machineIdFiles.set(ETC, null); machineIdFiles.set(DBUS, null); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index f9008816348..29278d0b26f 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -129,6 +129,17 @@ const MACHINE_ID_FILES = ['/etc/machine-id', '/var/lib/dbus/machine-id']; */ const UNINITIALIZED_MACHINE_ID = 'uninitialized'; +/** + * The all-zero id, which `machine-id(5)` reserves as invalid ("This ID may + * not be all zeros"). It is the same "no machine id" state as the empty + * file and the {@link UNINITIALIZED_MACHINE_ID} sentinel — the legacy, + * pre-sentinel convention for template and OSTree-style images — and needs + * rejecting for the same reason: the file exists and reads cleanly, so + * nothing else falls through to the next source, and every host in this + * state would otherwise agree on one `machineId`. + */ +const ALL_ZERO_MACHINE_ID = '0'.repeat(32); + /** * An opaque identifier for the machine this process is running on, or * `null` when none could be read. @@ -145,8 +156,11 @@ const UNINITIALIZED_MACHINE_ID = 'uninitialized'; * every pre-reboot record, but it would also make a record written before * the last reboot permanently unattributable — unsweepable, and (for * writers that refuse to overwrite another origin's record) able to block - * registration at that PID forever. Reboot-recycled PIDs are already the - * job of {@link readProcStartToken}, whose token is boot-relative. + * registration at that PID forever. Reboot-recycled PIDs are bounded by + * {@link isPidAlive} sweeping dead PIDs and by same-origin re-registration + * overwriting the stale record; {@link readProcStartToken}'s token is + * boot-relative, which is what leaves a narrow same-tick collision window + * across a reboot rather than what closes it. * * Falls back to the hostname where no machine id file is readable, which * covers every non-Linux platform. A hostname is weaker — it can change @@ -159,7 +173,13 @@ export function readMachineId(): string | null { for (const file of MACHINE_ID_FILES) { try { const id = fs.readFileSync(file, 'utf8').trim(); - if (id !== '' && id !== UNINITIALIZED_MACHINE_ID) return id; + if ( + id !== '' && + id !== UNINITIALIZED_MACHINE_ID && + id !== ALL_ZERO_MACHINE_ID + ) { + return id; + } } catch { // Not this one; try the next source. } @@ -168,6 +188,22 @@ export function readMachineId(): string | null { return hostname === '' ? null : hostname; } +/** + * True when this platform can produce start tokens at all, probed against + * the calling process — the one PID guaranteed to exist and to be ours. + * + * This is what turns a missing `procStart` from "written by a platform + * that has no token" into "written by something that is not this build": + * where a token is available, every record this code writes carries one, + * so a same-origin record without one is unprovable rather than merely + * unproven. Callers use it to withhold trust, never to delete — the + * writer might be a future version, and a wrong unlink hides a live + * session permanently (registration is startup-only). + */ +export function supportsProcStartToken(): boolean { + return readProcStartToken(process.pid) !== null; +} + /** * True when `pid` is alive AND is the same process that recorded * `procStart`. From 2423dcf94e849413fcc39e42e0c18c11e0bd6433 Mon Sep 17 00:00:00 2001 From: qqqys Date: Mon, 10 Aug 2026 03:49:42 +0800 Subject: [PATCH 09/17] fix(core): bind registry writes to the entry they validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the eight Criticals from review 4892317103. R1-4 / R1-5 — validation was not bound to the directory entry it authorized. `readRecord` proved things about an inode; every mutation that followed named a *path*, and in a directory a sandboxed co-tenant can write to the two stop agreeing the moment the read returns. Reads now go through one `O_NOFOLLOW` handle and carry the entry's dev/ino back with the bytes, and each mutation re-checks that identity at its commit step (`assertCanCommit` for the writes, as late as Node allows for the unlink — there is no `unlinkat`-by-inode). Registration onto a free name is no longer read-then-rename, which let two origins sharing `QWEN_HOME` and a PID number both see the gap: it writes a temp file and publishes it with `link(2)`, which fails `EEXIST` rather than replacing, and falls back to the old path only where hard links are unsupported. Replacing an unusable entry (a planted symlink, a truncated write) stays deliberate. R1-8 — an unreadable `/proc/self/ns/pid` collapsed to `null`, the same value as "this platform has no namespaces". Two containers behind a `hidepid` mount sharing a machine id then matched as one origin. Added `PID_NAMESPACE_UNREADABLE`, which never compares equal, not even to itself. R1-3 — `Config.refreshSessionId` queues its patch and returns without awaiting it, so a `/clear` before quit could be mid-flight when exit cleanup unlinked the record, and the patch's write would put a dead PID back on the register. Registry writes now run on one queue and `unregisterSession` retires the PID, so a patch on either side of it is dropped. R1-6 — enumeration started a read per `readdir` entry in one tick from attacker-chosen filenames. Bounded to 512 candidates per pass at 16 in flight, with the truncation logged. R1-7 — under `noFollow` the ownership-preservation fallback still committed by pathname, so a symlink swapped in after the `lstat` took the payload and the forced 0600. It now opens the validated target `O_WRONLY|O_TRUNC|O_NOFOLLOW` and writes, fsyncs and fchmods through the handle. Shared with the EXDEV fallback, which already worked this way. R1-1 — `--all` was unreachable: `qwen sessions ps` runs and exits inside yargs' argument parsing, long before `startInteractiveUI` registers anything, so there was never a self record to include and both settings printed the same thing. Removed, and a test now drives the real `builder` so it cannot come back without a registration behind it. R1-2 — `gemini.tsx` flushes `startupWarnings` to stderr before it calls `startInteractiveUI`, so the origin-conflict warning appended there reached only the TUI notification area, which onboarding can cover. It is now written to stderr as well. R1-9 / R1-10 — two assertions failed for environmental rather than code reasons: a numeric `procStart` was required where a restricted `/proc` returns null by design, and a fixture created with `writeFile(mode: 0644)` lands at 0600 under umask 077. Both now assert against what the environment actually provides. Verified: packages/core build and tsc --noEmit clean, packages/cli tsc --noEmit clean, eslint clean on all nine files. 670 core tests pass (session-registry 53, atomicFileWrite 85, process-liveness 23, runtimeStatus.config 10, config 500) and 229 cli tests (ps, sessions, gemini, AppContainer). Every new test was mutation-checked against a build with its fix reverted; where two guards overlap, against a build with both reverted. --- packages/cli/src/commands/sessions/ps.test.ts | 43 +- packages/cli/src/commands/sessions/ps.ts | 25 +- packages/cli/src/ui/startInteractiveUI.tsx | 17 +- .../src/services/session-registry.test.ts | 307 ++++++++- .../core/src/services/session-registry.ts | 614 ++++++++++++++---- .../core/src/utils/atomicFileWrite.test.ts | 66 ++ packages/core/src/utils/atomicFileWrite.ts | 84 ++- .../core/src/utils/process-liveness.test.ts | 27 +- packages/core/src/utils/process-liveness.ts | 37 +- 9 files changed, 1009 insertions(+), 211 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index 11bbb27fc29..7268a2eebb5 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -74,7 +74,7 @@ describe('formatAge', () => { describe('qwen sessions ps', () => { it('prints a table of live sessions', async () => { listLiveSessions.mockResolvedValue([record()]); - await run({ json: false, all: false }); + await run({ json: false }); expect(stdout[0]).toMatch(/^NAME\s+PID\s+AGE\s+DIRECTORY$/); expect(stdout[1]).toContain('app-ab'); @@ -84,13 +84,13 @@ describe('qwen sessions ps', () => { it('says so plainly when nothing else is running', async () => { listLiveSessions.mockResolvedValue([]); - await run({ json: false, all: false }); + await run({ json: false }); expect(stdout).toEqual(['No other Qwen Code sessions are running.']); }); it('emits one JSON object per line with no header', async () => { listLiveSessions.mockResolvedValue([record(), record({ pid: 7 })]); - await run({ json: true, all: false }); + await run({ json: true }); expect(stdout).toHaveLength(2); expect(JSON.parse(stdout[0]).pid).toBe(4242); @@ -99,25 +99,42 @@ describe('qwen sessions ps', () => { it('prints nothing on stdout for an empty JSON listing', async () => { listLiveSessions.mockResolvedValue([]); - await run({ json: true, all: false }); + await run({ json: true }); expect(stdout).toEqual([]); }); - it('excludes this process unless --all is passed', async () => { + it('asks for the default listing, with no self-inclusion switch', async () => { listLiveSessions.mockResolvedValue([]); - await run({ json: true, all: false }); - expect(listLiveSessions).toHaveBeenLastCalledWith({ includeSelf: false }); + await run({ json: true }); + expect(listLiveSessions).toHaveBeenLastCalledWith(); + }); - await run({ json: true, all: true }); - expect(listLiveSessions).toHaveBeenLastCalledWith({ includeSelf: true }); + it('exposes no flag that claims to include this process', () => { + // `qwen sessions ps` runs and exits inside yargs' argument parsing, so + // it never reaches `startInteractiveUI` and never registers itself. + // A `--all` toggling `includeSelf` therefore had nothing to include: + // both settings produced identical output. Pinned here so it cannot + // come back without a registration to go with it. + const options: Record = {}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const yargsStub: any = { + option(name: string, config: unknown) { + options[name] = config; + return yargsStub; + }, + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (psCommand.builder as any)(yargsStub); + + expect(Object.keys(options)).toEqual(['json']); }); it('neutralizes control sequences coming from another process record', async () => { listLiveSessions.mockResolvedValue([ record({ name: 'ev\x1b[31mil\r', cwd: '/w/a\nb' }), ]); - await run({ json: false, all: false }); + await run({ json: false }); const row = stdout[1]; expect(row).not.toContain('\x1b'); @@ -127,7 +144,7 @@ describe('qwen sessions ps', () => { it('truncates an over-long name instead of breaking the columns', async () => { listLiveSessions.mockResolvedValue([record({ name: 'x'.repeat(80) })]); - await run({ json: false, all: false }); + await run({ json: false }); expect(stdout[1]).toContain('...'); expect(stdout[1]).toContain('4242'); }); @@ -140,7 +157,7 @@ describe('qwen sessions ps', () => { // Asserting the cell exactly is what pins the accumulation loop — // "contains ..." survives a loop that copies nothing at all. listLiveSessions.mockResolvedValue([record({ name: '中'.repeat(15) })]); - await run({ json: false, all: false }); + await run({ json: false }); const cell = '中'.repeat(8) + '...'; expect(stdout[1]).toBe( @@ -154,7 +171,7 @@ describe('qwen sessions ps', () => { .spyOn(process, 'exit') .mockImplementation((() => undefined) as never); - await run({ json: false, all: false }); + await run({ json: false }); expect(stderr).toEqual([ 'Error: failed to read the session registry: registry on fire', diff --git a/packages/cli/src/commands/sessions/ps.ts b/packages/cli/src/commands/sessions/ps.ts index 2d34ebc9b78..013445de6ce 100644 --- a/packages/cli/src/commands/sessions/ps.ts +++ b/packages/cli/src/commands/sessions/ps.ts @@ -28,7 +28,6 @@ export const AGE_COL = 10; interface PsArgs { json?: boolean; - all?: boolean; } /** @@ -103,7 +102,13 @@ function outputHuman(records: SessionRegistryRecord[], now: number): void { async function handlePs(argv: PsArgs): Promise { let records: SessionRegistryRecord[]; try { - records = await listLiveSessions({ includeSelf: argv.all ?? false }); + // No `includeSelf`: this process is not a session and never registers + // one. `qwen sessions ps` is resolved and run during yargs' argument + // parsing, which finishes and exits long before `startInteractiveUI` + // — the only caller of `registerSession` — would run. So there is no + // record at this PID to include, and a flag offering to include it + // would be a switch with nothing on the other end. + records = await listLiveSessions(); } catch (err) { writeStderrLine( `Error: failed to read the session registry: ${ @@ -135,17 +140,11 @@ export const psCommand: CommandModule = { command: 'ps', describe: 'List Qwen Code sessions running right now', builder: (yargs: Argv) => - yargs - .option('json', { - type: 'boolean', - describe: 'Output as JSON Lines', - default: false, - }) - .option('all', { - type: 'boolean', - describe: 'Include this process, if it is itself a registered session', - default: false, - }), + yargs.option('json', { + type: 'boolean', + describe: 'Output as JSON Lines', + default: false, + }), handler: async (argv) => { await handlePs(argv); }, diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index 41690c13b24..012f77894bb 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -124,12 +124,19 @@ export async function startInteractiveUI( kind: 'interactive', qwenVersion: version, onOriginConflict: ({ filePath }) => { - startupWarnings.push( + const warning = `This session will not appear in \`qwen sessions ps\`: ${filePath} ` + - `holds a session record from another machine or PID namespace, ` + - `which Qwen Code will not overwrite. Remove that file if the ` + - `session it describes is gone.`, - ); + `holds a session record from another machine or PID namespace, ` + + `which Qwen Code will not overwrite. Remove that file if the ` + + `session it describes is gone.`; + startupWarnings.push(warning); + // The array alone is not enough. `gemini.tsx` flushes + // startupWarnings to stderr *before* it calls into this module, so + // anything appended here reaches only the TUI's notification area — + // which the onboarding flow can cover, and which scrolls away. + // That is the wrong channel for a blackout that never lifts, so + // emit it on the durable one too. + writeStderrLine(warning); }, }) ) { diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 06d2ebce310..e17b45bce9e 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -19,11 +19,31 @@ import { SESSION_REGISTRY_SCHEMA_VERSION, } from './session-registry.js'; import { + PID_NAMESPACE_UNREADABLE, readMachineId, readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; +/** + * Lets a test put this process in the state the sentinel exists for: a + * platform that has PID namespaces, on which our own could not be read. + * Everything else passes through to the real implementation. + */ +const selfNamespaceUnreadable = vi.hoisted(() => ({ value: false })); + +vi.mock('../utils/process-liveness.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + readPidNamespaceId: () => + selfNamespaceUnreadable.value + ? actual.PID_NAMESPACE_UNREADABLE + : actual.readPidNamespaceId(), + }; +}); + vi.mock('../config/storage.js', () => { let mockDir = '/tmp/session-registry-test'; return { @@ -143,11 +163,17 @@ describe('registerSession', () => { sweepStale: false, }); expect(live).toHaveLength(1); - if (process.platform === 'linux') { - expect(live[0].procStart).toBe(readProcStartToken(process.pid)); + // The recorded token must always equal what this process reads for + // itself — that is the regression this guards. The *shape* assertion + // is separate and conditional: `readProcStartToken` returns null by + // design where /proc is absent or restricted (a hardened container, a + // hidepid mount), and requiring digits there would fail the suite for + // an environmental reason rather than a code one. + const selfToken = + process.platform === 'linux' ? readProcStartToken(process.pid) : null; + expect(live[0].procStart).toBe(selfToken); + if (selfToken !== null) { expect(live[0].procStart).toMatch(/^\d+$/); - } else { - expect(live[0].procStart).toBeNull(); } }); @@ -166,9 +192,16 @@ describe('registerSession', () => { selfPid: DEAD_PID, sweepStale: false, }); - expect(record.pidNamespace).toBe(readPidNamespaceId()); + const selfNamespace = readPidNamespaceId(); + expect(record.pidNamespace).toBe(selfNamespace); if (process.platform === 'linux') { - expect(record.pidNamespace).toMatch(/^\d+$/); + // Either a namespace inode or the explicit "could not read it" + // sentinel — never null, which would mean "this platform has no + // namespaces" and let two containers agree they are one origin. + expect(record.pidNamespace).not.toBeNull(); + if (selfNamespace !== PID_NAMESPACE_UNREADABLE) { + expect(record.pidNamespace).toMatch(/^\d+$/); + } } else { expect(record.pidNamespace).toBeNull(); } @@ -890,3 +923,265 @@ describe('listLiveSessions', () => { expect(live.map((r) => r.sessionId)).toEqual(['s-parent', 's-self']); }); }); + +// Symlinks are not creatable without elevation on stock Windows. +describe.skipIf(process.platform === 'win32')( + 'reading a record does not follow a symlink at its path', + () => { + /** + * A valid record body sitting *outside* the registry directory, so + * the only way to reach it is through the planted link. Inside the + * directory it would be a sweep candidate in its own right and the + * assertions below could not tell the two causes apart. + */ + async function plantVictimOutsideRegistry(pid: number): Promise { + const victim = path.join(tmpDir, 'victim-record.json'); + await fs.writeFile( + victim, + JSON.stringify({ + schemaVersion: 1, + pid, + // A real token, so a build that followed the link would find a + // record it considers live and report it. `null` would be + // withheld anyway and the assertion below could not tell the + // two reasons apart. + procStart: readProcStartToken(pid), + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + sessionId: 's-victim', + cwd: '/w/victim', + name: 'victim-aa', + kind: 'interactive', + startedAt: 1000, + }), + ); + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + return victim; + } + + it('does not list what the link points at', async () => { + // The write paths already refuse to follow a link here, so the read + // that authorizes them must refuse too. Otherwise the *target* is + // what gets validated while the *link* is what gets mutated, and a + // co-tenant who aims `.json` at a sibling's record gets that + // record acted on for them. + // + // The parent process is the one PID other than our own that is + // reliably alive, which is what makes "not listed" load-bearing + // rather than a restatement of the staleness sweep. + const victim = await plantVictimOutsideRegistry(process.ppid); + await fs.symlink(victim, getSessionRecordPath(process.ppid)); + + expect(await listLiveSessions({ sweepStale: true })).toEqual([]); + await expect(fs.stat(victim)).resolves.toBeDefined(); + }); + + it('does not patch or unlink through one at its own record path', async () => { + const victim = await plantVictimOutsideRegistry(DEAD_PID); + await fs.symlink(victim, getSessionRecordPath(DEAD_PID)); + const before = await fs.readFile(victim, 'utf8'); + + await patchSessionRecord({ sessionId: 'rewritten' }, DEAD_PID); + await unregisterSession(DEAD_PID); + + expect(await fs.readFile(victim, 'utf8')).toBe(before); + // The link itself is left alone too: it is not a record this code + // wrote, so it is not this code's to delete. + expect( + (await fs.lstat(getSessionRecordPath(DEAD_PID))).isSymbolicLink(), + ).toBe(true); + }); + }, +); + +describe('an unreadable PID namespace is not an origin', () => { + it('neither lists nor sweeps a record that could not name its namespace', async () => { + // `null` means "this platform has no PID namespaces", which is a claim + // two peers can share. The sentinel is the absence of a claim, and two + // containers behind a hidepid mount would otherwise match on it and + // read each other's PID numbers as their own. + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: '1', + pidNamespace: PID_NAMESPACE_UNREADABLE, + sessionId: 's-elsewhere', + cwd: '/w/elsewhere', + name: 'elsewhere-dd', + kind: 'interactive', + startedAt: 1000, + }); + + expect(await listLiveSessions({ sweepStale: true })).toEqual([]); + // Not listed *and* not swept. A dead PID plus a same-origin verdict + // would have deleted it; the file surviving is what shows the origin + // gate rejected it first. + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('does not match another record that could not name its namespace either', async () => { + // The case the sentinel exists for, and the only one plain inequality + // does not already cover: two containers behind a `hidepid` mount, + // sharing a machine id and a `QWEN_HOME`. Both write the sentinel, so + // a `null`-collapsing build reads them as one origin and lets a + // matching PID number list, overwrite, patch or sweep the other's + // record. + selfNamespaceUnreadable.value = true; + try { + const filePath = await writeRaw(`${DEAD_PID}.json`, { + schemaVersion: 1, + pid: DEAD_PID, + procStart: '1', + pidNamespace: PID_NAMESPACE_UNREADABLE, + sessionId: 's-other-container', + cwd: '/w/other', + name: 'other-ee', + kind: 'interactive', + startedAt: 1000, + }); + + expect(await listLiveSessions({ sweepStale: true })).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + + // The write paths keep their hands off it too. + await patchSessionRecord({ sessionId: 'mine' }, DEAD_PID); + await unregisterSession(DEAD_PID); + const onDisk = JSON.parse(await fs.readFile(filePath, 'utf8')); + expect(onDisk.sessionId).toBe('s-other-container'); + } finally { + selfNamespaceUnreadable.value = false; + } + }); + + it('refuses to overwrite one at its own PID', async () => { + await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: '1', + pidNamespace: PID_NAMESPACE_UNREADABLE, + sessionId: 's-elsewhere', + cwd: '/w/elsewhere', + name: 'elsewhere-dd', + kind: 'interactive', + startedAt: 1000, + }); + + const conflicts: number[] = []; + await expect( + registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + onOriginConflict: ({ pid }) => conflicts.push(pid), + }), + ).resolves.toBe(false); + expect(conflicts).toEqual([process.pid]); + + const onDisk = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ); + expect(onDisk.sessionId).toBe('s-elsewhere'); + }); +}); + +describe('ordering registry writes against withdrawal', () => { + it('drops a patch that was queued before the record was withdrawn', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + pid: DEAD_PID, + }); + + // `Config.refreshSessionId` queues its patch and returns without + // awaiting it, so this is the production shape: a `/clear` still in + // flight when exit cleanup runs. Both orders have to hold — the patch + // must not land in the middle of the unlink, and must not run after + // it either. + const patch = patchSessionRecord({ sessionId: 's2' }, DEAD_PID); + const withdraw = unregisterSession(DEAD_PID); + await Promise.all([patch, withdraw]); + + await expect(fs.stat(getSessionRecordPath(DEAD_PID))).rejects.toThrow( + /ENOENT/, + ); + }); + + it('drops a patch issued after the record was withdrawn', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + pid: DEAD_PID, + }); + await unregisterSession(DEAD_PID); + await patchSessionRecord({ sessionId: 's2' }, DEAD_PID); + + await expect(fs.stat(getSessionRecordPath(DEAD_PID))).rejects.toThrow( + /ENOENT/, + ); + }); + + it('reopens the register when the PID registers again', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + pid: DEAD_PID, + }); + await unregisterSession(DEAD_PID); + await registerSession({ + sessionId: 's2', + cwd: '/w/app', + kind: 'interactive', + pid: DEAD_PID, + }); + await patchSessionRecord({ cwd: '/w/moved' }, DEAD_PID); + + const onDisk = JSON.parse( + await fs.readFile(getSessionRecordPath(DEAD_PID), 'utf8'), + ); + expect(onDisk.cwd).toBe('/w/moved'); + }); +}); + +describe('bounding what one enumeration will do', () => { + it('caps the records it examines per pass', async () => { + // The filenames and the file count are both attacker-supplied here: a + // sandboxed co-tenant can create `.json` at will, and an + // unbounded fan-out would open a descriptor for every one of them in + // a single tick. The sweep is what makes the ceiling observable — + // exactly the records that were examined are the ones that go away. + const total = 600; + const cap = 512; + for (let i = 0; i < total; i++) { + const pid = DEAD_PID - i; + await writeRaw(`${pid}.json`, { + schemaVersion: 1, + pid, + procStart: '1', + pidNamespace: readPidNamespaceId(), + sessionId: `s-${i}`, + cwd: '/w/app', + name: `app-${i}`, + kind: 'interactive', + startedAt: 1000, + }); + } + + expect(await listLiveSessions({ sweepStale: true })).toEqual([]); + + const left = (await fs.readdir(getSessionRegistryDir())).filter((n) => + /^\d+\.json$/.test(n), + ); + expect(left).toHaveLength(total - cap); + + // Not a leak: the next pass takes the remainder. + await listLiveSessions({ sweepStale: true }); + expect( + (await fs.readdir(getSessionRegistryDir())).filter((n) => + /^\d+\.json$/.test(n), + ), + ).toEqual([]); + }); +}); diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 2151d46a60e..30d98253c8d 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -56,7 +56,8 @@ * origin-disambiguated keying, not a wider guard here. */ -import { createHash } from 'node:crypto'; +import { createHash, randomBytes } from 'node:crypto'; +import * as fsSync from 'node:fs'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import { Storage } from '../config/storage.js'; @@ -64,6 +65,7 @@ import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSameProcess, + PID_NAMESPACE_UNREADABLE, readMachineId, readPidNamespaceId, readProcStartToken, @@ -98,6 +100,42 @@ const MAX_RECORD_BYTES = 64 * 1024; */ const RECORD_FILENAME = /^\d+\.json$/; +/** + * How many candidate records one enumeration will look at, and how many of + * those reads may be in flight at once. + * + * Both filename and file count are attacker-supplied under this + * directory's own threat model: a sandboxed co-tenant can create + * `.json` at will, and an unbounded `Promise.all` over `readdir` + * would then open a descriptor and allocate a promise per entry in one + * tick. `qwen sessions ps` sits on an interactive path with no back + * pressure of its own, so the ceiling has to live here. A real machine + * runs single-digit sessions; 512 is far above any honest reading and far + * below the point where either resource matters. + */ +const MAX_RECORDS_PER_SCAN = 512; +const SCAN_CONCURRENCY = 16; + +/** + * `O_NOFOLLOW` does not exist on Windows, where `fs.constants` simply + * omits it; `| undefined` would poison the whole flag word into `NaN`. + * Zero is the correct degradation — Windows has no symlink-in-a-shared- + * home threat model here, and every other guard still applies. + */ +const O_NOFOLLOW = fsSync.constants.O_NOFOLLOW ?? 0; + +/** A directory entry's identity, as observed through an open handle. */ +interface EntryIdentity { + dev: number; + ino: number; +} + +/** A validated record together with the entry the bytes came from. */ +interface ReadRecord { + record: SessionRegistryRecord; + entry: EntryIdentity; +} + export type SessionKind = 'interactive' | 'headless'; /** One live session, as recorded on disk. */ @@ -195,17 +233,93 @@ export function deriveSessionName(cwd: string, sessionId: string): string { * (a platform that exposes neither) and stays on the original * trust-the-path behaviour, while a null on one side only means the * writer made no claim we can check. + * + * {@link PID_NAMESPACE_UNREADABLE} is the one value that never matches, + * not even itself. `null` is a claim — "this platform has no namespaces" — + * and two peers making it are genuinely in the same (non-existent) + * namespace. The sentinel is the absence of a claim on a platform that + * *does* have namespaces, so two sides carrying it have established + * nothing: two containers behind a `hidepid` mount, sharing a machine id + * and a `QWEN_HOME`, would otherwise read each other's PID numbers as + * their own. */ function isSameOrigin( record: Pick, selfMachine: string | null, selfNamespace: string | null, ): boolean { + if ( + record.pidNamespace === PID_NAMESPACE_UNREADABLE || + selfNamespace === PID_NAMESPACE_UNREADABLE + ) { + return false; + } return ( record.machineId === selfMachine && record.pidNamespace === selfNamespace ); } +/** + * Serializes this process's own registry writes, so `unregisterSession` + * and an in-flight `patchSessionRecord` can never interleave. + * + * `Config.refreshSessionId` queues its patch on a fire-and-forget chain + * and returns without awaiting it, so a `/clear` immediately before quit + * can still be between its read and its write when exit cleanup runs. In + * that interleaving the unlink lands in the middle and the patch's write + * then *recreates* the record — a file advertising a PID that has already + * exited, which stands until some other session's sweep happens to notice. + * Ordering the two removes the interleaving; {@link retiredPids} handles + * the other direction, where the patch is merely queued behind the unlink + * and would otherwise resurrect it just the same. + */ +let writeQueue: Promise = Promise.resolve(); + +/** + * PIDs whose record this process has already withdrawn. A later patch for + * one is dropped rather than allowed to write the record back. + */ +const retiredPids = new Set(); + +function enqueueWrite(op: () => Promise): Promise { + // Both arms run `op`: a failed predecessor must not cancel its + // successor, since each of these is independently best-effort. + const run = writeQueue.then(op, op); + writeQueue = run.then( + () => undefined, + () => undefined, + ); + return run; +} + +/** + * Reject a write whose target is no longer the entry that was validated. + * + * `readRecord` proves things about an *inode* — its origin, its PID, that + * it parses — but every mutation that follows names a *path*, and in a + * directory a sandboxed co-tenant can write to, the two stop agreeing the + * moment the read returns: the entry can be unlinked and replaced with a + * foreign live record, which `patchSessionRecord` would then overwrite and + * `unregisterSession` would delete. Re-reading the entry immediately + * before the commit step binds them back together. + * + * `lstatSync` rather than the async form because `assertCanCommit` is the + * hook that runs *between* the last check and an irreversible `rename`; + * an `await` there would reopen the very window this closes. + */ +function assertSameEntry(filePath: string, expected: EntryIdentity): void { + const stat = fsSync.lstatSync(filePath); + if ( + !stat.isFile() || + stat.dev !== expected.dev || + stat.ino !== expected.ino + ) { + throw new Error( + `session registry entry ${filePath} changed between validation and write`, + ); + } +} + /** * Write this process's record. Best-effort: a read-only or full home * directory must not stop a session from starting, so failures are logged @@ -236,50 +350,176 @@ export async function registerSession( peerProtocol: PEER_PROTOCOL_VERSION, }; - try { - const dir = getSessionRegistryDir(); - await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); - // mkdir's mode is masked by the umask, and does nothing at all when - // the directory already exists — chmod is what actually guarantees - // 0700 on an upgrade from a build that created it more loosely. - await fs.chmod(dir, REGISTRY_DIR_MODE); + return enqueueWrite(async () => { + try { + const dir = getSessionRegistryDir(); + await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); + // mkdir's mode is masked by the umask, and does nothing at all when + // the directory already exists — chmod is what actually guarantees + // 0700 on an upgrade from a build that created it more loosely. + await fs.chmod(dir, REGISTRY_DIR_MODE); + + const filePath = getSessionRecordPath(pid); + const reportConflict = () => { + debugLogger.debug( + `registerSession skipped: ${filePath} holds a record from another origin`, + ); + try { + fields.onOriginConflict?.({ pid, filePath }); + } catch (error) { + // A reporting callback must not turn a discovery miss into a + // failed startup; registration is already best-effort. + debugLogger.debug(`onOriginConflict threw: ${describe(error)}`); + } + }; + + // Two passes at most. The first decides on what is there; if the + // exclusive create then loses a race, the second re-reads whatever + // won it and runs the winner through the same origin rule, exactly + // as if it had been there before we looked. + for (let attempt = 0; attempt < 2; attempt++) { + // Registration is the one write with nothing to merge into, so it + // is also the one that would happily clobber a stranger. A record + // from another origin at our PID number is not stale, it is not + // ours, and it cannot be proven dead from here. + const existing = await readRecord(filePath); + if ( + existing !== null && + !isSameOrigin(existing.record, record.machineId, record.pidNamespace) + ) { + reportConflict(); + return false; + } + + // `existing === null` covers two different situations and only one + // of them is a free name: nothing is there, or something is there + // that this code cannot honour (a planted symlink, a truncated + // write, a future schema). Replacing the second is deliberate and + // tested; claiming the first has to be exclusive. + if (existing === null && !(await entryExists(filePath))) { + // Claim the name with an operation the kernel makes exclusive, + // rather than reading "absent" and renaming over whatever + // arrived in between: two origins sharing `QWEN_HOME` and a PID + // number would both see the gap, and the later rename would + // silently replace the earlier live record. + const outcome = await linkRecordExclusive(filePath, record); + if (outcome === 'created') { + retiredPids.delete(pid); + return true; + } + // 'taken' — someone claimed it in between. Go round again and + // route whatever they wrote through the origin rule above, as if + // it had been there before we looked. + if (outcome === 'taken') continue; + // 'unsupported' — no hard links on this filesystem. Fall through + // to the replacing write, which is where this path has always + // been; the exclusivity gap is the price of the filesystem. + } + + // Either a same-origin record is present — the recycled-PID + // recovery path, where a predecessor died without unregistering — + // or hard links are unavailable. Replace it, but commit only if + // the directory entry is still the one that was validated. + // + // `noFollow` keeps a pre-planted `.json` symlink from + // redirecting this write (and its forced 0600) to a file outside + // the registry: the sandbox shares this directory across a trust + // boundary, so the planting side is not hypothetical. + await atomicWriteJSON(filePath, record, { + mode: REGISTRY_FILE_MODE, + forceMode: true, + noFollow: true, + assertCanCommit: existing + ? () => assertSameEntry(filePath, existing.entry) + : undefined, + }); + retiredPids.delete(pid); + return true; + } - const filePath = getSessionRecordPath(pid); - // Registration is the one write with nothing to merge into, so it is - // also the one that would happily clobber a stranger. A record from - // another origin at our PID number is not stale, it is not ours, and - // it cannot be proven dead from here. - const existing = await readRecord(filePath); - if ( - existing !== null && - !isSameOrigin(existing, record.machineId, record.pidNamespace) - ) { debugLogger.debug( - `registerSession skipped: ${filePath} holds a record from another origin`, + `registerSession skipped: lost the race for ${getSessionRecordPath(pid)} twice`, ); - try { - fields.onOriginConflict?.({ pid, filePath }); - } catch (error) { - // A reporting callback must not turn a discovery miss into a - // failed startup; registration is already best-effort. - debugLogger.debug(`onOriginConflict threw: ${describe(error)}`); - } + return false; + } catch (error) { + debugLogger.debug(`registerSession failed: ${describe(error)}`); return false; } + }); +} - // `noFollow` keeps a pre-planted `.json` symlink from redirecting - // this write (and its forced 0600) to a file outside the registry: - // the sandbox shares this directory across a trust boundary, so the - // planting side is not hypothetical. - await atomicWriteJSON(filePath, record, { - mode: REGISTRY_FILE_MODE, - forceMode: true, - noFollow: true, - }); +/** True when any directory entry exists at `filePath`, symlinks included. */ +async function entryExists(filePath: string): Promise { + try { + await fs.lstat(filePath); return true; } catch (error) { - debugLogger.debug(`registerSession failed: ${describe(error)}`); - return false; + // Anything other than "not there" — EACCES on the directory, say — + // is not evidence of a free name, so do not treat it as one. + return (error as NodeJS.ErrnoException)?.code !== 'ENOENT'; + } +} + +/** + * Create `filePath` holding `record`, or report that someone else got + * there first. Never replaces an existing entry of any kind. + * + * `link(2)` is the exclusivity primitive: it fails with `EEXIST` when the + * new name exists — symlinks included, which are an entry rather than a + * thing to follow — and it publishes a file that was already written and + * fsynced, so the record is never observable half-formed. `rename(2)`, + * the usual atomic-write commit, has the opposite property: it replaces. + * + * Returns `'unsupported'` where the filesystem has no hard links (some + * network and FUSE mounts), leaving the caller on its previous path. + */ +async function linkRecordExclusive( + filePath: string, + record: SessionRegistryRecord, +): Promise<'created' | 'taken' | 'unsupported'> { + const tmpPath = path.join( + path.dirname(filePath), + `.${process.pid}.${randomBytes(6).toString('hex')}.tmp`, + ); + try { + const handle = await fs.open( + tmpPath, + fsSync.constants.O_WRONLY | + fsSync.constants.O_CREAT | + fsSync.constants.O_EXCL, + REGISTRY_FILE_MODE, + ); + try { + await handle.writeFile(JSON.stringify(record, null, 2)); + await handle.sync(); + // open()'s mode argument is masked by the umask; fchmod is not, and + // goes through the handle so it cannot be redirected. + await handle.chmod(REGISTRY_FILE_MODE); + } finally { + await handle.close(); + } + + try { + await fs.link(tmpPath, filePath); + return 'created'; + } catch (error) { + const code = (error as NodeJS.ErrnoException)?.code; + if (code === 'EEXIST') return 'taken'; + if ( + code === 'EPERM' || + code === 'ENOSYS' || + code === 'ENOTSUP' || + code === 'EOPNOTSUPP' || + code === 'EMLINK' || + code === 'EXDEV' + ) { + return 'unsupported'; + } + throw error; + } + } finally { + // The link, if it was made, keeps the inode alive under its real name. + await fs.unlink(tmpPath).catch(() => {}); } } @@ -295,25 +535,39 @@ export async function registerSession( * record would be missing whatever else registration would have set. * No-ops too when the record present at this PID came from another * origin — merging into it would rewrite a stranger's sessionId, cwd and - * name, sending discovery to the wrong transcript. + * name, sending discovery to the wrong transcript — and once + * {@link unregisterSession} has withdrawn this PID, since a patch landing + * after the withdrawal would put a dead process back on the register. */ export async function patchSessionRecord( patch: Partial>, pid: number = process.pid, ): Promise { - const filePath = getSessionRecordPath(pid); - try { - const existing = await readRecord(filePath); - if (existing === null) return; - if (!isSameOrigin(existing, readMachineId(), readPidNamespaceId())) return; - await atomicWriteJSON( - filePath, - { ...existing, ...patch }, - { mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true }, - ); - } catch (error) { - debugLogger.debug(`patchSessionRecord failed: ${describe(error)}`); - } + await enqueueWrite(async () => { + if (retiredPids.has(pid)) return; + const filePath = getSessionRecordPath(pid); + try { + const existing = await readRecord(filePath); + if (existing === null) return; + if ( + !isSameOrigin(existing.record, readMachineId(), readPidNamespaceId()) + ) { + return; + } + await atomicWriteJSON( + filePath, + { ...existing.record, ...patch }, + { + mode: REGISTRY_FILE_MODE, + forceMode: true, + noFollow: true, + assertCanCommit: () => assertSameEntry(filePath, existing.entry), + }, + ); + } catch (error) { + debugLogger.debug(`patchSessionRecord failed: ${describe(error)}`); + } + }); } /** @@ -324,20 +578,38 @@ export async function patchSessionRecord( * somewhere, and one that will not re-register (registration is * startup-only). Anything unparseable is left too — it is not a record * this code wrote, so it is not this code's to delete. + * + * Also closes the register for this PID, so a `patchSessionRecord` queued + * behind this call is dropped instead of writing the record back. */ export async function unregisterSession( pid: number = process.pid, ): Promise { - const filePath = getSessionRecordPath(pid); - try { - const existing = await readRecord(filePath); - if (existing === null || existing.pid !== pid) return; - if (!isSameOrigin(existing, readMachineId(), readPidNamespaceId())) return; - await fs.unlink(filePath); - } catch (error) { - if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; - debugLogger.debug(`unregisterSession failed: ${describe(error)}`); - } + await enqueueWrite(async () => { + // Before the unlink, not after: a patch queued behind this one must be + // refused even if the unlink itself finds nothing to do. + retiredPids.add(pid); + const filePath = getSessionRecordPath(pid); + try { + const existing = await readRecord(filePath); + if (existing === null || existing.record.pid !== pid) return; + if ( + !isSameOrigin(existing.record, readMachineId(), readPidNamespaceId()) + ) { + return; + } + // Node exposes no `unlinkat`-by-inode, so the entry is re-checked as + // late as it can be. That narrows the swap window to the syscall + // pair rather than to the whole validating read — the same binding + // the write paths get from `assertCanCommit`, minus a primitive the + // platform does not offer. + assertSameEntry(filePath, existing.entry); + await fs.unlink(filePath); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; + debugLogger.debug(`unregisterSession failed: ${describe(error)}`); + } + }); } export interface ListLiveSessionsOptions { @@ -386,86 +658,123 @@ export async function listLiveSessions( const selfMachine = readMachineId(); const tokensAvailable = supportsProcStartToken(); + // Take the candidates before doing any work on them. Both how many + // there are and what they are named is outside this process's control + // (see MAX_RECORDS_PER_SCAN), so the ceiling has to be applied to the + // list, not discovered while walking it. + const candidates = entries.filter((name) => RECORD_FILENAME.test(name)); + if (candidates.length > MAX_RECORDS_PER_SCAN) { + debugLogger.debug( + `listLiveSessions: ${candidates.length} candidate records in ${dir}, examining ${MAX_RECORDS_PER_SCAN}`, + ); + candidates.length = MAX_RECORDS_PER_SCAN; + } + const live: SessionRegistryRecord[] = []; - await Promise.all( - entries - .filter((name) => RECORD_FILENAME.test(name)) - .map(async (name) => { - const filePath = path.join(dir, name); - const record = await readRecord(filePath); - if (record === null) return; - - // A record whose filename disagrees with its contents was not - // written by this code (or was renamed by hand). Skip it, and - // never sweep it — we cannot reason about which PID it describes. - if (`${record.pid}.json` !== name) return; - - // Every check below — the self-PID comparison included — reads - // `record.pid` as a number on *our* machine in *our* PID - // namespace. When the record came from another origin, or from a - // writer whose origin we cannot pin down, that reading is - // meaningless: the record is neither reported (the PID would name - // some unrelated local process, up to and including this one) nor - // swept (a local ESRCH says nothing about a process elsewhere, - // and registration is startup-only, so an unlink here would hide - // a live session for the rest of its life). This has to run - // before the self-PID shortcut below, or a foreign record sitting - // at our own PID number is adopted as our session without ever - // reaching the gate. - if (!isSameOrigin(record, selfMachine, selfNamespace)) return; - - if (record.pid === selfPid) { - // Report the record at our own PID without probing: the origin - // gate above has established it describes this machine and this - // namespace, and the PID is ours, so it is alive by - // construction. (It is not necessarily *this session's* record - // — a same-origin predecessor that died on this PID leaves one - // behind — but that is a liveness-of-content question, not one - // this shortcut answers.) - if (includeSelf) live.push(record); - return; - } + await mapWithConcurrency( + candidates, + SCAN_CONCURRENCY, + async (name: string) => { + const filePath = path.join(dir, name); + const read = await readRecord(filePath); + if (read === null) return; + const record = read.record; + + // A record whose filename disagrees with its contents was not + // written by this code (or was renamed by hand). Skip it, and + // never sweep it — we cannot reason about which PID it describes. + if (`${record.pid}.json` !== name) return; + + // Every check below — the self-PID comparison included — reads + // `record.pid` as a number on *our* machine in *our* PID + // namespace. When the record came from another origin, or from a + // writer whose origin we cannot pin down, that reading is + // meaningless: the record is neither reported (the PID would name + // some unrelated local process, up to and including this one) nor + // swept (a local ESRCH says nothing about a process elsewhere, + // and registration is startup-only, so an unlink here would hide + // a live session for the rest of its life). This has to run + // before the self-PID shortcut below, or a foreign record sitting + // at our own PID number is adopted as our session without ever + // reaching the gate. + if (!isSameOrigin(record, selfMachine, selfNamespace)) return; + + if (record.pid === selfPid) { + // Report the record at our own PID without probing: the origin + // gate above has established it describes this machine and this + // namespace, and the PID is ours, so it is alive by + // construction. (It is not necessarily *this session's* record + // — a same-origin predecessor that died on this PID leaves one + // behind — but that is a liveness-of-content question, not one + // this shortcut answers.) + if (includeSelf) live.push(record); + return; + } - if (isSameProcess(record.pid, record.procStart)) { - // Alive — but on a platform that has start tokens, a record - // without one was not written by this build, which always - // records one. `isSameProcess` has just degraded to a bare - // liveness check, so all this record proves is that *some* - // process holds that PID; the session it describes — - // sessionId, cwd, name — is whoever wrote the file's to choose, - // and the origin fields needed to get this far are plaintext in - // every sibling record. Withhold it from callers, but do not - // sweep it: the PID is live, and it may equally be a future - // version's record, which an unlink would erase for good. - if (tokensAvailable && record.procStart == null) return; - live.push(record); - return; - } + if (isSameProcess(record.pid, record.procStart)) { + // Alive — but on a platform that has start tokens, a record + // without one was not written by this build, which always + // records one. `isSameProcess` has just degraded to a bare + // liveness check, so all this record proves is that *some* + // process holds that PID; the session it describes — + // sessionId, cwd, name — is whoever wrote the file's to choose, + // and the origin fields needed to get this far are plaintext in + // every sibling record. Withhold it from callers, but do not + // sweep it: the PID is live, and it may equally be a future + // version's record, which an unlink would erase for good. + if (tokensAvailable && record.procStart == null) return; + live.push(record); + return; + } - if (sweepStale) { - try { - await fs.unlink(filePath); - } catch { - // Raced with another session's sweep, or not ours to delete. - } + if (sweepStale) { + try { + // Same binding as unregisterSession's: the entry that proved + // itself stale is the only one this may remove, so a co-tenant + // who swaps a live foreign record into the name after the read + // does not get it deleted on their behalf. + assertSameEntry(filePath, read.entry); + await fs.unlink(filePath); + } catch { + // Raced with another session's sweep, replaced under us, or not + // ours to delete. } - }), + } + }, ); return live.sort((a, b) => b.startedAt - a.startedAt); } -/** Read and validate one record. Returns null for anything unusable. */ -async function readRecord( - filePath: string, -): Promise { +/** + * Read and validate one record. Returns null for anything unusable. + * + * Everything is read through a single handle opened `O_NOFOLLOW`, and the + * entry's identity comes back with the bytes. Two reasons: the write paths + * already refuse to follow a symlink planted at this name, so the read + * that authorizes them must not follow one either; and every caller + * mutates by *path* afterwards, which is only sound if it can check the + * path still resolves to the inode that was validated (see + * {@link assertSameEntry}). + */ +async function readRecord(filePath: string): Promise { let raw: string; + let entry: EntryIdentity; + let handle: fs.FileHandle; try { - const stat = await fs.stat(filePath); + handle = await fs.open(filePath, fsSync.constants.O_RDONLY | O_NOFOLLOW); + } catch { + return null; + } + try { + const stat = await handle.stat(); if (!stat.isFile() || stat.size > MAX_RECORD_BYTES) return null; - raw = await fs.readFile(filePath, 'utf8'); + entry = { dev: stat.dev, ino: stat.ino }; + raw = await handle.readFile('utf8'); } catch { return null; + } finally { + await handle.close().catch(() => {}); } let parsed: unknown; @@ -517,21 +826,48 @@ async function readRecord( const peerProtocol = value['peerProtocol']; return { - schemaVersion, - pid, - procStart: typeof procStart === 'string' ? procStart : null, - pidNamespace: typeof pidNamespace === 'string' ? pidNamespace : null, - machineId: typeof machineId === 'string' ? machineId : null, - sessionId, - cwd, - name, - kind, - startedAt, - qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, - peerProtocol: typeof peerProtocol === 'number' ? peerProtocol : 0, + entry, + record: { + schemaVersion, + pid, + procStart: typeof procStart === 'string' ? procStart : null, + pidNamespace: typeof pidNamespace === 'string' ? pidNamespace : null, + machineId: typeof machineId === 'string' ? machineId : null, + sessionId, + cwd, + name, + kind, + startedAt, + qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, + peerProtocol: typeof peerProtocol === 'number' ? peerProtocol : 0, + }, }; } +/** + * Run `fn` over `items` with at most `limit` in flight. + * + * Deliberately not `Promise.all(items.map(...))`: `items` here is derived + * from a directory a sandboxed co-tenant can write to, and that form + * starts every read in the same tick. + */ +async function mapWithConcurrency( + items: readonly T[], + limit: number, + fn: (item: T) => Promise, +): Promise { + let next = 0; + const worker = async () => { + while (next < items.length) { + const item = items[next++]; + if (item !== undefined) await fn(item); + } + }; + await Promise.all( + Array.from({ length: Math.min(limit, items.length) }, worker), + ); +} + function describe(error: unknown): string { return error instanceof Error ? `${error.name}: ${error.message}` diff --git a/packages/core/src/utils/atomicFileWrite.test.ts b/packages/core/src/utils/atomicFileWrite.test.ts index a1c52962a62..1422e14f3a3 100644 --- a/packages/core/src/utils/atomicFileWrite.test.ts +++ b/packages/core/src/utils/atomicFileWrite.test.ts @@ -409,6 +409,11 @@ describe('atomicWriteFile', () => { const victim = path.join(tmpDir, 'victim.txt'); const planted = path.join(tmpDir, 'planted.json'); await fs.writeFile(victim, 'PRECIOUS', { mode: 0o644 }); + // writeFile's mode is masked by the umask, so under umask 077 the + // fixture lands at 0o600 and the "unchanged at 0o644" assertion + // below fails for a reason that has nothing to do with the code + // under test. chmod is not masked. + await fs.chmod(victim, 0o644); await fs.symlink(victim, planted); const realGeteuid = process.geteuid!; @@ -1695,6 +1700,67 @@ describe('noFollow option — symlink protection', () => { expect(await fs.readFile(target, 'utf-8')).toBe('NEW'); }); + // The ownership-preservation fallback is the *other* path that commits + // by pathname under noFollow. lstat proving the entry is a regular file + // does not survive the `writeFile(targetPath)` + `chmod(targetPath)` + // that follow it: both re-resolve the name, so an attacker who swaps a + // symlink in during that window gets the payload and the forced 0o600 + // delivered to a file of their choosing. O_NOFOLLOW makes the kernel + // refuse a symlink at the final component, and the handle then pins + // every mutation to the inode that was validated. The existing + // behavioral test pre-places a static link, so it never races this + // window — only the flags show whether the window is closed. + it.skipIf( + process.platform === 'win32' || typeof process.geteuid !== 'function', + )( + 'atomicWriteFile: noFollow ownership fallback opens the target with O_NOFOLLOW and mutates only through the handle', + async () => { + const target = path.join(tmpDir, 'ownership-nofollow.json'); + await fs.writeFile(target, 'OLD'); + const targetStat = await fs.stat(target); + + const openSpy = vi.fn(fs.open); + const chmodSpy = vi.fn(fs.chmod); + const fchmodSpy = vi.fn((fh: fs.FileHandle, mode: number) => + fh.chmod(mode), + ); + + const realGeteuid = process.geteuid!; + process.geteuid = () => targetStat.uid + 1; + try { + await atomicWriteFile( + target, + 'NEW', + { mode: 0o600, forceMode: true, noFollow: true }, + { open: openSpy, chmod: chmodSpy, fchmod: fchmodSpy }, + ); + } finally { + process.geteuid = realGeteuid; + } + + expect(openSpy).toHaveBeenCalledTimes(1); + const [openedPath, flags] = openSpy.mock.calls[0] as [string, number]; + expect(openedPath).toBe(target); + expect(flags & fsSync.constants.O_NOFOLLOW).toBe( + fsSync.constants.O_NOFOLLOW, + ); + expect(flags & fsSync.constants.O_TRUNC).toBe(fsSync.constants.O_TRUNC); + // No O_CREAT: this branch runs precisely because the file is there, + // and keeping its inode is the point — a rename would reset the uid. + expect(flags & fsSync.constants.O_CREAT).toBe(0); + + // The mode goes on through the fd, never through the pathname. + expect(fchmodSpy).toHaveBeenCalledTimes(1); + expect(chmodSpy).not.toHaveBeenCalled(); + + // The inode is preserved (that is what the fallback exists for) and + // holds the new content at the forced mode. + expect(await fs.readFile(target, 'utf-8')).toBe('NEW'); + expect((await fs.stat(target)).ino).toBe(targetStat.ino); + expect((await fs.stat(target)).mode & 0o7777).toBe(0o600); + }, + ); + it('atomicWriteFileSync: noFollow EXDEV opens the target with O_EXCL (no-clobber create)', () => { const target = path.join(tmpDir, 'oexcl-sync.txt'); const exdevRename = () => { diff --git a/packages/core/src/utils/atomicFileWrite.ts b/packages/core/src/utils/atomicFileWrite.ts index b05f5dc0468..a0e9e4f153d 100644 --- a/packages/core/src/utils/atomicFileWrite.ts +++ b/packages/core/src/utils/atomicFileWrite.ts @@ -261,12 +261,65 @@ export async function atomicWriteFile( return existingStat.uid !== euid; }; + // Write `data` through an already-open handle, then fsync and fchmod it. + // Shared by the two paths that must not re-resolve `targetPath` after + // validating it — the ownership fallback and the noFollow EXDEV + // fallback. The narrowed chmod catch is FAT/exFAT only, so a sandbox + // EPERM, a transient EIO or a read-only EROFS fails loudly instead of + // leaving a credential file at the umask-masked mode with no trail. + const writeThroughHandle = async (fd: fs.FileHandle): Promise => { + await fd.writeFile( + typeof data === 'string' ? Buffer.from(data, encoding) : data, + ); + if (flush) await fd.sync(); + if (desiredMode !== undefined) { + try { + await fchmodImpl(fd, desiredMode); + } catch (chmodErr) { + if ( + !isNodeError(chmodErr) || + (chmodErr.code !== 'ENOSYS' && chmodErr.code !== 'ENOTSUP') + ) { + throw chmodErr; + } + } + } + }; + if ( existingStat !== undefined && existingStat.isFile() && ownershipWouldChange() ) { options?.assertCanCommit?.(); + if (options?.noFollow) { + // The lstat above proved this entry was a regular file, but + // `writeFile` and `chmod` name a *path*, and re-resolve it. On the + // shared directory noFollow exists for, an attacker who swaps in a + // symlink inside that window gets both the payload and the forced + // mode delivered to a file of their choosing — the same clobber the + // EXDEV fallback below already had to close. + // + // O_NOFOLLOW makes the kernel refuse a symlink at the final + // component, and every mutation then goes through the resulting + // handle, which is pinned to the inode that was validated. No + // O_CREAT: this branch exists because the file is already there, and + // preserving its inode is the entire point (a rename would reset the + // owner). O_NOFOLLOW is absent on Windows, which never reaches here + // anyway — `ownershipWouldChange` returns false on win32. + const fd = await openImpl( + targetPath, + fsSync.constants.O_WRONLY | + fsSync.constants.O_TRUNC | + (fsSync.constants.O_NOFOLLOW ?? 0), + ); + try { + await writeThroughHandle(fd); + } finally { + await fd.close(); + } + return; + } await fs.writeFile(targetPath, data, writeOptions); await tryChmod(targetPath); return; @@ -330,32 +383,11 @@ export async function atomicWriteFile( let writeOk = false; try { try { - await fd.writeFile( - typeof data === 'string' ? Buffer.from(data, encoding) : data, - ); - if (flush) await fd.sync(); - // fchmod via the open fd — immune to symlink swap between - // close and a path-based chmod, which would otherwise redirect - // the 0o600 onto an attacker-pointed target and silently - // defeat noFollow on the EXDEV fallback path. - // - // Narrow the catch to FAT/exFAT signatures (ENOSYS / ENOTSUP). - // Operations on credential files are security-sensitive enough - // that a sandbox EPERM, transient EIO, or read-only EROFS - // should fail loudly rather than leave the file at the - // umask-masked open() mode with no diagnostic trail. - if (desiredMode !== undefined) { - try { - await fchmodImpl(fd, desiredMode); - } catch (chmodErr) { - if ( - !isNodeError(chmodErr) || - (chmodErr.code !== 'ENOSYS' && chmodErr.code !== 'ENOTSUP') - ) { - throw chmodErr; - } - } - } + // fchmod via the open fd — immune to a symlink swap between + // close and a path-based chmod, which would otherwise + // redirect the 0o600 onto an attacker-pointed target and + // silently defeat noFollow on this fallback path. + await writeThroughHandle(fd); writeOk = true; } finally { await fd.close(); diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index 1bc63263276..aa215e08252 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -11,6 +11,7 @@ import { hostname } from 'node:os'; import { isPidAlive, isSameProcess, + PID_NAMESPACE_UNREADABLE, readMachineId, readPidNamespaceId, readProcStartToken, @@ -25,6 +26,9 @@ const procReadFails = vi.hoisted(() => ({ value: false })); /** The same, for the `/proc/self/ns/pid` readlink. */ const nsReadFails = vi.hoisted(() => ({ value: false })); +/** Serves a chosen `/proc/self/ns/pid` target instead of the real one. */ +const nsLinkTarget = vi.hoisted(() => ({ value: null as string | null })); + /** * Stands in for the machine-id sources, which cannot be arranged on the * real filesystem: a path present here is served from the map (a string @@ -63,6 +67,9 @@ vi.mock('node:fs', async (importOriginal) => { code: 'EACCES', }); } + if (nsLinkTarget.value !== null && args[0] === '/proc/self/ns/pid') { + return nsLinkTarget.value; + } // eslint-disable-next-line @typescript-eslint/no-explicit-any return (actual.readlinkSync as any)(...args); }) as typeof actual.readlinkSync, @@ -227,14 +234,30 @@ describe('readPidNamespaceId', () => { expect(target).not.toBe(readlinkSync('/proc/self/ns/mnt')); }); - it('returns null instead of throwing when the link cannot be read', () => { + it('reports an unreadable link as unprovable, not as "no namespaces"', () => { nsReadFails.value = true; try { - expect(readPidNamespaceId()).toBeNull(); + // Specifically NOT null. null is the claim "this platform has no PID + // namespaces", which two peers can legitimately agree on; an + // unreadable `/proc/self/ns/pid` is the absence of any claim. Two + // containers behind a hidepid mount that shared a machine id and a + // QWEN_HOME would otherwise match as one origin and read each + // other's PID numbers as their own. + expect(readPidNamespaceId()).toBe(PID_NAMESPACE_UNREADABLE); + expect(readPidNamespaceId()).not.toBeNull(); } finally { nsReadFails.value = false; } }); + + it('reports a link whose target does not parse as unprovable too', () => { + nsLinkTarget.value = 'pid:[not-an-inode]'; + try { + expect(readPidNamespaceId()).toBe(PID_NAMESPACE_UNREADABLE); + } finally { + nsLinkTarget.value = null; + } + }); }); describe('readMachineId', () => { diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index 29278d0b26f..12b431edf61 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -79,8 +79,29 @@ export function readProcStartToken(pid: number): string | null { } /** - * An opaque identifier for the PID namespace this process lives in, or - * `null` where the platform does not expose one. + * Recorded in place of a namespace id when the platform *has* PID + * namespaces but this process could not read its own — a `hidepid=2` + * mount, a seccomp filter, a `/proc` that is not mounted at all. + * + * It exists because `null` is already spoken for. `null` means "this + * platform has no namespaces", which is a positive statement two peers can + * agree on; an unreadable `/proc/self/ns/pid` is the opposite, a total + * absence of evidence. Collapsing the two would make two containers that + * share a machine id and a `QWEN_HOME` but can neither read their own + * namespace agree that they are one origin, and a matching PID number + * would then be enough to list, patch, overwrite or sweep the other + * container's record. + * + * Never equal to a real id — those are decimal inodes — so + * `isSameOrigin`-style comparisons reject it on whichever side it appears. + */ +export const PID_NAMESPACE_UNREADABLE = 'unreadable'; + +/** + * An opaque identifier for the PID namespace this process lives in, + * `null` where the platform does not expose one, or + * {@link PID_NAMESPACE_UNREADABLE} where it does but the id could not be + * read. * * A PID only means something inside one namespace. Anything that writes a * PID into a directory another namespace can also read — a container and @@ -91,18 +112,20 @@ export function readProcStartToken(pid: number): string | null { * Backed by `/proc/self/ns/pid`, a symlink whose target is * `pid:[]`; the inode is stable for the namespace's lifetime and * identical for two processes exactly when a PID means the same thing to - * both of them. Returns `null` off Linux, where the concept does not - * exist — callers must treat a `null` on both sides as "no namespace - * boundary to worry about", and a mismatch of any kind as unprovable. + * both of them. Callers must treat a `null` on both sides as "no namespace + * boundary to worry about", and both a mismatch and an unreadable id as + * unprovable. */ export function readPidNamespaceId(): string | null { if (process.platform !== 'linux') return null; try { const target = fs.readlinkSync('/proc/self/ns/pid'); const match = /^pid:\[(\d+)\]$/.exec(target); - return match?.[1] ?? null; + // A target that exists but does not parse is the same evidential + // state as one that could not be read: Linux, namespaces, no id. + return match?.[1] ?? PID_NAMESPACE_UNREADABLE; } catch { - return null; + return PID_NAMESPACE_UNREADABLE; } } From 9c16eb1085cc113537367c623f72736cf0e28a49 Mon Sep 17 00:00:00 2001 From: qqqys Date: Mon, 10 Aug 2026 05:24:56 +0800 Subject: [PATCH 10/17] fix(core): stop the registry's fs.constants read from failing module load `Test (ubuntu-latest, Node 22.x)` failed on 2423dcf94 with six failed suites and zero failed tests: marketplace, npm, client, geminiChat, mcp-client and nextSpeakerChecker never started. All six substitute `node:fs` without a `constants` export, and all six now reach `session-registry.ts` transitively -- marketplace/npm through `config/config.ts:213`, the rest through the barrel at `index.ts:292`. `const O_NOFOLLOW = fsSync.constants.O_NOFOLLOW ?? 0` ran at module scope, so that read happened during *initialization* of every consumer of either entry point. The `?? 0` guarded a `constants` object without the flag, which is the Windows case it documents; it could not guard a `node:fs` that has no `constants` at all. Read the flag at the call site instead. Production behaviour is unchanged -- real `node:fs` always exports `constants`, and the Windows degradation to 0 is preserved -- but module load no longer depends on it. Pinned by session-registry.module-init.test.ts, which imports the module under exactly that mock shape. Reintroducing the module-scope read fails it with the CI error verbatim. Verified: the six suites above plus session-registry.test.ts all pass (813 + 85 tests); `npm run typecheck`, `npm run build`, eslint and prettier clean in packages/core. --- .../session-registry.module-init.test.ts | 36 +++++++++++++++++++ .../core/src/services/session-registry.ts | 14 ++++++-- 2 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/services/session-registry.module-init.test.ts diff --git a/packages/core/src/services/session-registry.module-init.test.ts b/packages/core/src/services/session-registry.module-init.test.ts new file mode 100644 index 00000000000..6531c8aee5c --- /dev/null +++ b/packages/core/src/services/session-registry.module-init.test.ts @@ -0,0 +1,36 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * `session-registry.ts` is reachable from `config.ts` and from the package + * barrel, so anything it evaluates at module scope is evaluated by every + * consumer that imports either. Reading `fs.constants` at module scope + * turned that into a hard dependency: six suites that substitute `node:fs` + * without a `constants` export failed to load at all — no assertion ran, + * they simply never started. + * + * This suite pins the load-time contract from the outside: a `node:fs` + * substitute that omits `constants` must still let the module initialize. + * It lives in its own file because the mock below is process-wide and would + * otherwise defeat `session-registry.test.ts`, which exercises real I/O. + */ + +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('node:fs', () => ({ + // Deliberately no `constants` — this is the shape the failing suites use. + existsSync: vi.fn(), + readFileSync: vi.fn(), +})); + +describe('session-registry module initialization', () => { + it('loads when node:fs is substituted without a constants export', async () => { + const mod = await import('./session-registry.js'); + + expect(typeof mod.registerSession).toBe('function'); + expect(typeof mod.listLiveSessions).toBe('function'); + }); +}); diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 30d98253c8d..e4bb1b62201 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -121,8 +121,15 @@ const SCAN_CONCURRENCY = 16; * omits it; `| undefined` would poison the whole flag word into `NaN`. * Zero is the correct degradation — Windows has no symlink-in-a-shared- * home threat model here, and every other guard still applies. + * + * Read at the call site rather than at module load. This module is + * reachable from `config.ts` and from the package barrel, so a top-level + * `fs.constants` read makes module *initialization* depend on that export + * and takes down every consumer that substitutes `node:fs` without it. */ -const O_NOFOLLOW = fsSync.constants.O_NOFOLLOW ?? 0; +function noFollowFlag(): number { + return fsSync.constants.O_NOFOLLOW ?? 0; +} /** A directory entry's identity, as observed through an open handle. */ interface EntryIdentity { @@ -762,7 +769,10 @@ async function readRecord(filePath: string): Promise { let entry: EntryIdentity; let handle: fs.FileHandle; try { - handle = await fs.open(filePath, fsSync.constants.O_RDONLY | O_NOFOLLOW); + handle = await fs.open( + filePath, + fsSync.constants.O_RDONLY | noFollowFlag(), + ); } catch { return null; } From 8621320f2cd0af4e9f254fdc5a8b65c16bbaa861 Mon Sep 17 00:00:00 2001 From: qqqys Date: Mon, 10 Aug 2026 10:40:02 +0800 Subject: [PATCH 11/17] fix(core): stop registry reads hanging, over-reading, or clobbering strangers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6's three Criticals, all on the path `qwen sessions ps` and startup registration share. R6-1 — a FIFO planted at `.json` hangs every reader. `readRecord` opened with a bare `O_RDONLY`, and the `isFile()` rejection that would have thrown the entry out cannot run until the open returns. Under this directory's own threat model a co-tenant names `.json` at will, so `mkfifo` there hangs `qwen sessions ps`, hangs a session's own registration when it sits at that session's PID, and exhausts libuv's four-thread fs pool a few entries in. Every registry read now opens `O_NONBLOCK` alongside `O_NOFOLLOW`, as gitUtils, skill-curator and session-writer-lease already do. R6-3 — the `MAX_RECORD_BYTES` ceiling was advisory. The size came from `fstat` and the bytes came from a `readFile()` to EOF, two observations of an inode a co-tenant can still be growing, so a record that passed the check at eleven bytes could return hundreds of megabytes. Reads now go through a bounded `read()` into a cap+1 buffer, which makes post-check growth irrelevant: the ceiling is enforced on the bytes actually allocated. R6-2 — the replacing write skipped both guarantees round 5 established. An entry `readRecord` refuses reaches it with the record's origin thrown away along with the bytes, so a live foreign record one schema version ahead was overwritten silently, with no `onOriginConflict` — precisely the outcome the origin rule exists to prevent, reached by another route. That write now peeks the entry's `machineId`/`pidNamespace` and runs the same `isSameOrigin` gate, and pins the commit to the inode it inspected. An entry that cannot be attributed at all stays replaceable: refusing those would strand registration permanently on one truncated write, and registration only runs at startup. The pin is identity-only there, since a planted symlink or stray directory is what the write is there to clear. Verification: 6 new tests in session-registry.test.ts, all mutation-checked — dropping `O_NONBLOCK` fails both FIFO tests with "blocked for over 4s"; restoring `readFile()` lists the padded 64KiB record the cap should have rejected; removing the origin peek returns true and clobbers the foreign record; flipping the new pin's `requireFile` back to true breaks the existing planted-symlink test. session-registry, atomicFileWrite and process-liveness: 166 passed. `npm run build` and `tsc --noEmit` clean for core and cli; eslint clean on both changed files. --- .../src/services/session-registry.test.ts | 195 ++++++++++++++ .../core/src/services/session-registry.ts | 246 +++++++++++++++--- 2 files changed, 408 insertions(+), 33 deletions(-) diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index e17b45bce9e..4e4f8a4e136 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -44,6 +45,33 @@ vi.mock('../utils/process-liveness.js', async (importOriginal) => { }; }); +/** + * Lets a test reproduce the one state a real attacker produces and a + * fixture cannot: an entry whose `fstat` size and whose actual byte count + * disagree, because the inode grew between the two. Everything else about + * `node:fs/promises` passes straight through. + */ +const statSizeLie = vi.hoisted(() => ({ value: null as number | null })); + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + const open: typeof actual.open = async (...args) => { + const handle = await actual.open(...args); + const realStat = handle.stat.bind(handle); + handle.stat = async (options?: Parameters[0]) => { + const stat = await realStat(options); + if (statSizeLie.value === null) return stat; + // Prototype-chained rather than spread: `isFile()` and friends live + // on `Stats.prototype`, and a spread copy would lose them. + return Object.create(stat, { + size: { value: statSizeLie.value, enumerable: true }, + }); + }; + return handle; + }; + return { ...actual, default: { ...actual, open }, open }; +}); + vi.mock('../config/storage.js', () => { let mockDir = '/tmp/session-registry-test'; return { @@ -70,6 +98,7 @@ beforeEach(async () => { }); afterEach(async () => { + statSizeLie.value = null; await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -93,6 +122,26 @@ async function writeRaw(fileName: string, body: unknown): Promise { return filePath; } +/** + * Fail a hang as a normal assertion rather than as a suite timeout. + * + * A blocked FIFO open is held by a libuv fs thread that nothing can + * cancel, so letting the test time out would take the rest of the file + * down with it. Four seconds is far past any honest read of a directory + * holding a handful of small files. + */ +async function withinFourSeconds(work: Promise): Promise { + let timer: NodeJS.Timeout; + const deadline = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('blocked for over 4s')), 4000); + }); + try { + return await Promise.race([work, deadline]); + } finally { + clearTimeout(timer!); + } +} + describe('deriveSessionName', () => { it('combines the cwd basename with a session-derived suffix', () => { const name = deriveSessionName('/home/u/projects/qwen-code', 'abc-123'); @@ -449,6 +498,107 @@ describe('registerSession', () => { }), ).toBe(false); }); + + it('refuses a foreign record it could not parse, instead of clobbering it', async () => { + // A record one schema version ahead is rejected by the reader with its + // origin thrown away, which lands it on the *replacing* write rather + // than the origin gate. Being unreadable to us is not what makes a + // record ours: this one is live somewhere else, nothing sweeps a + // foreign entry, and registration only runs at startup — so the + // clobber would take that session out of discovery for good. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + pid: process.pid, + procStart: null, + pidNamespace: readPidNamespaceId(), + machineId: 'another-machine', + sessionId: 's-theirs', + cwd: '/w/theirs', + name: 'theirs-aa', + kind: 'interactive', + startedAt: 1000, + }); + const before = await fs.readFile(filePath, 'utf8'); + + const conflicts: Array<{ pid: number; filePath: string }> = []; + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + onOriginConflict: (info) => conflicts.push(info), + }), + ).toBe(false); + + expect(await fs.readFile(filePath, 'utf8')).toBe(before); + expect(conflicts).toEqual([{ pid: process.pid, filePath }]); + }); + + it('still replaces an unusable entry that claims this origin', async () => { + // The other half of the rule above: a truncated write from a previous + // run of *this* session is exactly what the replacing write is for. + // Refusing everything unparseable would strand registration on it + // permanently. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + }); + + const onOriginConflict = vi.fn(); + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + onOriginConflict, + }), + ).toBe(true); + + expect(onOriginConflict).not.toHaveBeenCalled(); + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-ours', + }); + }); + + it('replaces an entry whose bytes carry no origin claim at all', async () => { + const filePath = await writeRaw(`${process.pid}.json`, 'not json at all'); + + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + }), + ).toBe(true); + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-ours', + }); + }); + + // `mkfifo` has no Windows equivalent, and the flag it needs is absent + // from `fs.constants` there. + it.skipIf(process.platform === 'win32')( + 'does not hang on a FIFO planted at its own record path', + async () => { + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + execFileSync('mkfifo', [getSessionRecordPath()]); + + // A blocking `O_RDONLY` open on a FIFO waits for a writer that never + // comes, and the type check that would reject it cannot run until + // the open returns — so startup registration never completes. + await expect( + withinFourSeconds( + registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ), + ).resolves.toBe(true); + expect((await fs.lstat(getSessionRecordPath())).isFIFO()).toBe(false); + }, + ); }); describe('patchSessionRecord', () => { @@ -1184,4 +1334,49 @@ describe('bounding what one enumeration will do', () => { ), ).toEqual([]); }); + + it('bounds the bytes it reads even when the size check was told otherwise', async () => { + // The size check and a read-to-EOF are two observations of an inode a + // co-tenant is still writing to, so the check passing at eleven bytes + // says nothing about what the read returns. With up to 512 candidates + // per pass and a free retry on every `qwen sessions ps`, an unbounded + // read there is a memory-exhaustion lever, not a parse error. + // + // The padding sits inside an otherwise *valid, live* record on + // purpose: garbage would be rejected by the parser either way, and the + // cap would look enforced while nothing enforced it. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: readProcStartToken(process.pid), + pidNamespace: readPidNamespaceId(), + sessionId: 's-oversized', + cwd: '/w/app', + name: 'app-aa', + kind: 'interactive', + startedAt: Date.now(), + filler: 'x'.repeat(64 * 1024), + }); + expect((await fs.stat(filePath)).size).toBeGreaterThan(64 * 1024); + statSizeLie.value = 11; + + expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + // Rejected on the byte count, not swept: nothing here proved it dead. + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + // `mkfifo` has no Windows equivalent. + it.skipIf(process.platform === 'win32')( + 'does not hang on a FIFO planted among the candidates', + async () => { + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + execFileSync('mkfifo', [getSessionRecordPath(DEAD_PID)]); + + // Under this directory's own threat model a co-tenant names + // `.json` at will, so `mkfifo` there is a one-command hang of + // every `qwen sessions ps` on the box — and a handful of them + // exhausts libuv's four-thread fs pool for the whole process. + await expect(withinFourSeconds(listLiveSessions())).resolves.toEqual([]); + }, + ); }); diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index e4bb1b62201..79d7fd3fdec 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -131,6 +131,30 @@ function noFollowFlag(): number { return fsSync.constants.O_NOFOLLOW ?? 0; } +/** + * `O_NONBLOCK` is what keeps the *open* from hanging, and it has to be + * paired with every read of a registry entry. + * + * The `isFile()` rejection can only run once `fs.open` has returned, and a + * co-tenant who can name `.json` in the shared directory can make it + * a FIFO — a blocking `O_RDONLY` open on one waits for a writer that never + * arrives, which hangs `qwen sessions ps`, hangs a session's own startup + * registration when the FIFO sits at its PID, and saturates libuv's + * four-thread fs pool a few entries in. On the regular files this is + * actually for it does nothing. + * + * Absent on Windows and read at the call site, both for the same reasons + * as {@link noFollowFlag}. + */ +function nonBlockingFlag(): number { + return fsSync.constants.O_NONBLOCK ?? 0; +} + +/** The flags every read of a registry entry opens with. */ +function readEntryFlags(): number { + return fsSync.constants.O_RDONLY | noFollowFlag() | nonBlockingFlag(); +} + /** A directory entry's identity, as observed through an open handle. */ interface EntryIdentity { dev: number; @@ -313,11 +337,20 @@ function enqueueWrite(op: () => Promise): Promise { * `lstatSync` rather than the async form because `assertCanCommit` is the * hook that runs *between* the last check and an irreversible `rename`; * an `await` there would reopen the very window this closes. + * + * `requireFile` is false for the one caller replacing an entry + * {@link readRecord} *refused*, where the entry's type is part of what + * made it unusable: a planted symlink or a stray directory is precisely + * what that write is there to clear, so only the identity is pinned. */ -function assertSameEntry(filePath: string, expected: EntryIdentity): void { +function assertSameEntry( + filePath: string, + expected: EntryIdentity, + requireFile: boolean, +): void { const stat = fsSync.lstatSync(filePath); if ( - !stat.isFile() || + (requireFile && !stat.isFile()) || stat.dev !== expected.dev || stat.ino !== expected.ino ) { @@ -398,46 +431,88 @@ export async function registerSession( return false; } + // The entry the replacing write below is allowed to overwrite, + // pinned by identity so the commit can refuse a swap. Left + // undefined only when nothing could be pinned at all. + let replacing: + | { entry: EntryIdentity; requireFile: boolean } + | undefined = existing + ? { entry: existing.entry, requireFile: true } + : undefined; + // `existing === null` covers two different situations and only one // of them is a free name: nothing is there, or something is there // that this code cannot honour (a planted symlink, a truncated // write, a future schema). Replacing the second is deliberate and // tested; claiming the first has to be exclusive. - if (existing === null && !(await entryExists(filePath))) { - // Claim the name with an operation the kernel makes exclusive, - // rather than reading "absent" and renaming over whatever - // arrived in between: two origins sharing `QWEN_HOME` and a PID - // number would both see the gap, and the later rename would - // silently replace the earlier live record. - const outcome = await linkRecordExclusive(filePath, record); - if (outcome === 'created') { - retiredPids.delete(pid); - return true; + if (existing === null) { + if (!(await entryExists(filePath))) { + // Claim the name with an operation the kernel makes exclusive, + // rather than reading "absent" and renaming over whatever + // arrived in between: two origins sharing `QWEN_HOME` and a PID + // number would both see the gap, and the later rename would + // silently replace the earlier live record. + const outcome = await linkRecordExclusive(filePath, record); + if (outcome === 'created') { + retiredPids.delete(pid); + return true; + } + // 'taken' — someone claimed it in between. Go round again and + // route whatever they wrote through the origin rule above, as + // if it had been there before we looked. + if (outcome === 'taken') continue; + // 'unsupported' — no hard links on this filesystem. Fall + // through to the replacing write, which is where this path has + // always been; the exclusivity gap is the price of the + // filesystem. + } else { + // Something unusable is there, and `readRecord` discarded its + // origin along with the bytes it refused. Being unreadable to + // us is not what makes a record ours: a live foreign record + // one schema version ahead reaches exactly this branch, and + // without the peek it would be clobbered silently, with no + // `onOriginConflict` — the outcome the origin rule above + // exists to prevent, arrived at by a different route. + const unusable = await inspectUnusableEntry(filePath); + if ( + unusable.origin !== null && + !isSameOrigin( + unusable.origin, + record.machineId, + record.pidNamespace, + ) + ) { + reportConflict(); + return false; + } + // An entry that cannot be attributed at all — unparseable, or + // past the read cap, so not something this code ever wrote — + // stays replaceable. Refusing it instead would strand + // registration permanently on one truncated write, and + // registration only ever runs at startup. + replacing = unusable.entry + ? { entry: unusable.entry, requireFile: false } + : undefined; } - // 'taken' — someone claimed it in between. Go round again and - // route whatever they wrote through the origin rule above, as if - // it had been there before we looked. - if (outcome === 'taken') continue; - // 'unsupported' — no hard links on this filesystem. Fall through - // to the replacing write, which is where this path has always - // been; the exclusivity gap is the price of the filesystem. } // Either a same-origin record is present — the recycled-PID // recovery path, where a predecessor died without unregistering — - // or hard links are unavailable. Replace it, but commit only if - // the directory entry is still the one that was validated. + // or an unusable entry is, or hard links are unavailable. Replace + // it, but commit only if the directory entry is still the one that + // was inspected. // // `noFollow` keeps a pre-planted `.json` symlink from // redirecting this write (and its forced 0600) to a file outside // the registry: the sandbox shares this directory across a trust // boundary, so the planting side is not hypothetical. + const pinned = replacing; await atomicWriteJSON(filePath, record, { mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true, - assertCanCommit: existing - ? () => assertSameEntry(filePath, existing.entry) + assertCanCommit: pinned + ? () => assertSameEntry(filePath, pinned.entry, pinned.requireFile) : undefined, }); retiredPids.delete(pid); @@ -455,6 +530,77 @@ export async function registerSession( }); } +/** + * What can still be learned about an entry {@link readRecord} refused: + * which inode it is, and — when the bytes parse that far — whose origin it + * claims. + * + * Both are things `readRecord` throws away along with the record it + * rejects, and both are things the write that replaces it needs: the + * identity to pin the commit against a swap, the origin to run the same + * rule a *readable* foreign record gets. Neither is load-bearing on its + * own — an unknown identity leaves the write unpinned, exactly where it + * was before, and an unattributable entry stays replaceable. + * + * Read through the same capped, non-following, non-blocking handle as + * `readRecord`, for the same reasons: a FIFO here would hang startup, and + * an unbounded read here would be the same allocation lever. + */ +async function inspectUnusableEntry(filePath: string): Promise<{ + entry: EntryIdentity | null; + origin: Pick | null; +}> { + let entry: EntryIdentity | null = null; + try { + const stat = await fs.lstat(filePath); + entry = { dev: stat.dev, ino: stat.ino }; + } catch { + return { entry: null, origin: null }; + } + + let handle: fs.FileHandle; + try { + handle = await fs.open(filePath, readEntryFlags()); + } catch { + // A symlink (`O_NOFOLLOW` → ELOOP), a directory, a device: nothing + // that can carry an origin claim. + return { entry, origin: null }; + } + let raw: string | null; + try { + const stat = await handle.stat(); + if (!stat.isFile()) return { entry, origin: null }; + raw = await readCapped(handle); + } catch { + return { entry, origin: null }; + } finally { + await handle.close().catch(() => {}); + } + if (raw === null) return { entry, origin: null }; + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return { entry, origin: null }; + } + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + return { entry, origin: null }; + } + const value = parsed as Record; + const machineId = value['machineId']; + const pidNamespace = value['pidNamespace']; + // Absent reads as `undefined` here, which neither arm accepts — a body + // that makes no origin claim has not been attributed. + if ( + (typeof machineId !== 'string' && machineId !== null) || + (typeof pidNamespace !== 'string' && pidNamespace !== null) + ) { + return { entry, origin: null }; + } + return { entry, origin: { machineId, pidNamespace } }; +} + /** True when any directory entry exists at `filePath`, symlinks included. */ async function entryExists(filePath: string): Promise { try { @@ -568,7 +714,8 @@ export async function patchSessionRecord( mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true, - assertCanCommit: () => assertSameEntry(filePath, existing.entry), + assertCanCommit: () => + assertSameEntry(filePath, existing.entry, true), }, ); } catch (error) { @@ -610,7 +757,7 @@ export async function unregisterSession( // pair rather than to the whole validating read — the same binding // the write paths get from `assertCanCommit`, minus a primitive the // platform does not offer. - assertSameEntry(filePath, existing.entry); + assertSameEntry(filePath, existing.entry, true); await fs.unlink(filePath); } catch (error) { if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; @@ -740,7 +887,7 @@ export async function listLiveSessions( // itself stale is the only one this may remove, so a co-tenant // who swaps a live foreign record into the name after the read // does not get it deleted on their behalf. - assertSameEntry(filePath, read.entry); + assertSameEntry(filePath, read.entry, true); await fs.unlink(filePath); } catch { // Raced with another session's sweep, replaced under us, or not @@ -764,32 +911,65 @@ export async function listLiveSessions( * path still resolves to the inode that was validated (see * {@link assertSameEntry}). */ +/** + * Read at most {@link MAX_RECORD_BYTES} through an already-open handle. + * Returns null when the entry holds more than that. + * + * The `stat.size` check at the call site is a cheap early reject, not the + * ceiling. It and a `readFile()` to EOF are two separate observations of + * an inode a co-tenant can still be writing to, so an entry that passes + * the check at eleven bytes can be grown to hundreds of megabytes before + * the read runs — the cap becomes advisory and `qwen sessions ps`, which + * examines up to {@link MAX_RECORDS_PER_SCAN} attacker-named candidates + * per invocation, turns into a memory-exhaustion lever. Bounding the read + * makes what happens after the check irrelevant: the ceiling is enforced + * on the bytes this process actually allocates. + * + * The loop is for short reads, which `read()` is allowed to return at any + * point before EOF; the buffer is one byte past the cap so that filling it + * is itself the overflow signal. + */ +async function readCapped(handle: fs.FileHandle): Promise { + const buffer = Buffer.alloc(MAX_RECORD_BYTES + 1); + let filled = 0; + while (filled < buffer.length) { + const { bytesRead } = await handle.read( + buffer, + filled, + buffer.length - filled, + filled, + ); + if (bytesRead === 0) break; + filled += bytesRead; + } + if (filled > MAX_RECORD_BYTES) return null; + return buffer.toString('utf8', 0, filled); +} + async function readRecord(filePath: string): Promise { - let raw: string; let entry: EntryIdentity; let handle: fs.FileHandle; try { - handle = await fs.open( - filePath, - fsSync.constants.O_RDONLY | noFollowFlag(), - ); + handle = await fs.open(filePath, readEntryFlags()); } catch { return null; } + let bytes: string | null; try { const stat = await handle.stat(); if (!stat.isFile() || stat.size > MAX_RECORD_BYTES) return null; entry = { dev: stat.dev, ino: stat.ino }; - raw = await handle.readFile('utf8'); + bytes = await readCapped(handle); } catch { return null; } finally { await handle.close().catch(() => {}); } + if (bytes === null) return null; let parsed: unknown; try { - parsed = JSON.parse(raw); + parsed = JSON.parse(bytes); } catch { return null; } From a5bac99c96f050df9990cc997596c7aab60e6d2f Mon Sep 17 00:00:00 2001 From: qqqys Date: Mon, 10 Aug 2026 16:10:59 +0800 Subject: [PATCH 12/17] fix(core): close the three Criticals from review round 7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R7-1: two `readPidNamespaceId` tests asserted the unreadable sentinel with no platform guard. Off Linux the function returns null as its first statement, so the mocks never ran and both tests failed deterministically — the macOS and Windows merge-queue jobs, which run the suite unfiltered, would have gone red on this PR. R7-7: registration's replacing write went through atomicWriteFile's uid-preserving in-place fallback, which opens the target O_WRONLY and cannot write a 0600 file owned by another uid. A `sudo qwen` predecessor at a recycled PID therefore blacked out its successor's registration silently, for the session's whole life, with no onOriginConflict. A `.json` is a slot, not a document with an author: added `preserveOwner` to AtomicWriteFileOptions and set it false here, so the write replaces the entry via rename instead of writing into it. R7-19: a concurrent sweep unlinking the stale predecessor mid-write turned the commit assertion's ENOENT into a permanent registration failure — the catch sat outside the two-pass loop, so the now-free name was never re-examined. The replace path now retries the decision on a lost race, matching the create path's 'taken' → continue; non-race errors still fall through. Each fix is pinned by a test that fails when it is reverted: the darwin probe (null, not the sentinel), a new-inode assertion on a foreign-uid predecessor, and a sweep landing between the staged write and the commit. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/services/session-registry.test.ts | 119 +++++++++++++++++- .../core/src/services/session-registry.ts | 79 ++++++++++-- .../core/src/utils/atomicFileWrite.test.ts | 37 ++++++ packages/core/src/utils/atomicFileWrite.ts | 27 +++- .../core/src/utils/process-liveness.test.ts | 7 ++ 5 files changed, 258 insertions(+), 11 deletions(-) diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 4e4f8a4e136..1b7c6a37985 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -53,8 +53,31 @@ vi.mock('../utils/process-liveness.js', async (importOriginal) => { */ const statSizeLie = vi.hoisted(() => ({ value: null as number | null })); +/** + * Lets a test land another session's sweep in the one window a fixture + * cannot reach: after a replacing write has staged its temp file and + * before it commits, which is where the entry it pinned can still be + * unlinked out from under it. Armed with the path to remove; fires once. + */ +const sweepBeforeCommit = vi.hoisted(() => ({ value: null as string | null })); + vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal(); + const writeFile: typeof actual.writeFile = async (...args) => { + const result = await actual.writeFile(...args); + const victim = sweepBeforeCommit.value; + // Only the staged temp file: the arming test's own fixture write goes + // through here too, and disarming on it would fire in the wrong place. + if ( + victim !== null && + typeof args[0] === 'string' && + args[0].endsWith('.tmp') + ) { + sweepBeforeCommit.value = null; + await actual.rm(victim, { force: true }); + } + return result; + }; const open: typeof actual.open = async (...args) => { const handle = await actual.open(...args); const realStat = handle.stat.bind(handle); @@ -69,7 +92,12 @@ vi.mock('node:fs/promises', async (importOriginal) => { }; return handle; }; - return { ...actual, default: { ...actual, open }, open }; + return { + ...actual, + default: { ...actual, open, writeFile }, + open, + writeFile, + }; }); vi.mock('../config/storage.js', () => { @@ -99,6 +127,7 @@ beforeEach(async () => { afterEach(async () => { statSizeLie.value = null; + sweepBeforeCommit.value = null; await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -339,6 +368,94 @@ describe('registerSession', () => { }); }); + it.skipIf( + process.platform === 'win32' || typeof process.geteuid !== 'function', + )( + 'takes over a same-origin record another user owns, rather than writing into it', + async () => { + // `sudo qwen` against the same HOME — a deployment this module's + // threat model already names — leaves a 0600 record owned by root + // at that PID. It is same-origin, so the recovery path above + // applies, but a write that preserved the predecessor's inode + // could not open it: registration would fail silently, and + // nothing re-runs it. Replacement is what the entry is for. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: '1', + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + sessionId: 's-predecessor', + cwd: '/w/before', + name: 'before-aa', + kind: 'interactive', + startedAt: 1000, + }); + await fs.chmod(filePath, 0o600); + const before = await fs.stat(filePath); + + // The fixture cannot be given a foreign uid without root, so the + // comparison the decision actually reads is moved instead. + const realGeteuid = process.geteuid!; + process.geteuid = () => before.uid + 1; + try { + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + }), + ).toBe(true); + } finally { + process.geteuid = realGeteuid; + } + + // A new inode is the proof: the in-place path preserves it, and is + // the one that cannot work on a file this process does not own. + expect((await fs.stat(filePath)).ino).not.toBe(before.ino); + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-ours', + cwd: '/w/ours', + }); + }, + ); + + it('re-decides when a sweep unlinks the record it was replacing', async () => { + // The replace path's version of the create path's lost race. Another + // session's `qwen sessions ps` legitimately sweeps the stale + // predecessor while this write sits between validation and commit; + // the commit assertion then finds nothing. Giving up there costs the + // session its entire lifetime on the register, because registration + // is startup-only and nothing else writes the record. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: '1', + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + sessionId: 's-predecessor', + cwd: '/w/before', + name: 'before-aa', + kind: 'interactive', + startedAt: 1000, + }); + sweepBeforeCommit.value = filePath; + + expect( + await registerSession({ + sessionId: 's-ours', + cwd: '/w/ours', + kind: 'interactive', + }), + ).toBe(true); + + // Second pass sees a free name and claims it exclusively. + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 's-ours', + cwd: '/w/ours', + }); + }); + it('reports an origin conflict to the caller', async () => { const filePath = await writeRaw(`${process.pid}.json`, { schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 79d7fd3fdec..3be5a6ea61b 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -354,12 +354,40 @@ function assertSameEntry( stat.dev !== expected.dev || stat.ino !== expected.ino ) { - throw new Error( + throw new EntryChangedError( `session registry entry ${filePath} changed between validation and write`, ); } } +/** + * The pinned entry is not the one that was validated: it was swapped for + * another inode, or replaced by a directory or a link. + * + * A named type rather than a bare `Error` so a caller can tell this apart + * from the I/O errors a write raises for its own reasons — the two want + * opposite responses, retry the decision versus give up on it. + */ +class EntryChangedError extends Error {} + +/** + * Whether a failed commit assertion means the name is simply no longer + * what it was — swapped ({@link EntryChangedError}) or gone (`ENOENT`, + * thrown by `assertSameEntry`'s own `lstatSync`). + * + * Both say the same thing to a writer that pinned an entry: the decision + * that chose a replacing write was made about a directory entry that no + * longer exists, so it has to be made again rather than reported as a + * failure. Only ever consulted for errors raised *inside* the assertion, + * so an `ENOENT` from elsewhere in the write is not mistaken for this. + */ +function isEntryRace(error: unknown): boolean { + return ( + error instanceof EntryChangedError || + (error as NodeJS.ErrnoException)?.code === 'ENOENT' + ); +} + /** * Write this process's record. Best-effort: a read-only or full home * directory must not stop a session from starting, so failures are logged @@ -507,14 +535,47 @@ export async function registerSession( // the registry: the sandbox shares this directory across a trust // boundary, so the planting side is not hypothetical. const pinned = replacing; - await atomicWriteJSON(filePath, record, { - mode: REGISTRY_FILE_MODE, - forceMode: true, - noFollow: true, - assertCanCommit: pinned - ? () => assertSameEntry(filePath, pinned.entry, pinned.requireFile) - : undefined, - }); + // Set by the assertion below, and only by it: a commit that failed + // because the pinned entry moved is a lost race, not a failed + // write, and the two leave through the same `catch`. + let raced = false; + try { + await atomicWriteJSON(filePath, record, { + mode: REGISTRY_FILE_MODE, + forceMode: true, + noFollow: true, + // `.json` is a name for a slot, not a document with an + // author: whoever holds that PID now owns the entry, and the + // replacement is the whole point of this branch. Preserving + // the predecessor's uid instead would write in place, which + // is EACCES on the 0600 record a root-run session leaves + // behind — registration would fail for a name this process + // is entitled to, silently and for its whole lifetime. + preserveOwner: false, + assertCanCommit: pinned + ? () => { + try { + assertSameEntry(filePath, pinned.entry, pinned.requireFile); + } catch (error) { + raced = isEntryRace(error); + throw error; + } + } + : undefined, + }); + } catch (error) { + if (!raced) throw error; + // The replace path's counterpart to the create path's 'taken' + // above: a concurrent sweep legitimately unlinked the stale + // predecessor, so the name this write was going to replace is + // free (or holds a record that arrived since). Go round again + // and decide about what is there now — the exclusive create + // will claim it. Without this the ENOENT reached the outer + // catch and registration, which only ever runs at startup, + // returned false: a session absent from `qwen sessions ps` for + // its entire life, with no `onOriginConflict` to explain it. + continue; + } retiredPids.delete(pid); return true; } diff --git a/packages/core/src/utils/atomicFileWrite.test.ts b/packages/core/src/utils/atomicFileWrite.test.ts index 1422e14f3a3..d688363c971 100644 --- a/packages/core/src/utils/atomicFileWrite.test.ts +++ b/packages/core/src/utils/atomicFileWrite.test.ts @@ -329,6 +329,43 @@ describe('atomicWriteFile', () => { }, ); + it.skipIf( + process.platform === 'win32' || typeof process.geteuid !== 'function', + )( + 'replaces a foreign-uid target rather than writing in place under preserveOwner: false', + async () => { + // The fallback the sibling above pins is a dead end for callers + // whose file is a *slot* named after a resource: the in-place open + // is O_WRONLY on another uid's 0600 file, which is EACCES, and the + // write fails for a name the caller is entitled to claim. A new + // inode is the signal that the replacing rename ran instead. + const filePath = path.join(tmpDir, 'slot.json'); + await fs.writeFile(filePath, 'predecessor'); + await fs.chmod(filePath, 0o600); + + const realStat = await fs.stat(filePath); + const realGeteuid = process.geteuid!; + process.geteuid = () => realStat.uid + 1; + + try { + await atomicWriteFile(filePath, 'mine', { + mode: 0o600, + forceMode: true, + noFollow: true, + preserveOwner: false, + }); + } finally { + process.geteuid = realGeteuid; + } + + expect(await fs.readFile(filePath, 'utf-8')).toBe('mine'); + const statAfter = await fs.stat(filePath); + expect(statAfter.ino).not.toBe(realStat.ino); + expect(statAfter.mode & 0o777).toBe(0o600); + expect(await fs.readdir(tmpDir)).toEqual(['slot.json']); + }, + ); + it.skipIf( process.platform === 'win32' || typeof process.geteuid !== 'function', )( diff --git a/packages/core/src/utils/atomicFileWrite.ts b/packages/core/src/utils/atomicFileWrite.ts index a0e9e4f153d..4854d1d9716 100644 --- a/packages/core/src/utils/atomicFileWrite.ts +++ b/packages/core/src/utils/atomicFileWrite.ts @@ -48,6 +48,24 @@ export interface AtomicWriteFileOptions extends AtomicWriteOptions { * semantics. Default: false (follow symlinks). See PR #4333 review. */ noFollow?: boolean; + /** + * Keep the existing target's inode — and therefore its uid — when it is + * owned by another user, by writing in place instead of renaming over + * it. Default: true. + * + * Pass `false` where the file *is* the thing being replaced rather than + * a document being edited: a registry or lock entry named after a + * resource, whose contents belong to whoever holds the resource now. + * Preserving a stranger's inode there is not a courtesy, it is a write + * that cannot succeed — an in-place open of a 0600 file owned by + * another uid is EACCES, and the caller ends up unable to claim a name + * it is entitled to. Replacement needs write permission on the + * *directory*, which such callers own. + * + * No effect on {@link atomicWriteFileSync}, which has no ownership + * fallback: it always replaces. + */ + preserveOwner?: boolean; /** Reject the write immediately before its irreversible commit step. */ assertCanCommit?: () => void; } @@ -130,7 +148,9 @@ async function resolveSymlinkChain(filePath: string): Promise { * Atomically write content to a file (write-to-temp + rename). * * Falls back to in-place write when the existing file's uid differs - * from the process's euid — POSIX rename would reset ownership. + * from the process's euid — POSIX rename would reset ownership — unless + * the caller passes `preserveOwner: false`, which keeps the replacing + * rename for entries whose owner is not part of what is being written. * Also falls back on EXDEV (cross-device). Both fallbacks lose crash * atomicity but preserve the existing inode's uid. * @@ -254,6 +274,11 @@ export async function atomicWriteFile( // directory's GID for new files, making egid !== file.gid a // false positive on the most common dev platform. const ownershipWouldChange = (): boolean => { + // `preserveOwner: false` is a caller saying the entry's owner is not + // part of what it is writing (see the option's doc). Deciding it here + // keeps the fallback's own invariants — validated inode, no re-resolved + // path — intact for everyone who does preserve ownership. + if (options?.preserveOwner === false) return false; if (existingStat === undefined) return false; if (process.platform === 'win32') return false; const euid = process.geteuid?.(); diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index aa215e08252..4a51d4693f3 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -235,6 +235,12 @@ describe('readPidNamespaceId', () => { }); it('reports an unreadable link as unprovable, not as "no namespaces"', () => { + // Same guard as the sibling above, and for a stronger reason than + // "the fixture is Linux-shaped": `readPidNamespaceId` returns null + // before it reads anything at all off Linux, so the mock below is + // never reached and the assertion is against the wrong platform's + // answer. Without this, the macOS and Windows CI jobs go red. + if (process.platform !== 'linux') return; nsReadFails.value = true; try { // Specifically NOT null. null is the claim "this platform has no PID @@ -251,6 +257,7 @@ describe('readPidNamespaceId', () => { }); it('reports a link whose target does not parse as unprovable too', () => { + if (process.platform !== 'linux') return; nsLinkTarget.value = 'pid:[not-an-inode]'; try { expect(readPidNamespaceId()).toBe(PID_NAMESPACE_UNREADABLE); From 01d9d6a0ef3649ee8a22dcb2282c1c5c4bea7c1f Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 11 Aug 2026 14:49:37 +0800 Subject: [PATCH 13/17] fix(core): clear a non-directory squatting on the registry dir path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A regular file planted at `~/.qwen/sessions` permanently disabled the whole registry. `mkdir(recursive)` throws against it, nothing else in the module ever created that directory so nothing would ever clear it, and `listLiveSessions`' readdir failed with ENOTDIR into its catch-all and reported an empty machine. Under this module's own threat model — a co-tenant with write access to the shared qwen dir — that is two syscalls for a permanent registration blackout, the silent failure mode the directory's own comments call out as the wrong one. `registerSession` now lstats the path when mkdir fails and, if what is there is not a directory, unlinks it and retries once. That is the rule already applied one level down, where an unattributable entry at `.json` is replaceable: a non-directory at a path that must be a directory carries no record anyone could lose. A directory found there means mkdir failed for another reason (EACCES on a parent) and is rethrown to the existing best-effort handler, which still reports false. Scope was measured, not assumed: mkdir throws EEXIST on a regular file and on a symlink to one, and ENOENT on a dangling symlink — all three are repaired. It succeeds on a symlink resolving to a real directory, so that case never reaches this path and is documented as unaddressed rather than implied fixed; refusing it belongs with the directory's own hardening. Tests: three cases that each fail without the fix (planted file, dangling symlink, symlink to a file — the last also pinning that the unlink takes the link and never its target), plus a control that a healthy directory and a sibling's record are left untouched. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/services/session-registry.test.ts | 86 +++++++++++++++++++ .../core/src/services/session-registry.ts | 61 ++++++++++++- 2 files changed, 146 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 1b7c6a37985..22b409390e2 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -223,6 +223,92 @@ describe('registerSession', () => { expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); }); + it('clears a file planted at the registry directory path and registers', async () => { + // The co-tenant's two syscalls: move the directory aside, drop a + // regular file at its name. Without a repair path this is a permanent + // registration blackout — mkdir throws EEXIST here forever after. + const dir = getSessionRegistryDir(); + await fs.writeFile(dir, 'not a directory'); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + expect((await fs.lstat(dir)).isDirectory()).toBe(true); + const live = await listLiveSessions({ includeSelf: true }); + expect(live).toHaveLength(1); + expect(live[0].sessionId).toBe('s1'); + }); + + it('clears a dangling symlink planted at the registry directory path', async () => { + // Fails mkdir with ENOENT rather than EEXIST, so it only gets repaired + // if the branch keys on what is at the path rather than on the errno. + const dir = getSessionRegistryDir(); + await fs.symlink(path.join(tmpDir, 'nowhere'), dir); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + expect((await fs.lstat(dir)).isDirectory()).toBe(true); + expect(await listLiveSessions({ includeSelf: true })).toHaveLength(1); + }); + + it('clears a symlink to a file without following it', async () => { + // The unlink must remove the link, never the thing it points at: a + // repair that followed would delete an arbitrary attacker-named path. + const dir = getSessionRegistryDir(); + const target = path.join(tmpDir, 'victim'); + await fs.writeFile(target, 'must survive'); + await fs.symlink(target, dir); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + expect((await fs.lstat(dir)).isDirectory()).toBe(true); + expect(await fs.readFile(target, 'utf8')).toBe('must survive'); + }); + + it('leaves an existing registry directory and its records alone', async () => { + // The repair is reachable only through a failed mkdir. If it ever fired + // on the healthy path it would unlink the directory every other live + // session is registered in. + const sibling = await writeRaw('4242.json', { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: 4242, + sessionId: 'sibling', + cwd: '/w/other', + name: 'other-aa', + kind: 'interactive', + startedAt: Date.now(), + pidNamespace: readPidNamespaceId(), + procStart: null, + }); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + await expect(fs.stat(sibling)).resolves.toBeDefined(); + }); + it('records a start-time token so a recycled pid cannot resurrect it', async () => { await registerSession({ sessionId: 's1', diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 3be5a6ea61b..f164e47d2d2 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -388,6 +388,65 @@ function isEntryRace(error: unknown): boolean { ); } +/** + * Create the registry directory, clearing a non-directory squatting on its + * path first. + * + * `mkdir(recursive)` is a no-op when the directory exists and throws when + * anything else does, so a plain file — or a symlink that resolves to one, + * or to nothing — planted at `~/.qwen/sessions` fails every registration + * from here on, and `listLiveSessions`' `readdir` fails with `ENOTDIR` into + * its catch-all and reports an empty machine. Nothing else in this module + * creates the directory, so nothing else would ever clear it: the blackout + * would last until a human deleted the file by hand. Under this module's + * threat model — a co-tenant with write access to the shared qwen dir — + * that is two syscalls for a permanent denial of discovery. + * + * An obstruction is unlinked and the mkdir retried exactly once. That is + * the same rule `registerSession` already applies one level down, where an + * unattributable entry at `.json` is replaceable: a non-directory at + * a path that must be a directory carries no record anyone could lose. + * + * `lstat`, not `stat`, so a symlink is judged as the symlink it is rather + * than by what it points at — following one would let it decide the verdict + * on a directory somewhere else entirely. A directory found here means the + * mkdir failed for some other reason (`EACCES` on a parent, most likely), + * which is not an obstruction and is rethrown to the caller's own handler. + * + * Scope, measured rather than assumed: `mkdir(recursive)` throws `EEXIST` + * on a regular file and on a symlink to one, and `ENOENT` on a dangling + * symlink — all three are repaired here. It *succeeds* on a symlink that + * resolves to a real directory, so that case never reaches this function + * and is not addressed by it; records would be written through the link. + * Refusing it belongs with the directory's own hardening (an `O_NOFOLLOW` + * open of the dir, or an `lstat` gate on the healthy path), not with a + * repair that only ever runs after a failure, and it trades against users + * who deliberately symlink the qwen dir onto another disk. + */ +async function ensureRegistryDir(dir: string): Promise { + try { + await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); + return; + } catch (error) { + let obstruction: fsSync.Stats; + try { + obstruction = await fs.lstat(dir); + } catch { + // Nothing there to blame the failure on — it vanished under us, or + // the parent is unreadable. Either way this is not the case being + // repaired. + throw error; + } + if (obstruction.isDirectory()) throw error; + + debugLogger.debug( + `session registry: clearing a non-directory at ${dir} (mode ${obstruction.mode.toString(8)})`, + ); + await fs.unlink(dir); + await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); + } +} + /** * Write this process's record. Best-effort: a read-only or full home * directory must not stop a session from starting, so failures are logged @@ -421,7 +480,7 @@ export async function registerSession( return enqueueWrite(async () => { try { const dir = getSessionRegistryDir(); - await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); + await ensureRegistryDir(dir); // mkdir's mode is masked by the umask, and does nothing at all when // the directory already exists — chmod is what actually guarantees // 0700 on an upgrade from a build that created it more loosely. From 36b987afaa80364601eafa2de5b741f1a4e0f942 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 11 Aug 2026 15:11:40 +0800 Subject: [PATCH 14/17] fix(cli): stop ellipsizing names that fit, and close three test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses wenshao's review batch on #8728. `sessions ps` truncated the NAME cell to `NAME_COL - 2`, folding the column gutter into the content budget. A name of display width 21-22 fits its cell and was ellipsized anyway, and what the ellipsis ate was the hash suffix `deriveSessionName` appends — the one part of the name that distinguishes two sessions started in the same directory, and the one part that cannot be inferred from the DIRECTORY column beside it. The suggested one-liner (budget = NAME_COL, cell = NAME_COL) fixes that band by regressing the common one: `deriveSessionName` caps its basename at 32, so names long enough to truncate are the norm, and every one of them would then fill the cell exactly and touch the PID digits. Cells are now joined by an explicit space and truncated to the full column width, which is what sibling `sessions list` already does — the gutter survives for every row and the 21-22 band renders whole. Test gaps, each verified by mutation: - `gemini.test.tsx` mocked `getProjectRoot` and `getTargetDir` to the same `/root`, so the registerSession contract test could not tell which one production read. Swapping the production call to `getProjectRoot()` kept the suite green; with the mocks given distinct values it fails (8 tests). - The "never registered" test asserted only the list output. A stub record written past the `existing === null` guard is rejected by `readRecord`, so the list stays empty while the stub sits at `.json` forever, never swept and invisible to readers. It now also asserts the filesystem. The registry directory has to exist first for this to bite at all — without it the stub write fails with ENOENT and the guard's removal is unobservable, which is why the first version of this assertion passed under mutation. - The multi-width truncation row hardcoded its PID/AGE padding, failing on column-width changes it does not test, and derived its age cell from wall-clock — a >30s stall between building the record and the handler's `Date.now()` rendered "2m" and failed spuriously. Paddings now come from the exported constants and the clock is frozen (Date only, so the handler's own awaits still run). Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/sessions/ps.test.ts | 60 +++++++++++++++--- packages/cli/src/commands/sessions/ps.ts | 35 ++++++++--- packages/cli/src/gemini.test.tsx | 11 +++- .../src/services/session-registry.test.ts | 63 ++++++++++++++++++- 4 files changed, 148 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index 7268a2eebb5..110b18d572d 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -21,7 +21,9 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ writeStderrLine: (line: string) => stderr.push(line), })); -const { psCommand, formatAge, NAME_COL } = await import('./ps.js'); +const { psCommand, formatAge, NAME_COL, PID_COL, AGE_COL } = await import( + './ps.js' +); function record( over: Partial = {}, @@ -52,9 +54,17 @@ beforeEach(() => { stdout.length = 0; stderr.length = 0; listLiveSessions.mockReset(); + // Only Date: the age cell is rendered from `Date.now()` read inside the + // handler, against a `startedAt` this file computes when it builds the + // record, so any real delay between the two shifts the rendered age and + // fails an exact-row assertion for a reason that has nothing to do with + // what the test covers. Faking the timer queue too would stall the + // handler's own awaits, so the fake is scoped to the clock. + vi.useFakeTimers({ toFake: ['Date'] }); }); afterEach(() => { + vi.useRealTimers(); vi.restoreAllMocks(); }); @@ -149,19 +159,51 @@ describe('qwen sessions ps', () => { expect(stdout[1]).toContain('4242'); }); + it('renders a name that exactly fills the column without an ellipsis', async () => { + // The band a budget of NAME_COL - 2 got wrong: a name this long fits + // its cell, and ellipsizing it both claims a truncation that did not + // happen and eats the hash suffix that distinguishes two sessions + // started in the same directory. + const name = 'a'.repeat(NAME_COL - 3) + '-7f'; + expect(name).toHaveLength(NAME_COL); + listLiveSessions.mockResolvedValue([record({ name })]); + await run({ json: false }); + + expect(stdout[1]).toContain(name); + expect(stdout[1]).not.toContain('...'); + // Still a column, not a collision: the cell keeps a separator from the + // PID beside it even when the name uses every one of its columns. + expect(stdout[1]).toContain(`${name} 4242`); + }); + it('cuts a multi-width name on a character boundary, not a column one', async () => { - // 15 full-width characters is 30 display columns against a 20-column - // budget. Subtracting the three columns "..." costs leaves 17: the - // eighth character ends at column 16, and a ninth would straddle the - // limit, so the cut lands at eight characters for a 19-column cell. - // Asserting the cell exactly is what pins the accumulation loop — - // "contains ..." survives a loop that copies nothing at all. + // 15 full-width characters is 30 display columns against the NAME_COL + // budget. Subtracting the three columns "..." costs leaves 19: the + // ninth character ends at column 18, and a tenth would straddle the + // limit, so the cut lands at nine characters. Asserting the cell + // exactly is what pins the accumulation loop — "contains ..." survives + // a loop that copies nothing at all. + // + // The sibling cells are derived from the exported widths rather than + // spelled out: they have nothing to do with truncation, and hardcoding + // their padding would fail this test for a column-width change it does + // not test. The age is pinned by the frozen clock in `beforeEach`, not + // by wall time — read from `Date.now()` inside the handler, a real + // delay between the record's `startedAt` and that call would render + // "2m" and fail here for a reason that is not truncation. listLiveSessions.mockResolvedValue([record({ name: '中'.repeat(15) })]); await run({ json: false }); - const cell = '中'.repeat(8) + '...'; + const cell = '中'.repeat(9) + '...'; + const pad = (text: string, width: number) => + text + ' '.repeat(width - text.length); expect(stdout[1]).toBe( - cell + ' '.repeat(NAME_COL - 19) + '4242 ' + '1m ' + '/w/app', + [ + cell + ' '.repeat(NAME_COL - 21), + pad('4242', PID_COL), + pad('1m', AGE_COL), + '/w/app', + ].join(' '), ); }); diff --git a/packages/cli/src/commands/sessions/ps.ts b/packages/cli/src/commands/sessions/ps.ts index 013445de6ce..98a7d131f0e 100644 --- a/packages/cli/src/commands/sessions/ps.ts +++ b/packages/cli/src/commands/sessions/ps.ts @@ -21,7 +21,21 @@ import stringWidth from 'string-width'; import { escapeAnsiCtrlCodes } from '../../ui/utils/textUtils.js'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -/** Fixed column widths for the human-readable table (exported for tests). */ +/** + * Fixed column widths for the human-readable table (exported for tests). + * + * These are content widths, and cells are joined by an explicit space + * rather than relying on padding to leave one — the same shape as sibling + * `sessions list`. Folding the gutter into the width instead (truncating + * to `NAME_COL - 2`) costs two columns on *every* row to protect the rare + * full-width one, and `deriveSessionName` caps its basename at 32, so a + * name long enough to truncate is the common case rather than the edge: + * every one of them would lose two more characters, and a 21–22 column + * name would be ellipsized while its cell sat two columns empty. The + * suffix those two columns eat is the hash that tells two sessions in the + * same directory apart, which is the one part of the name that cannot be + * inferred from the DIRECTORY column beside it. + */ export const NAME_COL = 22; export const PID_COL = 9; export const AGE_COL = 10; @@ -84,17 +98,20 @@ export function formatAge(ms: number): string { function outputHuman(records: SessionRegistryRecord[], now: number): void { writeStdoutLine( - padDisplay('NAME', NAME_COL) + - padDisplay('PID', PID_COL) + - padDisplay('AGE', AGE_COL) + - 'DIRECTORY', + `${padDisplay('NAME', NAME_COL)} ${padDisplay('PID', PID_COL)} ${padDisplay( + 'AGE', + AGE_COL, + )} DIRECTORY`, ); for (const record of records) { writeStdoutLine( - padDisplay(truncate(sanitize(record.name), NAME_COL - 2), NAME_COL) + - padDisplay(String(record.pid), PID_COL) + - padDisplay(formatAge(now - record.startedAt), AGE_COL) + - sanitize(record.cwd), + `${padDisplay( + truncate(sanitize(record.name), NAME_COL), + NAME_COL, + )} ${padDisplay(String(record.pid), PID_COL)} ${padDisplay( + formatAge(now - record.startedAt), + AGE_COL, + )} ${sanitize(record.cwd)}`, ); } } diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 22e35c23165..31661597347 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -2499,9 +2499,16 @@ describe('validateDnsResolutionOrder', () => { describe('startInteractiveUI', () => { // Mock dependencies const mockConfig = { + // Deliberately different from getTargetDir. Production registers + // `cwd: config.getTargetDir()`; with both getters returning the same + // string the registerSession contract assertion below cannot tell + // which one it read, and a refactor swapping in getProjectRoot() would + // keep this whole suite green while `qwen sessions ps` silently showed + // the git project root instead of the session's working directory + // whenever qwen was launched from a subdirectory. getProjectRoot: () => '/root', getSessionId: () => 'test-session-id', - getTargetDir: () => '/root', + getTargetDir: () => '/root/work', getScreenReader: () => false, isTelemetryInitializationDeferred: () => true, getChatRecordingService: () => undefined, @@ -3315,7 +3322,7 @@ describe('startInteractiveUI', () => { expect(registerSession).toHaveBeenCalledTimes(1); expect(registerSession).toHaveBeenCalledWith({ sessionId: 'test-session-id', - cwd: '/root', + cwd: '/root/work', kind: 'interactive', qwenVersion: '1.0.0', onOriginConflict: expect.any(Function), diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 22b409390e2..7dba08c30af 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -61,8 +61,33 @@ const statSizeLie = vi.hoisted(() => ({ value: null as number | null })); */ const sweepBeforeCommit = vi.hoisted(() => ({ value: null as string | null })); +/** + * Lets a test make `readdir` fail the way an unreadable registry directory + * does, without depending on the uid the suite runs as. A chmod-based + * fixture proves nothing under root — which is what CI containers and this + * repo's own sandbox commonly run as — and would silently degrade to a + * no-op assertion there. Armed with an errno; fires once. + */ +const readdirFails = vi.hoisted(() => ({ value: null as string | null })); + vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal(); + // Cast rather than annotated: `readdir` is an overload set whose return + // type varies with `withFileTypes`, and a single async wrapper satisfies + // none of the signatures directly. The wrapper is pass-through, so the + // real types still hold at every call site. + const readdir = (async (...args: unknown[]) => { + const code = readdirFails.value; + if (code !== null) { + readdirFails.value = null; + const error: NodeJS.ErrnoException = new Error( + `${code}: permission denied, scandir`, + ); + error.code = code; + throw error; + } + return (actual.readdir as (...a: unknown[]) => Promise)(...args); + }) as unknown as typeof actual.readdir; const writeFile: typeof actual.writeFile = async (...args) => { const result = await actual.writeFile(...args); const victim = sweepBeforeCommit.value; @@ -94,9 +119,10 @@ vi.mock('node:fs/promises', async (importOriginal) => { }; return { ...actual, - default: { ...actual, open, writeFile }, + default: { ...actual, open, writeFile, readdir }, open, writeFile, + readdir, }; }); @@ -128,6 +154,7 @@ beforeEach(async () => { afterEach(async () => { statSizeLie.value = null; sweepBeforeCommit.value = null; + readdirFails.value = null; await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -825,8 +852,27 @@ describe('patchSessionRecord', () => { }); it('does not create a record for a session that never registered', async () => { + // The directory has to already exist, and that is not scene-setting: + // it is what makes the failure reachable. With no `sessions/` at all + // the stub write fails with ENOENT into `patchSessionRecord`'s catch, + // so the guard could be gone and nothing would be written either way. + // Production almost always has the directory — any other session on + // the machine creates it — so the state this pins is the normal one, + // not the empty-machine one. + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + await patchSessionRecord({ sessionId: 'new' }); expect(await listLiveSessions({ includeSelf: true })).toEqual([]); + // The list assertion alone cannot see this. Drop the `existing === null` + // guard and the merge writes a stub `{sessionId: 'new'}` with no + // schemaVersion, kind or startedAt; `readRecord` rejects it, so the + // list is still empty and this test would stay green while the stub + // sat at `.json` forever — never swept, because a record that + // fails to read is skipped by the sweep, and invisible to every + // reader. Production reaches that state whenever startup registration + // fails (the best-effort path startInteractiveUI deliberately allows) + // and a later `/clear` or cwd change patches anyway. + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); }); it('leaves a record from another origin unmerged', async () => { @@ -952,6 +998,21 @@ describe('listLiveSessions', () => { expect(await listLiveSessions({ includeSelf: true })).toEqual([]); }); + it('throws rather than reporting an empty machine it could not read', async () => { + // "Nothing is running" and "I could not look" are different facts, and + // a diagnostic command that renders them identically is the one that + // gets believed. ENOENT stays an empty list — see the test above — but + // EACCES is the registry directory re-created by another uid, a + // restrictive NFS export, or a sandbox uid mapping, and `qwen sessions + // ps` has to be able to say so and exit non-zero. + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + readdirFails.value = 'EACCES'; + + await expect(listLiveSessions({ includeSelf: true })).rejects.toThrow( + /EACCES/, + ); + }); + it('sweeps a record whose process is gone', async () => { const filePath = await writeRaw(`${DEAD_PID}.json`, { schemaVersion: 1, From 65661f047c29114aa406ee5d8c1a0b427fd53f6d Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 11 Aug 2026 15:12:05 +0800 Subject: [PATCH 15/17] fix(core): let listLiveSessions report that it could not look MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `listLiveSessions` swallowed every non-ENOENT readdir failure into a debug log and returned `[]`, so "nothing is running on this machine" and "this process could not read the registry" rendered identically. For a diagnostic command that is the answer most likely to be believed and least likely to be true: with the directory re-created by another uid, a restrictive NFS export, or a sandbox uid mapping, `qwen sessions ps` printed "No other Qwen Code sessions are running." and exited 0. It also made `ps`'s own error branch dead code — its test passed only by mocking a rejection the real function could never produce, which blessed an unreachable path. ENOENT keeps returning `[]`, because that one is an answer: no session has ever registered here. Everything else now throws, and `ps` already catches, prints the reason and exits non-zero. `ps` is the only production caller on this branch, so nothing else changes shape. The new test injects the errno through the fs mock rather than chmod'ing the directory: a permission fixture proves nothing when the suite runs as root, which CI containers and this repo's sandbox commonly do, and would have degraded to a silently passing assertion there. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/services/session-registry.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index f164e47d2d2..e70559f486f 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -920,10 +920,20 @@ export async function listLiveSessions( try { entries = await fs.readdir(dir); } catch (error) { - if ((error as NodeJS.ErrnoException)?.code !== 'ENOENT') { - debugLogger.debug(`listLiveSessions readdir failed: ${describe(error)}`); - } - return []; + // ENOENT is an answer: no session has ever registered on this machine, + // so the empty list is the truth. Nothing else is. An EACCES — the + // directory re-created by another uid, a restrictive NFS export, a + // sandbox uid mapping — means this process could not look, and + // returning `[]` for it reports "no sessions are running" with exactly + // the same confidence. For a diagnostic command that is the answer + // most likely to be believed and least likely to be true. + // + // The caller is what knows how to say "I could not look": `qwen + // sessions ps` already catches, prints the reason and exits non-zero. + // Swallowing here is what made that branch unreachable, so its test + // could only pass by mocking a rejection this function never produced. + if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return []; + throw error; } // Read once per enumeration, not once per record: a process cannot From 375bb561ec48270a7f9b023d5cbf8a972c5f6bfe Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:05:47 +0800 Subject: [PATCH 16/17] fix(core): clear directory blocking session registration Handle the unresolved Critical on #8728 by removing an unattributable directory at the current PID record path before retrying the existing exclusive registration flow. Cover a non-empty planted directory so the permanent registration blackout cannot regress. Verified: packages/core session-registry 67 passed; core typecheck and build; targeted ESLint and Prettier. --- .../src/services/session-registry.test.ts | 20 +++++++++++++++++++ .../core/src/services/session-registry.ts | 18 +++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 7dba08c30af..48e8b2092aa 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -671,6 +671,26 @@ describe('registerSession', () => { }, ); + it('clears a directory planted at its record path and registers', async () => { + const recordPath = getSessionRecordPath(); + await fs.mkdir(recordPath, { recursive: true }); + await fs.writeFile(path.join(recordPath, 'obstruction'), 'planted'); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(true); + + expect((await fs.lstat(recordPath)).isFile()).toBe(true); + expect(JSON.parse(await fs.readFile(recordPath, 'utf8'))).toMatchObject({ + sessionId: 's1', + cwd: '/w/app', + }); + }); + // Windows synthesizes st_mode from file attributes (a writable dir reads // 0o777, a file 0o666) and chmod there can only toggle the read-only bit, // so POSIX permission bits are not assertable on the test_windows gate. diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index e70559f486f..d38204b228d 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -572,6 +572,24 @@ export async function registerSession( reportConflict(); return false; } + if (unusable.origin === null && unusable.entry !== null) { + try { + const stat = await fs.lstat(filePath); + if (stat.isDirectory()) { + // `rename(2)` cannot replace a directory with the staged + // record. It cannot carry a JSON origin claim either, so + // clear it and re-decide about the now-free name. Pin the + // entry immediately before removal for the same reason as + // every other mutation in this module. + assertSameEntry(filePath, unusable.entry, false); + await fs.rm(filePath, { recursive: true }); + continue; + } + } catch (error) { + if (isEntryRace(error)) continue; + throw error; + } + } // An entry that cannot be attributed at all — unparseable, or // past the read cap, so not something this code ever wrote — // stays replaceable. Refusing it instead would strand From 55cf2549b09532483fbbcd76ce0ef3c48ab608f4 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:36:51 +0800 Subject: [PATCH 17/17] fix(core): harden session registry cleanup --- .../src/services/session-registry.test.ts | 81 +++++++++++++++++++ .../core/src/services/session-registry.ts | 64 ++++++++++----- 2 files changed, 124 insertions(+), 21 deletions(-) diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 48e8b2092aa..4a0276bd2f3 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -61,6 +61,11 @@ const statSizeLie = vi.hoisted(() => ({ value: null as number | null })); */ const sweepBeforeCommit = vi.hoisted(() => ({ value: null as string | null })); +const sweepReplacement = vi.hoisted(() => ({ + path: null as string | null, + contents: null as string | null, +})); + /** * Lets a test make `readdir` fail the way an unreadable registry directory * does, without depending on the uid the suite runs as. A chmod-based @@ -103,6 +108,20 @@ vi.mock('node:fs/promises', async (importOriginal) => { } return result; }; + const rename: typeof actual.rename = async (oldPath, newPath) => { + if ( + typeof oldPath === 'string' && + oldPath === sweepReplacement.path && + sweepReplacement.contents !== null + ) { + const contents = sweepReplacement.contents; + sweepReplacement.path = null; + sweepReplacement.contents = null; + await actual.rm(oldPath, { force: true }); + await actual.writeFile(oldPath, contents); + } + return actual.rename(oldPath, newPath); + }; const open: typeof actual.open = async (...args) => { const handle = await actual.open(...args); const realStat = handle.stat.bind(handle); @@ -122,6 +141,7 @@ vi.mock('node:fs/promises', async (importOriginal) => { default: { ...actual, open, writeFile, readdir }, open, writeFile, + rename, readdir, }; }); @@ -154,6 +174,8 @@ beforeEach(async () => { afterEach(async () => { statSizeLie.value = null; sweepBeforeCommit.value = null; + sweepReplacement.path = null; + sweepReplacement.contents = null; readdirFails.value = null; await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -309,6 +331,26 @@ describe('registerSession', () => { expect(await fs.readFile(target, 'utf8')).toBe('must survive'); }); + it('refuses a registry directory symlink without touching its target', async () => { + const dir = getSessionRegistryDir(); + const target = path.join(tmpDir, 'victim-directory'); + await fs.mkdir(target); + await fs.chmod(target, 0o755); + await fs.symlink(target, dir); + + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + kind: 'interactive', + }), + ).toBe(false); + + expect((await fs.lstat(dir)).isSymbolicLink()).toBe(true); + expect((await fs.stat(target)).mode & 0o777).toBe(0o755); + expect(await fs.readdir(target)).toEqual([]); + }); + it('leaves an existing registry directory and its records alone', async () => { // The repair is reachable only through a failed mkdir. If it ever fired // on the healthy path it would unlink the directory every other live @@ -1089,6 +1131,45 @@ describe('listLiveSessions', () => { expect(await listLiveSessions({ selfPid: DEAD_PID })).toEqual([]); }); + it('does not sweep a newer record published after stale validation', async () => { + const pid = DEAD_PID; + const filePath = await writeRaw(`${pid}.json`, { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid, + sessionId: 'stale', + cwd: '/w/stale', + name: 'stale-aa', + kind: 'interactive', + startedAt: Date.now() - 1000, + qwenVersion: null, + peerProtocol: 1, + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + procStart: null, + }); + sweepReplacement.path = filePath; + sweepReplacement.contents = JSON.stringify({ + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid, + sessionId: 'replacement', + cwd: '/w/live', + name: 'live-aa', + kind: 'interactive', + startedAt: Date.now(), + qwenVersion: null, + peerProtocol: 1, + pidNamespace: readPidNamespaceId(), + machineId: readMachineId(), + procStart: null, + }); + + await listLiveSessions({ selfPid: -1 }); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8')).sessionId).toBe( + 'replacement', + ); + }); + it('neither lists nor sweeps a same-origin record with no start token', async () => { // Where a token is readable this build always records one, so a // same-origin record without one did not come from this code. Trusting diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index d38204b228d..67f19750c38 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -413,20 +413,12 @@ function isEntryRace(error: unknown): boolean { * mkdir failed for some other reason (`EACCES` on a parent, most likely), * which is not an obstruction and is rethrown to the caller's own handler. * - * Scope, measured rather than assumed: `mkdir(recursive)` throws `EEXIST` - * on a regular file and on a symlink to one, and `ENOENT` on a dangling - * symlink — all three are repaired here. It *succeeds* on a symlink that - * resolves to a real directory, so that case never reaches this function - * and is not addressed by it; records would be written through the link. - * Refusing it belongs with the directory's own hardening (an `O_NOFOLLOW` - * open of the dir, or an `lstat` gate on the healthy path), not with a - * repair that only ever runs after a failure, and it trades against users - * who deliberately symlink the qwen dir onto another disk. + * After creation, `lstat` rejects a symlink-to-directory and a directory + * owned by another uid before chmod or record writes can follow it. */ async function ensureRegistryDir(dir: string): Promise { try { await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); - return; } catch (error) { let obstruction: fsSync.Stats; try { @@ -445,6 +437,46 @@ async function ensureRegistryDir(dir: string): Promise { await fs.unlink(dir); await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); } + const stat = await fs.lstat(dir); + const uid = typeof process.getuid === 'function' ? process.getuid() : null; + if (!stat.isDirectory() || (uid !== null && stat.uid !== uid)) { + throw new Error(`session registry directory is not owned by this user`); + } +} + +async function sweepStaleEntry( + filePath: string, + expected: EntryIdentity, +): Promise { + const quarantinePath = path.join( + path.dirname(filePath), + `.${path.basename(filePath)}.${randomBytes(6).toString('hex')}.stale`, + ); + try { + await fs.rename(filePath, quarantinePath); + } catch { + return; + } + + let moved: fsSync.Stats; + try { + moved = await fs.lstat(quarantinePath); + } catch { + return; + } + if (moved.dev === expected.dev && moved.ino === expected.ino) { + await fs.unlink(quarantinePath).catch(() => {}); + return; + } + + try { + await fs.link(quarantinePath, filePath); + await fs.unlink(quarantinePath); + } catch (error) { + debugLogger.debug( + `listLiveSessions: preserved a raced registry entry at ${quarantinePath}: ${describe(error)}`, + ); + } } /** @@ -1030,17 +1062,7 @@ export async function listLiveSessions( } if (sweepStale) { - try { - // Same binding as unregisterSession's: the entry that proved - // itself stale is the only one this may remove, so a co-tenant - // who swaps a live foreign record into the name after the read - // does not get it deleted on their behalf. - assertSameEntry(filePath, read.entry, true); - await fs.unlink(filePath); - } catch { - // Raced with another session's sweep, replaced under us, or not - // ours to delete. - } + await sweepStaleEntry(filePath, read.entry); } }, );