-
Notifications
You must be signed in to change notification settings - Fork 3k
perf(cli): Cache persisted session catalogs #8892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
617e124
9b79a19
aa12aa9
4855eb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Session List persisted catalog cache | ||
|
|
||
| ## Problem | ||
|
|
||
| Organized and metadata-filtered session lists must load the complete persisted session catalog before applying organization, source, ordering, and cursor rules. Concurrent `all`, `pinned`, source-filtered, and LiveTask requests currently repeat the same JSONL and worktree-sidecar reads. The synchronous portions of those scans amplify event-loop lag when a workspace has many or large transcripts. | ||
|
|
||
| ## Design | ||
|
|
||
| The daemon keeps a process-local catalog snapshot keyed by the resolved session runtime root, the exact workspace identity, and active versus archived state. Query, group, source, cursor, trust, and live-merge options are intentionally excluded because they do not change persisted catalog contents. | ||
|
|
||
| The first request installs an in-flight Promise before starting the loader. Concurrent requests for the same generation await that Promise. A successful catalog, including worktree sidecars, remains available for two seconds from scan completion. Organization and every live bridge field are merged after lookup on every request. Default numeric pagination and the separate session-info counter do not use the catalog cache. | ||
|
|
||
| Each scope has a generation. Explicit metadata, close, delete, archive, and unarchive operations invalidate the affected states. An invalidated in-flight load may finish for requests that already joined it, but its generation cannot repopulate the cache. Failures are never cached and there is no stale-on-error fallback. | ||
|
|
||
| Metadata title persistence remains asynchronous in the bridge extension method. Invalidation prevents a pre-mutation generation from being installed, while the live merge exposes the new title immediately; it does not claim that the metadata response waits for durable JSONL persistence. A scan racing that background write can still publish its older file view for the normal two-second snapshot lifetime. | ||
|
|
||
| The cache retains at most 50,000 summaries across all workspaces. A snapshot larger than the limit is still returned to its current waiters but is not installed. Expiry timers are unreferenced and identity-checked, and the oldest completed snapshots are evicted before a new snapshot would exceed the limit. | ||
|
|
||
| ## Consistency and isolation | ||
|
|
||
| Daemon-managed callers pass the selected runtime root explicitly, and the complete read runs in that pinned Storage context. Secondary runtimes never fall back to the primary runtime. Read-only trust policy remains request-scoped; sharing the persisted snapshot does not enable live merging or debug logging. | ||
|
|
||
| The cache is not a filesystem transaction. Unknown writers can update a file after that file was read but before the snapshot finishes. Such a snapshot can remain visible for two seconds after publication. Live session state and organization changes are not subject to that window. | ||
|
|
||
| ## Observability | ||
|
|
||
| Request spans distinguish physical scans, cache hits, and single-flight waiters before awaiting the shared Promise, so failures retain their cache status. Successful lookups also record archive state, query kind, summary count, scan pages, truncation, and either leader scan duration or cache age. Paths, session identifiers, titles, and source identifiers are never attached. | ||
|
|
||
| ## Out of scope | ||
|
|
||
| This change does not alter public protocols, Web Shell polling, the session-info scan, cross-workspace scan scheduling, core filesystem APIs, or daemon timeout policy. The outer lifecycle timeout fix remains necessary for a single slow cold scan. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7390,6 +7390,93 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('session mutations invalidate active and archived organized catalogs', async () => { | ||
| await withRuntimeDir(async () => { | ||
| const sessionId = '550e8400-e29b-41d4-a716-446655440017'; | ||
| await writeStoredSession(sessionId); | ||
| const connId = await initialize(); | ||
| const stream = await openStream(connId); | ||
| const reader = frameReader(stream); | ||
|
|
||
| const list = async ( | ||
| id: number, | ||
| archiveState: 'active' | 'archived', | ||
| ) => { | ||
| await post(connId, { | ||
| jsonrpc: '2.0', | ||
| id, | ||
| method: 'session/list', | ||
| params: { | ||
| workspaceCwd: TEST_WORKSPACE, | ||
| view: 'organized', | ||
| archiveState, | ||
| }, | ||
| }); | ||
| return reader.next(); | ||
| }; | ||
|
|
||
| await expect(list(75, 'active')).resolves.toMatchObject({ | ||
| result: { sessions: [{ sessionId }] }, | ||
| }); | ||
| await expect(list(76, 'archived')).resolves.toMatchObject({ | ||
| result: { sessions: [] }, | ||
| }); | ||
|
|
||
| await post(connId, { | ||
| jsonrpc: '2.0', | ||
| id: 77, | ||
| method: '_qwen/sessions/archive', | ||
| params: { sessionIds: [sessionId] }, | ||
| }); | ||
| expect(await reader.next()).toMatchObject({ | ||
| id: 77, | ||
| result: { archived: [sessionId], errors: [] }, | ||
| }); | ||
|
|
||
| await expect(list(78, 'active')).resolves.toMatchObject({ | ||
| result: { sessions: [] }, | ||
| }); | ||
| await expect(list(79, 'archived')).resolves.toMatchObject({ | ||
| result: { sessions: [{ sessionId, isArchived: true }] }, | ||
| }); | ||
|
|
||
| await post(connId, { | ||
| jsonrpc: '2.0', | ||
| id: 80, | ||
| method: '_qwen/sessions/unarchive', | ||
| params: { sessionIds: [sessionId] }, | ||
| }); | ||
| expect(await reader.next()).toMatchObject({ | ||
| id: 80, | ||
| result: { unarchived: [sessionId], errors: [] }, | ||
| }); | ||
| await expect(list(81, 'active')).resolves.toMatchObject({ | ||
| result: { sessions: [{ sessionId, isArchived: false }] }, | ||
| }); | ||
| await expect(list(82, 'archived')).resolves.toMatchObject({ | ||
| result: { sessions: [] }, | ||
| }); | ||
|
|
||
| await post(connId, { | ||
| jsonrpc: '2.0', | ||
| id: 83, | ||
| method: '_qwen/sessions/delete', | ||
| params: { sessionIds: [sessionId] }, | ||
| }); | ||
| expect(await reader.next()).toMatchObject({ | ||
| id: 83, | ||
| result: { removed: [sessionId], errors: [] }, | ||
| }); | ||
| await expect(list(84, 'active')).resolves.toMatchObject({ | ||
| result: { sessions: [] }, | ||
| }); | ||
| await expect(list(85, 'archived')).resolves.toMatchObject({ | ||
| result: { sessions: [] }, | ||
| }); | ||
|
Comment on lines
+7473
to
+7475
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The delete step of this new over-the-wire test cannot observe invalidation of the archived catalog: the archived catalog's last warm before delete ( 中文说明这个新的 over-the-wire 测试的 delete 步骤无法观察到 archived 目录的失效:delete 之前 archived 目录最后一次预热( — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| reader.close(); | ||
| }); | ||
| }); | ||
|
|
||
| it('_qwen/session/update_organization assigns a color echoed by session/list', async () => { | ||
| await withRuntimeDir(async () => { | ||
| const sessionId = '550e8400-e29b-41d4-a716-446655440011'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import path from 'node:path'; | ||||||||||||||||||||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||||||||||||||||||||
| import { SessionNotFoundError } from '@qwen-code/acp-bridge/bridgeErrors'; | ||||||||||||||||||||
| import type { | ||||||||||||||||||||
|
|
@@ -31,6 +32,7 @@ const sessionSources = vi.hoisted( | |||||||||||||||||||
| const removeSessionMock = vi.hoisted(() => | ||||||||||||||||||||
| vi.fn(async (_sessionId: string) => true), | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| const removeSessionRuntimeBaseDirs = vi.hoisted(() => new Array<string>()); | ||||||||||||||||||||
| const listWorkspaceSessionsForResponse = vi.hoisted(() => vi.fn()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { | ||||||||||||||||||||
|
|
@@ -71,6 +73,7 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { | |||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| removeSession(sessionId: string) { | ||||||||||||||||||||
| removeSessionRuntimeBaseDirs.push(actual.Storage.getRuntimeBaseDir()); | ||||||||||||||||||||
| return removeSessionMock(sessionId); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }, | ||||||||||||||||||||
|
|
@@ -247,13 +250,15 @@ function makeHarness() { | |||||||||||||||||||
| const runtime = { | ||||||||||||||||||||
| workspaceId: 'conversations', | ||||||||||||||||||||
| workspaceCwd: '/conversations', | ||||||||||||||||||||
| sessionRuntimeBaseDir: '/runtime/conversations', | ||||||||||||||||||||
| provenance: 'live-conversation', | ||||||||||||||||||||
| bridge, | ||||||||||||||||||||
| } as WorkspaceRuntime; | ||||||||||||||||||||
| const projectBridge = { ...bridge } as AcpSessionBridge; | ||||||||||||||||||||
| const projectRuntime = { | ||||||||||||||||||||
| workspaceId: 'project-1', | ||||||||||||||||||||
| workspaceCwd: '/project', | ||||||||||||||||||||
| sessionRuntimeBaseDir: '/runtime/project', | ||||||||||||||||||||
| bridge: projectBridge, | ||||||||||||||||||||
| } as WorkspaceRuntime; | ||||||||||||||||||||
| const registry = { | ||||||||||||||||||||
|
|
@@ -291,6 +296,7 @@ function makeHarness() { | |||||||||||||||||||
| return { | ||||||||||||||||||||
| service, | ||||||||||||||||||||
| bridge, | ||||||||||||||||||||
| projectBridge, | ||||||||||||||||||||
| runtime, | ||||||||||||||||||||
| summaries, | ||||||||||||||||||||
| resident, | ||||||||||||||||||||
|
|
@@ -307,6 +313,7 @@ beforeEach(() => { | |||||||||||||||||||
| parentSessions.clear(); | ||||||||||||||||||||
| sessionSources.clear(); | ||||||||||||||||||||
| removeSessionMock.mockClear(); | ||||||||||||||||||||
| removeSessionRuntimeBaseDirs.length = 0; | ||||||||||||||||||||
| listWorkspaceSessionsForResponse.mockReset(); | ||||||||||||||||||||
| listWorkspaceSessionsForResponse.mockResolvedValue({ | ||||||||||||||||||||
| sessions: [], | ||||||||||||||||||||
|
|
@@ -365,6 +372,23 @@ describe('LiveTaskService', () => { | |||||||||||||||||||
| ], | ||||||||||||||||||||
| threads: [{ id: 'ordinary', status: 'idle', updatedAt: 1_785_369_601 }], | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| expect(listWorkspaceSessionsForResponse).toHaveBeenNthCalledWith( | ||||||||||||||||||||
| 1, | ||||||||||||||||||||
| harness.bridge, | ||||||||||||||||||||
| '/conversations', | ||||||||||||||||||||
| expect.objectContaining({ view: 'organized', group: 'all' }), | ||||||||||||||||||||
| { runtimeBaseDir: '/runtime/conversations' }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| expect(listWorkspaceSessionsForResponse).toHaveBeenNthCalledWith( | ||||||||||||||||||||
| 2, | ||||||||||||||||||||
| harness.projectBridge, | ||||||||||||||||||||
| '/project', | ||||||||||||||||||||
| expect.objectContaining({ view: 'organized', group: 'all' }), | ||||||||||||||||||||
| { runtimeBaseDir: '/runtime/project' }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
doudouOUC marked this conversation as resolved.
|
||||||||||||||||||||
| expect(listWorkspaceSessionsForResponse.mock.calls[1]?.[0]).toBe( | ||||||||||||||||||||
| harness.projectBridge, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
Comment on lines
+389
to
+391
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The R1-7 fix pins per-runtime bridge pairing by identity only for the SECOND list call; the first call's bridge argument remains constrained only by structural equality, which cannot discriminate the two harness bridges because
Suggested change
中文说明R1-7 的修复只用身份相等固定了第二次 list 调用的 per-runtime bridge 配对;第一次调用的 bridge 参数仍只受结构相等约束,而 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||||||||||||||||||||
| expect(harness.bridge.spawnOrAttach).not.toHaveBeenCalled(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -897,6 +921,10 @@ describe('LiveTaskService', () => { | |||||||||||||||||||
| expect(harness.bridge.killSession).toHaveBeenCalledWith('new-task', { | ||||||||||||||||||||
| requireZeroAttaches: true, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| expect(removeSessionMock).toHaveBeenCalledWith('new-task'); | ||||||||||||||||||||
| expect(removeSessionRuntimeBaseDirs).toEqual([ | ||||||||||||||||||||
| path.resolve('/runtime/conversations'), | ||||||||||||||||||||
| ]); | ||||||||||||||||||||
| expect(harness.sendPrompt).not.toHaveBeenCalled(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-1 (round-1 finding, re-verified this round, still standing): the ACP
session/closeand_qwen/session/update_metadatainvalidation call sites have no over-the-wire test — the new transport test covers only_qwen/sessions/{archive,unarchive,delete}. The round-1 deferral is acknowledged; re-reporting because the gap is unchanged at this commit. — Failure scenario: re-verified by probe this round — deleting bothinvalidateSessionLists(['active'])call sites leaves all 346 ACP tests green (transport.test.ts + workspace-qualified-acp.test.ts). An ACP client that closes a session (or updates its metadata) and re-lists within two seconds would then be served the stale pre-mutation persisted snapshot — the regression the REST-side close/metadata tests were written to catch. Suggested fix: extend the transport test (or add a sibling) to warm an organizedsession/list, runsession/closeand_qwen/session/update_metadata, and assert the immediate re-list reflects the post-mutation state without asserting synchronous title durability.中文说明
R1-1(第一轮发现,本轮重新验证后仍然存在):ACP
session/close与_qwen/session/update_metadata的失效调用点没有 over-the-wire 测试——新的 transport 测试只覆盖了_qwen/sessions/{archive,unarchive,delete}。已知悉第一轮的延期决定;因该缺口在本提交仍未变化,故再次报告。——失败场景:本轮探针重新验证——删除两处invalidateSessionLists(['active'])调用点后,全部 346 个 ACP 测试仍为绿(transport.test.ts + workspace-qualified-acp.test.ts)。ACP 客户端关闭会话(或更新其 metadata)后两秒内重新 list,将得到变更前的过期持久化快照——这正是 REST 侧 close/metadata 测试要防范的回归。建议修复:扩展 transport 测试(或新增同级测试),先预热 organizedsession/list,再执行session/close与_qwen/session/update_metadata,断言紧随其后的重新 list 反映变更后状态,但不要求标题同步落盘。— qwen3.8-max via Qwen Code /review (v0.21.9)