Skip to content

The gateway daemon leaks until GC thrash: recorder retention, unbounded dedupe and seed scans (LLP 0204) - #683

Merged
philcunliffe merged 3 commits into
masterfrom
fix/gateway-daemon-memory-leak
Aug 9, 2026
Merged

The gateway daemon leaks until GC thrash: recorder retention, unbounded dedupe and seed scans (LLP 0204)#683
philcunliffe merged 3 commits into
masterfrom
fix/gateway-daemon-memory-leak

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Incident

On 2026-08-08 ~23:30 UTC the hyp daemon gateway on hypebox-1 (proxying all Claude Code traffic for 3 autonomous 24/7 reconcile loops) grew to ~4.8 GB RSS and entered GC thrash: the port accepted but never answered, and every loop timed out for ~3.5 h. The daemon had been supervisor-restarted ~daily for the 9 days prior, and a fresh daemon reached ~870 MB RSS within 2 minutes of boot. Full diagnosis in LLP 0204.

Fixes

  1. Recorder retention (the daily-death driver). recorder.js added every exchange to its active set and never removed it, so every finished exchange (raw body buffers, decoded body string, SSE events, headers) stayed reachable for the listener's lifetime: gigabytes/day at loop volume. Finished exchanges now self-remove via finishedSignal, and finalize() releases the raw chunk buffers once the decoded bodies are on the row.
  2. Unbounded flush-time dedupe (the GC-thrash driver). The settle pass built a Set of every part_id ever written (millions of entries, hundreds of MB) on every fallback-carrying flush tick. The committed scan is now restricted to the batch's keys and stops reading once they are all resolved; backfill keeps the unrestricted scan its per-run memo legitimately needs.
  3. Per-new-session whole-table seed scan. The seed scan's partition skip keyed on part.partition?.session_id, but directory partitioning is by source= only, so the guard never fired and every NEW session id (which autonomous loops mint constantly) scanned the entire table to find nothing. A lazily-built committed-session-id index (one session_id-column scan shared per listener) lets unseen sessions skip the scan; a miss older than 10 min rebuilds the index once to cover concurrent backfill writers, inside the seed scan's documented best-effort envelope.
  4. Projector chain state. Per-thread message-id history was an unbounded ordered array whose only read was its tail, with linear includes per message (O(n²) over a long thread); replaced with { seen: Set, last }.
  5. OTLP exporter. pending export promises now self-drain on settle instead of accumulating until a forceFlush() the daemon never calls.

Not fixed here (follow-ups listed in LLP 0204)

Projector state eviction (maps still grow with session count), the claude projector's per-exchange transcript re-parse, and a daemon self-guard for the supervisor's hung-but-alive blind spot.

Verification

  • New regression tests: finished/force-drained exchanges leave the recorder's active set; finalize releases chunk buffers; the settle dedupe scan early-exits at batch keys and keeps uncommitted rows; fresh sessions share one index build and skip the per-session scan.
  • npm test: the only failures are 2 pre-existing on master (verified via stash: leave-command, usage-policy-fold). npm run typecheck clean.
  • ref-check: no new errors (the 2 in touched files are pre-existing LLP 0066#enforcement anchors).

🤖 Generated with Claude Code

…ed dedupe and seed scans (LLP 0204)

Incident 2026-08-08: the daemon grew to ~4.8 GB RSS and wedged, after
~daily supervisor restarts for 9 days. Four leaks fixed:

- recorder.js: finished exchanges never left the active set, retaining
  every proxied body and stream event for the listener's lifetime (the
  daily-death driver); they now self-remove on the finished signal, and
  finalize releases the raw chunk buffers.
- dataset.js: the flush-time settle dedupe materialized EVERY committed
  part_id per fallback-carrying tick; the scan is now restricted to the
  batch's keys and early-exits once they resolve (backfill keeps the
  unrestricted scan its per-run memo needs).
