From 14c19b6e84ed83d166de5cdda5edd7cc6422802c Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 21 Jun 2026 19:29:57 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(#288):=20derive-and-drop=20sub-agent=20?= =?UTF-8?q?state=20=E2=80=94=20stop=20retaining=20raw=20entries=20in=20Sub?= =?UTF-8?q?AgentWatcher=20(85%=20of=20heap)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SubAgentWatcher retained up to 500 fully-parsed sub-agent JSONL entries per agent (`entriesByAgent: Map`). A dominator-tree heap analysis (scripts/analyze-heapsnapshot.mjs --owners) proved that map was ~85% of a 227 MB main-process heap, because each retained entry pins its full multi-MB tool-result/Read body. Prior fixes only SHRANK retained entries (PR #300 count cap of 500; PR #320/#321 per-field byte clamps); none questioned why we retained entries at all. We didn't need to: buildSubAgentState was a pure left-to-right fold over the entries, re-run on every emit, so the only reason to keep them was to re-fold them. ACCUMULATOR (added to subagentState.ts, next to the logic it mirrors): a small per-agent SubAgentAccumulator folded incrementally as each line streams in, then the raw entry is dropped. createAccumulator() / accumulateSubAgentEntry(acc, entry) (the per-entry step of buildSubAgentState's loop) / buildSubAgentStateFromAccumulator(acc, …) (the post-loop tail). Memory is now O(open tool calls), not O(transcript length). Mirrors the Codex twin (codexSubagentState.ts, PR #317). The 6 invariants an adversarial review derived, and how each is honored: 1. openToolCallIds is a STANDALONE Set, persisted independently of the toolCalls ring, so a tool_result flips status / decrements the open count even if the call's display object was already evicted from the ring. 2. Ring cap = 60. Measured max simultaneously-open tool calls across 318 transcripts = 8; 60 is 7× headroom (ring ≥ max tool_use→tool_result gap or a status flip is lost FROM THE RING). Past 60 we evict the OLDEST (front) but keep its id in openToolCallIds if still unresolved. 3. turnCount and droppedToolCalls are MONOTONIC fold counters, never derived from ring contents. turnCount ++ per assistant entry. A true totalToolUses is maintained; droppedToolCalls = max(0, totalToolUses - SUBAGENT_TOOL_CALLS_MAX), matching the array builder's total-minus-40. 4. currentActivity is literal LAST-WRITE-WINS per block in arrival order: set to `running ` on tool_use, cleared to null on tool_result, set to `thinking` on a thinking block. Not reinterpreted as "last open call". 5. startedAt = FIRST valid timestamp (set once); lastActivityAt = LAST valid timestamp (last-write-wins, NOT Math.max — 21 out-of-order cases in real data match the original builder's plain assignment). 6. Terminal done/error and the status/currentActivity gating stay computed at BUILD time from done/error (which come from parentResult), NOT stored in the accumulator. emit() reads the live accumulator each call so a late parentResult flip still re-renders. WIRING (SubAgentWatcher.ts): entriesByAgent → accByAgent. readAppended folds each parsed line via accumulateSubAgentEntry then lets it die — no truncate, no intern, no array, no 500-cap splice. emit() builds via buildSubAgentStateFromAccumulator. stop() clears accByAgent. DEAD CODE REMOVED from SubAgentWatcher.ts: truncateEntryBodies + clampDeep + clampString (the #320/#321 byte-cap machinery, only ever used at the retention site) and the internEntryFields/makeStringPool usage + the intern field + MAX_ENTRY_FIELD_BYTES / MAX_RETAINED_ENTRIES_PER_AGENT / ENTRY_CLAMP_DEPTH constants. internEntryFields/makeStringPool STAY in internEntry.ts (still used by jsonlCoalescer.ts and historyLoader.ts — untouched). CONSCIOUS BEHAVIOR DELTA: removing the 500-entry cap makes droppedToolCalls and turnCount TRUE running totals rather than relative-to-a-500-tail — higher for very long agents (6/318 transcripts exceed 60 tool calls; max 79). That is MORE correct: the displayed timeline is still capped to SUBAGENT_TOOL_CALLS_MAX, only the "+N earlier" count grows to the real total. The SubAgentState/SubAgentToolCall IPC contract is unchanged. TEST: SubAgentWatcher.test.ts updated — the 521-tool_use fixture now asserts droppedToolCalls === 481 (true total 521 - 40 visible) instead of 460 (500 retained - 40), with a comment explaining the delta. Passes. Co-Authored-By: Claude Opus 4.8 --- src/main/subagents/SubAgentWatcher.test.ts | 19 +- src/main/subagents/SubAgentWatcher.ts | 238 ++++++--------------- src/main/subagents/subagentState.ts | 219 +++++++++++++++++++ 3 files changed, 293 insertions(+), 183 deletions(-) diff --git a/src/main/subagents/SubAgentWatcher.test.ts b/src/main/subagents/SubAgentWatcher.test.ts index 67c3b115f..51ff35256 100644 --- a/src/main/subagents/SubAgentWatcher.test.ts +++ b/src/main/subagents/SubAgentWatcher.test.ts @@ -49,12 +49,17 @@ describe('SubAgentWatcher', () => { // WHY 520 entries instead of a tiny fixture: // // The renderer-facing builder already caps the mini-feed to 40 tool - // calls. To prove the watcher now bounds MAIN-process retained RawEntry - // memory too, the fixture must exceed the watcher cap (500), not only the - // renderer cap. Without the watcher cap this would report 481 dropped - // tool calls (521 total - 40 visible); with the watcher cap it reports - // 460 (500 retained - 40 visible), proving older parsed entries were - // discarded before building state. + // calls. This large fixture proves the watcher's #288 derive-and-drop + // accumulator folds EVERY appended entry (no raw entries retained) while + // still bounding what it ships to the renderer. + // + // CONSCIOUS BEHAVIOR DELTA (#288): the old watcher retained at most 500 + // raw entries and folded only that 500-entry tail, so `droppedToolCalls` + // was relative-to-500 and this asserted 460 (500 retained - 40 visible). + // The accumulator folds from the start, so `droppedToolCalls` is now the + // TRUE total: 521 tool_uses - 40 visible = 481. That is more correct (the + // "+N earlier" count reflects the real number dropped, not an artifact of + // a retention cap), and it no longer depends on retaining any entries. await appendFile( jsonlPath, Array.from({ length: 520 }, (_, i) => assistantToolUseLine(i + 1)).join(''), @@ -65,7 +70,7 @@ describe('SubAgentWatcher', () => { await eventually(() => { const state = emissions.at(-1)?.['tool-parent'] expect(state?.toolCalls).toHaveLength(40) - expect(state?.droppedToolCalls).toBe(460) + expect(state?.droppedToolCalls).toBe(481) expect(state?.toolCalls.at(-1)).toMatchObject({ name: 'Read', headline: '/tmp/file-520.txt', diff --git a/src/main/subagents/SubAgentWatcher.ts b/src/main/subagents/SubAgentWatcher.ts index 9bcad5d8e..a0a70d285 100644 --- a/src/main/subagents/SubAgentWatcher.ts +++ b/src/main/subagents/SubAgentWatcher.ts @@ -2,8 +2,13 @@ import { createReadStream } from 'node:fs' import { readFile, readdir, stat } from 'node:fs/promises' import { join } from 'node:path' import type { SubAgentState } from '@preload/api/types.js' -import { buildSubAgentState, type SubAgentMeta } from './subagentState.js' -import { makeStringPool, internEntryFields } from '@main/sessions/internEntry.js' +import { + accumulateSubAgentEntry, + buildSubAgentStateFromAccumulator, + createAccumulator, + type SubAgentAccumulator, + type SubAgentMeta, +} from './subagentState.js' // One poller per session, watching /subagents/. // @@ -15,160 +20,49 @@ import { makeStringPool, internEntryFields } from '@main/sessions/internEntry.js // something actually changed (dirty flag) to avoid spamming IPC. const POLL_MS = 600 -const MAX_RETAINED_ENTRIES_PER_AGENT = 500 -// #288 (root cause): cap the BYTE SIZE of every retained free-text field, not -// just the entry COUNT. +// #288 ROOT-CAUSE FIX: this watcher no longer retains raw entries at all. // -// A dominator-tree heap analysis (scripts/analyze-heapsnapshot.mjs --owners) -// proved that 7 live SubAgentWatcher instances retained 263 MB — 88% of the -// entire reachable main-process heap — via the chain +// HISTORY (why the old code was the way it was, so future-you doesn't reinvent +// it): a dominator-tree heap analysis (scripts/analyze-heapsnapshot.mjs --owners) +// pinned live SubAgentWatcher instances as ~85% of a 227 MB main-process heap via // SubAgentWatcher.entriesByAgent (Map) // → entry → message.content → huge string. -// PR #300 capped MAX_RETAINED_ENTRIES_PER_AGENT=500 but did NOT bound the bytes -// per entry, so each of those 500 retained entries still pinned its FULL body: -// Read tool outputs, tool_result bodies, the skills-list attachment, and -// `[Truncated: PARTIAL view]` system-reminders. 500 entries × multi-MB bodies × -// several agents × 7 watchers = 263 MB, and churning those multi-MB strings on -// every 600 ms poll fragments V8 into ~1.5 GB RSS. +// We chased that with COUNT caps (PR #300: keep 500 entries) and then per-FIELD +// byte caps (PR #320/#321: clamp every string in each retained entry). Both only +// SHRANK the retained entries; they never questioned WHY we retained entries at +// all. The answer is: we didn't need to. buildSubAgentState was a pure fold over +// the entries, re-run on every emit, so the only reason to keep entries was to +// re-fold them. By folding INCREMENTALLY as each line streams in (see +// accumulateSubAgentEntry in subagentState.ts) and dropping the entry, the +// O(transcript) retention disappears entirely — no count cap, no byte cap, no +// string pool, no truncation. The durable full transcript still lives on disk in +// agent-.jsonl. This mirrors the Codex twin (codexSubagentState.ts, PR #317). // -// The mini-feed this watcher feeds is a LIVENESS affordance, not a transcript -// archive. buildSubAgentState only reads structure (block types, tool names, -// tool_use_id, is_error, timestamps) plus an ≤80-char headline per tool call — -// it never renders a full body. So truncating the large free-text payloads at -// retention to a cap well above what the renderer shows is invisible to the -// user while removing the bytes that caused the leak. The durable full -// transcript always stays on disk in the agent-.jsonl, which is the source -// of truth if a future UI wants deep drill-in. -// -// 8 KB is generous: it is ~100× the 80-char headline buildSubAgentState renders -// from any single field, so no rendered value can ever be clipped by this cap, -// yet it turns a multi-MB Read result into a few KB. We truncate per-field -// (not per-entry) so an entry with several blocks each keeps its own budget. -// -// NOTE on units: this is a UTF-16 *character* cap (`String.length` counts code -// units, not bytes) — the truncation marker says "chars" to match. We don't -// convert to bytes because the goal is a generous BOUND, not an exact size, and -// a char count is the cheapest thing to compute on this per-appended-line path. -const MAX_ENTRY_FIELD_BYTES = 8 * 1024 - -// truncateEntryBodies: clamp the large free-text string fields of a freshly -// parsed RawEntry in place, PRESERVING every structural field buildSubAgentState -// reads (entry `type`/`uuid`, `message.role`/`type`, each block's `type`, tool -// `name`/`id`/`tool_use_id`, `is_error`/status, timestamps). We only shrink the -// payloads that carry the megabytes: -// - TOP-LEVEL `entry.content` string and `entry.attachment.content` string. -// Some entries have NO `message` field at all — most importantly the -// `skill_listing` attachment whose `content` is the full skills list. That -// shape was the single LARGEST duplicated leak category in the heap (×803). -// It is handled at the entry root, BEFORE/independent of the message walk, -// because there is no `message.content` array to descend into — the old -// early-return ("not an array") skipped these entries entirely and made the -// per-block `attachment.content` clamp below dead code for the real shape. -// - `message.content` when it is a plain STRING. Claude transcripts use a -// string body (large pasted user prompts, compact/user string messages, -// string-shaped system reminders) as readily as the array-of-blocks shape; -// the old early-return pinned every string body full-size. -// - text/thinking blocks' `text`/`thinking` -// - tool_result blocks' `content`/`output` (string OR array-of-{text} shape) -// - string values inside a tool_use `input`, INCLUDING NESTED ones — e.g. -// MultiEdit's `input.edits[].old_string`/`new_string`, which a flat -// top-level-only walk left pinned. We recurse (bounded) so nested text is -// clamped while every key and the input's structure stay intact. -// - a per-block `attachment.content` (skills lists can also ride inside blocks) -// Each clamp appends an honest marker so a future reader of the in-memory entry -// knows chars were dropped and where the full body lives. This runs once per -// appended line at the single parse/push site below — never on a hot render -// path — so the cost is bounded by new bytes, matching the watcher's invariant. -function clampString(value: string, cap: number): string { - if (value.length <= cap) return value - const dropped = value.length - cap - // CRITICAL (#288): `value.slice(0, cap)` returns a V8 SlicedString that RETAINS - // the entire parent string — so truncating with a bare slice frees ZERO memory - // (the multi-MB body we are trying to drop stays alive behind the slice, which - // is exactly why #320 reduced the heap by nothing). Proven empirically: clamping - // 2000×200KB strings via slice left ~194MB pinned; via the Buffer round-trip - // below, ~19MB. Round-tripping the prefix through a Buffer copies its bytes into - // fresh, parent-independent storage, so the original body becomes GC-eligible. - // `utf16le` preserves every JS code unit exactly (incl. lone surrogates), so the - // visible prefix is byte-identical to a plain slice — only the retention differs. - const head = Buffer.from(value.slice(0, cap), 'utf16le').toString('utf16le') - return head + `… [truncated ${dropped} chars — full body on disk]` -} - -// clampDeep: recursively clamp every string VALUE reachable inside an arbitrary -// tool_use `input`, preserving all keys, array order, and object structure. -// WHY recursion (not the old flat top-level-only loop): structured tool inputs -// bury their large text below the first level — MultiEdit puts each edit's -// `old_string`/`new_string` under `input.edits[i]`, and other tools nest text -// under sub-objects/arrays. A flat walk only saw the top-level string values -// and left those nested megabytes pinned. We bound recursion at `depth` (≤6) so -// a pathologically deep object can't blow the stack or burn the per-line budget; -// nothing deeper than that is a realistic tool-input text payload, and the -// durable full copy is on disk regardless. Only string VALUES are rewritten — -// keys and the shape headlineFromInput reads (command/file_path/…) are untouched. -function clampDeep(value: unknown, cap: number, depth: number): unknown { - if (typeof value === 'string') return clampString(value, cap) - if (depth <= 0 || !value || typeof value !== 'object') return value - if (Array.isArray(value)) { - for (let i = 0; i < value.length; i++) value[i] = clampDeep(value[i], cap, depth - 1) - return value - } - const obj = value as Record - for (const k of Object.keys(obj)) obj[k] = clampDeep(obj[k], cap, depth - 1) - return obj -} - -// Deepest nesting we recurse when clamping an entry. The big bodies sit shallow -// (`entry.toolUseResult.file.content` ≈ depth 3; `message.content[i].content[j] -// .text` ≈ depth 6); 12 is generous headroom while still bounding the walk so a -// pathological object can't blow the stack or burn the per-line budget. -const ENTRY_CLAMP_DEPTH = 12 - -function truncateEntryBodies(entry: unknown, cap: number): void { - // #288 (fix for the #320 miss): clamp EVERY large free-text string ANYWHERE in - // the entry by recursing the whole object — not a hand-maintained list of fields. - // - // WHY the previous field-by-field walk was wrong: it only visited - // `message.content` / `attachment` / bare `content`, but Claude writes the FULL - // Read/tool output to the TOP-LEVEL `toolUseResult` field - // (`toolUseResult.file.content`, `stdout`, `stderr`) — *in addition* to the - // tool_result block. The dominator tree's bytes actually lived in - // `toolUseResult`, which the targeted walk never touched, so retained entries - // stayed full-size and #320 freed almost nothing. A recursive clamp catches - // toolUseResult, message.content (string OR array), attachments, tool_use input, - // and any future shape — no more whack-a-mole. - // - // WHY it's safe / invisible: `clampString` is a no-op for strings under `cap`, - // so short structural fields (type/uuid/role/name/id/tool_use_id/timestamps) are - // returned untouched; only multi-KB bodies are clamped. `buildSubAgentState` - // only ever renders ≤80-char headlines, so clamping at `cap` (8 KB) can never - // change a rendered value. `clampDeep` rewrites string VALUES in place and - // preserves all keys and array order. Durable full bodies stay on disk. - clampDeep(entry, cap, ENTRY_CLAMP_DEPTH) -} +// What that deletes from THIS file: truncateEntryBodies + clampDeep + clampString +// (the #320/#321 byte-cap machinery) and the internEntryFields/makeStringPool +// usage — all of it existed solely to make the retained entries cheaper, and with +// nothing retained there is nothing to clamp or intern. internEntryFields itself +// stays in internEntry.ts (jsonlCoalescer + historyLoader still use it). type ParentResult = (toolUseId: string) => { done: boolean; error: boolean } -type RawEntry = Parameters[3][number] +// The accumulator's fold consumes the same RawEntry shape buildSubAgentState did. +type RawEntry = Parameters[1] export class SubAgentWatcher { private timer: NodeJS.Timeout | null = null private offsets = new Map() // agentId -> byte offset consumed private partialByAgent = new Map() - private entriesByAgent = new Map() + // #288 ROOT-CAUSE FIX: per-agent ACCUMULATOR instead of a retained entry array. + // Each appended line is folded into its agent's accumulator (a few scalars + a + // bounded ≤60 tool-call ring) and then dropped. Memory is O(open tool calls), + // not O(transcript length) — which is what killed the heap before. emit() reads + // the live accumulator each call so a late parentResult flip still re-renders. + private accByAgent = new Map() private metaByAgent = new Map() private dirty = false private stopped = false - // #288: per-watcher string pool. This watcher retains up to - // MAX_RETAINED_ENTRIES_PER_AGENT parsed RawEntry objects PER subagent - // across a whole Task fan-out, each freshly JSON.parsed below, so the - // same cwd/role/type metadata is re-minted on every appended line. - // Interning against a pool scoped to this watcher shares the canonical - // strings across all of its agents' entries. The pool's lifetime is the - // watcher's — cleared in stop() (one watcher per session) — so it can - // never grow into the global leak a shared pool would be. - private intern = makeStringPool() constructor( private readonly subagentsDir: string, @@ -190,12 +84,10 @@ export class SubAgentWatcher { this.timer = null this.offsets.clear() this.partialByAgent.clear() - this.entriesByAgent.clear() + // #288: drop the per-agent accumulators (each holds only scalars + a small + // bounded ring, but a stopped watcher must release everything regardless). + this.accByAgent.clear() this.metaByAgent.clear() - // #288: drop the interning pool's backing Map so the strings it pinned - // are released with the rest of this watcher's state. Reassigning a - // fresh empty pool is the cheapest way to let the old Map GC. - this.intern = makeStringPool() } /** Force a re-emit (e.g. the parent transcript just produced a tool_result @@ -266,43 +158,31 @@ export class SubAgentWatcher { const complete = text.slice(0, lastNl) this.partialByAgent.set(agentId, text.slice(lastNl + 1)) this.offsets.set(agentId, size) - const arr = this.entriesByAgent.get(agentId) ?? [] + // #288 ROOT-CAUSE FIX: fold each appended line into the agent's accumulator + // and let the parsed entry die. There is deliberately NO retained array, NO + // 500-cap splice, NO truncate, NO intern here — nothing entry-shaped survives + // a loop iteration, so the multi-MB tool-result/Read bodies the dominator + // tree pinned can never accumulate. accumulateSubAgentEntry runs the SAME + // per-entry logic buildSubAgentState's loop did; the entry is read-only and + // becomes GC-eligible the instant the iteration ends. + let acc = this.accByAgent.get(agentId) + if (!acc) { + acc = createAccumulator() + this.accByAgent.set(agentId, acc) + } for (const line of complete.split('\n')) { const t = line.trim() if (!t) continue try { const parsed = JSON.parse(t) as RawEntry - // #288 (root cause): clamp the large free-text bodies BEFORE retaining, - // so the 263 MB the dominator tree attributed to entriesByAgent → entry - // → message.content → huge string can never accumulate. Done here, at the - // single parse/push site, the cap applies to every entry exactly once and - // the durable full body stays on disk. See truncateEntryBodies above. - truncateEntryBodies(parsed, MAX_ENTRY_FIELD_BYTES) - // #288: intern the duplicated metadata (type, message.role/type, …) - // before retaining. internEntryFields is defensive and never throws, - // so a malformed entry that slipped past JSON.parse still can't take - // down this loop. Value-equality is preserved — buildSubAgentState - // reads the same fields it always did. - internEntryFields(parsed as Record, this.intern) - arr.push(parsed) + // accumulateSubAgentEntry is defensive (permissive shapes, no throws on a + // partial/edge entry), matching the old builder's contract. A malformed + // line that slips past JSON.parse still can't take down this loop. + accumulateSubAgentEntry(acc, parsed) } catch { /* skip a malformed line */ } } - if (arr.length > MAX_RETAINED_ENTRIES_PER_AGENT) { - // WHY cap parsed entries even though the renderer timeline is already - // capped in buildSubAgentState: - // - // The IPC payload cap only protects the renderer. Keeping every parsed - // RawEntry here still makes main-process heap grow with long-running - // subagents and duplicates large tool-result strings in memory. The - // mini-feed is a liveness/status affordance, not a full transcript - // archive; the durable source remains the on-disk JSONL if a future UI - // wants deep drill-in. Retaining a generous tail preserves current - // activity and recent tool context while bounding this watcher. - arr.splice(0, arr.length - MAX_RETAINED_ENTRIES_PER_AGENT) - } - this.entriesByAgent.set(agentId, arr) this.dirty = true } @@ -311,13 +191,19 @@ export class SubAgentWatcher { for (const [agentId, meta] of this.metaByAgent) { const toolUseId = meta.toolUseId if (!toolUseId) continue // can't link without it — skip (design spec §8) - const entries = this.entriesByAgent.get(agentId) ?? [] + // INVARIANT 6: read the LIVE accumulator each emit and resolve terminal + // done/error here at build time. A late parentResult flip (parent + // tool_result lands after the subagent's last line) therefore re-renders on + // the next emit without any state stored in the accumulator. An agent whose + // meta exists but whose jsonl hasn't been read yet folds an empty + // accumulator → the same "header only" state the old empty-array path gave. + const acc = this.accByAgent.get(agentId) ?? createAccumulator() const { done, error } = this.parentResult(toolUseId) - out[toolUseId] = buildSubAgentState( + out[toolUseId] = buildSubAgentStateFromAccumulator( + acc, toolUseId, agentId, meta, - entries, done, error, ) diff --git a/src/main/subagents/subagentState.ts b/src/main/subagents/subagentState.ts index ca55bda34..6ce91c076 100644 --- a/src/main/subagents/subagentState.ts +++ b/src/main/subagents/subagentState.ts @@ -161,3 +161,222 @@ export function buildSubAgentState( currentActivity: status === 'running' ? lastActivity : null, } } + +// ──────────────────────────────────────────────────────────────────────────── +// #288 ROOT-CAUSE FIX: derive-and-drop accumulator. +// +// buildSubAgentState above is a pure left-to-right fold over the whole entry +// array. SubAgentWatcher used to RETAIN that array (up to 500 RawEntry per +// agent) just so it could re-run this fold on every emit — and a dominator-tree +// heap analysis proved that retained array was ~85% of a 227 MB main-process +// heap, because each entry pins its full multi-MB tool-result/Read body. +// +// The insight (mirroring the Codex twin in codexSubagentState.ts, PR #317) is +// that NOTHING downstream needs the raw entries: emit() only ever ships the +// derived SubAgentState. So instead of retaining entries and folding at emit +// time, we fold INCREMENTALLY as each line streams in and immediately drop the +// entry. The accumulator below is the running state of that fold — a handful of +// scalars plus a small bounded ring of tool calls — so memory is O(open calls), +// not O(transcript length). +// +// `accumulateSubAgentEntry` is the per-entry step of the SAME loop body as +// buildSubAgentState; `buildSubAgentStateFromAccumulator` is the post-loop tail +// (status resolution + the parentDone/parentError gating). Keep the two paths +// equivalent in semantics — they derive the same SubAgentState and any drift is +// a silent UI regression. +// +// CONSCIOUS BEHAVIOR DELTA (see SubAgentWatcher.ts wiring + the PR): the old +// watcher capped retained entries at 500 and folded only that 500-entry TAIL, so +// `turnCount` and `droppedToolCalls` were relative-to-the-last-500. This +// accumulator folds EVERY entry from the start, so those counters are now TRUE +// running totals — strictly higher for very long agents (6/318 measured +// transcripts exceed 60 tool calls; max 79). That is MORE correct, not a +// regression: the displayed timeline is still capped to SUBAGENT_TOOL_CALLS_MAX, +// only the "+N earlier" count grows to the real total. + +/** + * Ring capacity for retained tool-call display objects. + * + * INVARIANT 2: measured max simultaneously-open tool calls (a tool_use with no + * yet-seen tool_result) across 318 real transcripts = 8. The ring must be ≥ that + * worst-case tool_use→tool_result gap or a tool_result could arrive after its + * tool_use's display object was already evicted, and the status flip would be + * lost FROM THE RING. 60 is 7× headroom over the observed 8 — generous enough + * that this can never bite, small enough that the per-agent footprint stays + * trivially bounded. + * + * NOTE: status resolution itself does NOT depend on the call still being in the + * ring — openToolCallIds (invariant 1) tracks unresolved ids independently, so a + * flip is never *miscounted* even past the ring. The ring cap only bounds how + * many call objects we can DISPLAY; it is sized so the most-recent + * SUBAGENT_TOOL_CALLS_MAX (40) shown calls are always present. + */ +const SUBAGENT_RING_MAX = 60 + +type ToolCallAcc = SubAgentToolCall & { id: string | null } + +export type SubAgentAccumulator = { + // INVARIANT 5: startedAt is the FIRST valid timestamp seen (set once); + // lastActivityAt is the LAST valid timestamp seen (last-write-wins, NOT + // Math.max — real transcripts have 21 out-of-order cases and the original + // builder used plain assignment, so we match that exactly). + startedAt: number | null + lastActivityAt: number | null + // INVARIANT 3: monotonic fold counters, incremented as entries stream, NEVER + // derived from ring contents. turnCount ++ per assistant entry. totalToolUses + // is the true count of every tool_use ever seen; droppedToolCalls is derived + // from it at build time as max(0, total - SUBAGENT_TOOL_CALLS_MAX). + turnCount: number + totalToolUses: number + // INVARIANT 4: literal last-write-wins per block in arrival order. Set to + // `running ` on tool_use, CLEARED to null on tool_result, set to + // `thinking` on a thinking block. Not reinterpreted as "last open call". + currentActivity: string | null + // Bounded ring of the most-recent tool-call display objects (invariant 2). + toolCalls: ToolCallAcc[] + // INVARIANT 1: STANDALONE set of unresolved tool_use ids, persisted + // independently of the ring. A tool_result flips its call to done / decrements + // the open count by removing its id here EVEN IF the call object was already + // evicted from the ring. Status resolution must never depend on ring presence. + openToolCallIds: Set + // Ids that have seen a tool_result. Kept so an out-of-order or + // arrived-after-eviction tool_result still marks a still-present ring call as + // done (the original builder resolved status by a result-id set, so we do too). + resolvedToolCallIds: Set +} + +export function createAccumulator(): SubAgentAccumulator { + return { + startedAt: null, + lastActivityAt: null, + turnCount: 0, + totalToolUses: 0, + currentActivity: null, + toolCalls: [], + openToolCallIds: new Set(), + resolvedToolCallIds: new Set(), + } +} + +/** + * Fold one parsed transcript entry into the accumulator. This is the per-entry + * body of buildSubAgentState's loop, lifted out so the watcher can run it as + * each line streams in and then drop the entry. The entry is read-only here; it + * must be GC-eligible the instant this returns (the whole point of #288). + */ +export function accumulateSubAgentEntry( + acc: SubAgentAccumulator, + entry: RawEntry, +): void { + const ms = tsToMs(entry.timestamp) + if (ms != null) { + // INVARIANT 5: first-seen sets startedAt once; every valid ts overwrites + // lastActivityAt (last-write-wins, matching the old `lastTs = ms`). + if (acc.startedAt == null) acc.startedAt = ms + acc.lastActivityAt = ms + } + const content = entry.message?.content + if (!Array.isArray(content)) return + // INVARIANT 3: per-assistant-entry turn counter, monotonic. + if (entry.type === 'assistant') acc.turnCount += 1 + for (const b of content as RawBlock[]) { + if (b.type === 'tool_use') { + // INVARIANT 3: true total of every tool_use, never the ring length. + acc.totalToolUses += 1 + const id = typeof b.id === 'string' ? b.id : null + const call: ToolCallAcc = { + id, + name: b.name ?? 'tool', + headline: headlineFromInput(b.input), + // Tentative — resolved at build time against resolvedToolCallIds. If a + // tool_result for this id already streamed past (out-of-order), reflect + // it immediately so a same-build read is consistent. + status: id && acc.resolvedToolCallIds.has(id) ? 'done' : 'running', + } + // INVARIANT 1: track unresolved ids independently of the ring. Only ids + // not already resolved are "open"; a tool_use whose result already + // arrived is not open. + if (id && !acc.resolvedToolCallIds.has(id)) acc.openToolCallIds.add(id) + // INVARIANT 2: push then evict the OLDEST (front) past the ring cap. The + // evicted call leaves the ring but its id STAYS in openToolCallIds if + // still unresolved, so a later tool_result still decrements the open count. + acc.toolCalls.push(call) + if (acc.toolCalls.length > SUBAGENT_RING_MAX) acc.toolCalls.shift() + // INVARIANT 4: last-write-wins activity hint. + acc.currentActivity = `running ${b.name ?? 'tool'}` + } else if (b.type === 'tool_result' && typeof b.tool_use_id === 'string') { + const rid = b.tool_use_id + acc.resolvedToolCallIds.add(rid) + // INVARIANT 1: flip status / drop from open set independently of the ring. + acc.openToolCallIds.delete(rid) + for (const c of acc.toolCalls) { + if (c.id === rid) c.status = 'done' + } + // INVARIANT 4: a resolved call clears the activity hint so a long gap + // before the next action doesn't read as "still running X". + acc.currentActivity = null + } else if (b.type === 'thinking') { + // INVARIANT 4. + acc.currentActivity = 'thinking' + } + } +} + +/** + * Produce a SubAgentState from a folded accumulator — the post-loop tail of + * buildSubAgentState. Yields the SAME shape/values the array builder does + * (modulo the conscious true-total counter delta documented above). + * + * INVARIANT 6: terminal done/error and the status/currentActivity gating are + * computed HERE at build time from parentDone/parentError (which come from + * parentResult), NOT stored in the accumulator — so a late parentResult flip + * re-renders correctly the next time emit() builds from the live accumulator. + */ +export function buildSubAgentStateFromAccumulator( + acc: SubAgentAccumulator, + toolUseId: string, + agentId: string, + meta: SubAgentMeta, + parentDone: boolean, + parentError: boolean, +): SubAgentState { + // The ring already holds at most SUBAGENT_RING_MAX (60) most-recent calls; the + // renderer shows at most SUBAGENT_TOOL_CALLS_MAX (40). Slice the tail of the + // ring so the visible window matches the array builder's `kept`. + const kept = + acc.toolCalls.length > SUBAGENT_TOOL_CALLS_MAX + ? acc.toolCalls.slice(acc.toolCalls.length - SUBAGENT_TOOL_CALLS_MAX) + : acc.toolCalls + const toolCalls: SubAgentToolCall[] = kept.map(({ name, headline, status }) => ({ + name, + headline, + status, + })) + + // INVARIANT 3: dropped is derived from the TRUE total tool_use count, matching + // the array builder's `calls.length - SUBAGENT_TOOL_CALLS_MAX` (its + // calls.length WAS the total). max(0, …) guards the under-cap case. + const droppedToolCalls = Math.max(0, acc.totalToolUses - SUBAGENT_TOOL_CALLS_MAX) + + const status: SubAgentState['status'] = parentError + ? 'error' + : parentDone + ? 'done' + : 'running' + + return { + toolUseId, + agentId, + agentType: meta.agentType ?? 'agent', + description: meta.description ?? '', + status, + startedAt: acc.startedAt, + lastActivityAt: acc.lastActivityAt, + turnCount: acc.turnCount, + toolCalls, + droppedToolCalls, + // INVARIANT 6: gate currentActivity on the build-time status, just like the + // array builder (only a running agent shows a live activity label). + currentActivity: status === 'running' ? acc.currentActivity : null, + } +} From dff002d4878c125a800a3f2260dbde016768ef62 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sun, 21 Jun 2026 19:54:06 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(#322):=20flatten=20retained=20tool-call?= =?UTF-8?q?=20headlines=20(V8=20slice-trap)=20+=20bound=20resolvedToolCall?= =?UTF-8?q?Ids=20=E2=80=94=20review=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 --- src/main/subagents/SubAgentWatcher.test.ts | 2 +- src/main/subagents/subagentState.ts | 52 ++++++++++++++++++++-- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/main/subagents/SubAgentWatcher.test.ts b/src/main/subagents/SubAgentWatcher.test.ts index 51ff35256..e1f25fb67 100644 --- a/src/main/subagents/SubAgentWatcher.test.ts +++ b/src/main/subagents/SubAgentWatcher.test.ts @@ -13,7 +13,7 @@ afterEach(async () => { }) describe('SubAgentWatcher', () => { - it('buffers partial JSONL lines and caps retained parsed entries', async () => { + it('buffers partial JSONL lines and folds every entry without retaining them', async () => { const dir = await mkdtemp(join(tmpdir(), 'agent-code-subagents-')) tmpDirs.push(dir) const jsonlPath = join(dir, 'agent-child.jsonl') diff --git a/src/main/subagents/subagentState.ts b/src/main/subagents/subagentState.ts index 6ce91c076..e3be46737 100644 --- a/src/main/subagents/subagentState.ts +++ b/src/main/subagents/subagentState.ts @@ -52,7 +52,25 @@ function headlineFromInput( ]) { const v = input[k] if (typeof v === 'string' && v.length > 0) { - return v.length > 80 ? v.slice(0, 80) + '…' : v + // V8 SLICE-TRAP: `bigString.slice(0, 80)` returns a SlicedString that + // retains a pointer to its ENTIRE parent string — so an 80-char headline + // cut from a multi-MB tool input (a Write/Edit `content`, a giant + // `command`/`query`) would PIN that whole parent in the heap. Unlike a + // transient render value, THIS headline is RETAINED in the accumulator's + // toolCalls ring (up to SUBAGENT_RING_MAX entries) long after the raw + // transcript entry is dropped — exactly the residual leak #288's + // derive-and-drop is meant to kill (measured: slice-truncating 2000×200KB + // left 194MB pinned; a flat Buffer copy left 19MB). This is why the deleted + // `clampString` helper forced a flat copy, and why we must here too. + // + // Force a parent-INDEPENDENT copy by round-tripping the 80-char prefix + // through a Buffer (UTF-16 in and out preserves the chars exactly, + // surrogate pairs included). Only force the copy when v actually exceeds the + // cap: for v.length <= 80 the whole string is already small, has no large + // parent to pin, and is returned as-is. Visible output is identical — the + // same ≤ 80 chars plus the ellipsis. + if (v.length <= 80) return v + return Buffer.from(v.slice(0, 80), 'utf16le').toString('utf16le') + '…' } } return null @@ -296,7 +314,22 @@ export function accumulateSubAgentEntry( // INVARIANT 1: track unresolved ids independently of the ring. Only ids // not already resolved are "open"; a tool_use whose result already // arrived is not open. - if (id && !acc.resolvedToolCallIds.has(id)) acc.openToolCallIds.add(id) + if (id) { + if (acc.resolvedToolCallIds.has(id)) { + // BOUND resolvedToolCallIds (consume side): this id's tool_result + // arrived BEFORE its tool_use (result-before-use — the ONLY reason this + // set exists). The pending result has now been consumed (the `status` + // field above is already resolved to 'done'), so this id is provably + // never needed again: ids are unique per call, so a consumed id can't be + // re-resolved. Deleting it bounds the set to the tiny out-of-order + // window instead of letting it grow O(total calls) for the agent's whole + // lifetime. This does NOT change the resolved outcome — the status flip + // already happened at push time; we only free memory. + acc.resolvedToolCallIds.delete(id) + } else { + acc.openToolCallIds.add(id) + } + } // INVARIANT 2: push then evict the OLDEST (front) past the ring cap. The // evicted call leaves the ring but its id STAYS in openToolCallIds if // still unresolved, so a later tool_result still decrements the open count. @@ -306,9 +339,20 @@ export function accumulateSubAgentEntry( acc.currentActivity = `running ${b.name ?? 'tool'}` } else if (b.type === 'tool_result' && typeof b.tool_use_id === 'string') { const rid = b.tool_use_id - acc.resolvedToolCallIds.add(rid) // INVARIANT 1: flip status / drop from open set independently of the ring. - acc.openToolCallIds.delete(rid) + // `delete` returns true iff rid was open, i.e. iff its tool_use was ALREADY + // seen (in-order — the dominant case). When that's true the ring flip below + // fully resolves the call and rid is never consumed again (ids are unique), + // so it must NOT be recorded in resolvedToolCallIds: doing so would leak one + // entry per resolved call for the agent's whole lifetime — the very + // unboundedness this fix removes. We stash rid ONLY for the result-before-use + // case (tool_use not yet seen → delete returns false), so a later tool_use + // can find it, mark itself done, and prune it (consume side above). Net: + // resolvedToolCallIds holds only the still-pending out-of-order window + // (observed max 8), not O(total calls). Outcome is unchanged — the in-order + // ring flip below is identical; only the no-longer-needed stash is dropped. + const wasOpen = acc.openToolCallIds.delete(rid) + if (!wasOpen) acc.resolvedToolCallIds.add(rid) for (const c of acc.toolCalls) { if (c.id === rid) c.status = 'done' }