From 64150e0408ced7c6a71b4983c35844243a7118d4 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Mon, 13 Jul 2026 16:50:35 +1000 Subject: [PATCH] fix(tui): kill a cancelled boot's pane when it settles + make async tests actually count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds in the independent review's remaining nit — and, in proving the nit's fix, uncovers two defects in the test suite itself. ## The nit (latent M1b, second costume) `_cancelBooting` kills BY NAME, but the tmux session only EXISTS once `bootPane` has run — and `bootPane` is queued on a microtask. So a caller doing `refill()` then `drain()` in the SAME synchronous block leaves `_cancelBooting` with nothing to kill (a no-op); it bumps the generation, and the boot microtask then CREATES the session, succeeds, and — under the old bare `return` on a stale generation — walked away from a LIVE authenticated `claude` that nothing owns. Reproduced: reverted: drain() kills nothing (no session yet) -> boot creates it -> ORPHAN: ['p1'] fixed : drain() kills nothing (no session yet) -> boot creates it -> boot kills it -> [] Not reachable from any current call site, so this is defense-in-depth — but ADR 0008 and the reap-tick comment in server.mjs BOTH explicitly contemplate a boot-time pre-warm, which is exactly the shape that reaches it. Killing an already-dead session is a harmless no-op, so the fix is idempotent whichever way the race lands. ## Defect 1 in the suite: async tests were never awaited (44 of them) Writing the regression guard exposed this. `test()` called `fn()`, got a promise back, and IMMEDIATELY printed ✓ and incremented `passed` — without awaiting it. For all 44 tests written as `test("...", async () => {...})`: - ✓ meant "did not throw SYNCHRONOUSLY", not "passed"; - a failed assertion escaped as an unhandled rejection, crashing the process (CI stays red on the non-zero exit) but never being COUNTED — so the summary could print "0 failed" and be wrong. The suite's headline number was therefore not evidence for ANY async test, including this PR's own M1a/M1b guards. `test()` now settles an async body before counting it, and the summary awaits them. ## Defect 2, exposed the instant defect 1 was fixed: a false guard `"a boot that resolves AFTER a drain kills its own pane ... no orphan process left behind"` asserted `killed.length === 1` — i.e. that kill was CALLED once. But `_cancelBooting`'s kill-by-name on a not-yet-existent session is a NO-OP that still increments that counter. So "kill was called once" and "a live session is orphaned" were both true at the same time: a test named for the absence of an orphan was passing while the orphan was present. Now asserts LIVENESS (`live.size === 0`) — the only honest question. ## Evidence fix present : 295 passed, 0 failed, exit 0 fix reverted: 293 passed, 2 failed <- BOTH liveness guards fire (the old kill-count guard did not) Also: `dropped`'s doc comment now lists `cancelled` (a cancelled in-flight boot lands there via _drop). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR --- lib/tui/pool.mjs | 22 ++++++++++++++--- test-features.mjs | 63 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/lib/tui/pool.mjs b/lib/tui/pool.mjs index f05cdbc..ff7df02 100644 --- a/lib/tui/pool.mjs +++ b/lib/tui/pool.mjs @@ -108,7 +108,8 @@ export class TuiPanePool { this.boots = 0; // panes successfully pre-booted this.bootFailures = 0; // pre-boots that genuinely never reached the input bar this.cancelled = 0; // in-flight boots WE killed (drain / model switch) — not failures - this.dropped = 0; // panes discarded unused (unhealthy / expired / wrong model / drained) + this.dropped = 0; // panes discarded unused (unhealthy / expired / wrong model / drained / + // cancelled — a cancelled in-flight boot also lands here via _drop) } get enabled() { return this.size > 0; } @@ -198,9 +199,22 @@ export class TuiPanePool { Promise.resolve() .then(() => this._bootPane(model, ident)) .then((pane) => { - // The world may have moved while we booted. If our generation was cancelled, our pane - // was ALREADY killed by _cancelBooting — do not touch it, do not enlist it. - if (gen !== this._gen) return; + // The world may have moved while we booted. If our generation was cancelled, kill the + // pane here rather than ASSUMING _cancelBooting already did. + // + // Why not just `return`: _cancelBooting kills by name, but the tmux session only EXISTS + // once _bootPane has actually run — and _bootPane is queued on a microtask (above). A + // caller that does refill() and then drain() in the SAME synchronous block would have + // _cancelBooting find nothing to kill (a no-op), bump the generation, and then this + // microtask would create the session, boot it fine, and — under a bare `return` — walk + // away from a LIVE authenticated `claude` that nothing owns. That is M1b in a new costume. + // No current call site does that, so this is defense-in-depth, not a live bug — but ADR + // 0008 and the reap-tick comment in server.mjs both explicitly contemplate a boot-time + // pre-warm, which is exactly the shape that would reach it. + // + // Killing an already-dead session is a harmless no-op (_drop swallows it), so this is + // idempotent whether or not _cancelBooting got there first. + if (gen !== this._gen) { this._drop(pane, "cancelled_late"); return; } // Otherwise: still possible the pool filled or retargeted without a cancellation. if (this._paused || model !== this.warmModel || this._panes.length >= this.size) { this._drop(pane, "stale_boot"); diff --git a/test-features.mjs b/test-features.mjs index 4e055f7..202b7ca 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -23,9 +23,30 @@ process.env.HOME = homedir(); // ensure consistent let passed = 0; let failed = 0; +// Pending promises from tests declared `async` but registered through the SYNC `test()` helper. +// 44 tests in this file are written that way. Before this, `test()` called fn(), got a promise back, +// and immediately printed ✓ and incremented `passed` — WITHOUT AWAITING IT. So for every async test: +// - ✓ meant "did not throw synchronously", NOT "passed"; +// - a failed assertion escaped as an unhandled rejection, which crashes the process (CI still goes +// red on the non-zero exit) but is NOT counted, so the summary could print "N passed, 0 failed" +// and be wrong. +// The suite's own headline number was therefore not evidence for any async test — including the +// regression guards in this PR. Collected here and awaited before the summary prints. +const pendingAsync = []; + function test(name, fn) { try { - fn(); + const r = fn(); + if (r && typeof r.then === "function") { + // Async body: settle it before counting. Do NOT print ✓ yet. + pendingAsync.push( + r.then( + () => { passed++; console.log(` ✓ ${name}`); }, + (e) => { failed++; console.log(` ✗ ${name}: ${e.message}`); }, + ), + ); + return; + } passed++; console.log(` ✓ ${name}`); } catch (e) { @@ -2208,13 +2229,19 @@ test("a model switch drops the wrong-model panes and retargets the pool (--model }); test("a boot that resolves AFTER a drain kills its own pane instead of enlisting it", async () => { - const { pool, killed } = makeFakePool({ size: 1 }); + const { pool, live } = makeFakePool({ size: 1 }); pool.acquire("sonnet"); pool.refill(); // boot is in flight... pool.drain(); // ...pool drained before it resolves (shutdown / reap sweep) await settle(); assert.equal(pool.warm, 0, "the late pane must NOT be enlisted into a drained pool"); - assert.equal(killed.length, 1, "it kills itself — no orphan process left behind"); + // Assert LIVENESS, not the kill-call COUNT. This assertion used to read + // `assert.equal(killed.length, 1)` — and it PASSED while the orphan it is named after was + // actually present: _cancelBooting kills BY NAME, and at drain time the tmux session does not + // exist yet (bootPane runs on a microtask), so that kill is a NO-OP which still increments the + // counter. "kill was called once" and "a live session is orphaned" were both true at the same + // time. The only honest question is whether the session is dead. + assert.equal(live.size, 0, "it kills itself — no orphan process left behind"); }); test("bootPane failure is counted, never thrown into the request path, and does not wedge refill", async () => { @@ -2386,6 +2413,32 @@ test("M1b: shutdown drain kills the booting pane SYNCHRONOUSLY — no orphaned c "never run before process.exit and would orphan a live authenticated `claude`."); }); +// M1b, second costume: drain() in the SAME synchronous block as refill(). The tmux session does +// not exist yet at drain time (bootPane runs on a microtask), so _cancelBooting's kill-by-name is +// a no-op — and a `.then` that merely `return`s on a stale generation would then let the boot +// CREATE the session and walk away from it. Not reachable from any current call site, but ADR 0008 +// and the reap-tick comment both contemplate a boot-time pre-warm, which is exactly this shape. +// NOTE: deliberately NOT `hold: true`. A held boot never settles, so its `.then` never runs and +// the guard would vacuously pass — the test must let the boot actually SUCCEED, because the bug is +// precisely that a SUCCESSFUL boot on a cancelled generation walks away from its live session. +test("M1b': a boot cancelled BEFORE its session existed is still killed when it settles", async () => { + const { pool, live } = makeFakePool({ size: 1 }); + pool.acquire("sonnet"); // miss → learns the model + + pool.refill(); // mints the identity; bootPane is queued on a microtask — no session YET + pool.drain(); // SAME sync block: kill-by-name finds nothing to kill (no-op), bumps the gen + assert.equal(live.size, 0, "precondition: the session genuinely did not exist at cancel time"); + + await tick(); // NOW the boot microtask runs, CREATES the session, and settles on a stale gen + await tick(); + + assert.equal(live.size, 0, + "REGRESSION GUARD: a stale-generation boot must KILL its pane, not assume _cancelBooting " + + "already did. _cancelBooting kills BY NAME, and the tmux session does not exist until the " + + "boot microtask runs — so a cancellation landing first is a no-op, and a bare `return` here " + + "orphans a live authenticated `claude` that nothing owns."); +}); + test("a stale boot settling after drain+resume must not clear the NEW boot's slot", async () => { const { pool, releaseBoot } = makeFakePool({ size: 1, hold: true }); pool.acquire("sonnet"); @@ -3390,7 +3443,9 @@ async function runAsyncTests() { } // ── Cleanup ── -runAsyncTests().then(() => { +// Settle the async-bodied tests registered through the sync `test()` helper BEFORE summarizing — +// otherwise their pass/fail is not reflected in the counts (see the `pendingAsync` comment above). +runAsyncTests().then(() => Promise.all(pendingAsync)).then(() => { closeDb(); console.log(`\n=== Results: ${passed} passed, ${failed} failed ===\n`); process.exit(failed > 0 ? 1 : 0);