- message_projector.js: every NEW session id paid a whole-table seed scan
  (the session_id partition skip was dead code, directory partitioning is
  source= only); a lazily-built committed-session-id index lets unseen
  sessions skip the scan, with a TTL rebuild on miss for concurrent
  backfill writers. Per-thread chain state drops the unbounded id array
  with linear includes for { seen: Set, last }.
- otlp_exporters.js: pending export promises self-drain on settle instead
  of accumulating until a forceFlush that the daemon never calls.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe philcunliffe added neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) labels Aug 9, 2026
test and others added 2 commits August 9, 2026 17:56
drain()'s force-finish timeout used an unref'd setTimeout so it wouldn't
keep the process alive after normal completion. But an unref'd timer can
be dropped by the event loop before it fires whenever nothing else is
keeping the loop alive, leaving the Promise.race await pending forever
instead of resolving to the timeout outcome. node:test on Node 22
detects this as a dangling promise (cancelledByParent) once the file's
own event loop work is done; Node 24's runner tolerates it, which is why
only the Node 22 CI job failed.

Ref the timer (so the race always settles) and clearTimeout it in a
finally block (so drain() doesn't hold the timer open past its own
return). Fixes the two cancelled subtests in
test/plugins/ai-gateway-recorder-retention.test.js on Node 22.

Co-Authored-By: Claude <noreply@anthropic.com>
…honesty

- mightHaveCommittedRows re-checks `built` after the await so only the
  first stale caller rebuilds the index; concurrent callers past the
  rebuild window now share one rebuild instead of each triggering their
  own whole-table session_id scan.
- scanCommittedSessionIds signals a failed discoverCachePartitions with
  undefined (distinct from a successful empty scan), and a failed build
  is neither cached for the rebuild window nor treated as "definitely no
  committed rows" (mightHaveCommittedRows now errs toward scanning, per
  its own JSDoc).
- Thread an injectable clock through createCommittedSessionIndex and add
  tests pinning the rebuild-after-window behavior and the
  single-rebuild-under-concurrency invariant (verified to fail against
  the pre-fix code: 7 scans instead of 2 for 6 concurrent misses).
- Correct the index comment: in a fresh-session-heavy workload it is one
  whole-table scan per rebuild window, indefinitely, not a single build.
- Record recorder.js's drain-timer ref/unref change and its event-loop
  consequence in LLP 0204's Fix section so the existing @ref stays honest.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 1 — findings (1 blocker, 3 preferences), all fixed

Reviewed head c76eec2, all CI green at that head. Single-family review (Codex not available on this host). Fixes pushed as bdec4a0; CI is green there too.

The four in-scope fixes this PR makes (recorder active removal, chunk release, batch-restricted part_id scan, chain-state Set, OTLP self-drain) read as correct and behaviour-preserving. One newly-added structure had a real defect.


Blocker 1 — committed-session-index rebuild stampede: N concurrent whole-table scans

hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js:348-355

let current = built ?? rebuild()
if ((await current.ids).has(sessionId)) return true
if (Date.now() - current.atMs < SESSION_INDEX_REBUILD_MS) return false
current = rebuild()                       // <- unconditional

current is captured before an await. Every caller that suspended on await current.ids holding the same stale entry re-tests the stale atMs after resuming and rebuilds again, even though another caller already refreshed built. projectExchange is not serialized (finalizeOnce stores but does not await the finalizer promise, per the comment at message_projector.js:264-269), and seedPromises only dedupes within one session id, so N distinct fresh sessions arriving concurrently produce N rebuilds.

Measured against this head, with a stub storage counting discoverCachePartitions / session_id readRows, clock advanced past the window, six fresh sessions via Promise.all:

after prime:                                    discover=1  sessionScans=1
after 6 concurrent fresh sessions past window:  discover=7  sessionScans=7

Each is a full session_id column read across every partition of a multi-GB table, each materializing its own Set of every committed session id, six live at once. That is the workload this LLP names (autonomous loops minting session ids constantly), so it reintroduces a scaled-down version of the churn the PR exists to remove. It also contradicts LLP 0204's own Fix section, which states a stale miss "rebuilds the index once".

