fix(sdk): bound a session host that attached and then lost its endpoint

[probepark]

Fixes #4126.

The defect

The session-host idle reaper added by #4035 (which closed #4010) could not fire in the case it was written for. This is a residual hole in my own fix, not someone else's regression.

watchSessionHostClientAttachment (packages/coding-agent/src/commands/sdk.ts:183-199) enumerated over (attached, everAttached) like this:

attached	everAttached	bound applied
> 0	any	none needed — a client is present
0	true	idleGraceMs (30 min)
0	false	firstAttachGraceMs (60 min)
undefined	false	firstAttachGraceMs (60 min)
undefined	true	none — loops forever

The !everAttached guard short-circuited and every other branch was else if, so the last row matched nothing and the host slept until reboot.

That row is the ordinary lifecycle, not a corner case. sessionHostAttachedClients (sdk/broker/lifecycle.ts:4489-4502) sums over sessionHostRuntimes and returns undefined when the set is empty; its own doc comment names the condition — undefined means "before startup, after teardown, or when every reader itself fails". A client attaches (everAttached = true), prompts, disconnects, its runtime retracts the registration, and the count reads undefined from then on. The hosts that did real work were exactly the ones exempted from every bound.

Measured

On a binary built after #4035 merged, with the reaper provably compiled in (strings finds watchSessionHostClientAttachment and SESSION_HOST_DETACHED_IDLE_GRACE_MS): 26 leaked session-host-internal children under one healthy broker, oldest 13h09m, ~3.1 GB RSS, each burning ~5 minutes of CPU across 13 hours — idle, not wedged. Grace windows are 30 and 60 minutes; these ran 26x over. Several belonged to worktrees whose PRs had merged or closed 13 hours earlier.

The change

Two semantic lines in the ambiguous branch:

• the first-attach bound now accrues for every host, not only never-attached ones
• an observed client refreshes unattendedSince, so the bound is measured from the last positive sign that somebody wanted this host rather than from process start

Deliberately preserved: a transient unreadable poll still never reaps instantly, and readWorkInFlight() still holds a host open mid-prompt. Neither exported grace constant changes value.

Verification

bun test packages/coding-agent/test/sdk-session-host-idle-reap.test.ts → 16 pass / 0 fail.

Load-bearing, proven by mutation:

tree	result
with the fix	16 pass / 0 fail
sdk.ts alone restored from origin/dev	15 pass / 1 fail

The single failure is exactly the new row — state table: unreadable count after attachment bounded by first-attach grace. The other fifteen pass either way, which is the point: the pre-existing suite could not see this hole, so new coverage had to be the thing that catches it.

The function is fully injectable (now, sleep, readAttachedClients, readWorkInFlight), so the suite drives it on a virtual clock — no processes, no real timers.

Protocol note

ACP is silent on agent-side idle reclamation. v1 defines only the client-driven path: clients MUST call session/close, after which the agent frees resources. That leaves no recovery when a client never closes — it drops, crashes, or is killed — so an agent that frees only on session/close leaks by construction under an imperfect client. A local bound is not a protocol violation; it is the only defense the spec leaves available.

Base CI

