From c375ff5a9290c35eb103ac3ad63b3e27fadc85ce Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 26 May 2026 22:48:13 +0200 Subject: [PATCH] fix(coding-agent): resolved agent session /exit Ctrl+C hang in dispose - Short-circuited `agent_end` when `#checkCompaction` deferred handoff, skipping rewind/todo passes and `agent.continue()` race. - Aborted retry/compaction paths in `AgentSession.dispose()` before draining post-prompt tasks so `/exit` and Ctrl+C no longer hang. - Added handoff-deadlock regression tests in `agent-session-handoff.test.ts` to prevent reordering races. --- packages/coding-agent/CHANGELOG.md | 2 + .../coding-agent/src/session/agent-session.ts | 120 ++++++++++++++---- .../test/agent-session-handoff.test.ts | 112 ++++++++++++++++ 3 files changed, 207 insertions(+), 27 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f3c843f30..1c4681dfd 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,8 @@ - Fixed the agent loop and bash executor busy-spinning when scheduler waits returned early. napi `uv_async_send` callbacks can wake the event loop after only ~1–2 ms even when the caller asked for a longer sleep, so `yieldIfDue()` now compensates by retrying `Bun.sleep()` until the requested wall-clock duration has elapsed, and the bash executor wraps its `runPromise`/timeout/abort race in an `ExponentialYield` (20 ms → 10 s) so long-running commands stop hot-looping on the race. Yields are additionally throttled by a module-level 50 ms gate, and losing timers in the race are aborted via `AbortSignal` so they don't keep firing after a winner is selected ([#1396](https://github.com/can1357/oh-my-pi/pull/1396) by [@hezhiyang2000](https://github.com/hezhiyang2000), closes [#1384](https://github.com/can1357/oh-my-pi/issues/1384)). - Fixed streaming edit previews going blank for inline-payload ops. `LINE↓payload` / `A-B:payload` are valid single-line writes, but the natural-order preview was skipping the entire op token until a newline arrived — so the first character the model typed after the sigil only appeared after the next line ended, and the renderer would fall back to rendering the raw `A-B: bla bla bla` input. Inline-body content on `op-insert` and `op-replace` tokens is now emitted as a `+payload` line on the same op tick. - Fixed `/usage` and the status-line reset countdown rendering stale negative deltas after a Codex window had elapsed: Codex keeps reporting the prior window's `reset_at` until a new request opens a fresh window, which turned the `resets in …` suffix into a meaningless negative duration. `formatDuration` now clamps non-positive, NaN, and infinite inputs to `0ms`, and both renderers suppress the `resets in …` suffix entirely once `resetsAt <= now`. +- Fixed auto-handoff race at the context threshold: when `compaction.strategy = handoff` fired at `agent_end` with an active checkpoint or incomplete todos, the deferred handoff post-prompt task and the rewind/todo-completion path both scheduled work concurrently, so a fresh `agent.continue()` streamed a new assistant turn alongside the handoff LLM call (visible as the "Auto-handoff" loader plus an assistant message still streaming, with the chat container then rebuilt mid-stream). `#checkCompaction` now reports whether it deferred a handoff and the `agent_end` handler short-circuits the rewind/todo passes; `#scheduleAgentContinue` also skips when `isCompacting || isGeneratingHandoff`. The pre-prompt `#checkCompaction` call now forces inline execution (`allowDefer = false`) so the new turn cannot begin until the maintenance settles. +- Fixed `/exit` and Ctrl+C-double-tap hanging when a deferred handoff was mid-flight: `AgentSession.dispose()` now aborts retry/compaction (auto-compaction + handoff) and the agent stream before draining `#cancelPostPromptTasks`, so the post-prompt task awaiting `generateHandoff` rejects and `Promise.allSettled` can resolve. Tool work (bash/eval/python) is intentionally still left for the existing dispose paths so shared kernels continue to survive across session dispose. ## [15.4.3] - 2026-05-26 diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 45c0faf28..d8a6d2259 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -898,7 +898,7 @@ export class AgentSession { * combined with `Date.now()` so tags stay unique even across rapid * same-tick enqueues. */ #customDisplayTagCounter = 0; - #postPromptTasks = new Set>(); + #postPromptTasks = new Set>(); #postPromptTasksPromise: Promise | undefined = undefined; #postPromptTasksResolve: (() => void) | undefined = undefined; #postPromptTasksAbortController = new AbortController(); @@ -1786,12 +1786,19 @@ export class AgentSession { const compactionTask = this.#checkCompaction(msg); this.#trackPostPromptTask(compactionTask); - await compactionTask; + const compactionDeferredHandoff = await compactionTask; // Check for incomplete todos only after a final assistant stop, not intermediate tool-use turns. const hasToolCalls = msg.content.some(content => content.type === "toolCall"); if (hasToolCalls) { return; } + // When checkCompaction scheduled a deferred handoff, skip the rewind/todo passes: + // any reminder we append here would race the handoff's session reset, and + // #scheduleAgentContinue would start a fresh streaming turn alongside the handoff + // LLM call (visible as "Auto-handoff" loader + an assistant message still streaming). + if (compactionDeferredHandoff) { + return; + } if (msg.stopReason !== "error" && msg.stopReason !== "aborted") { if (this.#enforceRewindBeforeYield()) { return; @@ -1840,7 +1847,7 @@ export class AgentSession { this.#postPromptTasksPromise = undefined; } - #trackPostPromptTask(task: Promise): void { + #trackPostPromptTask(task: Promise): void { this.#postPromptTasks.add(task); this.#ensurePostPromptTasksPromise(); void task @@ -1889,8 +1896,17 @@ export class AgentSession { }): void { this.#schedulePostPromptTask( async () => { + // Defense in depth: if compaction/handoff slipped onto the post-prompt queue + // alongside us (e.g. via a scheduler we don't own), refuse to start a fresh + // streaming turn — agent.continue() here would race the handoff's session + // reset. The first-class fix is in #checkCompaction/the agent_end handler, + // but this guard catches anything that bypasses that path. + if (this.isCompacting || this.isGeneratingHandoff) { + options?.onSkip?.(); + return; + } if (options?.shouldContinue && !options.shouldContinue()) { - options.onSkip?.(); + options?.onSkip?.(); return; } try { @@ -2756,6 +2772,21 @@ export class AgentSession { } catch (error) { logger.warn("Failed to emit session_shutdown event", { error: String(error) }); } + // Abort post-prompt work so the drain below can complete. Without this, a + // deferred-handoff task that has already advanced into + // `await this.handoff(...) → generateHandoff(...)` keeps awaiting a live LLM stream + // — Promise.allSettled() in #cancelPostPromptTasks then waits forever, freezing + // /exit and Ctrl+C-double-tap. The post-prompt task's own AbortSignal does not + // propagate into the inner handoff/compaction controllers, so we abort them + // explicitly. agent.abort() is needed for an agent.continue() that may have + // raced the deferred handoff (its streaming loop is awaited by the wrapper IIFE). + // + // Tool work (bash/eval/python) is NOT aborted here — those have their own + // dispose paths and shared kernels are contractually allowed to survive a + // session's dispose. + this.abortRetry(); + this.abortCompaction(); + this.agent.abort(); await this.#cancelPostPromptTasks(); // Cancel jobs this agent registered so a subagent's teardown doesn't // leak its background bash/task work into the parent's manager. Only @@ -4050,10 +4081,13 @@ export class AgentSession { ); } - // Check if we need to compact before sending (catches aborted responses) + // Check if we need to compact before sending (catches aborted responses). Run + // inline (allowDefer=false) so the handoff/maintenance fully settles before this + // prompt's agent loop starts — otherwise a deferred handoff would fire on the + // next microtask alongside the new turn. const lastAssistant = this.#findLastAssistantMessage(); if (lastAssistant && !options?.skipCompactionCheck) { - await this.#checkCompaction(lastAssistant, false); + await this.#checkCompaction(lastAssistant, false, false); } // Build messages array (session context, eager todo prelude, then active prompt message) @@ -5602,10 +5636,23 @@ export class AgentSession { * * @param assistantMessage The assistant message to check * @param skipAbortedCheck If false, include aborted messages (for pre-prompt check). Default: true + * @param allowDefer If true, threshold-driven handoff strategy may schedule itself as a + * deferred post-prompt task instead of running inline. Callers running inside the + * `agent_end` handler set this to true so `session.prompt()` resolves cleanly; callers + * on the pre-prompt path (where the next agent turn is about to start) set it to false + * to avoid racing the deferred handoff against the new turn. + * @returns true when a deferred handoff was scheduled. Callers MUST then skip any + * subsequent `#scheduleAgentContinue` / reminder appends for this turn — the + * handoff will replace session state and a concurrent `agent.continue()` would + * stream into the soon-to-be-discarded session. */ - async #checkCompaction(assistantMessage: AssistantMessage, skipAbortedCheck = true): Promise { + async #checkCompaction( + assistantMessage: AssistantMessage, + skipAbortedCheck = true, + allowDefer = true, + ): Promise { // Skip if message was aborted (user cancelled) - unless skipAbortedCheck is false - if (skipAbortedCheck && assistantMessage.stopReason === "aborted") return; + if (skipAbortedCheck && assistantMessage.stopReason === "aborted") return false; const contextWindow = this.model?.contextWindow ?? 0; const generation = this.#promptGeneration; // Skip overflow check if the message came from a different model. @@ -5634,22 +5681,22 @@ export class AgentSession { if (promoted) { // Retry on the promoted (larger) model without compacting this.#scheduleAgentContinue({ delayMs: 100, generation }); - return; + return false; } // No promotion target available fall through to compaction const compactionSettings = this.settings.getGroup("compaction"); if (compactionSettings.enabled && compactionSettings.strategy !== "off") { - await this.#runAutoCompaction("overflow", true); + await this.#runAutoCompaction("overflow", true, false, allowDefer); } - return; + return false; } const compactionSettings = this.settings.getGroup("compaction"); - if (!compactionSettings.enabled || compactionSettings.strategy === "off") return; + if (!compactionSettings.enabled || compactionSettings.strategy === "off") return false; // Case 2: Threshold - turn succeeded but context is getting large // Skip if this was an error (non-overflow errors don't have usage data) - if (assistantMessage.stopReason === "error") return; + if (assistantMessage.stopReason === "error") return false; const pruneResult = await this.#pruneToolOutputs(); let contextTokens = calculateContextTokens(assistantMessage.usage); if (pruneResult) { @@ -5659,9 +5706,10 @@ export class AgentSession { // Try promotion first — if a larger model is available, switch instead of compacting const promoted = await this.#tryContextPromotion(assistantMessage); if (!promoted) { - await this.#runAutoCompaction("threshold", false); + return await this.#runAutoCompaction("threshold", false, false, allowDefer); } } + return false; } #assistantEndedWithSuccessfulYield(assistantMessage: AssistantMessage): boolean { const toolCallId = this.#lastSuccessfulYieldToolCallId; @@ -6352,17 +6400,34 @@ export class AgentSession { /** * Internal: Run auto-compaction with events. + * + * @param allowDefer If true (default), threshold-driven handoff strategy is allowed to + * schedule itself as a deferred post-prompt task and return `true` immediately. The + * caller MUST treat that as "compaction will happen async — do not also schedule + * `agent.continue()` for this turn", otherwise the deferred handoff races a fresh + * streaming turn (the symptom: "Auto-handoff" loader + assistant message still + * streaming). Callers on a path that is about to start a new agent turn (e.g. + * the pre-prompt check in `#promptWithMessage`) pass `false` to force inline + * execution so the handoff completes before the new turn begins. + * @returns true when a deferred handoff was scheduled. Inline runs always return false. */ async #runAutoCompaction( reason: "overflow" | "threshold" | "idle", willRetry: boolean, deferred = false, - ): Promise { + allowDefer = true, + ): Promise { const compactionSettings = this.settings.getGroup("compaction"); - if (compactionSettings.strategy === "off") return; - if (reason !== "idle" && !compactionSettings.enabled) return; + if (compactionSettings.strategy === "off") return false; + if (reason !== "idle" && !compactionSettings.enabled) return false; const generation = this.#promptGeneration; - if (!deferred && reason !== "overflow" && reason !== "idle" && compactionSettings.strategy === "handoff") { + if ( + !deferred && + allowDefer && + reason !== "overflow" && + reason !== "idle" && + compactionSettings.strategy === "handoff" + ) { this.#schedulePostPromptTask( async signal => { await Promise.resolve(); @@ -6371,7 +6436,7 @@ export class AgentSession { }, { generation }, ); - return; + return true; } let action: "context-full" | "handoff" = @@ -6400,7 +6465,7 @@ export class AgentSession { aborted: true, willRetry: false, }); - return; + return false; } logger.warn("Auto-handoff returned no document; falling back to context-full maintenance", { reason, @@ -6418,7 +6483,7 @@ export class AgentSession { if (!autoCompactionSignal.aborted && reason !== "idle" && compactionSettings.autoContinue !== false) { this.#scheduleAutoContinuePrompt(generation); } - return; + return false; } } @@ -6431,7 +6496,7 @@ export class AgentSession { willRetry: false, skipped: true, }); - return; + return false; } const availableModels = this.#modelRegistry.getAvailable(); @@ -6444,7 +6509,7 @@ export class AgentSession { willRetry: false, skipped: true, }); - return; + return false; } const pathEntries = this.sessionManager.getBranch(); @@ -6466,7 +6531,7 @@ export class AgentSession { shouldContinue: () => this.agent.hasQueuedMessages(), }); } - return; + return false; } let hookCompaction: CompactionResult | undefined; @@ -6490,7 +6555,7 @@ export class AgentSession { aborted: true, willRetry: false, }); - return; + return false; } if (hookResult?.compaction) { @@ -6621,7 +6686,7 @@ export class AgentSession { aborted: true, willRetry: false, }); - return; + return false; } this.sessionManager.appendCompaction( @@ -6692,7 +6757,7 @@ export class AgentSession { aborted: true, willRetry: false, }); - return; + return false; } const errorMessage = error instanceof Error ? error.message : "compaction failed"; await this.#emitSessionEvent({ @@ -6711,6 +6776,7 @@ export class AgentSession { this.#autoCompactionAbortController = undefined; } } + return false; } /** diff --git a/packages/coding-agent/test/agent-session-handoff.test.ts b/packages/coding-agent/test/agent-session-handoff.test.ts index a2b7b0977..b3a970bd5 100644 --- a/packages/coding-agent/test/agent-session-handoff.test.ts +++ b/packages/coding-agent/test/agent-session-handoff.test.ts @@ -432,6 +432,118 @@ describe("AgentSession handoff", () => { expect(sessionManager.getEntries().filter(entry => entry.type === "compaction")).toHaveLength(0); }); + it("does not start agent.continue when threshold-handoff defers and todos are incomplete", async () => { + // Reproduces the user-reported race: at agent_end, threshold + handoff strategy + // schedules a deferred handoff and returns. The handler used to fall through to + // #checkTodoCompletion, which scheduled agent.continue() — both fired concurrently, + // rendering as "Auto-handoff" loader + an assistant message still streaming. + session.settings.set("compaction.strategy", "handoff"); + session.settings.set("compaction.thresholdPercent", 1); + session.settings.set("contextPromotion.enabled", false); + session.settings.set("todo.enabled", true); + session.settings.set("todo.reminders", true); + + // Active todo phase with an incomplete task so #checkTodoCompletion would normally fire. + session.setTodoPhases([{ name: "Phase 1", tasks: [{ content: "unfinished work", status: "pending" }] }]); + + const model = session.model; + if (!model) { + throw new Error("Expected model to be set"); + } + + const handoffSpy = vi + .spyOn(session, "handoff") + .mockResolvedValue({ document: "## Goal\nContinue", savedPath: undefined }); + const continueSpy = vi.spyOn(session.agent, "continue"); + + const assistantMessage: AssistantMessage = { + role: "assistant", + content: [{ type: "text", text: "maintenance trigger" }], + api: model.api, + provider: model.provider, + model: model.id, + stopReason: "stop", + usage: { + input: 10_000, + output: 1_000, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 11_000, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }; + + session.agent.emitExternalEvent({ type: "message_end", message: assistantMessage }); + session.agent.emitExternalEvent({ type: "agent_end", messages: [assistantMessage] }); + await Bun.sleep(20); + + expect(handoffSpy).toHaveBeenCalledTimes(1); + // The bug surfaced as agent.continue() racing the deferred handoff. With the fix, + // the agent_end handler short-circuits after the deferred-handoff signal. + expect(continueSpy).not.toHaveBeenCalled(); + }); + + it("dispose unblocks the post-prompt drain when a deferred handoff is mid-flight", async () => { + // Reproduces /exit / Ctrl+C-double-tap hanging when a deferred handoff is awaiting + // the LLM call: dispose() now aborts the handoff controller before draining post-prompt + // tasks, so Promise.allSettled() in #cancelPostPromptTasks can resolve. + session.settings.set("compaction.strategy", "handoff"); + session.settings.set("compaction.thresholdPercent", 1); + session.settings.set("contextPromotion.enabled", false); + + const model = session.model; + if (!model) { + throw new Error("Expected model to be set"); + } + + const { promise: handoffPending, resolve: resolveHandoff } = Promise.withResolvers(); + const generateHandoffSpy = vi + .spyOn(compactionModule, "generateHandoff") + .mockImplementation(async (_msgs, _model, _key, _opts, signal) => { + // Mirror the real generateHandoff contract: reject when the caller aborts. + return await new Promise((resolve, reject) => { + signal?.addEventListener("abort", () => reject(new Error("Handoff cancelled")), { once: true }); + handoffPending.then(resolve, reject); + }); + }); + + const assistantMessage: AssistantMessage = { + role: "assistant", + content: [{ type: "text", text: "maintenance trigger" }], + api: model.api, + provider: model.provider, + model: model.id, + stopReason: "stop", + usage: { + input: 10_000, + output: 1_000, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 11_000, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }; + + session.agent.emitExternalEvent({ type: "message_end", message: assistantMessage }); + session.agent.emitExternalEvent({ type: "agent_end", messages: [assistantMessage] }); + // Let the deferred handoff post-prompt task enter the generateHandoff await. + await Bun.sleep(20); + expect(generateHandoffSpy).toHaveBeenCalledTimes(1); + expect(session.isGeneratingHandoff).toBe(true); + + // dispose must NOT wait for the LLM call to resolve on its own — it must abort it. + const disposed = Promise.race([ + session.dispose().then(() => "disposed" as const), + Bun.sleep(2_000).then(() => "timeout" as const), + ]); + + await expect(disposed).resolves.toBe("disposed"); + // Releasing after the fact must not leak into other tests. + resolveHandoff(undefined); + }); + it("falls back to context-full when handoff strategy returns no document", async () => { session.settings.set("compaction.strategy", "handoff"); session.settings.set("compaction.thresholdPercent", 1);