Fixed in bdec4a0 (message_projector.js:363-376): re-read built after the await so only the first stale caller rebuilds. Repro drops to discover=2 / sessionScans=2. A new regression test pins the invariant and was confirmed to fail against the unfixed code (7 discovers, expected 2).

Preference 2 — index-build failure reported as "definitely no committed rows", contradicting the code's own JSDoc

message_projector.js:294-296 (JSDoc) vs :377-382, :349-354

The JSDoc says "an index that cannot answer errs toward scanning"; the code did the opposite. scanCommittedSessionIds returned an empty Set when discoverCachePartitions threw, indistinguishable from a successful empty scan, so the per-session seed was skipped — and that failed build was cached for the full 10-minute window, un-seeding every session whose first exchange landed in it. Consequence is duplicate part_id rows on a restart replay, which stays inside this LLP's documented failure envelope (settlement/compaction collapse it), hence preference rather than blocker. But the code did not do what its comment said.

Fixed: scanCommittedSessionIds now returns undefined on a discovery failure (distinct from an empty scan), mightHaveCommittedRows returns true on it, and a failed build clears itself out of built rather than standing in as authoritative.

Preference 3 — @ref LLP 0204#fix on drain() was not supported by the section it cites

recorder.js:94

The ## Fix recorder bullet covered only active removal and chunk release. Nothing there was about the drain timer, unref, or settlement, yet c76eec2 added that behaviour with no doc edit. Dropping unref() is also a real lifecycle change worth recording: drain() now holds the event loop open for up to timeoutMs (5s, source.js:295) when an exchange never settles, where before the process could exit early.

Fixed: added a Fix-section bullet to llp/0204-gateway-daemon-memory-leak.issue.md recording the change and its consequence, so the existing ref is honest. (LLP 0204 is Status: Draft, so still editable.)

Preference 4 — overstated comment, and zero coverage of the rebuild path

message_projector.js:171-177 claimed "one single-column scan, built lazily and shared by every session". In this workload it is one whole-table scan per 10-minute window, indefinitely, since a fresh session id is always a miss. SESSION_INDEX_REBUILD_MS was also hard-coded with no seam, and no test touched the rebuild branch.

Fixed: comment corrected; an injectable clock threaded through createCommittedSessionIndex; two tests added covering rebuild-after-window and single-rebuild-under-concurrency.


Verified clean, no findings

  • Recorder retention: active.delete on finishedSignal is sound; finishedSignal only resolves from finalize(), and the sole non-startExchange retainer is proxy.js's per-request closure, which dies with the request. Nothing outside recorder.js reads requestChunks/responseChunks, so clearing them at finalize() (recorder.js:348-349) cannot drop data: source.js:226 takes the decoded row first.
  • drain() timer logic itself is correct: handle is assigned synchronously by the new Promise executor so it is always defined in finally; settled cannot reject (each finishedSignal is .catch()-wrapped and resolve-only); concurrent drain() calls each own their timer; the timeout path snapshots Array.from(active) before iterating.
  • dedupeByPartId restriction (dataset.js:466-489): membership answers for batch keys are identical to the unrestricted set; in-batch seen.add(key) fold-in preserved; restrictTo.size === 0 short-circuits; key === undefined rows still pass through. Backfill correctly keeps the unrestricted scan. Per-flush I/O is unchanged (the early exit only fires when all batch keys are already committed), and the PR's own test asserts that explicitly rather than overclaiming.
  • Chain state (message_projector.js:507-517, :606-650): { seen, last } is exactly equivalent to the old array, which only pushed when !includes, so the tail never moved on a repeat either. slice(-1) to [last]/[] preserves the 0/1-element previous_message_id contract.
  • OTLP self-drain (otlp_exporters.js:35-42): indexOf resolves correctly (the .finally callback is always async, so pending.push has already run) and cannot race forceFlush's splice(0).
  • Style: no semicolons and no em dashes in any added line.

Verification

Isolated detached worktree at each head; /work/hypaware never switched or edited. Full npm test (3857 pass / 0 fail / 6 pre-existing skips) and npm run typecheck clean after the fixes. CI green at bdec4a0.

