fix(sdk): stop the broker and its session hosts from leaking, wedging, and failing silently - #4035
Merged
Yeachan-Heo merged 6 commits intoAug 9, 2026
Conversation
…uggable A machine accumulated 54 `.broker.lock.stale-*` tombstones, a 126 MB `lifecycle-ledger.jsonl.corrupt` next to a 6 MB live ledger, and a dead-owner lock that wedged broker startup. The reclaim path renamed dead locks to tombstones but nothing ever removed them, and the quarantine sidecar was appended to forever. The broker now reaps abandoned lock artifacts on startup after it owns the lock, the corrupt sidecar rotates at a bounded size, and the spawn no longer throws away its own diagnostics: `stdio: "ignore"` is replaced by a truncated per-spawn log whose stderr tail is attached to the discovery failure. Lore-id: 9d24af61 Constraint: reaping is fail-closed -- owner-alive, unreadable, or ambiguous artifacts are kept with a reason Constraint: only the lock holder reaps, so concurrent brokers cannot race the removal Constraint: corruption evidence must survive rotation, not be silently dropped Rejected: reap before acquiring the lock | racing brokers would delete each other's artifacts Rejected: delete the corrupt sidecar outright | destroys the only evidence of what was quarantined Confidence: high Scope-risk: narrow Reversibility: easy Tested: aged tombstones removed, live-owner and unreadable artifacts retained with reasons Tested: corrupt quarantine rotates at the cap instead of growing unbounded Tested: a broker that exits before discovery now reports why Not-tested: multi-machine concurrent broker contention
Detached session hosts had no exit path while the broker stayed alive. The one automatic reaper, watchSessionHostBrokerLiveness, is conditioned on broker discovery going ABSENT, and the default warm broker never disappears — so the grace window never opened. The other leg, session.close, is never issued by ordinary SDK usage. The reporter measured 119 leaked hosts holding 4.1 GB under one healthy 6-day broker; this machine reached 29 hosts for 9 agents with 20 of them holding zero connections, within hours. Hosts now self-reap after a bounded idle interval with no attached client, and the broker drops registrations whose host is provably gone. The broker-absence watcher is untouched; this is a second, independent bound. Lore-id: e81c4a37 Constraint: a host that has never seen its first attachment must not be reaped -- the client may still be dialing Constraint: attachment is decided from the host's own subscription state, never by scraping ps or lsof Constraint: detached + unref stays -- the spawn shape is deliberate, the missing bound was the bug Rejected: rely on session.close alone | ordinary SDK usage never issues it, which is exactly how the leak starts Rejected: cap concurrent hosts | evicts live sessions under legitimate parallel load Confidence: high Scope-risk: narrow Reversibility: easy Tested: a host whose client detaches exits after the idle bound Tested: a host with an attached client is never reaped, driven well past the bound Tested: a freshly spawned host survives the first-attach grace Tested: broker-absence behavior unchanged Not-tested: multi-day accumulation against a live warm broker
Every broker-launched session ran the local memory backend's phase-1 stage-1 LLM jobs synchronously inside the 10s readiness window. Whenever the queue had pending rollouts, those calls ate the whole budget, the child was killed at the cutoff, and the broker answered "No ready SDK endpoint remains available." Six such failures landed in ~15h; the same shape recurred while running this batch. Lifecycle sessions now defer memory startup as an invariant and resume it once readiness is published, unawaited, with failure logged rather than fatal. The ACP carve-out in main.ts goes away with it, and postmortem stops collapsing a thrown record to "[object Object]" so the next cutoff keeps its phase/reason. Lore-id: 3f6ad920 Constraint: session readiness must never depend on LLM calls -- startup work is unbounded Constraint: memory failure after readiness is a degraded-memory condition, not a startup failure Rejected: widen READY_TIMEOUT_MS | startup work scales with rollout backlog and provider latency, so any constant is just a wider flake window Rejected: keep deferral a caller option for lifecycle sessions | the broker deadline makes it an invariant, not a choice Confidence: high Scope-risk: narrow Reversibility: easy Directive: never await memory startup on a path that publishes readiness Tested: readiness is published before a memory start that outlives the budget resolves Tested: a post-readiness memory rejection neither tears down the session nor becomes an unhandled rejection Tested: a non-Error throwable keeps its payload in the crash record Not-tested: live broker under a real multi-day rollout backlog
`session/new` failed intermittently with "Lifecycle terminal evidence could not be verified after persistence" — ~29% in a large tree, 0% in small ones, and non-deterministic. One failed probe permanently poisons an ACP host: the provider goes `status: "error"` for the daemon's whole lifetime, so gjc cannot be selected until the host restarts. Observed on this machine today. The race: a recovery pass stamps `terminal_uncertain` on every row it finds mid-flight, so a slow operation can have that marker interleaved before its own terminal row lands. `readTerminal` treated only `terminal_ok`/`terminal_error` as terminal, so it reported a row that WAS on disk as unpersisted and replaced its real reason with the generic uncertainty error. Reading back the durable record fixes the report at its source. Lore-id: b6d02e59 Constraint: genuinely unverifiable evidence must still report terminal_uncertain -- the signal is not suppressed Constraint: a proven terminal row stays immutable and admits no successor Rejected: retry the verification | the slow path is normal on a large tree, so it must be handled, not out-waited Rejected: widen the startup scan deadline | being slow on a big tree is legitimate, not the defect Confidence: high Scope-risk: narrow Reversibility: easy Tested: the owner's terminal record is read after a concurrent recovery stamps its in-flight row Tested: a durable terminal_uncertain is read back instead of reported unpersisted Tested: proof is still withheld when a persisted row cannot be reproduced Not-tested: live repeated session/new against a multi-gigabyte home directory
This was referenced Aug 8, 2026
Merged
Collaborator
Author
|
Self-review before asking anyone else: REQUEST_CHANGES on my own PR. A red-team pass found a reachable way for the new idle reaper to kill a host that still has clients attached. I reproduced it independently. Blocker: the attachment reader is a module global, so one runtime's teardown blinds another's watcher
export function publishSessionHostAttachmentReader(reader: SessionHostAttachmentReader | undefined): void {
sessionHostAttachmentReader = reader; // module-global, single slot
}
The watcher ( const attached = deps.readAttachedClients();
if (attached === undefined) {
if (!everAttached && now() - startedAt >= firstAttachGraceMs) return;
}So a host whose clients are attached — but whose first attachment was never observed, e.g. a client that dials and immediately issues Reproduced against the real exported The doc comment on What the fix has to beRetraction must be identity-scoped: only the runtime that installed a reader may clear it (compare-and-clear, or a per-runtime handle instead of a single global slot). Separately, Not merging until that is in with a regression test. The reviewer also raised ledger-reopen, spawn-log race, and cross-realm postmortem findings which I am verifying separately; I will post those outcomes too, including any I reject. Reviews of the other split PRs are in flight. |
The session-host attachment reader was a single module-global slot, and teardown retracted it unconditionally — any second SDK runtime in the process that stopped cleared the live runtime's reader. The idle watcher then read undefined, treated manufactured absence as absence of evidence, and a host whose first attachment was never observed fell through the first-attach bound: runSessionHost stopped a host with clients attached, mid-turn. Retraction is now compare-and-clear: only the runtime that installed a reader may remove it, and a stale retraction is a no-op. The watcher no longer reaps on ambiguity alone while work is observably in flight; a host whose SDK runtime never came up is still bounded by the first-attach grace. The spawn log race and the cross-realm postmortem collapse found in the same review are fixed alongside. Lore-id: 1e9d4c72 Constraint: a host idle with no attached client still self-reaps (Yeachan-Heo#4010 holds) Constraint: a host with an attached client is never reaped Rejected: per-runtime reader registry keyed by id | a single compare-and-clear slot is sufficient and keeps the surface flat Confidence: high Scope-risk: narrow Reversibility: easy Tested: a second runtime's retraction does not clear a live reader Tested: attached-but-unobserved hosts are not reaped; runtimes that never come up are still bounded Tested: concurrent brokers no longer race the per-spawn log Tested: a non-Error throwable keeps its payload across realms Not-tested: multi-runtime hosts under production load
Collaborator
Author
|
Blocker fixed in |
The three new evidence tests asserted a pristine module global, but a shard peer may legitimately hold its own publication — and identity-scoped retraction is precisely what forbids clearing it. Assertions are now relative to the observed baseline, which is also the truer statement of the contract. Lore-id: 7d3e9a56 Confidence: high Scope-risk: narrow Reversibility: easy Tested: file passes alone and alongside the full sdk-* shard locally
Yeachan-Heo
reviewed
Aug 9, 2026
Collaborator
Author
|
Understood — holding. Acknowledging the HOLD and adding the overlap detail I can measure, plus one thing you need to know that is my fault. The overlap, measuredAgainst #4015's file list, the only file this PR shares is What actually made #4015 DIRTYIt was my merge, not drift. The good news is that the conflict is trivial: $ git merge-tree --write-tree --name-only origin/dev origin/issue-3963-sdk-broker
packages/coding-agent/CHANGELOG.md
CONFLICT (content): Merge conflict in packages/coding-agent/CHANGELOG.md
Auto-merging packages/coding-agent/src/sdk/broker/broker.ts # clean
Disclosure on #4060#4060 should not have been merged when it was. My harvest script read a dead reviewer process as an approval — it grepped That and four sibling findings are written up in #4063. A fix for the drain path is in progress on This PR stays parked until #4015 is reconciled and green at its exact head. |
This was referenced Aug 9, 2026
probepark
added a commit
to probepark/gajae-code
that referenced
this pull request
Aug 9, 2026
… at a time `errorForDiagnostic` returned a same-realm `Error` raw, skipping the defensive field snapshot every other shape received, and `formatFatalError` then read `name`, `message` and `stack` unguarded. An `Error` whose `message` getter throws -- a lazily computed message is an ordinary pattern, not only a hostile one -- destroyed the entire crash record at the moment it mattered most. The bare `instanceof` could itself throw for a `Proxy` with a `getPrototypeOf` trap. This module was hardened ten times, one producer at a time: cross-realm values, then `Object.prototype.toString`, then a hostile `Proxy`, then same-realm `Error`. Each round left a sibling path, because there are unbounded ways to construct a bad throwable and only a few places that read one. The guard now lives at that single read: whatever answers is kept, whatever refuses is named unreadable, and the outermost layer does not depend on the inner ones holding -- which is the belief every previous round was built on. Developed during Yeachan-Heo#4035 and lost when that PR merged from an older head. Constraint: crash recording must never itself throw -- it is the last diagnostic Constraint: an unreadable field is reported, never silently dropped Rejected: guarding each consumer | ten rounds proved the producers are unbounded Rejected: the `instanceof Error` fast path | it was the path that skipped the guard Confidence: high Scope-risk: narrow Reversibility: easy Tested: throwing message/name/stack getters, a Proxy answering nothing, cross-realm objects, plain records, primitives, and each field read exactly once Not-tested: a getter that throws only on its second read
Yeachan-Heo
pushed a commit
that referenced
this pull request
Aug 9, 2026
… keeps (#4081) * fix(utils): make the crash reader total instead of guarding one shape at a time `errorForDiagnostic` returned a same-realm `Error` raw, skipping the defensive field snapshot every other shape received, and `formatFatalError` then read `name`, `message` and `stack` unguarded. An `Error` whose `message` getter throws -- a lazily computed message is an ordinary pattern, not only a hostile one -- destroyed the entire crash record at the moment it mattered most. The bare `instanceof` could itself throw for a `Proxy` with a `getPrototypeOf` trap. This module was hardened ten times, one producer at a time: cross-realm values, then `Object.prototype.toString`, then a hostile `Proxy`, then same-realm `Error`. Each round left a sibling path, because there are unbounded ways to construct a bad throwable and only a few places that read one. The guard now lives at that single read: whatever answers is kept, whatever refuses is named unreadable, and the outermost layer does not depend on the inner ones holding -- which is the belief every previous round was built on. Developed during #4035 and lost when that PR merged from an older head. Constraint: crash recording must never itself throw -- it is the last diagnostic Constraint: an unreadable field is reported, never silently dropped Rejected: guarding each consumer | ten rounds proved the producers are unbounded Rejected: the `instanceof Error` fast path | it was the path that skipped the guard Confidence: high Scope-risk: narrow Reversibility: easy Tested: throwing message/name/stack getters, a Proxy answering nothing, cross-realm objects, plain records, primitives, and each field read exactly once Not-tested: a getter that throws only on its second read * fix(utils): read each crash field independently instead of gating on two The previous commit claimed "whatever answers is kept, whatever refuses is named unreadable", and the code did not do that. A strict `name`/`message` gate returned before `stack` was ever read, so a throwable whose `name` refused lost its stack too -- a partial refusal discarding the parts that were readable, which is the exact failure eleven rounds of work existed to end. An all-or-nothing gate is the `instanceof` fast path wearing different clothes. Each of `name`, `message` and `stack` is now attempted on its own. One throwing does not suppress its siblings; a field that refuses is named unreadable while the rest survive. The record degrades field by field and never collapses. Constraint: a partial refusal MUST NOT discard readable siblings Rejected: gating on name and message together | that was the defect Confidence: high Scope-risk: narrow Reversibility: easy Tested: name refuses while stack reads; a Proxy throwing from getPrototypeOf; each field read exactly once Not-tested: a getter that throws only on its second read * fix(utils): keep every field a throwable yields, without classifying it first Twelve rounds oscillated between two failures. Too strict: a gate demanded more than it needed and discarded a throwable that could have answered partially -- `instanceof Error`, then the `name`/`message` gate. Too loose: a gate accepted anything vaguely error-shaped and rendered only those fields -- `carriesErrorField` promoted `{ phase, reason, message }` to error-shaped on its `message` alone and dropped `phase` and `reason`, which is the exact record that started this effort. Both branches existed because the code decided what kind of thing it had before deciding what to keep. There is no longer a classification step to get wrong: the reader keeps whatever any field yields, error-shaped or not, and names what refused. A record degrades field by field and never collapses. Constraint: no classification decides what is worth keeping Constraint: a partial refusal never discards readable siblings Rejected: tuning the gate again | twelve rounds of tuning produced this oscillation Confidence: high Scope-risk: narrow Reversibility: easy Tested: {phase,reason,message} keeps all three; a same-realm Error whose name throws still yields its stack; a Proxy refusing everything yields a record naming it unreadable; no field is read twice Not-tested: a getter that throws only on its second read * fix(utils): emit every crash field from one path, and keep the identifying ones Removing the classification step left two emission paths behind: one rendered a payload and hard-set `stack: ""`, the other rendered the name/message/stack triple. Any Error carrying an own enumerable property took the payload branch -- which is essentially every Node and Bun system error, plus NonZeroExitError, AbortError and TimeoutError. The errors that actually crash this product were the ones losing their stack. There is one emission path now. A payload and a stack are not alternatives, so neither suppresses the other, and `stack: ""` survives only where nothing was readable at all. Truncation order was wrong for the same reason. `capturePayload` serialized `Object.keys` order first and appended name/message/stack last, while `boundCrashRecord` keeps the head -- so one oversized early property evicted exactly the three fields most likely to identify the crash, in the case where they matter most. Identifying fields now lead. A field holding a function or symbol was read successfully and then dropped with no marker, contradicting the claim that an unreadable field is reported rather than silently discarded. It is reported. Constraint: one emission path; no branch decides which fields exist Constraint: truncation evicts context before identity Rejected: keeping the payload/triple split | it is the classification step under another name Confidence: high Scope-risk: narrow Reversibility: easy Tested: Node system error with enumerable context and a real stack, NonZeroExitError with own fields, cross-realm error, plain record with stack and context, functions and symbols reported, oversized early property Not-tested: a getter that throws only on its second read * fix(utils): redact and bound own error fields instead of emitting them verbatim Fourteen rounds argued that discarding a field is a bug. True for name, message and stack; false the moment it was generalized to arbitrary own properties, because own error fields are exactly where credentials live -- config.headers.authorization, tokens, cookies, signed URLs. HEAD before: 200464 bytes, 1 match for 'Bearer sk-' origin/dev: 191 bytes, 0 matches HEAD after: 206 bytes, 0 matches Keeping a field and keeping it verbatim are separate decisions and only the first had been made. Own properties are now redacted for credential shapes and bounded per field rather than only in aggregate -- one 200 KB body defeated a single whole-record cap and evicted the identifying fields the record exists for. The payload also re-emitted name/message/stack that the header already carried, roughly 5x amplification, so NonZeroExitError(7, 20KB) lost exitCode entirely. It survives now. The previous commit's claim that an empty stack survives only where nothing was readable was false: {name,message,other} yielded an empty stack line. Corrected. Constraint: no own property is emitted verbatim -- redacted and bounded per field Constraint: identity survives truncation; context is evicted first Rejected: preserving own fields as-is | that is the leak Confidence: high Scope-risk: narrow Reversibility: easy Tested: credential-shaped own field redacted, 200KB body bounded, NonZeroExitError keeps exitCode at 20KB, empty-stack claim corrected Not-tested: a credential shape the redactor does not recognize
6 tasks
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scoped replacement for the SDK broker/host lifecycle portion of the closed #4021. Four commits, 15 files, one subsystem: the broker and its session hosts must not leak, wedge, or fail without saying why.
Fixes #3963, #4010, #4013, #3903. Every defect was reproduced on a real machine; three of them were actively breaking agent launches while this work was being done.
The defects
1.
session/newintermittently reportedterminal_uncertain, poisoning an ACP host (dd563b442, #3903)~29% failure in a large tree, 0% in small ones, non-deterministic. One failed probe marks the provider
status: "error"for a host's entire lifetime, so gjc cannot be selected until the host restarts.The race: a recovery pass stamps
terminal_uncertainon every row it finds mid-flight, so a slow operation can have that marker interleaved before its own terminal row lands.readTerminaltreated onlyterminal_ok/terminal_erroras terminal, so it reported a row that was on disk as unpersisted and replaced its real reason with the generic uncertainty error. Reading back the durable record fixes the report at its source. A genuinely unverifiable terminal still reportsterminal_uncertain— the signal is not suppressed.2. Memory phase-1 LLM jobs ran inside the readiness window (
97b77562a, #4013)Every broker-launched session ran the local memory backend's stage-1 jobs synchronously inside the 10 s readiness budget. Whenever the queue had pending rollouts, those model calls ate the whole budget, the child was killed at the cutoff, and the broker answered
No ready SDK endpoint remains available.Six such failures in ~15 h, and it recurred while running this very batch — it is why parallel agent launches intermittently refused to start.Lifecycle sessions now defer memory startup as an invariant and resume it once readiness is published, unawaited, with failure logged rather than fatal. The ACP carve-out in
main.tsgoes away with it.Deliberately not done: widening
READY_TIMEOUT_MS. Startup work scales with rollout backlog and provider latency, so any constant is just a wider flake window.Secondary half:
handleFatalErrorcollapsed a thrown record to[object Object], which is why six earlier occurrences left no diagnostic at all. Non-Errorthrowables now keep their payload.3. Session hosts leaked indefinitely under a healthy broker (
5b4c42576, #4010)watchSessionHostBrokerLivenessis conditioned on broker discovery going absent, and the default warm broker never disappears — so the grace window never opened. The other leg,session.close, is never issued by ordinary SDK usage. The reporter measured 119 leaked hosts holding 4.1 GB under one healthy 6-day broker; this machine reached 29 hosts for 9 agents with 20 holding zero connections, within hours.Hosts now self-reap after a bounded idle interval with no attached client, and the broker drops registrations whose host is provably gone. Attachment is decided from the host's own subscription state — never by scraping
psorlsof— and a host that has not yet seen its first attachment survives a startup grace.detached: true+unref()stays; the spawn shape is deliberate, the missing bound was the bug.4. Stale broker lock artifacts accumulated, and a clean exit said nothing (
b51ca2256, #3963)54
.broker.lock.stale-*tombstones, a 126 MBlifecycle-ledger.jsonl.corruptbeside a 6 MB live ledger, and a dead-owner lock that wedged startup — withstdio: "ignore"on the spawn, the caller saw only "exited before discovery". Confirmed live: 17 zombie processes from a deleted checkout held the lock and made every launch fail withCLI entrypoint is not a readable regular file.Startup now reaps abandoned lock artifacts (after acquiring the lock, so concurrent brokers cannot race the removal), the corrupt sidecar rotates at a bounded size preserving the evidence, and the spawn writes a truncated per-spawn log whose stderr tail is attached to the discovery failure. Reaping is fail-closed: owner-alive, unreadable, or ambiguous artifacts are kept with a reason.
Verification
Load-bearing proof — restoring
packages/coding-agent/srcandpackages/utils/srcfromorigin/devand re-running the four new suites:Tests run against temp state roots only; the real
~/.gjcis never touched.Relationship to #4021
#4021 bundled twelve unrelated defects into 53 files and was closed with the instruction to open fresh, scoped PRs. This is the third, after #4031 (ACP turn lifecycle) and #4033 (chat daemon reconnect). Remaining: gc/disk retention, tool diagnostics.
Closes #3963, #4010, #4013, #3903