Merge PR #6305: fix(coding-agent/launch): wake ready waits on sticky marker not live state (@roboomp)

2026-07-22 21:13:19 +02:00
parent 5477e74bb3 b2ac625377
commit cfe296220d
5 changed files with 223 additions and 5 deletions
@@ -13,6 +13,7 @@
 - Fixed repeated OpenRouter Gemini reasoning-to-payload stream closures consuming the full ten-attempt retry budget; thinking-bearing `server_error: stream closed with reason: error` failures now get one recovery attempt before surfacing.
 - Fixed Agent Hub freezing for tens of seconds when opening a large read-only Advisor transcript. On cold open the viewer laid out every synthetic `Session update` input as full Markdown before `ScrollView` clipped the viewport (a 6.5 MiB `__advisor.jsonl` blocked `render()` for ~27s in a repro). Synthetic (agent-attributed) inputs now collapse to a compact summary row (`<heading> · <size> · <n> lines · ctrl+o`) and build their Markdown body only when expanded, so blocks above the viewport never pay layout cost on first frame ([#6308](https://github.com/can1357/oh-my-pi/issues/6308)).
 - Fixed `/agents` showing prewalk as off for the bundled `task` agent when `task.prewalk` enables its runtime default ([#6306](https://github.com/can1357/oh-my-pi/issues/6306)).
+- Fixed `hub start`/`for:"ready"` waits blocking for the full readiness timeout when the launched process became ready and exited within one poll interval (or exited before ever becoming ready). The wait now wakes on the sticky `readyAt` marker or any terminal state instead of sampling the transient live state, and `start` reports `Process exited before readiness was observed.` for a pre-ready exit ([#6303](https://github.com/can1357/oh-my-pi/issues/6303)).

 ## [17.0.7] - 2026-07-21

@@ -491,8 +491,17 @@ class DaemonBroker {
 		await this.#launch(record);
 		let readyTimedOut = false;
 		if (spec.ready && !terminalState(record.snapshot.state)) {
-			const ready = await this.#waitUntil(record, () => record.snapshot.state === "ready", spec.ready.timeoutMs);
-			readyTimedOut = !ready && !terminalState(record.snapshot.state);
+			// Wake on the sticky readyAt marker or any terminal state, not the live
+			// state: a fast process flips starting→ready→exited within one poll
+			// interval, so sampling `state === "ready"` never observes readiness even
+			// though #markReady durably recorded readyAt. A pre-ready exit must also
+			// wake the wait rather than block for the full timeout.
+			const ready = await this.#waitUntil(
+				record,
+				() => record.snapshot.readyAt !== undefined || terminalState(record.snapshot.state),
+				spec.ready.timeoutMs,
+			);
+			readyTimedOut = !ready;
 		}
 		await record.persistQueue;
 		return { op: "start", daemon: record.snapshot, readyTimedOut };
@@ -748,6 +757,11 @@ class DaemonBroker {
 			const uptime = Date.now() - record.snapshot.startedAt;
 			record.consecutiveFailures = uptime >= 30_000 ? 0 : record.consecutiveFailures + 1;
 			record.snapshot.restartCount++;
+			// Readiness belongs to the exited generation; clear it before the backoff
+			// so start / for:"ready" waits don't treat a dead service as ready during
+			// the restart window (readyAt is re-set by #launch once the child is up).
+			record.snapshot.readyAt = undefined;
+			record.snapshot.readyMatch = undefined;
 			record.snapshot.state = "restarting";
 			const delay = Math.min(1_000 * 2 ** Math.min(record.consecutiveFailures, 5), RESTART_MAX_DELAY_MS);
 			record.log?.append(
@@ -812,6 +826,12 @@ class DaemonBroker {
 				throw new Error(`Invalid wait regex: ${error instanceof Error ? error.message : String(error)}`);
 			}
 		}
+		// Readiness was actually observed: the sticky readyAt survives a fast
+		// ready→exit, a live "ready" state, or a "running" daemon with no ready spec.
+		const readyObserved = (): boolean =>
+			record.snapshot.readyAt !== undefined ||
+			record.snapshot.state === "ready" ||
+			(record.snapshot.state === "running" && !record.spec.ready);
 		const condition = (): boolean => {
 			if (pattern) {
 				const match = pattern.exec(record.readinessBuffer);
@@ -820,10 +840,16 @@ class DaemonBroker {
 				return true;
 			}
 			if (operation.for === "exit") return terminalState(record.snapshot.state);
-			return record.snapshot.state === "ready" || (record.snapshot.state === "running" && !record.spec.ready);
+			// Wake on observed readiness or any terminal state so the wait never
+			// blocks for the full timeout; success is judged by readyObserved below.
+			return readyObserved() || terminalState(record.snapshot.state);
 		};
-		const reached = condition() || (await this.#waitUntil(record, condition, operation.timeoutMs));
-		return { op: "wait", daemon: record.snapshot, matched, timedOut: !reached };
+		const woke = condition() || (await this.#waitUntil(record, condition, operation.timeoutMs));
+		// A for:"ready" wait that woke on a terminal exit without ever observing
+		// readiness is still "not ready" — surface it as timed out so callers and the
+		// renderer don't chain work against a dead process.
+		const timedOut = operation.for === "ready" && !pattern ? !readyObserved() : !woke;
+		return { op: "wait", daemon: record.snapshot, matched, timedOut };
 	}

 	async #send(operation: Extract<DaemonOperation, { op: "send" }>): Promise<DaemonRpcResult> {
@@ -74,6 +74,9 @@ const KEY_INPUT: Record<string, string> = {
 	LEFT: "\u001b[D",
 };

+/** Terminal daemon lifecycle states — the process is no longer running. */
+const TERMINAL_STATES: Partial<Record<DaemonState, true>> = { exited: true, failed: true };
+
 /** Structured launch state retained for compact TUI rendering. */
 export interface LaunchToolDetails {
 	op: LaunchParams["op"];
@@ -229,6 +232,8 @@ function toolContent(result: DaemonRpcResult, params: LaunchParams): string {
 				lines.push(
 					`NOT ready — readiness timed out after ${params.ready?.timeout ?? 30}s${cause}. The process is still running (state: ${daemon.state}); follow its logs or stop it.`,
 				);
+			} else if (params.ready && daemon.readyAt === undefined && TERMINAL_STATES[daemon.state]) {
+				lines.push("Process exited before readiness was observed.");
 			}
 			return lines.join("\n");
 		}
@@ -437,6 +442,8 @@ export function launchRenderResult(
 								: "Readiness timed out; the process is still running.",
 						),
 					);
+				} else if (params.ready && daemon && daemon.readyAt === undefined && TERMINAL_STATES[daemon.state]) {
+					body.push(theme.fg("warning", "Process exited before readiness was observed."));
 				}
 				break;
 			}
@@ -167,6 +167,34 @@ describe("hub launch rendering", () => {
 		expect(rendered.some(line => line.includes("spawn bun ENOENT"))).toBe(true);
 	});

+	it("surfaces a pre-ready exit in the interactive start result", async () => {
+		const uiTheme = await theme();
+		const rendered = lines(
+			hubToolRenderer.renderResult(
+				{
+					content: [{ type: "text", text: "Process exited before readiness was observed." }],
+					details: {
+						op: "start",
+						timedOut: false,
+						daemon: daemon({
+							state: "exited",
+							pid: undefined,
+							exitedAt: Date.now(),
+							exitCode: 0,
+							readyAt: undefined,
+						}),
+					} satisfies LaunchToolDetails,
+				},
+				{ expanded: false, isPartial: false },
+				uiTheme,
+				{ op: "start", name: "web", application: "bun", ready: { log: "LISTENING" } },
+			),
+		);
+		expect(rendered[0]).toContain("Launch start");
+		expect(rendered[0]).toContain("exited");
+		expect(rendered.some(line => line.includes("Process exited before readiness was observed."))).toBe(true);
+	});
+
 	it("names the unmet readiness condition instead of a contradictory Ready + timed-out pair", async () => {
 		const uiTheme = await theme();
 		const rendered = lines(
@@ -478,4 +478,160 @@ esac
 			await shutdown(client);
 		}
 	}, 20_000);
+
+	// Regression: a process that flips starting→ready→exited within one 50ms poll
+	// interval used to hang `start` for the full readiness timeout, because
+	// #waitUntil sampled the live (already "exited") state instead of the sticky
+	// readyAt marker #markReady durably recorded.
+	it("returns promptly when the process becomes ready then exits within a poll", async () => {
+		const projectDir = await tempDir("omp-daemon-fast-project-");
+		const runtimeDir = await tempDir("omp-daemon-fast-runtime-");
+		const scriptPath = path.join(projectDir, "fast.ts");
+		await Bun.write(scriptPath, `process.stdout.write("done\\n");\n`);
+		const client = await createDaemonBrokerClient(projectDir, { runtimeDir, idleGraceMs: 5_000 });
+		try {
+			const spec: DaemonSpec = {
+				name: "fast",
+				application: process.execPath,
+				args: [scriptPath],
+				env: {},
+				cwd: projectDir,
+				pty: false,
+				ready: { log: ".+", timeoutMs: 60_000 },
+				restart: "no",
+				persist: false,
+				detached: false,
+			};
+			const t0 = Date.now();
+			const started = await client.request({ op: "start", spec });
+			const elapsed = Date.now() - t0;
+			expect(started.op).toBe("start");
+			if (started.op !== "start") throw new Error("unexpected start result");
+			// Woke on readyAt/terminal, not the full 60s timeout.
+			expect(elapsed).toBeLessThan(10_000);
+			expect(started.readyTimedOut).toBeFalse();
+			expect(started.daemon.readyAt).toBeDefined();
+
+			// A for:"ready" wait on the settled daemon reports success via the sticky
+			// readyAt marker even though the process has already exited.
+			const waited = await client.request({
+				op: "wait",
+				name: "fast",
+				for: "ready",
+				timeoutMs: 60_000,
+			});
+			expect(waited.op).toBe("wait");
+			if (waited.op !== "wait") throw new Error("unexpected wait result");
+			expect(waited.timedOut).toBeFalse();
+			expect(waited.daemon.readyAt).toBeDefined();
+		} finally {
+			await shutdown(client);
+		}
+	}, 20_000);
+
+	// Regression: a process that exits before ever becoming ready used to block the
+	// caller for the full timeout, and a for:"ready" wait on the settled daemon did
+	// the same. Terminal states now wake both waits immediately.
+	it('wakes start and for:"ready" waits when the process exits before readiness', async () => {
+		const projectDir = await tempDir("omp-daemon-preexit-project-");
+		const runtimeDir = await tempDir("omp-daemon-preexit-runtime-");
+		const scriptPath = path.join(projectDir, "preexit.ts");
+		// Exits without ever printing the ready pattern.
+		await Bun.write(scriptPath, `process.stdout.write("nope\\n"); process.exit(0);\n`);
+		const client = await createDaemonBrokerClient(projectDir, { runtimeDir, idleGraceMs: 5_000 });
+		try {
+			const spec: DaemonSpec = {
+				name: "preexit",
+				application: process.execPath,
+				args: [scriptPath],
+				env: {},
+				cwd: projectDir,
+				pty: false,
+				ready: { log: "LISTENING", timeoutMs: 60_000 },
+				restart: "no",
+				persist: false,
+				detached: false,
+			};
+			const t0 = Date.now();
+			const started = await client.request({ op: "start", spec });
+			const startElapsed = Date.now() - t0;
+			expect(started.op).toBe("start");
+			if (started.op !== "start") throw new Error("unexpected start result");
+			expect(startElapsed).toBeLessThan(10_000);
+			// Woke on the terminal exit rather than timing out; the readyAt marker is
+			// absent because the ready pattern never matched.
+			expect(started.readyTimedOut).toBeFalse();
+			expect(started.daemon.readyAt).toBeUndefined();
+			expect(["exited", "failed"]).toContain(started.daemon.state);
+
+			// A for:"ready" wait on the already-settled daemon must wake immediately,
+			// but a process that never became ready is surfaced as not ready
+			// (timedOut) so callers don't chain work against a dead process.
+			const t1 = Date.now();
+			const waited = await client.request({
+				op: "wait",
+				name: "preexit",
+				for: "ready",
+				timeoutMs: 60_000,
+			});
+			const waitElapsed = Date.now() - t1;
+			expect(waited.op).toBe("wait");
+			if (waited.op !== "wait") throw new Error("unexpected wait result");
+			expect(waitElapsed).toBeLessThan(10_000);
+			expect(waited.timedOut).toBeTrue();
+			expect(waited.daemon.readyAt).toBeUndefined();
+		} finally {
+			await shutdown(client);
+		}
+	}, 20_000);
+
+	// Regression (PR #6305 review): readyAt belongs to the exited generation, so a
+	// daemon in the restart backoff window must not report readiness. #settle now
+	// clears readyAt/readyMatch when entering "restarting"; without that, start and
+	// for:"ready" waits race a dead service during the backoff.
+	it("clears stale readiness while a daemon is restarting", async () => {
+		const projectDir = await tempDir("omp-daemon-restart-project-");
+		const runtimeDir = await tempDir("omp-daemon-restart-runtime-");
+		const scriptPath = path.join(projectDir, "flap.ts");
+		// Becomes ready (prints the pattern), then crashes shortly after.
+		await Bun.write(scriptPath, `process.stdout.write("READY\\n"); setTimeout(() => process.exit(1), 50);\n`);
+		const client = await createDaemonBrokerClient(projectDir, { runtimeDir, idleGraceMs: 5_000 });
+		try {
+			const spec: DaemonSpec = {
+				name: "flap",
+				application: process.execPath,
+				args: [scriptPath],
+				env: {},
+				cwd: projectDir,
+				pty: false,
+				ready: { log: "READY", timeoutMs: 60_000 },
+				restart: "on-failure",
+				persist: false,
+				detached: false,
+			};
+			const started = await client.request({ op: "start", spec });
+			expect(started.op).toBe("start");
+			if (started.op !== "start") throw new Error("unexpected start result");
+			expect(started.daemon.readyAt).toBeDefined();
+
+			// Catch the backoff window: once restarting, readiness must be cleared.
+			const restarting = await waitUntil(async () => {
+				const listed = await client.request({ op: "list" });
+				if (listed.op !== "list") return false;
+				const daemon = listed.daemons.find(d => d.name === "flap");
+				return daemon?.state === "restarting";
+			}, 15_000);
+			expect(restarting).toBeTrue();
+			const listed = await client.request({ op: "list" });
+			if (listed.op !== "list") throw new Error("unexpected list result");
+			const daemon = listed.daemons.find(d => d.name === "flap");
+			expect(daemon?.state).toBe("restarting");
+			expect(daemon?.readyAt).toBeUndefined();
+			expect(daemon?.readyMatch).toBeUndefined();
+
+			await client.request({ op: "stop", name: "flap", timeoutMs: 2_000 });
+		} finally {
+			await shutdown(client);
+		}
+	}, 30_000);
 });