fix(daemon): Preserve sessions when active-work close is refused - #9134

[Critical] R4-1: The refusal branch resets entry.closing and returns false but arms no tombstone — and on the direct path none exists: spawnOwnerWantedKill is only written in the requireZeroAttaches && attachCount > 0 bail above (bridge.ts:11167), and spawn-owner registrations carry no attach ref, so a direct kill at attachCount === 0 (the common spawn-owner disconnect / orphan-cleanup shape) that meets a definitive refusal returns false to callers that are all one-shot and best-effort — nothing retries. maybeCloseIdleSession's deferred branch requires spawnOwnerWantedKill; the reaper/auto-close paths are vetoed by entryIsAutoCloseCandidate for held-work and incomplete-reporting children; the channel idle timer needs an empty session set. The comment above — "the deferred tombstone completes it on the next settle event" — describes a mechanism that does not exist on this path. — Failure scenario: a cleanup kill arrives while the child holds its own close gate (cd or restore in flight — the routine state this tolerance was written for) → the child answers 'Session close is already in progress' → killSession returns false and nothing retries → on a version-skewed or held-work child the session entry, its child process, and the persisted archive copy linger until daemon restart. Pre-PR this window escalated to a whole-channel kill (worse), so the tolerant direction is right — the missing tombstone leaves it incomplete.

Witness (bridge-harness probe: incomplete-reporting child, direct killSession({requireZeroAttaches: true}) refused, then gate released + idle snapshot):

PR side: sessionCount stays 1 — nothing completes the kill
with entry.spawnOwnerWantedKill = true added to this branch:
         sessionCount 0 on the next settle event; existing
         refusal / deferred-kill tests still pass

(If dropping direct kills on refusal is intended instead, scope the comment to the tombstone case.)

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Critical] R4-2: This comment's premise is wider than the predicate below it. isDefinitiveAcpRequestError (bridge.ts:300) accepts any record with an integer code + string message — but the ACP SDK flattens every child-thrown error into that same shape, so this branch tolerates not only the gate-held refusal it was written for but also child drain timeouts and close-handler crashes. Verified over the real wire: a child drain timeout (a plain Error from waitForSessionDrain) is wrapped by the SDK as {code: -32603, message: 'Internal error', data: {details: 'Session close timed out after 8000ms'}}, and the predicate accepts it. A drain timeout is a failed close with an unknown outcome — not a refusal — and the budget coupling makes this the norm: sessionCloseDrainBudgetMs guarantees the child deadline fires before the daemon's outer wait, so every child-side close failure arrives as a "definitive" record; the unknown-outcome escalation the helper docstring describes is unreachable for exactly the failures the budget exists to surface. Compounds R4-1: a misclassified timeout on the direct path arms no tombstone either. — Failure scenario: spawn-owner disconnect (attachCount === 0) → reaper kill → the turn is cancelled but slow to settle, so the child's close consumes its full drain budget and times out → the timeout is tolerated as a refusal → nothing retries → on an incomplete-category reporter the session persists until daemon restart (pre-PR: channel kill).

Witness (probe over the real bridge + real AgentSideConnection wire):

child drain timeout (plain Error) → observed record
  {"code":-32603,"message":"Internal error",
   "data":{"details":"Session close timed out after 8000ms"}}  predicate=true
PR arm:  killSession=false channelKilled=false sessionCount=1
FLIP A (branch reverted = pre-PR): channelKilled=true sessionCount=0
FLIP B (predicate excludes -32603): timeout escalates (channelKilled=true)
         while the production refusal still tolerates
         (killSession=false, sessionCount=1)

Suggested fix: classify the actual refusal, not the record shape — e.g. have Session.beginClose's refusal carry a marker matched in data (RequestError.invalidParams({errorKind: 'close_in_progress'}, …)), or restrict this call site to the -32602 refusal and let -32603/internal errors take the unknown-outcome escalation. Combined with R4-1's tombstone, a tolerated failure still completes later instead of leaking.

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-3: This fake throws new RequestError(-32603, 'Session close is already in progress'), but a production refusal is RequestError.invalidParams → over the wire {code: -32602, message: 'Invalid params: Session close is already in progress'} (Session.ts:3017, observed by probe). The predicate is code-agnostic today, so both shapes pass — but this test's oracle is wired to bytes production never sends: a natural R4-2 fix that narrows the predicate to the shape this test pins ships green while production refusals fall through to killChannelWithLog — the exact sibling-session cascade this PR exists to prevent. — Failure scenario: the classifier is narrowed to match this test's bytes → production refusals escalate to a whole-channel kill while the suite stays green. Probe reproduced exactly this: predicate narrowed to the test's shape → production-refusal arm killSession=true channelKilled=true sessionCount=0 while this test still passes (1 passed, 672 skipped).

Witness:

production refusal observed on the wire:
  {"code":-32602,"message":"Invalid params: Session close is already in progress"}
narrowed to this test's shape: production refusal escalates to
  channel kill; this test stays green

(and add one -32603/internal-error case asserting the escalation side — the R4-2 boundary)

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-4: The retry phase here does not pin the entry.closing = false reset it claims to verify: resolves.toBe(true) + sessionCount 0 are identical on the intended clean-retry path and on killSession's closing-branch channel-kill path (which also kills the channel and returns true), and handle.killed is never asserted after the retry. — Failure scenario: delete entry.closing = false; from the refusal branch → the second kill enters the closing branch and channel-kills — resolves.toBe(true) and sessionCount 0 still hold, so the mutant's detection depends on microtask timing of the channel-exit reap rather than a deliberate assertion. A regression latching refused entries closing-true turns every later kill of that session into a whole-channel kill (siblings included) with this test green on timing-favorable runs.

