From e0b9271798430987327fa8f9ede5c1d4c6762c4d Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 12 Aug 2026 11:14:53 +0800 Subject: [PATCH 1/8] 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 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` and is never unlinked, so presence carries no liveness signal. The cost of asking it this question is visible in `isSessionRuntimeActive` — ~150 lines of candidate-directory guessing plus a recursive scan, and that only answers whether one *known* session is alive. Enumerating every live session that way is that cost times N. The two coexist: runtime.json stays the kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start identity of `:` read from /proc, so neither a recycled PID nor a reboot can resurrect a dead session's record. `session-writer-lease.ts` composes the same Linux identity and is deliberately left alone — its token is a persisted format with takeover semantics. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out, each one a real failure mode rather than defensive habit: 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 written `noFollow`, so a pre-planted symlink cannot redirect a registration write; only `.json` is ever considered a record, because a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote; and a record that fails validation is skipped without being swept, since we cannot reason about what we cannot parse. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. Record fields come from other processes, so the table renders them through `sanitizeTerminalText` — ANSI, control bytes, and bidi overrides (CVE-2021-42572 class) all matter when DIRECTORY is the column a user relies on to tell two sessions apart. Registration happens after first paint: nothing on screen depends on it, and it is an mkdir plus an fsync'd write. `/clear` and `/resume` patch the record's session id, and a directory switch patches its cwd, but never its name — that name is the handle a user just read out of `ps`. Co-Authored-By: Claude Opus 5 (1M context) --- docs/users/features/commands.md | 40 ++ packages/cli/src/commands/sessions.test.ts | 14 +- packages/cli/src/commands/sessions.ts | 2 + packages/cli/src/commands/sessions/ps.test.ts | 217 ++++++++ packages/cli/src/commands/sessions/ps.ts | 126 +++++ packages/cli/src/ui/startInteractiveUI.tsx | 31 ++ packages/core/src/agents/team/teamHelpers.ts | 15 +- packages/core/src/config/config.ts | 24 + packages/core/src/index.ts | 2 + .../src/services/session-registry.test.ts | 518 ++++++++++++++++++ .../core/src/services/session-registry.ts | 339 ++++++++++++ .../core/src/utils/process-liveness.test.ts | 244 +++++++++ packages/core/src/utils/process-liveness.ts | 139 +++++ 13 files changed, 1695 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/docs/users/features/commands.md b/docs/users/features/commands.md index f17bc7e60fe..b663b0e1d7e 100644 --- a/docs/users/features/commands.md +++ b/docs/users/features/commands.md @@ -643,6 +643,7 @@ These commands are run from the shell as `qwen ` before starting an | Command | Description | Usage Examples | | -------------------- | --------------------------------- | ------------------------------------------------------------ | | `qwen sessions list` | List recent conversation sessions | `qwen sessions list`, `qwen sessions list --json --limit 50` | +| `qwen sessions ps` | List sessions running right now | `qwen sessions ps`, `qwen sessions ps --json` | #### `qwen sessions list` @@ -681,3 +682,42 @@ qwen sessions list --limit 50 # Output as JSON for scripting qwen sessions list --json | jq . ``` + +#### `qwen sessions ps` + +Lists the Qwen Code sessions running on this machine right now. `sessions +list` walks saved transcripts ("what have I worked on"); this walks the +live-process registry ("what is running at this moment"). Records left +behind by a killed session are swept as they are found. + +**Flags:** + +| Flag | Type | Default | Description | +| -------- | ------- | ------- | ----------------------------------------------- | +| `--json` | boolean | `false` | Output as JSON Lines (one JSON object per line) | + +**Human-readable output (default):** + +A table with columns: NAME, PID, AGE, DIRECTORY. + +**JSON output (`--json`):** + +Outputs JSON Lines on stdout, newest session first. Each line is a JSON +object with fields: + +``` +schemaVersion, pid, procStart, sessionId, cwd, name, startedAt, qwenVersion +``` + +Nothing else is written to stdout — an empty listing prints nothing at +all — so `qwen sessions ps --json | jq .` is safe to script against. + +**Examples:** + +```bash +# Show the other live sessions +qwen sessions ps + +# Which directories are busy right now? +qwen sessions ps --json | jq -r .cwd +``` 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..b7e2eb7d4da --- /dev/null +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -0,0 +1,217 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import stringWidth from 'string-width'; +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, NAME_COL, PID_COL, AGE_COL } = 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', + startedAt: Date.now() - 90_000, + qwenVersion: '1.0.0', + ...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'); + }); + + it('changes unit exactly at the boundary, never one step late', () => { + expect(formatAge(59_999)).toBe('59s'); + expect(formatAge(60_000)).toBe('1m'); + expect(formatAge(3_599_000)).toBe('59m'); + expect(formatAge(3_600_000)).toBe('1h'); + expect(formatAge(24 * 3_600_000 - 1_000)).toBe('23h'); + expect(formatAge(24 * 3_600_000)).toBe('1d'); + }); +}); + +describe('qwen sessions ps', () => { + it('prints a table of live sessions', async () => { + listLiveSessions.mockResolvedValue([record()]); + await run({ json: 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('puts every column at its declared offset', async () => { + // `toContain` cannot tell a laid-out table from four values joined by + // one space, and it cannot see the age at all. Pin the whole row. + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date('2026-01-01T00:00:00Z')); + listLiveSessions.mockResolvedValue([ + record({ startedAt: Date.now() - 5_000 }), + ]); + await run({ json: false }); + } finally { + vi.useRealTimers(); + } + + expect(stdout[0]).toBe( + 'NAME'.padEnd(NAME_COL) + + 'PID'.padEnd(PID_COL) + + 'AGE'.padEnd(AGE_COL) + + 'DIRECTORY', + ); + expect(stdout[1]).toBe( + 'app-ab'.padEnd(NAME_COL) + + '4242'.padEnd(PID_COL) + + '5s'.padEnd(AGE_COL) + + '/w/app', + ); + expect([NAME_COL, PID_COL, AGE_COL]).toEqual([22, 9, 10]); + }); + + it('says so plainly when nothing else is running', async () => { + listLiveSessions.mockResolvedValue([]); + 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 }); + + expect(stdout).toHaveLength(2); + expect(JSON.parse(stdout[0]).pid).toBe(4242); + expect(JSON.parse(stdout[1]).pid).toBe(7); + }); + + it('emits each record as one whole line of JSON Lines', async () => { + // JSON Lines is line-delimited by definition: a pretty-printed record + // still round-trips through JSON.parse but breaks every consumer that + // reads it a line at a time, and drops no field on the way. + const rec = record(); + listLiveSessions.mockResolvedValue([rec]); + await run({ json: true }); + + expect(stdout).toEqual([JSON.stringify(rec)]); + expect(stdout[0]).not.toContain('\n'); + }); + + it('prints nothing on stdout for an empty JSON listing', async () => { + listLiveSessions.mockResolvedValue([]); + await run({ json: true }); + expect(stdout).toEqual([]); + }); + + it('neutralizes control sequences coming from another process record', async () => { + listLiveSessions.mockResolvedValue([ + record({ name: 'evil\r', cwd: '/w/a\nb' }), + ]); + await run({ json: false }); + + const row = stdout[1]; + expect(row).not.toContain(''); + expect(row).not.toContain('\r'); + expect(row).not.toContain('\n'); + }); + + it('strips bidi overrides that would reorder the rendered row', async () => { + listLiveSessions.mockResolvedValue([ + record({ name: 'a\u202Eb', cwd: '/w/\u202Dsafe\u2069' }), + ]); + await run({ json: false }); + + expect(stdout[1]).not.toMatch(/[\u202A-\u202E\u2066-\u2069]/); + expect(stdout[1]).toContain('/w/safe'); + }); + + it('truncates an over-long name instead of breaking the columns', async () => { + listLiveSessions.mockResolvedValue([record({ name: 'x'.repeat(80) })]); + await run({ json: false }); + expect(stdout[1]).toContain('\u2026'); + expect(stdout[1]).toContain('4242'); + }); + + it('truncates the name two cells short of its column, leaving a gutter', async () => { + // The gutter is what keeps a maximally long name from touching the PID + // beside it; truncating to the full column width would remove it. + listLiveSessions.mockResolvedValue([record({ name: 'x'.repeat(80) })]); + await run({ json: false }); + + expect(stdout[1].slice(0, NAME_COL)).toBe(`${'x'.repeat(19)}\u2026 `); + }); + + it('declares --json as a boolean that is off by default', async () => { + const options: Record = {}; + const yargs = { + option: vi.fn((key: string, config: unknown) => { + options[key] = config; + return yargs; + }), + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (psCommand.builder as any)(yargs); + + expect(psCommand.command).toBe('ps'); + expect(options['json']).toMatchObject({ type: 'boolean', default: false }); + }); + + it('keeps a CJK name inside its column instead of shifting the row', async () => { + listLiveSessions.mockResolvedValue([record({ name: '项目'.repeat(20) })]); + await run({ json: false }); + + // Padding is measured in terminal cells, not code units: a 2-cell CJK + // character must not push the PID column one cell right per character. + const row = stdout[1]; + expect(stringWidth(row.slice(0, row.indexOf('4242')))).toBe(22); + }); +}); diff --git a/packages/cli/src/commands/sessions/ps.ts b/packages/cli/src/commands/sessions/ps.ts new file mode 100644 index 00000000000..1b1360acdd7 --- /dev/null +++ b/packages/cli/src/commands/sessions/ps.ts @@ -0,0 +1,126 @@ +/** + * @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 { + sanitizeTerminalText, + truncateToWidth, +} from '../../ui/utils/textUtils.js'; +import { writeStdoutLine } 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; +} + +/** + * Sanitize a record field for terminal output. + * + * `cwd` and `name` are written by another process, so they are + * attacker-influenced: an ANSI sequence could repaint the table, a bare + * control byte could misalign it, and a bidi override (Trojan Source, + * CVE-2021-42572) could make a directory render as a path that does not + * exist. `sanitizeTerminalText` is the single source of truth for all + * three classes; it deliberately preserves TAB and LF for multi-line + * render sites, so a one-line table cell drops those two on top of it. + */ +function sanitize(value: string): string { + return sanitizeTerminalText(value).replace(/[\t\n]/g, ''); +} + +function padDisplay(str: string, width: number): string { + const currentWidth = stringWidth(str); + if (currentWidth >= width) return str; + return str + ' '.repeat(width - currentWidth); +} + +/** + * 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( + truncateToWidth(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 { + // listLiveSessions reports "cannot look" as "no peers" rather than + // throwing, so there is no failure path to surface here. + const records = await listLiveSessions(); + 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, + }), + handler: async (argv) => { + await handlePs(argv); + }, +}; diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index c3c26022c7d..fa125f58aa5 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -11,7 +11,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'; @@ -284,6 +286,35 @@ export async function startInteractiveUI( config.isTelemetryInitializationDeferred(), }); + // 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". + // + // Deliberately after render(): the write is an mkdir + chmod + fsync'd + // atomic write, and nothing on the first screen depends on it, so it + // belongs with the other post-first-paint work rather than in front of + // the user's first frame. + // + // Wrapped like every other startup side-effect in this function (the + // sidecar above, the dual-output bridge, the remote-input watcher): + // discovery is a convenience and must not be able to abort a session. + try { + if ( + await registerSession({ + sessionId: config.getSessionId(), + cwd: config.getTargetDir(), + qwenVersion: version, + }) + ) { + // Only arm cleanup for a record that exists; registration fails on + // a read-only home, and there is then nothing to unlink. + registerCleanup(() => unregisterSession()); + } + } catch (err) { + debugLogger.debug(`session registration skipped: ${String(err)}`); + } + // Periodic memory-pressure check for the interactive session. The interval // is unref'd (can't keep the loop alive on its own) and cleared on cleanup. const pressureMonitor = config.getMemoryPressureMonitor?.(); 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 c14265316f2..5e57d4f2437 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -216,6 +216,7 @@ import { clearRuntimeStatus, writeRuntimeStatus, } from '../utils/runtimeStatus.js'; +import { patchSessionRecord } from '../services/session-registry.js'; import { SessionService, type ResumedSessionData, @@ -3892,6 +3893,24 @@ export class Config { workDir, qwenVersion: cliVersion, }); + // Keep the machine-wide session registry in step: this PID's + // record would otherwise point discovery at the previous + // transcript. Keyed by PID, so a swap is a patch rather than a + // delete-and-rewrite. + // + // This rides the sidecar's `runtimeStatusEnabled` gate rather + // than having its own. Not the same rule — the sidecar is gated + // because a short-lived process must not delete a sibling's + // file, and PID-keyed records have no such hazard — but the two + // lifecycles coincide, and the only divergence (sidecar write + // failed, registration succeeded) costs a stale `sessionId` in + // `ps --json` until exit. + // + // `name` is deliberately not patched: it is the handle a user + // just read out of `qwen sessions ps`, and re-deriving it here + // would rename a live session on every /clear for no gain — the + // directory it names has not changed. + await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); }); } @@ -3941,6 +3960,11 @@ export class Config { qwenVersion: this.cliVersion ?? null, }, ); + // The registry's DIRECTORY column is how a user tells two live + // sessions apart, so a mid-session directory switch has to reach it + // too — otherwise `qwen sessions ps` keeps advertising the folder + // this session left. + await patchSessionRecord({ cwd: workDir }); }); await this.flushRuntimeStatusWrites(); } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index dc6c59e45cf..83bafec54c5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -290,6 +290,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 { @@ -583,6 +584,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..c767c7309a3 --- /dev/null +++ b/packages/core/src/services/session-registry.test.ts @@ -0,0 +1,518 @@ +/** + * @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'; + +/** + * Records the paths `readRecord` stats, while the real filesystem does the + * work. The `.json` filename filter is invisible from the outside — + * the filename/contents agreement check downstream rejects everything a + * looser regex would let through — so the only way to hold the filter to + * its stated job, not reading whatever else lives in `~/.qwen/sessions`, + * is to watch what it opens. + */ +const statCalls: string[] = []; +let recordStatCalls = false; + +vi.mock('node:fs/promises', async () => { + const real = + await vi.importActual( + 'node:fs/promises', + ); + return { + ...real, + default: real, + stat: (...args: Parameters) => { + if (recordStatCalls) statCalls.push(path.basename(String(args[0]))); + return real.stat(...args); + }, + }; +}); + +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; +} + +/** + * A record body that `listLiveSessions` must accept and return verbatim: + * schema 1, this process's (live) PID, and no start token, so the liveness + * check degrades to "the PID is running" on every platform. + * + * Every rejection case below is this body with one field spoiled, so a + * spoiled field that stops being rejected shows up as a listed record + * rather than as nothing at all. + */ +function liveBody(over: Record = {}): Record { + return { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: null, + sessionId: 's', + cwd: '/w', + name: 'n', + startedAt: 5, + qwenVersion: null, + ...over, + }; +} + +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}$/); + }); + + it('caps the basename at 32 characters so the name fits a table cell', () => { + const name = deriveSessionName(`/w/${'a'.repeat(80)}`, 's1'); + expect(name).toMatch(/^a{32}-[0-9a-f]{2}$/); + expect(name).toHaveLength(35); + }); +}); + +describe('registerSession', () => { + it('writes a record for this process and lists it back', async () => { + expect( + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + qwenVersion: '1.2.3', + }), + ).toBe(true); + + const live = await listLiveSessions(); + expect(live).toHaveLength(1); + expect(live[0]).toMatchObject({ + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + sessionId: 's1', + cwd: '/w/app', + qwenVersion: '1.2.3', + }); + expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); + }); + + // Windows synthesizes st_mode from file attributes and `chmod` can only + // toggle the read-only bit, so permission assertions are meaningless + // there. Guarded rather than deleted, the same way + // `session-writer-lease.test.ts` guards its identical 0700/0600 pair. + const itPosix = it.runIf(process.platform !== 'win32'); + + itPosix('creates the registry directory as 0700', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + }); + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }); + + itPosix('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', + }); + + const stat = await fs.stat(getSessionRegistryDir()); + expect(stat.mode & 0o777).toBe(0o700); + }); + + itPosix('writes the record as 0600', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + }); + const stat = await fs.stat(getSessionRecordPath()); + expect(stat.mode & 0o777).toBe(0o600); + }); + + itPosix( + 'replaces a pre-planted symlink instead of writing through it', + async () => { + // Anything that can create a file in the registry directory could park + // a symlink at `.json` and redirect the registration write. With + // `noFollow` the rename replaces the link; without it the write + // resolves the chain and lands on the attacker's target. + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + const outside = path.join(tmpDir, 'outside.txt'); + await fs.writeFile(outside, 'untouched'); + await fs.symlink(outside, getSessionRecordPath()); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + true, + ); + + expect(await fs.readFile(outside, 'utf8')).toBe('untouched'); + expect((await fs.lstat(getSessionRecordPath())).isSymbolicLink()).toBe( + false, + ); + }, + ); + + 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', + }), + ).toBe(false); + }); +}); + +describe('patchSessionRecord', () => { + it('updates a field without dropping the others', async () => { + await registerSession({ + sessionId: 'old', + cwd: '/w/app', + qwenVersion: '1.2.3', + }); + + await patchSessionRecord({ sessionId: 'new', name: 'renamed' }); + + const [record] = await listLiveSessions(); + 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 () => { + // The registry directory exists, so the only thing standing between a + // patch and a half-populated record on disk is the missing-record + // guard. Asserting an empty listing is not enough: a record built from + // the patch alone fails validation and so lists as nothing either way. + await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + + await patchSessionRecord({ sessionId: 'new' }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + }); + + it('leaves a foreign record sitting at this pid’s path untouched', async () => { + // `readRecord` does not check filename/contents agreement, so without + // the pid guard a patch would merge into someone else's record and + // write back something `listLiveSessions` will neither show nor sweep. + const foreign = liveBody({ pid: process.pid + 1, sessionId: 'theirs' }); + const filePath = await writeRaw(`${process.pid}.json`, foreign); + + await patchSessionRecord({ sessionId: 'mine' }); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toEqual(foreign); + }); + + const itPosixPatch = it.runIf(process.platform !== 'win32'); + + itPosixPatch('keeps the record at 0600 across a patch', async () => { + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + await patchSessionRecord({ sessionId: 'new' }); + const stat = await fs.stat(getSessionRecordPath()); + expect(stat.mode & 0o777).toBe(0o600); + }); +}); + +describe('unregisterSession', () => { + it('removes the record', async () => { + await registerSession({ + sessionId: 's1', + cwd: '/w/app', + }); + await unregisterSession(); + expect(await listLiveSessions()).toEqual([]); + }); + + it('is a no-op when nothing was registered', async () => { + await expect(unregisterSession()).resolves.toBeUndefined(); + }); +}); + +describe('listLiveSessions', () => { + it('returns an empty list when the registry does not exist', async () => { + expect(await listLiveSessions()).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', + startedAt: Date.now(), + qwenVersion: null, + }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).rejects.toThrow(); + }); + + // Only Linux has a start token to disagree with, so elsewhere this is a + // visible skip rather than a test that passes without asserting. + it.runIf(process.platform === 'linux')( + '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. + const filePath = await writeRaw(`${process.pid}.json`, { + schemaVersion: 1, + pid: process.pid, + procStart: 'not-this-boot:1', + sessionId: 's-recycled', + cwd: '/w/app', + name: 'app-aa', + startedAt: Date.now(), + }); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).rejects.toThrow(); + }, + ); + + 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', + startedAt: 1, + }); + + expect(await listLiveSessions()).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('never opens a file that is not named .json', async () => { + await writeRaw('2026-planning-notes.json', { hello: 'world' }); + await writeRaw('notes.txt', 'nope'); + // Anchored at both ends, with the dot escaped: `session-2026.json` + // defeats a regex missing its `^`, `12.json.bak` one missing its `$`, + // and `12xjson` one whose `.` is a wildcard. + await writeRaw('session-2026.json', { hello: 'world' }); + await writeRaw('12.json.bak', { hello: 'world' }); + await writeRaw('12xjson', { hello: 'world' }); + await writeRaw(`${process.pid}.json`, liveBody()); + + statCalls.length = 0; + recordStatCalls = true; + try { + expect(await listLiveSessions()).toEqual([liveBody()]); + } finally { + recordStatCalls = false; + } + + expect(statCalls).toEqual([`${process.pid}.json`]); + }); + + 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', + startedAt: 1, + }); + expect(await listLiveSessions()).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', + startedAt: 1, + }); + await writeRaw('13.json', { + schemaVersion: 1, + pid: 13, + sessionId: 's', + cwd: '/w', + name: 42, + startedAt: 1, + }); + + expect(await listLiveSessions()).toEqual([]); + expect((await fs.readdir(getSessionRegistryDir())).sort()).toEqual([ + '11.json', + '12.json', + '13.json', + ]); + }); + + it('returns a well-formed record verbatim', async () => { + // The control for every rejection case below, and the only assertion + // that pins the exact field set a reader gets back. + await writeRaw(`${process.pid}.json`, liveBody()); + expect(await listLiveSessions()).toEqual([liveBody()]); + }); + + it('nulls optional fields of the wrong type rather than passing them on', async () => { + // A numeric `procStart` handed to `isSameProcess` would never equal the + // string token it reads back, so a live session would be swept; a + // numeric `qwenVersion` would reach every consumer typed as a string. + await writeRaw( + `${process.pid}.json`, + liveBody({ procStart: 12345, qwenVersion: 7 }), + ); + expect(await listLiveSessions()).toEqual([liveBody()]); + }); + + it.each([ + ['a string schemaVersion', { schemaVersion: '1' }], + ['no schemaVersion at all', { schemaVersion: undefined }], + ['a string pid', { pid: String(process.pid) }], + ['a non-string sessionId', { sessionId: 42 }], + ['a non-string cwd', { cwd: null }], + ['a non-string name', { name: 42 }], + ['a string startedAt', { startedAt: '5' }], + ])( + 'skips a record with %s, and never sweeps it', + async (_what, over: Record) => { + const filePath = await writeRaw(`${process.pid}.json`, liveBody(over)); + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }, + ); + + it('skips a record whose startedAt is not finite', async () => { + // JSON has no Infinity literal, but 1e999 parses to one — and an + // Infinity `startedAt` sorts every real session below it forever. + const filePath = await writeRaw( + `${process.pid}.json`, + JSON.stringify(liveBody()).replace('"startedAt":5', '"startedAt":1e999'), + ); + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('never sweeps 0.json, which no real process can own', async () => { + // `0.json` clears the filename regex and agrees with its own contents, + // so only the `pid <= 0` check stops `process.kill(0, 0)` — a + // whole-process-group signal — from deciding a stranger's file's fate. + const filePath = await writeRaw('0.json', liveBody({ pid: 0 })); + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('refuses to parse a record larger than 64 KiB', async () => { + const filePath = await writeRaw( + `${process.pid}.json`, + liveBody({ cwd: `/w/${'x'.repeat(70_000)}` }), + ); + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('sorts newest first', async () => { + await registerSession({ + sessionId: 's-self', + cwd: '/w/app', + }); + await patchSessionRecord({ startedAt: 1000 }); + await writeRaw(`${process.ppid}.json`, { + schemaVersion: 1, + pid: process.ppid, + sessionId: 's-parent', + cwd: '/w/other', + name: 'other-bb', + startedAt: 2000, + }); + + const live = await listLiveSessions(); + 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..baab0a25b4e --- /dev/null +++ b/packages/core/src/services/session-registry.ts @@ -0,0 +1,339 @@ +/** + * @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 it is + * indexed by session id rather than by liveness. `isSessionRuntimeActive` + * in `worktreeSessionService.ts` shows what the reverse lookup costs + * there: candidate runtime-base guessing plus a recursive scan, to + * answer the question for a *single already-known* session id. + * - 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; + +/** + * 0700 keeps the listing — session names and their work dirs — readable + * only by its owner. The records are also written `noFollow`, so a + * pre-planted symlink at `.json` cannot redirect a registration + * write elsewhere; `session-writer-lease.ts` gets the same guard from + * `O_NOFOLLOW`. + */ +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$/; + +/** 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; + /** Epoch milliseconds. */ + startedAt: number; + qwenVersion: string | null; +} + +export interface RegisterSessionFields { + sessionId: string; + cwd: string; + qwenVersion?: string | null; +} + +export function getSessionRegistryDir(): string { + return path.join(Storage.getGlobalQwenDir(), 'sessions'); +} + +/** This process's record path. Records are keyed by PID. */ +export function getSessionRecordPath(): string { + return path.join(getSessionRegistryDir(), `${process.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 record: SessionRegistryRecord = { + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, + pid: process.pid, + procStart: readProcStartToken(process.pid), + sessionId: fields.sessionId, + cwd: fields.cwd, + name: deriveSessionName(fields.cwd, fields.sessionId), + startedAt: Date.now(), + qwenVersion: fields.qwenVersion ?? null, + }; + + 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(), record, { + mode: REGISTRY_FILE_MODE, + forceMode: true, + noFollow: 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>, +): Promise { + const filePath = getSessionRecordPath(); + try { + const existing = await readRecord(filePath); + // Missing, or not actually a record for this PID: `readRecord` does + // not check the filename/contents agreement that `listLiveSessions` + // insists on, so merging into a foreign `.json` would write back + // a record the reader will neither show nor sweep — permanent litter. + if (existing === null || existing.pid !== process.pid) return; + await atomicWriteJSON( + filePath, + { ...existing, ...patch }, + { 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. */ +export async function unregisterSession(): Promise { + try { + await fs.unlink(getSessionRecordPath()); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; + debugLogger.debug(`unregisterSession failed: ${describe(error)}`); + } +} + +/** + * 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(): Promise { + let dir: string; + let entries: string[]; + try { + // Inside the try as well: `getGlobalQwenDir()` resolves the home + // directory and can throw. Callers are told this never throws, and + // `ps` dropped its error path on the strength of that promise. + dir = getSessionRegistryDir(); + 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 (isSameProcess(record.pid, record.procStart)) { + live.push(record); + return; + } + + 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 startedAt = value['startedAt']; + if ( + typeof pid !== 'number' || + !Number.isInteger(pid) || + pid <= 0 || + typeof sessionId !== 'string' || + typeof cwd !== 'string' || + typeof name !== 'string' || + typeof startedAt !== 'number' || + !Number.isFinite(startedAt) + ) { + return null; + } + + const procStart = value['procStart']; + const qwenVersion = value['qwenVersion']; + + return { + schemaVersion, + pid, + procStart: typeof procStart === 'string' ? procStart : null, + sessionId, + cwd, + name, + startedAt, + qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, + }; +} + +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..d6cdeba2ae5 --- /dev/null +++ b/packages/core/src/utils/process-liveness.test.ts @@ -0,0 +1,244 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { + isPidAlive, + isSameProcess, + readProcStartToken, +} from './process-liveness.js'; + +/** A PID that is essentially certain not to be running. */ +const DEAD_PID = 0x7ffffffe; + +const FAKE_BOOT_ID = '1e0d09fd-4d0b-4b9d-9d1b-2f0c1a3b4c5d'; +const BOOT_ID_PATH = '/proc/sys/kernel/random/boot_id'; + +/** + * A synthetic `/proc//stat` line. + * + * The field arithmetic in `readProcStartToken` is only testable against a + * fake: a real `/proc` entry cannot be made to hold a `comm` containing + * ')', a non-numeric field 22, or a missing boot id, and every neighbour + * of field 22 in a real line is also a plain integer — so reading the + * wrong index off a real process still yields something that looks like a + * valid token. + */ +function statLine(comm: string, startTime: string): string { + // Fields 3..22. Once the parenthesised `comm` is stripped, field N sits + // at index N - 3, so `startTime` (field 22, `starttime`) is the + // twentieth entry. The neighbours are deliberately distinct values so an + // off-by-one read is visible. + // prettier-ignore + const fields = [ + 'S', '1', '2', '3', '4', '-1', '4194304', '100', '0', '200', + '0', '10', '20', '30', '40', '20', '0', '1', '0', startTime, + ]; + return `4242 (${comm}) ${fields.join(' ')} 1000 2000 3000\n`; +} + +interface FakeProc { + mod: typeof import('./process-liveness.js'); + reads: string[]; +} + +/** + * Load a fresh copy of the module with `/proc` served out of `files` and + * the platform forced to Linux, so the parser is exercised on every CI + * runner rather than only the Linux one. + */ +async function withFakeProc(files: Record): Promise { + const reads: string[] = []; + vi.resetModules(); + vi.doMock('node:fs', () => ({ + readFileSync: (p: unknown) => { + reads.push(String(p)); + const body = files[String(p)]; + if (body === undefined) { + throw Object.assign(new Error(`ENOENT: ${p}`), { code: 'ENOENT' }); + } + return body; + }, + })); + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + const mod = await import('./process-liveness.js'); + return { mod, reads }; +} + +afterEach(() => { + vi.doUnmock('node:fs'); + vi.resetModules(); + vi.restoreAllMocks(); +}); + +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); + }); + + // EPERM is the whole reason this helper is not a bare try/catch: a + // process owned by another user is alive, and calling it dead would let + // one user's sweep delete another user's registry record. The test suite + // cannot rely on such a process existing, so the errno is injected. + it('treats EPERM — another user’s process — as alive', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { + code: 'EPERM', + }); + }); + expect(isPidAlive(4242)).toBe(true); + }); + + it('treats ESRCH as dead', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('no such process'), { code: 'ESRCH' }); + }); + expect(isPidAlive(4242)).toBe(false); + }); +}); + +describe('readProcStartToken', () => { + it('returns a boot-scoped token for a live process on Linux', () => { + const token = readProcStartToken(process.pid); + if (process.platform !== 'linux') { + expect(token).toBeNull(); + return; + } + // : — the boot id is what keeps a record from a + // previous boot from matching a recycled PID. + expect(token).toMatch(/^[0-9a-f-]+:\d+$/i); + }); + + 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(); + }); + + it('reads starttime as field 22, counting from the last ")" in comm', async () => { + const { mod } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + // A `comm` holding both a space and a ')' — legal, and fatal to any + // parser that splits the whole line or anchors on the first ')'. + '/proc/4242/stat': statLine('we ) ird', '987654'), + }); + expect(mod.readProcStartToken(4242)).toBe(`${FAKE_BOOT_ID}:987654`); + }); + + it('returns null when the stat line has no comm parentheses at all', async () => { + const { mod } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + '/proc/4242/stat': + '4242 qwen S 1 2 3 4 -1 4194304 100 0 200 0 10 20 30 40 20 0 1 0 987654 1000\n', + }); + // Without the `commEnd === -1` bail this counts from the start of the + // line and confidently returns field 20 as if it were starttime. + expect(mod.readProcStartToken(4242)).toBeNull(); + }); + + it('returns null when field 22 is not a number', async () => { + const { mod } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + '/proc/4242/stat': statLine('qwen', 'not-a-number'), + }); + expect(mod.readProcStartToken(4242)).toBeNull(); + }); + + it('returns null rather than a bare tick count when the boot id is unreadable', async () => { + // Two token shapes on one machine would let a reader that has the boot + // id "mismatch" a live session recorded without it and sweep it. + const { mod } = await withFakeProc({ + '/proc/4242/stat': statLine('qwen', '987654'), + }); + expect(mod.readProcStartToken(4242)).toBeNull(); + }); + + it('rejects a boot id that is not a hex-and-dash uuid', async () => { + const { mod } = await withFakeProc({ + [BOOT_ID_PATH]: 'not a uuid\n', + '/proc/4242/stat': statLine('qwen', '987654'), + }); + expect(mod.readProcStartToken(4242)).toBeNull(); + }); + + it('reads the boot id once however many records are checked', async () => { + const { mod, reads } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + '/proc/4242/stat': statLine('qwen', '987654'), + '/proc/4243/stat': statLine('qwen', '987655'), + }); + mod.readProcStartToken(4242); + mod.readProcStartToken(4243); + expect(reads.filter((p) => p === BOOT_ID_PATH)).toHaveLength(1); + expect(reads.filter((p) => p.endsWith('/stat'))).toHaveLength(2); + }); + + it('rejects nonsense pids before touching /proc', async () => { + const { mod, reads } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + // Planted so a missing pid guard would find something to return. + '/proc/0/stat': statLine('qwen', '111'), + '/proc/1.5/stat': statLine('qwen', '222'), + }); + expect(mod.readProcStartToken(0)).toBeNull(); + expect(mod.readProcStartToken(-1)).toBeNull(); + expect(mod.readProcStartToken(1.5)).toBeNull(); + expect(reads.filter((p) => p.endsWith('/stat'))).toEqual([]); + }); +}); + +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); + }); + + // Only Linux produces a token to disagree with; elsewhere this is a + // visible skip rather than a test that passes without asserting. + it.runIf(process.platform === 'linux')( + 'rejects a live pid whose token has changed', + () => { + expect(isSameProcess(process.pid, 'definitely-not-the-token')).toBe( + false, + ); + }, + ); + + it('keeps a live session whose token cannot be read right now', async () => { + // /proc unreadable in a container, or the boot id missing: a record + // that carries a token must still count as live, because deleting a + // running session's record is the worse of the two failures. + const { mod } = await withFakeProc({}); + expect(mod.isSameProcess(process.pid, 'a-token-we-cannot-compare')).toBe( + true, + ); + }); +}); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts new file mode 100644 index 00000000000..5241b93848e --- /dev/null +++ b/packages/core/src/utils/process-liveness.ts @@ -0,0 +1,139 @@ +/** + * @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 `boot_id` plus the `starttime` field of `/proc//stat` on + * Linux — the process start time in clock ticks since boot. Two processes + * sharing a PID within one boot will not share `starttime`; across a + * reboot they can, and a registry record outlives a reboot whenever the + * machine crashes or loses power, so the boot id is what makes every + * pre-reboot token provably foreign. + * + * `session-writer-lease.ts`'s `readProcessStartIdentity` builds the same + * Linux identity, and is deliberately left alone: its token is a + * persisted on-disk format with takeover semantics, so unifying them is a + * change to that file's contract rather than a refactor. If a third + * caller ever needs this, it imports from here — two is already the + * limit. + * + * When the boot id is unreadable this returns `null` rather than a bare + * tick count: emitting two token shapes on one machine would let a reader + * that has the boot id "mismatch" a live session recorded without it and + * sweep its record. A `null` degrades to a plain liveness check instead. + * + * 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; + + const bootId = readBootId(); + if (bootId === null) 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) + ? `${bootId}:${startTime}` + : null; +} + +/** + * The kernel's per-boot UUID, or `null` when it cannot be read. Cached: it + * cannot change while this process lives, and enumeration reads a token + * per record. + */ +let cachedBootId: string | null | undefined; + +function readBootId(): string | null { + if (cachedBootId === undefined) { + try { + const value = fs + .readFileSync('/proc/sys/kernel/random/boot_id', 'utf8') + .trim(); + cachedBootId = /^[0-9a-f-]+$/i.test(value) ? value : null; + } catch { + cachedBootId = null; + } + } + return cachedBootId; +} + +/** + * 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 09576726dfe6dc035a5960dde83a4353549f29ea Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 10:18:42 +0000 Subject: [PATCH 2/8] fix(core): harden the session registry per review feedback (#8969) --- docs/users/features/commands.md | 27 ++- packages/cli/src/commands/sessions.test.ts | 2 +- packages/cli/src/commands/sessions/ps.test.ts | 30 ++- packages/cli/src/commands/sessions/ps.ts | 16 +- .../cli/src/ui/startInteractiveUI.test.tsx | 126 +++++++++++ .../core/src/agents/team/teamHelpers.test.ts | 20 ++ packages/core/src/config/config.test.ts | 13 ++ packages/core/src/config/config.ts | 14 +- .../src/services/session-registry.test.ts | 213 +++++++++++++++--- .../core/src/services/session-registry.ts | 62 ++++- .../core/src/utils/process-liveness.test.ts | 85 ++++++- packages/core/src/utils/process-liveness.ts | 97 ++++++-- .../src/utils/runtimeStatus.config.test.ts | 51 +++++ 13 files changed, 675 insertions(+), 81 deletions(-) create mode 100644 packages/cli/src/ui/startInteractiveUI.test.tsx diff --git a/docs/users/features/commands.md b/docs/users/features/commands.md index b663b0e1d7e..55c0ede7baf 100644 --- a/docs/users/features/commands.md +++ b/docs/users/features/commands.md @@ -640,10 +640,10 @@ These commands are run from the shell as `qwen ` before starting an ### Session Management -| Command | Description | Usage Examples | -| -------------------- | --------------------------------- | ------------------------------------------------------------ | -| `qwen sessions list` | List recent conversation sessions | `qwen sessions list`, `qwen sessions list --json --limit 50` | -| `qwen sessions ps` | List sessions running right now | `qwen sessions ps`, `qwen sessions ps --json` | +| Command | Description | Usage Examples | +| -------------------- | ------------------------------------------- | ------------------------------------------------------------ | +| `qwen sessions list` | List recent conversation sessions | `qwen sessions list`, `qwen sessions list --json --limit 50` | +| `qwen sessions ps` | List interactive sessions running right now | `qwen sessions ps`, `qwen sessions ps --json` | #### `qwen sessions list` @@ -685,10 +685,12 @@ qwen sessions list --json | jq . #### `qwen sessions ps` -Lists the Qwen Code sessions running on this machine right now. `sessions -list` walks saved transcripts ("what have I worked on"); this walks the -live-process registry ("what is running at this moment"). Records left -behind by a killed session are swept as they are found. +Lists the interactive Qwen Code sessions running on this machine right +now. `sessions list` walks saved transcripts ("what have I worked on"); +this walks the live-process registry ("what is running at this moment"). +Records left behind by a killed session are swept as they are found. +Headless sessions (`qwen -p`) do not register with the live-process +registry, so they are not shown. **Flags:** @@ -706,12 +708,17 @@ Outputs JSON Lines on stdout, newest session first. Each line is a JSON object with fields: ``` -schemaVersion, pid, procStart, sessionId, cwd, name, startedAt, qwenVersion +schemaVersion, pid, procStart, pidNs, sessionId, cwd, name, startedAt, +qwenVersion ``` Nothing else is written to stdout — an empty listing prints nothing at all — so `qwen sessions ps --json | jq .` is safe to script against. +JSON output is raw data: field values are emitted exactly as recorded, +with no terminal sanitization. Treat them as data, and sanitize before +rendering them in a terminal. + **Examples:** ```bash @@ -719,5 +726,7 @@ all — so `qwen sessions ps --json | jq .` is safe to script against. qwen sessions ps # Which directories are busy right now? +# Note: `jq -r` renders the raw recorded value in your terminal (see the +# raw-data note above); pipe through a sanitizer if the path is untrusted. qwen sessions ps --json | jq -r .cwd ``` diff --git a/packages/cli/src/commands/sessions.test.ts b/packages/cli/src/commands/sessions.test.ts index 9fd4bdb306d..20fc52de4fe 100644 --- a/packages/cli/src/commands/sessions.test.ts +++ b/packages/cli/src/commands/sessions.test.ts @@ -18,7 +18,7 @@ vi.mock('./sessions/list.js', () => ({ vi.mock('./sessions/ps.js', () => ({ psCommand: { command: 'ps', - describe: 'List Qwen Code sessions running right now', + describe: 'List interactive Qwen Code sessions running right now', }, })); diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index b7e2eb7d4da..72af57d06a2 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -33,6 +33,7 @@ function record( schemaVersion: 1, pid: 4242, procStart: '123', + pidNs: null, sessionId: 'sess-1', cwd: '/w/app', name: 'app-ab', @@ -122,7 +123,9 @@ describe('qwen sessions ps', () => { it('says so plainly when nothing else is running', async () => { listLiveSessions.mockResolvedValue([]); await run({ json: false }); - expect(stdout).toEqual(['No other Qwen Code sessions are running.']); + expect(stdout).toEqual([ + 'No other interactive Qwen Code sessions are running.', + ]); }); it('emits one JSON object per line with no header', async () => { @@ -139,10 +142,15 @@ describe('qwen sessions ps', () => { // still round-trips through JSON.parse but breaks every consumer that // reads it a line at a time, and drops no field on the way. const rec = record(); + // Snapshotted before the run: the mock hands the handler the object + // itself, so computing the expectation afterwards would observe the + // very object the handler (mutatingly) emitted and could never catch + // an in-place field deletion. + const expected = JSON.stringify(rec); listLiveSessions.mockResolvedValue([rec]); await run({ json: true }); - expect(stdout).toEqual([JSON.stringify(rec)]); + expect(stdout).toEqual([expected]); expect(stdout[0]).not.toContain('\n'); }); @@ -154,7 +162,7 @@ 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: 'evil\r', cwd: '/w/a\nb\tc' }), ]); await run({ json: false }); @@ -162,6 +170,11 @@ describe('qwen sessions ps', () => { expect(row).not.toContain(''); expect(row).not.toContain('\r'); expect(row).not.toContain('\n'); + // sanitizeTerminalText deliberately preserves TAB for multi-line + // render sites; the one-line table cell drops it on top — a literal + // TAB in a cwd (legal in POSIX filenames) would otherwise expand to + // the next tab stop and misalign every column after AGE. + expect(row).not.toContain('\t'); }); it('strips bidi overrides that would reorder the rendered row', async () => { @@ -174,6 +187,17 @@ describe('qwen sessions ps', () => { expect(stdout[1]).toContain('/w/safe'); }); + it('emits --json values raw, leaving terminal sanitization to the consumer', async () => { + // The contract the docs state: JSON output is data, not display. + // Bidi overrides that the table path strips must round-trip here — + // sanitizing them would rewrite the recorded path for every tooling + // consumer and diverge from the sibling `sessions list --json`. + listLiveSessions.mockResolvedValue([record({ cwd: '/w/\u202Ereorder' })]); + await run({ json: true }); + + expect(JSON.parse(stdout[0]).cwd).toBe('/w/\u202Ereorder'); + }); + it('truncates an over-long name instead of breaking the columns', async () => { listLiveSessions.mockResolvedValue([record({ name: 'x'.repeat(80) })]); await run({ json: false }); diff --git a/packages/cli/src/commands/sessions/ps.ts b/packages/cli/src/commands/sessions/ps.ts index 1b1360acdd7..88ff5a769b3 100644 --- a/packages/cli/src/commands/sessions/ps.ts +++ b/packages/cli/src/commands/sessions/ps.ts @@ -5,11 +5,16 @@ */ /** - * `qwen sessions ps` — list the Qwen Code sessions running right now. + * `qwen sessions ps` — list the interactive 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". + * + * "Interactive" is a registration fact, not a filter: only the + * interactive UI registers sessions, so headless runs (`qwen -p`) never + * appear here. */ import type { CommandModule, Argv } from 'yargs'; @@ -98,13 +103,18 @@ async function handlePs(argv: PsArgs): Promise { if (argv.json) { for (const record of records) { + // Deliberately raw: field values are emitted exactly as recorded, + // with none of the table path's terminal sanitization. That keeps + // the output honest data for tooling (and matches the sibling + // `sessions list --json`); consumers that RENDER these values in a + // terminal own the sanitization. writeStdoutLine(JSON.stringify(record)); } return; } if (records.length === 0) { - writeStdoutLine('No other Qwen Code sessions are running.'); + writeStdoutLine('No other interactive Qwen Code sessions are running.'); return; } @@ -113,7 +123,7 @@ async function handlePs(argv: PsArgs): Promise { export const psCommand: CommandModule = { command: 'ps', - describe: 'List Qwen Code sessions running right now', + describe: 'List interactive Qwen Code sessions running right now', builder: (yargs: Argv) => yargs.option('json', { type: 'boolean', diff --git a/packages/cli/src/ui/startInteractiveUI.test.tsx b/packages/cli/src/ui/startInteractiveUI.test.tsx new file mode 100644 index 00000000000..1d7386453af --- /dev/null +++ b/packages/cli/src/ui/startInteractiveUI.test.tsx @@ -0,0 +1,126 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + * + * Pins the session-registry wiring in startInteractiveUI: registration + * arguments, cleanup armed only on success, and failures swallowed. + * Deleting the import or the registration block keeps every other test + * green — without this file, interactive sessions could silently stop + * appearing in `qwen sessions ps` (or never disappear from it). + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import type { Config } from '@qwen-code/qwen-code-core'; +import type { LoadedSettings } from '../config/settings.js'; +import type { InitializationResult } from '../core/initializer.js'; + +const registerSession = vi.hoisted(() => vi.fn()); +const unregisterSession = vi.hoisted(() => vi.fn()); +const registerCleanup = vi.hoisted(() => vi.fn()); + +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + registerSession: (...args: unknown[]) => registerSession(...args), + unregisterSession: (...args: unknown[]) => unregisterSession(...args), + }; +}); + +vi.mock('ink', () => ({ + render: vi.fn(() => ({ unmount: vi.fn() })), +})); + +vi.mock('../utils/cleanup.js', () => ({ + registerCleanup: (...args: unknown[]) => registerCleanup(...args), + runExitCleanup: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../utils/version.js', () => ({ + getCliVersion: vi.fn(() => Promise.resolve('9.9.9')), +})); + +vi.mock('../startup/startup-prefetch.js', () => ({ + startPostRenderPrefetches: vi.fn(), +})); + +vi.mock('../utils/earlyInputCapture.js', () => ({ + stopAndGetCapturedInput: vi.fn(() => ''), +})); + +const { startInteractiveUI } = await import('./startInteractiveUI.js'); + +function makeConfig(): Config { + return { + getSessionId: () => 'session-123', + getTargetDir: () => '/work/app', + getScreenReader: () => false, + getChatRecordingService: () => undefined, + isTelemetryInitializationDeferred: () => false, + } as unknown as Config; +} + +const settings = { + merged: { ui: { hideWindowTitle: true } }, +} as unknown as LoadedSettings; + +const initializationResult = { + authError: null, + themeError: null, + shouldOpenAuthDialog: false, + geminiMdFileCount: 0, +} as InitializationResult; + +async function start(): Promise { + await startInteractiveUI( + makeConfig(), + settings, + [], + '/work/app', + initializationResult, + ); +} + +describe('startInteractiveUI session registration', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('registers the session with its id, target dir, and CLI version', async () => { + registerSession.mockResolvedValue(true); + + await start(); + + expect(registerSession).toHaveBeenCalledWith({ + sessionId: 'session-123', + cwd: '/work/app', + qwenVersion: '9.9.9', + }); + }); + + it('arms the unregister cleanup only when registration succeeded', async () => { + // startInteractiveUI registers exactly one other cleanup (the UI + // teardown) unconditionally, so the count differs by exactly one + // depending on whether the unregister callback was armed. + registerSession.mockResolvedValue(true); + await start(); + expect(registerCleanup).toHaveBeenCalledTimes(2); + expect(unregisterSession).not.toHaveBeenCalled(); + + vi.clearAllMocks(); + registerSession.mockResolvedValue(false); + await start(); + expect(registerCleanup).toHaveBeenCalledTimes(1); + }); + + it('swallows a registration rejection without aborting startup', async () => { + // A read-only home must not keep the TUI from launching; the + // registration is wrapped precisely so its failure cannot propagate. + registerSession.mockRejectedValue(new Error('read-only home')); + + await expect(start()).resolves.toBeUndefined(); + expect(registerCleanup).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/core/src/agents/team/teamHelpers.test.ts b/packages/core/src/agents/team/teamHelpers.test.ts index 80f54920bc8..8e2fa5eb731 100644 --- a/packages/core/src/agents/team/teamHelpers.test.ts +++ b/packages/core/src/agents/team/teamHelpers.test.ts @@ -379,6 +379,10 @@ describe('file I/O', () => { }); describe('tryReclaimStaleTeam', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + /** PID of a process that has already exited. */ function deadPid(): number { const child = spawnSync(process.execPath, ['-e', '']); @@ -424,5 +428,21 @@ describe('file I/O', () => { await expect(tryReclaimStaleTeam('ghost')).resolves.toBe(true); await expect(fs.access(tasksDir)).rejects.toThrow(); }); + + it('does not reclaim when the lead PID is alive but owned by another user', async () => { + // Pins teamHelpers to the shared isPidAlive: EACCES (Windows' + // other-user errno, alongside EPERM) means the process exists — + // the duplicate local copy this replaced treated it as dead and + // would have reclaimed a live team. + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('access denied'), { + code: 'EACCES', + }); + }); + await writeTeamFile('other-user', makeTeamFile({ leadPid: 424242 })); + + await expect(tryReclaimStaleTeam('other-user')).resolves.toBe(false); + expect(await readTeamFile('other-user')).toBeDefined(); + }); }); }); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 4279991a0b1..029e4e76ae4 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -81,6 +81,7 @@ import { import { syncTeamMemory } from '../memory/team-memory-sync.js'; import { getTeamMemoryShareabilityWarning } from '../memory/team-memory-git-status.js'; import * as runtimeStatus from '../utils/runtimeStatus.js'; +import * as sessionRegistry from '../services/session-registry.js'; import { ExtensionManager } from '../extension/extensionManager.js'; import { SkillManager } from '../skills/skill-manager.js'; import { maybeRunAutoSkillCurator } from '../skills/skill-curator.js'; @@ -6073,6 +6074,10 @@ describe('Server Config (config.ts)', () => { return checked === oldRuntimeStatusPath || checked === newDir; }); + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockResolvedValue(undefined); + await config.relocateWorkingDirectory(newDir); expect(fs.renameSync).toHaveBeenCalledWith( @@ -6084,8 +6089,16 @@ describe('Server Config (config.ts)', () => { workDir: newDir, qwenVersion: null, }); + // The registry's DIRECTORY column is how a user tells two live + // sessions apart; the switch must reach it (and the directory-derived + // name) or `qwen sessions ps` keeps showing the folder that was left. + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + cwd: newDir, + name: sessionRegistry.deriveSessionName(newDir, sessionId), + }); writeRuntimeStatusSpy.mockRestore(); + patchSessionRecordSpy.mockRestore(); chdirSpy.mockRestore(); cwdSpy.mockRestore(); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 5e57d4f2437..9e6481a6d26 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -216,7 +216,10 @@ import { clearRuntimeStatus, writeRuntimeStatus, } from '../utils/runtimeStatus.js'; -import { patchSessionRecord } from '../services/session-registry.js'; +import { + deriveSessionName, + patchSessionRecord, +} from '../services/session-registry.js'; import { SessionService, type ResumedSessionData, @@ -3963,8 +3966,13 @@ export class Config { // The registry's DIRECTORY column is how a user tells two live // sessions apart, so a mid-session directory switch has to reach it // too — otherwise `qwen sessions ps` keeps advertising the folder - // this session left. - await patchSessionRecord({ cwd: workDir }); + // this session left. Unlike the /clear path, `name` follows: it is + // derived from the directory's basename, which is exactly what + // changed here. + await patchSessionRecord({ + cwd: workDir, + name: deriveSessionName(workDir, this.sessionId), + }); }); await this.flushRuntimeStatusWrites(); } diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index c767c7309a3..35c38e0c4c6 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 { readPidNamespaceId } from '../utils/process-liveness.js'; /** * Records the paths `readRecord` stats, while the real filesystem does the @@ -46,12 +47,20 @@ vi.mock('node:fs/promises', async () => { }); vi.mock('../config/storage.js', () => { - let mockDir = '/tmp/session-registry-test'; + let mockDir: string | null = '/tmp/session-registry-test'; return { Storage: { - getGlobalQwenDir: () => mockDir, + getGlobalQwenDir: () => { + if (mockDir === null) { + // Simulates os.homedir() failing (HOME unset, passwd lookup + // gone) — the registry's "never throws" promise is tested + // against exactly this. + throw new Error('home directory unavailable'); + } + return mockDir; + }, }, - __setMockGlobalDir: (d: string) => { + __setMockGlobalDir: (d: string | null) => { mockDir = d; }, }; @@ -99,6 +108,10 @@ function liveBody(over: Record = {}): Record { schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, pid: process.pid, procStart: null, + // Records planted here model a writer in THIS process, so the + // namespace identity is the caller's own — anything else would be + // skipped by the namespace guard before its fields even matter. + pidNs: readPidNamespaceId(), sessionId: 's', cwd: '/w', name: 'n', @@ -131,10 +144,25 @@ describe('deriveSessionName', () => { expect(name).toMatch(/^[\w.-]+$/); }); + it('keeps non-ASCII letters instead of stripping them to a dash', () => { + // An ASCII-only character class reduces every CJK basename to the + // same bare dash — zero identifying information for exactly the + // projects whose names are not ASCII. + const a = deriveSessionName('/home/u/项目', 's1'); + const b = deriveSessionName('/home/u/別項目', 's1'); + expect(a).toMatch(/^项目-[0-9a-f]{2}$/); + expect(b).toMatch(/^別項目-[0-9a-f]{2}$/); + expect(a).not.toBe(b); + }); + it('falls back to a placeholder when the basename is empty', () => { expect(deriveSessionName('/', 's1')).toMatch(/^session-[0-9a-f]{2}$/); }); + it('falls back to a placeholder when the basename strips to dashes only', () => { + expect(deriveSessionName('/w/!!!', 's1')).toMatch(/^session-[0-9a-f]{2}$/); + }); + it('caps the basename at 32 characters so the name fits a table cell', () => { const name = deriveSessionName(`/w/${'a'.repeat(80)}`, 's1'); expect(name).toMatch(/^a{32}-[0-9a-f]{2}$/); @@ -164,6 +192,41 @@ describe('registerSession', () => { expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); }); + it('records the writer’s PID namespace identity', async () => { + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw['pidNs']).toBe(readPidNamespaceId()); + }); + + it('records an explicit null qwenVersion when it is omitted', async () => { + // The key must exist as null, not be silently dropped by + // JSON.stringify(undefined): the record format has a declared + // schemaVersion, and schema drift on an optional field is still drift. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw).toHaveProperty('qwenVersion', null); + }); + + // Only Linux has a start token to record; elsewhere this is a visible + // skip rather than a test that passes without asserting. + it.runIf(process.platform === 'linux')( + 'records the live start token on registration', + async () => { + // The writer side of the PID-reuse guard: a null token here would + // degrade every later liveness check to a bare kill(pid, 0) and + // let a recycled PID resurrect this record after exit. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw['procStart']).toMatch(/^[0-9a-f-]+:\d+$/i); + }, + ); + // Windows synthesizes st_mode from file attributes and `chmod` can only // toggle the read-only bit, so permission assertions are meaningless // there. Guarded rather than deleted, the same way @@ -235,6 +298,25 @@ describe('registerSession', () => { }); }); +describe('never-throw guarantee', () => { + it('every entry point resolves when the home directory cannot be resolved', async () => { + // `os.homedir()` throws when HOME is unset and the passwd lookup + // fails (some containers/CI images). `ps` has no catch on the + // strength of the registry's promise, so a rejection here would be + // an unhandled rejection in exactly that environment. + __setMockGlobalDir(null); + + await expect(listLiveSessions()).resolves.toEqual([]); + await expect( + patchSessionRecord({ sessionId: 'new' }), + ).resolves.toBeUndefined(); + await expect( + registerSession({ sessionId: 's1', cwd: '/w/app' }), + ).resolves.toBe(false); + await expect(unregisterSession()).resolves.toBeUndefined(); + }); +}); + describe('patchSessionRecord', () => { it('updates a field without dropping the others', async () => { await registerSession({ @@ -279,6 +361,38 @@ describe('patchSessionRecord', () => { expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toEqual(foreign); }); + it.runIf(process.platform === 'linux')( + 'refuses to merge into a stale record left by a dead previous incarnation of this PID', + async () => { + // Session A died without unlinking (SIGKILL); PID P was recycled by + // session B whose registration failed. B's patch passes the pid + // comparison alone — only the start token proves the record is not + // A's, and without it the merge would graft B's fields onto A's + // startedAt/version/name and list the chimera as live. + const filePath = await writeRaw( + `${process.pid}.json`, + liveBody({ procStart: 'not-this-boot:1', sessionId: 'incarnation-a' }), + ); + + await patchSessionRecord({ sessionId: 'incarnation-b' }); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toMatchObject({ + sessionId: 'incarnation-a', + }); + }, + ); + + it('still patches a record written without a start token', async () => { + // Tokenless platforms must keep working through the pid comparison — + // the guard only fires when BOTH sides have a token to compare. + await writeRaw(`${process.pid}.json`, liveBody()); + + await patchSessionRecord({ sessionId: 'new' }); + + const [record] = await listLiveSessions(); + expect(record.sessionId).toBe('new'); + }); + const itPosixPatch = it.runIf(process.platform !== 'win32'); itPosixPatch('keeps the record at 0600 across a patch', async () => { @@ -310,16 +424,16 @@ describe('listLiveSessions', () => { }); 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', - startedAt: Date.now(), - qwenVersion: null, - }); + const filePath = await writeRaw( + `${DEAD_PID}.json`, + liveBody({ + pid: DEAD_PID, + sessionId: 's-dead', + cwd: '/w/app', + name: 'app-aa', + startedAt: Date.now(), + }), + ); expect(await listLiveSessions()).toEqual([]); await expect(fs.stat(filePath)).rejects.toThrow(); @@ -332,21 +446,50 @@ describe('listLiveSessions', () => { 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. - const filePath = await writeRaw(`${process.pid}.json`, { - schemaVersion: 1, - pid: process.pid, - procStart: 'not-this-boot:1', - sessionId: 's-recycled', - cwd: '/w/app', - name: 'app-aa', - startedAt: Date.now(), - }); + const filePath = await writeRaw( + `${process.pid}.json`, + liveBody({ + procStart: 'not-this-boot:1', + sessionId: 's-recycled', + cwd: '/w/app', + name: 'app-aa', + startedAt: Date.now(), + }), + ); expect(await listLiveSessions()).toEqual([]); await expect(fs.stat(filePath)).rejects.toThrow(); }, ); + it('neither lists nor sweeps a record from a different PID namespace, even a dead one', async () => { + // PID numbers do not resolve across the namespace boundary: kill(pid, 0) + // over here reports ESRCH for a process alive over there, and a + // "matching" starttime can belong to an unrelated process. Liveness + // proved on the wrong side is worse than no answer, so the record is + // left for a reader on the writer's own side — even when its PID + // looks dead here. + const filePath = await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, pidNs: 1 }), + ); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }); + + it('does not list a foreign-namespace record under a live PID', async () => { + // The sharp end of the guard: without it this record passes plain + // liveness (the PID is us) and is listed as our session. + const filePath = await writeRaw( + `${process.pid}.json`, + liveBody({ pidNs: 1 }), + ); + + expect(await listLiveSessions()).toEqual([]); + 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'); @@ -440,6 +583,14 @@ describe('listLiveSessions', () => { expect(await listLiveSessions()).toEqual([liveBody()]); }); + it('drops unknown fields rather than passing them on', async () => { + // Without the drop, arbitrary keys from a hand-planted .json + // would ride the typed record into `ps --json` output — and be + // re-persisted permanently by patchSessionRecord's merge. + await writeRaw(`${process.pid}.json`, { ...liveBody(), extraField: 'x' }); + expect(await listLiveSessions()).toEqual([liveBody()]); + }); + it('nulls optional fields of the wrong type rather than passing them on', async () => { // A numeric `procStart` handed to `isSameProcess` would never equal the // string token it reads back, so a live session would be swept; a @@ -503,14 +654,16 @@ describe('listLiveSessions', () => { cwd: '/w/app', }); await patchSessionRecord({ startedAt: 1000 }); - await writeRaw(`${process.ppid}.json`, { - schemaVersion: 1, - pid: process.ppid, - sessionId: 's-parent', - cwd: '/w/other', - name: 'other-bb', - startedAt: 2000, - }); + await writeRaw( + `${process.ppid}.json`, + liveBody({ + pid: process.ppid, + sessionId: 's-parent', + cwd: '/w/other', + name: 'other-bb', + startedAt: 2000, + }), + ); const live = await listLiveSessions(); 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 index baab0a25b4e..e7da7261633 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -32,11 +32,14 @@ * * ## 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. + * A record is live when it was written from the reader's own PID + * namespace and 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 the liveness check are + * swept during enumeration; records from another PID namespace are + * neither listed nor swept, because PID numbers do not resolve across + * that boundary in either direction. Anything else we cannot positively + * prove dead is left alone. */ import { createHash } from 'node:crypto'; @@ -47,6 +50,7 @@ import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSameProcess, + readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; @@ -82,6 +86,11 @@ export interface SessionRegistryRecord { pid: number; /** Start-time token guarding against PID reuse; null where unavailable. */ procStart: string | null; + /** + * PID-namespace identity of the writer (see `readPidNamespaceId`); + * null where the platform does not expose one. + */ + pidNs: number | null; sessionId: string; cwd: string; /** Short human-facing label, unique-ish per session. */ @@ -115,11 +124,16 @@ export function getSessionRecordPath(): string { * 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. + * + * Letters and digits are matched Unicode-aware: an ASCII-only class would + * strip a CJK basename down to a bare dash, leaving every such project + * with an identical, information-free label. */ export function deriveSessionName(cwd: string, sessionId: string): string { const base = path .basename(cwd) - .replace(/[^\w.-]+/g, '-') + .replace(/[^\p{L}\p{N}._-]+/gu, '-') + .replace(/^-+|-+$/g, '') .slice(0, 32); const suffix = createHash('sha256') .update(sessionId) @@ -142,6 +156,7 @@ export async function registerSession( schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, pid: process.pid, procStart: readProcStartToken(process.pid), + pidNs: readPidNamespaceId(), sessionId: fields.sessionId, cwd: fields.cwd, name: deriveSessionName(fields.cwd, fields.sessionId), @@ -178,18 +193,39 @@ 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. + * `procStart` and `pidNs` are excluded from the patch for the same + * reason the pid is: they are the identity the sweep trusts, and a + * caller-supplied value could only corrupt it. */ export async function patchSessionRecord( - patch: Partial>, + patch: Partial< + Omit + >, ): Promise { - const filePath = getSessionRecordPath(); try { + // Inside the try: `getGlobalQwenDir()` resolves the home directory + // and can throw, and this function promises never to reject. + const filePath = getSessionRecordPath(); const existing = await readRecord(filePath); // Missing, or not actually a record for this PID: `readRecord` does // not check the filename/contents agreement that `listLiveSessions` // insists on, so merging into a foreign `.json` would write back // a record the reader will neither show nor sweep — permanent litter. if (existing === null || existing.pid !== process.pid) return; + // The pid comparison alone also passes for a stale record left by a + // DEAD previous incarnation of this PID (session A dies without + // unlinking; the PID is recycled by session B whose registration + // failed). When both sides carry a start token, require it to agree + // before merging — otherwise the patch grafts B's fields onto A's + // record and lists the chimera as live. + const currentToken = readProcStartToken(process.pid); + if ( + existing.procStart !== null && + currentToken !== null && + existing.procStart !== currentToken + ) { + return; + } await atomicWriteJSON( filePath, { ...existing, ...patch }, @@ -234,6 +270,7 @@ export async function listLiveSessions(): Promise { return []; } + const ownNamespace = readPidNamespaceId(); const live: SessionRegistryRecord[] = []; await Promise.all( entries @@ -248,6 +285,13 @@ export async function listLiveSessions(): Promise { // never sweep it — we cannot reason about which PID it describes. if (`${record.pid}.json` !== name) return; + // A record from another PID namespace describes PIDs that do not + // resolve in ours: kill(pid, 0) reports ESRCH for a process that + // is alive over there, and a "matching" starttime can belong to + // an unrelated process. Neither listing nor sweeping is safe — + // leave it to a reader on the writer's own side. + if (record.pidNs !== ownNamespace) return; + if (isSameProcess(record.pid, record.procStart)) { live.push(record); return; @@ -318,12 +362,14 @@ async function readRecord( } const procStart = value['procStart']; + const pidNs = value['pidNs']; const qwenVersion = value['qwenVersion']; return { schemaVersion, pid, procStart: typeof procStart === 'string' ? procStart : null, + pidNs: typeof pidNs === 'number' && Number.isFinite(pidNs) ? pidNs : 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 d6cdeba2ae5..ee59f7d05d2 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, afterEach, vi } from 'vitest'; import { isPidAlive, isSameProcess, + readPidNamespaceId, readProcStartToken, } from './process-liveness.js'; @@ -27,19 +28,22 @@ const BOOT_ID_PATH = '/proc/sys/kernel/random/boot_id'; * wrong index off a real process still yields something that looks like a * valid token. */ -function statLine(comm: string, startTime: string): string { +function statLine(comm: string, startTime: string, state = 'S'): string { // Fields 3..22. Once the parenthesised `comm` is stripped, field N sits // at index N - 3, so `startTime` (field 22, `starttime`) is the // twentieth entry. The neighbours are deliberately distinct values so an - // off-by-one read is visible. + // off-by-one read is visible. Field 3 is the process state. // prettier-ignore const fields = [ - 'S', '1', '2', '3', '4', '-1', '4194304', '100', '0', '200', + state, '1', '2', '3', '4', '-1', '4194304', '100', '0', '200', '0', '10', '20', '30', '40', '20', '0', '1', '0', startTime, ]; return `4242 (${comm}) ${fields.join(' ')} 1000 2000 3000\n`; } +const PID_NS_PATH = '/proc/self/ns/pid'; +const FAKE_PID_NS_INO = 4026531836; + interface FakeProc { mod: typeof import('./process-liveness.js'); reads: string[]; @@ -50,7 +54,12 @@ interface FakeProc { * the platform forced to Linux, so the parser is exercised on every CI * runner rather than only the Linux one. */ -async function withFakeProc(files: Record): Promise { +async function withFakeProc( + files: Record, + options: { pidNsIno?: number | null } = {}, +): Promise { + const pidNsIno = + options.pidNsIno === undefined ? FAKE_PID_NS_INO : options.pidNsIno; const reads: string[] = []; vi.resetModules(); vi.doMock('node:fs', () => ({ @@ -62,6 +71,13 @@ async function withFakeProc(files: Record): Promise { } return body; }, + statSync: (p: unknown) => { + reads.push(String(p)); + if (String(p) === PID_NS_PATH && pidNsIno !== null) { + return { ino: pidNsIno }; + } + throw Object.assign(new Error(`ENOENT: ${p}`), { code: 'ENOENT' }); + }, })); vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); const mod = await import('./process-liveness.js'); @@ -109,6 +125,34 @@ describe('isPidAlive', () => { }); expect(isPidAlive(4242)).toBe(false); }); + + // On Windows the "process exists but is owned by another user" errno is + // EACCES, not EPERM; missing it there would let a sweep delete a live + // session's record. + it('treats EACCES — Windows’ other-user errno — as alive', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('access denied'), { code: 'EACCES' }); + }); + expect(isPidAlive(4242)).toBe(true); + }); + + it('treats a zombie — exited but unreaped — as dead on Linux', async () => { + // A zombie still answers kill(pid, 0): the PID exists until the parent + // reaps it. Only the state field of /proc//stat proves it has + // already exited, so without the Z check the record stays listed for + // the parent's whole lifetime. + const { mod } = await withFakeProc({ + '/proc/4242/stat': statLine('qwen', '987654', 'Z'), + }); + vi.spyOn(process, 'kill').mockImplementation(() => true); + expect(mod.isPidAlive(4242)).toBe(false); + }); + + it('keeps a live process whose /proc state cannot be read', async () => { + const { mod } = await withFakeProc({}); + vi.spyOn(process, 'kill').mockImplementation(() => true); + expect(mod.isPidAlive(4242)).toBe(true); + }); }); describe('readProcStartToken', () => { @@ -191,6 +235,22 @@ describe('readProcStartToken', () => { expect(reads.filter((p) => p.endsWith('/stat'))).toHaveLength(2); }); + it('retries the boot id after a failed read instead of caching the failure', async () => { + // Both first-read moments — startup registration and the first + // concurrent sweep — are fd-pressure moments. Caching a transient + // EMFILE as a permanent null would silently disable PID-reuse + // protection for the whole process lifetime. + const files: Record = { + '/proc/4242/stat': statLine('qwen', '987654'), + }; + const { mod, reads } = await withFakeProc(files); + + expect(mod.readProcStartToken(4242)).toBeNull(); + files[BOOT_ID_PATH] = `${FAKE_BOOT_ID}\n`; + expect(mod.readProcStartToken(4242)).toBe(`${FAKE_BOOT_ID}:987654`); + expect(reads.filter((p) => p === BOOT_ID_PATH)).toHaveLength(2); + }); + it('rejects nonsense pids before touching /proc', async () => { const { mod, reads } = await withFakeProc({ [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, @@ -205,6 +265,23 @@ describe('readProcStartToken', () => { }); }); +describe('readPidNamespaceId', () => { + it('returns the PID namespace inode on Linux', async () => { + const { mod } = await withFakeProc({}); + expect(mod.readPidNamespaceId()).toBe(FAKE_PID_NS_INO); + }); + + it('returns null when the namespace file is unreadable', async () => { + const { mod } = await withFakeProc({}, { pidNsIno: null }); + expect(mod.readPidNamespaceId()).toBeNull(); + }); + + it('returns null on platforms without /proc', () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin'); + expect(readPidNamespaceId()).toBeNull(); + }); +}); + describe('isSameProcess', () => { it('is false for a dead pid regardless of token', () => { expect(isSameProcess(DEAD_PID, null)).toBe(false); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index 5241b93848e..61d72740c6e 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -22,26 +22,57 @@ 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. + * `EPERM` (and Windows' `EACCES`) 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; + // A zombie answers kill(pid, 0) — the PID still exists — but the + // process has already exited and only waits for its parent to reap + // it. It will never act again, so for registry purposes it is dead; + // without this, a spawn-but-never-wait parent keeps the record + // listed for its entire lifetime. + return !isZombie(pid); } catch (err) { - return isNodeError(err) && err.code === 'EPERM'; + return isNodeError(err) && (err.code === 'EPERM' || err.code === 'EACCES'); } } +/** + * True when `pid` is a zombie: exited but not yet reaped. Field 3 + * (state) of the same `/proc//stat` line the token parser reads + * carries it; `Z` is the one state that means "no longer running". + * Unreadable `/proc` degrades to "not a zombie" — the conservative + * direction, since the sweep only ever acts on a positive answer. + */ +function isZombie(pid: number): boolean { + if (process.platform !== 'linux') return false; + let raw: string; + try { + raw = fs.readFileSync(`/proc/${pid}/stat`, 'utf8'); + } catch { + return false; + } + // The state is the first token after the parenthesized `comm`, which + // may itself contain spaces and ')' — anchor on the LAST ')'. + const commEnd = raw.lastIndexOf(')'); + if (commEnd === -1) return false; + return raw + .slice(commEnd + 1) + .trimStart() + .startsWith('Z'); +} + /** * An opaque token that changes when a PID is recycled, or `null` when the * platform does not expose one cheaply. * - * Backed by `boot_id` plus the `starttime` field of `/proc//stat` on - * Linux — the process start time in clock ticks since boot. Two processes + * Backed by `boot_id` plus the `starttime` field of `/proc//stat` + * on Linux — the process start time in clock ticks since boot. Two processes * sharing a PID within one boot will not share `starttime`; across a * reboot they can, and a registry record outlives a reboot whenever the * machine crashes or loses power, so the boot id is what makes every @@ -98,24 +129,50 @@ export function readProcStartToken(pid: number): string | null { } /** - * The kernel's per-boot UUID, or `null` when it cannot be read. Cached: it - * cannot change while this process lives, and enumeration reads a token - * per record. + * The kernel's per-boot UUID, or `null` when it cannot be read. Successes + * are cached — the value cannot change while this process lives, and + * enumeration reads a token per record — but failures are not: both + * first-read moments (startup registration, first concurrent sweep) are + * fd-pressure moments, and pinning the cache to a transient EMFILE would + * silently disable PID-reuse protection for the whole process lifetime. */ -let cachedBootId: string | null | undefined; +let cachedBootId: string | undefined; function readBootId(): string | null { - if (cachedBootId === undefined) { - try { - const value = fs - .readFileSync('/proc/sys/kernel/random/boot_id', 'utf8') - .trim(); - cachedBootId = /^[0-9a-f-]+$/i.test(value) ? value : null; - } catch { - cachedBootId = null; + if (cachedBootId !== undefined) return cachedBootId; + try { + const value = fs + .readFileSync('/proc/sys/kernel/random/boot_id', 'utf8') + .trim(); + if (/^[0-9a-f-]+$/i.test(value)) { + cachedBootId = value; + return value; } + } catch { + // Retried on the next call. + } + return null; +} + +/** + * The identity of the PID namespace this process lives in (the inode of + * `/proc/self/ns/pid`), or `null` where the platform does not expose it. + * + * PID numbers and start-time tokens are only meaningful within the + * namespace that assigned them: two sessions in separate namespaces can + * share one `~/.qwen` (host + devcontainer with a mounted home, sibling + * CI containers, NFS homes), and each side's sweep would otherwise judge + * the other's records by PIDs that resolve to nothing — or worse, to a + * different process — on its own side. Records carry this identity so a + * reader can tell its own namespace's records from a foreign one's. + */ +export function readPidNamespaceId(): number | null { + if (process.platform !== 'linux') return null; + try { + return fs.statSync('/proc/self/ns/pid').ino; + } catch { + return null; } - return cachedBootId; } /** diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index 9865844aad9..937663141d5 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -18,6 +18,11 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; import { readRuntimeStatus, writeRuntimeStatus } from './runtimeStatus.js'; +import { + listLiveSessions, + registerSession, + unregisterSession, +} from '../services/session-registry.js'; let tmpDir: string; let runtimeDir: string; @@ -150,6 +155,52 @@ describe('Config.startNewSession runtime.json swap', () => { }); }); +describe('Config.startNewSession session-registry patch', () => { + let prevQwenHome: string | undefined; + + beforeEach(() => { + // Keep the registry inside this test's tmpdir — the patch seam must + // be exercised against the real registerSession/listLiveSessions + // round trip, and the default location is the runner's real home. + prevQwenHome = process.env['QWEN_HOME']; + process.env['QWEN_HOME'] = path.join(tmpDir, 'qwen-home'); + }); + + afterEach(async () => { + await unregisterSession(); + if (prevQwenHome === undefined) { + delete process.env['QWEN_HOME']; + } else { + process.env['QWEN_HOME'] = prevQwenHome; + } + }); + + it('keeps the registry record pointing at the swapped session id', async () => { + const sessionA = 'aaaaaaaa-1111-2222-3333-aaaaaaaaaaaa'; + const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; + const config = makeConfig(sessionA); + config.markRuntimeStatusEnabled(); + await registerSession({ + sessionId: sessionA, + cwd: tmpDir, + qwenVersion: '0.0.0-test', + }); + + config.startNewSession(sessionB); + + // Without the patch seam, `qwen sessions ps --json` would keep + // advertising sessionA's transcript for this live session — the exact + // stale-pointer bug the seam exists to prevent. + const after = await waitFor(async () => { + const [record] = await listLiveSessions(); + return record?.sessionId === sessionB ? record : null; + }); + expect(after).not.toBeNull(); + expect(after!.pid).toBe(process.pid); + expect(after!.cwd).toBe(tmpDir); + }); +}); + describe('Storage.getRuntimeStatusPath', () => { it('co-locates the sidecar under /chats/', () => { const storage = new Storage(tmpDir); From fe54a9235b5a3b4b966f893a8d686e68d270e576 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 15:08:49 +0000 Subject: [PATCH 3/8] fix(core): guard session registry identities per machine and boot (#8969) --- packages/cli/src/commands/sessions/ps.test.ts | 4 +- .../cli/src/ui/startInteractiveUI.test.tsx | 7 + packages/cli/src/ui/startInteractiveUI.tsx | 6 +- packages/core/src/config/config.test.ts | 15 +- packages/core/src/config/config.ts | 113 +++++---- .../src/services/session-registry.test.ts | 203 +++++++++++++++- .../core/src/services/session-registry.ts | 228 ++++++++++++++---- .../core/src/utils/process-liveness.test.ts | 47 +++- packages/core/src/utils/process-liveness.ts | 22 +- .../src/utils/runtimeStatus.config.test.ts | 32 +++ 10 files changed, 562 insertions(+), 115 deletions(-) diff --git a/packages/cli/src/commands/sessions/ps.test.ts b/packages/cli/src/commands/sessions/ps.test.ts index 72af57d06a2..cc2097106a5 100644 --- a/packages/cli/src/commands/sessions/ps.test.ts +++ b/packages/cli/src/commands/sessions/ps.test.ts @@ -162,12 +162,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\tc' }), + record({ name: 'ev\x1b[31mil\r', cwd: '/w/a\nb\tc' }), ]); await run({ json: false }); const row = stdout[1]; - expect(row).not.toContain(''); + expect(row).not.toContain('\x1b'); expect(row).not.toContain('\r'); expect(row).not.toContain('\n'); // sanitizeTerminalText deliberately preserves TAB for multi-line diff --git a/packages/cli/src/ui/startInteractiveUI.test.tsx b/packages/cli/src/ui/startInteractiveUI.test.tsx index 1d7386453af..26a05394ac1 100644 --- a/packages/cli/src/ui/startInteractiveUI.test.tsx +++ b/packages/cli/src/ui/startInteractiveUI.test.tsx @@ -108,6 +108,13 @@ describe('startInteractiveUI session registration', () => { await start(); expect(registerCleanup).toHaveBeenCalledTimes(2); expect(unregisterSession).not.toHaveBeenCalled(); + // Armed is not the contract — invoke the armed callback: a future + // edit emptying its body would pass the call-count pins while every + // session's record survives exit and `ps` lists it as running. + const armUnregister = registerCleanup.mock + .calls[0]?.[0] as () => Promise; + await armUnregister(); + expect(unregisterSession).toHaveBeenCalledTimes(1); vi.clearAllMocks(); registerSession.mockResolvedValue(false); diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index fa125f58aa5..216cd75fafd 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -286,7 +286,7 @@ export async function startInteractiveUI( config.isTelemetryInitializationDeferred(), }); - // Announce this session in the machine-wide registry so sibling + // Announce this session in the session 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". @@ -310,6 +310,10 @@ export async function startInteractiveUI( // Only arm cleanup for a record that exists; registration fails on // a read-only home, and there is then nothing to unlink. registerCleanup(() => unregisterSession()); + // Arms the mid-session registry patches (`/clear`, `/cd`) on this + // Config; without it the record would keep advertising the values + // the session started with. + config.markSessionRegistered(); } } catch (err) { debugLogger.debug(`session registration skipped: ${String(err)}`); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 029e4e76ae4..85ac687aa64 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -6056,6 +6056,7 @@ describe('Server Config (config.ts)', () => { it('relocateWorkingDirectory should refresh runtime status after moving session artifacts', async () => { const config = new Config(baseParams); config.markRuntimeStatusEnabled(); + config.markSessionRegistered(); const sessionId = config.getSessionId(); const newDir = path.resolve('/path/to/other'); const oldStorage = new Storage(config.getTargetDir()); @@ -6074,11 +6075,22 @@ describe('Server Config (config.ts)', () => { return checked === oldRuntimeStatusPath || checked === newDir; }); + // `toHaveBeenCalledWith` records the call the moment the promise is + // created, so it cannot distinguish an awaited patch from a + // fire-and-forget one; the settlement log pins that the registry + // update completes before `relocateWorkingDirectory` resolves — the + // user-visible guarantee is that `ps` already shows the new + // directory once `/cd` returns. + const settled: string[] = []; const patchSessionRecordSpy = vi .spyOn(sessionRegistry, 'patchSessionRecord') - .mockResolvedValue(undefined); + .mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + settled.push('patch'); + }); await config.relocateWorkingDirectory(newDir); + settled.push('relocated'); expect(fs.renameSync).toHaveBeenCalledWith( oldRuntimeStatusPath, @@ -6096,6 +6108,7 @@ describe('Server Config (config.ts)', () => { cwd: newDir, name: sessionRegistry.deriveSessionName(newDir, sessionId), }); + expect(settled).toEqual(['patch', 'relocated']); writeRuntimeStatusSpy.mockRestore(); patchSessionRecordSpy.mockRestore(); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 9e6481a6d26..38462c9368d 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1989,6 +1989,7 @@ export class Config { private readonly cliVersion?: string; private runtimeStatusEnabled = false; + private sessionRegistered = false; private readonly experimentalZedIntegration: boolean = false; private readonly sessionWriterLeaseEnabled: boolean = false; private readonly cronEnabled: boolean = true; @@ -3883,38 +3884,47 @@ export class Config { // 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) { - 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 (previousSessionId !== this.sessionId) { + if (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, + }); }); - // Keep the machine-wide session registry in step: this PID's - // record would otherwise point discovery at the previous - // transcript. Keyed by PID, so a swap is a patch rather than a - // delete-and-rewrite. + } + if (this.sessionRegistered) { + const workDir = this.targetDir; + const newSessionId = this.sessionId; + // Keep the session registry in step: this PID's record would + // otherwise point discovery at the previous transcript. Keyed by + // PID, so a swap is a patch rather than a delete-and-rewrite. // - // This rides the sidecar's `runtimeStatusEnabled` gate rather - // than having its own. Not the same rule — the sidecar is gated - // because a short-lived process must not delete a sibling's - // file, and PID-keyed records have no such hazard — but the two - // lifecycles coincide, and the only divergence (sidecar write - // failed, registration succeeded) costs a stale `sessionId` in - // `ps --json` until exit. + // Gated on registration success rather than the sidecar's + // `runtimeStatusEnabled`: the failure domains are independent + // (project-local `chats/` dir vs the global dir), and riding the + // sidecar gate would skip every patch for a whole session when + // the sidecar write failed at startup while registration + // succeeded — `ps` would keep advertising the pre-/clear session + // id and the pre-/cd directory until exit. The inverse + // divergence is safe on its own: `patchSessionRecord` no-ops on + // a missing record. // // `name` is deliberately not patched: it is the handle a user // just read out of `qwen sessions ps`, and re-deriving it here // would rename a live session on every /clear for no gain — the // directory it names has not changed. - await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); - }); + this.queueRuntimeStatusWrite(async () => { + await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); + }); + } } return this.sessionId; @@ -3933,6 +3943,17 @@ export class Config { this.runtimeStatusEnabled = true; } + /** + * Marks this Config as registered in the session registry. Call once + * after `registerSession` returned true (from the interactive UI + * bootstrap). Gates the mid-session registry patches + * (startNewSession, refreshCurrentRuntimeStatus), so they ride the + * registry's own success rather than the sidecar's. + */ + markSessionRegistered(): void { + this.sessionRegistered = true; + } + private queueRuntimeStatusWrite(write: () => Promise): void { this.runtimeStatusWrite = this.runtimeStatusWrite .catch(() => { @@ -3951,28 +3972,32 @@ export class Config { } private async refreshCurrentRuntimeStatus(workDir: string): Promise { - if (!this.runtimeStatusEnabled) { + if (!this.runtimeStatusEnabled && !this.sessionRegistered) { return; } this.queueRuntimeStatusWrite(async () => { - await writeRuntimeStatus( - this.storage.getRuntimeStatusPath(this.sessionId), - { - sessionId: this.sessionId, - workDir, - qwenVersion: this.cliVersion ?? null, - }, - ); - // The registry's DIRECTORY column is how a user tells two live - // sessions apart, so a mid-session directory switch has to reach it - // too — otherwise `qwen sessions ps` keeps advertising the folder - // this session left. Unlike the /clear path, `name` follows: it is - // derived from the directory's basename, which is exactly what - // changed here. - await patchSessionRecord({ - cwd: workDir, - name: deriveSessionName(workDir, this.sessionId), - }); + if (this.runtimeStatusEnabled) { + await writeRuntimeStatus( + this.storage.getRuntimeStatusPath(this.sessionId), + { + sessionId: this.sessionId, + workDir, + qwenVersion: this.cliVersion ?? null, + }, + ); + } + if (this.sessionRegistered) { + // The registry's DIRECTORY column is how a user tells two live + // sessions apart, so a mid-session directory switch has to reach + // it too — otherwise `qwen sessions ps` keeps advertising the + // folder this session left. Unlike the /clear path, `name` + // follows: it is derived from the directory's basename, which is + // exactly what changed here. + await patchSessionRecord({ + cwd: workDir, + name: deriveSessionName(workDir, this.sessionId), + }); + } }); await this.flushRuntimeStatusWrites(); } diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 35c38e0c4c6..26680c9e708 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 { readPidNamespaceId } from '../utils/process-liveness.js'; +import { + readLocalBootId, + readPidNamespaceId, +} from '../utils/process-liveness.js'; /** * Records the paths `readRecord` stats, while the real filesystem does the @@ -172,6 +175,7 @@ describe('deriveSessionName', () => { describe('registerSession', () => { it('writes a record for this process and lists it back', async () => { + const before = Date.now(); expect( await registerSession({ sessionId: 's1', @@ -179,6 +183,7 @@ describe('registerSession', () => { qwenVersion: '1.2.3', }), ).toBe(true); + const after = Date.now(); const live = await listLiveSessions(); expect(live).toHaveLength(1); @@ -190,6 +195,11 @@ describe('registerSession', () => { qwenVersion: '1.2.3', }); expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); + // Bounds pin the epoch: a seconds-vs-milliseconds refactor (or a + // constant) ships green through every other assertion here, then + // breaks the AGE column and the newest-first ordering for everyone. + expect(live[0].startedAt).toBeGreaterThanOrEqual(before); + expect(live[0].startedAt).toBeLessThanOrEqual(after); }); it('records the writer’s PID namespace identity', async () => { @@ -287,6 +297,46 @@ describe('registerSession', () => { }, ); + it('refuses to overwrite a record held by another PID namespace', async () => { + // Host + devcontainer (or sibling containers) sharing one home can + // collide on a PID number; the loser of an overwrite would lose its + // record when the winner exits. First writer wins instead, and the + // second session stays undiscoverable — degraded but safe. + const foreign = liveBody({ pidNs: 1, sessionId: 'theirs' }); + await writeRaw(`${process.pid}.json`, foreign); + + expect(await registerSession({ sessionId: 'mine', cwd: '/w/app' })).toBe( + false, + ); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }); + + it.runIf(process.platform === 'linux')( + 'refuses to overwrite a record held by another machine’s boot', + async () => { + // The initial PID namespace inode is a kernel constant identical on + // every Linux machine, so machines sharing a home over NFS pass + // the namespace comparison — only the boot prefix separates them. + expect(readLocalBootId()).not.toBeNull(); + const foreign = liveBody({ + procStart: 'not-this-boot:1', + sessionId: 'theirs', + }); + await writeRaw(`${process.pid}.json`, foreign); + + expect(await registerSession({ sessionId: 'mine', cwd: '/w/app' })).toBe( + false, + ); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }, + ); + it('reports failure instead of throwing when the home dir is unwritable', async () => { __setMockGlobalDir(path.join(tmpDir, 'nope', '\0invalid')); expect( @@ -324,6 +374,7 @@ describe('patchSessionRecord', () => { cwd: '/w/app', qwenVersion: '1.2.3', }); + const [before] = await listLiveSessions(); await patchSessionRecord({ sessionId: 'new', name: 'renamed' }); @@ -334,6 +385,10 @@ describe('patchSessionRecord', () => { cwd: '/w/app', qwenVersion: '1.2.3', }); + // Both production patch sites omit `startedAt`; a re-stamp would + // reset the AGE column and the newest-first ordering on every + // /clear and /cd. + expect(record.startedAt).toBe(before.startedAt); }); it('does not create a record for a session that never registered', async () => { @@ -365,13 +420,17 @@ describe('patchSessionRecord', () => { 'refuses to merge into a stale record left by a dead previous incarnation of this PID', async () => { // Session A died without unlinking (SIGKILL); PID P was recycled by - // session B whose registration failed. B's patch passes the pid - // comparison alone — only the start token proves the record is not - // A's, and without it the merge would graft B's fields onto A's - // startedAt/version/name and list the chimera as live. + // session B whose registration failed. B's patch passes the pid, + // namespace and boot checks — same machine — and only the start + // token proves the record is not A's; without it the merge would + // graft B's fields onto A's startedAt/version/name and list the + // chimera as live. (A foreign boot prefix would be refused by the + // machine-identity check instead.) + const bootId = readLocalBootId(); + expect(bootId).not.toBeNull(); const filePath = await writeRaw( `${process.pid}.json`, - liveBody({ procStart: 'not-this-boot:1', sessionId: 'incarnation-a' }), + liveBody({ procStart: `${bootId}:1`, sessionId: 'incarnation-a' }), ); await patchSessionRecord({ sessionId: 'incarnation-b' }); @@ -382,6 +441,30 @@ describe('patchSessionRecord', () => { }, ); + it('preserves the identity fields across the patch merge', async () => { + // Both production patch sites run in ordinary use; a merge that + // drops `procStart` degrades every later liveness check to a bare + // kill(pid, 0) — a recycled PID resurrects the record — and a + // dropped `pidNs` hides the session behind the namespace guard. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const before = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + if (process.platform === 'linux') { + // Otherwise the procStart equality pin below is vacuous. + expect(before['procStart']).not.toBeNull(); + } + + await patchSessionRecord({ sessionId: 'new', cwd: '/w/b' }); + + const after = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(after['procStart']).toBe(before['procStart']); + expect(after['pidNs']).toBe(before['pidNs']); + expect(after['pid']).toBe(process.pid); + }); + it('still patches a record written without a start token', async () => { // Tokenless platforms must keep working through the pid comparison — // the guard only fires when BOTH sides have a token to compare. @@ -401,6 +484,35 @@ describe('patchSessionRecord', () => { const stat = await fs.stat(getSessionRecordPath()); expect(stat.mode & 0o777).toBe(0o600); }); + + itPosixPatch( + 'replaces a symlinked record instead of patching through it', + async () => { + // Mirror of the registration symlink test: the patch path is + // written on every /clear and /cd, so a dropped `noFollow` would + // redirect those writes through a pre-planted link just the same. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const outside = path.join(tmpDir, 'outside.json'); + await fs.writeFile( + outside, + JSON.stringify(liveBody({ sessionId: 'outside' })), + ); + await fs.rm(getSessionRecordPath()); + await fs.symlink(outside, getSessionRecordPath()); + + await patchSessionRecord({ sessionId: 'patched' }); + + const outsideRecord = JSON.parse( + await fs.readFile(outside, 'utf8'), + ) as Record; + expect(outsideRecord['sessionId']).toBe('outside'); + expect((await fs.lstat(getSessionRecordPath())).isSymbolicLink()).toBe( + false, + ); + const [record] = await listLiveSessions(); + expect(record?.sessionId).toBe('patched'); + }, + ); }); describe('unregisterSession', () => { @@ -413,6 +525,41 @@ describe('unregisterSession', () => { expect(await listLiveSessions()).toEqual([]); }); + it('removes only this process’s record, leaving siblings intact', async () => { + // Plant a live sibling: a broadened deletion ("also clean up stale + // records on exit") would wipe it too, and registration is one-shot + // — the victim would stay invisible in `ps` for its whole lifetime. + await writeRaw( + `${process.ppid}.json`, + liveBody({ + pid: process.ppid, + sessionId: 's-sibling', + startedAt: Date.now(), + }), + ); + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + + await unregisterSession(); + + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + const live = await listLiveSessions(); + expect(live.map((r) => r.sessionId)).toEqual(['s-sibling']); + }); + + it('leaves a foreign-identity record at its path alone', async () => { + // The path is keyed by PID alone: the record sitting there may + // belong to a live session in another namespace that shares the + // number. Unlinking it would hide that session until it restarts. + const foreign = liveBody({ pidNs: 1, sessionId: 'theirs' }); + await writeRaw(`${process.pid}.json`, foreign); + + await unregisterSession(); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }); + it('is a no-op when nothing was registered', async () => { await expect(unregisterSession()).resolves.toBeUndefined(); }); @@ -445,11 +592,15 @@ describe('listLiveSessions', () => { '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. + // different process on THIS boot — so the record describes a + // session that is gone. (A foreign boot prefix would model another + // machine, which is skipped rather than swept.) + const bootId = readLocalBootId(); + expect(bootId).not.toBeNull(); const filePath = await writeRaw( `${process.pid}.json`, liveBody({ - procStart: 'not-this-boot:1', + procStart: `${bootId}:1`, sessionId: 's-recycled', cwd: '/w/app', name: 'app-aa', @@ -490,6 +641,42 @@ describe('listLiveSessions', () => { await expect(fs.stat(filePath)).resolves.toBeDefined(); }); + it.runIf(process.platform === 'linux')( + 'neither lists nor sweeps a record from another machine’s boot', + async () => { + // Same initial-namespace inode on every Linux machine, so the + // namespace guard cannot separate two machines sharing one home — + // the boot prefix must. The recorded PID is dead on this side, so + // without the guard the sweep unlinks a live session's record on + // the other machine. + expect(readLocalBootId()).not.toBeNull(); + const filePath = await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, procStart: 'not-this-boot:1' }), + ); + + expect(await listLiveSessions()).toEqual([]); + await expect(fs.stat(filePath)).resolves.toBeDefined(); + }, + ); + + it('sweeps registration temp files orphaned by a crashed write', async () => { + // A writer that dies between the temp write and the rename leaves + // the temp behind; nothing else ever removes it. + const orphan = await writeRaw(`${process.pid}.json.0123456789ab.tmp`, '{}'); + const stale = new Date(Date.now() - 6 * 60 * 1000); + await fs.utimes(orphan, stale, stale); + + // A fresh temp may belong to a writer mid-rename — the age check + // must spare it. + const fresh = await writeRaw(`${process.pid}.json.fedcba987654.tmp`, '{}'); + + await listLiveSessions(); + + await expect(fs.stat(orphan)).rejects.toThrow(); + await expect(fs.stat(fresh)).resolves.toBeDefined(); + }); + it('ignores files that are not .json', async () => { await writeRaw('2026-planning-notes.json', { hello: 'world' }); await writeRaw('notes.txt', 'nope'); diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index e7da7261633..e9d77bf1d53 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -5,13 +5,19 @@ */ /** - * A machine-wide index of the Qwen Code sessions that are running right - * now. + * An 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. + * Each top-level session writes `/sessions/.json` at + * startup and unlinks it on exit. The directory is flat and keyed by PID + * so that "who else is running" is one `readdir` plus a handful of small + * reads. + * + * The index is scoped to one Qwen home: the global dir resolves + * `QWEN_HOME` on every call, so a session started under a redirected + * `QWEN_HOME` registers elsewhere and stays invisible to readers under + * the default home (and vice versa). It also assumes a single machine on + * platforms without a start token: there, no identity separates two + * machines sharing one home, so the registry does not support that. * * ## Why this is not `runtime.json` * @@ -33,13 +39,19 @@ * ## Staleness * * A record is live when it was written from the reader's own PID - * namespace and 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 the liveness check are - * swept during enumeration; records from another PID namespace are - * neither listed nor swept, because PID numbers do not resolve across - * that boundary in either direction. Anything else we cannot positively - * prove dead is left alone. + * namespace and boot, 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 the liveness check are + * swept during enumeration; records from another PID namespace — or from + * another machine's boot, which the namespace inode alone does not + * separate, because the initial namespace inode is a kernel constant + * identical on every Linux machine — are neither listed nor swept, + * because PID numbers do not resolve across those boundaries in either + * direction. The same isolation extends to records whose writer's + * namespace no longer exists (an ephemeral container destroyed without + * cleanup): no future reader shares that namespace, so the record is + * never reclaimed and stays until removed by hand. Anything else we + * cannot positively prove dead is left alone. */ import { createHash } from 'node:crypto'; @@ -50,6 +62,7 @@ import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { isSameProcess, + readLocalBootId, readPidNamespaceId, readProcStartToken, } from '../utils/process-liveness.js'; @@ -80,6 +93,16 @@ const MAX_RECORD_BYTES = 64 * 1024; */ const RECORD_FILENAME = /^\d+\.json$/; +/** + * Temp files left by `atomicWriteJSON` next to their target. Matched + * separately from `RECORD_FILENAME` so enumeration can reap the orphans + * a crashed registration leaves behind (see `sweepOrphanedTempFile`). + */ +const TEMP_FILENAME = /^\d+\.json\.[0-9a-f]{12}\.tmp$/; + +/** Younger temps may belong to a writer mid-rename — leave them alone. */ +const TEMP_MAX_AGE_MS = 5 * 60 * 1000; + /** One live session, as recorded on disk. */ export interface SessionRegistryRecord { schemaVersion: number; @@ -110,7 +133,12 @@ export function getSessionRegistryDir(): string { return path.join(Storage.getGlobalQwenDir(), 'sessions'); } -/** This process's record path. Records are keyed by PID. */ +/** + * This process's record path. Records are keyed by PID, which collides + * across PID namespaces and machines; the identity fields inside a + * record (`pidNs`, the token's boot prefix) decide which side owns a + * colliding path. + */ export function getSessionRecordPath(): string { return path.join(getSessionRegistryDir(), `${process.pid}.json`); } @@ -147,7 +175,12 @@ export function deriveSessionName(cwd: string, sessionId: string): string { * 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 + * writing — when the path is already held by a record carrying another + * namespace's or another machine's identity: a colliding session on the + * other side is live, and overwriting its record would hide it from + * discovery and destroy it when we exit. This session then simply stays + * undiscoverable. */ export async function registerSession( fields: RegisterSessionFields, @@ -166,12 +199,25 @@ export async function registerSession( try { const dir = getSessionRegistryDir(); + const filePath = getSessionRecordPath(); + // A record already at our path may belong to a live session that + // collided with us on the PID number across a namespace or machine + // boundary (host + devcontainer, sibling containers, NFS-shared + // homes). Overwriting it would hide that session from discovery and + // destroy its record when we exit — refuse instead. + const existing = await readRecord(filePath); + if (existing !== null && !matchesLocalIdentity(existing)) { + debugLogger.debug( + 'registerSession: record path held by a foreign-identity record', + ); + return false; + } 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(), record, { + await atomicWriteJSON(filePath, record, { mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true, @@ -207,12 +253,14 @@ export async function patchSessionRecord( // and can throw, and this function promises never to reject. const filePath = getSessionRecordPath(); const existing = await readRecord(filePath); - // Missing, or not actually a record for this PID: `readRecord` does - // not check the filename/contents agreement that `listLiveSessions` - // insists on, so merging into a foreign `.json` would write back - // a record the reader will neither show nor sweep — permanent litter. - if (existing === null || existing.pid !== process.pid) return; - // The pid comparison alone also passes for a stale record left by a + // Missing, or not actually a record for this PID, namespace and + // boot: the path is keyed by PID alone, and `readRecord` does not + // check the filename/contents agreement that `listLiveSessions` + // insists on, so merging into a foreign record would write back + // something the reader will neither show nor sweep — permanent + // litter. + if (existing === null || !matchesLocalIdentity(existing)) return; + // The identity check alone also passes for a stale record left by a // DEAD previous incarnation of this PID (session A dies without // unlinking; the PID is recycled by session B whose registration // failed). When both sides carry a start token, require it to agree @@ -239,7 +287,15 @@ export async function patchSessionRecord( /** Remove this process's record. Safe to call when none was written. */ export async function unregisterSession(): Promise { try { - await fs.unlink(getSessionRecordPath()); + const filePath = getSessionRecordPath(); + // The path is keyed by PID alone, and PIDs collide across namespace + // and machine boundaries: the record sitting at our path may belong + // to a live session on the other side of a shared home. Unlink only + // what this side wrote; an unreadable one (a write torn by a crash) + // cannot belong to anyone else and goes too. + const existing = await readRecord(filePath); + if (existing !== null && !matchesLocalIdentity(existing)) return; + await fs.unlink(filePath); } catch (error) { if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; debugLogger.debug(`unregisterSession failed: ${describe(error)}`); @@ -271,43 +327,109 @@ export async function listLiveSessions(): Promise { } const ownNamespace = readPidNamespaceId(); + const ownBootId = readLocalBootId(); 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; - - // A record from another PID namespace describes PIDs that do not - // resolve in ours: kill(pid, 0) reports ESRCH for a process that - // is alive over there, and a "matching" starttime can belong to - // an unrelated process. Neither listing nor sweeping is safe — - // leave it to a reader on the writer's own side. - if (record.pidNs !== ownNamespace) return; - - if (isSameProcess(record.pid, record.procStart)) { - live.push(record); - return; - } - - try { - await fs.unlink(filePath); - } catch { - // Raced with another session's sweep, or not ours to delete. - } - }), + entries.map(async (name) => { + const filePath = path.join(dir, name); + if (!RECORD_FILENAME.test(name)) { + await sweepOrphanedTempFile(filePath, name); + return; + } + + 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; + + // A record from another PID namespace describes PIDs that do not + // resolve in ours: kill(pid, 0) reports ESRCH for a process that + // is alive over there, and a "matching" starttime can belong to + // an unrelated process. Neither listing nor sweeping is safe — + // leave it to a reader on the writer's own side. + if (record.pidNs !== ownNamespace) return; + + // The namespace inode does not separate machines either: the + // initial-namespace inode is a kernel constant identical on every + // Linux machine, and a shared home (NFS/AFS) lets two machines + // write one directory. A token whose boot prefix names another + // boot names another machine — its PIDs resolve to the wrong + // processes here, so leave it to a reader on its own side, and + // never sweep it. + const recordBootId = + record.procStart === null ? null : bootIdOf(record.procStart); + if ( + ownBootId !== null && + recordBootId !== null && + recordBootId !== ownBootId + ) { + return; + } + + if (isSameProcess(record.pid, record.procStart)) { + live.push(record); + return; + } + + 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); } +/** + * True when a record found at this process's path was written from this + * PID namespace and this boot. The path is keyed by PID alone, and PID + * numbers collide across both boundaries; these are the fields a + * colliding writer on another side differs on. A missing token or an + * unreadable local boot id degrades to the namespace comparison alone, + * matching `isSameProcess`'s conservatism. + */ +function matchesLocalIdentity(record: SessionRegistryRecord): boolean { + if (record.pid !== process.pid) return false; + if (record.pidNs !== readPidNamespaceId()) return false; + if (record.procStart === null) return true; + const ownBootId = readLocalBootId(); + if (ownBootId === null) return true; + const recordBootId = bootIdOf(record.procStart); + return recordBootId === null || recordBootId === ownBootId; +} + +/** The boot-id prefix of a `:` token, or null. */ +function bootIdOf(procStart: string): string | null { + const sep = procStart.indexOf(':'); + return sep === -1 ? null : procStart.slice(0, sep); +} + +/** + * Reap a crashed registration's temp file. `atomicWriteJSON` writes + * `.json.<12hex>.tmp` and renames; a writer that dies in between + * (kill -9, OOM, power loss) leaves the temp behind, and nothing else + * ever removes it. The age check spares a writer mid-rename. + */ +async function sweepOrphanedTempFile( + filePath: string, + name: string, +): Promise { + if (!TEMP_FILENAME.test(name)) return; + try { + const stat = await fs.stat(filePath); + if (!stat.isFile()) return; + if (Date.now() - stat.mtimeMs < TEMP_MAX_AGE_MS) return; + await fs.unlink(filePath); + } catch { + // Raced with the writer's rename or another sweeper. + } +} + /** Read and validate one record. Returns null for anything unusable. */ async function readRecord( filePath: string, diff --git a/packages/core/src/utils/process-liveness.test.ts b/packages/core/src/utils/process-liveness.test.ts index ee59f7d05d2..30dae8eebae 100644 --- a/packages/core/src/utils/process-liveness.test.ts +++ b/packages/core/src/utils/process-liveness.test.ts @@ -126,6 +126,21 @@ describe('isPidAlive', () => { expect(isPidAlive(4242)).toBe(false); }); + it('treats another user’s zombie as dead despite EPERM', async () => { + // The kernel permission-checks signal 0 regardless of the target's + // state, so a cross-user zombie reaches the EPERM catch; without the + // zombie check there it stays listed until its parent reaps it. + const { mod } = await withFakeProc({ + '/proc/4242/stat': statLine('qwen', '987654', 'Z'), + }); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { + code: 'EPERM', + }); + }); + expect(mod.isPidAlive(4242)).toBe(false); + }); + // On Windows the "process exists but is owned by another user" errno is // EACCES, not EPERM; missing it there would let a sweep delete a live // session's record. @@ -140,14 +155,27 @@ describe('isPidAlive', () => { // A zombie still answers kill(pid, 0): the PID exists until the parent // reaps it. Only the state field of /proc//stat proves it has // already exited, so without the Z check the record stays listed for - // the parent's whole lifetime. + // the parent's whole lifetime. The comm carries a ')' so the + // lastIndexOf(')') anchoring is part of what this test pins. const { mod } = await withFakeProc({ - '/proc/4242/stat': statLine('qwen', '987654', 'Z'), + '/proc/4242/stat': statLine('we ) ird', '987654', 'Z'), }); vi.spyOn(process, 'kill').mockImplementation(() => true); expect(mod.isPidAlive(4242)).toBe(false); }); + it('does not read a zombie state out of a comm containing ")"', async () => { + // Anchoring on the FIRST ')' parses the rest of the comm as the + // state field, and a comm tail starting with 'Z' then marks a live + // process as a zombie — the sweep would delete a live session's + // record. + const { mod } = await withFakeProc({ + '/proc/4242/stat': statLine('a)Zx', '987654'), + }); + vi.spyOn(process, 'kill').mockImplementation(() => true); + expect(mod.isPidAlive(4242)).toBe(true); + }); + it('keeps a live process whose /proc state cannot be read', async () => { const { mod } = await withFakeProc({}); vi.spyOn(process, 'kill').mockImplementation(() => true); @@ -318,4 +346,19 @@ describe('isSameProcess', () => { true, ); }); + + it('rejects a zombie even when its start token still matches', async () => { + // A zombie's /proc//stat persists with its original starttime + // until the parent reaps it, so the recorded token still agrees — + // only the liveness check (with its Z exclusion) can catch it. This + // pins the liveness-before-token ordering: a regression that + // compares tokens first (or folds the two /proc reads into one and + // drops the Z exclusion) keeps the zombie listed. + const { mod } = await withFakeProc({ + [BOOT_ID_PATH]: `${FAKE_BOOT_ID}\n`, + '/proc/4242/stat': statLine('qwen', '987654', 'Z'), + }); + vi.spyOn(process, 'kill').mockImplementation(() => true); + expect(mod.isSameProcess(4242, `${FAKE_BOOT_ID}:987654`)).toBe(false); + }); }); diff --git a/packages/core/src/utils/process-liveness.ts b/packages/core/src/utils/process-liveness.ts index 61d72740c6e..8d97dd99e5f 100644 --- a/packages/core/src/utils/process-liveness.ts +++ b/packages/core/src/utils/process-liveness.ts @@ -25,7 +25,9 @@ import { isNodeError } from './errors.js'; * `EPERM` (and Windows' `EACCES`) 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. + * directory. The zombie exclusion still applies on that path: the kernel + * permission-checks signal 0 regardless of the target's state, so a + * cross-user zombie reaches the catch too. */ export function isPidAlive(pid: number): boolean { if (!Number.isInteger(pid) || pid <= 0) return false; @@ -38,7 +40,14 @@ export function isPidAlive(pid: number): boolean { // listed for its entire lifetime. return !isZombie(pid); } catch (err) { - return isNodeError(err) && (err.code === 'EPERM' || err.code === 'EACCES'); + // `/proc//stat` is world-readable by default, so the zombie + // check works here as well; where it is not readable (hidepid), + // `isZombie` degrades to false and behavior is unchanged. + return ( + isNodeError(err) && + (err.code === 'EPERM' || err.code === 'EACCES') && + !isZombie(pid) + ); } } @@ -99,7 +108,7 @@ export function readProcStartToken(pid: number): string | null { if (process.platform !== 'linux') return null; if (!Number.isInteger(pid) || pid <= 0) return null; - const bootId = readBootId(); + const bootId = readLocalBootId(); if (bootId === null) return null; let raw: string; @@ -135,10 +144,15 @@ export function readProcStartToken(pid: number): string | null { * first-read moments (startup registration, first concurrent sweep) are * fd-pressure moments, and pinning the cache to a transient EMFILE would * silently disable PID-reuse protection for the whole process lifetime. + * + * Exported for the session registry: two machines sharing one home can + * collide on PID number and even on PID-namespace inode (the initial + * namespace inode is a kernel constant), so a record's boot-id prefix + * is the only identity that proves which machine wrote it. */ let cachedBootId: string | undefined; -function readBootId(): string | null { +export function readLocalBootId(): string | null { if (cachedBootId !== undefined) return cachedBootId; try { const value = fs diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index 937663141d5..37eef4119b9 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -185,6 +185,9 @@ describe('Config.startNewSession session-registry patch', () => { cwd: tmpDir, qwenVersion: '0.0.0-test', }); + config.markSessionRegistered(); + + const [before] = await listLiveSessions(); config.startNewSession(sessionB); @@ -198,6 +201,35 @@ describe('Config.startNewSession session-registry patch', () => { expect(after).not.toBeNull(); expect(after!.pid).toBe(process.pid); expect(after!.cwd).toBe(tmpDir); + // `name` is deliberately not patched on /clear: deriveSessionName + // hashes the session id into the suffix, so re-deriving it would + // rename the session a user just read out of `ps`. + expect(after!.name).toBe(before!.name); + }); + + it('patches the registry even when the sidecar write failed at startup', async () => { + // The two failure domains are independent: the sidecar lives in the + // project's chats/ dir, the registry in the global dir. When the + // sidecar write fails but registration succeeds, the patches must + // keep going — otherwise `ps` shows stale values until exit. + const sessionA = 'aaaaaaaa-1111-2222-3333-aaaaaaaaaaaa'; + const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; + const config = makeConfig(sessionA); + // No markRuntimeStatusEnabled(): models the failed sidecar write. + await registerSession({ + sessionId: sessionA, + cwd: tmpDir, + qwenVersion: '0.0.0-test', + }); + config.markSessionRegistered(); + + config.startNewSession(sessionB); + + const after = await waitFor(async () => { + const [record] = await listLiveSessions(); + return record?.sessionId === sessionB ? record : null; + }); + expect(after).not.toBeNull(); }); }); From f1bb3a3b2d7896f914019a115a34d13b773d3289 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 20:27:40 +0000 Subject: [PATCH 4/8] fix(core): harden session registry identity guards under boot-id and schema outages (#8969) Boot-id unreadability now degrades the write paths (register/patch/ unregister) to accepting only tokenless records and never disables the reader-side cross-machine guard, so a foreign machine's live record can no longer be overwritten, merged into, unlinked or swept during an outage. readRecord discriminates newer-schema records from torn files and the write paths refuse them like foreign-identity records instead of treating them as unowned. Registration on Linux retries the start token once and refuses rather than writing an impersonable tokenless record. The /cd refresh queues the sidecar write and the registry patch as separate entries so a sidecar failure cannot skip the patch, and patch/ unregister reuse the record path captured at registration so a relative QWEN_HOME resolving against a moved cwd keeps working. deriveSessionName NFC-normalizes, keeps combining marks, and truncates by code point. Co-authored-by: Qwen-Coder --- .../cli/src/ui/startInteractiveUI.test.tsx | 34 ++- packages/core/src/config/config.test.ts | 83 ++++++ packages/core/src/config/config.ts | 38 +-- .../src/services/session-registry.test.ts | 255 +++++++++++++++++- .../core/src/services/session-registry.ts | 229 +++++++++++----- .../src/utils/runtimeStatus.config.test.ts | 27 +- 6 files changed, 559 insertions(+), 107 deletions(-) diff --git a/packages/cli/src/ui/startInteractiveUI.test.tsx b/packages/cli/src/ui/startInteractiveUI.test.tsx index 26a05394ac1..4b1bb9ac5ec 100644 --- a/packages/cli/src/ui/startInteractiveUI.test.tsx +++ b/packages/cli/src/ui/startInteractiveUI.test.tsx @@ -52,14 +52,22 @@ vi.mock('../utils/earlyInputCapture.js', () => ({ const { startInteractiveUI } = await import('./startInteractiveUI.js'); -function makeConfig(): Config { +function makeConfig(): Config & { + markSessionRegistered: ReturnType; +} { return { getSessionId: () => 'session-123', getTargetDir: () => '/work/app', getScreenReader: () => false, getChatRecordingService: () => undefined, isTelemetryInitializationDeferred: () => false, - } as unknown as Config; + // Must exist: the production success path calls it, and a missing + // member would throw a TypeError that the catch under test swallows — + // leaving every assertion green while registration silently no-ops. + markSessionRegistered: vi.fn(), + } as unknown as Config & { + markSessionRegistered: ReturnType; + }; } const settings = { @@ -73,9 +81,9 @@ const initializationResult = { geminiMdFileCount: 0, } as InitializationResult; -async function start(): Promise { +async function start(config: Config = makeConfig()): Promise { await startInteractiveUI( - makeConfig(), + config, settings, [], '/work/app', @@ -90,14 +98,18 @@ describe('startInteractiveUI session registration', () => { it('registers the session with its id, target dir, and CLI version', async () => { registerSession.mockResolvedValue(true); + const config = makeConfig(); - await start(); + await start(config); expect(registerSession).toHaveBeenCalledWith({ sessionId: 'session-123', cwd: '/work/app', qwenVersion: '9.9.9', }); + // Arms the mid-session /clear and /cd registry patches on this + // Config; if this never fires, `ps` shows stale values until exit. + expect(config.markSessionRegistered).toHaveBeenCalledTimes(1); }); it('arms the unregister cleanup only when registration succeeded', async () => { @@ -105,9 +117,11 @@ describe('startInteractiveUI session registration', () => { // teardown) unconditionally, so the count differs by exactly one // depending on whether the unregister callback was armed. registerSession.mockResolvedValue(true); - await start(); + const successConfig = makeConfig(); + await start(successConfig); expect(registerCleanup).toHaveBeenCalledTimes(2); expect(unregisterSession).not.toHaveBeenCalled(); + expect(successConfig.markSessionRegistered).toHaveBeenCalledTimes(1); // Armed is not the contract — invoke the armed callback: a future // edit emptying its body would pass the call-count pins while every // session's record survives exit and `ps` lists it as running. @@ -118,16 +132,20 @@ describe('startInteractiveUI session registration', () => { vi.clearAllMocks(); registerSession.mockResolvedValue(false); - await start(); + const refusedConfig = makeConfig(); + await start(refusedConfig); expect(registerCleanup).toHaveBeenCalledTimes(1); + expect(refusedConfig.markSessionRegistered).not.toHaveBeenCalled(); }); it('swallows a registration rejection without aborting startup', async () => { // A read-only home must not keep the TUI from launching; the // registration is wrapped precisely so its failure cannot propagate. registerSession.mockRejectedValue(new Error('read-only home')); + const config = makeConfig(); - await expect(start()).resolves.toBeUndefined(); + await expect(start(config)).resolves.toBeUndefined(); expect(registerCleanup).toHaveBeenCalledTimes(1); + expect(config.markSessionRegistered).not.toHaveBeenCalled(); }); }); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 2d44040fdf3..738f2150f99 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -6310,6 +6310,89 @@ describe('Server Config (config.ts)', () => { cwdSpy.mockRestore(); }); + it('relocateWorkingDirectory should patch the registry even when the sidecar write failed at startup', async () => { + // Mirror of the startNewSession divergence pin: registration writes + // to the global dir, the sidecar to the project's chats/ dir — + // independent failure domains, so the registered-but-sidecar-off + // state is reachable and the /cd patch must survive it. + const config = new Config(baseParams); + // No markRuntimeStatusEnabled(): models the failed sidecar write. + config.markSessionRegistered(); + const sessionId = config.getSessionId(); + const newDir = path.resolve('/path/to/other'); + const chdirSpy = vi.spyOn(process, 'chdir').mockImplementation(() => { + // Keep the test process in its original directory. + }); + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(newDir); + const writeRuntimeStatusSpy = vi + .spyOn(runtimeStatus, 'writeRuntimeStatus') + .mockResolvedValue('unused'); + vi.mocked(fs.existsSync).mockImplementation( + (pathToCheck) => pathToCheck.toString() === newDir, + ); + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockResolvedValue(undefined); + + await config.relocateWorkingDirectory(newDir); + + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + cwd: newDir, + name: sessionRegistry.deriveSessionName(newDir, sessionId), + }); + expect(writeRuntimeStatusSpy).not.toHaveBeenCalled(); + + writeRuntimeStatusSpy.mockRestore(); + patchSessionRecordSpy.mockRestore(); + chdirSpy.mockRestore(); + cwdSpy.mockRestore(); + }); + + it('relocateWorkingDirectory should refresh the sidecar even when registration failed', async () => { + // The opposite divergence: the sidecar write succeeded at startup + // but registerSession returned false (foreign-identity refusal, + // unwritable global dir), so only the sidecar gate is armed. A gate + // regressed to `if (!this.sessionRegistered) return;` would silently + // stop refreshing runtime.json on /cd for these sessions. + const config = new Config(baseParams); + config.markRuntimeStatusEnabled(); + // No markSessionRegistered(): models the failed registration. + const sessionId = config.getSessionId(); + const newDir = path.resolve('/path/to/other'); + const newStorage = new Storage(newDir); + const oldStorage = new Storage(config.getTargetDir()); + const oldRuntimeStatusPath = oldStorage.getRuntimeStatusPath(sessionId); + const newRuntimeStatusPath = newStorage.getRuntimeStatusPath(sessionId); + const chdirSpy = vi.spyOn(process, 'chdir').mockImplementation(() => { + // Keep the test process in its original directory. + }); + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(newDir); + const writeRuntimeStatusSpy = vi + .spyOn(runtimeStatus, 'writeRuntimeStatus') + .mockResolvedValue(newRuntimeStatusPath); + vi.mocked(fs.existsSync).mockImplementation((pathToCheck) => { + const checked = pathToCheck.toString(); + return checked === oldRuntimeStatusPath || checked === newDir; + }); + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockResolvedValue(undefined); + + await config.relocateWorkingDirectory(newDir); + + expect(writeRuntimeStatusSpy).toHaveBeenCalledWith(newRuntimeStatusPath, { + sessionId, + workDir: newDir, + qwenVersion: null, + }); + expect(patchSessionRecordSpy).not.toHaveBeenCalled(); + + writeRuntimeStatusSpy.mockRestore(); + patchSessionRecordSpy.mockRestore(); + chdirSpy.mockRestore(); + cwdSpy.mockRestore(); + }); + it('relocateWorkingDirectory should reject and roll back when session artifact migration fails', async () => { const config = new Config({ ...baseParams, chatRecording: true }); const disposeResidentAgents = vi.spyOn( diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index e62c2de5ead..f9be30d1ea2 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -4007,21 +4007,25 @@ export class Config { } private async refreshCurrentRuntimeStatus(workDir: string): Promise { - if (!this.runtimeStatusEnabled && !this.sessionRegistered) { - return; + const sessionId = this.sessionId; + // Two separate queue entries, mirroring startNewSession: + // `writeRuntimeStatus` propagates exceptions, so sharing one closure + // would let a sidecar failure (read-only or full project filesystem) + // reject the entry before the registry patch runs — and the queue's + // trailing catch would swallow it, leaving `ps` advertising the + // folder this session left. The failure domains are independent. + if (this.runtimeStatusEnabled) { + const sidecarPath = this.storage.getRuntimeStatusPath(sessionId); + this.queueRuntimeStatusWrite(async () => { + await writeRuntimeStatus(sidecarPath, { + sessionId, + workDir, + qwenVersion: this.cliVersion ?? null, + }); + }); } - this.queueRuntimeStatusWrite(async () => { - if (this.runtimeStatusEnabled) { - await writeRuntimeStatus( - this.storage.getRuntimeStatusPath(this.sessionId), - { - sessionId: this.sessionId, - workDir, - qwenVersion: this.cliVersion ?? null, - }, - ); - } - if (this.sessionRegistered) { + if (this.sessionRegistered) { + this.queueRuntimeStatusWrite(async () => { // The registry's DIRECTORY column is how a user tells two live // sessions apart, so a mid-session directory switch has to reach // it too — otherwise `qwen sessions ps` keeps advertising the @@ -4030,10 +4034,10 @@ export class Config { // exactly what changed here. await patchSessionRecord({ cwd: workDir, - name: deriveSessionName(workDir, this.sessionId), + name: deriveSessionName(workDir, sessionId), }); - } - }); + }); + } await this.flushRuntimeStatusWrites(); } diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 26680c9e708..18438cc95f7 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -22,6 +22,9 @@ import { readLocalBootId, readPidNamespaceId, } from '../utils/process-liveness.js'; +// Namespace import: the boot-id/token outage scenarios below spy on the +// module's exports, which also intercepts the registry's internal calls. +import * as processLiveness from '../utils/process-liveness.js'; /** * Records the paths `readRecord` stats, while the real filesystem does the @@ -83,6 +86,7 @@ beforeEach(async () => { }); afterEach(async () => { + vi.restoreAllMocks(); await fs.rm(tmpDir, { recursive: true, force: true }); }); @@ -171,6 +175,41 @@ describe('deriveSessionName', () => { expect(name).toMatch(/^a{32}-[0-9a-f]{2}$/); expect(name).toHaveLength(35); }); + + it('keeps an accent spelled in NFD, the macOS default normalization', () => { + // NFD spells the accent as a separate combining mark; a class without + // \p{M} replaces it with a dash and the label loses the accent. + expect(deriveSessionName('/w/cafe\u0301', 's1')).toMatch( + /^caf\u00e9-[0-9a-f]{2}$/, + ); + }); + + it('treats canonically equivalent spellings as the same name', () => { + expect(deriveSessionName('/w/cafe\u0301', 's1')).toBe( + deriveSessionName('/w/caf\u00e9', 's1'), + ); + }); + + it('keeps combining marks instead of dashing through them', () => { + // Devanagari vowel signs are combining marks; without \p{M} in the + // class each one becomes a dash mid-word — the exact mangling the + // Unicode-aware class exists to prevent. + expect( + deriveSessionName( + '/w/\u092a\u0930\u093f\u092f\u094b\u091c\u0928\u093e', + 's1', + ), + ).toMatch(/^\u092a\u0930\u093f\u092f\u094b\u091c\u0928\u093e-[0-9a-f]{2}$/); + }); + + it('truncates by code point without splitting an astral character', () => { + // U+20000 is two UTF-16 units; a code-unit slice at the boundary + // would store a trailing lone surrogate in the record. + const name = deriveSessionName(`/w/${'a'.repeat(31)}\u{20000}`, 's1'); + expect(name.startsWith('a'.repeat(31) + '\u{20000}-')).toBe(true); + // 31 a's + astral char (2 UTF-16 units) + dash + 2-digit suffix. + expect(name).toHaveLength(36); + }); }); describe('registerSession', () => { @@ -337,6 +376,121 @@ describe('registerSession', () => { }, ); + it.runIf(process.platform === 'linux')( + 'overwrites a stale record left by a dead previous incarnation of this PID', + async () => { + // Same machine and PID number, but the recorded start token belongs + // to a process from this boot that is gone: registration must + // replace the record, or a session whose PID was recycled stays + // hidden from `ps` for its whole lifetime. + const bootId = readLocalBootId(); + expect(bootId).not.toBeNull(); + await writeRaw( + `${process.pid}.json`, + liveBody({ procStart: `${bootId}:1`, sessionId: 'stale' }), + ); + + expect(await registerSession({ sessionId: 'mine', cwd: '/w/app' })).toBe( + true, + ); + + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw['sessionId']).toBe('mine'); + expect(raw['procStart']).toMatch(/^[0-9a-f-]+:\d+$/i); + expect(raw['procStart']).not.toBe(`${bootId}:1`); + }, + ); + + it.runIf(process.platform === 'linux')( + 'refuses to write a record when the start token stays unreadable', + async () => { + // A tokenless Linux record is impersonable by any same-namespace + // reader — including another machine sharing the home, since the + // initial-namespace inode is identical everywhere. Staying + // undiscoverable beats writing a record another machine can + // destroy ours through. + vi.spyOn(processLiveness, 'readProcStartToken').mockReturnValue(null); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + false, + ); + + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + expect(await listLiveSessions()).toEqual([]); + }, + ); + + it.runIf(process.platform === 'linux')( + 'retries the start token once before refusing', + async () => { + // Boot-id read failures are not cached, so the retry recovers from + // a transient fd-pressure moment; only a persistent outage refuses. + vi.spyOn(processLiveness, 'readProcStartToken').mockReturnValueOnce(null); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + true, + ); + + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw['procStart']).toMatch(/^[0-9a-f-]+:\d+$/i); + }, + ); + + it('leaves a newer-schema record at its path alone on every write path', async () => { + // A record written by a newer build is readable but not safely + // parsable, and under a shared home it may belong to a live session + // on another machine across a schema bump. All three write paths + // must treat it the way they treat a parsed foreign-identity record: + // refuse the overwrite, skip the merge, return without unlinking. + // Register first so patch and unlink run against this test's path. + await registerSession({ sessionId: 's0', cwd: '/w/app' }); + const future = liveBody({ + schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION + 1, + sessionId: 'theirs', + }); + await writeRaw(`${process.pid}.json`, future); + + expect(await registerSession({ sessionId: 'mine', cwd: '/w/app' })).toBe( + false, + ); + await patchSessionRecord({ sessionId: 'mine' }); + await unregisterSession(); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(future); + }); + + it('keeps patching and unlinking the registered record after the home resolution moved', async () => { + // A relative QWEN_HOME resolves against the current cwd on every + // call, and /cd changes the cwd mid-session: patch and unregister + // must keep hitting the directory registration wrote to, or the /cd + // patch silently no-ops and exit leaks the record. + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + true, + ); + const originalPath = getSessionRecordPath(); + + __setMockGlobalDir(path.join(tmpDir, 'moved-home')); + + await patchSessionRecord({ sessionId: 'moved' }); + + // The patch reached the original record, not the moved resolution — + // where nothing was created at all. + const raw = JSON.parse(await fs.readFile(originalPath, 'utf8')); + expect(raw.sessionId).toBe('moved'); + expect(await listLiveSessions()).toEqual([]); + + // Unregister likewise unlinks the original record via the captured + // path, without consulting the moved resolution. + await unregisterSession(); + await expect(fs.stat(originalPath)).rejects.toThrow(); + }); + it('reports failure instead of throwing when the home dir is unwritable', async () => { __setMockGlobalDir(path.join(tmpDir, 'nope', '\0invalid')); expect( @@ -391,12 +545,14 @@ describe('patchSessionRecord', () => { expect(record.startedAt).toBe(before.startedAt); }); - it('does not create a record for a session that never registered', async () => { - // The registry directory exists, so the only thing standing between a - // patch and a half-populated record on disk is the missing-record - // guard. Asserting an empty listing is not enough: a record built from - // the patch alone fails validation and so lists as nothing either way. - await fs.mkdir(getSessionRegistryDir(), { recursive: true }); + it('does not recreate a record once the session’s record is gone', async () => { + // register + unregister leaves the directory but no record; the only + // thing standing between a patch and a half-populated record on disk + // is the missing-record guard. Asserting an empty listing is not + // enough: a record built from the patch alone fails validation and so + // lists as nothing either way. + await registerSession({ sessionId: 'old', cwd: '/w/app' }); + await unregisterSession(); await patchSessionRecord({ sessionId: 'new' }); @@ -408,6 +564,7 @@ describe('patchSessionRecord', () => { // `readRecord` does not check filename/contents agreement, so without // the pid guard a patch would merge into someone else's record and // write back something `listLiveSessions` will neither show nor sweep. + await registerSession({ sessionId: 's0', cwd: '/w/app' }); const foreign = liveBody({ pid: process.pid + 1, sessionId: 'theirs' }); const filePath = await writeRaw(`${process.pid}.json`, foreign); @@ -428,6 +585,7 @@ describe('patchSessionRecord', () => { // machine-identity check instead.) const bootId = readLocalBootId(); expect(bootId).not.toBeNull(); + await registerSession({ sessionId: 's0', cwd: '/w/app' }); const filePath = await writeRaw( `${process.pid}.json`, liveBody({ procStart: `${bootId}:1`, sessionId: 'incarnation-a' }), @@ -468,6 +626,7 @@ describe('patchSessionRecord', () => { it('still patches a record written without a start token', async () => { // Tokenless platforms must keep working through the pid comparison — // the guard only fires when BOTH sides have a token to compare. + await registerSession({ sessionId: 's0', cwd: '/w/app' }); await writeRaw(`${process.pid}.json`, liveBody()); await patchSessionRecord({ sessionId: 'new' }); @@ -476,6 +635,44 @@ describe('patchSessionRecord', () => { expect(record.sessionId).toBe('new'); }); + it.runIf(process.platform === 'linux')( + 'refuses to merge into a boot-prefixed record while the local boot id is unreadable', + async () => { + // "Cannot compare" must not become "is ours" on a write path: the + // record may belong to another machine sharing the home, and a + // merge would corrupt it. A writer with an unreadable boot id + // writes a TOKENLESS record, so refusing boot-prefixed ones here + // costs nothing. + vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); + await registerSession({ sessionId: 's0', cwd: '/w/app' }); + const foreign = liveBody({ + procStart: 'not-this-boot:1', + sessionId: 'theirs', + }); + const filePath = await writeRaw(`${process.pid}.json`, foreign); + + await patchSessionRecord({ sessionId: 'mine' }); + + expect(JSON.parse(await fs.readFile(filePath, 'utf8'))).toEqual(foreign); + }, + ); + + it.runIf(process.platform === 'linux')( + 'still patches a tokenless record while the local boot id is unreadable', + async () => { + // The other half of the outage rule: without a boot id to compare, + // only TOKENLESS records are accepted — and they still are. + vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); + await registerSession({ sessionId: 's0', cwd: '/w/app' }); + await writeRaw(`${process.pid}.json`, liveBody()); + + await patchSessionRecord({ sessionId: 'new' }); + + const raw = JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')); + expect(raw.sessionId).toBe('new'); + }, + ); + const itPosixPatch = it.runIf(process.platform !== 'win32'); itPosixPatch('keeps the record at 0600 across a patch', async () => { @@ -550,6 +747,7 @@ describe('unregisterSession', () => { // The path is keyed by PID alone: the record sitting there may // belong to a live session in another namespace that shares the // number. Unlinking it would hide that session until it restarts. + await registerSession({ sessionId: 's0', cwd: '/w/app' }); const foreign = liveBody({ pidNs: 1, sessionId: 'theirs' }); await writeRaw(`${process.pid}.json`, foreign); @@ -560,6 +758,29 @@ describe('unregisterSession', () => { ).toEqual(foreign); }); + it.runIf(process.platform === 'linux')( + 'leaves a record from another machine’s boot at its path alone', + async () => { + // Mirror of the register-side pin: the boot prefix is the ONLY + // identity separating two machines sharing one home (the initial + // namespace inode is a kernel constant), so exit must not unlink + // the other machine's live record on a PID collision. + expect(readLocalBootId()).not.toBeNull(); + await registerSession({ sessionId: 's0', cwd: '/w/app' }); + const foreign = liveBody({ + procStart: 'not-this-boot:1', + sessionId: 'theirs', + }); + await writeRaw(`${process.pid}.json`, foreign); + + await unregisterSession(); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }, + ); + it('is a no-op when nothing was registered', async () => { await expect(unregisterSession()).resolves.toBeUndefined(); }); @@ -660,6 +881,28 @@ describe('listLiveSessions', () => { }, ); + it.runIf(process.platform === 'linux')( + 'leaves every boot-prefixed record alone while the local boot id is unreadable', + async () => { + // The guard must not be disabled by OUR outage: without this, the + // foreign record falls through to `isSameProcess`, which degrades + // to bare liveness in the same outage, and the sweep unlinks + // another machine's live record. Tokenless records keep listing — + // the outage degrades the boot check, not the whole registry. + vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); + const foreign = await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, procStart: 'not-this-boot:1' }), + ); + await writeRaw(`${process.pid}.json`, liveBody()); + + const live = await listLiveSessions(); + + expect(live.map((r) => r.sessionId)).toEqual(['s']); + await expect(fs.stat(foreign)).resolves.toBeDefined(); + }, + ); + it('sweeps registration temp files orphaned by a crashed write', async () => { // A writer that dies between the temp write and the rename leaves // the temp behind; nothing else ever removes it. diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index e9d77bf1d53..5d99102dfe6 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -52,6 +52,17 @@ * cleanup): no future reader shares that namespace, so the record is * never reclaimed and stays until removed by hand. Anything else we * cannot positively prove dead is left alone. + * + * One consequence of the boot prefix being the only machine identity: it + * does not survive a reboot, and the model has no reboot-surviving + * machine component to pair it with, so this machine's own records from + * a PREVIOUS boot — provably dead, since a boot ending kills every + * process in it — are indistinguishable from another machine's live + * records. Both conservative rules above therefore also strand them: a + * hard reset (power loss, panic, kill -9 plus reboot) leaves a record + * nothing ever reclaims, and a later session that draws the same PID + * number is refused by registration on the boot mismatch and stays + * undiscoverable until the file is removed by hand. */ import { createHash } from 'node:crypto'; @@ -143,6 +154,20 @@ export function getSessionRecordPath(): string { return path.join(getSessionRegistryDir(), `${process.pid}.json`); } +/** + * The record path captured when registration succeeds. `getGlobalQwenDir()` + * resolves a relative `QWEN_HOME` against the CURRENT `process.cwd()` on + * every call, and `/cd` changes the cwd mid-session — so patch and + * unregister must keep operating on the directory registration wrote to + * rather than wherever the cwd moved afterwards. Null before a successful + * registration (both seams no-op then anyway) and consumed by unregister. + */ +let registeredRecordPath: string | null = null; + +function thisProcessRecordPath(): string { + return registeredRecordPath ?? getSessionRecordPath(); +} + /** * A short, stable, human-readable label: the working directory's basename * plus two hex characters derived from the session id. @@ -153,16 +178,25 @@ export function getSessionRecordPath(): string { * same-directory collision unlikely rather than certain; callers that * need a guaranteed-unique handle should use the session id. * - * Letters and digits are matched Unicode-aware: an ASCII-only class would - * strip a CJK basename down to a bare dash, leaving every such project - * with an identical, information-free label. + * Letters, digits and combining marks are matched Unicode-aware: an + * ASCII-only class would strip a CJK basename down to a bare dash, + * leaving every such project with an identical, information-free label. + * The basename is NFC-normalized first and combining marks are kept, so + * an NFD accent (macOS' default normalization) or an Indic vowel sign is + * not dashed away mid-word, and the cap counts code points rather than + * UTF-16 units, so an astral character at the boundary cannot be cut + * into a lone surrogate. */ export function deriveSessionName(cwd: string, sessionId: string): string { - const base = path - .basename(cwd) - .replace(/[^\p{L}\p{N}._-]+/gu, '-') - .replace(/^-+|-+$/g, '') - .slice(0, 32); + const base = Array.from( + path + .basename(cwd) + .normalize('NFC') + .replace(/[^\p{L}\p{M}\p{N}._-]+/gu, '-') + .replace(/^-+|-+$/g, ''), + ) + .slice(0, 32) + .join(''); const suffix = createHash('sha256') .update(sessionId) .digest('hex') @@ -177,18 +211,34 @@ export function deriveSessionName(cwd: string, sessionId: string): string { * * Returns true when the record was written. Returns false — without * writing — when the path is already held by a record carrying another - * namespace's or another machine's identity: a colliding session on the - * other side is live, and overwriting its record would hide it from - * discovery and destroy it when we exit. This session then simply stays + * namespace's or another machine's identity, or one this build cannot + * parse because of a newer schema version: a colliding session on the + * other side may be live, and overwriting its record would hide it from + * discovery and destroy it when we exit. Also refuses on Linux when the + * start token is unreadable even after a retry — a tokenless record is + * impersonable by any same-namespace reader, including another machine + * sharing the home. In all of these cases this session simply stays * undiscoverable. */ export async function registerSession( fields: RegisterSessionFields, ): Promise { + let procStart = readProcStartToken(process.pid); + if (process.platform === 'linux' && procStart === null) { + // Boot-id read failures are not cached, so a second attempt recovers + // from the transient fd-pressure moment startup registration sits in. + procStart = readProcStartToken(process.pid); + if (procStart === null) { + debugLogger.debug( + 'registerSession: start token unreadable; refusing to write an impersonable record', + ); + return false; + } + } const record: SessionRegistryRecord = { schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, pid: process.pid, - procStart: readProcStartToken(process.pid), + procStart, pidNs: readPidNamespaceId(), sessionId: fields.sessionId, cwd: fields.cwd, @@ -204,9 +254,16 @@ export async function registerSession( // collided with us on the PID number across a namespace or machine // boundary (host + devcontainer, sibling containers, NFS-shared // homes). Overwriting it would hide that session from discovery and - // destroy its record when we exit — refuse instead. + // destroy its record when we exit — refuse instead. The same holds + // for a newer-schema record: readable, but not safely parsable. const existing = await readRecord(filePath); - if (existing !== null && !matchesLocalIdentity(existing)) { + if (existing.status === 'unsupported-version') { + debugLogger.debug( + 'registerSession: record path held by a newer-schema record', + ); + return false; + } + if (existing.status === 'ok' && !matchesLocalIdentity(existing.record)) { debugLogger.debug( 'registerSession: record path held by a foreign-identity record', ); @@ -222,6 +279,7 @@ export async function registerSession( forceMode: true, noFollow: true, }); + registeredRecordPath = filePath; return true; } catch (error) { debugLogger.debug(`registerSession failed: ${describe(error)}`); @@ -249,17 +307,21 @@ export async function patchSessionRecord( >, ): Promise { try { - // Inside the try: `getGlobalQwenDir()` resolves the home directory - // and can throw, and this function promises never to reject. - const filePath = getSessionRecordPath(); + // Inside the try: the fallback `getGlobalQwenDir()` resolution reads + // the home directory and can throw, and this function promises never + // to reject. + const filePath = thisProcessRecordPath(); const existing = await readRecord(filePath); - // Missing, or not actually a record for this PID, namespace and - // boot: the path is keyed by PID alone, and `readRecord` does not - // check the filename/contents agreement that `listLiveSessions` - // insists on, so merging into a foreign record would write back - // something the reader will neither show nor sweep — permanent - // litter. - if (existing === null || !matchesLocalIdentity(existing)) return; + // Missing, unreadable, newer-schema, or not actually a record for + // this PID, namespace and boot: the path is keyed by PID alone, and + // `readRecord` does not check the filename/contents agreement that + // `listLiveSessions` insists on, so merging into a foreign record + // would write back something the reader will neither show nor sweep + // — permanent litter. + if (existing.status !== 'ok' || !matchesLocalIdentity(existing.record)) { + return; + } + const record = existing.record; // The identity check alone also passes for a stale record left by a // DEAD previous incarnation of this PID (session A dies without // unlinking; the PID is recycled by session B whose registration @@ -268,15 +330,15 @@ export async function patchSessionRecord( // record and lists the chimera as live. const currentToken = readProcStartToken(process.pid); if ( - existing.procStart !== null && + record.procStart !== null && currentToken !== null && - existing.procStart !== currentToken + record.procStart !== currentToken ) { return; } await atomicWriteJSON( filePath, - { ...existing, ...patch }, + { ...record, ...patch }, { mode: REGISTRY_FILE_MODE, forceMode: true, noFollow: true }, ); } catch (error) { @@ -287,14 +349,21 @@ export async function patchSessionRecord( /** Remove this process's record. Safe to call when none was written. */ export async function unregisterSession(): Promise { try { - const filePath = getSessionRecordPath(); + const filePath = thisProcessRecordPath(); + // Consume the capture regardless of the outcome below: after this + // call returns, this process no longer holds a record at the path. + registeredRecordPath = null; // The path is keyed by PID alone, and PIDs collide across namespace // and machine boundaries: the record sitting at our path may belong // to a live session on the other side of a shared home. Unlink only // what this side wrote; an unreadable one (a write torn by a crash) - // cannot belong to anyone else and goes too. + // cannot belong to anyone else and goes too. A newer-schema record + // is readable but not safely parsable, so it might be — leave it. const existing = await readRecord(filePath); - if (existing !== null && !matchesLocalIdentity(existing)) return; + if (existing.status === 'unsupported-version') return; + if (existing.status === 'ok' && !matchesLocalIdentity(existing.record)) { + return; + } await fs.unlink(filePath); } catch (error) { if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') return; @@ -337,8 +406,11 @@ export async function listLiveSessions(): Promise { return; } - const record = await readRecord(filePath); - if (record === null) return; + const read = await readRecord(filePath); + // Malformed and newer-schema records are skipped without sweeping + // — the list path never deletes what it cannot fully understand. + if (read.status !== 'ok') 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 @@ -358,14 +430,13 @@ export async function listLiveSessions(): Promise { // write one directory. A token whose boot prefix names another // boot names another machine — its PIDs resolve to the wrong // processes here, so leave it to a reader on its own side, and - // never sweep it. + // never sweep it. The guard must not depend on OUR boot id being + // readable: during the same outage `isSameProcess` degrades to a + // bare liveness check, so a disabled guard here would let the + // sweep unlink another machine's live record. const recordBootId = record.procStart === null ? null : bootIdOf(record.procStart); - if ( - ownBootId !== null && - recordBootId !== null && - recordBootId !== ownBootId - ) { + if (recordBootId !== null && recordBootId !== ownBootId) { return; } @@ -389,17 +460,24 @@ export async function listLiveSessions(): Promise { * True when a record found at this process's path was written from this * PID namespace and this boot. The path is keyed by PID alone, and PID * numbers collide across both boundaries; these are the fields a - * colliding writer on another side differs on. A missing token or an - * unreadable local boot id degrades to the namespace comparison alone, - * matching `isSameProcess`'s conservatism. + * colliding writer on another side differs on. A missing token degrades + * to the namespace comparison alone, matching `isSameProcess`'s + * conservatism. + * + * An unreadable local boot id is different: `isSameProcess`'s + * conservatism is "keep when unprovable", which is right for a read + * path, but the callers here decide to OVERWRITE, MERGE or UNLINK — on + * those write paths "cannot compare" must mean "not ours". A writer + * that cannot read its boot id writes a tokenless record, so accepting + * only tokenless records in this state costs nothing. */ function matchesLocalIdentity(record: SessionRegistryRecord): boolean { if (record.pid !== process.pid) return false; if (record.pidNs !== readPidNamespaceId()) return false; if (record.procStart === null) return true; const ownBootId = readLocalBootId(); - if (ownBootId === null) return true; const recordBootId = bootIdOf(record.procStart); + if (ownBootId === null) return recordBootId === null; return recordBootId === null || recordBootId === ownBootId; } @@ -430,39 +508,55 @@ async function sweepOrphanedTempFile( } } -/** Read and validate one record. Returns null for anything unusable. */ -async function readRecord( - filePath: string, -): Promise { +/** + * One record file as read from disk: parsed, or unusable. + * + * `unreadable` covers a missing, corrupt or torn file — content this + * code never wrote in a usable form. `unsupported-version` is a + * well-formed record written by a NEWER build: readable, and it may + * belong to a live session this build cannot parse, so the write paths + * must treat it the way they treat a parsed foreign-identity record. + */ +type ReadRecordResult = + | { status: 'ok'; record: SessionRegistryRecord } + | { status: 'unreadable' } + | { status: 'unsupported-version' }; + +const UNREADABLE: ReadRecordResult = { status: 'unreadable' }; +const UNSUPPORTED_VERSION: ReadRecordResult = { + status: 'unsupported-version', +}; + +/** Read and validate one record, discriminating newer-schema files. */ +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; + if (!stat.isFile() || stat.size > MAX_RECORD_BYTES) return UNREADABLE; raw = await fs.readFile(filePath, 'utf8'); } catch { - return null; + return UNREADABLE; } let parsed: unknown; try { parsed = JSON.parse(raw); } catch { - return null; + return UNREADABLE; } if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { - return null; + return UNREADABLE; } 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. + // an unknown *higher* version is reported as such rather than guessed + // at — and, unlike a torn file, must not be treated as unowned. const schemaVersion = value['schemaVersion']; - if ( - typeof schemaVersion !== 'number' || - schemaVersion > SESSION_REGISTRY_SCHEMA_VERSION - ) { - return null; + if (typeof schemaVersion !== 'number') return UNREADABLE; + if (schemaVersion > SESSION_REGISTRY_SCHEMA_VERSION) { + return UNSUPPORTED_VERSION; } const pid = value['pid']; @@ -480,7 +574,7 @@ async function readRecord( typeof startedAt !== 'number' || !Number.isFinite(startedAt) ) { - return null; + return UNREADABLE; } const procStart = value['procStart']; @@ -488,15 +582,18 @@ async function readRecord( const qwenVersion = value['qwenVersion']; return { - schemaVersion, - pid, - procStart: typeof procStart === 'string' ? procStart : null, - pidNs: typeof pidNs === 'number' && Number.isFinite(pidNs) ? pidNs : null, - sessionId, - cwd, - name, - startedAt, - qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, + status: 'ok', + record: { + schemaVersion, + pid, + procStart: typeof procStart === 'string' ? procStart : null, + pidNs: typeof pidNs === 'number' && Number.isFinite(pidNs) ? pidNs : null, + sessionId, + cwd, + name, + startedAt, + qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, + }, }; } diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index 37eef4119b9..585c61cbe7b 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -180,11 +180,16 @@ describe('Config.startNewSession session-registry patch', () => { const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; const config = makeConfig(sessionA); config.markRuntimeStatusEnabled(); - await registerSession({ - sessionId: sessionA, - cwd: tmpDir, - qwenVersion: '0.0.0-test', - }); + // registerSession never throws — it returns false on a failed write. + // Assert the seam itself before relying on anything downstream of + // it, or a write failure surfaces as a confusing patch-path error. + expect( + await registerSession({ + sessionId: sessionA, + cwd: tmpDir, + qwenVersion: '0.0.0-test', + }), + ).toBe(true); config.markSessionRegistered(); const [before] = await listLiveSessions(); @@ -216,11 +221,13 @@ describe('Config.startNewSession session-registry patch', () => { const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; const config = makeConfig(sessionA); // No markRuntimeStatusEnabled(): models the failed sidecar write. - await registerSession({ - sessionId: sessionA, - cwd: tmpDir, - qwenVersion: '0.0.0-test', - }); + expect( + await registerSession({ + sessionId: sessionA, + cwd: tmpDir, + qwenVersion: '0.0.0-test', + }), + ).toBe(true); config.markSessionRegistered(); config.startNewSession(sessionB); From 8a69962f77dc6a685a8fe87d16f2d3c798f41297 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 22:58:43 +0000 Subject: [PATCH 5/8] fix(cli): keep the review-context manifest under the resolved-file bound (#8969) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The committed-manifest tripwire failed once main's coordinate skill landed: the every-rule-co-matched relatedPaths expansion grew from 128 to 129 resolved files, one past the wire bound. Narrow the core-skills rule's relatedPaths to the infrastructure files directly under packages/core/src/skills/ — bundled skill content is self-contained, arrives in the diff itself when it changes, and grows with every new bundled skill, so leaving bundled/** in the glob would spend the bound's headroom on each addition. All fail-closed bounds stay pinned. Co-authored-by: Qwen-Coder --- .qwen/review-context.json | 2 +- .../review/lib/manifest-repository-context.committed.test.ts | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.qwen/review-context.json b/.qwen/review-context.json index f3c553bb593..f61e25daa32 100644 --- a/.qwen/review-context.json +++ b/.qwen/review-context.json @@ -21,7 +21,7 @@ }, { "paths": ["packages/core/src/skills/**"], - "relatedPaths": ["packages/core/src/skills/**"], + "relatedPaths": ["packages/core/src/skills/*"], "domains": ["core-skills"] }, { diff --git a/packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts b/packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts index 0b22dd7c11f..b0e3e41652e 100644 --- a/packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts +++ b/packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts @@ -47,7 +47,7 @@ const expectedManifest = { }, { paths: ['packages/core/src/skills/**'], - relatedPaths: ['packages/core/src/skills/**'], + relatedPaths: ['packages/core/src/skills/*'], domains: ['core-skills'], }, { @@ -95,8 +95,7 @@ const expectedManifest = { const relatedPathSentinels: Readonly> = { 'packages/core/src/config/**': 'packages/core/src/config/config.ts', - 'packages/core/src/skills/**': - 'packages/core/src/skills/bundled/review/SKILL.md', + 'packages/core/src/skills/*': 'packages/core/src/skills/skill-manager.ts', 'packages/web-shell/client/adapters/**': 'packages/web-shell/client/adapters/types.ts', 'packages/web-shell/client/completions/**': From e2309807e5b2db2f938475892b1130c2a8003f38 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 04:29:17 +0000 Subject: [PATCH 6/8] fix(core): close session registry race and outage gaps per round-4 review (#8969) Refuse registration when the Linux PID-namespace id stays unreadable (the record would be unreclaimable litter poisoning its PID slot), retry once like the start token. Treat non-ENOENT read failures as intact-foreign ("read-error") in register/unregister instead of unowned. Require a readable matching start token before patch merges. Re-read a record before the sweep unlinks it so a registration winning the window is not deleted. Tolerate ENOSYS/ENOTSUP on the registry-dir chmod. Move registry patches off the sidecar write chain onto their own never-awaited chain so a rejecting or hanging sidecar write can neither skip a patch nor hang /cd on the HOME write. --- packages/core/src/config/config.test.ts | 100 ++++++- packages/core/src/config/config.ts | 44 ++- .../src/services/session-registry.test.ts | 260 +++++++++++++++++- .../core/src/services/session-registry.ts | 139 ++++++++-- 4 files changed, 495 insertions(+), 48 deletions(-) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 738f2150f99..4d825761106 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -6269,12 +6269,11 @@ describe('Server Config (config.ts)', () => { return checked === oldRuntimeStatusPath || checked === newDir; }); - // `toHaveBeenCalledWith` records the call the moment the promise is - // created, so it cannot distinguish an awaited patch from a - // fire-and-forget one; the settlement log pins that the registry - // update completes before `relocateWorkingDirectory` resolves — the - // user-visible guarantee is that `ps` already shows the new - // directory once `/cd` returns. + // The registry patch rides its own chain and `/cd` deliberately does + // not await it: the patch writes the HOME filesystem, and awaiting + // it in the flush would hang `/cd` whenever HOME stalls while the + // project directory is healthy. The settlement log pins the new + // contract — `/cd` returns first, and `ps` settles a tick later. const settled: string[] = []; const patchSessionRecordSpy = vi .spyOn(sessionRegistry, 'patchSessionRecord') @@ -6298,11 +6297,14 @@ describe('Server Config (config.ts)', () => { // The registry's DIRECTORY column is how a user tells two live // sessions apart; the switch must reach it (and the directory-derived // name) or `qwen sessions ps` keeps showing the folder that was left. - expect(patchSessionRecordSpy).toHaveBeenCalledWith({ - cwd: newDir, - name: sessionRegistry.deriveSessionName(newDir, sessionId), + await vi.waitFor(() => { + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + cwd: newDir, + name: sessionRegistry.deriveSessionName(newDir, sessionId), + }); + expect(settled).toContain('patch'); }); - expect(settled).toEqual(['patch', 'relocated']); + expect(settled[0]).toBe('relocated'); writeRuntimeStatusSpy.mockRestore(); patchSessionRecordSpy.mockRestore(); @@ -6336,10 +6338,13 @@ describe('Server Config (config.ts)', () => { await config.relocateWorkingDirectory(newDir); - expect(patchSessionRecordSpy).toHaveBeenCalledWith({ - cwd: newDir, - name: sessionRegistry.deriveSessionName(newDir, sessionId), - }); + // The patch rides its own fire-and-forget chain; let it settle. + await vi.waitFor(() => + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + cwd: newDir, + name: sessionRegistry.deriveSessionName(newDir, sessionId), + }), + ); expect(writeRuntimeStatusSpy).not.toHaveBeenCalled(); writeRuntimeStatusSpy.mockRestore(); @@ -6393,6 +6398,73 @@ describe('Server Config (config.ts)', () => { cwdSpy.mockRestore(); }); + it('startNewSession patches the registry even when the sidecar write rejects', async () => { + // The sidecar (project-local chats/) and the registry (global dir) + // are independent failure domains: a sidecar write rejecting on a + // read-only or full project filesystem mid-session must not skip + // the registry patch, or `ps` advertises the pre-/clear session id + // until process exit. + const config = new Config(baseParams); + config.markRuntimeStatusEnabled(); + config.markSessionRegistered(); + const writeRuntimeStatusSpy = vi + .spyOn(runtimeStatus, 'writeRuntimeStatus') + .mockRejectedValue(new Error('read-only project fs')); + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockResolvedValue(undefined); + + const newSessionId = config.startNewSession('replacement-session'); + + await vi.waitFor(() => + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + sessionId: newSessionId, + cwd: config.getTargetDir(), + }), + ); + + writeRuntimeStatusSpy.mockRestore(); + patchSessionRecordSpy.mockRestore(); + }); + + it('relocateWorkingDirectory patches the registry even when the sidecar write rejects', async () => { + // The /cd-side mirror of the /clear pin: a rejecting sidecar write + // must not skip the directory patch, and its rejection must not + // surface through relocateWorkingDirectory either. + const config = new Config(baseParams); + config.markRuntimeStatusEnabled(); + config.markSessionRegistered(); + const sessionId = config.getSessionId(); + const newDir = path.resolve('/path/to/other'); + const chdirSpy = vi.spyOn(process, 'chdir').mockImplementation(() => { + // Keep the test process in its original directory. + }); + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(newDir); + const writeRuntimeStatusSpy = vi + .spyOn(runtimeStatus, 'writeRuntimeStatus') + .mockRejectedValue(new Error('read-only project fs')); + vi.mocked(fs.existsSync).mockImplementation( + (pathToCheck) => pathToCheck.toString() === newDir, + ); + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockResolvedValue(undefined); + + await config.relocateWorkingDirectory(newDir); + + await vi.waitFor(() => + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + cwd: newDir, + name: sessionRegistry.deriveSessionName(newDir, sessionId), + }), + ); + + writeRuntimeStatusSpy.mockRestore(); + patchSessionRecordSpy.mockRestore(); + chdirSpy.mockRestore(); + cwdSpy.mockRestore(); + }); + it('relocateWorkingDirectory should reject and roll back when session artifact migration fails', async () => { const config = new Config({ ...baseParams, chatRecording: true }); const disposeResidentAgents = vi.spyOn( diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index f9be30d1ea2..fcc96653909 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2071,6 +2071,7 @@ export class Config { private proxyDispatcherReady?: Promise; storage: Storage; private runtimeStatusWrite: Promise = Promise.resolve(); + private sessionRegistryWrite: Promise = Promise.resolve(); private readonly fileExclusions: FileExclusions; private readonly truncateToolOutputThreshold: number; private readonly truncateToolOutputLines: number; @@ -3956,7 +3957,7 @@ export class Config { // just read out of `qwen sessions ps`, and re-deriving it here // would rename a live session on every /clear for no gain — the // directory it names has not changed. - this.queueRuntimeStatusWrite(async () => { + this.queueSessionRegistryWrite(async () => { await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); }); } @@ -4000,6 +4001,35 @@ export class Config { }); } + /** + * Queue a session-registry patch on its own serial chain. + * + * The chain is separate from `runtimeStatusWrite` and is deliberately + * never awaited by `flushRuntimeStatusWrites`: + * + * - A sidecar write that rejects or hangs must not skip or block the + * patch — the two target independent failure domains (project-local + * `chats/` dir vs the global Qwen dir). + * - The patch writes the HOME filesystem, while `/cd` flushes the + * sidecar chain: awaiting the patch there would hang `/cd` whenever + * HOME stalls while the project directory is healthy. Registry + * patches are best-effort by contract — `ps` settles a tick after + * the transition returns — so nothing ever awaits this chain. + * + * Patches still serialize among themselves so back-to-back /clear and + * /cd transitions cannot interleave their read-modify-write. + */ + private queueSessionRegistryWrite(write: () => Promise): void { + this.sessionRegistryWrite = this.sessionRegistryWrite + .catch(() => { + // Keep later patches alive after a best-effort patch failure. + }) + .then(write) + .catch(() => { + // ignored: registry patches must not disrupt session control flow. + }); + } + private async flushRuntimeStatusWrites(): Promise { await this.runtimeStatusWrite.catch(() => { // ignored: runtime status is best-effort. @@ -4008,12 +4038,10 @@ export class Config { private async refreshCurrentRuntimeStatus(workDir: string): Promise { const sessionId = this.sessionId; - // Two separate queue entries, mirroring startNewSession: - // `writeRuntimeStatus` propagates exceptions, so sharing one closure - // would let a sidecar failure (read-only or full project filesystem) - // reject the entry before the registry patch runs — and the queue's - // trailing catch would swallow it, leaving `ps` advertising the - // folder this session left. The failure domains are independent. + // The sidecar write and the registry patch ride separate chains + // (see queueSessionRegistryWrite): a sidecar failure on the + // project filesystem must neither skip the patch nor hang `/cd` on + // the HOME write. The failure domains are independent. if (this.runtimeStatusEnabled) { const sidecarPath = this.storage.getRuntimeStatusPath(sessionId); this.queueRuntimeStatusWrite(async () => { @@ -4025,7 +4053,7 @@ export class Config { }); } if (this.sessionRegistered) { - this.queueRuntimeStatusWrite(async () => { + this.queueSessionRegistryWrite(async () => { // The registry's DIRECTORY column is how a user tells two live // sessions apart, so a mid-session directory switch has to reach // it too — otherwise `qwen sessions ps` keeps advertising the diff --git a/packages/core/src/services/session-registry.test.ts b/packages/core/src/services/session-registry.test.ts index 18438cc95f7..aa191ab90f2 100644 --- a/packages/core/src/services/session-registry.test.ts +++ b/packages/core/src/services/session-registry.test.ts @@ -15,6 +15,7 @@ import { listLiveSessions, patchSessionRecord, registerSession, + resetRegisteredRecordPathForTest, unregisterSession, SESSION_REGISTRY_SCHEMA_VERSION, } from './session-registry.js'; @@ -83,6 +84,9 @@ const DEAD_PID = 0x7ffffffe; beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'session-registry-')); __setMockGlobalDir(tmpDir); + // The registry captures the registered path at module level; reset it + // so tests that need the unregistered state never ride test order. + resetRegisteredRecordPathForTest(); }); afterEach(async () => { @@ -440,6 +444,95 @@ describe('registerSession', () => { }, ); + it.runIf(process.platform === 'linux')( + 'refuses to write a record when the namespace id stays unreadable', + async () => { + // A namespace-less Linux record is unreclaimable litter: every + // healthy reader's own namespace is a number, so the namespace + // guard hides the record from listing AND from the sweep's unlink, + // every later patch fails matchesLocalIdentity, and exit leaves it + // in place — poisoning the PID slot for the next session. + vi.spyOn(processLiveness, 'readPidNamespaceId').mockReturnValue(null); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + false, + ); + + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + expect(await listLiveSessions()).toEqual([]); + }, + ); + + it.runIf(process.platform === 'linux')( + 'retries the namespace id once before refusing', + async () => { + // Mirrors the start-token retry: statSync failures are transient, + // and only a persistent outage refuses. + vi.spyOn(processLiveness, 'readPidNamespaceId').mockReturnValueOnce(null); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + true, + ); + + const raw = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ) as Record; + expect(raw['pidNs']).toBeTypeOf('number'); + }, + ); + + it('refuses to overwrite a record whose read fails transiently', async () => { + // A stat/readFile failure with a code other than ENOENT (EMFILE, + // EIO, NFS ESTALE) is a momentary outage on an INTACT file — the + // record may belong to a live session on another machine sharing + // the home, so the failure must not be treated as "unowned". + const foreign = liveBody({ pidNs: 1, sessionId: 'theirs' }); + await writeRaw(`${process.pid}.json`, foreign); + const err = new Error('stale file handle') as NodeJS.ErrnoException; + err.code = 'ESTALE'; + vi.spyOn(fs, 'readFile').mockRejectedValueOnce(err); + + expect(await registerSession({ sessionId: 'mine', cwd: '/w/app' })).toBe( + false, + ); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }); + + it('still registers when the filesystem does not support chmod', async () => { + // FAT/exFAT/FUSE-class mounts reject chmod with ENOTSUP/ENOSYS. The + // record write already tolerates that (atomicWriteJSON's tryChmod), + // and 0700 is unachievable on such a filesystem anyway — the + // directory chmod must not abort registration there either, or the + // session stays invisible to every `ps` for its whole lifetime. + const err = new Error('chmod unsupported') as NodeJS.ErrnoException; + err.code = 'ENOTSUP'; + vi.spyOn(fs, 'chmod').mockRejectedValue(err); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + true, + ); + + const raw = JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')); + expect(raw.sessionId).toBe('s1'); + }); + + it('still fails registration on a security-relevant chmod error', async () => { + // The ENOSYS/ENOTSUP tolerance is narrow: sandbox EPERM, EIO and + // friends still abort the registration. + const err = new Error('operation not permitted') as NodeJS.ErrnoException; + err.code = 'EPERM'; + vi.spyOn(fs, 'chmod').mockRejectedValue(err); + + expect(await registerSession({ sessionId: 's1', cwd: '/w/app' })).toBe( + false, + ); + + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + }); + it('leaves a newer-schema record at its path alone on every write path', async () => { // A record written by a newer build is readable but not safely // parsable, and under a shared home it may belong to a live session @@ -599,6 +692,29 @@ describe('patchSessionRecord', () => { }, ); + it.runIf(process.platform === 'linux')( + "refuses to merge when this process's own start token is unreadable", + async () => { + // The merge path's mirror of the boot-id outage rule: under the + // fd-pressure window these patches run in, our own token read can + // fail while a DEAD previous incarnation's record holds the path. + // "Cannot compare" must mean "not ours" — the patch is skipped + // and a later /clear or /cd retries it; merging would graft this + // session's fields onto the stale record and list the chimera. + await registerSession({ sessionId: 's0', cwd: '/w/app' }); + const before = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ); + vi.spyOn(processLiveness, 'readProcStartToken').mockReturnValue(null); + + await patchSessionRecord({ sessionId: 'incarnation-b' }); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(before); + }, + ); + it('preserves the identity fields across the patch merge', async () => { // Both production patch sites run in ordinary use; a merge that // drops `procStart` degrades every later liveness check to a bare @@ -643,8 +759,15 @@ describe('patchSessionRecord', () => { // merge would corrupt it. A writer with an unreadable boot id // writes a TOKENLESS record, so refusing boot-prefixed ones here // costs nothing. - vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); await registerSession({ sessionId: 's0', cwd: '/w/app' }); + vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); + // Also pin the token read to the planted record's token: left + // real, the stale-incarnation guard would refuse the merge on its + // own (a foreign boot prefix never equals this process's real + // token) and shadow the identity rule under test. + vi.spyOn(processLiveness, 'readProcStartToken').mockReturnValue( + 'not-this-boot:1', + ); const foreign = liveBody({ procStart: 'not-this-boot:1', sessionId: 'theirs', @@ -781,6 +904,60 @@ describe('unregisterSession', () => { }, ); + it("unlinks a corrupt record at this process's own path", async () => { + // A write torn by a crash at OUR path cannot belong to anyone else, + // and exit is the only reclaimer: listLiveSessions never deletes + // what it cannot parse. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + await writeRaw(`${process.pid}.json`, 'not json at all'); + + await unregisterSession(); + + await expect(fs.stat(getSessionRecordPath())).rejects.toThrow(); + }); + + it.runIf(process.platform === 'linux')( + 'leaves a boot-prefixed record alone on exit while the local boot id is unreadable', + async () => { + // Exit is an UNLINK path, so the outage rule applies: "cannot + // compare" means "not ours". The record may belong to another + // machine sharing the home on a PID collision, and a boot-id + // outage must not let exit destroy it. The unregister path has no + // stale-incarnation guard that could shadow the rule here. + vi.spyOn(processLiveness, 'readLocalBootId').mockReturnValue(null); + const foreign = liveBody({ + procStart: 'not-this-boot:1', + sessionId: 'theirs', + }); + await writeRaw(`${process.pid}.json`, foreign); + + await unregisterSession(); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(foreign); + }, + ); + + it('leaves a record alone on exit when its read fails transiently', async () => { + // The file is intact and only momentarily unreadable (EMFILE, EIO, + // NFS ESTALE) — it may be a foreign live record on a PID collision, + // so exit must not unlink it on the strength of a read failure. + await registerSession({ sessionId: 's1', cwd: '/w/app' }); + const before = JSON.parse( + await fs.readFile(getSessionRecordPath(), 'utf8'), + ); + const err = new Error('stale file handle') as NodeJS.ErrnoException; + err.code = 'ESTALE'; + vi.spyOn(fs, 'readFile').mockRejectedValueOnce(err); + + await unregisterSession(); + + expect( + JSON.parse(await fs.readFile(getSessionRecordPath(), 'utf8')), + ).toEqual(before); + }); + it('is a no-op when nothing was registered', async () => { await expect(unregisterSession()).resolves.toBeUndefined(); }); @@ -791,6 +968,18 @@ describe('listLiveSessions', () => { expect(await listLiveSessions()).toEqual([]); }); + it('resolves to an empty list when readdir itself fails', async () => { + // EACCES (a root-owned sessions/ left by a containerized run) or + // ESTALE/EIO on a degraded shared home must read as "no peers", + // never as a rejection — `ps` awaits this with no catch. + await writeRaw(`${process.pid}.json`, liveBody()); + const err = new Error('permission denied') as NodeJS.ErrnoException; + err.code = 'EACCES'; + vi.spyOn(fs, 'readdir').mockRejectedValueOnce(err); + + await expect(listLiveSessions()).resolves.toEqual([]); + }); + it('sweeps a record whose process is gone', async () => { const filePath = await writeRaw( `${DEAD_PID}.json`, @@ -834,6 +1023,75 @@ describe('listLiveSessions', () => { }, ); + it('does not unlink a record replaced between the liveness verdict and the sweep', async () => { + // Session A died uncleanly and the sweep judges its record dead. In + // the window before the unlink, the recycled PID's registration + // passes matchesLocalIdentity against the stale record and renames + // its fresh record onto the same path — the sweep must not delete + // the fresh record it never judged, or that session runs its whole + // lifetime invisible to `ps`. + await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, sessionId: 'stale-a', startedAt: 5 }), + ); + + const realReadFile = fs.readFile; + let readsOfRecord = 0; + let releaseReread!: () => void; + const rereadGate = new Promise((resolve) => { + releaseReread = resolve; + }); + vi.spyOn(fs, 'readFile').mockImplementation((async (filePath: unknown) => { + if (String(filePath).endsWith(`${DEAD_PID}.json`)) { + readsOfRecord += 1; + // Hold the sweep at the RE-READ (the snapshot read is the + // first hit) so the replacement lands in the exact window + // between the verdict and the re-read. + if (readsOfRecord === 2) await rereadGate; + } + return realReadFile(filePath as string, 'utf8'); + }) as unknown as typeof fs.readFile); + + const listing = listLiveSessions(); + await vi.waitFor(() => expect(readsOfRecord).toBe(2)); + + await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, sessionId: 'recycled-b', startedAt: 6 }), + ); + releaseReread(); + + expect(await listing).toEqual([]); + await expect( + fs.stat(path.join(getSessionRegistryDir(), `${DEAD_PID}.json`)), + ).resolves.toBeDefined(); + }); + + it("still lists live records when a sweep's unlink fails", async () => { + // The sweep's unlink can fail (EACCES, ESTALE) while the rest of + // the enumeration succeeds; one undeletable dead record must not + // reject the promise or hide the live sessions. + await writeRaw( + `${DEAD_PID}.json`, + liveBody({ pid: DEAD_PID, sessionId: 's-dead', startedAt: 1 }), + ); + await writeRaw( + `${process.pid}.json`, + liveBody({ sessionId: 's-live', startedAt: 2 }), + ); + const err = new Error('permission denied') as NodeJS.ErrnoException; + err.code = 'EACCES'; + vi.spyOn(fs, 'unlink').mockRejectedValueOnce(err); + + const live = await listLiveSessions(); + + expect(live.map((r) => r.sessionId)).toEqual(['s-live']); + // The failed unlink leaves the dead record for the next sweep. + await expect( + fs.stat(path.join(getSessionRegistryDir(), `${DEAD_PID}.json`)), + ).resolves.toBeDefined(); + }); + it('neither lists nor sweeps a record from a different PID namespace, even a dead one', async () => { // PID numbers do not resolve across the namespace boundary: kill(pid, 0) // over here reports ESRCH for a process alive over there, and a diff --git a/packages/core/src/services/session-registry.ts b/packages/core/src/services/session-registry.ts index 5d99102dfe6..e2533a3b8d4 100644 --- a/packages/core/src/services/session-registry.ts +++ b/packages/core/src/services/session-registry.ts @@ -164,6 +164,16 @@ export function getSessionRecordPath(): string { */ let registeredRecordPath: string | null = null; +/** + * Test-only: clear the registration-path capture so a suite starts each + * test from the unregistered state regardless of what an earlier test + * registered. Without a reset, tests that need the capture null only + * pass by accident of test order. + */ +export function resetRegisteredRecordPathForTest(): void { + registeredRecordPath = null; +} + function thisProcessRecordPath(): string { return registeredRecordPath ?? getSessionRecordPath(); } @@ -212,12 +222,16 @@ export function deriveSessionName(cwd: string, sessionId: string): string { * Returns true when the record was written. Returns false — without * writing — when the path is already held by a record carrying another * namespace's or another machine's identity, or one this build cannot - * parse because of a newer schema version: a colliding session on the - * other side may be live, and overwriting its record would hide it from - * discovery and destroy it when we exit. Also refuses on Linux when the - * start token is unreadable even after a retry — a tokenless record is - * impersonable by any same-namespace reader, including another machine - * sharing the home. In all of these cases this session simply stays + * parse because of a newer schema version, or one whose read failed + * transiently (an intact file cannot be proved unowned this moment): a + * colliding session on the other side may be live, and overwriting its + * record would hide it from discovery and destroy it when we exit. + * Also refuses on Linux when the start token or the PID namespace id + * stays unreadable after a retry — a tokenless record is impersonable + * by any same-namespace reader, including another machine sharing the + * home, and a namespace-less record is neither listed, patched nor + * swept by any healthy reader, so it would poison its PID slot until + * removed by hand. In all of these cases this session simply stays * undiscoverable. */ export async function registerSession( @@ -235,11 +249,28 @@ export async function registerSession( return false; } } + let pidNs = readPidNamespaceId(); + if (process.platform === 'linux' && pidNs === null) { + // Same transient fd-pressure window as the start token above, + // retried the same way. A namespace-less record is unreclaimable + // litter on Linux: every healthy reader's own namespace is a + // number, so the namespace guard hides the record from listing AND + // from the sweep's unlink, every later patch fails + // matchesLocalIdentity, and exit leaves it in place — poisoning + // the PID slot for the next session. Refuse rather than write it. + pidNs = readPidNamespaceId(); + if (pidNs === null) { + debugLogger.debug( + 'registerSession: namespace id unreadable; refusing to write an unreclaimable record', + ); + return false; + } + } const record: SessionRegistryRecord = { schemaVersion: SESSION_REGISTRY_SCHEMA_VERSION, pid: process.pid, procStart, - pidNs: readPidNamespaceId(), + pidNs, sessionId: fields.sessionId, cwd: fields.cwd, name: deriveSessionName(fields.cwd, fields.sessionId), @@ -255,8 +286,17 @@ export async function registerSession( // boundary (host + devcontainer, sibling containers, NFS-shared // homes). Overwriting it would hide that session from discovery and // destroy its record when we exit — refuse instead. The same holds - // for a newer-schema record: readable, but not safely parsable. + // for a newer-schema record: readable, but not safely parsable. And + // for a read that failed transiently (EMFILE, EIO, NFS ESTALE): the + // file is intact and only momentarily out of reach, so the failure + // must not be treated as "unowned". const existing = await readRecord(filePath); + if (existing.status === 'read-error') { + debugLogger.debug( + 'registerSession: record path read failed transiently; refusing to overwrite an intact record', + ); + return false; + } if (existing.status === 'unsupported-version') { debugLogger.debug( 'registerSession: record path held by a newer-schema record', @@ -273,7 +313,18 @@ export async function registerSession( // 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); + // Filesystems without POSIX permissions (FAT/exFAT, some FUSE + // mounts) reject chmod with ENOSYS/ENOTSUP — tolerate exactly those + // the way atomicWriteFile's tryChmod does for the record itself: + // 0700 is unachievable there anyway, and aborting would hide this + // session from every `ps` for its whole lifetime. Other errors + // still abort the registration. + try { + await fs.chmod(dir, REGISTRY_DIR_MODE); + } catch (error) { + const code = (error as NodeJS.ErrnoException)?.code; + if (code !== 'ENOSYS' && code !== 'ENOTSUP') throw error; + } await atomicWriteJSON(filePath, record, { mode: REGISTRY_FILE_MODE, forceMode: true, @@ -325,15 +376,15 @@ export async function patchSessionRecord( // The identity check alone also passes for a stale record left by a // DEAD previous incarnation of this PID (session A dies without // unlinking; the PID is recycled by session B whose registration - // failed). When both sides carry a start token, require it to agree - // before merging — otherwise the patch grafts B's fields onto A's - // record and lists the chimera as live. + // failed). A tokened record therefore also requires this process's + // own token to be readable and to agree before merging — otherwise + // the patch grafts B's fields onto A's record and lists the chimera + // as live. When our own token is unreadable (the fd-pressure window + // these patches run in), "cannot compare" means "not ours", the + // same rule matchesLocalIdentity applies on write paths: skip this + // patch and let a later /clear or /cd retry it. const currentToken = readProcStartToken(process.pid); - if ( - record.procStart !== null && - currentToken !== null && - record.procStart !== currentToken - ) { + if (record.procStart !== null && record.procStart !== currentToken) { return; } await atomicWriteJSON( @@ -358,9 +409,16 @@ export async function unregisterSession(): Promise { // to a live session on the other side of a shared home. Unlink only // what this side wrote; an unreadable one (a write torn by a crash) // cannot belong to anyone else and goes too. A newer-schema record - // is readable but not safely parsable, so it might be — leave it. + // is readable but not safely parsable, so it might be — leave it, + // and the same holds for a read that failed transiently: the file + // is intact and may be a foreign live record. const existing = await readRecord(filePath); - if (existing.status === 'unsupported-version') return; + if ( + existing.status === 'unsupported-version' || + existing.status === 'read-error' + ) { + return; + } if (existing.status === 'ok' && !matchesLocalIdentity(existing.record)) { return; } @@ -445,6 +503,24 @@ export async function listLiveSessions(): Promise { return; } + // A registration can win this path between the liveness verdict + // and the unlink: the recycled PID passes matchesLocalIdentity + // against the stale record and renames its fresh record onto the + // path. Re-read immediately before deleting and unlink only what + // is still the record the verdict was computed from. `startedAt` + // joins the comparison because on platforms without a start token + // nothing else separates two incarnations of one PID number. + const reread = await readRecord(filePath); + if ( + reread.status !== 'ok' || + reread.record.pid !== record.pid || + reread.record.pidNs !== record.pidNs || + reread.record.procStart !== record.procStart || + reread.record.startedAt !== record.startedAt + ) { + return; + } + try { await fs.unlink(filePath); } catch { @@ -512,17 +588,24 @@ async function sweepOrphanedTempFile( * One record file as read from disk: parsed, or unusable. * * `unreadable` covers a missing, corrupt or torn file — content this - * code never wrote in a usable form. `unsupported-version` is a - * well-formed record written by a NEWER build: readable, and it may - * belong to a live session this build cannot parse, so the write paths - * must treat it the way they treat a parsed foreign-identity record. + * code never wrote in a usable form. `read-error` is different: the + * stat/read itself failed with something other than ENOENT (EMFILE, + * EIO, NFS ESTALE), so the file is intact and only momentarily out of + * reach — it may belong to a live foreign session, and the write paths + * must refuse it the way they refuse `unsupported-version` instead of + * treating it as unowned. `unsupported-version` is a well-formed record + * written by a NEWER build: readable, and it may belong to a live + * session this build cannot parse, so the write paths must treat it the + * way they treat a parsed foreign-identity record. */ type ReadRecordResult = | { status: 'ok'; record: SessionRegistryRecord } | { status: 'unreadable' } + | { status: 'read-error' } | { status: 'unsupported-version' }; const UNREADABLE: ReadRecordResult = { status: 'unreadable' }; +const READ_ERROR: ReadRecordResult = { status: 'read-error' }; const UNSUPPORTED_VERSION: ReadRecordResult = { status: 'unsupported-version', }; @@ -534,8 +617,14 @@ async function readRecord(filePath: string): Promise { const stat = await fs.stat(filePath); if (!stat.isFile() || stat.size > MAX_RECORD_BYTES) return UNREADABLE; raw = await fs.readFile(filePath, 'utf8'); - } catch { - return UNREADABLE; + } catch (error) { + // ENOENT (missing, or deleted between the stat and the read) is an + // unowned path; any other coded failure is an intact file we cannot + // read this moment. + if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { + return UNREADABLE; + } + return READ_ERROR; } let parsed: unknown; From 9f2dbc1bb846d81cf632bada08314028f1215514 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:18:51 +0800 Subject: [PATCH 7/8] fix(core): serialize session registry lifecycle --- .../cli/src/ui/startInteractiveUI.test.tsx | 70 +++++++++---------- packages/cli/src/ui/startInteractiveUI.tsx | 48 ++++--------- packages/core/src/config/config.test.ts | 68 ++++++++++++++++-- packages/core/src/config/config.ts | 67 ++++++++++++------ .../core/src/utils/atomicFileWrite.test.ts | 25 +++++++ packages/core/src/utils/atomicFileWrite.ts | 14 ++-- .../src/utils/runtimeStatus.config.test.ts | 4 +- 7 files changed, 195 insertions(+), 101 deletions(-) diff --git a/packages/cli/src/ui/startInteractiveUI.test.tsx b/packages/cli/src/ui/startInteractiveUI.test.tsx index 4b1bb9ac5ec..2ffe8318d43 100644 --- a/packages/cli/src/ui/startInteractiveUI.test.tsx +++ b/packages/cli/src/ui/startInteractiveUI.test.tsx @@ -16,7 +16,6 @@ import type { LoadedSettings } from '../config/settings.js'; import type { InitializationResult } from '../core/initializer.js'; const registerSession = vi.hoisted(() => vi.fn()); -const unregisterSession = vi.hoisted(() => vi.fn()); const registerCleanup = vi.hoisted(() => vi.fn()); vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { @@ -25,7 +24,6 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { return { ...actual, registerSession: (...args: unknown[]) => registerSession(...args), - unregisterSession: (...args: unknown[]) => unregisterSession(...args), }; }); @@ -53,20 +51,23 @@ vi.mock('../utils/earlyInputCapture.js', () => ({ const { startInteractiveUI } = await import('./startInteractiveUI.js'); function makeConfig(): Config & { - markSessionRegistered: ReturnType; + trackSessionRegistration: ReturnType; + unregisterSessionRegistry: ReturnType; } { + const trackSessionRegistration = vi.fn((registration: Promise) => { + void registration.catch(() => undefined); + }); return { getSessionId: () => 'session-123', getTargetDir: () => '/work/app', getScreenReader: () => false, getChatRecordingService: () => undefined, isTelemetryInitializationDeferred: () => false, - // Must exist: the production success path calls it, and a missing - // member would throw a TypeError that the catch under test swallows — - // leaving every assertion green while registration silently no-ops. - markSessionRegistered: vi.fn(), + trackSessionRegistration, + unregisterSessionRegistry: vi.fn().mockResolvedValue(undefined), } as unknown as Config & { - markSessionRegistered: ReturnType; + trackSessionRegistration: ReturnType; + unregisterSessionRegistry: ReturnType; }; } @@ -107,45 +108,44 @@ describe('startInteractiveUI session registration', () => { cwd: '/work/app', qwenVersion: '9.9.9', }); - // Arms the mid-session /clear and /cd registry patches on this - // Config; if this never fires, `ps` shows stale values until exit. - expect(config.markSessionRegistered).toHaveBeenCalledTimes(1); + expect(config.trackSessionRegistration).toHaveBeenCalledTimes(1); + await expect( + config.trackSessionRegistration.mock.calls[0]?.[0], + ).resolves.toBe(true); }); - it('arms the unregister cleanup only when registration succeeded', async () => { - // startInteractiveUI registers exactly one other cleanup (the UI - // teardown) unconditionally, so the count differs by exactly one - // depending on whether the unregister callback was armed. + it('arms teardown before serialized registry cleanup', async () => { registerSession.mockResolvedValue(true); - const successConfig = makeConfig(); - await start(successConfig); + const config = makeConfig(); + await start(config); + expect(registerCleanup).toHaveBeenCalledTimes(2); - expect(unregisterSession).not.toHaveBeenCalled(); - expect(successConfig.markSessionRegistered).toHaveBeenCalledTimes(1); - // Armed is not the contract — invoke the armed callback: a future - // edit emptying its body would pass the call-count pins while every - // session's record survives exit and `ps` lists it as running. const armUnregister = registerCleanup.mock - .calls[0]?.[0] as () => Promise; + .calls[1]?.[0] as () => Promise | void; await armUnregister(); - expect(unregisterSession).toHaveBeenCalledTimes(1); + expect(config.unregisterSessionRegistry).toHaveBeenCalledTimes(1); + }); - vi.clearAllMocks(); - registerSession.mockResolvedValue(false); - const refusedConfig = makeConfig(); - await start(refusedConfig); - expect(registerCleanup).toHaveBeenCalledTimes(1); - expect(refusedConfig.markSessionRegistered).not.toHaveBeenCalled(); + it('does not await a stalled registration before returning startup', async () => { + registerSession.mockReturnValue(new Promise(() => undefined)); + const config = makeConfig(); + + const result = await Promise.race([ + start(config).then(() => 'started'), + new Promise((resolve) => + setTimeout(() => resolve('timed-out'), 50), + ), + ]); + + expect(result).toBe('started'); + expect(registerCleanup).toHaveBeenCalledTimes(2); }); - it('swallows a registration rejection without aborting startup', async () => { - // A read-only home must not keep the TUI from launching; the - // registration is wrapped precisely so its failure cannot propagate. + it('tracks a registration rejection without aborting startup', async () => { registerSession.mockRejectedValue(new Error('read-only home')); const config = makeConfig(); await expect(start(config)).resolves.toBeUndefined(); - expect(registerCleanup).toHaveBeenCalledTimes(1); - expect(config.markSessionRegistered).not.toHaveBeenCalled(); + expect(config.trackSessionRegistration).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/cli/src/ui/startInteractiveUI.tsx b/packages/cli/src/ui/startInteractiveUI.tsx index 216cd75fafd..7e000daccc5 100644 --- a/packages/cli/src/ui/startInteractiveUI.tsx +++ b/packages/cli/src/ui/startInteractiveUI.tsx @@ -13,7 +13,6 @@ import { isDebugLogFileEnabled, registerSession, type Config, - unregisterSession, writeRuntimeStatus, } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../config/settings.js'; @@ -286,39 +285,6 @@ export async function startInteractiveUI( config.isTelemetryInitializationDeferred(), }); - // Announce this session in the session 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". - // - // Deliberately after render(): the write is an mkdir + chmod + fsync'd - // atomic write, and nothing on the first screen depends on it, so it - // belongs with the other post-first-paint work rather than in front of - // the user's first frame. - // - // Wrapped like every other startup side-effect in this function (the - // sidecar above, the dual-output bridge, the remote-input watcher): - // discovery is a convenience and must not be able to abort a session. - try { - if ( - await registerSession({ - sessionId: config.getSessionId(), - cwd: config.getTargetDir(), - qwenVersion: version, - }) - ) { - // Only arm cleanup for a record that exists; registration fails on - // a read-only home, and there is then nothing to unlink. - registerCleanup(() => unregisterSession()); - // Arms the mid-session registry patches (`/clear`, `/cd`) on this - // Config; without it the record would keep advertising the values - // the session started with. - config.markSessionRegistered(); - } - } catch (err) { - debugLogger.debug(`session registration skipped: ${String(err)}`); - } - // Periodic memory-pressure check for the interactive session. The interval // is unref'd (can't keep the loop alive on its own) and cleared on cleanup. const pressureMonitor = config.getMemoryPressureMonitor?.(); @@ -411,6 +377,20 @@ export async function startInteractiveUI( // Best-effort: a hint must never block or break exit. } }); + + // Announce this session only after the terminal teardown cleanup above is + // armed. Registration writes HOME and can stall independently of the + // project filesystem, so startup and terminal restoration must not await it. + // Config owns the ordering with /clear, /cd, and exit: transitions queued + // while registration is pending run after it, and unregister runs last. + config.trackSessionRegistration( + registerSession({ + sessionId: config.getSessionId(), + cwd: config.getTargetDir(), + qwenVersion: version, + }), + ); + registerCleanup(() => config.unregisterSessionRegistry()); } function setWindowTitle(settings: LoadedSettings, folderName?: string) { diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 6f32669c3c8..b22b06716fb 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -6327,7 +6327,7 @@ describe('Server Config (config.ts)', () => { it('relocateWorkingDirectory should refresh runtime status after moving session artifacts', async () => { const config = new Config(baseParams); config.markRuntimeStatusEnabled(); - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); const sessionId = config.getSessionId(); const newDir = path.resolve('/path/to/other'); const oldStorage = new Storage(config.getTargetDir()); @@ -6396,7 +6396,7 @@ describe('Server Config (config.ts)', () => { // state is reachable and the /cd patch must survive it. const config = new Config(baseParams); // No markRuntimeStatusEnabled(): models the failed sidecar write. - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); const sessionId = config.getSessionId(); const newDir = path.resolve('/path/to/other'); const chdirSpy = vi.spyOn(process, 'chdir').mockImplementation(() => { @@ -6434,11 +6434,11 @@ describe('Server Config (config.ts)', () => { // The opposite divergence: the sidecar write succeeded at startup // but registerSession returned false (foreign-identity refusal, // unwritable global dir), so only the sidecar gate is armed. A gate - // regressed to `if (!this.sessionRegistered) return;` would silently + // regressed to `if (!this.sessionRegistryActive) return;` would silently // stop refreshing runtime.json on /cd for these sessions. const config = new Config(baseParams); config.markRuntimeStatusEnabled(); - // No markSessionRegistered(): models the failed registration. + // No trackSessionRegistration(): models the failed registration. const sessionId = config.getSessionId(); const newDir = path.resolve('/path/to/other'); const newStorage = new Storage(newDir); @@ -6483,7 +6483,7 @@ describe('Server Config (config.ts)', () => { // until process exit. const config = new Config(baseParams); config.markRuntimeStatusEnabled(); - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); const writeRuntimeStatusSpy = vi .spyOn(runtimeStatus, 'writeRuntimeStatus') .mockRejectedValue(new Error('read-only project fs')); @@ -6504,13 +6504,69 @@ describe('Server Config (config.ts)', () => { patchSessionRecordSpy.mockRestore(); }); + it('serializes pending registration, transitions, and unregister', async () => { + const config = new Config(baseParams); + let finishRegistration!: (registered: boolean) => void; + const registration = new Promise((resolve) => { + finishRegistration = resolve; + }); + let finishPatch!: () => void; + const patchSessionRecordSpy = vi + .spyOn(sessionRegistry, 'patchSessionRecord') + .mockImplementation( + () => + new Promise((resolve) => { + finishPatch = resolve; + }), + ); + const unregisterSessionSpy = vi + .spyOn(sessionRegistry, 'unregisterSession') + .mockResolvedValue(undefined); + + config.trackSessionRegistration(registration); + const newSessionId = config.startNewSession('replacement-session'); + const cleanup = config.unregisterSessionRegistry(); + + expect(patchSessionRecordSpy).not.toHaveBeenCalled(); + expect(unregisterSessionSpy).not.toHaveBeenCalled(); + + finishRegistration(true); + await vi.waitFor(() => { + expect(patchSessionRecordSpy).toHaveBeenCalledWith({ + sessionId: newSessionId, + cwd: config.getTargetDir(), + }); + }); + expect(unregisterSessionSpy).not.toHaveBeenCalled(); + + finishPatch(); + await cleanup; + expect(unregisterSessionSpy).toHaveBeenCalledTimes(1); + + patchSessionRecordSpy.mockRestore(); + unregisterSessionSpy.mockRestore(); + }); + + it('does not unregister when initial registration was refused', async () => { + const config = new Config(baseParams); + const unregisterSessionSpy = vi + .spyOn(sessionRegistry, 'unregisterSession') + .mockResolvedValue(undefined); + + config.trackSessionRegistration(Promise.resolve(false)); + await config.unregisterSessionRegistry(); + + expect(unregisterSessionSpy).not.toHaveBeenCalled(); + unregisterSessionSpy.mockRestore(); + }); + it('relocateWorkingDirectory patches the registry even when the sidecar write rejects', async () => { // The /cd-side mirror of the /clear pin: a rejecting sidecar write // must not skip the directory patch, and its rejection must not // surface through relocateWorkingDirectory either. const config = new Config(baseParams); config.markRuntimeStatusEnabled(); - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); const sessionId = config.getSessionId(); const newDir = path.resolve('/path/to/other'); const chdirSpy = vi.spyOn(process, 'chdir').mockImplementation(() => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 63be01c807c..53f5e5d47ca 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -221,6 +221,7 @@ import { import { deriveSessionName, patchSessionRecord, + unregisterSession, } from '../services/session-registry.js'; import { SessionService, @@ -2020,6 +2021,7 @@ export class Config { private readonly cliVersion?: string; private runtimeStatusEnabled = false; + private sessionRegistryActive = false; private sessionRegistered = false; private readonly experimentalZedIntegration: boolean = false; private readonly sessionWriterLeaseEnabled: boolean = false; @@ -4057,22 +4059,19 @@ export class Config { }); }); } - if (this.sessionRegistered) { + if (this.sessionRegistryActive) { const workDir = this.targetDir; const newSessionId = this.sessionId; // Keep the session registry in step: this PID's record would // otherwise point discovery at the previous transcript. Keyed by // PID, so a swap is a patch rather than a delete-and-rewrite. // - // Gated on registration success rather than the sidecar's - // `runtimeStatusEnabled`: the failure domains are independent - // (project-local `chats/` dir vs the global dir), and riding the - // sidecar gate would skip every patch for a whole session when - // the sidecar write failed at startup while registration - // succeeded — `ps` would keep advertising the pre-/clear session - // id and the pre-/cd directory until exit. The inverse - // divergence is safe on its own: `patchSessionRecord` no-ops on - // a missing record. + // Gated on the registry lifecycle rather than the sidecar's + // `runtimeStatusEnabled`: the failure domains are independent. + // When registration is still pending, this patch queues behind it; + // when registration fails, `patchSessionRecord` no-ops on the + // missing record. Either way a sidecar failure cannot leave `ps` + // advertising the pre-/clear session id until exit. // // `name` is deliberately not patched: it is the handle a user // just read out of `qwen sessions ps`, and re-deriving it here @@ -4101,14 +4100,41 @@ export class Config { } /** - * Marks this Config as registered in the session registry. Call once - * after `registerSession` returned true (from the interactive UI - * bootstrap). Gates the mid-session registry patches - * (startNewSession, refreshCurrentRuntimeStatus), so they ride the - * registry's own success rather than the sidecar's. + * Serializes initial registration with mid-session patches and cleanup. + * The registration promise is deliberately not awaited by UI startup. */ - markSessionRegistered(): void { - this.sessionRegistered = true; + trackSessionRegistration(registration: Promise): void { + this.sessionRegistryActive = true; + this.sessionRegistryWrite = this.sessionRegistryWrite + .catch(() => { + // Keep registration independent from an earlier best-effort write. + }) + .then(async () => { + this.sessionRegistered = await registration; + if (!this.sessionRegistered) this.sessionRegistryActive = false; + }) + .catch(() => { + this.sessionRegistered = false; + this.sessionRegistryActive = false; + }); + } + + /** Drain queued patches, then remove this process's registered record. */ + async unregisterSessionRegistry(): Promise { + this.sessionRegistryActive = false; + this.sessionRegistryWrite = this.sessionRegistryWrite + .catch(() => { + // Keep cleanup alive after a best-effort patch failure. + }) + .then(async () => { + if (!this.sessionRegistered) return; + this.sessionRegistered = false; + await unregisterSession(); + }) + .catch(() => { + // ignored: registry cleanup must not disrupt process teardown. + }); + await this.sessionRegistryWrite; } private queueRuntimeStatusWrite(write: () => Promise): void { @@ -4126,7 +4152,7 @@ export class Config { * Queue a session-registry patch on its own serial chain. * * The chain is separate from `runtimeStatusWrite` and is deliberately - * never awaited by `flushRuntimeStatusWrites`: + * never awaited by session-transition paths: * * - A sidecar write that rejects or hangs must not skip or block the * patch — the two target independent failure domains (project-local @@ -4135,7 +4161,8 @@ export class Config { * sidecar chain: awaiting the patch there would hang `/cd` whenever * HOME stalls while the project directory is healthy. Registry * patches are best-effort by contract — `ps` settles a tick after - * the transition returns — so nothing ever awaits this chain. + * the transition returns. Process cleanup drains the chain before + * unregistering so a late patch cannot recreate the deleted record. * * Patches still serialize among themselves so back-to-back /clear and * /cd transitions cannot interleave their read-modify-write. @@ -4173,7 +4200,7 @@ export class Config { }); }); } - if (this.sessionRegistered) { + if (this.sessionRegistryActive) { this.queueSessionRegistryWrite(async () => { // The registry's DIRECTORY column is how a user tells two live // sessions apart, so a mid-session directory switch has to reach diff --git a/packages/core/src/utils/atomicFileWrite.test.ts b/packages/core/src/utils/atomicFileWrite.test.ts index c073b0a15a1..ee5becd5028 100644 --- a/packages/core/src/utils/atomicFileWrite.test.ts +++ b/packages/core/src/utils/atomicFileWrite.test.ts @@ -1045,6 +1045,31 @@ describe('noFollow option — symlink protection', () => { expect(await fs.readFile(real, 'utf-8')).toBe('ORIGINAL'); }); + it.skipIf( + process.platform === 'win32' || typeof process.geteuid !== 'function', + )( + 'atomicWriteFile: noFollow still replaces a symlink when ownership differs', + async () => { + const real = path.join(tmpDir, 'owned-by-another-user.txt'); + const link = path.join(tmpDir, 'record.json'); + await fs.writeFile(real, 'ORIGINAL'); + await fs.symlink(real, link); + + const realGeteuid = process.geteuid!; + const targetStat = await fs.stat(real); + process.geteuid = () => targetStat.uid + 1; + try { + await atomicWriteFile(link, 'NEW', { noFollow: true, mode: 0o600 }); + } finally { + process.geteuid = realGeteuid; + } + + expect((await fs.lstat(link)).isSymbolicLink()).toBe(false); + expect(await fs.readFile(link, 'utf-8')).toBe('NEW'); + expect(await fs.readFile(real, 'utf-8')).toBe('ORIGINAL'); + }, + ); + it('atomicWriteFileSync: noFollow replaces a pre-placed symlink instead of writing through it', () => { const real = path.join(tmpDir, 'real.txt'); const link = path.join(tmpDir, 'link.txt'); diff --git a/packages/core/src/utils/atomicFileWrite.ts b/packages/core/src/utils/atomicFileWrite.ts index b6961cca6fb..d8fd4f9f4fb 100644 --- a/packages/core/src/utils/atomicFileWrite.ts +++ b/packages/core/src/utils/atomicFileWrite.ts @@ -180,12 +180,15 @@ export async function atomicWriteFile( throw annotateWriteError(err, filePath); }); - // Stat the target to preserve existing permissions and detect + // Inspect the target to preserve existing permissions and detect // ownership-changing renames (see the ownership-preservation note in - // the function doc). + // the function doc). noFollow must inspect the directory entry itself: + // following a symlink here can select the in-place write fallback below. 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; @@ -198,7 +201,10 @@ export async function atomicWriteFile( let existingMode: number | undefined; if (!options?.forceMode || options?.mode === undefined) { existingMode = - existingStat !== undefined ? existingStat.mode & 0o7777 : undefined; + existingStat !== undefined && + (!options?.noFollow || existingStat.isFile()) + ? existingStat.mode & 0o7777 + : undefined; } const desiredMode = existingMode ?? options?.mode; diff --git a/packages/core/src/utils/runtimeStatus.config.test.ts b/packages/core/src/utils/runtimeStatus.config.test.ts index 585c61cbe7b..c4974f62aac 100644 --- a/packages/core/src/utils/runtimeStatus.config.test.ts +++ b/packages/core/src/utils/runtimeStatus.config.test.ts @@ -190,7 +190,7 @@ describe('Config.startNewSession session-registry patch', () => { qwenVersion: '0.0.0-test', }), ).toBe(true); - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); const [before] = await listLiveSessions(); @@ -228,7 +228,7 @@ describe('Config.startNewSession session-registry patch', () => { qwenVersion: '0.0.0-test', }), ).toBe(true); - config.markSessionRegistered(); + config.trackSessionRegistration(Promise.resolve(true)); config.startNewSession(sessionB); From fd768c9058b0b4df8efb4274a52a61649eeec97b Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:58:43 +0800 Subject: [PATCH 8/8] test(cli): update session registry mocks --- packages/cli/src/gemini.test.tsx | 42 +++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 09f7820792d..4951c4c9801 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -51,6 +51,11 @@ const mockStopNonInteractiveOpenAILogHousekeeping = vi.hoisted(() => ); const mockUpdateBeforeRelaunch = vi.hoisted(() => vi.fn()); const mockGetInstallationInfo = vi.hoisted(() => vi.fn()); +const mockRegisterSession = vi.hoisted( + () => + (..._args: unknown[]) => + Promise.resolve(true), +); const lspConfigWatcherMock = vi.hoisted(() => ({ instances: [] as Array<{ listener?: (event: unknown) => void | Promise; @@ -59,6 +64,14 @@ const lspConfigWatcherMock = vi.hoisted(() => ({ }>, })); +const sessionRegistryConfigStub = { + getTargetDir: () => '/tmp/project', + trackSessionRegistration: (registration: Promise) => { + void registration.catch(() => undefined); + }, + unregisterSessionRegistry: async () => {}, +}; + describe('gemini import boundary', () => { it('does not statically import ACP or noninteractive auth branches', () => { const source = readFileSync('src/gemini.tsx', 'utf8'); @@ -102,6 +115,15 @@ vi.mock('./config/settings.js', async (importOriginal) => { }; }); +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + registerSession: (...args: unknown[]) => mockRegisterSession(...args), + }; +}); + vi.mock('./config/config.js', () => ({ loadCliConfig: vi.fn().mockResolvedValue({ getSandbox: vi.fn(() => false), @@ -1684,6 +1706,7 @@ describe('gemini.tsx main function kitty protocol', () => { geminiMdFileCount: 0, }); vi.mocked(loadCliConfig).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getQuestion: () => '', getSandbox: () => false, @@ -1809,6 +1832,7 @@ describe('gemini.tsx main function kitty protocol', () => { geminiMdFileCount: 0, }); vi.mocked(loadCliConfig).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getQuestion: () => 'hello from prompt-interactive', getSandbox: () => false, @@ -1932,6 +1956,7 @@ describe('gemini.tsx main function kitty protocol', () => { geminiMdFileCount: 0, }); vi.mocked(loadCliConfig).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getQuestion: () => '', getInputFile: () => '/tmp/qwen-input.jsonl', @@ -2054,6 +2079,7 @@ describe('gemini.tsx main function kitty protocol', () => { geminiMdFileCount: 0, }); vi.mocked(loadCliConfig).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getQuestion: () => '', getSandbox: () => false, @@ -2194,6 +2220,7 @@ describe('gemini.tsx main function kitty protocol', () => { vi.mocked( loadCliConfig as (typeof import('./config/config.js'))['loadCliConfig'], ).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getQuestion: () => '', getSandbox: () => false, @@ -2511,6 +2538,7 @@ describe('gemini.tsx main function kitty protocol', () => { }) as unknown as typeof process.exit); vi.mocked(loadCliConfig).mockResolvedValue({ + ...sessionRegistryConfigStub, isInteractive: () => true, getJsonSchema: () => ({ type: 'object' }), getQuestion: () => '', @@ -2607,6 +2635,8 @@ describe('validateDnsResolutionOrder', () => { describe('startInteractiveUI', () => { // Mock dependencies const mockConfig = { + ...sessionRegistryConfigStub, + getSessionId: () => 'test-session-id', getProjectRoot: () => '/root', getScreenReader: () => false, isTelemetryInitializationDeferred: () => true, @@ -2925,7 +2955,7 @@ describe('startInteractiveUI', () => { // Verify all startup tasks were called 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]; @@ -3027,7 +3057,7 @@ describe('startInteractiveUI', () => { ); const { registerCleanup } = await import('./utils/cleanup.js'); - const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0]?.[0] as | (() => Promise | void) | undefined; expect(cleanupFn).toBeTypeOf('function'); @@ -3061,7 +3091,7 @@ describe('startInteractiveUI', () => { ); const { registerCleanup } = await import('./utils/cleanup.js'); - const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0]?.[0] as | (() => Promise | void) | undefined; expect(cleanupFn).toBeTypeOf('function'); @@ -3094,7 +3124,7 @@ describe('startInteractiveUI', () => { ); const { registerCleanup } = await import('./utils/cleanup.js'); - const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0]?.[0] as | (() => Promise | void) | undefined; await cleanupFn?.(); @@ -3128,7 +3158,7 @@ describe('startInteractiveUI', () => { ); const { registerCleanup } = await import('./utils/cleanup.js'); - const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0]?.[0] as | (() => Promise | void) | undefined; expect(cleanupFn).toBeTypeOf('function'); @@ -3350,7 +3380,7 @@ describe('startInteractiveUI', () => { expect(beforeCleanup).toBeGreaterThan(0); const { registerCleanup } = await import('./utils/cleanup.js'); - const cleanupFn = vi.mocked(registerCleanup).mock.calls.at(-1)?.[0] as + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0]?.[0] as | (() => Promise | void) | undefined; expect(cleanupFn).toBeTypeOf('function');