Bound memory on the spool backfill-dedupe read (partial #280 hardening)

[philcunliffe]

What this lands

Streams readSpooledRows (src/core/cache/spool.js) so the backfill dedupe
scan reads the spool in bounded 64 KB chunks instead of fs.readFile(name, 'utf8') + split('\n'), which held ~2x the whole file (one V8 string + the
split-line array) resident before yielding a single row.

• Spool files can reach DEFAULT_SPOOL_BYTES_THRESHOLD (512 MB) of
  content-heavy ai_gateway_messages envelopes before they flush.
• scanSpooledPartIds -> createBackfillDedupe seeds its seen-set from the
  spool on a hyp backfill run, so a large-backfill dedupe scan was an
  unbounded whole-file async UTF-8 decode - the exact
  node::fs::FSReqCallback::Resolve -> StringDecoder::DecodeData -> NewStringFromUtf8 allocation signature in the Daemon OOM (4GB heap) on first central push after fleet enrollment; crash strands pending 'attach claude' action #280 crash dump.
• The fix mirrors streamFlushFile's bounded line loop that already lives right
  next to it. Row output and the envelope-validity contract are unchanged; the
  existing readSpooledRows parity tests stay green.

New regression test (test/core/cache-storage.test.js) writes a ~48 MB spool
file and asserts heap growth up to the first yielded row stays far below the
file size. It fails against the pre-fix whole-file read and passes after
(verified by temporarily reverting only the reader).

npm test: 2105 pass, 0 fail, 1 skipped.

Honest scope - this is a partial hardening, not the full #280 fix

I diagnosed both reported sub-bugs. Neither's stated hypothesis held up, and
the true root causes need design-level work, so I am using Refs #280 (not
Fixes) to avoid auto-closing the issue.

Bug A - daemon OOM. The issue's hypothesis (the @hypaware/central
request-sink push materializes the whole payload/files as strings) is already
handled: the sink streams rows via storage.readRowsSince() and POSTs in
bounded 5000-row / 4 MB chunks (LLP 0040), proven by
test/plugins/central-forward-chunking.test.js (49 green). The one code path
matching the fatal async-utf8 whole-file-decode stack is readSpooledRows
(fixed here) - but it runs in the hyp backfill subprocess, and in the
incident that backfill completed successfully (116,541 rows), so it did not
fire this time. The process that OOM'd (pid 85942, ~11.7 min uptime) is the
daemon, whose central-push surface has no async-utf8 whole-file read. So
the reported daemon OOM is a separate daemon-side accumulation (prime
suspects: the local IO resolver's readFileSync of the whole ~242 MB parquet
per open in src/core/cache/iceberg/resolver.js, compounding with icebird row
materialization / live-capture buffers). That is an architectural memory audit,
not a bounded bugfix - it needs a request/design LLP. This PR removes one
real latent OOM on the same enrollment+large-backfill path and matching the
crash signature, but does not by itself prove the daemon OOM is gone.

Bug B - stranded attach claude. The hypothesis ("the reconcile core skips
a requested action with no completion record") is false: the reconciler is
level-triggered and re-drives any non-done action whenever desired() names
it - already covered by test/core/action-reconciler.test.js ("a missed pass
(no marker yet) runs on the next reconcile call"). The real gap is two-layer:
(1) attach.desired() gates on ctx.clients.getClient(name) + a bound endpoint
while hyp status's client_attach_missing only checks (plugin enabled +
attach_probe + disk-not-marked), so when they diverge status shows [pending]
forever but the reconciler never attaches; and (2) the daemon schedules reconcile
at only two one-shot edges (confirm-edge, boot-already-confirmed) over a
once-resolved client seam, with no ongoing/tick retry, so a gap that could
not be closed at boot is stranded until the next restart. A robust fix
(seam re-resolution + ongoing reconcile trigger) overlaps directly with the
active PR #278 (action_attach.js / action_reconciler.js /
gateway_endpoint.js), so it needs design coordination, not a colliding
bounded patch.

Refs #280

[philcunliffe]

neutral review round - PR #282 (streamed readSpooledRows in bounded chunks)

Reviewed head: 620b0dc3573187a52536075466930a9d0e22f1c8
Verdict: approve after fixes. No correctness/security/perf defect. Two minor CLAUDE.md style violations were found and fixed in-worktree; new head a6d7073fa94a25cf0a068cdf4795bb960a05c25f.

What I checked

Correctness of the new chunked/streamed reader (readSpooledRows + the extracted rowsFromSpoolLine in src/core/cache/spool.js), and whether the regression test genuinely proves the memory bound.

