Conversation
Listing walked the projects directory and every transcript one await at a time, so a full scan serialized its round trips through the thread pool instead of overlapping them — and the catalog is listed once per search term, so the walk is paid again on files that did not change. With 1000 local transcripts the warm relist drops from ~20ms to ~5ms here, and the first listing from ~155ms to ~120ms. The directory walk, the per-file stats, and the summary derivations are now concurrent. Behavior is unchanged: per-item failures are still swallowed individually, the newest-wins dedup is order-independent (its tie-break names the path, not the iteration order), and the final sort is stable over the same input order. #parse also reuses the size the cache check already stat'ed rather than stat'ing the same file twice. Generated-by: ZCode
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
Findings ordered by severity. Line numbers are the head revision.
P1 (Must-Fix) — the new fan-out makes peak memory proportional to the whole transcript corpus, defeating the bound this module documents.
listSessions now derives every summary at once (packages/storage/src/claude-code-session-adapter.ts:123), and each derivation reads its transcript in full (#parse → readFile, :296). The same file documents the opposite invariant for this constant:
CLAUDE_TRANSCRIPT_MAX_BYTES = 64 * 1024 * 1024— "Bounded for the same reason the Codex rollout cap exists: a single hostile or runaway file must not be able to exhaust the Host's memory during an import the user asked for." (:59-61)
That cap bounds one file. Sequential iteration bounded the aggregate by holding one transcript at a time; the fan-out removes that aggregate bound, and the transcript count is unbounded (the class's own comment cites 1128, :91).
Evidence — a standalone Node reproduction of the same read shape (1128 files × 256 KB = 282 MB; every file far below the 64 MB per-file cap):
Promise.all (this PR): peak RSS +578.9 MiB
sequential (base): peak RSS +33.2 MiB (17.4x)
There is a second, quieter consequence on the same path. The fan-out opens one file per transcript simultaneously, and #parse swallows a failed read (catch { return undefined; }, :294-299). #summaryOf then caches that miss under an unchanged key (:162, :178), so a read that fails transiently (e.g. EMFILE under N simultaneous opens) removes the session from the catalog for the process lifetime, while readSession — which bypasses the cache — still returns it. That is the list/read disagreement this adapter is built to prevent.
Smallest sound fix: bound in-flight derivations with one small worker pool (K ≈ 8) in listSessions and in #transcriptFiles (which fans out readdir/stat the same way, :219, :243). The fs work runs on libuv's 4-thread pool, so K ≈ 8 keeps the measured warm speedup while capping peak buffers and file descriptors. RUNTIME_HOST_MAX_IN_FLIGHT_DOMAIN_REQUESTS = 64 (packages/runtime-host/src/protocol/index.ts:396) is the in-repo precedent for capping in-flight work.
P2 (Should-Fix) — the added test does not exercise this change.
The new test (packages/storage/src/__tests__/claude-code-session-adapter.test.ts) pins newest-wins dedup, which already holds on origin/main; the PR checklist itself concedes no test fails without the change. Nothing covers a batch larger than two files (set completeness and order under concurrency) or the read-failure path above. Either bound the fan-out and add a batch-listing test, or state that the resource regression is accepted — but the current test cannot detect a concurrency regression here.
P3 (Nice-to-have) — a second stat per transcript remains on the hot path.
#transcriptFiles stats every candidate for mtimeMs and discards the result, then #summaryOf stats the same path again (:154); knownSize only removes a third stat. Returning { path, sessionId, mtimeMs, size } from #transcriptFiles removes the redundant round trip and the extra parameter.
Review-relevant risks. No public contract, wire shape, security boundary, dependency, licensing, or release effect was identified in the diff. The observable change is confined to the Host's foreign-session catalog.
Required conclusion.
- Optimal for the actual problem? Not yet. The goal (stop serializing waiting) is right, but unbounded
Promise.alltrades away a documented memory bound for latency a bounded pool already delivers. - Production code that can be deleted?
none identified. - Low-quality tests to delete or replace?
none identified— the new test is sound; it needs the batch and partial-failure coverage noted above. - Deeper refactor required? No. One shared bounded-concurrency mapper in this file, with
#transcriptFilesreturning{ path, sessionId, mtimeMs, size }. - Ready to merge? Not while the fan-out is unbounded (P1).
- Residual risks / verification gaps: the description reports latency only, not peak memory or descriptors — the dimension this change regresses. I did not run the package suite; the above is from reading the change against
origin/mainplus the standalone reproduction.
Approval boundary. This is automated review; it is not an approval and must not be read as authorization to merge. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
An unbounded Promise.all held every transcript's bytes and one open file descriptor per session at once, so peak memory grew with the corpus instead of staying proportional to the per-file cap this module already enforces — measured +412–451 MiB RSS on 1000 × 256 KB transcripts, against +5 MiB sequential. Simultaneous opens also made a transient EMFILE likely, and a read that fails is cached as a missing session until restart. The walk, the stats, and the summary derivations now run through a pool of 8, which keeps libuv's default thread pool saturated while capping peak buffers and descriptors. Measured on the same corpus: warm relist 23 ms → 10 ms (was 7 ms unbounded), first listing 222 ms → 166–181 ms — faster than the unbounded version, whose allocation pressure was eating its own gain. Adds a batch test past the pool width pinning completeness and update order. Generated-by: ZCode
|
Thanks for the review — the P1 fan-out was a real regression and is fixed in 6c16ee2. Findings answered below. P1 (unbounded fan-out / aggregate memory): fixed. All three fan-outs (
Peak memory is now proportional to the pool width × the per-file cap rather than the corpus, and at most 8 descriptors are open at once. The warm-path speedup survives (23–24 ms → 10–11 ms), and the bounded version is faster on the first listing than the unbounded one was — the allocation pressure was eating its own gain. EMFILE / cached-miss consequence: trigger removed, semantics unchanged. With ≤ 8 in-flight opens, a listing can no longer open one descriptor per session, which was what made a transient EMFILE plausible on this path. The cache-a-failed-read behavior itself predates this PR (main caches a P2 (test coverage): partially addressed. Added a batch test past the pool width (24 transcripts across 3 projects) pinning listing completeness and update order under concurrent derivation, alongside the newest-wins dedup test. What I did not add is a read-failure-path test: EMFILE is not portably simulable from P3 (second stat per transcript): accepted as follow-up. Agreed the walker could return Verification for the current push:
Generated-by: ZCode |
me2seeks
left a comment
There was a problem hiding this comment.
Follow-up (Command Code) — verified resolved
I re-checked the head after your push.
The P1 is fixed. The listing now derives summaries through mapWithConcurrency with a bounded lane pool instead of an unbounded Promise.all, so peak buffers and open descriptors are capped rather than scaling with the whole transcript corpus. That is the shape I asked for.
The P2 is addressed too — the adapter suite gained focused cases covering the concurrent listing path, so the change is no longer pinned only by a test that holds on main.
The remaining P3 (the second stat per transcript; knownSize still present) is a nice-to-have I would not block on.
Thanks — that resolved the resource regression. (Automated review; not an approval.)
The walk already stat'ed every transcript it selected, and the size cap in #parse already took that stat's size — but the cache check in #summaryOf stat'ed the same path again, so a warm relist (once per search term) paid two stats per transcript where one answers both. #transcriptFiles now returns the mtime and size its single stat observed, and #summaryOf keys the cache on that observation. Within a listing the newest-wins choice and the cache key are now one observation of one file rather than two that can disagree; a transcript appended between walk and read lands under the older key and is re-read by the next listing, which sees the new mtime. readSession also reuses the walk's size for the byte cap. Warm relist on 1000 × 256 KB transcripts: 10–11 ms → 6–7 ms (was 23–24 ms sequential). Generated-by: ZCode
|
Follow-up: P3 is now implemented in 320f639 as well, so the listing does one
The cache-key semantics note from my previous comment applies as designed: the dedup choice and the cache key are now one observation of one file rather than two that can disagree. A transcript appended between the walk and the read lands in the cache under the older key and is re-read by the next listing, which sees the new mtime — this is documented on Re-measured on the same corpus (1000 × ~256 KB, 2 runs):
The warm relist — the once-per-search-term path — is now ~3.5× sequential. Verification unchanged: storage suite 1216 pass / 0 fail, typecheck and biome clean. Generated-by: ZCode |
…ries Upstream rewrote this adapter twice since this branch diverged — streaming transcript reads with head/tail summary windows (apache#5240) and Host-unified external session import paging (apache#5308) — so the conflict resolution re-applies this PR's three changes onto the new shape: - Bounded concurrency (LIST_CONCURRENCY = 8) for the walk, the stats, and the summary derivations; the derivation fan-out now runs in chunks whose boundaries double as early-exit points, preserving the per-page budget upstream added (at most one chunk of extra derivations past a full page, all of them cache entries the next page reuses). - One stat per transcript: the walk returns the mtime and size its single stat observed, and #summaryOf keys the cache on that observation instead of stat'ing again. All four pre-commit checks were run manually for this merge commit: biome (2.5.13, matching the merged package.json) over every staged file, ASF headers, and whitespace pass; the protocol epoch check reports a known false positive in --staged mode on merge commits (it treats upstream's historical declarations as newly added), and this branch touches no protocol files — the CI merge-result check is the authoritative gate. Generated-by: ZCode
|
Main moved under this branch — the adapter was rewritten upstream (#5240 streaming transcript reads with head/tail summary windows, #5308 Host-unified external-session paging) — so the conflict is resolved by re-porting this PR's changes onto the new shape rather than textual merge (0321e90):
Re-measured on the same 1000 × 256 KB corpus, against current main (which already includes upstream's window redesign):
The two adapter tests were re-ported: the newest-wins dedup test now uses the walk-order semantics upstream added (mtime-descending catalog), and the batch test past the pool width pins completeness and that order under chunked derivation. Upstream's per-page-budget test passes unchanged, which is the early-exit behavior the chunking must preserve. Verification: storage suite 1407 pass / 0 fail; typecheck, biome (2.5.13), ASF headers clean. One note on the merge commit: the pre-commit protocol epoch check reports a known false positive in Generated-by: ZCode |
Summary
Listing Claude Code transcripts walked the projects directory and every transcript one
awaitat a time, so a full scan serialized its round trips through the thread pool instead of overlapping them — and as the adapter's own comments note, the catalog is listed once per search term, so the walk is paid again on files that did not change.The directory walk, the per-file stats, and the summary derivations are now concurrent through a bounded pool (
LIST_CONCURRENCY = 8):listSessionsderives summaries viamapWithConcurrencyinstead of oneawaitper file.#summaryOfnever throws and only touches the summary cache at distinct keys (one file per session id), so the calls are independent; results keep the original order and the final sort is stable.#transcriptFileslists project directories and stats candidate files through the same pool. The newest-wins dedup stays a sequential, order-independent reduce (its tie-break names the path, not the iteration order), so completion order cannot change which file wins.#parse's byte cap.#summaryOfandreadSessionreuse the walk's observation instead of stat'ing again. Within a listing the dedup choice and the cache key are one observation of one file rather than two that can disagree; a transcript appended between walk and read lands under the older key and is re-read by the next listing, which sees the new mtime.The bound matters: an unbounded
Promise.allheld every transcript's bytes and one open descriptor per session at once, so peak memory grew with the corpus — the opposite of what the 64 MB per-file cap enforces. The pool keeps libuv's default thread pool saturated while capping peak buffers and descriptors at 8 in flight.Measured on 1000 seeded transcripts (20 projects, ~256 KB each):
Tests: one pinning the previously-untested dedup rule (one session id under two project directories must resolve to the newest copy in both list and read), and one past the pool width (24 transcripts) pinning listing completeness and update order under concurrent derivation.
Verification
npm --workspace @maka/storage run typecheck— cleannpm --workspace @maka/storage run test:dist— 1216 pass / 0 failnpx biome checkon the changed files — cleanAI use
Select exactly one:
Tool(s) and scope: ZCode authored the change, the tests, and this description; all benchmark and memory numbers are from real local runs.
Checklist
Note: this is a behavior-preserving performance change, so no test fails without it; the two new tests pin the invariants (newest-wins dedup; completeness and order under concurrent derivation) that the implementation must uphold.
Does this PR entail a change in behavior?