Skip to content

Commit a900cdb

Browse files
committed
fix(tui): keep the held queue on stay-alive worker stop
Stay-alive second Ctrl+C cancelled workers through the quit teardown, which wiped held follow-ups. Quit still wipes; stay-alive does not.
1 parent 39f7851 commit a900cdb

5 files changed

Lines changed: 190 additions & 59 deletions

File tree

‎src/tui/prompt-slash-exit.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,26 @@ describe("Ctrl+C exit", () => {
581581
});
582582
});
583583

584+
test("rejecting onStopWorkers stays STOPPED without quitting", async () => {
585+
await withShell(async ({ shell }) => {
586+
count = 1;
587+
stops = 0;
588+
exits = 0;
589+
setShellStopAffordance(shell, {
590+
liveWorkerCount: () => count,
591+
onStopWorkers: () => Promise.reject(new Error("cancelAll failed")),
592+
});
593+
setShellExitHandler(shell, () => onExit());
594+
handleCtrlC(shell, 0);
595+
handleCtrlC(shell, 1);
596+
await Promise.resolve();
597+
await Promise.resolve();
598+
expect(exits).toBe(0);
599+
handleCtrlC(shell, 2);
600+
expect(exits).toBe(1);
601+
});
602+
});
603+
584604
test("single press pauses only: neither stops nor exits", async () => {
585605
await withShell(async ({ shell }) => {
586606
wireWorkers(shell, 2);

‎src/tui/runner/wiring.stop-affordance.test.ts‎

Lines changed: 81 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,20 @@ import {
1313
} from "../shell/internals.js";
1414
import { handleCtrlC } from "../shell/keys.js";
1515
import { withTestRenderer } from "../harness.js";
16-
import { buildShellStopAffordance } from "./wiring.js";
16+
import {
17+
badgeCount,
18+
createSessionQueue,
19+
enqueue,
20+
isPaused,
21+
} from "../delivery-queue.js";
22+
import { buildShellStopAffordance, cancelWorkersForStop } from "./wiring.js";
1723

1824
// CL-10149 Phase 4 -- the integration half of Phase 3 deferred here.
1925
//
2026
// The runner wires the REAL stop affordance into the shell inside
2127
// wirePostStartup via buildShellStopAffordance: the affordance's
2228
// liveWorkerCount reports the underlying session store's live fleet and its
23-
// onStopWorkers drives the production `cancelWorkersForStop` closure. This test
29+
// onStopWorkers drives the production `cancelLiveWorkers` closure. This test
2430
// proves that end-to-end against a harnessed store: a 2nd Ctrl+C press stops
2531
// the live workers and the app STAYS RUNNING (no exit), and liveWorkerCount
2632
// reports the harnessed store's live fleet before and after the stop.
@@ -58,14 +64,11 @@ describe("shell stop affordance wiring (CL-10149 Phase 4)", () => {
5864
});
5965

6066
const store = harnessWithLiveWorkers(2);
61-
const bridge = { clearQueuedDelivery: () => undefined };
62-
// The production dep shapes feed cancelWorkersForStop; fleetRecords is
63-
// cleared after a stop so the idle-with-fleet gauge drops.
6467
const fleetRecords = { clear: () => undefined };
65-
const affordance = buildShellStopAffordance(
66-
{ bridge },
67-
{ subAgentSessions: store, toolset: { fleetRecords } },
68-
);
68+
const affordance = buildShellStopAffordance({
69+
subAgentSessions: store,
70+
toolset: { fleetRecords },
71+
});
6972
// The runner's wirePostStartup registers the affordance on the shell;
7073
// mirror that so the key machine's readStopAffordance sees it.
7174
setShellStopAffordance(shell, affordance);
@@ -82,7 +85,7 @@ describe("shell stop affordance wiring (CL-10149 Phase 4)", () => {
8285
expect(liveStripCount(store)).toBe(2);
8386

8487
// 2nd press inside the window stops the live workers via the real
85-
// cancelWorkersForStop closure -- app STAYS RUNNING (no exit).
88+
// cancelLiveWorkers closure -- app STAYS RUNNING (no exit).
8689
handleCtrlC(shell, 1);
8790
expect(liveStripCount(store)).toBe(0);
8891
expect(exits).toBe(0);
@@ -113,10 +116,10 @@ describe("shell stop affordance wiring (CL-10149 Phase 4)", () => {
113116
});
114117

115118
const store = createSubAgentSessionStore();
116-
const affordance = buildShellStopAffordance(
117-
{ bridge: { clearQueuedDelivery: () => undefined } },
118-
{ subAgentSessions: store, toolset: { fleetRecords: undefined } },
119-
);
119+
const affordance = buildShellStopAffordance({
120+
subAgentSessions: store,
121+
toolset: { fleetRecords: undefined },
122+
});
120123
setShellStopAffordance(shell, affordance);
121124
try {
122125
expect(affordance.liveWorkerCount()).toBe(0);
@@ -130,4 +133,68 @@ describe("shell stop affordance wiring (CL-10149 Phase 4)", () => {
130133
}
131134
});
132135
});
136+
137+
test("stay-alive stop keeps a held follow-up paused; quit still wipes", async () => {
138+
await withTestRenderer(async (h) => {
139+
const shell = createAppShell(h.renderer, {
140+
terminal: { columns: 80, rows: 24 },
141+
wireKeys: false,
142+
run: "busy",
143+
});
144+
let exits = 0;
145+
setShellExitHandler(shell, () => {
146+
exits += 1;
147+
});
148+
149+
const store = harnessWithLiveWorkers(2);
150+
let cleared = 0;
151+
const bridge = {
152+
clearQueuedDelivery: () => {
153+
cleared += 1;
154+
shell.session = createSessionQueue("idle");
155+
},
156+
};
157+
const fleetRecords = { clear: () => undefined };
158+
const inner = buildShellStopAffordance({
159+
subAgentSessions: store,
160+
toolset: { fleetRecords },
161+
});
162+
let stopDone: Promise<void> = Promise.resolve();
163+
const affordance = {
164+
liveWorkerCount: inner.liveWorkerCount,
165+
onStopWorkers: () => {
166+
stopDone = Promise.resolve(inner.onStopWorkers());
167+
return stopDone;
168+
},
169+
};
170+
setShellStopAffordance(shell, affordance);
171+
try {
172+
shell.session = enqueue(shell.session, "follow up later");
173+
expect(badgeCount(shell.session)).toBe(1);
174+
175+
handleCtrlC(shell, 0);
176+
expect(isPaused(shell.session)).toBe(true);
177+
expect(badgeCount(shell.session)).toBe(1);
178+
expect(liveStripCount(store)).toBe(2);
179+
180+
handleCtrlC(shell, 1);
181+
await stopDone;
182+
expect(exits).toBe(0);
183+
expect(liveStripCount(store)).toBe(0);
184+
expect(cleared).toBe(0);
185+
expect(badgeCount(shell.session)).toBe(1);
186+
expect(isPaused(shell.session)).toBe(true);
187+
188+
await cancelWorkersForStop({
189+
subAgentSessions: store,
190+
fleetRecords,
191+
bridge,
192+
});
193+
expect(cleared).toBe(1);
194+
expect(badgeCount(shell.session)).toBe(0);
195+
} finally {
196+
shell.dispose();
197+
}
198+
});
199+
});
133200
});

‎src/tui/runner/wiring.stop-idempotent.test.ts‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,24 @@ import {
55
type SubAgentSessionStore,
66
} from "../../subagent/session-store.js";
77
import { isLiveStrip } from "../../subagent/lifecycle.js";
8-
import { cancelWorkersForStop } from "./wiring.js";
8+
import { createAppShell } from "../shell/index.js";
9+
import {
10+
setShellExitHandler,
11+
setShellStopAffordance,
12+
} from "../shell/internals.js";
13+
import { handleCtrlC } from "../shell/keys.js";
14+
import { withTestRenderer } from "../harness.js";
15+
import { buildShellStopAffordance, cancelWorkersForStop } from "./wiring.js";
916

1017
// CL-10149 Phase 3 — idempotent stop-then-quit (three-press Ctrl+C).
1118
//
12-
// The 2nd-press stop and the 3rd-press quit both drive `cancelWorkersForStop`
13-
// (the affordance's onStopWorkers in Phase 4 wires the stop; the quit path
14-
// runs it again through shutdownRuntime). Because `cancelAll` snapshots live
15-
// sessions and leaves tombstones, a stop followed by a quit must cancel twice
16-
// total, with the second call finding nothing live — no double cancel. The
17-
// single-press pause (the interrupt path) must never call cancelAll at all.
18-
// These assertions pin that idempotency at the `cancelWorkersForStop` unit
19-
// boundary, before the affordance is wired into production (Phase 4).
19+
// The 2nd-press stay-alive stop drives `cancelLiveWorkers`; the 3rd-press
20+
// quit runs `cancelWorkersForStop` through shutdownRuntime. Both cancel
21+
// via `cancelAll`, which snapshots live sessions and leaves tombstones, so
22+
// a stop followed by a quit must cancel twice total, with the second call
23+
// finding nothing live — no double cancel. The single-press pause (the
24+
// interrupt path) must never call cancelAll at all. These assertions pin
25+
// that idempotency at the cancel unit boundary.
2026

2127
interface StopHarness {
2228
store: SubAgentSessionStore;
@@ -80,13 +86,29 @@ describe("cancelWorkersForStop idempotency (CL-10149 Phase 3)", () => {
8086
});
8187

8288
test("single-press pause never calls cancelAll", async () => {
83-
// The pause press must not drive the stop/quit path, so a worker that is
84-
// only paused stays live and cancelAll is never invoked (guards the
85-
// interrupt path, R4/R5).
86-
const h = harnessWithLiveWorkers(1);
87-
expect(liveStripCount(h.store)).toBe(1);
88-
expect(h.cancelCalls).toHaveLength(0);
89-
expect(liveStripCount(h.store)).toBe(1);
89+
await withTestRenderer(async (h) => {
90+
const shell = createAppShell(h.renderer, {
91+
terminal: { columns: 80, rows: 24 },
92+
wireKeys: false,
93+
run: "busy",
94+
});
95+
setShellExitHandler(shell, () => undefined);
96+
const harn = harnessWithLiveWorkers(1);
97+
setShellStopAffordance(
98+
shell,
99+
buildShellStopAffordance({
100+
subAgentSessions: harn.store,
101+
toolset: { fleetRecords: { clear: () => undefined } },
102+
}),
103+
);
104+
try {
105+
handleCtrlC(shell, 0);
106+
expect(harn.cancelCalls).toHaveLength(0);
107+
expect(liveStripCount(harn.store)).toBe(1);
108+
} finally {
109+
shell.dispose();
110+
}
111+
});
90112
});
91113

92114
test("a lone stop on many live workers still cancels exactly once", async () => {

‎src/tui/runner/wiring.ts‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -157,50 +157,58 @@ export interface StopTeardownDeps {
157157
bridge: { clearQueuedDelivery: () => void };
158158
}
159159

160+
/**
161+
* Cancel live workers and drop fleet records, leaving tombstones so a later
162+
* quit-path cancelAll is a no-op. Does not wipe the held delivery queue —
163+
* stay-alive 2nd-press stop uses this so follow-ups stay pending.
164+
*/
165+
export async function cancelLiveWorkers(
166+
deps: Omit<StopTeardownDeps, "bridge">,
167+
): Promise<void> {
168+
await deps.subAgentSessions.cancelAll("Session closed");
169+
deps.subAgentSessions.teardown("Session closed");
170+
deps.fleetRecords?.clear();
171+
}
172+
160173
/**
161174
* Stop teardown (CL-8016): cancel the workers, wipe the sessions (leaving
162175
* tombstones so a late send_input names the teardown), drop the mailbox
163176
* lanes that pin them, and clear the bridge queue so no wake-turn bound
164177
* outlives the sessions it was owed to. Exported so the stall-bound
165178
* regression test drives this exact production path instead of re-wiring
166-
* the three clears by hand.
179+
* the three clears by hand. Quit / shutdown keep this wipe; stay-alive
180+
* stop uses `cancelLiveWorkers` instead.
167181
*/
168182
export async function cancelWorkersForStop(
169183
deps: StopTeardownDeps,
170184
): Promise<void> {
171-
await deps.subAgentSessions.cancelAll("Session closed");
172-
deps.subAgentSessions.teardown("Session closed");
173-
deps.fleetRecords?.clear();
185+
await cancelLiveWorkers(deps);
174186
deps.bridge.clearQueuedDelivery();
175187
}
176188

177189
/**
178190
* Stop-teardown sources for the shell stop affordance (CL-10149 Phase 4).
179191
* Live-worker count is the authoritative `liveFleetCount` over the session
180-
* store (not the bridge-local mirror); the stop callback is the exact
181-
* production `cancelWorkersForStop` closure, so a 2nd-press stop and the
182-
* 3rd-press quit share the same teardown deps as `shutdownRuntime`
183-
* (wiring.ts:295-301) and stay idempotent. Extracted so the Phase 4 wiring
184-
* test can drive the real closure against a harnessed store without a full
185-
* runner harness.
192+
* store (not the bridge-local mirror); the stop callback cancels workers
193+
* and fleet records without wiping the held queue, so a 2nd-press stay-alive
194+
* stop keeps pending follow-ups. Quit still goes through
195+
* `cancelWorkersForStop` (wiring.ts shutdown path) and wipes. Extracted so
196+
* the Phase 4 wiring test can drive the real closure against a harnessed
197+
* store without a full runner harness.
186198
*/
187-
export function buildShellStopAffordance(
188-
host: { bridge: { clearQueuedDelivery: () => void } },
189-
deps: {
190-
subAgentSessions: Pick<
191-
RunnerServices["subAgentSessions"],
192-
"list" | "cancelAll" | "teardown"
193-
>;
194-
toolset: { fleetRecords?: { clear: () => void } | undefined };
195-
},
196-
): ShellStopAffordance {
199+
export function buildShellStopAffordance(deps: {
200+
subAgentSessions: Pick<
201+
RunnerServices["subAgentSessions"],
202+
"list" | "cancelAll" | "teardown"
203+
>;
204+
toolset: { fleetRecords?: { clear: () => void } | undefined };
205+
}): ShellStopAffordance {
197206
return {
198207
liveWorkerCount: () => liveFleetCount(deps.subAgentSessions.list()),
199208
onStopWorkers: () =>
200-
cancelWorkersForStop({
209+
cancelLiveWorkers({
201210
subAgentSessions: deps.subAgentSessions,
202211
fleetRecords: deps.toolset.fleetRecords,
203-
bridge: host.bridge,
204212
}),
205213
};
206214
}
@@ -340,14 +348,14 @@ export function wirePostStartup(
340348
setActiveDisposeHost(() => services.crashGuard.invokeDisposeHost());
341349

342350
// CL-10149 three-press: register the shell stop affordance so the 2nd Ctrl+C
343-
// (with live sub-agents) drives the same faithful stop-teardown closure the
344-
// quit path uses — the runner owns the source of truth, the shell stays
345-
// service-free (SOLUTION_SCOPE D1/D2). liveWorkerCount reports the live fleet
346-
// from the session store; onStopWorkers stops the workers while the app stays
347-
// running, and is idempotent so a later quit finds nothing live.
351+
// (with live sub-agents) stops workers while the app stays running. The
352+
// runner owns the source of truth; the shell stays service-free. Stay-alive
353+
// stop cancels workers without wiping the held queue; quit still wipes
354+
// through cancelWorkersForStop. Tombstones keep a later quit-path cancelAll
355+
// a no-op.
348356
setShellStopAffordance(
349357
hostOf(state).shell,
350-
buildShellStopAffordance({ bridge: hostOf(state).bridge }, services),
358+
buildShellStopAffordance(services),
351359
);
352360

353361
// Harness inference.error events omit providerId; stamp the live catalog id

‎src/tui/shell/keys.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
/**
22
* Key routing: paste guard, kill-ring chords, the onKey dispatcher body, Ctrl+C arming.
33
*/
4+
import { getLogger } from "@intx/log";
45
import {
56
ScrollBoxRenderable,
67
type BaseRenderable,
78
type KeyEvent,
89
type MouseEvent,
910
} from "@opentui/core";
11+
import { LOG_NAMESPACE_ROOT } from "../../branding.js";
1012
import { badgeCount, pause } from "../delivery-queue.js";
1113

1214
import {
@@ -91,6 +93,14 @@ import {
9193
} from "../sent-message-history.js";
9294
import { EXPAND_KEY } from "../stream.js";
9395

96+
const tuiLogger = getLogger([LOG_NAMESPACE_ROOT, "tui"]);
97+
98+
function logStopWorkersFailure(error: unknown): void {
99+
tuiLogger.warn("stop workers failed: {error}", {
100+
error: error instanceof Error ? error.message : String(error),
101+
});
102+
}
103+
94104
// Human keystrokes land tens of milliseconds apart at the fastest; a paste
95105
// replayed onto stdin without bracketed-paste framing lands effectively all
96106
// at once. 15ms is an empirical guess at a gap comfortably under normal
@@ -206,7 +216,11 @@ export function handleCtrlC(
206216
? { schedule: options.schedule }
207217
: {}),
208218
});
209-
void onStop();
219+
try {
220+
void Promise.resolve(onStop()).catch(logStopWorkersFailure);
221+
} catch (error: unknown) {
222+
logStopWorkersFailure(error);
223+
}
210224
return;
211225
}
212226
// Real quit (2nd press with no live workers, or 3rd press after stop).

0 commit comments

Comments
 (0)