diff --git a/docs/e2e-tests/worktree-phase-d.md b/docs/e2e-tests/worktree-phase-d.md index 4416077fc10..4952ee0678d 100644 --- a/docs/e2e-tests/worktree-phase-d.md +++ b/docs/e2e-tests/worktree-phase-d.md @@ -168,7 +168,7 @@ or "git init". ### B1: sidecar written with all six fields ```bash -SESSION_ID=$(uuidgen) +SESSION_ID=$(uuidgen | tr '[:upper:]' '[:lower:]') $QWEN --worktree b1-test --session-id "$SESSION_ID" "say hi" \ --approval-mode yolo --output-format json 2>/dev/null > /tmp/b1.out @@ -220,7 +220,7 @@ is inside the worktree. ```bash # Run 1: create a session with worktree "first" -SESSION_ID=$(uuidgen) +SESSION_ID=$(uuidgen | tr '[:upper:]' '[:lower:]') $QWEN --worktree first --session-id "$SESSION_ID" "say hi" \ --approval-mode yolo --output-format json 2>/dev/null > /tmp/c1-run1.out @@ -246,7 +246,7 @@ ls -d "$TEST_DIR/.qwen/worktrees/"* ### C2: stale sidecar (manually deleted dir) + `--worktree` → fresh worktree ```bash -SESSION_ID=$(uuidgen) +SESSION_ID=$(uuidgen | tr '[:upper:]' '[:lower:]') $QWEN --worktree c2 --session-id "$SESSION_ID" "say hi" \ --approval-mode yolo --output-format json 2>/dev/null > /tmp/c2-run1.out @@ -312,7 +312,7 @@ tmux kill-session -t d2 ### D3: Dialog → Remove → worktree + branch + sidecar all gone ```bash -SESSION_ID=$(uuidgen) +SESSION_ID=$(uuidgen | tr '[:upper:]' '[:lower:]') tmux new-session -d -s d3 -x 200 -y 50 \ "cd $TEST_DIR && $QWEN --worktree d3-test --session-id $SESSION_ID --approval-mode yolo" sleep 3 @@ -541,7 +541,7 @@ readlink "$TEST_DIR/.qwen/worktrees/pr-4174/node_modules" > the dry-run, or skip G1 entirely in baseline mode. ```bash -SESSION_ID=$(uuidgen) +SESSION_ID=$(uuidgen | tr '[:upper:]' '[:lower:]') tmux new-session -d -s g1 -x 200 -y 50 \ "cd $TEST_DIR && $QWEN --worktree g1-test --session-id $SESSION_ID --approval-mode yolo 2>&1 | tee /tmp/g1-stderr.out" sleep 3 diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index 24f512ce76b..bfe0f385af0 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -481,7 +481,7 @@ describe('qwen serve — transcript paging route', () => { expect(missing.status).toBe(404); }); - it('maps archived, conflicting, and unavailable transcript snapshots to 409', async () => { + it('reads exact conflicts from active and maps archived/unavailable snapshots to 409', async () => { const archivedId = '99999999-aaaa-bbbb-cccc-444444444444'; const archivedRecord = chatRecord( archivedId, @@ -497,19 +497,33 @@ describe('qwen serve — transcript paging route', () => { }); const conflictId = '99999999-aaaa-bbbb-cccc-555555555555'; - const conflictRecord = chatRecord( + const activeConflictRecord = chatRecord( conflictId, 'u1', null, - 'conflicting transcript', + 'active conflicting transcript', ); - writePersistedTranscript(conflictId, [conflictRecord]); - writePersistedTranscript(conflictId, [conflictRecord], 'archived'); + const archivedConflictRecord = chatRecord( + conflictId, + 'u1', + null, + 'archived conflicting transcript', + ); + writePersistedTranscript(conflictId, [activeConflictRecord]); + writePersistedTranscript(conflictId, [archivedConflictRecord], 'archived'); const conflict = await getTranscript(conflictId); - expect(conflict.status).toBe(409); - await expect(conflict.json()).resolves.toMatchObject({ - code: 'session_conflict', + expect(conflict.status).toBe(200); + const conflictBody = await conflict.json(); + expect(conflictBody).toMatchObject({ + sessionId: conflictId, + hasMore: false, }); + expect(JSON.stringify(conflictBody)).toContain( + 'active conflicting transcript', + ); + expect(JSON.stringify(conflictBody)).not.toContain( + 'archived conflicting transcript', + ); const unavailable = await getTranscript( '99999999-aaaa-bbbb-cccc-666666666666', diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 3276722c7f7..a6af339c841 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -17056,6 +17056,36 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { }, ); + it.each(['load', 'resume'] as const)( + '%s restores an exact persisted session from active when both states exist', + async (action) => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + const innerConfig = bindRestoreMocks({ + sessionExists: false, + resolverError: new SessionIdCaseConflictError(sessionId, sessionId), + }); + innerConfig.getSessionId.mockReturnValue(sessionId); + const { agent, agentPromise } = await spawnAgent(); + + try { + const request = { cwd: '/tmp', sessionId, mcpServers: [] }; + if (action === 'load') { + await agent.loadSession(request); + } else { + await agent.unstable_resumeSession(request); + } + + const argv = vi.mocked(loadCliConfig).mock.calls.at(-1)?.[1] as + | CliArgs + | undefined; + expect(argv?.resume).toBe(sessionId); + } finally { + mockConnectionState.resolve(); + await agentPromise; + } + }, + ); + it.each(['load', 'resume'] as const)( '%s surfaces the both-states conflict message as session_conflict', async (action) => { diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index b9052dadb1f..314d387f5a4 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -870,6 +870,30 @@ const RESUME_RESTORE_OPTIONS: SelectiveSessionRestoreOptions = { replay: { kind: 'none' }, }; +async function resolvePersistedSessionIdForRestore( + sessionService: SessionService, + sessionId: string, +): Promise { + try { + return await sessionService.findSessionIdIgnoringCase(sessionId); + } catch (error) { + if ( + error instanceof SessionIdCaseConflictError && + error.reason === 'case_conflict' && + error.candidateSessionId === sessionId + ) { + return sessionId; + } + if (error instanceof SessionIdCaseConflictError) { + throw RequestError.internalError( + { errorKind: 'session_conflict', sessionId }, + error.message, + ); + } + throw error; + } +} + function mapSessionRestoreRequestError( error: unknown, sessionId: string, @@ -5411,21 +5435,7 @@ class QwenAgent implements Agent { const persistedSessionId = await profiler.time('existence_check', () => this.runWithPinnedRuntimeBaseDir(settings, params.cwd, async () => { const sessionService = new SessionService(params.cwd); - try { - return await sessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if (error instanceof SessionIdCaseConflictError) { - // Parity with the daemon surfaces (toRpcError / REST 409): - // persisted-storage conflicts use `session_conflict`; - // `session_id_conflict` is reserved for live-id admission - // occupancy. - throw RequestError.internalError( - { errorKind: 'session_conflict', sessionId }, - error.message, - ); - } - throw error; - } + return resolvePersistedSessionIdForRestore(sessionService, sessionId); }), ); if (!persistedSessionId) { @@ -5740,21 +5750,7 @@ class QwenAgent implements Agent { const persistedSessionId = await profiler.time('existence_check', () => this.runWithPinnedRuntimeBaseDir(settings, params.cwd, async () => { const sessionService = new SessionService(params.cwd); - try { - return await sessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if (error instanceof SessionIdCaseConflictError) { - // Parity with the daemon surfaces (toRpcError / REST 409): - // persisted-storage conflicts use `session_conflict`; - // `session_id_conflict` is reserved for live-id admission - // occupancy. - throw RequestError.internalError( - { errorKind: 'session_conflict', sessionId }, - error.message, - ); - } - throw error; - } + return resolvePersistedSessionIdForRestore(sessionService, sessionId); }), ); if (!persistedSessionId) { diff --git a/packages/cli/src/runtime/live-session-source.test.ts b/packages/cli/src/runtime/live-session-source.test.ts index b27e0dfa77f..101ecca3db2 100644 --- a/packages/cli/src/runtime/live-session-source.test.ts +++ b/packages/cli/src/runtime/live-session-source.test.ts @@ -37,16 +37,18 @@ const STANDALONE_CHILD_OF_LIVE_ID = '550e8400-e29b-41d4-a716-446655440012'; const STANDALONE_CYCLE_A_ID = '550e8400-e29b-41d4-a716-446655440013'; const STANDALONE_CYCLE_B_ID = '550e8400-e29b-41d4-a716-446655440014'; const STANDALONE_CHILD_OF_LEGACY_ID = '550e8400-e29b-41d4-a716-446655440015'; +const NIL_PARENT_STANDALONE_ID = '550e8400-e29b-41d4-a716-446655440016'; +const V7_PARENT_ID = '01890a5d-ac96-774b-bcce-b302099a8057'; +const V7_CHILD_ID = '550e8400-e29b-41d4-a716-446655440017'; +const AGENT_PARENT_ID = `${LIVE_ID}-agent-worker`; +const AGENT_CHILD_ID = '550e8400-e29b-41d4-a716-446655440018'; function createStore( records: ReadonlyMap, ): ConversationSessionMetadataStore { return { - async getSessionLocation(sessionId) { - return records.has(sessionId) ? 'active' : undefined; - }, - async readCreationMetadataIfReadable(sessionId) { - return records.get(sessionId) ?? {}; + async readCreationMetadataIfReadable(sessionId, state) { + return state === 'active' ? records.get(sessionId) : undefined; }, }; } @@ -81,8 +83,8 @@ describe('conversation session source classification', () => { SELF_STANDALONE_ID, { sourceType: 'standalone', parentSessionId: SELF_STANDALONE_ID }, ], - // Without the `!isValidSessionId` conjunct the explicit-standalone - // shortcut below the parent guard would accept this record. + // A parent spelling the store cannot resolve leaves an explicit + // standalone child self-describing, like a deleted parent would. [ MALFORMED_PARENT_STANDALONE_ID, { sourceType: 'standalone', parentSessionId: 'not-a-session-id' }, @@ -115,6 +117,22 @@ describe('conversation session source classification', () => { STANDALONE_CHILD_OF_LEGACY_ID, { sourceType: 'standalone', parentSessionId: LEGACY_ID }, ], + // Parents older builds could persist: nil, v7, and agent-suffixed ids are + // not RFC-4122 v1-v5, but the store resolves them, so lineage must too. + [ + NIL_PARENT_STANDALONE_ID, + { + sourceType: 'standalone', + parentSessionId: '00000000-0000-0000-0000-000000000000', + }, + ], + [V7_PARENT_ID, { sourceType: 'default' }], + [V7_CHILD_ID, { parentSessionId: V7_PARENT_ID }], + [ + AGENT_PARENT_ID, + { sourceType: 'default', sourceId: 'realtime_voice:call-agent' }, + ], + [AGENT_CHILD_ID, { parentSessionId: AGENT_PARENT_ID }], ]); const store = createStore(records); @@ -157,6 +175,10 @@ describe('conversation session source classification', () => { [EXPLICIT_CHILD_ID, 'standalone', 'explicit'], [LEGACY_CHILD_OF_EXPLICIT_ID, 'standalone', 'legacy'], [STANDALONE_CHILD_OF_LEGACY_ID, 'standalone', 'explicit'], + [MALFORMED_PARENT_STANDALONE_ID, 'standalone', 'explicit'], + [NIL_PARENT_STANDALONE_ID, 'standalone', 'explicit'], + [V7_CHILD_ID, 'standalone', 'legacy'], + [AGENT_CHILD_ID, 'live', 'legacy'], ] as const)( 'classifies %s as %s %s', async (sessionId, kind, persistence) => { @@ -178,6 +200,8 @@ describe('conversation session source classification', () => { STANDALONE_CHILD_OF_LEGACY_ID, { kind: 'standalone', persistence: 'legacy' }, ], + [V7_CHILD_ID, { kind: 'standalone', persistence: 'legacy' }], + [AGENT_CHILD_ID, { kind: 'live', persistence: 'explicit' }], ] as const)( 'reports the proven parent lineage for %s', async (sessionId, lineage) => { @@ -187,17 +211,21 @@ describe('conversation session source classification', () => { }, ); - it.each([EXPLICIT_ID, LEGACY_ID, LIVE_ID, EXPLICIT_CHILD_ID])( - 'leaves the parent lineage unproven for %s', - async (sessionId) => { - // Top-level sessions have no parent, and an explicit standalone child - // whose parent was archived away or deleted cannot produce one. Callers - // that need proven lineage must reject on this rather than on `kind`. - const result = await readLoadableConversationSession(sessionId, store); - expect(result).toBeDefined(); - expect(result?.parentSource).toBeUndefined(); - }, - ); + it.each([ + EXPLICIT_ID, + LEGACY_ID, + LIVE_ID, + EXPLICIT_CHILD_ID, + MALFORMED_PARENT_STANDALONE_ID, + NIL_PARENT_STANDALONE_ID, + ])('leaves the parent lineage unproven for %s', async (sessionId) => { + // Top-level sessions have no parent, and an explicit standalone child + // whose parent was archived away or deleted cannot produce one. Callers + // that need proven lineage must reject on this rather than on `kind`. + const result = await readLoadableConversationSession(sessionId, store); + expect(result).toBeDefined(); + expect(result?.parentSource).toBeUndefined(); + }); it.each([ ORPHAN_ID, @@ -206,7 +234,6 @@ describe('conversation session source classification', () => { MALFORMED_LIVE_ID, SELF_ID, SELF_STANDALONE_ID, - MALFORMED_PARENT_STANDALONE_ID, ATTRIBUTED_STANDALONE_CHILD_ID, CYCLE_A_ID, // Readable parent contradicting depth-1 standalone lineage. @@ -240,12 +267,23 @@ describe('conversation session source classification', () => { ).resolves.toBeUndefined(); }); - it('rejects missing and conflicting transcripts without reading metadata', async () => { - let reads = 0; - const unavailableStore: ConversationSessionMetadataStore = { - async getSessionLocation(sessionId) { - return sessionId === LEGACY_ID ? 'conflict' : undefined; + it('resolves a conflicted transcript from its active copy', async () => { + // Both state copies exist (a crash inside archiveSessions leaves that + // behind): reads use the active copy, matching the CLI resume path. + const conflictedStore: ConversationSessionMetadataStore = { + async readCreationMetadataIfReadable(_sessionId, state) { + return state === 'active' ? { sourceType: 'default' } : {}; }, + }; + + await expect( + readLoadableConversationSession(LEGACY_ID, conflictedStore), + ).resolves.toMatchObject({ kind: 'standalone', persistence: 'legacy' }); + }); + + it('refuses path-unsafe ids without touching the store', async () => { + let reads = 0; + const store: ConversationSessionMetadataStore = { async readCreationMetadataIfReadable() { reads++; return {}; @@ -253,37 +291,45 @@ describe('conversation session source classification', () => { }; await expect( - readLoadableConversationSession(LEGACY_ID, unavailableStore), - ).resolves.toBeUndefined(); - await expect( - readLoadableConversationSession(EXPLICIT_ID, unavailableStore), + readLoadableConversationSession('../escape', store), ).resolves.toBeUndefined(); expect(reads).toBe(0); }); - it('rejects a transcript that disappears while its metadata is read', async () => { - let locationReads = 0; - const disappearingStore: ConversationSessionMetadataStore = { - async getSessionLocation() { - locationReads++; - return locationReads === 1 ? 'active' : undefined; - }, - async readCreationMetadataIfReadable() { - return {}; + it.each(['CON', 'nul', 'AUX.txt', 'PRN.', 'COM1', 'lpt9.jsonl'])( + 'refuses Windows device transcript name %s without touching the store', + async (sessionId) => { + let reads = 0; + const deviceStore: ConversationSessionMetadataStore = { + async readCreationMetadataIfReadable() { + reads++; + return {}; + }, + }; + + await expect( + readLoadableConversationSession(sessionId, deviceStore), + ).resolves.toBeUndefined(); + expect(reads).toBe(0); + }, + ); + + it('resolves a transcript that a concurrent archive moves mid-read', async () => { + const movingStore: ConversationSessionMetadataStore = { + async readCreationMetadataIfReadable(_sessionId, state) { + // The active copy vanished before the read; the archived copy has it. + return state === 'archived' ? { sourceType: 'default' } : undefined; }, }; await expect( - readLoadableConversationSession(LEGACY_ID, disappearingStore), - ).resolves.toBeUndefined(); + readLoadableConversationSession(LEGACY_ID, movingStore), + ).resolves.toMatchObject({ kind: 'standalone', persistence: 'legacy' }); }); it('rejects a transcript whose creation metadata is unreadable', async () => { const states: Array<'active' | 'archived'> = []; const unreadableStore: ConversationSessionMetadataStore = { - async getSessionLocation() { - return 'active'; - }, async readCreationMetadataIfReadable(_sessionId, state) { states.push(state); return undefined; @@ -296,24 +342,21 @@ describe('conversation session source classification', () => { await expect( readLoadableLiveConversationMetadata(LEGACY_ID, unreadableStore), ).resolves.toBeUndefined(); - expect(states).toEqual(['active', 'active']); + expect(states).toEqual(['active', 'archived', 'active', 'archived']); }); it('reads archived transcripts with the archived state', async () => { const states: Array<'active' | 'archived'> = []; const archivedStore: ConversationSessionMetadataStore = { - async getSessionLocation(sessionId) { - return records.has(sessionId) ? 'archived' : undefined; - }, async readCreationMetadataIfReadable(sessionId, state) { states.push(state); - return records.get(sessionId); + return state === 'archived' ? records.get(sessionId) : undefined; }, }; await expect( readLoadableConversationSession(LEGACY_ID, archivedStore), ).resolves.toMatchObject({ kind: 'standalone', persistence: 'legacy' }); - expect(states).toEqual(['archived']); + expect(states).toEqual(['active', 'archived']); }); }); diff --git a/packages/cli/src/runtime/live-session-source.ts b/packages/cli/src/runtime/live-session-source.ts index 28595f799ed..372201d8b8f 100644 --- a/packages/cli/src/runtime/live-session-source.ts +++ b/packages/cli/src/runtime/live-session-source.ts @@ -4,10 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - isValidSessionId, - normalizeSessionIdForLookup, -} from '../config/session-id.js'; +import { normalizeSessionIdForLookup } from '../config/session-id.js'; export const LIVE_SESSION_SOURCE_PREFIX = 'realtime_voice:'; export const STANDALONE_SESSION_SOURCE_TYPE = 'standalone'; @@ -19,9 +16,6 @@ export interface LiveSessionCreationMetadata { } export interface ConversationSessionMetadataStore { - getSessionLocation( - sessionId: string, - ): Promise<'active' | 'archived' | 'conflict' | undefined>; readCreationMetadataIfReadable( sessionId: string, state: 'active' | 'archived', @@ -108,19 +102,33 @@ export function classifyTopLevelConversationSource( return undefined; } +// Any single-path-segment name the transcript store could hold. Deliberately +// wider than the storage enumeration pattern: parent ids written by older +// builds (nil/v6/v7 UUIDs, agent-suffixed ids) must stay resolvable, while +// path separators can never reach the joined transcript path. +const SAFE_TRANSCRIPT_NAME_PATTERN = /^[A-Za-z0-9._-]{1,128}$/; +const WINDOWS_DEVICE_NAME_PATTERN = + /^(?:con|prn|aux|nul|com[1-9]|lpt[1-9])(?:\.|$)/i; + async function readExistingMetadata( sessionId: string, store: ConversationSessionMetadataStore, ): Promise { - const location = await store.getSessionLocation(sessionId); - if (location !== 'active' && location !== 'archived') return undefined; - const metadata = await store.readCreationMetadataIfReadable( - sessionId, - location, + if ( + !SAFE_TRANSCRIPT_NAME_PATTERN.test(sessionId) || + WINDOWS_DEVICE_NAME_PATTERN.test(sessionId) + ) { + return undefined; + } + // Creation metadata is immutable, so one tolerant read per location + // decides. Probing the location around the read would only turn a + // concurrent archive move into a spurious "not found": a session moved + // before the active read is found by the archived read, and a session + // moved after it was already read correctly. + return ( + (await store.readCreationMetadataIfReadable(sessionId, 'active')) ?? + (await store.readCreationMetadataIfReadable(sessionId, 'archived')) ); - if (!metadata) return undefined; - const confirmedLocation = await store.getSessionLocation(sessionId); - return confirmedLocation === location ? metadata : undefined; } export async function readLoadableConversationSession( @@ -134,9 +142,10 @@ export async function readLoadableConversationSession( if (topLevel) return topLevel; const parentSessionId = metadata.parentSessionId; + // No shape validation beyond the reader's own path-safety gate: the storage + // layer decides what resolves, so ids written by older builds keep working. if ( parentSessionId === undefined || - !isValidSessionId(parentSessionId) || normalizeSessionIdForLookup(parentSessionId) === normalizeSessionIdForLookup(sessionId) ) { diff --git a/packages/cli/src/serve/acp-http/dispatch.ts b/packages/cli/src/serve/acp-http/dispatch.ts index 0f669bd9b1b..cd5b4759034 100644 --- a/packages/cli/src/serve/acp-http/dispatch.ts +++ b/packages/cli/src/serve/acp-http/dispatch.ts @@ -12,7 +12,6 @@ import { GROUP_COLOR_OPTIONS, Storage, SessionService, - SessionIdCaseConflictError, SessionOrganizationError, SESSION_WRITER_RPC_CODES, type SessionGroupColor, @@ -37,7 +36,6 @@ import { PermissionForbiddenError, PermissionPolicyNotImplementedError, SessionArchivingError, - SessionConflictError, } from '../acp-session-bridge.js'; import type { BridgeChannelQuarantinedError, @@ -133,11 +131,12 @@ import { import { createSessionOrganizationService } from '../session-organization-helpers.js'; import { archiveDaemonSessions, - assertSessionLoadable, + assertSessionRestorable, deleteDaemonSessionIfOrphan, deleteDaemonSessions, DaemonDrainingError, logSessionArchiveWarning, + resolveSessionIdForRestore, SessionArchiveCoordinator, unarchiveDaemonSessions, } from '../server/session-archive.js'; @@ -1855,24 +1854,6 @@ export class AcpDispatcher { // of a caller id contends on one key), so the request spelling // alone covers the raw-spelled batch delete/archive/unarchive // locks (parity with the REST restore handler). - const guardSessionService = new SessionService(cwd, { - runtimeBaseDir: sessionRuntime.sessionRuntimeBaseDir, - }); - let persistedGuardId: string | undefined; - try { - persistedGuardId = - await guardSessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if ( - error instanceof SessionIdCaseConflictError && - (await guardSessionService.getSessionLocation( - error.candidateSessionId ?? sessionId, - )) === 'conflict' - ) { - throw new SessionConflictError(sessionId); - } - throw error; - } const restored = await this.archiveCoordinator.runSharedMany( [sessionId], async () => { @@ -1880,30 +1861,20 @@ export class AcpDispatcher { const sessionService = new SessionService(cwd, { runtimeBaseDir: sessionRuntime.sessionRuntimeBaseDir, }); - let storageSessionId = persistedGuardId ?? sessionId; - let persistedSessionId: string | undefined; - try { - persistedSessionId = - await sessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if ( - error instanceof SessionIdCaseConflictError && - (await sessionService.getSessionLocation( - error.candidateSessionId ?? sessionId, - )) === 'conflict' - ) { - throw new SessionConflictError(sessionId); - } - throw error; - } + let storageSessionId = sessionId; + const persistedSessionId = await resolveSessionIdForRestore( + sessionService, + sessionId, + ); if (persistedSessionId) { storageSessionId = persistedSessionId; } else if (this.liveSessionIsolation) { throw new SessionNotFoundError(sessionId); } - await assertSessionLoadable( + await assertSessionRestorable( cwd, storageSessionId, + sessionId, sessionRuntime.sessionRuntimeBaseDir, ); // Re-seed the persisted parent lineage so a restored sub-session diff --git a/packages/cli/src/serve/acp-http/transport.test.ts b/packages/cli/src/serve/acp-http/transport.test.ts index fc85bf520a1..80c1b28390c 100644 --- a/packages/cli/src/serve/acp-http/transport.test.ts +++ b/packages/cli/src/serve/acp-http/transport.test.ts @@ -4662,39 +4662,38 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { }, ); - it('session/load rejects active/archive conflicts', async () => { - await withRuntimeDir(async () => { - const sessionId = '550e8400-e29b-41d4-a716-446655440321'; - await writeStoredSession(sessionId); - await writeStoredSession(sessionId, 'archived'); + it.each(['session/load', 'session/resume'] as const)( + '%s restores an active/archive conflicted session from its active copy', + async (method) => { + await withRuntimeDir(async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440321'; + await writeStoredSession(sessionId); + await writeStoredSession(sessionId, 'archived'); - const connId = await initialize(); - const connStream = await openStream(connId); - const got = takeFrames(connStream, 1); - await new Promise((r) => setTimeout(r, 50)); - await post(connId, { - jsonrpc: '2.0', - id: 212, - method: 'session/load', - params: { sessionId }, - }); + const connId = await initialize(); + const connStream = await openStream(connId); + const got = takeFrames(connStream, 1); + await new Promise((r) => setTimeout(r, 50)); + await post(connId, { + jsonrpc: '2.0', + id: 212, + method, + params: { sessionId }, + }); - const [frame] = (await got) as Array<{ - id: number; - error: { - code: number; - message: string; - data?: { errorKind?: string }; - }; - }>; - expect(frame.id).toBe(212); - expect(frame.error.code).toBe(-32603); - expect(frame.error.message).toContain( - 'Delete the session with POST /sessions/delete', - ); - expect(frame.error.data?.errorKind).toBe('session_conflict'); - }); - }); + const [frame] = (await got) as Array<{ + id: number; + result?: unknown; + error?: { code: number; message: string }; + }>; + // Loads read the active copy (CLI resume parity): a session left in + // both states by a crashed archive stays loadable over ACP. + expect(frame.id).toBe(212); + expect(frame.error).toBeUndefined(); + expect(frame.result).toEqual(expect.any(Object)); + }); + }, + ); it('session/load preserves sanitized session writer RPC errors', async () => { await withRuntimeDir(async () => { @@ -5078,6 +5077,10 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { SessionArchiveCoordinator.prototype, 'runSharedMany', ); + const findSessionId = vi.spyOn( + SessionService.prototype, + 'findSessionIdIgnoringCase', + ); try { const connId = await initialize(); @@ -5103,7 +5106,9 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { [sessionId], expect.any(Function), ); + expect(findSessionId).toHaveBeenCalledTimes(1); } finally { + findSessionId.mockRestore(); runSharedMany.mockRestore(); } }); @@ -5111,7 +5116,7 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { ); it.each(['session/load', 'session/resume'] as const)( - '%s converts a pre-guard both-states conflict to actionable session conflict', + '%s keeps a differently spelled both-states conflict strict', async (method) => { await withRuntimeDir(async () => { const sessionId = @@ -5134,9 +5139,6 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { expect(await reader.next()).toMatchObject({ id: 231, error: { - message: expect.stringContaining( - 'Delete the session with POST /sessions/delete', - ), data: expect.objectContaining({ errorKind: 'session_conflict' }), }, }); @@ -5146,7 +5148,7 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { ); it.each(['session/load', 'session/resume'] as const)( - '%s converts an in-guard both-states conflict to actionable session conflict', + '%s preserves a known case conflict without secondary classification', async (method) => { await withRuntimeDir(async () => { const sessionId = @@ -5160,12 +5162,11 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { ); const findSessionId = vi .spyOn(SessionService.prototype, 'findSessionIdIgnoringCase') - .mockResolvedValueOnce(storageSessionId) .mockRejectedValue(conflict); const getSessionLocation = vi .spyOn(SessionService.prototype, 'getSessionLocation') - .mockImplementation(async (candidateId) => - candidateId === storageSessionId ? 'conflict' : undefined, + .mockRejectedValue( + Object.assign(new Error('catalog failed'), { code: 'EIO' }), ); try { @@ -5188,9 +5189,7 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { }, }); reader.close(); - // The conversion must re-check the resolver's candidate spelling: - // the request-case id finds nothing on a case-sensitive filesystem. - expect(getSessionLocation).toHaveBeenCalledWith(storageSessionId); + expect(getSessionLocation).not.toHaveBeenCalled(); } finally { getSessionLocation.mockRestore(); findSessionId.mockRestore(); @@ -5351,8 +5350,10 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { expect.objectContaining({ id: 219, error: expect.objectContaining({ - message: expect.stringContaining('by case'), - data: expect.objectContaining({ errorKind: 'session_conflict' }), + data: expect.objectContaining({ + errorKind: 'session_conflict', + sessionId, + }), }), }), ); diff --git a/packages/cli/src/serve/conversations/conversation-workspace.test.ts b/packages/cli/src/serve/conversations/conversation-workspace.test.ts index 8736cfe299f..d0e2545a0d5 100644 --- a/packages/cli/src/serve/conversations/conversation-workspace.test.ts +++ b/packages/cli/src/serve/conversations/conversation-workspace.test.ts @@ -27,6 +27,7 @@ import { import { ConversationDirectoryIdentityError } from '../../utils/conversation-directory-identity.js'; const plantOnExpectedInspect = vi.hoisted(() => ({ armed: false })); +const enoentOnInspect = vi.hoisted(() => ({ armed: false })); vi.mock( '../../utils/conversation-directory-identity.js', @@ -40,6 +41,14 @@ vi.mock( inspectConversationDirectoryIdentity: async ( ...args: Parameters ) => { + if (enoentOnInspect.armed) { + enoentOnInspect.armed = false; + throw new actual.ConversationDirectoryIdentityError( + 'root', + 'io_error', + Object.assign(new Error('root vanished'), { code: 'ENOENT' }), + ); + } const identity = await actual.inspectConversationDirectoryIdentity( ...args, ); @@ -272,6 +281,21 @@ describe('Live conversation workspace root', () => { expect((await lstat(occupied)).isDirectory()).toBe(true); }); + it('treats a root that vanishes mid-inspection as already discarded', async () => { + const home = await tempHome(); + const workspace = new ConversationWorkspace({ homeDir: home }); + await workspace.materializeConversationDirectory('empty-racy'); + + enoentOnInspect.armed = true; + try { + await expect( + workspace.discardEmptyConversationDirectory('empty-racy'), + ).resolves.toBe(false); + } finally { + enoentOnInspect.armed = false; + } + }); + it('prepares only a new or reusable empty standalone child', async () => { const home = await tempHome(); const workspace = new ConversationWorkspace({ homeDir: home }); diff --git a/packages/cli/src/serve/conversations/conversation-workspace.ts b/packages/cli/src/serve/conversations/conversation-workspace.ts index a72ac05ae06..63bd68613e9 100644 --- a/packages/cli/src/serve/conversations/conversation-workspace.ts +++ b/packages/cli/src/serve/conversations/conversation-workspace.ts @@ -204,6 +204,16 @@ export class ConversationWorkspace { this.directoryKey(sessionId), ); } catch (error) { + // A root that vanished mid-inspection means there is nothing left to + // discard: keep the ENOENT-race → `false` contract instead of + // reporting an identity violation. + if ( + error instanceof ConversationDirectoryIdentityError && + error.reason === 'io_error' && + (error.cause as NodeJS.ErrnoException | undefined)?.code === 'ENOENT' + ) { + return false; + } liveIdentityError(error); } if (!identity) return false; diff --git a/packages/cli/src/serve/multi-workspace-sessions.test.ts b/packages/cli/src/serve/multi-workspace-sessions.test.ts index 5338b054f1a..685830c9391 100644 --- a/packages/cli/src/serve/multi-workspace-sessions.test.ts +++ b/packages/cli/src/serve/multi-workspace-sessions.test.ts @@ -2039,6 +2039,57 @@ describe('multi-workspace session dispatch', () => { ); }); + it('reads exact active/archive conflicts from the active copy in an internal workspace', async () => { + await withRuntimeDir(async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440111'; + await writeStoredSession({ + sessionId, + cwd: SECONDARY_CWD, + timestamp: '2026-07-08T00:00:00.000Z', + prompt: 'archived internal copy', + mtime: new Date('2026-07-08T00:00:00.000Z'), + sourceType: 'default', + sourceId: `${LIVE_SESSION_SOURCE_PREFIX}${sessionId}`, + }); + await archiveStoredSession(SECONDARY_CWD, sessionId); + await writeStoredSession({ + sessionId, + cwd: SECONDARY_CWD, + timestamp: '2026-07-08T00:01:00.000Z', + prompt: 'active internal copy', + mtime: new Date('2026-07-08T00:01:00.000Z'), + sourceType: 'default', + sourceId: `${LIVE_SESSION_SOURCE_PREFIX}${sessionId}`, + }); + const { app } = makeHarness({ + secondaryProvenance: 'live-conversation', + secondaryRuntimeBaseDir: Storage.getRuntimeBaseDir(), + }); + + const transcript = await request(app) + .get(`/workspaces/secondary-id/session/${sessionId}/transcript`) + .set('Host', host()); + const exported = await request(app) + .get(`/workspaces/secondary-id/session/${sessionId}/export?format=json`) + .set('Host', host()); + const archive = await request(app) + .post('/workspaces/secondary-id/sessions/archive') + .set('Host', host()) + .send({ sessionIds: [sessionId] }); + + expect(transcript.status).toBe(200); + expect(JSON.stringify(transcript.body)).toContain('active internal copy'); + expect(JSON.stringify(transcript.body)).not.toContain( + 'archived internal copy', + ); + expect(exported.status).toBe(200); + expect(exported.text).toContain('active internal copy'); + expect(exported.text).not.toContain('archived internal copy'); + expect(archive.status).toBe(409); + expect(archive.body.code).toBe('session_conflict'); + }); + }); + it('keeps the private directory canonical when restoring a mixed-case transcript', async () => { const storageSessionId = LIVE_PROJECTLESS_TASK_ID.toUpperCase(); await withStoredProjectlessLiveTasks( @@ -4647,11 +4698,8 @@ describe('multi-workspace session dispatch', () => { const conflict = await request(trusted.app) .get(`/workspaces/secondary-id/session/${conflictId}/export`) .set('Host', host()); - expect(conflict.status).toBe(409); - expect(conflict.body).toMatchObject({ - code: 'session_conflict', - sessionId: conflictId, - }); + expect(conflict.status).toBe(200); + expect(conflict.text).toContain('conflicting secondary'); const invalidFormat = await request(trusted.app) .get( diff --git a/packages/cli/src/serve/routes/session-telemetry.test.ts b/packages/cli/src/serve/routes/session-telemetry.test.ts index 6f024a27dba..37c69f46700 100644 --- a/packages/cli/src/serve/routes/session-telemetry.test.ts +++ b/packages/cli/src/serve/routes/session-telemetry.test.ts @@ -222,6 +222,7 @@ describe('special session resolver telemetry publication', () => { secondaryCwd, 'secondary-session', path.join(secondaryCwd, '.runtime'), + { allowActiveConflict: true }, ); expect(telemetryMocks.setDaemonTelemetryWorkspace).toHaveBeenCalledTimes(1); expect(telemetryMocks.setDaemonTelemetryWorkspace).toHaveBeenCalledWith( @@ -265,11 +266,13 @@ describe('special session resolver telemetry publication', () => { primaryCwd, 'stored-secondary', path.join(primaryCwd, '.runtime'), + { allowActiveConflict: false }, ); expect(archiveMocks.assertSessionLoadable).toHaveBeenCalledWith( secondaryCwd, 'stored-secondary', path.join(secondaryCwd, '.runtime'), + { allowActiveConflict: false }, ); expect(telemetryMocks.setDaemonTelemetryWorkspace).toHaveBeenCalledTimes(1); expect(telemetryMocks.setDaemonTelemetryWorkspace).toHaveBeenCalledWith( diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 3767c500356..826df24ff39 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -13,7 +13,6 @@ import { GROUP_COLOR_OPTIONS, GitWorktreeService, SessionOrganizationError, - SessionIdCaseConflictError, SESSION_TRANSCRIPT_MAX_LIMIT, SESSION_TRANSCRIPT_MAX_EXPANDED_PAGE_BYTES, SESSION_TRANSCRIPT_MAX_PAGE_BYTES, @@ -101,9 +100,11 @@ import { archiveDaemonSessions, assertSessionArchived, assertSessionLoadable, + assertSessionRestorable, deleteDaemonSessionIfOrphan, deleteDaemonSessions, logSessionArchiveWarning, + resolveSessionIdForRestore, type SessionArchiveCoordinator, unarchiveDaemonSessions, } from '../server/session-archive.js'; @@ -1164,8 +1165,11 @@ export function registerSessionRoutes( const service = createWorkspaceRuntimeSessionService(runtime); for (const sessionId of sessionIds) { const location = await service.getSessionLocation(sessionId); - if (location === 'conflict') throw new SessionConflictError(sessionId); - if ( + if (location === 'conflict') { + if (archiveState !== 'active') { + throw new SessionConflictError(sessionId); + } + } else if ( location === undefined || (archiveState !== 'any' && location !== archiveState) ) { @@ -1320,6 +1324,7 @@ export function registerSessionRoutes( runtime.workspaceCwd, sessionId, runtime.sessionRuntimeBaseDir, + { allowActiveConflict: true }, ); } assertRuntimeGenerationOpen?.(); @@ -1648,11 +1653,13 @@ export function registerSessionRoutes( ): Promise => { const activeInRuntime = async ( runtime: WorkspaceRuntime, + allowActiveConflict = false, ): Promise => { const location = await assertSessionLoadable( runtime.workspaceCwd, sessionId, runtime.sessionRuntimeBaseDir, + { allowActiveConflict }, ); if (location !== 'active') return false; if (!isInternalWorkspaceRuntime(runtime)) return true; @@ -1744,7 +1751,7 @@ export function registerSessionRoutes( const runtime = workspaceRegistry.primary; if (loadError === undefined) return runtime; try { - if (await activeInRuntime(runtime)) return runtime; + if (await activeInRuntime(runtime, true)) return runtime; } catch (err) { recordLoadError(err); } @@ -1790,7 +1797,7 @@ export function registerSessionRoutes( } let active = false; try { - active = await activeInRuntime(liveOwner.runtime); + active = await activeInRuntime(liveOwner.runtime, true); } catch (err) { recordLoadError(err); } @@ -1802,6 +1809,34 @@ export function registerSessionRoutes( return undefined; } if (active) { + if (isInternalWorkspaceRuntime(liveOwner.runtime)) { + const ordinaryCollisions: WorkspaceRuntime[] = []; + for (const ordinaryRuntime of workspaceRegistry.list()) { + const ordinaryService = + createWorkspaceRuntimeSessionService(ordinaryRuntime); + if (await ordinaryService.sessionExistsInAnyState(sessionId)) { + ordinaryCollisions.push(ordinaryRuntime); + } + } + if ( + internalEntry && + internalGeneration && + !assertCurrentInternalGeneration( + internalEntry, + internalGeneration, + res, + ) + ) { + return undefined; + } + if (ordinaryCollisions.length > 0) { + sendAmbiguousSessionOwner(res, route, sessionId, [ + liveOwner.runtime, + ...ordinaryCollisions, + ]); + return undefined; + } + } setDaemonTelemetryWorkspace(res, liveOwner.runtime.workspaceCwd); return liveOwner.runtime; } @@ -1813,7 +1848,7 @@ export function registerSessionRoutes( const runtime = requirePrimarySessionRuntime(workspaceRegistry, res); if (!runtime) return undefined; try { - if (await activeInRuntime(runtime)) { + if (await activeInRuntime(runtime, true)) { return runtime; } } catch (err) { @@ -3161,49 +3196,24 @@ export function registerSessionRoutes( // The coordinator canonicalizes lock keys (every case variant of a // caller id contends on one key), so the request spelling alone // covers the raw-spelled batch delete/archive/unarchive locks. - const guardSessionService = - createWorkspaceRuntimeSessionService(runtime); - try { - await guardSessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if ( - error instanceof SessionIdCaseConflictError && - (await guardSessionService.getSessionLocation( - error.candidateSessionId ?? sessionId, - )) === 'conflict' - ) { - throw new SessionConflictError(sessionId); - } - throw error; - } const session = await archiveCoordinator.runSharedMany( [sessionId], async () => { const sessionService = createWorkspaceRuntimeSessionService(runtime); - let persistedSessionId: string | undefined; - try { - persistedSessionId = - await sessionService.findSessionIdIgnoringCase(sessionId); - } catch (error) { - if ( - error instanceof SessionIdCaseConflictError && - (await sessionService.getSessionLocation( - error.candidateSessionId ?? sessionId, - )) === 'conflict' - ) { - throw new SessionConflictError(sessionId); - } - throw error; - } + const persistedSessionId = await resolveSessionIdForRestore( + sessionService, + sessionId, + ); if (persistedSessionId) { restoredStorageSessionId = persistedSessionId; } else if (isInternalWorkspaceRuntime(runtime)) { throw new SessionNotFoundError(sessionId); } - const location = await assertSessionLoadable( + const location = await assertSessionRestorable( workspaceCwd, restoredStorageSessionId, + sessionId, runtime.sessionRuntimeBaseDir, ); if (location === undefined && isInternalWorkspaceRuntime(runtime)) { @@ -4034,6 +4044,7 @@ export function registerSessionRoutes( runtime.workspaceCwd, sessionId, runtime.sessionRuntimeBaseDir, + { allowActiveConflict: true }, ); } const codec = getTranscriptCursorCodec(runtime); diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 0029c91881c..52b5aed1833 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -13064,6 +13064,7 @@ describe('createServeApp', () => { [sessionId], expect.any(Function), ); + expect(findSessionId).toHaveBeenCalledTimes(1); } finally { runSharedMany.mockRestore(); findSessionId.mockRestore(); @@ -13073,15 +13074,11 @@ describe('createServeApp', () => { ); it.each(['load', 'resume'] as const)( - 'converts a pre-guard both-states %s conflict to actionable session conflict', + 'keeps a differently spelled both-states %s conflict strict', async (action) => { const sessionId = '550e8400-e29b-41d4-a716-446655440147'; const storageSessionId = sessionId.toUpperCase(); const bridge = fakeBridge(); - // Same spelling persisted in both active and archived states: the - // resolver carries the candidate spelling, so the conversion must - // re-check THAT spelling — the request-case id finds nothing on a - // case-sensitive filesystem and would skip SessionConflictError. const conflict = new SessionIdCaseConflictError( sessionId, storageSessionId, @@ -13091,62 +13088,8 @@ describe('createServeApp', () => { .mockRejectedValue(conflict); const getSessionLocation = vi .spyOn(SessionService.prototype, 'getSessionLocation') - .mockImplementation(async (candidateId) => - candidateId === storageSessionId ? 'conflict' : undefined, - ); - const runSharedMany = vi.spyOn( - SessionArchiveCoordinator.prototype, - 'runSharedMany', - ); - const app = createServeApp( - { ...baseOpts, workspace: WS_BOUND }, - undefined, - { bridge }, - ); - - try { - const res = await request(app) - .post(`/session/${sessionId}/${action}`) - .set('Host', `127.0.0.1:${baseOpts.port}`) - .send({}); - - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_conflict'); - expect(res.body.error).toContain( - 'Delete the session with POST /sessions/delete', - ); - expect(getSessionLocation).toHaveBeenCalledWith(storageSessionId); - // The pre-guard conversion must fire before the guard is entered, - // so the shared guard never runs. - expect(runSharedMany).not.toHaveBeenCalled(); - expect(bridge.loadCalls).toEqual([]); - expect(bridge.resumeCalls).toEqual([]); - } finally { - runSharedMany.mockRestore(); - getSessionLocation.mockRestore(); - findSessionId.mockRestore(); - } - }, - ); - - it.each(['load', 'resume'] as const)( - 'converts an in-guard both-states %s conflict to actionable session conflict', - async (action) => { - const sessionId = '550e8400-e29b-41d4-a716-446655440148'; - const storageSessionId = sessionId.toUpperCase(); - const bridge = fakeBridge(); - const conflict = new SessionIdCaseConflictError( - sessionId, - storageSessionId, - ); - const findSessionId = vi - .spyOn(SessionService.prototype, 'findSessionIdIgnoringCase') - .mockResolvedValueOnce(storageSessionId) - .mockRejectedValue(conflict); - const getSessionLocation = vi - .spyOn(SessionService.prototype, 'getSessionLocation') - .mockImplementation(async (candidateId) => - candidateId === storageSessionId ? 'conflict' : undefined, + .mockRejectedValue( + Object.assign(new Error('catalog failed'), { code: 'EIO' }), ); const app = createServeApp( { ...baseOpts, workspace: WS_BOUND }, @@ -13162,15 +13105,7 @@ describe('createServeApp', () => { expect(res.status).toBe(409); expect(res.body.code).toBe('session_conflict'); - expect(res.body.error).toContain( - 'Delete the session with POST /sessions/delete', - ); - expect(getSessionLocation).toHaveBeenCalledWith(storageSessionId); - // Without the call-count assertion below, replacing the in-guard - // re-resolve with the pre-guard result still yields the same 409 - // via assertSessionLoadable's mocked 'conflict' location — the - // count is what pins the second resolution. - expect(findSessionId).toHaveBeenCalledTimes(2); + expect(getSessionLocation).not.toHaveBeenCalled(); expect(bridge.loadCalls).toEqual([]); expect(bridge.resumeCalls).toEqual([]); } finally { @@ -23958,6 +23893,36 @@ describe('createServeApp', () => { expect(bridge.resumeCalls).toHaveLength(0); }); + it('reads and exports the active copy of an exact persisted conflict', async () => { + const sid = '55555555-bbbb-cccc-dddd-aaaaaaaaaaac'; + await writeTranscriptSession(sid); + await writeTranscriptSession(sid, 'archived'); + const bridge = fakeBridge({ + sessionTranscriptImpl: async (req) => ({ + v: 1, + sessionId: req.sessionId, + events: [], + hasMore: false, + }), + }); + const app = createServeApp({ ...baseOpts, workspace: wsDir }, undefined, { + bridge, + boundWorkspace: wsDir, + }); + + const transcript = await request(app) + .get(`/session/${sid}/transcript`) + .set('Host', `127.0.0.1:${baseOpts.port}`); + const exported = await request(app) + .get(`/session/${sid}/export?format=json`) + .set('Host', `127.0.0.1:${baseOpts.port}`); + + expect(transcript.status).toBe(200); + expect(exported.status).toBe(200); + expect(exported.text).toContain(sid); + expect(bridge.sessionTranscriptCalls).toEqual([{ sessionId: sid }]); + }); + it('redacts skill bodies from flat transcript events (#9234)', async () => { const sid = '55555555-bbbb-cccc-dddd-aaaaaaaaaaab'; const bridge = fakeBridge({ @@ -25535,27 +25500,27 @@ describe('createServeApp', () => { expect(bridge.resumeCalls).toHaveLength(0); }); - it('rejects load for active/archive conflicts with session_conflict', async () => { - const sid = '44444444-bbbb-cccc-dddd-eeeeeeeeeeef'; - await writeSession(sid); - await writeSession(sid, 'archived'); - const bridge = fakeBridge(); - const app = createArchiveApp(bridge); + it.each(['load', 'resume'] as const)( + '%s restores active/archive conflicted sessions from the active copy', + async (action) => { + const sid = '44444444-bbbb-cccc-dddd-eeeeeeeeeeef'; + await writeSession(sid); + await writeSession(sid, 'archived'); + const bridge = fakeBridge(); + const app = createArchiveApp(bridge); - const loadRes = await request(app) - .post(`/session/${sid}/load`) - .set('Host', `127.0.0.1:${baseOpts.port}`) - .send({ cwd: wsDir }); - expect(loadRes.status).toBe(409); - expect(loadRes.body).toMatchObject({ - code: 'session_conflict', - sessionId: sid, - }); - expect(loadRes.body.error).toContain( - 'Delete the session with POST /sessions/delete', - ); - expect(bridge.loadCalls).toHaveLength(0); - }); + const response = await request(app) + .post(`/session/${sid}/${action}`) + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: wsDir }); + // Loads read the active copy (CLI resume parity): a session left in + // both states by a crashed archive stays loadable. + expect(response.status).toBe(200); + expect( + action === 'load' ? bridge.loadCalls : bridge.resumeCalls, + ).toHaveLength(1); + }, + ); it('returns session_archiving for prompt while archive is in flight', async () => { const sid = '55555555-bbbb-cccc-dddd-eeeeeeeeeeee'; diff --git a/packages/cli/src/serve/server/session-archive.test.ts b/packages/cli/src/serve/server/session-archive.test.ts index e153a04daa0..6e496735f75 100644 --- a/packages/cli/src/serve/server/session-archive.test.ts +++ b/packages/cli/src/serve/server/session-archive.test.ts @@ -9,6 +9,7 @@ import os from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { + SessionIdCaseConflictError, SessionService, SessionWriterConflictError, SessionWriterLostError, @@ -29,8 +30,10 @@ import { archiveDaemonSessions, assertSessionArchived, assertSessionLoadable, + assertSessionRestorable, deleteDaemonSessionIfOrphan, deleteDaemonSessions, + resolveSessionIdForRestore, SessionArchiveCoordinator, unarchiveDaemonSessions, DaemonDrainingError, @@ -82,6 +85,85 @@ describe('assertSessionLoadable', () => { expect(getLocationSpy).toHaveBeenCalledWith(sessionId); }); + it('reads the active copy after restore selected an exact conflict', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + writeSessionFile(workspaceDir, sessionId, 'active'); + writeSessionFile(workspaceDir, sessionId, 'archived'); + + await expect( + assertSessionLoadable(workspaceDir, sessionId, undefined, { + allowActiveConflict: true, + }), + ).resolves.toBe('active'); + }); + + it('does not read a differently spelled active/archive conflict', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + const storageSessionId = sessionId.toUpperCase(); + vi.spyOn(SessionService.prototype, 'getSessionLocation').mockResolvedValue( + 'conflict', + ); + vi.spyOn( + SessionService.prototype, + 'findSessionIdIgnoringCase', + ).mockRejectedValue( + new SessionIdCaseConflictError(sessionId, storageSessionId), + ); + + await expect( + assertSessionLoadable(workspaceDir, sessionId, undefined, { + allowActiveConflict: true, + }), + ).rejects.toThrow(SessionConflictError); + }); + + it('resolves an exact active/archive conflict only for restore', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + writeSessionFile(workspaceDir, sessionId, 'active'); + writeSessionFile(workspaceDir, sessionId, 'archived'); + const service = new SessionService(workspaceDir); + + await expect(resolveSessionIdForRestore(service, sessionId)).resolves.toBe( + sessionId, + ); + await expect( + assertSessionRestorable(workspaceDir, sessionId, sessionId), + ).resolves.toBe('active'); + }); + + it('does not restore a differently spelled active/archive conflict', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + const storageSessionId = sessionId.toUpperCase(); + vi.spyOn(SessionService.prototype, 'getSessionLocation').mockResolvedValue( + 'conflict', + ); + + await expect( + assertSessionRestorable(workspaceDir, storageSessionId, sessionId), + ).rejects.toThrow(SessionConflictError); + }); + + it('maps a differently spelled active/archive conflict without another read', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440001'; + const candidateSessionId = sessionId.toUpperCase(); + const service = new SessionService(workspaceDir); + const conflict = new SessionIdCaseConflictError( + sessionId, + candidateSessionId, + ); + vi.spyOn(service, 'findSessionIdIgnoringCase').mockRejectedValue(conflict); + const getLocation = vi + .spyOn(service, 'getSessionLocation') + .mockRejectedValue( + Object.assign(new Error('catalog failed'), { code: 'EIO' }), + ); + + await expect( + resolveSessionIdForRestore(service, sessionId), + ).rejects.toThrow(SessionConflictError); + expect(getLocation).not.toHaveBeenCalled(); + }); + it('ignores archived files that do not belong to this project', async () => { const sessionId = '550e8400-e29b-41d4-a716-446655440010'; const otherWorkspace = fs.mkdtempSync( @@ -1018,6 +1100,37 @@ describe('deleteDaemonSessions', () => { vi.restoreAllMocks(); }); + it('deletes both copies of an exact active/archive conflict', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440069'; + writeSessionFile(workspaceDir, sessionId, 'active'); + writeSessionFile(workspaceDir, sessionId, 'archived'); + const service = new SessionService(workspaceDir); + const acquire = vi.spyOn(service, 'acquireSessionWriterLease'); + + const result = await deleteDaemonSessions({ + sessionIds: [sessionId], + service, + bridge: { + closeSession: vi.fn().mockResolvedValue(undefined), + deleteSessionAttachments: vi.fn().mockResolvedValue(undefined), + }, + coordinator: new SessionArchiveCoordinator(), + }); + + expect(result).toEqual({ + removed: [sessionId], + notFound: [], + errors: [], + }); + expect(acquire).toHaveBeenCalledOnce(); + expect(fs.existsSync(sessionPath(workspaceDir, sessionId, 'active'))).toBe( + false, + ); + expect( + fs.existsSync(sessionPath(workspaceDir, sessionId, 'archived')), + ).toBe(false); + }); + it('removes a scheduled task bound to the deleted session', async () => { const sessionId = '550e8400-e29b-41d4-a716-446655440070'; writeSessionFile(workspaceDir, sessionId, 'active'); diff --git a/packages/cli/src/serve/server/session-archive.ts b/packages/cli/src/serve/server/session-archive.ts index 4b4d6c99342..168f5095822 100644 --- a/packages/cli/src/serve/server/session-archive.ts +++ b/packages/cli/src/serve/server/session-archive.ts @@ -5,6 +5,7 @@ */ import { + SessionIdCaseConflictError, SessionService, type SessionLocation, } from '@qwen-code/qwen-code-core'; @@ -333,13 +334,6 @@ async function deletePersistedSessionWithLease( if (initialLocation === undefined) { return { kind: 'notFound', mutationApplied: false }; } - if (initialLocation === 'conflict') { - return { - kind: 'error', - error: sessionLocationError(sessionId), - mutationApplied: false, - }; - } const mutation = await runWithDaemonWriterLease({ action: 'delete', @@ -353,9 +347,6 @@ async function deletePersistedSessionWithLease( mutationApplied: false, }; } - if (lockedLocation === 'conflict') { - throw sessionLocationError(sessionId); - } await assertOwnedAndUnchanged(); const removed = await service.removeSession(sessionId); return { @@ -541,6 +532,55 @@ export async function assertSessionLoadable( workspaceCwd: string, sessionId: string, runtimeBaseDir?: string, + options: { allowActiveConflict?: boolean } = {}, +): Promise { + const service = new SessionService(workspaceCwd, { + runtimeBaseDir, + }); + const location = await service.getSessionLocation(sessionId); + if (location === 'archived') { + throw new SessionArchivedError(sessionId); + } + if (location === 'conflict') { + if (options.allowActiveConflict) { + try { + await service.findSessionIdIgnoringCase(sessionId); + } catch (error) { + if ( + error instanceof SessionIdCaseConflictError && + error.reason === 'case_conflict' && + error.candidateSessionId === sessionId + ) { + return 'active'; + } + if (!(error instanceof SessionIdCaseConflictError)) throw error; + } + } + throw new SessionConflictError(sessionId); + } + return location; +} + +export async function resolveSessionIdForRestore( + service: SessionService, + sessionId: string, +): Promise { + try { + return await service.findSessionIdIgnoringCase(sessionId); + } catch (error) { + if (error instanceof SessionIdCaseConflictError) { + if (error.candidateSessionId === sessionId) return sessionId; + throw new SessionConflictError(sessionId); + } + throw error; + } +} + +export async function assertSessionRestorable( + workspaceCwd: string, + sessionId: string, + requestedSessionId: string, + runtimeBaseDir?: string, ): Promise { const location = await new SessionService(workspaceCwd, { runtimeBaseDir, @@ -549,7 +589,10 @@ export async function assertSessionLoadable( throw new SessionArchivedError(sessionId); } if (location === 'conflict') { - throw new SessionConflictError(sessionId); + if (sessionId !== requestedSessionId) { + throw new SessionConflictError(requestedSessionId); + } + return 'active'; } return location; } diff --git a/packages/cli/src/utils/conversation-directory-identity.test.ts b/packages/cli/src/utils/conversation-directory-identity.test.ts index ed90dad763b..e7d7334afc4 100644 --- a/packages/cli/src/utils/conversation-directory-identity.test.ts +++ b/packages/cli/src/utils/conversation-directory-identity.test.ts @@ -10,6 +10,7 @@ import { lstat, mkdir, mkdtemp, + realpath, rename, rm, symlink, @@ -114,6 +115,29 @@ describe('conversation directory identity', () => { ).resolves.toBeUndefined(); }); + it('reports a child deleted between the lstat and the realpath as missing', async () => { + const { root } = await tempRoot(); + const created = await materializeConversationDirectoryIdentity( + root, + 'vanish', + ); + + const realRealpath = realFsPromises.realpath; + vi.mocked(realpath).mockImplementation((async (path: string) => { + if (path.endsWith(created.identity.name)) { + await rm(created.identity.canonicalPath, { recursive: true }); + } + return realRealpath(path); + }) as unknown as typeof realpath); + try { + await expect( + inspectConversationDirectoryIdentity(root, 'vanish'), + ).resolves.toBeUndefined(); + } finally { + vi.mocked(realpath).mockRestore(); + } + }); + it('rejects same-path replacement against an expected identity', async () => { const { root } = await tempRoot(); const original = await materializeConversationDirectoryIdentity( diff --git a/packages/cli/src/utils/conversation-directory-identity.ts b/packages/cli/src/utils/conversation-directory-identity.ts index 71ac5b767bc..d45ec87aa0d 100644 --- a/packages/cli/src/utils/conversation-directory-identity.ts +++ b/packages/cli/src/utils/conversation-directory-identity.ts @@ -318,9 +318,9 @@ export async function inspectConversationDirectoryIdentity( canonical = await realpath(candidate); after = await lstat(canonical); } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - throw new ConversationDirectoryIdentityError('child', 'identity_changed'); - } + // A child deleted between the lstat and the realpath is "already gone" — + // the same verdict as the initial-lstat ENOENT — not an identity change. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; throwIdentityIoError('child', error); } validateDirectoryStats(after, 'child'); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 8cf2dfa204c..ba24783f263 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3202,6 +3202,51 @@ describe('Server Config (config.ts)', () => { }, ); + it('adopts the active transcript when writer activation sees both states', async () => { + const sessionId = '550e8400-e29b-41d4-a716-446655440099'; + const sessionData = { + conversation: { + sessionId, + projectHash: 'test', + startTime: new Date(0).toISOString(), + lastUpdated: new Date(0).toISOString(), + messages: [], + }, + filePath: `/tmp/${sessionId}.jsonl`, + lastCompletedUuid: null, + } as ResumedSessionData; + const config = new Config({ + ...baseParams, + sessionId, + chatRecording: true, + experimentalZedIntegration: true, + sessionWriterLeaseEnabled: true, + }); + const service = config.getSessionService(); + vi.spyOn(service, 'getSessionLocation').mockResolvedValue('conflict'); + const loadSession = vi + .spyOn(service, 'loadSession') + .mockResolvedValue(sessionData); + const lease = { + sessionId, + transcriptExistedAtAcquire: true, + isReleased: false, + assertOwnedAndUnchanged: vi.fn().mockResolvedValue(undefined), + release: vi.fn().mockResolvedValue(undefined), + } as unknown as SessionWriterLease; + const acquire = vi + .spyOn(SessionWriterLease, 'acquire') + .mockResolvedValue(lease); + + await ( + config as unknown as { activateChatRecording(): Promise } + ).activateChatRecording(); + + expect(loadSession).toHaveBeenCalledWith(sessionId); + expect(config.hasSessionWriteOwnership()).toBe(true); + acquire.mockRestore(); + }); + it('releases a pending lease while a real baseline read is gated', async () => { const root = await mkdtemp(path.join(os.tmpdir(), 'qwen-config-writer-')); const runtimeBaseDir = path.join(root, 'runtime'); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 301eb4d6685..481b00c6b77 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -3241,7 +3241,7 @@ export class Config { if (this.sessionWriterShutdownRequested) { throw new SessionWriterShutdownError(); } - if (location === 'conflict' || location === 'archived') { + if (location === 'archived') { throw new SessionTranscriptChangedError(); } let authoritative: ResumedSessionData | undefined; diff --git a/packages/core/src/services/sessionService.test.ts b/packages/core/src/services/sessionService.test.ts index 702530d05bd..9cdde468023 100644 --- a/packages/core/src/services/sessionService.test.ts +++ b/packages/core/src/services/sessionService.test.ts @@ -2779,11 +2779,19 @@ describe('SessionService', () => { }); describe('findSessionIdIgnoringCase', () => { + let readdirSpy: MockInstance; + + beforeEach(() => { + readdirSpy = vi + .spyOn(fs.promises, 'readdir') + .mockResolvedValue([] as never); + }); + it('finds a legacy mixed-case transcript', async () => { const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never) - .mockReturnValueOnce([] as never); + readdirSpy + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never) + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( async (sessionId) => sessionId === legacySessionId ? 'active' : undefined, @@ -2796,11 +2804,12 @@ describe('SessionService', () => { it('returns the single authoritative spelling after scanning both states', async () => { const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([] as never) - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never); - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'archived', + readdirSpy + .mockResolvedValueOnce([] as never) + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never); + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (sessionId) => + sessionId === legacySessionId ? 'archived' : undefined, ); await expect( @@ -2809,16 +2818,19 @@ describe('SessionService', () => { }); it('rejects case-only duplicate spellings instead of choosing by enumeration order', async () => { - readdirSyncSpy - .mockReturnValueOnce([ - `${sessionIdA}.jsonl`, + readdirSpy + .mockResolvedValueOnce([ `${sessionIdA.toUpperCase()}.jsonl`, + `${sessionIdA.replace('e29b', 'E29b')}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); - // Both candidates are genuinely readable — a true conflict. + .mockResolvedValueOnce([] as never); + // Both twins are genuinely readable while the requested spelling + // resolves nothing — a true conflict. const getLocation = vi .spyOn(sessionService, 'getSessionLocation') - .mockResolvedValue('active'); + .mockImplementation(async (id) => + id === sessionIdA ? undefined : 'active', + ); await expect( sessionService.findSessionIdIgnoringCase(sessionIdA), @@ -2834,12 +2846,12 @@ describe('SessionService', () => { it('rejects case-only duplicates whose heads are all unreadable as occupying the id', async () => { // Neither spelling on disk is the requested one, so minting the request // beside them would add a third case-variant of the same id. - readdirSyncSpy - .mockReturnValueOnce([ + readdirSpy + .mockResolvedValueOnce([ `${sessionIdA.toUpperCase()}.jsonl`, `${sessionIdA.replace('e29b', 'E29b')}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); + .mockResolvedValueOnce([] as never); // Neither head recovers records, but both files persist on disk. vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( undefined, @@ -2857,7 +2869,7 @@ describe('SessionService', () => { }); it('rejects one spelling that exists in both active and archive state', async () => { - readdirSyncSpy.mockReturnValue([`${sessionIdA}.jsonl`] as never); + readdirSpy.mockResolvedValue([`${sessionIdA}.jsonl`] as never); const getLocation = vi .spyOn(sessionService, 'getSessionLocation') .mockResolvedValue('conflict'); @@ -2875,9 +2887,9 @@ describe('SessionService', () => { it('rejects a present-but-unreadable single candidate as occupying the id', async () => { const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never) - .mockReturnValueOnce([] as never); + readdirSpy + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never) + .mockResolvedValueOnce([] as never); // The head recovers no records (torn/empty/foreign), but the file // still occupies the id — admission must not mint a case-only twin. vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( @@ -2898,12 +2910,12 @@ describe('SessionService', () => { it('returns the sole readable spelling when a case twin is unreadable', async () => { const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([ + readdirSpy + .mockResolvedValueOnce([ `${sessionIdA}.jsonl`, `${legacySessionId}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( async (id) => (id === legacySessionId ? 'active' : undefined), ); @@ -2913,26 +2925,27 @@ describe('SessionService', () => { ).resolves.toBe(legacySessionId); }); - it('returns the spelling when one of its two state copies is unreadable', async () => { - readdirSyncSpy.mockReturnValue([`${sessionIdA}.jsonl`] as never); + it('returns a twin spelling when one of its two state copies is unreadable', async () => { + const legacySessionId = sessionIdA.toUpperCase(); + readdirSpy.mockResolvedValue([`${legacySessionId}.jsonl`] as never); // getSessionLocation counts only readable copies, so one garbage // twin still resolves to the surviving state. - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'active', + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (id) => (id === legacySessionId ? 'active' : undefined), ); await expect( sessionService.findSessionIdIgnoringCase(sessionIdA), - ).resolves.toBe(sessionIdA); + ).resolves.toBe(legacySessionId); }); it('returns undefined when the matching transcript disappears during resolution', async () => { // The candidate must differ in case from the request, otherwise the // self-escape short-circuits and the race loop below it never runs. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never) - .mockReturnValueOnce([] as never); + readdirSpy + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never) + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( undefined, ); @@ -2948,12 +2961,12 @@ describe('SessionService', () => { // nothing, but the *other* spelling still occupies the id: minting the // request beside it is what would make both permanently unrestorable. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([ + readdirSpy + .mockResolvedValueOnce([ `${sessionIdA}.jsonl`, `${legacySessionId}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( undefined, ); @@ -2972,12 +2985,12 @@ describe('SessionService', () => { // The twin raced away between enumeration and the presence check, so // nothing but the request's own unreadable file is left to occupy the id. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([ + readdirSpy + .mockResolvedValueOnce([ `${sessionIdA}.jsonl`, `${legacySessionId}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( undefined, ); @@ -2988,6 +3001,9 @@ describe('SessionService', () => { await expect( sessionService.findSessionIdIgnoringCase(sessionIdA), ).resolves.toBeUndefined(); + expect(existsSyncSpy).toHaveBeenCalledWith( + expect.stringContaining(`${legacySessionId}.jsonl`), + ); }); it('reports the requested spelling absent when its own transcript head is unreadable', async () => { @@ -2995,9 +3011,9 @@ describe('SessionService', () => { // transcript under the requested spelling. It is a case-only twin of // nothing, so reusing the id must stay possible — `getSessionLocation` // already reports the file as nonexistent. - readdirSyncSpy - .mockReturnValueOnce([`${sessionIdA}.jsonl`] as never) - .mockReturnValueOnce([] as never); + readdirSpy + .mockResolvedValueOnce([`${sessionIdA}.jsonl`] as never) + .mockResolvedValueOnce([] as never); vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( undefined, ); @@ -3013,9 +3029,9 @@ describe('SessionService', () => { // session id, but SESSION_FILE_PATTERN excludes them — enumerating them // here would report a healthy transcript as occupied-but-unreadable. const agentSessionId = `${sessionIdA}-agent-foo`; - readdirSyncSpy - .mockReturnValueOnce([`${agentSessionId}.jsonl`] as never) - .mockReturnValueOnce([] as never); + readdirSpy + .mockResolvedValueOnce([`${agentSessionId}.jsonl`] as never) + .mockResolvedValueOnce([] as never); const getLocation = vi.spyOn(sessionService, 'getSessionLocation'); existsSyncSpy.mockReturnValue(true); @@ -3029,11 +3045,12 @@ describe('SessionService', () => { // On a case-insensitive filesystem both spellings open the same file, so // each reports a readable location even though only one copy exists. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${sessionIdA}.jsonl`] as never) - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never); - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'active', + const mixedSessionId = sessionIdA.replace('e29b', 'E29b'); + readdirSpy + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never) + .mockResolvedValueOnce([`${mixedSessionId}.jsonl`] as never); + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (id) => (id === sessionIdA ? undefined : 'active'), ); statSyncSpy.mockReturnValue({ dev: 1, @@ -3043,19 +3060,20 @@ describe('SessionService', () => { await expect( sessionService.findSessionIdIgnoringCase(sessionIdA), - ).resolves.toBe(sessionIdA); + ).resolves.toBe(legacySessionId); }); it('still rejects two readable spellings backed by distinct files', async () => { const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([ - `${sessionIdA}.jsonl`, + const mixedSessionId = sessionIdA.replace('e29b', 'E29b'); + readdirSpy + .mockResolvedValueOnce([ `${legacySessionId}.jsonl`, + `${mixedSessionId}.jsonl`, ] as never) - .mockReturnValueOnce([] as never); - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'active', + .mockResolvedValueOnce([] as never); + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (id) => (id === sessionIdA ? undefined : 'active'), ); statSyncSpy.mockImplementation( (filePath: fs.PathLike) => @@ -3080,11 +3098,12 @@ describe('SessionService', () => { // cannot prove two spellings are one transcript. Without that proof the // pair must stay a conflict instead of silently resolving to one. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${sessionIdA}.jsonl`] as never) - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never); - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'active', + const mixedSessionId = sessionIdA.replace('e29b', 'E29b'); + readdirSpy + .mockResolvedValueOnce([`${mixedSessionId}.jsonl`] as never) + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never); + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (id) => (id === sessionIdA ? undefined : 'active'), ); statSyncSpy.mockReturnValue({ dev: 1, @@ -3104,11 +3123,12 @@ describe('SessionService', () => { // A transient EACCES/EMFILE says nothing about aliasing; laundering it // into `session_conflict` would report a retryable blip as permanent. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${sessionIdA}.jsonl`] as never) - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never); - vi.spyOn(sessionService, 'getSessionLocation').mockResolvedValue( - 'active', + const mixedSessionId = sessionIdA.replace('e29b', 'E29b'); + readdirSpy + .mockResolvedValueOnce([`${mixedSessionId}.jsonl`] as never) + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never); + vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( + async (id) => (id === sessionIdA ? undefined : 'active'), ); statSyncSpy.mockImplementation(() => { throw Object.assign(new Error('permission denied'), { @@ -3122,17 +3142,21 @@ describe('SessionService', () => { }); it('ignores a candidate whose transcript vanishes mid-resolution', async () => { - // The lowercase entry races away, so only the uppercase spelling is left - // to back the readable state and it resolves without a conflict. + // The mixed-case entry races away, so only the uppercase spelling is + // left to back the readable state and it resolves without a conflict. const legacySessionId = sessionIdA.toUpperCase(); - readdirSyncSpy - .mockReturnValueOnce([`${sessionIdA}.jsonl`] as never) - .mockReturnValueOnce([`${legacySessionId}.jsonl`] as never); + const mixedSessionId = sessionIdA.replace('e29b', 'E29b'); + readdirSpy + .mockResolvedValueOnce([`${mixedSessionId}.jsonl`] as never) + .mockResolvedValueOnce([`${legacySessionId}.jsonl`] as never); vi.spyOn(sessionService, 'getSessionLocation').mockImplementation( - async (id) => (id === legacySessionId ? 'archived' : 'active'), + async (id) => { + if (id === legacySessionId) return 'archived'; + return id === sessionIdA ? undefined : 'active'; + }, ); statSyncSpy.mockImplementation((filePath: fs.PathLike) => { - if (String(filePath).includes(`${sessionIdA}.jsonl`)) { + if (String(filePath).includes(`${mixedSessionId}.jsonl`)) { throw Object.assign(new Error('gone'), { code: 'ENOENT' }); } return { dev: 1, ino: 7, isFile: () => true } as fs.Stats; diff --git a/packages/core/src/services/sessionService.ts b/packages/core/src/services/sessionService.ts index 88c386abb1d..a8e12f8ef66 100644 --- a/packages/core/src/services/sessionService.ts +++ b/packages/core/src/services/sessionService.ts @@ -822,7 +822,7 @@ export class SessionService { for (const state of ['active', 'archived'] as const) { let fileNames: string[]; try { - fileNames = fs.readdirSync(this.getChatsDirForState(state)); + fileNames = await fs.promises.readdir(this.getChatsDirForState(state)); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; throw error;