Witness (mutation run, entry.closing = false deleted — test still passes in both arms):

baseline stderr: channel exited (..., 0 session(s) torn down)  — clean retry
mutant stderr:   channel exited (..., 1 session(s) torn down)  — retry was a channel kill

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-5: This keying fix (round-2 R2-1) has no discriminating test: every close-path test runs a bridge whose initTimeoutMs is the 10_000 default — equal to the only opts.timeoutMs ever applied (ACTIVE_WORK_CLOSE_TIMEOUT_MS on the condemned-channel path) — so the two arms of ?? are observationally identical across the suite; no test passes agentCloseTimeoutMs and none varies initializeTimeoutMs. — Failure scenario: a future edit reverting the keying to sessionCloseDrainBudgetMs(initTimeoutMs) ships green and reintroduces the R2-1 escalation — with --initialize-timeout-ms 30000 and a quarantined/restore-overdue channel, auto-close tells the child to drain for 24_000ms against a 10_000ms outer wait → the daemon deadline fires first → the unknown-outcome recovery kills the whole channel mid-drain.

Witness (mutation run: keying reverted to sessionCloseDrainBudgetMs(initTimeoutMs); full bridge.test.ts suite): Tests 673 passed (673) — the mutant survives.

Suggested fix: add a bridge test reaching closeIfChildUnheld's condemned path (quarantined channel or restoreSettlementOverdue) with makeBridge({ initializeTimeoutMs: 30_000 }), asserting the sent drainTimeoutMs is sessionCloseDrainBudgetMs(ACTIVE_WORK_CLOSE_TIMEOUT_MS) (8_000), not 24_000.

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-6: Only the true side of isDefinitiveAcpRequestError is tested at the killSession site: no test drives a non-definitive close failure — a plain Error from the child's sessionClose handler, or a wedged child hitting withTimeout's BridgeTimeoutError (no integer code) — through killSession, so the escalation the classifier guards (killChannelWithLog) is unpinned at this call site. — Failure scenario: mutate the predicate to always-true (tolerate every close failure) → killSession against a wedged child returns false and leaves the kill pending instead of killing the channel; for a child that can never settle the session lingers indefinitely and the channel is never reaped — defeating kill's force semantics. Verified: the mutant ships green (Tests 673 passed (673)).

Witness (mutation run: if (isDefinitiveAcpRequestError(error)) → always-true; full bridge.test.ts suite): Tests 673 passed (673) — the mutant survives.

Suggested fix: add a bridge test where the channel answers sessionClose with throw new Error('close failed') (or never resolves under a short initializeTimeoutMs), asserting killSession resolves true and handle.killed is true — pinning the escalation side of the classifier.

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-7: The identical remaining-budget expression appears twice in closeStoredSession — as the phase-2 waitForSessionDrain timeout (~4630) and as this runExclusiveHistoryMutation wait timeout — with no shared helper. The new tests pin the phase-2 site, but none exercises this site under timeout pressure (the timeout test rejects before reaching it; the success test resolves instantly), so a mutation at this second site ships green and the two copies can drift exactly where the comment above says the coupling must hold ("or the whole round trip can outlast the daemon's outer wait"). — Failure scenario: a future change to the clamp or fallback (a different minimum than 1, or a fresh budget instead of the residue) is applied to one site only → the mutation wait silently diverges from the phase-2 budget → the mutation wait outlasts the daemon's outer wait on the conditional path — the regression this PR's comments warn about.

Suggested fix:

const remainingDrainBudgetMs = () =>
  conditionalDrainDeadline === undefined
    ? drainTimeoutMs
    : Math.max(1, conditionalDrainDeadline - Date.now());

declared next to conditionalDrainDeadline and called at both sites — it must stay a function re-evaluated per call, since wall-clock time elapses between the phases.

— qwen3.8-max via Qwen Code /review (v0.21.11)

[Suggestion] R4-8: The docstring invariant — "strictly under the daemon's outer wait so the child deadline always fires first" — does not hold for outerWaitMs === 1: the clamp yields a budget equal to the outer wait (the new unit test even pins sessionCloseDrainBudgetMs(1) === 1 while its strictly-under loop starts at 2). initializeTimeoutMs is validated only as > 0 (bridge.ts:2220), so a 1ms outer wait is constructible and the two deadlines race — the failure mode the docstring says this function exists to prevent. Practical impact is negligible (a 1ms outer wait makes every close round trip an unknown outcome regardless of the budget), so scoping the doc is an adequate fix. — Failure scenario: initializeTimeoutMs: 1 → drainTimeoutMs: 1 against a 1ms outer wait → the outer fires first → close outcome unknown → the close path recovers by killing the whole channel.

Suggested fix: state the ≥2ms precondition in the doc comment (keeping the pinned test honest) — or, for outerWaitMs >= 2 where both invariants are satisfiable, return Math.max(1, Math.min(outerWaitMs - 1, Math.floor(outerWaitMs * 0.8))).

— qwen3.8-max via Qwen Code /review (v0.21.11)