dev is red across shards 1-8 (#4124). Red on this head matching that signature is inherited, not from this branch.

[probepark]

Additional verification the unit suite cannot give, since it injects fake grace windows: driving the real watcher against the shipped constants on a compressed clock.

idle grace   = 30 min
first-attach = 60 min
attached-then-unreadable: reaped at 1.00h
client-stays-attached:    STILL ALIVE after 240h
never-attached:           reaped at 1.00h

The leak shape — one client attaches, the runtime retracts, the count reads undefined from then on — now terminates at exactly the 60-minute first-attach bound instead of running forever. That is the row that had no bound at all.

The middle line is the control, and it is the intended result, not a second leak: a host whose client is still attached must never be reaped. My probe labelled that case "LEAK" before I read it; the label was wrong, the behaviour is right. Recording it because a run where every line says PASS usually means the probe cannot fail.

This matters beyond the unit tests because a disagreement between the constants and the loop would show up here and nowhere else — the suite passes injected windows, so it never exercises the shipped 30/60.

[probepark]

Housekeeping note, since this PR briefly showed zero files and auto-closed: that was my mistake, not a CI or GitHub fault.

CI was failing with Exact-head CI requires this PR head to contain base 10144dc8d — dev had moved. While rebasing, the lane worktree still had an agent running in it; the branch ended up reset to the old base with my work left uncommitted in the working tree, and I force-pushed that state. GitHub saw a PR with no diff and closed it.

Nothing was lost — the change was still in the worktree. Recovered by stashing, hard-resetting to current origin/dev, popping, and re-verifying from scratch rather than trusting the earlier run:

tree	result
with the fix, on 10144dc8d	16 pass / 0 fail
sdk.ts alone restored from origin/dev	15 pass / 1 fail — state table: unreadable count after attachment bounded by first-attach grace

Head is now be0464bf2, containing current dev. The lesson is mine to keep: never rebase a worktree while an agent still holds it.

The three sibling PRs (#4105, #4108, #4128) hit the same stale-base failure and were rebased onto 10144dc8d as well, each re-verified on its own suite.

[probepark]

Design justification, since the obvious objection to this PR is "the client should just call session/close" — that is true and it is not enough.

Comparable protocols converge on layered reclamation, not a single mechanism:

1. Explicit close — the graceful path. ACP v1: clients MUST call session/close, then the agent frees resources.
2. Parent/transport-death detection — LSP has the client pass processId in initialize precisely so the server can exit when its client dies without saying goodbye (initialize).
3. Guarded idle eviction — the last resort, for when neither of the above fired.

gjc has layer 1, and watchSessionHostBrokerLiveness is a partial layer 2 — but it watches the broker, and in the default warm-broker configuration the broker never dies. That is the exact gap #4010 was filed for. Layer 3 is what #4035 added, and this PR is what makes it reachable.

Worth naming explicitly: this PR does not add the layer that would have prevented all 26 leaks most directly. A host that watched the process of the client that dialed it — LSP's processId pattern — would reap in seconds rather than 60 minutes, because in every one of those 26 cases the client process had already exited. I am not doing that here: it changes the host/client contract and belongs in its own issue with its own review, and the timing bound is needed regardless as the backstop for a client that hangs rather than exits.

So this is deliberately the cheapest correct layer, not the best possible one. If a maintainer wants the processId watch, that is a separate PR worth having and I will file it — but the unbounded state should not stay open while that is decided.

[Yeachan-Heo]

Terminal review, owning #4126 on behalf of the repo owner. The issue has a live implementation PR: #4130 by @probepark.

State reconciliation. #4130 was briefly closed at 01:08:22Z during a head_ref_force_pushed (branch rewrite from 18e5433b → be0464bf), then reopened at 01:11:35Z by the author. That was supersession during a rewrite, not an accidental close, and not an invalid base: the head be0464bf sits exactly one commit above current dev (10144dc8d4). The issue remains open and unmerged.

The fix closes the state table. The (attached === undefined, everAttached === true) row previously matched no branch — every other branch was else if, so a host that ever served a client and then lost its runtime read undefined forever and slept until reboot. That is the ordinary lifecycle, not a corner: sessionHostAttachedClients sums over registered runtimes, so post-teardown the count is unreadable for good. The fix removes the !everAttached guard so the first-attach bound accrues for every host, and an observed client refreshes unattendedSince so the bound is measured from the last positive sign (attachment or work in flight) rather than from process start. A transient unreadable poll still never reaps instantly, and live work still holds the host open. No grace constant changes value.

Independent verification (virtual clock, no processes).

• bun test packages/coding-agent/test/sdk-session-host-idle-reap.test.ts at head be0464bf: 16 pass / 0 fail.
• Restoring src/commands/sdk.ts alone from origin/dev: 15 pass / 1 fail — the single failure is exactly the new row (state table: an unreadable count after an attachment is bounded by the first-attach grace, failing with "host never reaped"). The other fifteen pass either way, confirming the pre-existing suite could not see the hole and the new coverage is what catches it.
• bun --cwd=packages/coding-agent run check (biome + tsc --noEmit): exit 0.
• CI at exact head be0464bf (re-run after the earlier run was cancelled mid native-build, which cascaded a evidence producer failure): all check runs green, including gjc-state-gates, Affected path validation / plan, Affected path validation / native-build, Affected path validation / evidence producer, Affected path validation / test:…sdk-session-host-idle-reap.test.ts, and Local public surfaces. The re-run completed success; the earlier red was cancellation cascade, not a code failure.

VERDICT: APPROVE — the leak row is bounded, the regression is deterministic and mutation-proven, and the safety properties (no instant reap on transient ambiguity, no reap while work is in flight, no change to the shipped 30/60 minute constants) hold.

Leaving merge to the maintainers per ownership policy; not merging.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

Updated terminal review at current head 1a679c33 (the author force-pushed a rewritten commit after the earlier review; the PR head moved from be0464bf to 1a679c33, same parent 10144dc8d4 on dev).

The rewritten fix is better than the version I reviewed before, and it is the option the issue explicitly sanctioned. Instead of accruing the ambiguous state against the first-attach bound for every host, it unifies an observed zero and unreadable evidence as the same absence once a client has been seen — a runtime that retracts its registration during teardown stops publishing a count instead of reporting zero, so (undefined, everAttached=true) is the ordinary lifecycle, not a corner. Both now accrue against detachedSince/idleGraceMs, which removes a special case rather than adding one, and work in flight now clears the detached clock too, so a host mid-prompt is never reaped regardless of whether its count reads zero or stops being readable. Every reachable state carries a finite bound:

attached	everAttached	bound
> 0	any	none — client present / work defers
0	true	idleGraceMs via detachedSince
undefined	true	idleGraceMs via detachedSince — hole closed
undefined / 0	false	firstAttachGraceMs via unattendedSince

Independent verification at exact head 1a679c33 (virtual clock, no processes):

• bun test packages/coding-agent/test/sdk-session-host-idle-reap.test.ts: 13 pass / 0 fail.
• Restoring src/commands/sdk.ts alone from origin/dev: 12 pass / 1 fail — the single failure is exactly the flipped regression a host observed attached and then losing all endpoint evidence is reaped at the idle bound (got still-running, i.e. the leak). The other twelve pass either way, confirming the pre-existing suite could not see the hole.
• bun --cwd=packages/coding-agent run check:types (tsc --noEmit): exit 0.
• CI at exact head 1a679c33 (Dev CI run re-triggered after the earlier run was cancelled by a per-ref concurrency cancel when the head moved): all 22 check runs green — 16 success, 6 skipped (paths unaffected), 0 failures, including gjc-state-gates, Affected path validation / plan, native-build, evidence producer, and Affected path validation / test:…sdk-session-host-idle-reap.test.ts.

VERDICT: APPROVE — the leak row is bounded with the tighter idle bound, the regression is deterministic and mutation-proven, and the safety properties hold (no instant reap on the poll that first sees ambiguity, no reap while work is in flight, shipped 30/60-minute constants unchanged).

Leaving merge to the maintainers per ownership policy; not merging.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]