Head has moved, so the next tick re-reviews bdec4a0 as round 2.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 2 — findings, zero blockers. Approvable as-is.

Reviewed head bdec4a0, all CI green. This round targeted the round-1 fixes themselves, since those were new code that had never been reviewed. Every interleaving was attacked empirically as well as by reading. No wrong answer, no stampede, no new crash surface.

Nothing here blocks the merge. The four items below are preference-grade and are deliberately not fixed: this PR is at the two-round review cap, and pushing another commit would move the head into an unreviewed state at the cap, forcing a request-changes verdict on a PR that has no blockers. They are recorded here for you and @philcunliffe to take or leave.


Round-1 fixes: verified correct

  • Stampede fix is genuinely pinned. Patching mightHaveCommittedRows back to its pre-fix body makes the new test fail (not ok 31 - N concurrent fresh-session misses past the rebuild window share one rebuild). The test is real, not vacuous.
  • const next = built === current || !built ? rebuild() : built is sound. built is only ever assigned by rebuild() (monotonic) or cleared to undefined, so next is never older than current; no false negative is reachable through it. With the clock crossing the window while the first build is in flight, 6 concurrent callers produced 2 discoverCachePartitions calls, not 7.
  • The built === attempt guard is sufficient, in fact belt-and-braces: rebuild() is never called while another attempt is in flight (callers only rebuild after awaiting), and the .then clear is registered before any awaiter, so it always lands before a caller can observe the failed attempt. A later successful rebuild cannot be clobbered, and built cannot be left pointing at a failed attempt.
  • return true on failure reaches the intended fallback. A first projection against a storage whose discoverCachePartitions throws once makes 2 discover calls (index build + the per-session scanCommittedMessageIds fallback), the row is still emitted, and the next projection retries the build rather than trusting the failed one.
  • No false negative for a session that does have committed rows across the window boundary under concurrency: the probe emitted 0 rows for the committed session (correctly deduped) and 1 for the fresh one.

Preferences (non-blocking)

1. message_projector.js:408 — the round-1 failure-mode fix has zero test coverage.
Reverting return undefined to return ids, which re-introduces exactly the defect round 1 found, leaves the entire suite green (3857 pass, 0 fail). The existing "a throwing storage degrades to not-seeded" test passes either way, because it only asserts rows aren't dropped. Behaviour added in response to a review finding is worth regression-pinning. Suggested test: a storage whose discoverCachePartitions throws on call 1 and succeeds after, asserting the second projection rebuilds rather than trusting the failed build.

2. message_projector.js:344attempt.ids.then(...) has no rejection handler.
scanCommittedSessionIds is written not to reject (both awaits are inside try/catch), so this is unreachable today. But if it ever did reject, the derived promise is unhandled and Node kills the daemon, which is a worse outcome than pre-fix where the rejection had an awaiter. attempt.ids.then(handler, () => {}) makes the guard total for two characters.

3. message_projector.js:337atMs is stamped at scan start, not completion.
If a full session_id scan ever exceeds SESSION_INDEX_REBUILD_MS, the index is stale the moment it resolves and rebuilds back-to-back indefinitely. Verified this does not stampede (one chained rebuild at a time), and it remains strictly better than the pre-PR one-scan-per-session, so it is not a regression. Stamping atMs on completion would close it.

4. llp/0204-gateway-daemon-memory-leak.issue.md:96 — follow-ups omit seedPromises.
The "Projector state eviction" follow-up enumerates seenMessages, messageIdsByConversation, conversationStartedAt and toolCallLookupByConversation, but not seedPromises (Map<session_id, Promise<void>>, message_projector.js:170), which holds one entry per session for the listener's lifetime: same file, same unbounded-in-session-count class. Small per entry; doc completeness only.

