mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-19 09:44:07 +00:00
fix(tui): kill a cancelled boot's pane when it settles + make async tests actually count
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR
This commit is contained in:
+18
-4
@@ -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");
|
||||
|
||||
+59
-4
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user