• Line boundaries across chunk edges - correct. Each chunk is appended to tail before any line is sliced out (while (tail.indexOf('\n') !== -1)), so a line split across two 64 KB chunks stays buffered until its newline arrives. Identical loop shape to the adjacent streamFlushFile.
• Final partial line / empty file - correct and parity-preserving. After the loop, yield* rowsFromSpoolLine(tail) handles a trailing no-newline remnant exactly as the old whole-file split('\n') did (best-effort JSON.parse, drops on failure). A newline-terminated file leaves tail === '', and the line.length === 0 guard yields nothing (no spurious row). Empty file yields no chunks -> no rows.
• Multibyte UTF-8 at chunk boundaries - safe. createReadStream(..., { encoding: 'utf8' }) drives Node's internal StringDecoder, which buffers incomplete multibyte sequences across read boundaries, so 64 KB byte splits never corrupt a character. (This is why the encoding option is used rather than a manual decode.)
• Envelope-validity parity - exact. rowsFromSpoolLine applies the same version === 1 / Array.isArray(columns) / Array.isArray(rows) / object-not-array row checks as both the old inline code and streamFlushFile.
• No off-by-one - slice(0, newlineIdx) / slice(newlineIdx + 1) drop exactly the \n, nothing duplicated or lost.
• Error handling - a mid-read stream error is caught per-file and continues, matching the old per-file try/catch degrade-to-partial behavior for a provisional spool.

Regression test - does it prove the bound?

Yes, and I verified it independently. The test writes ~48 MB (1024 x ~48 KB envelopes) and asserts first-row heapUsed delta < fileBytes/2 (~24 MB). I temporarily reverted only the reader to the whole-file fs.readFile + split('\n') path and re-ran: the test fails (whole ~48 MB string resident at first yield), and passes on the streamed reader. GC noise can only lower heapUsed (satisfies the bound), so the 2x margin is robust, not flaky. Restored the file afterward.

Findings (both fixed and pushed)

1. minor - CLAUDE.md em-dash (U+2014) violation - src/core/cache/spool.js:269 (JSDoc for rowsFromSpoolLine) contained two U+2014 characters. CLAUDE.md forbids the em dash "anywhere: code, comments, JSDoc, strings, or docs." Fixed: rephrased with parentheses.
2. minor - CLAUDE.md em-dash (U+2014) violation - test/core/cache-storage.test.js:370 (test comment) contained one U+2014. Fixed: replaced with a colon.

Both fixes are comment-only, no behavior change. Verified: zero U+2014 remain in either file in the committed tree at a6d7073; node --test test/core/cache-storage.test.js stays green (13 pass, 0 fail).

Codex

Ran, but failed as expected in this repo (stream disconnected before completion on /backend-api/codex/responses - hypaware intercepts Codex's own gateway traffic). Verdict is on the Claude-side review plus my independent diff read and fail-before/pass-after check.

Advisory only; no merge attempted. Held for human merge gate.

[philcunliffe]

Round 2 review — CLEAN

Head reviewed: a6d7073fa94a25cf0a068cdf4795bb960a05c25f (base master)

Round 2 confirms the two em-dash fixes pushed after round 1 (which reviewed 620b0dc) and re-skims the change independently. No actionable findings.

Em-dash fixes: verified comment-only, zero U+2014

The delta 620b0dc..a6d7073 is exactly two edits, both in comments/JSDoc:

• src/core/cache/spool.js:269 — JSDoc reworded (otherwise it would dedupe against (and so refuse to materialize) rows...).
• test/core/cache-storage.test.js:370 — comment reworded (a single row: a giant async UTF-8 decode...).

No executable line changed. grep -P '\x{2014}' over both files returns nothing (clean).

Correctness still sound at this head

• readSpooledRows streams via createReadStream({ encoding: 'utf8', highWaterMark: 64 KB }); the utf8 StringDecoder keeps multibyte characters intact across chunk boundaries. The tail accumulate + indexOf('\n')/slice loop drains complete lines, then flushes the non-newline remnant once through rowsFromSpoolLine (partial-final-line parity with the old split('\n')). Mid-read errors continue per file, matching the old per-file try/catch.
• Extracted rowsFromSpoolLine reproduces the whole-file validity contract exactly: empty line, JSON-parse failure, version !== 1, and non-array columns/rows all yield nothing; only plain-object rows are yielded. fsSync is already imported (used by readdirSync).
• Regression test readSpooledRows streams a large spool file... (issue #280) asserts first-row heap delta < fileBytes/2 over a ~48 MB spool and full 1024-row yield. node --test test/core/cache-storage.test.js at this head: 13/13 pass.

Codex

Did not run. The dual-review skill checks the PR head out into the main working tree (which this worker must not perturb, and which currently holds unrelated uncommitted tracked changes), and hypaware intercepts Codex /backend-api/codex/responses traffic regardless. Verdict rests on the Claude-side review plus a direct diff read in an isolated detached worktree.

Verdict: clean — no blocker/major/minor actionable findings. Nothing changed; holding for the human merge gate.