diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index a5d92a30311..03095cb415a 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -92,6 +92,7 @@ vi.mock('@qwen-code/qwen-code-core', () => ({ _args: args, })), SessionService: vi.fn(), + SESSION_TITLE_MAX_LENGTH: 200, tokenLimit: vi.fn(), SessionStartSource: { Startup: 'startup', @@ -103,6 +104,16 @@ vi.mock('@qwen-code/qwen-code-core', () => ({ }, })); +vi.mock('./runtimeOutputDirContext.js', () => ({ + runWithAcpRuntimeOutputDir: vi.fn( + async ( + _settings: unknown, + _cwd: string, + fn: () => T | Promise, + ): Promise => fn(), + ), +})); + vi.mock('./authMethods.js', () => ({ buildAuthMethods: vi.fn() })); vi.mock('./service/filesystem.js', () => ({ AcpFileSystemService: vi.fn(), @@ -126,7 +137,11 @@ import { import type { Config } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../config/settings.js'; import type { CliArgs } from '../config/config.js'; -import { SessionEndReason, MCPServerConfig } from '@qwen-code/qwen-code-core'; +import { + SessionEndReason, + MCPServerConfig, + SessionService, +} from '@qwen-code/qwen-code-core'; import type { McpServer } from '@agentclientprotocol/sdk'; import { AgentSideConnection } from '@agentclientprotocol/sdk'; import { loadSettings } from '../config/settings.js'; @@ -894,3 +909,222 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); }); + +// Regression coverage for the MR-review finding that ACP renameSession +// bypassed any live ChatRecordingService. The disk-only path left the +// recording service's in-memory `currentCustomTitle` stale, and the next +// re-anchor (every 32KB) or finalize() silently reverted the rename by +// re-emitting the cached old title at EOF. +describe('QwenAgent extMethod renameSession routing', () => { + type AgentSideConnectionLike = { closed: Promise }; + type AgentLike = { + initialize: (args: Record) => Promise; + newSession: (args: Record) => Promise; + extMethod: ( + method: string, + params: Record, + ) => Promise>; + }; + + let capturedAgentFactory: + | ((conn: AgentSideConnectionLike) => AgentLike) + | undefined; + let mockConfig: Config; + + // Live session sessionId is whatever `getSessionId()` on the inner config + // returns; matches the existing test scaffolding. + const liveSessionId = '550e8400-e29b-41d4-a716-446655440000'; + + beforeEach(() => { + vi.clearAllMocks(); + mockConnectionState.reset(); + capturedAgentFactory = undefined; + + vi.mocked(AgentSideConnection).mockImplementation((factory: unknown) => { + capturedAgentFactory = factory as typeof capturedAgentFactory; + return { + get closed() { + return mockConnectionState.promise; + }, + } as unknown as InstanceType; + }); + + mockConfig = { + initialize: vi.fn().mockResolvedValue(undefined), + getHookSystem: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(false), + hasHooksForEvent: vi.fn().mockReturnValue(false), + getModel: vi.fn().mockReturnValue('test-model'), + getModelsConfig: vi.fn().mockReturnValue({ + getCurrentAuthType: vi.fn().mockReturnValue('api-key'), + }), + refreshAuth: vi.fn().mockResolvedValue(undefined), + } as unknown as Config; + }); + + function makeRecordingService() { + return { + recordCustomTitle: vi.fn().mockReturnValue(true), + flush: vi.fn().mockResolvedValue(undefined), + }; + } + + function makeLiveSessionInnerConfig( + recording: ReturnType | null, + ) { + return { + initialize: vi.fn().mockResolvedValue(undefined), + getModelsConfig: vi.fn().mockReturnValue({ + getCurrentAuthType: vi.fn().mockReturnValue('api-key'), + }), + refreshAuth: vi.fn().mockResolvedValue(undefined), + getModel: vi.fn().mockReturnValue('m'), + getContentGeneratorConfig: vi.fn().mockReturnValue({}), + getAvailableModels: vi.fn().mockReturnValue([]), + getModes: vi.fn().mockReturnValue([]), + getApprovalMode: vi.fn().mockReturnValue('default'), + getSessionId: vi.fn().mockReturnValue(liveSessionId), + getAuthType: vi.fn().mockReturnValue('api-key'), + getAllConfiguredModels: vi.fn().mockReturnValue([]), + getGeminiClient: vi.fn().mockReturnValue({ + isInitialized: vi.fn().mockReturnValue(true), + initialize: vi.fn().mockResolvedValue(undefined), + }), + getFileSystemService: vi.fn().mockReturnValue(undefined), + setFileSystemService: vi.fn(), + getHookSystem: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(true), + hasHooksForEvent: vi.fn().mockReturnValue(false), + getChatRecordingService: vi.fn().mockReturnValue(recording), + }; + } + + function makeAcpSettings() { + return { + merged: { mcpServers: {} }, + getUserHooks: vi.fn().mockReturnValue({}), + getProjectHooks: vi.fn().mockReturnValue({}), + } as unknown as LoadedSettings; + } + + async function bootAgent( + innerConfig: ReturnType, + ) { + vi.mocked(loadSettings).mockReturnValue(makeAcpSettings()); + vi.mocked(loadCliConfig).mockResolvedValue( + innerConfig as unknown as Config, + ); + vi.mocked(Session).mockImplementation( + () => + ({ + getId: vi.fn().mockReturnValue(liveSessionId), + getConfig: vi.fn().mockReturnValue(innerConfig), + sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined), + replayHistory: vi.fn().mockResolvedValue(undefined), + installRewriter: vi.fn(), + }) as unknown as InstanceType, + ); + + const agentPromise = runAcpAgent( + mockConfig, + makeAcpSettings(), + {} as CliArgs, + ); + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + const agent = capturedAgentFactory!({ + get closed() { + return mockConnectionState.promise; + }, + }) as AgentLike; + return { agent, agentPromise }; + } + + it('routes through ChatRecordingService.recordCustomTitle when the target session is live', async () => { + const recording = makeRecordingService(); + const innerConfig = makeLiveSessionInnerConfig(recording); + const { agent, agentPromise } = await bootAgent(innerConfig); + + // Populate `this.sessions` so the rename target is "live". + await agent.newSession({ cwd: '/tmp', mcpServers: [] }); + + const result = await agent.extMethod('renameSession', { + cwd: '/tmp', + sessionId: liveSessionId, + title: 'New Title', + }); + + expect(recording.recordCustomTitle).toHaveBeenCalledWith( + 'New Title', + 'manual', + ); + // Awaited so the rename is durable before the response returns — + // a follow-up listSessions can't race the queued write. + expect(recording.flush).toHaveBeenCalledOnce(); + // The disk-only fallback must NOT fire when a live session exists, + // otherwise we'd double-write (and the second writer would be the + // SessionService that lacks the in-memory cache update). + expect(SessionService).not.toHaveBeenCalled(); + expect(result).toEqual({ success: true }); + + mockConnectionState.resolve(); + await agentPromise; + }); + + it('falls back to SessionService.renameSession when no live session matches the sessionId', async () => { + const recording = makeRecordingService(); + const innerConfig = makeLiveSessionInnerConfig(recording); + const { agent, agentPromise } = await bootAgent(innerConfig); + + await agent.newSession({ cwd: '/tmp', mcpServers: [] }); + + const renameSpy = vi.fn().mockResolvedValue(true); + vi.mocked(SessionService).mockImplementation( + () => + ({ + renameSession: renameSpy, + }) as unknown as InstanceType, + ); + + const deadSessionId = '6ba7b810-9dad-11d1-80b4-00c04fd430c8'; + const result = await agent.extMethod('renameSession', { + cwd: '/tmp', + sessionId: deadSessionId, + title: 'Renamed Offline', + }); + + expect(SessionService).toHaveBeenCalledWith('/tmp'); + expect(renameSpy).toHaveBeenCalledWith(deadSessionId, 'Renamed Offline'); + // The live recording belongs to a *different* sessionId; it must + // be left untouched, otherwise we'd corrupt an unrelated session's + // title cache. + expect(recording.recordCustomTitle).not.toHaveBeenCalled(); + expect(result).toEqual({ success: true }); + + mockConnectionState.resolve(); + await agentPromise; + }); + + it('returns success=false when the live ChatRecordingService rejects the title (I/O error)', async () => { + const recording = makeRecordingService(); + recording.recordCustomTitle.mockReturnValue(false); + const innerConfig = makeLiveSessionInnerConfig(recording); + const { agent, agentPromise } = await bootAgent(innerConfig); + + await agent.newSession({ cwd: '/tmp', mcpServers: [] }); + + const result = await agent.extMethod('renameSession', { + cwd: '/tmp', + sessionId: liveSessionId, + title: 'New Title', + }); + + // Even on failure we still flush so the writeChain settles before + // responding — keeps subsequent reads consistent and surfaces any + // queued earlier failure to the caller. + expect(recording.flush).toHaveBeenCalledOnce(); + expect(result).toEqual({ success: false }); + + mockConnectionState.resolve(); + await agentPromise; + }); +}); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 30d25fa56ba..390b30fc92b 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -341,7 +341,11 @@ class QwenAgent implements Agent { }); const sessions: SessionInfo[] = result.items.map((item) => ({ - cwd: item.cwd, + // `cwd` is now optional on SessionListItem (lite tier omits it), + // but `listSessions` returns enriched items so it's populated in + // practice. Fall back to the request's `cwd` for any edge case + // where the head-record decode failed mid-page. + cwd: item.cwd ?? cwd, sessionId: item.sessionId, title: item.customTitle || item.prompt || '(session)', updatedAt: new Date(item.mtime).toISOString(), @@ -486,6 +490,23 @@ class QwenAgent implements Agent { `Title too long (max ${SESSION_TITLE_MAX_LENGTH} chars)`, ); } + // When the target session is currently live in this process, route + // through its ChatRecordingService so the in-memory `currentCustomTitle` + // stays in sync. Writing directly to disk via SessionService here + // would leave the live recording's cache stale; the next title + // re-anchor (every 32KB of writes) or finalize() would re-emit the + // old title and silently revert the rename. The disk-only path + // remains for the dead-session case (e.g., another client renaming + // a session that isn't active in this process). + const liveRecording = this.sessions + .get(sessionId) + ?.getConfig() + .getChatRecordingService(); + if (liveRecording) { + const ok = liveRecording.recordCustomTitle(title, 'manual'); + await liveRecording.flush(); + return { success: ok }; + } const success = await runWithAcpRuntimeOutputDir( this.settings, cwd, diff --git a/packages/cli/src/ui/components/SessionPicker.tsx b/packages/cli/src/ui/components/SessionPicker.tsx index d13f0c295c3..e1c95e89953 100644 --- a/packages/cli/src/ui/components/SessionPicker.tsx +++ b/packages/cli/src/ui/components/SessionPicker.tsx @@ -88,7 +88,15 @@ function SessionListItemView({ boldSelectedPrefix = true, }: SessionListItemViewProps): React.JSX.Element { const timeAgo = formatRelativeTime(session.mtime); - const messageText = formatMessageCount(session.messageCount); + // `messageCount` is now optional on `SessionListItem` because counting + // requires a full readline pass over the JSONL — far too expensive to do + // in the listing path. The row simply omits the "N messages" segment + // when the count isn't available; preview-style consumers that care can + // call `SessionService.countSessionMessages(sessionId)` lazily. + const messageText = + typeof session.messageCount === 'number' + ? formatMessageCount(session.messageCount) + : undefined; const showUpIndicator = isFirst && showScrollUp; const showDownIndicator = isLast && showScrollDown; @@ -101,11 +109,20 @@ function SessionListItemView({ ? prefixChars.scrollDown : prefixChars.normal; - const promptText = session.customTitle || session.prompt || '(empty prompt)'; + // Pending rows have come back from `listSessionsLite` but enrichment + // hasn't filled in the title/prompt yet. Show a short sessionId + // prefix as a placeholder so the row is visibly "loading" rather + // than blank — once enrichment completes the row re-renders with + // the real title. + const isPending = session.pending === true; + const promptText = isPending + ? `…${session.sessionId.slice(0, 8)}` + : session.customTitle || session.prompt || '(empty prompt)'; const truncatedPrompt = truncateText(promptText, maxPromptWidth); // Dim auto-generated titles so users can distinguish a model guess from // a title they chose themselves with `/rename`. Selected row keeps the // accent color — legibility of the focused row wins over source hinting. + // Pending rows also dim — they're not actionable until enriched. const isAutoTitle = session.titleSource === 'auto' && Boolean(session.customTitle); @@ -128,7 +145,7 @@ function SessionListItemView({ color={ isSelected ? theme.text.accent - : isAutoTitle + : isAutoTitle || isPending ? theme.text.secondary : theme.text.primary } @@ -139,7 +156,8 @@ function SessionListItemView({ - {timeAgo} · {messageText} + {timeAgo} + {messageText !== undefined && ` · ${messageText}`} {session.gitBranch && ` · ${session.gitBranch}`} diff --git a/packages/cli/src/ui/components/SessionPreview.test.tsx b/packages/cli/src/ui/components/SessionPreview.test.tsx index 479eeee5408..b559c666b23 100644 --- a/packages/cli/src/ui/components/SessionPreview.test.tsx +++ b/packages/cli/src/ui/components/SessionPreview.test.tsx @@ -134,6 +134,28 @@ describe('SessionPreview', () => { expect(frame).toContain('feat/preview'); }); + it('falls back to count from loaded conversation when messageCount prop is absent', async () => { + // listSessions() now omits messageCount, so the picker passes undefined + // through to SessionPreview. The footer must still show a count, derived + // from the loaded ResumedSessionData using unique user/assistant UUIDs. + const svc = mockService(fakeResumedData()); + const { lastFrame } = render( + + + , + ); + await wait(100); + const frame = lastFrame() ?? ''; + expect(frame).toMatch(/2\s*messages/); + expect(frame).not.toContain('undefined'); + }); + it('calls onExit when Escape is pressed', async () => { const onExit = vi.fn(); const svc = mockService(fakeResumedData()); diff --git a/packages/cli/src/ui/components/SessionPreview.tsx b/packages/cli/src/ui/components/SessionPreview.tsx index c8b9cb4ddbb..08327ab291d 100644 --- a/packages/cli/src/ui/components/SessionPreview.tsx +++ b/packages/cli/src/ui/components/SessionPreview.tsx @@ -78,6 +78,23 @@ export function SessionPreview(props: SessionPreviewProps) { return buildResumedHistoryItems(data, null); }, [data]); + // `listSessions` omits `messageCount` for perf, so the prop is usually + // undefined in practice. Compute the count from the loaded conversation + // using the same unique-user/assistant-uuid semantics as + // `SessionService.countSessionMessages` — the data is already in memory, + // so this is free and avoids an extra disk read. + const computedMessageCount = useMemo(() => { + if (!data) return undefined; + const seen = new Set(); + for (const msg of data.conversation.messages) { + if (msg.type === 'user' || msg.type === 'assistant') { + seen.add(msg.uuid); + } + } + return seen.size; + }, [data]); + const displayMessageCount = messageCount ?? computedMessageCount; + useKeypress( (key) => { const { name, ctrl } = key; @@ -98,8 +115,8 @@ export function SessionPreview(props: SessionPreviewProps) { const separatorWidth = Math.max(0, boxWidth - 2); const metaParts: string[] = []; - if (typeof messageCount === 'number') { - metaParts.push(formatMessageCount(messageCount)); + if (typeof displayMessageCount === 'number') { + metaParts.push(formatMessageCount(displayMessageCount)); } if (typeof mtime === 'number') { metaParts.push(formatRelativeTime(mtime)); diff --git a/packages/cli/src/ui/components/StandaloneSessionPicker.test.tsx b/packages/cli/src/ui/components/StandaloneSessionPicker.test.tsx index 3a8bef6c522..c38dd3c2735 100644 --- a/packages/cli/src/ui/components/StandaloneSessionPicker.test.tsx +++ b/packages/cli/src/ui/components/StandaloneSessionPicker.test.tsx @@ -71,12 +71,35 @@ function createMockSessionService( sessions: SessionListItem[] = [], hasMore = false, ) { + // Two-phase listing API. Tests pass already-enriched session + // objects; the lite stub returns them as `pending: true` (so the + // first frame matches the lite contract) and enrichSessions echoes + // the same enriched items back. The legacy `listSessions` stays + // around for any test that still calls it directly. + const liteItems = sessions.map((s) => ({ ...s, pending: true })); return { listSessions: vi.fn().mockResolvedValue({ items: sessions, hasMore, nextCursor: hasMore ? Date.now() : undefined, } as ListSessionsResult), + listSessionsLite: vi.fn().mockResolvedValue({ + items: liteItems, + hasMore, + nextCursor: hasMore ? Date.now() : undefined, + } as ListSessionsResult), + enrichSessions: vi + .fn() + .mockImplementation(async (items: SessionListItem[]) => { + // Echo back the originally provided enriched objects, matching + // by sessionId. Items not in the original list (shouldn't + // happen in these tests) are dropped, mirroring the real + // service's project-mismatch drop. + const byId = new Map(sessions.map((s) => [s.sessionId, s])); + return items + .map((it) => byId.get(it.sessionId)) + .filter((s): s is SessionListItem => s !== undefined); + }), loadSession: vi.fn(), loadLastSession: vi .fn() @@ -1404,6 +1427,47 @@ describe('SessionPicker', () => { expect(output).toContain('feature-branch'); }); + it('renders the metadata line cleanly when messageCount is undefined', async () => { + // `listSessions()` now omits `messageCount` for perf, so this is the + // default production shape. Pin the row's render contract: time and + // branch still show, and the line must not contain a stray "messages" + // word, the literal "undefined", or a dangling " · " from the missing + // count segment. + const sessions = [ + createMockSession({ + sessionId: 'lazy-count', + prompt: 'No count yet', + messageCount: undefined, + gitBranch: 'feature-branch', + }), + ]; + const mockService = createMockSessionService(sessions); + + const { lastFrame } = render( + + + , + ); + + await wait(100); + + const output = lastFrame() ?? ''; + expect(output).toContain('No count yet'); + expect(output).toContain('feature-branch'); + // Negative assertions guard the omit-count branch. + expect(output).not.toContain('messages'); + expect(output).not.toContain('undefined'); + // The metadata line should not contain a doubled separator. We isolate + // the row's metadata line (the one with the gitBranch) and check it. + const metaLine = + output.split('\n').find((l) => l.includes('feature-branch')) ?? ''; + expect(metaLine).not.toMatch(/·\s*·/); + }); + it('should show header and footer', async () => { const sessions = [createMockSession({ messageCount: 1 })]; const mockService = createMockSessionService(sessions); @@ -1506,6 +1570,63 @@ describe('SessionPicker', () => { }); describe('Pagination', () => { + it('renders the first frame from lite items before enrichment lands', async () => { + // The two-phase load contract: listSessionsLite resolves + // synchronously-ish, the picker renders pending placeholders, + // and only later does enrichSessions resolve to fill in titles. + // This test holds enrichSessions open while inspecting the + // intermediate frame. + const enriched = [ + createMockSession({ + sessionId: 'enriched-1', + prompt: 'final title', + messageCount: 1, + }), + ]; + const liteItems = enriched.map((s) => ({ ...s, pending: true })); + + let resolveEnrich: (items: SessionListItem[]) => void = () => {}; + const enrichPromise = new Promise((resolve) => { + resolveEnrich = resolve; + }); + + const mockService = { + listSessions: vi.fn(), + listSessionsLite: vi.fn().mockResolvedValue({ + items: liteItems, + hasMore: false, + }), + enrichSessions: vi.fn().mockReturnValue(enrichPromise), + loadSession: vi.fn(), + loadLastSession: vi.fn().mockResolvedValue({}), + }; + + const { lastFrame, unmount } = render( + + + , + ); + + // Lite resolves; picker should paint pending placeholders. + await wait(50); + const pendingFrame = lastFrame() ?? ''; + // Placeholder format: '…' followed by sessionId prefix. + expect(pendingFrame).toContain('…enriched'); + expect(pendingFrame).not.toContain('final title'); + + // Now release enrichment; the row swaps to the real title. + resolveEnrich(enriched); + await wait(50); + const enrichedFrame = lastFrame() ?? ''; + expect(enrichedFrame).toContain('final title'); + + unmount(); + }); + it('should load more sessions when scrolling to bottom', async () => { const firstPage = Array.from({ length: 5 }, (_, i) => createMockSession({ @@ -1524,19 +1645,28 @@ describe('SessionPicker', () => { }), ); + // Two-phase load: the picker calls listSessionsLite for each + // page (first frame) and enrichSessions to hydrate. Stub both. + const liteFirst = firstPage.map((s) => ({ ...s, pending: true })); + const liteSecond = secondPage.map((s) => ({ ...s, pending: true })); const mockService = { - listSessions: vi + listSessions: vi.fn(), + listSessionsLite: vi .fn() .mockResolvedValueOnce({ - items: firstPage, + items: liteFirst, hasMore: true, nextCursor: Date.now() - 5000, }) .mockResolvedValueOnce({ - items: secondPage, + items: liteSecond, hasMore: false, nextCursor: undefined, }), + enrichSessions: vi + .fn() + .mockResolvedValueOnce(firstPage) + .mockResolvedValueOnce(secondPage), loadSession: vi.fn(), loadLastSession: vi.fn().mockResolvedValue({}), }; @@ -1556,8 +1686,9 @@ describe('SessionPicker', () => { await wait(200); - // First page should be loaded - expect(mockService.listSessions).toHaveBeenCalled(); + // First page should be loaded via the lite-then-enrich path. + expect(mockService.listSessionsLite).toHaveBeenCalled(); + expect(mockService.enrichSessions).toHaveBeenCalled(); unmount(); }); diff --git a/packages/cli/src/ui/hooks/useSessionPicker.ts b/packages/cli/src/ui/hooks/useSessionPicker.ts index 969498a81f6..edafb3562b1 100644 --- a/packages/cli/src/ui/hooks/useSessionPicker.ts +++ b/packages/cli/src/ui/hooks/useSessionPicker.ts @@ -15,7 +15,6 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import type { - ListSessionsResult, SessionListItem, SessionService, } from '@qwen-code/qwen-code-core'; @@ -168,29 +167,76 @@ export function useSessionPicker({ const showScrollDown = scrollOffset + maxVisibleItems < filteredSessions.length; + // Two-phase load: lite (stat-only) renders the first frame + // immediately as placeholder rows; enrich runs in the background + // and replaces each row with its full metadata. Picker unmount + // signals an abort so a long enrichment doesn't try to setState + // on an unmounted hook. + const enrichAbortRef = useRef(null); + useEffect( + () => () => { + enrichAbortRef.current?.abort(); + }, + [], + ); + + const replaceEnriched = useCallback((enriched: SessionListItem[]) => { + // Project-mismatch / disappeared-file items get filtered out by + // enrichSessions. Drop those from local state by replacing only + // the rows whose sessionId came back; the rest stay as their + // pending placeholders or get removed if the enrichment didn't + // return them at all. + setSessionState((prev) => { + const enrichedById = new Map(enriched.map((s) => [s.sessionId, s])); + const next: SessionListItem[] = []; + for (const row of prev.sessions) { + if (!row.pending) { + next.push(row); + continue; + } + const hit = enrichedById.get(row.sessionId); + if (hit) { + next.push(hit); + } + // else: lite row didn't survive enrichment — drop it. + } + return { ...prev, sessions: next }; + }); + }, []); + // Initial load — skip when pre-filtered sessions are provided useEffect(() => { if (!sessionService || hasInitialSessions) { return; } + const ctrl = new AbortController(); + enrichAbortRef.current?.abort(); + enrichAbortRef.current = ctrl; + const loadInitialSessions = async () => { try { - const result: ListSessionsResult = await sessionService.listSessions({ + const lite = await sessionService.listSessionsLite({ size: SESSION_PAGE_SIZE, }); + if (ctrl.signal.aborted) return; setSessionState({ - sessions: result.items, - hasMore: result.hasMore, - nextCursor: result.nextCursor, + sessions: lite.items, + hasMore: lite.hasMore, + nextCursor: lite.nextCursor, }); - } finally { + setIsLoading(false); + + const enriched = await sessionService.enrichSessions(lite.items); + if (ctrl.signal.aborted) return; + replaceEnriched(enriched); + } catch { setIsLoading(false); } }; void loadInitialSessions(); - }, [sessionService, hasInitialSessions]); + }, [sessionService, hasInitialSessions, replaceEnriched]); const loadMoreSessions = useCallback(async () => { if (!sessionService || !sessionState.hasMore || isLoadingMoreRef.current) { @@ -199,19 +245,27 @@ export function useSessionPicker({ isLoadingMoreRef.current = true; try { - const result: ListSessionsResult = await sessionService.listSessions({ + const lite = await sessionService.listSessionsLite({ size: SESSION_PAGE_SIZE, cursor: sessionState.nextCursor, }); setSessionState((prev) => ({ - sessions: [...prev.sessions, ...result.items], - hasMore: result.hasMore && result.nextCursor !== undefined, - nextCursor: result.nextCursor, + sessions: [...prev.sessions, ...lite.items], + hasMore: lite.hasMore && lite.nextCursor !== undefined, + nextCursor: lite.nextCursor, })); + + const enriched = await sessionService.enrichSessions(lite.items); + replaceEnriched(enriched); } finally { isLoadingMoreRef.current = false; } - }, [sessionService, sessionState.hasMore, sessionState.nextCursor]); + }, [ + sessionService, + sessionState.hasMore, + sessionState.nextCursor, + replaceEnriched, + ]); // Reset selection when any filter changes (branch toggle or text query). useEffect(() => { diff --git a/packages/core/src/services/chatRecordingService.customTitle.test.ts b/packages/core/src/services/chatRecordingService.customTitle.test.ts index 4fa9246de78..9b4f3094f34 100644 --- a/packages/core/src/services/chatRecordingService.customTitle.test.ts +++ b/packages/core/src/services/chatRecordingService.customTitle.test.ts @@ -177,4 +177,210 @@ describe('ChatRecordingService - recordCustomTitle', () => { }); }); }); + + describe('title re-anchor invariant', () => { + it('re-anchors the title once enough non-title bytes accumulate', async () => { + // Write a title, then keep appending bulky messages until the + // running tally crosses the 32KB threshold. The first non-title + // record after the threshold should provoke a fresh + // custom_title append at EOF — keeping the title within the + // 64KB tail window the picker scans even if no lifecycle event + // (finalize) has fired. + chatRecordingService.recordCustomTitle('long-running-task'); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); + + // Each user message carries ~2KB of text — 20 of them put well + // over 32KB on the wire (counting the ~200B per-record envelope). + const bulkText = 'x'.repeat(2000); + for (let i = 0; i < 20; i++) { + chatRecordingService.recordUserMessage([{ text: bulkText }]); + } + await chatRecordingService.flush(); + + const writes = vi.mocked(jsonl.writeLine).mock.calls; + const titleAppendsAfterClear = writes.filter(([, record]) => { + const r = record as ChatRecord; + return r.type === 'system' && r.subtype === 'custom_title'; + }); + + expect(titleAppendsAfterClear.length).toBeGreaterThanOrEqual(1); + // The re-anchored record must carry the same title + source as + // the original — it's a copy, not a fresh rename. + const reanchored = titleAppendsAfterClear[0][1] as ChatRecord; + expect(reanchored.systemPayload).toEqual({ + customTitle: 'long-running-task', + titleSource: 'manual', + }); + }); + + it('does not re-anchor when no title has been set', async () => { + // The counter only matters when there's a title to keep alive; + // sessions that never set one shouldn't pay for spurious writes. + const bulkText = 'x'.repeat(2000); + for (let i = 0; i < 30; i++) { + chatRecordingService.recordUserMessage([{ text: bulkText }]); + } + await chatRecordingService.flush(); + + const titleAppends = vi + .mocked(jsonl.writeLine) + .mock.calls.filter(([, record]) => { + const r = record as ChatRecord; + return r.type === 'system' && r.subtype === 'custom_title'; + }); + expect(titleAppends).toHaveLength(0); + }); + + it('omits titleSource on re-anchor when source is unknown (legacy resumed session)', async () => { + // The picker dim-styling depends on the persisted `titleSource` + // discriminator. Legacy `custom_title` records (written before + // the field existed) have no source — `getSessionTitleInfo` + // returns `source: undefined` for those, and the writer's + // re-anchor invariant must mirror that exact shape: emit + // `customTitle` alone, never a hardcoded `'manual'`. Otherwise + // resuming a legacy session on a current build would silently + // reclassify it the first time the threshold fires. + vi.mocked(mockConfig.getResumedSessionData).mockReturnValue({ + lastCompletedUuid: null, + } as unknown as ReturnType); + const getSessionTitleInfo = vi + .fn() + .mockReturnValue({ title: 'legacy-title', source: undefined }); + ( + mockConfig as unknown as { + getSessionService: () => { + getSessionTitleInfo: typeof getSessionTitleInfo; + }; + } + ).getSessionService = () => ({ getSessionTitleInfo }); + + const svc = new ChatRecordingService(mockConfig); + // Constructor's finalize re-appends a custom_title record on resume + // — clear it out so we can isolate the threshold-triggered re-anchor. + await svc.flush(); + vi.mocked(jsonl.writeLine).mockClear(); + + const bulkText = 'x'.repeat(2000); + for (let i = 0; i < 20; i++) { + svc.recordUserMessage([{ text: bulkText }]); + } + await svc.flush(); + + const titleAppends = vi + .mocked(jsonl.writeLine) + .mock.calls.filter(([, record]) => { + const r = record as ChatRecord; + return r.type === 'system' && r.subtype === 'custom_title'; + }); + + expect(titleAppends.length).toBeGreaterThanOrEqual(1); + const reanchored = titleAppends[0][1] as ChatRecord; + // Key must be ABSENT, not present-and-undefined — JSON.stringify + // would still serialize an explicit `undefined` away, but the + // record-shape contract is "no key when no source", so pin it. + expect(reanchored.systemPayload).toEqual({ customTitle: 'legacy-title' }); + expect( + Object.prototype.hasOwnProperty.call( + reanchored.systemPayload as object, + 'titleSource', + ), + ).toBe(false); + }); + + it('counts UTF-8 bytes, not UTF-16 code units, when measuring bulk writes', async () => { + // CJK characters are 1 UTF-16 code unit but 3 UTF-8 bytes. The wire + // format is UTF-8 (jsonl.writeLine emits utf8), so a per-record + // `String.length` undercounts a multi-byte payload by ~3×. A naive + // length-based counter would let ~96KB of CJK content land on disk + // before the 32KB threshold thinks it has — pushing the title past + // the 64KB tail window the picker scans. + // + // Twelve 1500-char CJK messages ≈ 21K UTF-16 units (under threshold) + // but ≈ 57K UTF-8 bytes (over). Anchor fires only when the counter + // measures bytes, not chars. + chatRecordingService.recordCustomTitle('cjk-session'); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); + + const cjkText = '汉'.repeat(1500); + for (let i = 0; i < 12; i++) { + chatRecordingService.recordUserMessage([{ text: cjkText }]); + } + await chatRecordingService.flush(); + + const titleAppends = vi + .mocked(jsonl.writeLine) + .mock.calls.filter(([, record]) => { + const r = record as ChatRecord; + return r.type === 'system' && r.subtype === 'custom_title'; + }); + expect(titleAppends.length).toBeGreaterThanOrEqual(1); + }); + + it('resets the byte counter when re-anchor fails — no retry storm', async () => { + // If reanchorTitle throws (disk full, permission revoked) and we + // leave the byte counter pinned at the threshold, every subsequent + // appendRecord will re-fire the failing reanchor — an unbounded + // retry storm that amplifies I/O pressure on an already-degraded + // system. Resetting on failure trades one missed anchor for + // bounded recovery; finalize() will re-emit on the next lifecycle + // event. + chatRecordingService.recordCustomTitle('long-running-task'); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); + + // Wrap the private appendRecord so any custom_title append (i.e. + // a re-anchor — the initial title write already happened) throws. + // Bulk records pass through to the real implementation so the + // byte counter still accumulates exactly as production would. + let reanchorAttempts = 0; + const svc = chatRecordingService as unknown as { + appendRecord(record: ChatRecord): void; + }; + const originalAppendRecord = svc.appendRecord.bind(chatRecordingService); + svc.appendRecord = (record: ChatRecord) => { + if (record.type === 'system' && record.subtype === 'custom_title') { + reanchorAttempts++; + throw new Error('simulated disk-full'); + } + return originalAppendRecord(record); + }; + + // 25 × 2KB ≈ 50KB > 32KB → first re-anchor fires (and throws). + // With the counter-reset fix, it stays reset; without it, every + // subsequent message would re-trigger reanchor. + const bulkText = 'x'.repeat(2000); + for (let i = 0; i < 25; i++) { + chatRecordingService.recordUserMessage([{ text: bulkText }]); + } + await chatRecordingService.flush(); + + // One failed attempt is acceptable; multiple means the counter was + // pinned and turned a single fault into a per-record loop. + expect(reanchorAttempts).toBe(1); + }); + + it('does not re-anchor on small write bursts under threshold', async () => { + // A handful of small messages must not trigger a re-anchor — + // the cost would defeat the whole point. Threshold is 32KB; + // five 200B user messages stay safely under it. + chatRecordingService.recordCustomTitle('quick-session'); + await chatRecordingService.flush(); + vi.mocked(jsonl.writeLine).mockClear(); + + for (let i = 0; i < 5; i++) { + chatRecordingService.recordUserMessage([{ text: 'short' }]); + } + await chatRecordingService.flush(); + + const titleAppends = vi + .mocked(jsonl.writeLine) + .mock.calls.filter(([, record]) => { + const r = record as ChatRecord; + return r.type === 'system' && r.subtype === 'custom_title'; + }); + expect(titleAppends).toHaveLength(0); + }); + }); }); diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index c7690382a14..114472c153a 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -41,6 +41,17 @@ const SESSION_FILE_DIFF_AGGREGATE_CHAR_LIMIT = 100_000; const SESSION_FILE_DIFF_CHAR_LIMIT = 50_000; const SESSION_FILE_CONTENT_CHAR_LIMIT = 16_000; +/** + * Re-append a fresh `custom_title` record to EOF once this many bytes + * of other JSONL content have been written since the last title + * anchor. Half of the picker's 64KB tail-read window so that even an + * oversized record landing right at the threshold keeps the title + * within scan range. Lifting this above 64KB would let the title + * fall out of the tail window between re-anchors; lowering it + * trades extra writes for a tighter safety margin. + */ +const TITLE_REANCHOR_BYTES = 32 * 1024; + function isFileDiffDisplay(resultDisplay: unknown): resultDisplay is FileDiff { if ( typeof resultDisplay !== 'object' || @@ -505,6 +516,20 @@ export class ChatRecordingService { */ private lastAttributionSnapshotJson: string | undefined; + /** + * Approximate bytes of JSONL content appended since the last + * `custom_title` record landed in this file. Used by the title + * re-anchor invariant: once enough non-title content accumulates + * past the last anchor, {@link appendRecord} re-appends a fresh + * `custom_title` to EOF so the picker's tail-window scan + * ({@link readSessionTitleFromFile}) keeps finding it. + * + * Without this, a long agentic turn that streams >64KB of tool + * output could push the only `custom_title` record past the 64KB + * tail window, forcing the picker into a full-file fallback. + */ + private bytesSinceTitleAnchor = 0; + constructor(config: Config) { this.config = config; this.lastRecordUuid = @@ -691,6 +716,79 @@ export class ChatRecordingService { } } }); + this.updateTitleAnchorTracking(record); + } + + /** + * Maintain the "title is always in the tail window" invariant by + * counting bytes appended since the last `custom_title` record and + * re-anchoring once enough non-title content has been written. + * + * - A `custom_title` record IS the new anchor — reset the counter. + * - Without a current title (never set), the counter is irrelevant. + * - Otherwise accumulate this record's serialized size; if the + * running total breaches the threshold, re-append a fresh + * `custom_title` to EOF. The recursive `appendRecord` call will + * land this branch's first arm (subtype === 'custom_title') and + * reset the counter to 0. + * + * Size estimate uses `JSON.stringify` for parity with the actual + * write path (`jsonl.writeLine` serializes the same way). It's an + * extra serialize per record, but appendRecord is already gated by + * an async I/O write whose cost dominates by orders of magnitude. + * + * Byte count uses `Buffer.byteLength(..., 'utf8')`, not `String.length`: + * `String.length` counts UTF-16 code units, but `jsonl.writeLine` + * emits UTF-8 — multi-byte characters (CJK, emoji) are 2–3× larger + * on disk than `.length` reports, and undercounting would let the + * actual on-disk distance from the last anchor blow past the 64KB + * tail window before the threshold fires. + */ + private updateTitleAnchorTracking(record: ChatRecord): void { + if (record.type === 'system' && record.subtype === 'custom_title') { + this.bytesSinceTitleAnchor = 0; + return; + } + if (!this.currentCustomTitle) return; + // +1 for the trailing newline jsonl.writeLine appends. + this.bytesSinceTitleAnchor += + Buffer.byteLength(JSON.stringify(record), 'utf8') + 1; + if (this.bytesSinceTitleAnchor >= TITLE_REANCHOR_BYTES) { + this.reanchorTitle(); + } + } + + /** + * Append a fresh `custom_title` record to EOF using the in-memory + * cached title. Mirrors {@link finalize}'s record shape — invoked + * mid-session (every {@link TITLE_REANCHOR_BYTES} of other writes) + * so the picker's tail-window scan never has to fall back to + * scanning the middle of the file. + */ + private reanchorTitle(): void { + if (!this.currentCustomTitle) return; + try { + const record: ChatRecord = { + ...this.createBaseRecord('system'), + type: 'system', + subtype: 'custom_title', + systemPayload: { + customTitle: this.currentCustomTitle, + ...(this.currentTitleSource + ? { titleSource: this.currentTitleSource } + : {}), + }, + }; + this.appendRecord(record); + } catch (error) { + // Reset the counter even on failure: otherwise every subsequent + // appendRecord re-fires reanchorTitle (counter still ≥ threshold) + // and turns a transient I/O issue into an unbounded retry storm. + // Skipping a single anchor write is the right tradeoff — finalize() + // will re-emit one on the next lifecycle event. + this.bytesSinceTitleAnchor = 0; + debugLogger.error('Error re-anchoring custom title:', error); + } } /** diff --git a/packages/core/src/services/sessionService.corruption.test.ts b/packages/core/src/services/sessionService.corruption.test.ts index 804b0600e22..f1034af7d61 100644 --- a/packages/core/src/services/sessionService.corruption.test.ts +++ b/packages/core/src/services/sessionService.corruption.test.ts @@ -8,8 +8,8 @@ * Integration tests for SessionService corruption-recovery paths. * * Lives in its own file (no module-level `vi.mock`) because both - * `countSessionMessages` and `readLastRecordUuid` walk real bytes from disk - * via `fs.createReadStream` / `fs.readSync`, and need the real + * `countSessionMessagesFromPath` and `readLastRecordUuid` walk real bytes + * from disk via `fs.createReadStream` / `fs.readSync`, and need the real * `jsonl.parseLineTolerant` to exercise the `}{`-glued recovery path * introduced for #3606. The unit-test file (sessionService.test.ts) mocks * jsonl-utils wholesale, so corruption shapes can't be exercised there. @@ -59,11 +59,14 @@ function writeJsonl(name: string, content: string): string { return p; } -describe('SessionService.countSessionMessages (corruption recovery)', () => { +describe('SessionService.countSessionMessagesFromPath (corruption recovery)', () => { // The method is private; cast is the cheapest way to test the unit - // without exposing it on the public surface. + // without exposing it on the public surface. The public + // `countSessionMessages(sessionId)` enforces the SESSION_FILE_PATTERN + // and project-scoping check before delegating here, neither of which + // is what these corruption-recovery tests are about. type Privates = { - countSessionMessages: (filePath: string) => Promise; + countSessionMessagesFromPath: (filePath: string) => Promise; }; let svc: Privates; @@ -80,7 +83,7 @@ describe('SessionService.countSessionMessages (corruption recovery)', () => { const r3 = JSON.stringify(recordFor('u3', 'user', 'u2')); const file = writeJsonl('glued.jsonl', `${r1}${r2}\n${r3}\n`); - expect(await svc.countSessionMessages(file)).toBe(3); + expect(await svc.countSessionMessagesFromPath(file)).toBe(3); }); it('does not zero out the count when a line is valid JSON but not an object', async () => { @@ -92,7 +95,7 @@ describe('SessionService.countSessionMessages (corruption recovery)', () => { const r2 = JSON.stringify(recordFor('u2', 'assistant', 'u1')); const file = writeJsonl('scalar-line.jsonl', `${r1}\nnull\n${r2}\n`); - expect(await svc.countSessionMessages(file)).toBe(2); + expect(await svc.countSessionMessagesFromPath(file)).toBe(2); }); it('deduplicates uuids across recovered fragments', async () => { @@ -101,12 +104,12 @@ describe('SessionService.countSessionMessages (corruption recovery)', () => { const r1 = JSON.stringify(recordFor('u1', 'user', null)); const file = writeJsonl('dup.jsonl', `${r1}${r1}\n`); - expect(await svc.countSessionMessages(file)).toBe(1); + expect(await svc.countSessionMessagesFromPath(file)).toBe(1); }); it('returns 0 for a missing file', async () => { expect( - await svc.countSessionMessages(path.join(tmpRoot, 'nope.jsonl')), + await svc.countSessionMessagesFromPath(path.join(tmpRoot, 'nope.jsonl')), ).toBe(0); }); }); diff --git a/packages/core/src/services/sessionService.rename.test.ts b/packages/core/src/services/sessionService.rename.test.ts index 87d2e5ce710..48ac6d26695 100644 --- a/packages/core/src/services/sessionService.rename.test.ts +++ b/packages/core/src/services/sessionService.rename.test.ts @@ -323,6 +323,42 @@ describe('SessionService - rename and custom title', () => { expect(matches[0].sessionId).toBe(sessionIdA); }); + it('omits messageCount and avoids createReadStream (perf contract)', async () => { + // findSessionsByTitle is the second user-facing call site that the + // perf work removed `messageCount` from. This test pins both + // contracts: matched items must have `messageCount === undefined`, + // and the per-match `fs.createReadStream` count pass must not run + // — re-introducing it would silently bring back the O(file-size) + // cost without any other test failing. + const titleContent = + JSON.stringify({ + type: 'system', + subtype: 'custom_title', + systemPayload: { customTitle: 'my-feature' }, + }) + '\n'; + + setupSessionFiles([ + { id: sessionIdA, record: recordA1, mtime: now, titleContent }, + ]); + + readSyncSpy.mockImplementation( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (_fd: number, buffer: any) => { + const data = Buffer.from(titleContent); + data.copy(buffer); + return data.length; + }, + ); + + const createReadStreamSpy = vi.spyOn(fs, 'createReadStream'); + + const matches = await sessionService.findSessionsByTitle('my-feature'); + + expect(matches).toHaveLength(1); + expect(matches[0].messageCount).toBeUndefined(); + expect(createReadStreamSpy).not.toHaveBeenCalled(); + }); + it('should return empty array when no session matches', async () => { setupSessionFiles([{ id: sessionIdA, record: recordA1, mtime: now }]); diff --git a/packages/core/src/services/sessionService.test.ts b/packages/core/src/services/sessionService.test.ts index a6b07afe932..075b671b420 100644 --- a/packages/core/src/services/sessionService.test.ts +++ b/packages/core/src/services/sessionService.test.ts @@ -6,6 +6,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { Readable } from 'node:stream'; import { afterEach, beforeEach, @@ -191,6 +192,111 @@ describe('SessionService', () => { expect(result.items[0].gitBranch).toBe('main'); }); + it('listSessionsLite returns stat-only items (no head/tail IO)', async () => { + // Pin the fast-path contract: lite items expose only the + // cheap stat-derived fields (sessionId / mtime / filePath / + // fileSize / pending). No head record means no `cwd`, no + // `prompt`, no `customTitle` etc. Future refactors that + // re-introduce head IO into listSessionsLite will fail here. + readdirSyncSpy.mockReturnValue([ + `${sessionIdA}.jsonl`, + ] as unknown as Array>); + statSyncSpy.mockReturnValue({ + mtimeMs: 12345, + size: 4096, + isFile: () => true, + } as fs.Stats); + + const result = await sessionService.listSessionsLite(); + + expect(vi.mocked(jsonl.readLines)).not.toHaveBeenCalled(); + expect(result.items).toHaveLength(1); + const item = result.items[0]; + expect(item.sessionId).toBe(sessionIdA); + expect(item.mtime).toBe(12345); + expect(item.fileSize).toBe(4096); + expect(item.pending).toBe(true); + expect(item.cwd).toBeUndefined(); + expect(item.prompt).toBeUndefined(); + expect(item.customTitle).toBeUndefined(); + }); + + it('enrichSessions populates metadata and applies the project filter', async () => { + // Two lite items: one whose head record matches our project + // hash, one that doesn't (sibling project sharing the same + // chats dir). Enrichment must keep the first and drop the + // second — same defensive filter the pre-split listSessions + // applied during the head-read step. + const wrongProject = 'other-project-hash'; + vi.mocked(getProjectHash).mockImplementation((cwd: string) => { + if (cwd === '/test/project/root') return 'test-project-hash'; + return wrongProject; + }); + const otherProjectRecord: ChatRecord = { + ...recordA1, + cwd: '/somewhere/else', + sessionId: sessionIdB, + }; + vi.mocked(jsonl.readLines).mockImplementation( + async (filePath: string) => { + if (filePath.includes(sessionIdA)) return [recordA1]; + if (filePath.includes(sessionIdB)) return [otherProjectRecord]; + return []; + }, + ); + + const lite: ReturnType = + Promise.resolve({ + items: [ + { + sessionId: sessionIdA, + mtime: 200, + filePath: `/chats/${sessionIdA}.jsonl`, + fileSize: 1024, + pending: true, + }, + { + sessionId: sessionIdB, + mtime: 100, + filePath: `/chats/${sessionIdB}.jsonl`, + fileSize: 1024, + pending: true, + }, + ], + hasMore: false, + }); + + const enriched = await sessionService.enrichSessions((await lite).items); + + expect(enriched).toHaveLength(1); + expect(enriched[0].sessionId).toBe(sessionIdA); + expect(enriched[0].pending).toBe(false); + expect(enriched[0].cwd).toBe('/test/project/root'); + expect(enriched[0].prompt).toBe('hello session a'); + }); + + it('should NOT populate messageCount during listing', async () => { + // Listing must avoid the full-file readline that counting requires + // — message counts are now lazy and provided by + // `countSessionMessages(sessionId)` only when a UI surface (e.g. + // a session preview) is about to display them. Pinning this + // contract here so future refactors can't quietly re-introduce + // the per-file scan that used to dominate /resume open time. + readdirSyncSpy.mockReturnValue([ + `${sessionIdA}.jsonl`, + ] as unknown as Array>); + statSyncSpy.mockReturnValue({ + mtimeMs: Date.now(), + isFile: () => true, + } as fs.Stats); + vi.mocked(jsonl.readLines).mockResolvedValue([recordA1]); + + const result = await sessionService.listSessions(); + + expect(result.items).toHaveLength(1); + expect(result.items[0].messageCount).toBeUndefined(); + }); + it('should truncate long prompts', async () => { const longPrompt = 'A'.repeat(300); const recordWithLongPrompt: ChatRecord = { @@ -209,8 +315,11 @@ describe('SessionService', () => { const result = await sessionService.listSessions(); - expect(result.items[0].prompt.length).toBe(203); // 200 + '...' - expect(result.items[0].prompt.endsWith('...')).toBe(true); + // `prompt` is now optional on SessionListItem (lite items omit + // it). After the listSessions wrapper enriches, it should be + // populated for valid same-project sessions. + expect(result.items[0].prompt!.length).toBe(203); // 200 + '...' + expect(result.items[0].prompt!.endsWith('...')).toBe(true); }); it('should paginate with size parameter', async () => { @@ -557,6 +666,117 @@ describe('SessionService', () => { }); }); + describe('countSessionMessages', () => { + // The lazy counter that replaces the per-file readline scan from + // listSessions. Four contracts to pin: it actually counts what it + // promises, it short-circuits on bad input without touching the disk, + // it returns 0 on any read failure (caller must not see an exception + // bubble up — the picker treats 0 as "unknown"), and it scopes to + // the current project (mirroring deleteSession/renameSession's + // first-record cwd check). + + const stubCreateReadStream = ( + lines: string[], + ): MockInstance => + vi + .spyOn(fs, 'createReadStream') + .mockImplementation( + () => Readable.from([lines.join('\n')]) as unknown as fs.ReadStream, + ); + + it('should count unique user/assistant uuids and ignore other record types', async () => { + // Project scoping reads the first record before the count stream; + // give it a record from this project so the count proceeds. + vi.mocked(jsonl.readLines).mockResolvedValue([recordA1]); + // Real countSessionMessagesFromPath routes each line through + // parseLineTolerant. The default mock is a no-op; for this test we + // need it to actually decode the JSON so the uuid set is populated. + vi.mocked(jsonl.parseLineTolerant).mockImplementation((line) => { + try { + const parsed = JSON.parse(line); + return Array.isArray(parsed) ? parsed : [parsed]; + } catch { + return []; + } + }); + const lines = [ + // Two user records sharing a uuid — should be counted once + JSON.stringify({ uuid: 'u1', type: 'user' }), + JSON.stringify({ uuid: 'u1', type: 'user' }), + JSON.stringify({ uuid: 'a1', type: 'assistant' }), + // System / summary records aren't messages + JSON.stringify({ uuid: 's1', type: 'system' }), + JSON.stringify({ uuid: 'sum1', type: 'summary' }), + // Empty and malformed lines must not throw + '', + ' ', + 'not-json', + JSON.stringify({ uuid: 'u2', type: 'user' }), + ]; + const createReadStreamSpy = stubCreateReadStream(lines); + + const count = await sessionService.countSessionMessages(sessionIdA); + + expect(count).toBe(3); // u1, a1, u2 + expect(createReadStreamSpy).toHaveBeenCalledTimes(1); + }); + + it('should return 0 for invalid sessionId without touching the filesystem', async () => { + const createReadStreamSpy = vi.spyOn(fs, 'createReadStream'); + + const count = await sessionService.countSessionMessages('not-a-uuid'); + + expect(count).toBe(0); + expect(createReadStreamSpy).not.toHaveBeenCalled(); + }); + + it('should return 0 when the session file is missing (ENOENT)', async () => { + // The first-record read fires before the count stream, so simulate + // ENOENT there too — readLines surfaces it as a thrown error. + vi.mocked(jsonl.readLines).mockRejectedValue( + Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), + ); + + const count = await sessionService.countSessionMessages(sessionIdA); + + expect(count).toBe(0); + }); + + it('should return 0 when the session belongs to a different project', async () => { + // A valid session ID can exist in the shared chats directory while + // its first-record cwd hashes to a different project. Lazy-count + // callers must not bypass project scoping. + const otherProjectRecord: ChatRecord = { + ...recordA1, + cwd: '/some/other/project', + }; + vi.mocked(jsonl.readLines).mockResolvedValue([otherProjectRecord]); + // Make the projectHash mock context-sensitive so the cwd check + // actually distinguishes projects. + vi.mocked(getProjectHash).mockImplementation((cwd) => + cwd === '/test/project/root' ? 'test-project-hash' : 'other-hash', + ); + const createReadStreamSpy = vi.spyOn(fs, 'createReadStream'); + + const count = await sessionService.countSessionMessages(sessionIdA); + + expect(count).toBe(0); + // No streaming pass should have started — the project check + // short-circuits before the expensive part. + expect(createReadStreamSpy).not.toHaveBeenCalled(); + }); + + it('should return 0 when the session file has no records (empty file)', async () => { + vi.mocked(jsonl.readLines).mockResolvedValue([]); + const createReadStreamSpy = vi.spyOn(fs, 'createReadStream'); + + const count = await sessionService.countSessionMessages(sessionIdA); + + expect(count).toBe(0); + expect(createReadStreamSpy).not.toHaveBeenCalled(); + }); + }); + describe('loadLastSession', () => { it('should return the most recent session (same as getLatestSession)', async () => { const now = Date.now(); diff --git a/packages/core/src/services/sessionService.ts b/packages/core/src/services/sessionService.ts index 9137dda5886..c182003920f 100644 --- a/packages/core/src/services/sessionService.ts +++ b/packages/core/src/services/sessionService.ts @@ -21,6 +21,7 @@ import type { import { uiTelemetryService } from '../telemetry/uiTelemetry.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { + LITE_READ_BUF_SIZE, readLastJsonStringFieldSync, readLastJsonStringFieldsSync, } from '../utils/sessionStorageUtils.js'; @@ -29,25 +30,49 @@ const debugLogger = createDebugLogger('SESSION'); /** * Session item for list display. - * Contains essential info extracted from the first record of a session file. + * + * The picker reads this in two phases: a "lite" first pass (just stat- + * level metadata: sessionId, mtime, filePath, fileSize) so the frame + * can render instantly, then an "enrich" pass that fills the head- + * derived (cwd, startTime, prompt, gitBranch) and tail-derived + * (customTitle, titleSource) fields. Every field beyond the stat- + * level minimum is optional because lite items omit them; consumers + * must guard accordingly. + * + * `pending: true` marks an item that hasn't been enriched yet — the + * UI uses it to show a placeholder row instead of an empty one. */ export interface SessionListItem { - /** Unique session identifier */ + /** Unique session identifier (always present, derived from filename). */ sessionId: string; - /** Working directory at session start */ - cwd: string; - /** ISO 8601 timestamp when session started */ - startTime: string; - /** File modification time (used for ordering and pagination) */ + /** File modification time (used for ordering and pagination). */ mtime: number; - /** First user prompt text (truncated for display) */ - prompt: string; - /** Git branch at session start, if available */ - gitBranch?: string; - /** Full path to the session file */ + /** Full path to the session file (always present). */ filePath: string; - /** Number of messages in the session (unique message UUIDs) */ - messageCount: number; + /** Size of the underlying file in bytes (cheap stat field). */ + fileSize?: number; + /** + * True while the item is still in lite form — head/tail metadata has + * not been read yet. Picker renders these as placeholders. Becomes + * `false` (or absent) once `enrichSessions` populates the rest. + */ + pending?: boolean; + /** Working directory at session start (populated during enrichment). */ + cwd?: string; + /** ISO 8601 timestamp when session started (populated during enrichment). */ + startTime?: string; + /** First user prompt text, truncated for display (populated during enrichment). */ + prompt?: string; + /** Git branch at session start, if available (populated during enrichment). */ + gitBranch?: string; + /** + * Number of unique-UUID user/assistant messages, when known. Listing + * does NOT compute this — counting requires a full readline pass over + * the JSONL and was the dominant cost of `listSessions` once a project + * accumulated many sessions. Use {@link SessionService.countSessionMessages} + * (or compute from the resumed conversation) when the count is needed. + */ + messageCount?: number; /** Custom title set via /rename or auto-generated by the title service, if any */ customTitle?: string; /** @@ -170,12 +195,15 @@ export class SessionService { * * Delegates to {@link readLastJsonStringFieldSync}, which scans the tail * window first (fast path; almost always hits because finalize() re-appends - * the title on every lifecycle event) and falls back to a full-file scan - * when the tail has no match. The `custom_title` line-marker guards against - * false matches from user content that happens to include a `customTitle` - * field. + * the title on every lifecycle event) and falls back to a bounded head + * window when the tail has no match. The `custom_title` line-marker guards + * against false matches from user content that happens to include a + * `customTitle` field. */ - private readSessionTitleFromFile(filePath: string): string | undefined { + private readSessionTitleFromFile( + filePath: string, + tailBuffer?: Buffer, + ): string | undefined { // Match only on actual custom_title system records. `'custom_title'` as // a loose substring can land on a user message that happens to contain // the literal "custom_title" (code review of this very file, etc.); @@ -186,6 +214,7 @@ export class SessionService { filePath, 'customTitle', '"subtype":"custom_title"', + tailBuffer, ); } @@ -199,7 +228,10 @@ export class SessionService { * introduced — callers treat `undefined` as equivalent to `'manual'` so a * user's pre-upgrade rename is never displayed as if it were auto-generated. */ - private readSessionTitleInfoFromFile(filePath: string): { + private readSessionTitleInfoFromFile( + filePath: string, + tailBuffer?: Buffer, + ): { title?: string; source?: TitleSource; } { @@ -208,6 +240,7 @@ export class SessionService { 'customTitle', ['titleSource'], '"subtype":"custom_title"', + tailBuffer, ); const title = hit['customTitle']; if (!title) return {}; @@ -339,14 +372,47 @@ export class SessionService { } /** - * Counts unique message UUIDs in a session file. - * This gives the number of logical messages in the session. + * Counts unique user/assistant message UUIDs in a session file by + * streaming the JSONL line-by-line. Each physical line is routed + * through `jsonl.parseLineTolerant` so a `}{`-glued line (#3606 + * corruption shape) still contributes both records, instead of being + * silently dropped. * - * Streams the file and routes each physical line through - * `jsonl.parseLineTolerant` so a `}{`-glued line (#3606 corruption shape) - * still contributes both records, instead of being silently dropped. + * Project-scoped: returns 0 if the file's first record belongs to a + * different project. Sibling public methods (deleteSession, + * renameSession, loadSession) apply the same first-record cwd check; + * mirroring it here keeps lazy-count callers from accidentally + * counting a session that lives in the shared chats directory but + * belongs to another project. + * + * This is intentionally NOT called from {@link listSessions} or + * {@link findSessionsByTitle} — it would be O(total bytes on disk) per + * picker open, dominating wall time once a project accumulates dozens + * of multi-MB sessions. Call this lazily, only when a specific + * session's message count is about to be displayed (e.g., from a + * preview panel) or computed from a resumed conversation. */ - private async countSessionMessages(filePath: string): Promise { + async countSessionMessages(sessionId: string): Promise { + if (!SESSION_FILE_PATTERN.test(`${sessionId}.jsonl`)) { + return 0; + } + const chatsDir = this.getChatsDir(); + const filePath = path.join(chatsDir, `${sessionId}.jsonl`); + + try { + const firstRecords = await jsonl.readLines(filePath, 1); + if (firstRecords.length === 0) return 0; + if (getProjectHash(firstRecords[0].cwd) !== this.projectHash) return 0; + } catch { + return 0; + } + + return this.countSessionMessagesFromPath(filePath); + } + + private async countSessionMessagesFromPath( + filePath: string, + ): Promise { const uniqueUuids = new Set(); try { const fileStream = fs.createReadStream(filePath); @@ -375,36 +441,90 @@ export class SessionService { } /** - * Lists sessions for the current project with pagination. + * Lists sessions for the current project with pagination — fully + * enriched (head + tail reads applied, project filter resolved). * - * Sessions are ordered by file modification time (most recent first). - * Uses cursor-based pagination with mtime as the cursor. + * Backward-compatible wrapper: composes {@link listSessionsLite} + + * {@link enrichSessions}. New callers that want the picker to render + * a first frame before any per-file IO completes should call those + * two methods directly so the lite phase can drive an immediate + * render and the enrichment can hydrate progressively. + */ + async listSessions( + options: ListSessionsOptions = {}, + ): Promise { + const { cursor, size = 20 } = options; + const enriched: SessionListItem[] = []; + let nextCursor: number | undefined = cursor; + let exhausted = false; + + // Some lite items drop out during enrichment (file disappeared + // mid-page, project-hash mismatch in a shared chats dir). Loop + // until we either fill the page or exhaust the disk — matches + // the pre-split behaviour where listSessions paged through + // filtered-out files automatically. + while (enriched.length < size && !exhausted) { + const lite = await this.listSessionsLite({ + cursor: nextCursor, + size: size - enriched.length, + }); + if (lite.items.length === 0) { + exhausted = true; + break; + } + const batch = await this.enrichSessions(lite.items); + enriched.push(...batch); + nextCursor = lite.nextCursor; + if (!lite.hasMore) { + exhausted = true; + } + } + + const items = enriched.slice(0, size); + const lastMtime = items[items.length - 1]?.mtime; + return { + items, + hasMore: !exhausted || enriched.length > size, + nextCursor: exhausted && enriched.length <= size ? undefined : lastMtime, + }; + } + + /** + * Phase 1 of the two-phase listing path: stat-only enumeration. + * + * Reads `chatsDir`, filters by the session-file naming pattern, and + * returns a page of `pending: true` items carrying just the cheap + * stat-level fields (sessionId, mtime, filePath, fileSize). NO + * JSONL parsing happens here, so this completes in roughly + * `O(readdir + N · stat)` — measured in ms even for thousands of + * sessions on local disk. * - * Only reads the first line of each JSONL file for efficiency. - * Files are filtered by UUID pattern first, then by project hash. + * The returned items are NOT project-filtered yet (the project-hash + * check requires reading the first record, which is enrichment + * territory). Callers that don't pass through {@link enrichSessions} + * will see sessions belonging to sibling projects when path + * sanitization causes a chats-dir collision; callers that DO pass + * through enrichment get the same filtered list as the legacy + * {@link listSessions} would. * - * @param options Pagination options - * @returns Paginated list of sessions + * Pagination uses the same `mtime`-cursor as `listSessions`. */ - async listSessions( + async listSessionsLite( options: ListSessionsOptions = {}, ): Promise { const { cursor, size = 20 } = options; const chatsDir = this.getChatsDir(); - // Get all valid session files (matching UUID pattern) with their stats - let files: Array<{ name: string; mtime: number }> = []; + let files: Array<{ name: string; mtime: number; size: number }> = []; try { const fileNames = fs.readdirSync(chatsDir); for (const name of fileNames) { - // Only process files matching session file pattern if (!SESSION_FILE_PATTERN.test(name)) continue; const filePath = path.join(chatsDir, name); try { const stats = fs.statSync(filePath); - files.push({ name, mtime: stats.mtimeMs }); + files.push({ name, mtime: stats.mtimeMs, size: stats.size }); } catch { - // Skip files we can't stat continue; } } @@ -415,83 +535,106 @@ export class SessionService { throw error; } - // Sort by mtime descending (most recent first) files.sort((a, b) => b.mtime - a.mtime); - - // Apply cursor filter (items with mtime < cursor) if (cursor !== undefined) { files = files.filter((f) => f.mtime < cursor); } - // Iterate through files until we have enough matching ones. - // Different projects may share the same chats directory due to path sanitization, - // so we need to filter by project hash and continue until we have enough items. - const items: SessionListItem[] = []; - let filesProcessed = 0; - let lastProcessedMtime: number | undefined; - let hasMoreFiles = false; + const cap = Math.min(size, MAX_FILES_TO_PROCESS); + const slice = files.slice(0, cap); + const hasMore = files.length > cap; + const lastMtime = slice[slice.length - 1]?.mtime; - for (const file of files) { - // Safety limit to prevent performance issues - if (filesProcessed >= MAX_FILES_TO_PROCESS) { - hasMoreFiles = true; - break; - } + const items: SessionListItem[] = slice.map((f) => ({ + sessionId: f.name.replace(/\.jsonl$/, ''), + mtime: f.mtime, + filePath: path.join(chatsDir, f.name), + fileSize: f.size, + pending: true, + })); - // Stop if we have enough items - if (items.length >= size) { - hasMoreFiles = true; - break; - } - - filesProcessed++; - lastProcessedMtime = file.mtime; + return { + items, + hasMore, + nextCursor: hasMore ? lastMtime : undefined, + }; + } - const filePath = path.join(chatsDir, file.name); - const records = await jsonl.readLines( - filePath, - MAX_PROMPT_SCAN_LINES, - ); + /** + * Phase 2 of the two-phase listing path: hydrate a batch of lite + * items with head- and tail-derived metadata. + * + * For each item this: + * 1. Reads the head (`MAX_PROMPT_SCAN_LINES` lines via jsonl.readLines) + * to recover the first record (cwd, startTime, gitBranch) and + * the first user prompt. + * 2. Drops the item if the first record's cwd hashes to a + * different project — same defensive filter the pre-split + * `listSessions` did, just deferred. + * 3. Tail-reads (pooled buffer) for customTitle / titleSource. + * 4. Returns a fresh, fully populated SessionListItem with + * `pending` cleared. + * + * Items already enriched (`pending` falsy) are returned unchanged. + * Items that fail enrichment (missing/empty file, wrong project) + * are silently dropped from the result. Callers that need to know + * which items vanished should diff the input vs output by + * `sessionId`. + */ + async enrichSessions(items: SessionListItem[]): Promise { + if (items.length === 0) return items; - if (records.length === 0) continue; - const firstRecord = records[0]; + // Pool one tail buffer for the whole batch. Same rationale as the + // pre-split implementation; mirrors claude-code's enrichLogs. + const tailBuffer = Buffer.alloc(LITE_READ_BUF_SIZE); + const out: SessionListItem[] = []; - // Skip if not matching current project - // We use cwd comparison since first record doesn't have projectHash - const recordProjectHash = getProjectHash(firstRecord.cwd); - if (recordProjectHash !== this.projectHash) continue; + for (const item of items) { + if (!item.pending) { + out.push(item); + continue; + } + const enriched = await this.enrichOne(item, tailBuffer); + if (enriched) out.push(enriched); + } - // Count messages for this session - const messageCount = await this.countSessionMessages(filePath); + return out; + } - const prompt = this.extractFirstPromptFromRecords(records); + private async enrichOne( + item: SessionListItem, + tailBuffer: Buffer, + ): Promise { + const records = await jsonl.readLines( + item.filePath, + MAX_PROMPT_SCAN_LINES, + ); + if (records.length === 0) return undefined; + const firstRecord = records[0]; - const titleInfo = this.readSessionTitleInfoFromFile(filePath); - items.push({ - sessionId: firstRecord.sessionId, - cwd: firstRecord.cwd, - startTime: firstRecord.timestamp, - mtime: file.mtime, - prompt, - gitBranch: firstRecord.gitBranch, - filePath, - messageCount, - customTitle: titleInfo.title, - titleSource: titleInfo.source, - }); + if (getProjectHash(firstRecord.cwd) !== this.projectHash) { + return undefined; } - // Determine next cursor (mtime of last processed file) - // Only set if there are more files to process - const nextCursor = - hasMoreFiles && lastProcessedMtime !== undefined - ? lastProcessedMtime - : undefined; + const prompt = this.extractFirstPromptFromRecords(records); + const titleInfo = this.readSessionTitleInfoFromFile( + item.filePath, + tailBuffer, + ); return { - items, - nextCursor, - hasMore: hasMoreFiles, + sessionId: firstRecord.sessionId, + mtime: item.mtime, + filePath: item.filePath, + fileSize: item.fileSize, + cwd: firstRecord.cwd, + startTime: firstRecord.timestamp, + prompt, + gitBranch: firstRecord.gitBranch, + customTitle: titleInfo.title, + titleSource: titleInfo.source, + // messageCount intentionally omitted; see SessionListItem. + pending: false, }; } @@ -895,6 +1038,10 @@ export class SessionService { // are deterministic even when multiple files share an mtime. files.sort((a, b) => b.mtime - a.mtime || a.name.localeCompare(b.name)); + // Pool the tail-read buffer across files; the title scan in the loop + // body is otherwise the dominant alloc cost when many candidates exist. + const tailBuffer = Buffer.alloc(LITE_READ_BUF_SIZE); + let filesProcessed = 0; for (const file of files) { if (filesProcessed >= MAX_FILES_TO_PROCESS) break; @@ -904,8 +1051,8 @@ export class SessionService { // Cheap check first: tail-read the title and skip non-matches before // doing the full hydration work (first-record read, project filter, - // message count, prompt extraction). - const titleInfo = this.readSessionTitleInfoFromFile(filePath); + // prompt extraction). + const titleInfo = this.readSessionTitleInfoFromFile(filePath, tailBuffer); if (titleInfo.title?.toLowerCase().trim() !== normalizedTitle) continue; const records = await jsonl.readLines( @@ -918,7 +1065,6 @@ export class SessionService { const recordProjectHash = getProjectHash(firstRecord.cwd); if (recordProjectHash !== this.projectHash) continue; - const messageCount = await this.countSessionMessages(filePath); const prompt = this.extractFirstPromptFromRecords(records); matches.push({ @@ -929,7 +1075,8 @@ export class SessionService { prompt, gitBranch: firstRecord.gitBranch, filePath, - messageCount, + // messageCount intentionally omitted; see SessionListItem for + // the rationale and `countSessionMessages` for on-demand use. customTitle: titleInfo.title, titleSource: titleInfo.source, }); diff --git a/packages/core/src/utils/sessionStorageUtils.test.ts b/packages/core/src/utils/sessionStorageUtils.test.ts index 0e526367736..ac450431936 100644 --- a/packages/core/src/utils/sessionStorageUtils.test.ts +++ b/packages/core/src/utils/sessionStorageUtils.test.ts @@ -201,14 +201,16 @@ describe('sessionStorageUtils', () => { ).toBe('new'); }); - it('falls back to full-file scan when tail has no match (Phase 2)', () => { - // Build a file whose custom_title record is near the start, followed by - // enough filler bytes (> LITE_READ_BUF_SIZE) that the tail window is - // entirely filler. The old head+tail reader would have hit this via the - // head window; this test verifies the new tail-first + full-scan - // strategy still resolves it. + it('falls back to head window when tail has no match', () => { + // Tail-first + head-fallback strategy: the title record sits in + // the first 64KB but is pushed out of the last 64KB by enough + // filler. The reader resolves it via the head scan without ever + // touching the middle of the file — bounded I/O regardless of + // file size. (Modern sessions don't reach this branch; the + // ChatRecordingService re-anchor invariant keeps the title in + // the tail. This is the legacy / pre-invariant safety net.) const titleLine = - '{"subtype":"custom_title","customTitle":"buried-in-middle"}'; + '{"subtype":"custom_title","customTitle":"in-head-window"}'; const filler = '{"type":"user","message":"' + 'x'.repeat(256) + '"}'; // ~4x the tail window, guaranteed to push the title line out of tail. const fillerCount = Math.ceil((LITE_READ_BUF_SIZE * 4) / filler.length); @@ -218,32 +220,45 @@ describe('sessionStorageUtils', () => { Array.from({ length: fillerCount }, () => filler).join('\n') + '\n'; - const p = writeFile('phase2.jsonl', content); + const p = writeFile('head-fallback.jsonl', content); expect(fs.statSync(p).size).toBeGreaterThan(LITE_READ_BUF_SIZE * 3); expect( readLastJsonStringFieldSync(p, 'customTitle', 'custom_title'), - ).toBe('buried-in-middle'); - }); - - it('returns the last occurrence even when multiple land in the full-scan region', () => { - const early = '{"subtype":"custom_title","customTitle":"first-rename"}'; - const middle = '{"subtype":"custom_title","customTitle":"second-rename"}'; - const filler = '{"type":"user","message":"' + 'x'.repeat(256) + '"}'; - const fillerCount = Math.ceil((LITE_READ_BUF_SIZE * 3) / filler.length); - + ).toBe('in-head-window'); + }); + + it('returns undefined when title is buried beyond both head and tail windows', () => { + // Anti-test for the previous Phase-2 full-file scan: a title + // record stranded in the middle of a >2× tail-window file is + // intentionally NOT found. The contract changed — listing + // latency is bounded to 2 × LITE_READ_BUF_SIZE per file at the + // cost of giving up on legacy sessions whose writer never + // re-anchored the title. Callers downgrade to firstPrompt. + const padTo = (label: string, byteCount: number) => { + const filler = + '{"type":"user","message":"' + + 'x'.repeat(Math.max(0, byteCount - 30)) + + '"}'; + return label + '\n' + filler + '\n'; + }; + + // Layout: 80KB filler, then the title (>= LITE_READ_BUF_SIZE + // from offset 0), then 80KB more filler (>= LITE_READ_BUF_SIZE + // from EOF). Title falls in neither window. + const buryWindow = LITE_READ_BUF_SIZE + 16 * 1024; + const titleLine = + '{"subtype":"custom_title","customTitle":"buried-out-of-reach"}'; const content = - early + - '\n' + - middle + + padTo('{"type":"user"}', buryWindow) + + titleLine + '\n' + - Array.from({ length: fillerCount }, () => filler).join('\n') + - '\n'; + padTo('{"type":"user"}', buryWindow); - const p = writeFile('phase2-multi.jsonl', content); + const p = writeFile('buried.jsonl', content); expect( readLastJsonStringFieldSync(p, 'customTitle', 'custom_title'), - ).toBe('second-rename'); + ).toBeUndefined(); }); it('respects the lineContains filter when scanning', () => { @@ -260,7 +275,11 @@ describe('sessionStorageUtils', () => { ).toBe('legit'); }); - it('returns undefined when neither phase finds the field', () => { + it('returns undefined when neither head nor tail contains the field', () => { + // Same shape as the legacy "no title anywhere" case — the + // file is a long stream of user records with no metadata. + // Both windows scan in vain; we return undefined cheaply + // instead of paying for a full-file scan. const line = '{"type":"user","message":"' + 'x'.repeat(512) + '"}'; const lineCount = Math.ceil((LITE_READ_BUF_SIZE * 3) / line.length); const content = @@ -281,6 +300,91 @@ describe('sessionStorageUtils', () => { readLastJsonStringFieldSync(p, 'customTitle', 'custom_title'), ).toBe('last'); }); + + it('does not pick up a customTitle from a partial trailing line in the head window', () => { + // The head buffer is a fixed 64KB slice — its last bytes can fall + // mid-record. Without trimming to the last newline, the extractor + // sees a partial line whose `customTitle` value happens to be + // closed within the buffer, picks it up as the latest match, and + // returns the (possibly-misleading) value from a record we never + // saw end. The fix drops everything past the final newline before + // running the extractor, so only complete lines vote. + // + // Layout: + // line1: a complete custom_title record at offset 0 + // line2: a record that begins inside the head window with both + // `"customTitle":"phantom"` and `"subtype":"custom_title"` + // fully visible (and a closed value), but whose body + // extends >64KB so its trailing `\n` is past the head + // boundary + // filler: pads file size past 2× LITE_READ_BUF_SIZE so head + // fallback runs and tail has no match + const line1 = + '{"type":"system","subtype":"custom_title","customTitle":"complete"}\n'; + const line2Prefix = + '{"type":"system","subtype":"custom_title","customTitle":"phantom","filler":"'; + // Make line2 long enough that its closing `"}\n` is past the head + // window (LITE_READ_BUF_SIZE = 64KB). 70KB of `x` guarantees that. + const line2 = + line2Prefix + 'x'.repeat(LITE_READ_BUF_SIZE + 8 * 1024) + '"}\n'; + // Push file size past 2 × LITE_READ_BUF_SIZE so listSessions-style + // callers go through head fallback (tail has no match). + const tailFiller = + '{"type":"user","message":"' + + 'a'.repeat(LITE_READ_BUF_SIZE + 4 * 1024) + + '"}\n'; + const p = writeFile( + 'partial-line-head.jsonl', + line1 + line2 + tailFiller, + ); + + // Head trim drops the partial line2 prefix; only the complete + // line1 contributes a match. Without the fix, "phantom" would + // win by virtue of being later in the buffer. + expect( + readLastJsonStringFieldSync(p, 'customTitle', 'custom_title'), + ).toBe('complete'); + }); + + it('reuses a caller-provided scratch buffer across tail and head reads', () => { + // Smoke test for the buffer-pool plumbing: when the caller hands + // in a scratch buffer (as `listSessions` does on every page), the + // function must produce the same result as the no-buffer path. + // The same buffer backs the tail read AND the head fallback, so + // a tail-then-head sequence on different file sizes must not + // leak data between reads — bytes-read bounds the decode, never + // the buffer's full capacity. + const big = writeFile( + 'big.jsonl', + '{"subtype":"custom_title","customTitle":"big-file"}\n', + ); + const small = writeFile( + 'small.jsonl', + '{"subtype":"custom_title","customTitle":"x"}\n', + ); + + const scratch = Buffer.alloc(LITE_READ_BUF_SIZE); + // Pre-fill the buffer with a sentinel byte so a buggy decode that + // reads past `bytesRead` would produce a corrupted return value. + scratch.fill(0x55); + + expect( + readLastJsonStringFieldSync( + big, + 'customTitle', + 'custom_title', + scratch, + ), + ).toBe('big-file'); + expect( + readLastJsonStringFieldSync( + small, + 'customTitle', + 'custom_title', + scratch, + ), + ).toBe('x'); + }); }); describe('extractLastJsonStringFields', () => { @@ -444,13 +548,16 @@ describe('sessionStorageUtils', () => { ).toEqual({ customTitle: 'A', titleSource: 'auto' }); }); - it('falls through to full-file scan when tail has no match and finds the pair', () => { - // Primary+secondary near start, filler > LITE_READ_BUF_SIZE, nothing in tail. + it('falls through to head window when tail has no match and finds the pair', () => { + // Primary+secondary near start, filler > LITE_READ_BUF_SIZE + // pushes them out of the tail. The head window catches the + // pair atomically — both fields come from the same line, the + // whole point of the multi-field variant. const header = '{"subtype":"custom_title","customTitle":"X","titleSource":"auto"}\n'; const filler = '{"type":"user","message":"' + 'x'.repeat(LITE_READ_BUF_SIZE) + '"}\n'; - const p = writeFile('phase2.jsonl', header + filler); + const p = writeFile('head-fallback.jsonl', header + filler); expect( readLastJsonStringFieldsSync( p, @@ -461,39 +568,11 @@ describe('sessionStorageUtils', () => { ).toEqual({ customTitle: 'X', titleSource: 'auto' }); }); - it('handles records straddling a Phase-2 chunk boundary', () => { - // Goal: place the winning custom_title record so it begins in Phase-2 - // chunk N and ends in chunk N+1. That exercises the `carry` logic in - // readLastJsonStringFieldsSync — without it, the partial first chunk - // wouldn't contain the closing quote and the match would be missed. - // - // Layout: - // [padA bytes..............][custom_title line][padB bytes................] - // with padA + fixed header bytes + half of the title line < - // LITE_READ_BUF_SIZE, and the rest of the title line spilling into - // the next chunk. padB is >> tail window so Phase-2 fires (tail miss). - const titleLine = - '{"subtype":"custom_title","customTitle":"spanner","titleSource":"manual"}'; - const userHeader = '{"type":"user","message":"'; - const userFooter = '"}\n'; - // Position the title line so its middle lands on the chunk boundary. - const padALen = - LITE_READ_BUF_SIZE - - userHeader.length - - userFooter.length - - Math.floor(titleLine.length / 2); - const padA = 'a'.repeat(padALen); - const padB = 'b'.repeat(LITE_READ_BUF_SIZE * 2); + it('does not let a truncated trailing partial record win', () => { const p = writeFile( - 'straddle.jsonl', - userHeader + - padA + - userFooter + - titleLine + - '\n' + - userHeader + - padB + - userFooter, + 'truncated.jsonl', + '{"subtype":"custom_title","customTitle":"A","titleSource":"auto"}\n' + + '{"subtype":"custom_title","customTitle":"B', ); expect( readLastJsonStringFieldsSync( @@ -502,23 +581,48 @@ describe('sessionStorageUtils', () => { ['titleSource'], 'custom_title', ), - ).toEqual({ customTitle: 'spanner', titleSource: 'manual' }); + ).toEqual({ customTitle: 'A', titleSource: 'auto' }); }); - it('does not let a truncated trailing partial record win', () => { - const p = writeFile( - 'truncated.jsonl', - '{"subtype":"custom_title","customTitle":"A","titleSource":"auto"}\n' + - '{"subtype":"custom_title","customTitle":"B', + it('reuses a caller-provided scratch buffer across tail and head reads', () => { + // Mirror of the single-field variant's pool test. The multi-field + // path runs the same buffer through tail-then-head, so a buggy + // decode that ignored `bytesRead` would observe sentinel bytes + // left from the previous (larger) read and corrupt one of the + // fields. Drive a tail-hit followed by a head-fallback on a + // smaller file, sharing the buffer across both calls. + const tailHit = writeFile( + 'tail-pair.jsonl', + '{"subtype":"custom_title","customTitle":"big","titleSource":"manual"}\n', ); + const header = + '{"subtype":"custom_title","customTitle":"x","titleSource":"auto"}\n'; + const filler = + '{"type":"user","message":"' + 'y'.repeat(LITE_READ_BUF_SIZE) + '"}\n'; + const headFallback = writeFile('head-pair.jsonl', header + filler); + + const scratch = Buffer.alloc(LITE_READ_BUF_SIZE); + scratch.fill(0x55); + expect( readLastJsonStringFieldsSync( - p, + tailHit, 'customTitle', ['titleSource'], 'custom_title', + scratch, ), - ).toEqual({ customTitle: 'A', titleSource: 'auto' }); + ).toEqual({ customTitle: 'big', titleSource: 'manual' }); + + expect( + readLastJsonStringFieldsSync( + headFallback, + 'customTitle', + ['titleSource'], + 'custom_title', + scratch, + ), + ).toEqual({ customTitle: 'x', titleSource: 'auto' }); }); }); }); diff --git a/packages/core/src/utils/sessionStorageUtils.ts b/packages/core/src/utils/sessionStorageUtils.ts index 3c76e528eeb..47defdec654 100644 --- a/packages/core/src/utils/sessionStorageUtils.ts +++ b/packages/core/src/utils/sessionStorageUtils.ts @@ -16,16 +16,6 @@ import fs from 'node:fs'; /** Size of the head/tail buffer for lite metadata reads (64KB). */ export const LITE_READ_BUF_SIZE = 64 * 1024; -/** - * Maximum size (bytes) we'll scan in the Phase-2 full-file fallback. Tail- - * read fast path covers the realistic case (metadata is re-appended on every - * session lifecycle event). A pathological / corrupt session file that's - * tens of GB should NOT block the picker for minutes while we scan it all. - * The session picker renders on the main event loop, so blocking I/O here - * freezes the UI. - */ -export const MAX_FULL_SCAN_BYTES = 64 * 1024 * 1024; - /** * Flags used when opening session files for metadata reads. `O_NOFOLLOW` * refuses to follow symlinks — defense in depth so a symlink planted in @@ -233,36 +223,50 @@ export function extractLastJsonStringField( } // --------------------------------------------------------------------------- -// File I/O — tail-first scan with full-file fallback +// File I/O — tail-first scan with head-window fallback // --------------------------------------------------------------------------- /** * Reads a JSON string field value from a JSONL file, returning the latest * occurrence (last in file order). * - * Two-phase strategy: - * 1. Scan the last LITE_READ_BUF_SIZE bytes of the file; if the field is - * present, return it immediately. This is the common path because - * ChatRecordingService.finalize() re-appends metadata records to EOF - * on every session lifecycle event, keeping the latest title near the - * end of the file. - * 2. If the tail window has no match, stream the entire file in chunks - * and return the last hit. This guarantees we never miss a record that - * landed between the head and tail windows in a large file — a blind - * spot the previous head+tail approach had. + * Two bounded windows, never a full-file scan: + * 1. Scan the last LITE_READ_BUF_SIZE bytes of the file. This is the + * common path because `ChatRecordingService` re-anchors metadata + * records to EOF every {@link TITLE_REANCHOR_BYTES} (≤ tail-window + * size) and on every lifecycle event (turn end, session switch, + * shutdown, resume). + * 2. If the tail has no match, scan the FIRST LITE_READ_BUF_SIZE bytes + * of the file. The metadata record set on a brand-new session lands + * near offset 0 before any user/assistant turns push it forward, so + * the head window catches the legacy case where a session was + * created on a build prior to the re-anchor invariant. * - * Phase 2 is a full-file scan and is intentionally slower; it is only paid - * when Phase 1 misses. + * If neither window contains the field, returns `undefined`. Callers + * that need a stronger guarantee must arrange for the writer to + * maintain the head-or-tail invariant — by design we never trade + * picker latency for completeness here. * - * Returns `undefined` on any I/O error or when the field is not found. + * Worst-case I/O: 2 × LITE_READ_BUF_SIZE = 128KB per file, fixed. * * @param lineContains Optional substring that must appear on the same line * as the matched field. See {@link extractLastJsonStringField}. + * @param scratchBuffer Optional caller-owned Buffer reused across many + * files in the same listing pass. Must be at least + * {@link LITE_READ_BUF_SIZE} bytes; only the leading `length` bytes + * are touched and decoded each call, so old data past the read region + * is never observed (we never read past the bytes we just wrote). + * The same buffer backs both the tail and head reads — they happen + * sequentially, so reuse is safe. When omitted, the function + * allocates per-call — preserves the simple call site for one-off + * reads (rename, single-session lookup) while letting `listSessions` + * skip the per-file alloc. */ export function readLastJsonStringFieldSync( filePath: string, key: string, lineContains?: string, + scratchBuffer?: Buffer, ): string | undefined { let fd: number | undefined; try { @@ -272,62 +276,52 @@ export function readLastJsonStringFieldSync( fd = fs.openSync(filePath, getReadOpenFlags()); - // Phase 1: tail window — fast path. + // Phase 1: tail window — fast path. This is where every well-behaved + // session keeps its current title (ChatRecordingService re-anchors + // it within the tail window). const tailLength = Math.min(fileSize, LITE_READ_BUF_SIZE); const tailOffset = fileSize - tailLength; - const tailBuffer = Buffer.alloc(tailLength); - const tailBytes = fs.readSync(fd, tailBuffer, 0, tailLength, tailOffset); + const buffer = + scratchBuffer && scratchBuffer.length >= LITE_READ_BUF_SIZE + ? scratchBuffer + : Buffer.alloc(LITE_READ_BUF_SIZE); + const tailBytes = fs.readSync(fd, buffer, 0, tailLength, tailOffset); if (tailBytes > 0) { - const tailText = tailBuffer.toString('utf-8', 0, tailBytes); + const tailText = buffer.toString('utf-8', 0, tailBytes); const tailHit = extractLastJsonStringField(tailText, key, lineContains); if (tailHit !== undefined) { return tailHit; } } - // If the whole file already fit in the tail window, there is nothing left - // to scan. + // If the whole file fit in the tail window, head == tail; nothing more + // to do. if (tailOffset === 0) return undefined; - // Phase 2: stream the file up to MAX_FULL_SCAN_BYTES and return the last - // hit. Scanning from offset 0 (rather than [0, tailOffset)) avoids the - // edge case where a single record straddles the Phase 1/Phase 2 boundary - // — duplicate work on the tail bytes is harmless because we only care - // about the final match. The hard cap bounds worst-case latency for - // pathologically large session files (which would freeze the picker). - let lastHit: string | undefined; - let readOffset = 0; - let carry = ''; - const scanLimit = Math.min(fileSize, MAX_FULL_SCAN_BYTES); - while (readOffset < scanLimit) { - const toRead = Math.min(LITE_READ_BUF_SIZE, scanLimit - readOffset); - const buf = Buffer.alloc(toRead); - const bytesRead = fs.readSync(fd, buf, 0, toRead, readOffset); - if (bytesRead === 0) break; - readOffset += bytesRead; - - const chunk = carry + buf.toString('utf-8', 0, bytesRead); - const lastNewline = chunk.lastIndexOf('\n'); - if (lastNewline < 0) { - // No newline yet — the entire chunk is a partial line; keep carrying. - carry = chunk; - continue; + // Phase 2: head window — fallback for legacy sessions and the + // edge case where the title got written near offset 0 and the + // re-anchor invariant hasn't kicked in yet (e.g. a session + // recorded by a build that predates the re-anchor logic). + const headLength = Math.min(fileSize, LITE_READ_BUF_SIZE); + const headBytes = fs.readSync(fd, buffer, 0, headLength, 0); + if (headBytes > 0) { + const rawHead = buffer.toString('utf-8', 0, headBytes); + // Drop the trailing partial line: a record that started inside the + // head window but whose closing quote lives past 64KB would be + // silently skipped by the extractor (no terminating `"` before EOS). + // For boundary-straddling pre-invariant records, that means the title + // is lost. Truncating at the last newline keeps us on whole lines. + const headText = + headBytes < fileSize + ? rawHead.slice(0, rawHead.lastIndexOf('\n') + 1) + : rawHead; + const headHit = extractLastJsonStringField(headText, key, lineContains); + if (headHit !== undefined) { + return headHit; } - - const complete = chunk.slice(0, lastNewline + 1); - carry = chunk.slice(lastNewline + 1); - - const hit = extractLastJsonStringField(complete, key, lineContains); - if (hit !== undefined) lastHit = hit; - } - - // Final trailing line without a newline terminator. - if (carry) { - const hit = extractLastJsonStringField(carry, key, lineContains); - if (hit !== undefined) lastHit = hit; } - return lastHit; + return undefined; } catch { return undefined; } finally { @@ -359,6 +353,7 @@ export function readLastJsonStringFieldsSync( primaryKey: string, otherKeys: string[], lineContains?: string, + scratchBuffer?: Buffer, ): Record { const emptyResult: Record = {}; emptyResult[primaryKey] = undefined; @@ -372,13 +367,17 @@ export function readLastJsonStringFieldsSync( fd = fs.openSync(filePath, getReadOpenFlags()); - // Phase 1: tail window fast path. + // Phase 1: tail window fast path. See the single-field variant for + // the head-or-tail invariant and buffer-pool semantics. const tailLength = Math.min(fileSize, LITE_READ_BUF_SIZE); const tailOffset = fileSize - tailLength; - const tailBuffer = Buffer.alloc(tailLength); - const tailBytes = fs.readSync(fd, tailBuffer, 0, tailLength, tailOffset); + const buffer = + scratchBuffer && scratchBuffer.length >= LITE_READ_BUF_SIZE + ? scratchBuffer + : Buffer.alloc(LITE_READ_BUF_SIZE); + const tailBytes = fs.readSync(fd, buffer, 0, tailLength, tailOffset); if (tailBytes > 0) { - const tailText = tailBuffer.toString('utf-8', 0, tailBytes); + const tailText = buffer.toString('utf-8', 0, tailBytes); const hit = extractLastJsonStringFields( tailText, primaryKey, @@ -390,46 +389,27 @@ export function readLastJsonStringFieldsSync( if (tailOffset === 0) return emptyResult; - // Phase 2: stream the file up to MAX_FULL_SCAN_BYTES, track the latest - // match. Hard cap bounds worst-case latency on pathological files. - let latest: Record | undefined; - let readOffset = 0; - let carry = ''; - const scanLimit = Math.min(fileSize, MAX_FULL_SCAN_BYTES); - while (readOffset < scanLimit) { - const toRead = Math.min(LITE_READ_BUF_SIZE, scanLimit - readOffset); - const buf = Buffer.alloc(toRead); - const bytesRead = fs.readSync(fd, buf, 0, toRead, readOffset); - if (bytesRead === 0) break; - readOffset += bytesRead; - const chunk = carry + buf.toString('utf-8', 0, bytesRead); - const lastNewline = chunk.lastIndexOf('\n'); - if (lastNewline < 0) { - carry = chunk; - continue; - } - const complete = chunk.slice(0, lastNewline + 1); - carry = chunk.slice(lastNewline + 1); - - const hit = extractLastJsonStringFields( - complete, - primaryKey, - otherKeys, - lineContains, - ); - if (hit[primaryKey] !== undefined) latest = hit; - } - if (carry) { + // Phase 2: head window — fallback for legacy sessions written + // before the title-anchor invariant existed. + const headLength = Math.min(fileSize, LITE_READ_BUF_SIZE); + const headBytes = fs.readSync(fd, buffer, 0, headLength, 0); + if (headBytes > 0) { + const rawHead = buffer.toString('utf-8', 0, headBytes); + // Truncate to whole lines — see the single-field variant for why. + const headText = + headBytes < fileSize + ? rawHead.slice(0, rawHead.lastIndexOf('\n') + 1) + : rawHead; const hit = extractLastJsonStringFields( - carry, + headText, primaryKey, otherKeys, lineContains, ); - if (hit[primaryKey] !== undefined) latest = hit; + if (hit[primaryKey] !== undefined) return hit; } - return latest ?? emptyResult; + return emptyResult; } catch { return emptyResult; } finally {