fix(agent-adapter,engine): stream codex rollout reads and evict expired history caches - #466
fix(agent-adapter,engine): stream codex rollout reads and evict expired history caches#466Zerlight wants to merge 3 commits into
Conversation
…ed history caches
There was a problem hiding this comment.
Pull request overview
This PR reduces Codex rollout memory usage and improves history-cache cleanup.
Changes:
- Streams rollout summaries with bounded concurrency.
- Adds filename-based transcript lookup.
- Evicts expired history entries and clears Codex stream state on teardown.
- Adds regression tests for parsing, lookup, and cache eviction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Summary |
|---|---|
packages/host/engine/src/session/history-service.ts |
Adds expired-cache eviction. |
packages/host/engine/src/__tests__/history-service.test.ts |
Tests cache eviction behavior. |
packages/host/agent-adapter/src/native/codex/history.ts |
Implements streaming summaries and fast lookup; retains unbounded prompt and marker text during summarization. |
packages/host/agent-adapter/src/native/codex/adapter.ts |
Clears streamed state during teardown. |
packages/host/agent-adapter/src/__tests__/codex-history-files.test.ts |
Tests rollout parsing and lookup paths. |
Suppressed comments (4)
packages/host/agent-adapter/src/native/codex/history.ts:296
- The workers append summaries as each file finishes, so this changes the result order from the collected-file order to completion order.
CodexAdapter.listHistorysorts only byupdatedAt; sessions with equal or missing timestamps therefore get a race-dependent order, which can move entries between cursor pages across identical list calls. Preserve each summary's original file index (or apply a deterministic tie-breaker) before returning.
if (summary) summaries.push(summary);
packages/host/agent-adapter/src/native/codex/history.ts:485
- This no longer preserves the old message-count rule for user rows: the previous code skipped rows whose rendered text was empty, but this only checks
content.length. Image-only rows and whitespace-only text therefore incrementmessageCount(and can make list summaries report phantom user messages). Skip rows with no non-empty text as well, e.g. by requiringuserRow.previewto be present.
if (isSyntheticCodexUserDigest(userRow.digest, promptTexts) || userRow.empty) continue;
messageCount += 1;
if (userRow.preview !== undefined) firstUserText ??= userRow.preview;
packages/host/engine/src/session/history-service.ts:280
- This sweep is only invoked at the start of
listandread, so expiry remains lazy. If the last request loads a large transcript (or runs longer thanttlMs) and no later history request arrives, the expiredeventCacheentry stays strongly referenced for the daemon's lifetime; the added test only exercises a later request. Use a timer/periodic sweep or otherwise remove entries when they expire if this is meant to reclaim memory.
private sweepExpired(now: number): void {
for (const [key, entry] of this.listCache) {
if (entry.expiresAt <= now) this.listCache.delete(key);
}
packages/host/engine/src/session/history-service.ts:283
- Sweeping
eventCachedoes not remove the companionhistoryCwdByIdentries. Every cwd-scoped list adds one of these keys, andclear()is the only removal, so repeated history lists can still grow this map without bound after both cache entries expire. Tie cwd mappings to an expiry/live list reference (while preserving the cwd needed for cold reads) instead of evicting only the event entry.
for (const [key, entry] of this.eventCache) {
if (entry.expiresAt <= now) this.eventCache.delete(key);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
ℹ️ Minor suggestions inline.
The fix holds up. I read the full diff and then chased the four things that could have made it wrong; all four came back clean:
- Streaming fidelity —
forEachJsonlRowreproduces the whole-file pass exactly forid/cwd/model/createdAt/updatedAt/messageCount/title/ empty-file-undefined, including out-of-orderevent_msgrows. Deferring user rows intoPendingUserRowand deciding synthetic-marker rescue in a post-stream pass is equivalent to the oldcollectCodexPromptTextspre-pass. - Error safety — I probed this rather than assuming it:
createReadStreamENOENT/EISDIR does surface through the readline async iterator into the outercatch, so a session that codex rotates or archives out from under a read still reads as empty instead of throwing an unhandled'error'. No fd leak across 500 sequential or 200-file × 8-way concurrent reads. - Fast path can't return the wrong transcript — basename-as-id is covered by
name === id,endsWithfalse positives die on thesummary.id === id || summary.path === idcheck, and every miss falls through to the full scan. - Cache sweep is behavior-preserving —
expiresAt <= nowandexpiresAt > noware exact complements, so sweeping before thegetcan only remove entries the freshness check would have rejected anyway. Verified againstpartialCursorpagination,invalidateEventCacheFromList,sessionFingerprint,importSession,resumeHistory,forceRefresh. LeavinghistoryCwdByIdunswept is right — it's a small id→cwd map, not a transcript.
Two things worth a look that have no diff line to anchor to, then nitpicks.
The concurrency bound is per-invocation, not per-process
SUMMARY_READ_CONCURRENCY = 8 caps one readCodexTranscriptSummaries call. HistoryService has no in-flight request coalescing — I checked; there are only the four agentHistoryOperation('history.list' | 'history.read', …) call sites and no pending-promise map — so N concurrent cache-missing calls fan out to 8×N concurrent streams.
That is the exact shape of the incident in the PR body: mobile auto-resume plus repeated attach/detach, each attach seeding a transcript, all of them missing a 30s TTL that has just expired. The per-file residual is now a fraction of file size instead of the whole file, so 8×N is survivable where the old unbounded scan wasn't — this isn't a hole in the fix. But if CODE-605 recurs at lower amplitude, coalescing identical in-flight list/read keys in HistoryService is the next lever, not a smaller constant here.
readHistory is still O(file)
CodexAdapter.readHistory still does readJsonlFile(summary.path) → all rows → mapCodexHistoryEvents → all events, and only then sliceHistoryEventPage. The PR body cites two 60 MB files in the synthetic corpus, so the 2350 MB → 67 MB measurement plausibly covers the summary/list pass only — a single readHistory on a 60 MB rollout still materialises the file plus its full event array. Is leaving the single-transcript path unstreamed a deliberate scope call for this PR? Attach-time transcript seeding goes through readHistory, so it's on the same hot path that produced the OOM.
Nitpicks
readCodexTranscriptSummariesno longer returns results in corpus order — workers push as they finish. The oldPromise.all(files.map(...))did preserve input order, solistHistory's(b.updatedAt ?? 0) - (a.updatedAt ?? 0)sort over it was deterministic for ties (V8's sort is stable). Sessions with an identicalupdatedAtcan now shuffle between paginatedlistHistorycalls and duplicate or skip across a page boundary. Ties are rare sinceupdatedAtfalls back to file mtime, so this is genuinely small — a.sort()onfilesbefore the pool, or a tiebreak onid, closes it.findCodexTranscriptwalks the rollout tree twice on a fast-path miss (once forcandidates, again insidereadCodexTranscriptSummaries). Passing the already-collectedfilesthrough would save the second traversal.forEachJsonlRow's innertrywraps theonRow(parsed)call as well asJSON.parse, so a throwing callback is silently misattributed as a corrupt line. Same structure as the old code, so not a regression — just noting it.streamedTextLen.clear()inteardown()is redundant at normal turn end:handleTurnCompletedalready clears the map and then callsteardown(). It only bites on cancel/finalizeServer, and the entries are numbers keyed by item id, so the leak plugged is tiny. Correct, just narrower than the comment implies.
Mode: Review (initial) · Files reviewed: 5 · Commits reviewed: 1 · Base master · Head ruocheng/code-605 (88ccb00) · Prior pullfrog review: none
Claude Opus | 𝕏
… lookup fast path
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/host/agent-adapter/src/native/codex/history.ts:314
- The bounded workers append summaries in file-completion order, so this changes the result order from the previous
Promise.all(files.map(...))behavior.CodexAdapter.listHistorysorts only byupdatedAt(adapter.ts:610-615); when timestamps are equal or absent, the stable-sort order is now timing-dependent, which can move entries across cursor-paginated pages after a refresh. Preserve each summary's file index (or add a deterministic tie-breaker) before returning.
const summaries: CodexTranscriptSummary[] = [];
const workers = createFixedArray(Math.min(SUMMARY_READ_CONCURRENCY, files.length)).map(
async () => {
for (let file = files.pop(); file !== undefined; file = files.pop()) {
// eslint-disable-next-line no-await-in-loop -- the loop is one bounded-concurrency worker.
const summary = await readCodexTranscriptSummary(file, index);
if (summary) summaries.push(summary);
There was a problem hiding this comment.
✅ No new issues found.
Both follow-ups landed cleanly, and I verified each rather than taking the reply at face value.
- The fast-path test now has teeth. I re-ran my exact mutation (
if (fastHit && Date.now() < 0) return fastHit;) against9a23b19: the test fails atexpect(opened).not.toContain(decoyPath)because the fallback full scan opens the decoy rollout. Asserting on which files were opened is a better guard than the distinguishable-content approach I suggested — it pins the actual property the fix is for (one file read instead of the corpus) instead of just the returned answer. - The fingerprint swap is exactly behavior-preserving. Hashing is applied on both sides at all three comparison sites,
echoedFingerprint === ''is never compared (theif (!digest.hasImage) return trueguard precedes it, matching the oldechoedTextsentinel), andcollectCodexPromptFingerprints'sif (message)guard means neither the old nor the new set can hold the empty value.isSyntheticCodexUserPayload's second parameter kept itsReadonlySet<string>type through the rename, so a stale raw-text call site would have typechecked silently — there are exactly two callers, both inhistory.ts, both passing fingerprint sets. - Retention is genuinely bounded now. What survives one row's callback is
promptFingerprints(~43 B per real prompt),userRows(~300 B per entry — hashes plus apreviewthatpreviewTexthard-caps at 120 chars), and a handful of scalars: all O(row count), never O(file bytes). The residual peak is one JSONL line (readline buffer + parsed string + parsed object), freed as soon as the callback returns. Copilot's unbounded-prompt-body concern is closed, not just relabelled.
Local gates on 9a23b19: the whole agent-adapter suite 540/540, history-service.test.ts 6/6, eslint clean on all four changed files, and tsc --build --noEmit clean for both packages.
Reviewed changes — the delta since 88ccb00:
- Fingerprinted every echoed-prompt comparison —
collectCodexPromptTextsbecamecollectCodexPromptFingerprints,CodexUserRowDigestnow carriesmarkedFingerprints/echoedFingerprint, and a privatepromptTextFingerprint()(SHA-256 → base64url) hashes both sides, so a summary scan retains ~43 bytes per prompt instead of the body. - Gave the filename fast path a failing-capable test — a file-scoped
vi.mock('node:fs')delegating spy oncreateReadStreamlets the test assert the decoy rollout is never opened, which no assertion on the returned summary could distinguish.
Claude Opus | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
The delta is one argument and one test, so I verified both directly rather than reasoning about them.
- The collision was real. In Node 26,
createHash('sha256').update('…\uD800').digest()equals the digest of'…\uD801'under both the default and an explicit'utf8'— the utf8 encoder maps every unpaired surrogate to U+FFFD, collapsing distinct texts onto one fingerprint. Under'utf16le'they differ. So on9a23b19, an injected marker row could be paired with a different echoed prompt and replayed into history; the fix closes that. utf16leadds no collisions of its own. It writes each UTF-16 code unit as two LE bytes, a bijection onto even-length byte strings, so the fingerprint is now a faithful code-unit identity rather than a lossy one. Fingerprints are process-local and never persisted, so there is no compatibility surface to the encoding switch.- The new test has teeth. Reverting to
update(text)and re-runningcodex-history.test.tsfails'does not pair distinct marker prompts containing lone surrogates'withexpected 0, received 1— the injected# AGENTS.md instructions\u{D801}row gets rescued by the\u{D800}echo. Passing without the mutation, 46/46. - Reachability is thin but not nil, which is what makes the fix worth its one argument: a Rust
Stringcannot hold a lone surrogate, so codex's own serde_json writer cannot emit one — butJSON.parsedoes preserve a\ud800escape, so any rollout line written by something other than the Rust CLI reaches it.
Local gates on a3f9eb1: agent-adapter/src/__tests__ plus history-service.test.ts 547/547 green, tsc --build --noEmit clean for both packages.
One thing from the first review is still outstanding and unanswered rather than resolved: CodexAdapter.readHistory remains O(file) — whole transcript materialised before sliceHistoryEventPage — and attach-time transcript seeding runs through it. Worth a word on whether that's a deliberate scope call for CODE-605 before merge, though it doesn't block anything in this delta.
Reviewed changes — the delta since 9a23b19:
- Hashed prompt fingerprints over UTF-16 code units —
promptTextFingerprintnow passes'utf16le'tohash.update, so texts differing only in unpaired surrogates no longer share a fingerprint via utf8's U+FFFD substitution. - Added a lone-surrogate pairing regression test — a
\u{D800}event_msgecho against a\u{D801}marker-bearingresponse_itemmust produce zero events; it fails under the pre-fix hashing.
Claude Opus | 𝕏

Fixes CODE-605.
Problem
The dev daemon OOM'd (
Ineffective mark-compacts near heap limit, ~3966MB) after 78 minutes of a codex session error-looping on its usage limit with mobile auto-resume and repeated attach/detach.Root cause
Every codex
listHistory/readHistoryparsed the entire~/.codexrollout corpus (3.6GB / 1,230 files on the affected machine): whole-filereadFile+split('\n')per rollout, unboundedPromise.allacross all files, andfindCodexTranscriptscanning everything to locate one id — retriggered by every attach-time transcript seed once the engine's 30s history cache expired. ExpiredHistoryServicecache entries (each holding a full transcript's events) were also never evicted.Fix
findCodexTranscriptfast path: rollout filenames end with the thread id, so a suffix match parses one file; session_meta remains the identity check, full scan is the fallback.HistoryServicecache entries; clear the codex per-item stream ledger on teardown.Verification
codex-history-files.test.ts) cover streaming summary semantics (synthetic-row filtering, event_msg rescue, titles, counts), corrupt-line tolerance, the filename fast path with a decoy, and the full-scan fallback; plus a cache-eviction test inhistory-service.test.ts.pnpm check:cigreen; fullpnpm testgreen exceptrelease-artifact.test.ts(3 failures reproduced identically on cleanorigin/master— pre-existing, unrelated).