Fresh full-diff pass (4fb95dc to bdec4a0) — nothing new

  • dataset.js restricted scan: batchKeys is a superset of every defined key in the batch and seen a subset of batchKeys, so membership answers are exact for every row; in-batch dedupe preserved; the seen.size >= restrictTo.size early exit is sound; keyless rows still pass through; backfill (line 590) correctly keeps the unrestricted scan, matching its JSDoc.
  • recorder.js: finalize() is idempotent via _cachedRow, and the chunk arrays are cleared only after the last read of them. Every termination path in proxy.js (upstreamRes end/error, upstreamReq error, req error, res close/client_aborted) calls finalizeOnce, so no exchange can be stranded in active by the new signal-driven removal.
  • otlp_exporters.js: forceFlush splices in place, so a concurrent settle's indexOf returns -1 and cannot corrupt a later batch; request is always assigned before the finally callback runs.
  • Conventions: no em dashes, no code semicolons, JSDoc-only types, no inline import() types. All five @ref LLP 0204#fix annotations map to real Fix bullets.

Commands run

Command Result
npm test at bdec4a0 3857 pass / 0 fail / 6 skipped, exit 0
npm run typecheck clean, exit 0
npm test with fix #2 reverted 3857 pass / 0 fail (proves the coverage gap)
node --test with pre-fix mightHaveCommittedRows not ok 31 (proves the concurrency test is genuine)
4-scenario concurrency probe (failing build; 6 concurrent callers on a failing build; committed-session dedupe past the window; clock crossing the window mid-build) all correct, no unhandled rejections

@philcunliffe philcunliffe added the neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides) label Aug 9, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Verdict: changes-requestedno blockers, residual preferences handed back

Two review rounds are exhausted at head bdec4a0, and the round-2 record still carries findings, so the reconciler hands them back to the author rather than deferring them to a follow-up issue (this is a contributor's PR, not neutral's).

Read this label as "there are open review comments", not "this PR is broken." To be explicit about the state:

  • Zero blockers. Round 2 found nothing that could cause a production defect.
  • All CI is green at bdec4a0 (test 22/24, typecheck 22/24, duplicate-numbers).
  • The round-1 blocker (the committed-session-index rebuild stampede, 7 whole-table scans for 6 concurrent sessions) is fixed and regression-pinned by a test confirmed to fail against the unfixed code.
  • The four residual items are preference-grade: one missing regression test for the round-1 failure-mode fix, one two-character defensive rejection handler, one atMs-stamped-at-start nit that is not a regression, and one doc-completeness omission in the LLP follow-ups list. All four, with evidence and suggested fixes, are in the round-2 review record directly above.

This does not block a merge. Readying and merging a contributor's PR is the maintainer's call, never neutral's, and @philcunliffe can merge as-is if these preferences aren't worth another round. Neutral's own read is that the PR is approvable.

To take another round: push to the branch. A contributor push moves the head, which re-opens the review ladder from the top and clears this verdict automatically. Neutral monitors this thread and will re-engage on its next tick, so a reply here works too.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Residual review findings tracked; changes-requested cleared

The maintainer is merging this PR, so the four non-blocking findings from the round-2 review are now tracked as follow-up issues rather than held against the head:

Issue Finding Delegated
#684 Failed-build fallback (return undefined) has no regression coverage: reverting it leaves the suite green neutral:fix
#685 Self-clearing guard has no rejection handler; a rejecting scan would take down the daemon (unreachable today) neutral:fix
#686 atMs stamped at scan start, so a scan longer than the rebuild window never serves a cache hit neutral:fix
#687 LLP 0204's projector-state-eviction follow-up omits seedPromises doc-only, unlabelled

#687 is deliberately left without neutral:fix: the autonomous fix path gates every PR on a regression test that fails before the fix and passes after, and a documentation edit cannot produce one, so labelling it would drive it straight to stuck.

Swapped neutral:changes-requested for neutral:approved. The verdict marker in the description is left as-is, since it is the record of what the review concluded at bdec4a0 and should not be rewritten after the fact. Nothing outstanding blocks this merge: zero blockers, CI green at bdec4a0, and the round-1 stampede fix is regression-pinned.

@philcunliffe philcunliffe added neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) and removed neutral:changes-requested neutral reviewed an adopted PR and requests changes (non-binding; maintainer decides) labels Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant