diff --git a/CHANGELOG.md b/CHANGELOG.md index db4373ac..380b9ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,15 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename the subtree-scoping rule for those is written and tested but not yet wired to a live call site. `task()` is unchanged and still the only spawn verb. +### Fixed + +- **Interrupting a turn no longer risks a startup crash.** If an interrupt hit + the agent mid-teardown, a failed close could leave its in-process workdir + lock stuck held, and the immediate rebuild threw "an agent is already open" + as an unhandled rejection. A failed close now short-circuits the rebuild + with a clear, catchable error instead of retrying a doomed second + acquisition. + ## [0.2.107] - 2026-08-24 ### Agent diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 5aedcb89..fccb5c85 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -6,6 +6,7 @@ import { defineTool, createDirectorRegistry, defineDirector, + AgentContextLockError, type Agent, } from "@intx/agent"; import { noopAuditStore, permissiveAuthorize } from "@intx/agent/testing"; @@ -283,6 +284,46 @@ export function resumeTranscriptLoadErrorBlock(err: unknown): { return { type: "error", message: `Could not load prior session transcript: ${message}` }; } +// The agent package releases its workdir lock at the very end of close(), +// after reactor.abort()/sendQueue.drain() and the shutdown-complete race have +// all run. If any of that throws (most likely right when an operator +// interrupts mid-inference, which is exactly when those paths are under +// stress), the lock is never released — and because the agent is already +// marked closed internally, retrying close() is a silent no-op that can +// never release it either. Every rebuild site that reuses the *same* workdir +// (interrupt, reloadIfIdle) must treat that as fatal for the current rebuild +// instead of calling buildAgent() again: a second createAgent() for the same +// workdir is then guaranteed to throw AgentContextLockError for a lock +// nothing will ever free, which is the "agent already open" crash. Session +// rotation (newSession) is the one rebuild site that does NOT route through +// this helper: it always points buildAgent() at a freshly minted workdir +// before rebuilding, so a leaked lock on the old workdir can never be +// re-acquired there — see the comment at its close() call for why. +export async function closeAgentForRebuild(agent: Agent, context: string): Promise { + try { + await agent.close(); + return true; + } catch (err) { + tuiLogger.debug(`agent.close during ${context} teardown failed: {error}`, { + error: err instanceof Error ? err.message : String(err), + }); + return false; + } +} + +// Every rebuild site funnels its failure (a lock left held by a failed +// close, or any other buildAgent failure) through here so it surfaces as a +// plain-language, caught error rather than an unhandled rejection. +export function agentRebuildFailure(err: unknown): Error { + return err instanceof AgentContextLockError + ? new Error( + "Could not start a new agent: the previous one did not shut down cleanly. Restart Corbits to continue.", + ) + : err instanceof Error + ? err + : new Error(String(err)); +} + export interface ResumeSeed { turnsUsed: number; mcpServers: ConnectedMcpServer[]; @@ -1646,21 +1687,25 @@ export async function runTUI(initialConfig: Config): Promise { if (!pendingReload || inFlight > 0) return; pendingReload = false; void enqueueOp(async () => { - const old = currentAgent; - await old.close().catch((err: unknown) => { - tuiLogger.debug("agent.close during reload teardown failed: {error}", { - error: err instanceof Error ? err.message : String(err), - }); - }); - await streamPromise.catch((err: unknown) => { - tuiLogger.debug("stream drain during reload teardown failed: {error}", { - error: err instanceof Error ? err.message : String(err), + try { + const old = currentAgent; + const closedCleanly = await closeAgentForRebuild(old, "reload"); + await streamPromise.catch((err: unknown) => { + tuiLogger.debug("stream drain during reload teardown failed: {error}", { + error: err instanceof Error ? err.message : String(err), + }); }); - }); - currentAgent = await buildAgent(); - streamPromise = consumeStream(currentAgent.stream(), streamSink); - // The rebuild made a fresh director; re-attach the active workflow. - workflowController.reattach(); + if (!closedCleanly) { + throw new AgentContextLockError(workdir); + } + currentAgent = await buildAgent(); + streamPromise = consumeStream(currentAgent.stream(), streamSink); + // The rebuild made a fresh director; re-attach the active workflow. + workflowController.reattach(); + } catch (err) { + recordRunError(err); + fatalBuildError = agentRebuildFailure(err); + } }); }; @@ -1812,16 +1857,15 @@ export async function runTUI(initialConfig: Config): Promise { // and salvages the buffer before that teardown, so it is never lost // or misattributed to the rebuilt agent's next cycle. await cycleRecorder.dispose("interrupted"); - await currentAgent.close().catch((err: unknown) => { - tuiLogger.debug("agent.close during interrupt teardown failed: {error}", { - error: err instanceof Error ? err.message : String(err), - }); - }); + const closedCleanly = await closeAgentForRebuild(currentAgent, "interrupt"); await streamPromise.catch((err: unknown) => { tuiLogger.debug("stream drain during interrupt teardown failed: {error}", { error: err instanceof Error ? err.message : String(err), }); }); + if (!closedCleanly) { + throw new AgentContextLockError(workdir); + } currentAgent = await buildAgent(); cycleRecorder.reset(); streamPromise = consumeStream(currentAgent.stream(), streamSink); @@ -1829,7 +1873,7 @@ export async function runTUI(initialConfig: Config): Promise { fatalBuildError = null; } catch (err) { recordRunError(err); - fatalBuildError = err instanceof Error ? err : new Error(String(err)); + fatalBuildError = agentRebuildFailure(err); } }); }; @@ -1861,6 +1905,15 @@ export async function runTUI(initialConfig: Config): Promise { // settles, and a dead cycle's partial must land in the session that // produced it, not the fresh one. await cycleRecorder.dispose("rotation"); + // Deliberately not routed through closeAgentForRebuild/ + // agentRebuildFailure (unlike interrupt and reloadIfIdle, CL-5753): + // rotation mints a fresh sessionId/workdir below before calling + // buildAgent(), so even a close() that leaks the old workdir's lock + // (see closeAgentForRebuild's doc comment) can never cause a second + // acquisition on that same workdir — buildAgent() always targets + // the new, unlocked directory. The old lock still leaks for the + // rest of the process, but nothing ever tries to re-acquire it, so + // there is no crash to guard against here. await currentAgent.close().catch((err: unknown) => { tuiLogger.debug("agent.close during session-rotation teardown failed: {error}", { error: err instanceof Error ? err.message : String(err), diff --git a/tests/unit/tui/runner.test.ts b/tests/unit/tui/runner.test.ts index d663d1e6..8114531e 100644 --- a/tests/unit/tui/runner.test.ts +++ b/tests/unit/tui/runner.test.ts @@ -1,11 +1,15 @@ import { test, expect } from "bun:test"; import { EventEmitter } from "node:events"; +import { AgentContextLockError, type Agent } from "@intx/agent"; import { + agentRebuildFailure, + closeAgentForRebuild, createTUIEventEmitter, getTUIRunSummaryStatus, loadLocalSettingsWriteBase, resumeTranscriptLoadErrorBlock, } from "../../../src/tui/runner.js"; +import { createSessionOperationQueue } from "../../../src/tui/session-operation-queue.js"; import { createRunSink } from "../../../src/session/run-sink.js"; test("createTUIEventEmitter returns an EventEmitter", () => { @@ -97,3 +101,134 @@ test("rotation resets run-sink so a new session starts from a clean state", () = expect(runSink.getStatus()).toBe("done"); expect(collectorAfterReset.getTurns()).toHaveLength(0); }); + +// CL-5753: an interrupt can hit close() while reactor.abort()/sendQueue.drain() +// are mid-teardown, throwing before @intx/agent's close() ever reaches +// lock.release(). Once that happens the agent is already marked closed, so a +// retried close() is a silent no-op that can never free the lock either — the +// workdir's lock is stuck held for the rest of the process. The next +// buildAgent() for that same workdir is then guaranteed to throw +// AgentContextLockError ("an agent is already open for workdir: ..."), which +// is the crash from the ticket. These tests cover the two functions the +// runner now routes every rebuild through so that failure is reported in +// plain language rather than escaping as an unhandled rejection. +function stubAgent(closeImpl: () => Promise): Agent { + return { close: closeImpl } as unknown as Agent; +} + +test("closeAgentForRebuild reports a failed close without throwing", async () => { + const agent = stubAgent(() => Promise.reject(new AgentContextLockError("/tmp/workdir"))); + const closedCleanly = await closeAgentForRebuild(agent, "interrupt"); + expect(closedCleanly).toBe(false); +}); + +test("closeAgentForRebuild reports success when close() resolves", async () => { + const agent = stubAgent(() => Promise.resolve()); + const closedCleanly = await closeAgentForRebuild(agent, "interrupt"); + expect(closedCleanly).toBe(true); +}); + +test("agentRebuildFailure turns a stale-lock AgentContextLockError into a plain-language message", () => { + // Simulates the second acquisition throwing after a failed close left the + // lock held: buildAgent() surfaces AgentContextLockError, which must not + // reach the caller as a raw stack trace. + const err = agentRebuildFailure(new AgentContextLockError("/tmp/workdir")); + expect(err.message).not.toContain("already open"); + expect(err.message).toMatch(/restart/i); +}); + +test("agentRebuildFailure passes other errors through unchanged", () => { + const original = new Error("network unreachable"); + expect(agentRebuildFailure(original)).toBe(original); +}); + +test("a failed close followed by a lock error never surfaces as a raw AgentContextLockError", async () => { + // End-to-end shape of the fix: close() throws (lock leaked in-process), + // the rebuild site short-circuits instead of calling buildAgent() again, + // and the resulting error is the plain-language one — never the raw + // AgentContextLockError a bare `throw` would have produced. + const agent = stubAgent(() => Promise.reject(new AgentContextLockError("/tmp/workdir"))); + let rebuildError: Error | null = null; + try { + const closedCleanly = await closeAgentForRebuild(agent, "interrupt"); + if (!closedCleanly) { + throw new AgentContextLockError("/tmp/workdir"); + } + } catch (err) { + rebuildError = agentRebuildFailure(err); + } + expect(rebuildError).not.toBeNull(); + expect(rebuildError).not.toBeInstanceOf(AgentContextLockError); + expect(rebuildError!.message).toMatch(/restart/i); +}); + +// reloadIfIdle itself is a closure captured inside runTUI's single ~2500-line +// scope (currentAgent, buildAgent, streamPromise, workflowController, +// pendingReload/inFlight, fatalBuildError, etc. are all local variables of +// that function), with no seam to construct or call it in isolation short of +// standing up the full TUI runner — provider config, plugin discovery, MCP +// wiring, and a real OpenTUI host. That is out of scope for this fix; it +// would be its own extraction. What can be driven directly, and is exactly +// the failure this bug reports, is the real `session-operation-queue.ts` +// queue exercised the same way every rebuild site uses it: `void +// enqueueOp(async () => { try { ... } catch (err) { fatalBuildError = ... } })`. +// `enqueue` is `tail = tail.then(op, op); return tail;` — if `op` rejects and +// nothing internally catches it, that returned promise is the only thing +// that ever observes the rejection, and `void` discards it, which is +// precisely how the unhandled rejection in the ticket escaped. +test("a rejecting reload op through the real session-operation-queue never triggers an unhandled rejection", async () => { + const { enqueue, awaitTail } = createSessionOperationQueue(); + const agent = stubAgent(() => Promise.reject(new AgentContextLockError("/tmp/workdir"))); + + let unhandled: unknown = null; + const onUnhandledRejection = (reason: unknown): void => { + unhandled = reason; + }; + process.on("unhandledRejection", onUnhandledRejection); + + let fatalBuildError: Error | null = null; + try { + // Mirrors reloadIfIdle's body verbatim: close the current agent through + // closeAgentForRebuild, skip buildAgent() and throw instead of + // re-acquiring on a failed close, and land any failure in + // fatalBuildError via agentRebuildFailure — all behind `void enqueueOp`, + // exactly as the runner calls it. + void enqueue(async () => { + try { + const closedCleanly = await closeAgentForRebuild(agent, "reload"); + if (!closedCleanly) { + throw new AgentContextLockError("/tmp/workdir"); + } + } catch (err) { + fatalBuildError = agentRebuildFailure(err); + } + }); + + await awaitTail(); + // Give any unhandled rejection queued by the engine a chance to fire + // before asserting its absence — it lands on a later microtask/macrotask + // than the awaited queue settlement. + await new Promise((resolve) => setTimeout(resolve, 0)); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + } + + expect(unhandled).toBeNull(); + expect(fatalBuildError).not.toBeNull(); + expect(fatalBuildError).not.toBeInstanceOf(AgentContextLockError); + expect(fatalBuildError!.message).toMatch(/restart/i); +}); + +// A true negative control (reproducing reloadIfIdle's pre-fix shape — no +// try/catch around the queued op — and asserting the rejection escapes) was +// attempted here and deliberately removed: bun:test installs its own +// `unhandledRejection` listener that fails whichever test is running the +// instant one fires, regardless of what that test asserts, so a test +// designed to prove an unhandled rejection *does* escape cannot pass in this +// harness — it is intercepted before the assertion runs. That interception +// is itself the strongest available evidence for the bug this fix removes: +// the pre-fix `reloadIfIdle` body run through this exact harness fails the +// suite outright (confirmed manually while writing this test), rather than +// failing a single assertion. The test above is the harness-compatible half +// of that pair: same real queue, same real helpers, proving the fixed shape +// produces no such failure.