fix(agent-core-v2): cache workspace alias resolution across calls - #3325
Conversation
resolveAliasIds re-read the workspace catalog and the whole session index from disk on every call, so the by_workspace grouping loop and the per-workspace session counts paid repeated full-file reads per workspace per request (~2.4s per 50-group page at 1.1k workspaces, 23 pages serially during a client startup drain). Cache both files as precomputed snapshots (by-id map plus a root-key -> alias ids index) invalidated by the storage watch events, which cover atomic rewrites and cross-process writes; storage backends without watch fall back to reading through. The first resolution primes the workspace merge via IWorkspaceService.list() so the cached catalog matches what WorkspaceService.get() would have returned.
|
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abcbe5ea13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ication Concurrent cold resolveAliasIds callers (the /workspaces route fans out per-workspace counts with Promise.all) all passed the cache check before any caller finished loading, re-running the full catalog and session index reads the cache exists to avoid; memoize the in-flight load promise so a cold batch shares one read. Also capture the invalidation generation before each read and publish the snapshot only when it is unchanged, so a mid-read file replacement cannot leave a stale snapshot installed over the watch invalidation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b26483749f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…nce owner A debounced fs watch was the only invalidation channel for the alias catalog snapshot, so an in-process catalog write stayed invisible to resolveAliasIds for up to the watch debounce window while the previous read-through code observed every completed write immediately. IWorkspacePersistence now exposes onDidChange: FileWorkspacePersistence fires it synchronously on save and re-fires the underlying document watch (covering atomic rewrites and cross-process writers), and the aliases service subscribes to it instead of watching raw storage keys. The session index snapshot keeps the filesystem watch, matching the read-side ownership of that file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3f7bc3cfd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…writes A flushed session_index.jsonl append was invisible to resolveAliasIds for up to the fs-watch debounce window, so a sessions request issued right after a session create could resolve the workspace's aliases from the pre-append snapshot. IAppendLogStore now publishes onDidWrite after each durable flush (append batches and rewrites), and the aliases service drops its session-index snapshot through that event; the raw filesystem watch stays as the channel for cross-process writers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 843aad2b00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ites Once a key has a LogState, every global flush() (WireService flushes after ordinary agent persistence) completed it successfully and fired onDidWrite unconditionally, so idle agent activity kept dropping the alias session-index snapshot and forced full re-reads of an unchanged index. drain() now reports whether it appended anything and the write event fires only when a flush actually persisted a batch or a rewrite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88d4413269
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Callers joining an in-flight single-flight load after a completed write still received the pre-write snapshot: the generation check only guarded cache publication, not the value returned to awaiters. Each load now carries the generation it started at, and catalog()/sessionIndex() re-read (coalesced through the same single-flight) when the settled load's generation is stale.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 872a7c4706
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…fails A drain that persisted one batch and then failed the next threw without recording the durable write, so onDidWrite never fired for records that were in fact persisted (the alias session-index snapshot then missed its synchronous invalidation). The write box now threads through the whole owned flush: each successful batch marks it, and the event fires before the failure propagates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e1f241447
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The per-snapshot retry guarded each read on its own, so a write landing between the catalog and session-index reads returned an alias set assembled across two generations. resolveAliasIds now captures the generation once, reads both snapshots together, and retries the whole resolution when either input was invalidated mid-flight. The spanned-write test is reworked to gate after the load (so the snapshot content genuinely predates the write), and a new case covers the cross-generation mix directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9100b68751
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A resident chokidar watcher per server on the shared home directory degraded watch delivery for unrelated files under test-suite boot volume (the prompts suite lost the config.toml reload race and the catalog missed a just-written model). In-process appends were already covered synchronously by the append-log write event; cross-process writers now surface through a per-call size comparison on the append-only file, which costs one stat per resolve and needs no resident watcher.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3411fd134b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
1 participant
Related Issue
No linked issue — the problem was measured directly on a 1.1k-workspace install (client HAR traces plus server-side timing).
Problem
GET /api/v2/sessions?view=by_workspacere-computes the full grouping on every page, and the grouping loop callsresolveAliasIdsonce per unique workspace — each call re-readworkspaces.json(twice, viaWorkspaceService.get()andIWorkspacePersistence.load()) and re-read+parsed the wholesession_index.jsonl, with zero caching and serialized throughWorkspaceService's exclusive lock.GET /api/v1/workspacespays the same per-workspace cost for itssession_counts.Measured with 1122 workspaces / 1852 sessions (synthetic home shaped from a real install):
/workspaces: ~2.45sWhat changed
WorkspaceAliasesServicenow caches both inputs as precomputed snapshots — a by-id map plus a root-key → alias ids index — soresolveAliasIdsis O(1) after the first call:watchevents (directory-level, debounced), which cover atomic tmp+rename rewrites and cross-process writers (CLI and server share the same home). Backends withoutwatchfall back to reading through, so no caching semantics change there.IWorkspaceService.list(), so the cached catalog matches whatWorkspaceService.get()would have returned (merge runs once per process; later cache misses just re-read the catalog).fileWorkspacePersistenceexports the catalog scope/key constants for the watch subscription (same pattern as the session-index constants).Measured after the fix, same data: group pages 0.009–0.12s,
/workspaces0.11s warm, full 23-page drain 1.9s; client startup gate 7.7s → 2.4s. One ~3.4s one-time cost remains per server process (read-model build + initial catalog merge), unchanged by this PR.Tests: two new cases in the existing
workspaceAliasesService.test.ts— zero disk reads after warm, and cache pickup of catalog/session-index changes via the file watchers. Related suites (workspaceAliases, workspaceSessions, workspace, kap-server v2Sessions) all pass; lint and typecheck clean.Checklist
/approve). — N/A, no linked issue (measured directly).gen-changesetsskill — no changeset by decision: the user-facing entry ships with the app changelog (kimi-code-app#433).gen-docsskill, or this PR needs no doc update.