Skip to content

fix(agent-adapter,engine): stream codex rollout reads and evict expired history caches - #466

Open
Zerlight wants to merge 3 commits into
masterfrom
ruocheng/code-605
Open

fix(agent-adapter,engine): stream codex rollout reads and evict expired history caches#466
Zerlight wants to merge 3 commits into
masterfrom
ruocheng/code-605

Conversation

@Zerlight

Copy link
Copy Markdown
Member

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/readHistory parsed the entire ~/.codex rollout corpus (3.6GB / 1,230 files on the affected machine): whole-file readFile + split('\n') per rollout, unbounded Promise.all across all files, and findCodexTranscript scanning everything to locate one id — retriggered by every attach-time transcript seed once the engine's 30s history cache expired. Expired HistoryService cache entries (each holding a full transcript's events) were also never evicted.

Fix

  • Stream rollout rows line-by-line; the summary pass holds only small per-row digests, never whole files.
  • Bound summary-scan concurrency to 8 files.
  • findCodexTranscript fast 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.
  • Sweep expired HistoryService cache entries; clear the codex per-item stream ledger on teardown.

Verification

  • Measured on a 744MB synthetic corpus (122 files, two 60MB): peak heap 2350MB → 67MB, ~2.5× faster.
  • New fs-based regression tests (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 in history-service.test.ts.
  • pnpm check:ci green; full pnpm test green except release-artifact.test.ts (3 failures reproduced identically on clean origin/master — pre-existing, unrelated).

Copilot AI lite review requested due to automatic review settings August 21, 2026 10:50
@linear-code

linear-code Bot commented Aug 21, 2026

Copy link
Copy Markdown

CODE-605

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.listHistory sorts only by updatedAt; 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 increment messageCount (and can make list summaries report phantom user messages). Skip rows with no non-empty text as well, e.g. by requiring userRow.preview to 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 list and read, so expiry remains lazy. If the last request loads a large transcript (or runs longer than ttlMs) and no later history request arrives, the expired eventCache entry 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 eventCache does not remove the companion historyCwdById entries. Every cwd-scoped list adds one of these keys, and clear() 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.

Comment thread packages/host/agent-adapter/src/native/codex/history.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 fidelityforEachJsonlRow reproduces the whole-file pass exactly for id / cwd / model / createdAt / updatedAt / messageCount / title / empty-file-undefined, including out-of-order event_msg rows. Deferring user rows into PendingUserRow and deciding synthetic-marker rescue in a post-stream pass is equivalent to the old collectCodexPromptTexts pre-pass.
  • Error safety — I probed this rather than assuming it: createReadStream ENOENT/EISDIR does surface through the readline async iterator into the outer catch, 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, endsWith false positives die on the summary.id === id || summary.path === id check, and every miss falls through to the full scan.
  • Cache sweep is behavior-preservingexpiresAt <= now and expiresAt > now are exact complements, so sweeping before the get can only remove entries the freshness check would have rejected anyway. Verified against partialCursor pagination, invalidateEventCacheFromList, sessionFingerprint, importSession, resumeHistory, forceRefresh. Leaving historyCwdById unswept 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

  • readCodexTranscriptSummaries no longer returns results in corpus order — workers push as they finish. The old Promise.all(files.map(...)) did preserve input order, so listHistory's (b.updatedAt ?? 0) - (a.updatedAt ?? 0) sort over it was deterministic for ties (V8's sort is stable). Sessions with an identical updatedAt can now shuffle between paginated listHistory calls and duplicate or skip across a page boundary. Ties are rare since updatedAt falls back to file mtime, so this is genuinely small — a .sort() on files before the pool, or a tiebreak on id, closes it.
  • findCodexTranscript walks the rollout tree twice on a fast-path miss (once for candidates, again inside readCodexTranscriptSummaries). Passing the already-collected files through would save the second traversal.
  • forEachJsonlRow's inner try wraps the onRow(parsed) call as well as JSON.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() in teardown() is redundant at normal turn end: handleTurnCompleted already clears the map and then calls teardown(). 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

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Copilot AI review requested due to automatic review settings August 21, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.listHistory sorts only by updatedAt (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);

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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;) against 9a23b19: the test fails at expect(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 (the if (!digest.hasImage) return true guard precedes it, matching the old echoedText sentinel), and collectCodexPromptFingerprints's if (message) guard means neither the old nor the new set can hold the empty value. isSyntheticCodexUserPayload's second parameter kept its ReadonlySet<string> type through the rename, so a stale raw-text call site would have typechecked silently — there are exactly two callers, both in history.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 a preview that previewText hard-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 comparisoncollectCodexPromptTexts became collectCodexPromptFingerprints, CodexUserRowDigest now carries markedFingerprints/echoedFingerprint, and a private promptTextFingerprint() (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 on createReadStream lets the test assert the decoy rollout is never opened, which no assertion on the returned summary could distinguish.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copilot AI review requested due to automatic review settings August 21, 2026 11:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 on 9a23b19, an injected marker row could be paired with a different echoed prompt and replayed into history; the fix closes that.
  • utf16le adds 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-running codex-history.test.ts fails 'does not pair distinct marker prompts containing lone surrogates' with expected 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 String cannot hold a lone surrogate, so codex's own serde_json writer cannot emit one — but JSON.parse does preserve a \ud800 escape, 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 unitspromptTextFingerprint now passes 'utf16le' to hash.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_msg echo against a \u{D801} marker-bearing response_item must produce zero events; it fails under the pre-fix hashing.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants