From febf3eb73dc49b6672b7b2c3ae43f7c8c77be9df Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 23:42:18 +0000 Subject: [PATCH] fix(coding-agent): bounded cancelled vibe teardown vibe_kill and Vibe-mode exit / session-switch suspension awaited every cancelled turn's job promise unconditionally. When a provider or tool ignores the abort signal the promise never settles, so teardown hung indefinitely. Give cancelled jobs a 250ms unref'd settlement grace, then log and detach any still-pending job while preserving cancelled state. Ported from @RensTillmann's fix (65446c4). Fixes #5303 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/vibe/runtime.ts | 24 +++- .../test/vibe/vibe-runtime.test.ts | 111 +++++++++++++++--- 3 files changed, 120 insertions(+), 16 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4676cc148..4257621c2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed persisted vibe workers disappearing or being replaced across graceful restarts, session switches, failed mode exits, and late cancelled initialization: resumable conversations now restore safely, mode exit atomically commits worker tombstones with the mode change and rolls back cleanly on storage failure, explicit kills tear workers down monotonically while repairing uncertain append tails, killed transcripts remain readable but non-revivable, and stale initializers cannot overwrite newer same-ID workers ([#5303](https://github.com/can1357/oh-my-pi/issues/5303) by [@mastertyko](https://github.com/mastertyko)). +- Bounded vibe teardown so `vibe_kill` and Vibe-mode exit / session-switch suspension no longer hang when a cancelled turn's provider or tool ignores the abort signal: cancelled jobs get a short unref'd settlement grace, then detach while staying marked cancelled ([#5303](https://github.com/can1357/oh-my-pi/issues/5303) by [@RensTillmann](https://github.com/RensTillmann)). ## [16.4.8] - 2026-07-12 diff --git a/packages/coding-agent/src/vibe/runtime.ts b/packages/coding-agent/src/vibe/runtime.ts index a040d5b95..56214dfa7 100644 --- a/packages/coding-agent/src/vibe/runtime.ts +++ b/packages/coding-agent/src/vibe/runtime.ts @@ -69,6 +69,8 @@ const TRACE_LINE_MAX = 120; const DEFAULT_WAIT_TIMEOUT_MS = 30_000; /** Response text cap inside a delivered turn result; full output stays at agent://. */ const RESPONSE_PREVIEW_MAX = 6000; +/** Grace period for abort-aware turns before teardown detaches a stuck provider/tool call. */ +const CANCELLED_TURN_SETTLE_GRACE_MS = 250; const VIBE_LIFECYCLE_CUSTOM_TYPE = "vibe-session-lifecycle"; const VIBE_LIFECYCLE_VERSION = 1; @@ -351,6 +353,24 @@ function mergeTrace(turn: VibeTurn, progress: AgentProgress): void { /** Thrown from a turn job body so the job manager marks the job failed while carrying the formatted result. */ export class VibeTurnError extends Error {} +async function awaitCancelledTurnJobs(jobs: ReadonlySet): Promise { + if (jobs.size === 0) return; + const settled = Promise.allSettled([...jobs].map(job => job.promise)).then(() => true); + const timeout = Promise.withResolvers(); + const timer = setTimeout(() => timeout.resolve(false), CANCELLED_TURN_SETTLE_GRACE_MS); + timer.unref(); + try { + if (!(await Promise.race([settled, timeout.promise]))) { + logger.warn("vibe: detached cancelled turn that did not settle within teardown grace period", { + jobCount: jobs.size, + graceMs: CANCELLED_TURN_SETTLE_GRACE_MS, + }); + } + } finally { + clearTimeout(timer); + } +} + /** * Process-global registry of vibe worker sessions, scoped by both owner agent * id and stable parent session id. Persisted lifecycle events rebuild idle @@ -1043,7 +1063,7 @@ export class VibeSessionRegistry { }); } } - await Promise.allSettled(teardown.flatMap(entry => (entry.job ? [entry.job.promise] : []))); + await awaitCancelledTurnJobs(new Set(teardown.flatMap(entry => (entry.job ? [entry.job] : [])))); return records.length; } @@ -1154,7 +1174,7 @@ export class VibeSessionRegistry { }); } } - await Promise.allSettled([...settlingJobs].map(job => job.promise)); + await awaitCancelledTurnJobs(settlingJobs); const terminalRef = registered ?? this.#registeredAgent(record) ?? null; if (record.childSessionFile) { try { diff --git a/packages/coding-agent/test/vibe/vibe-runtime.test.ts b/packages/coding-agent/test/vibe/vibe-runtime.test.ts index 3eeb044e2..e745acb6b 100644 --- a/packages/coding-agent/test/vibe/vibe-runtime.test.ts +++ b/packages/coding-agent/test/vibe/vibe-runtime.test.ts @@ -803,6 +803,61 @@ describe("vibe session registry", () => { expect(turnJob.resultText).toContain('turn="2"'); }); + it("bounds parent-session suspension when a cancelled turn ignores abort and settles late", async () => { + const gate = deferred(); + const started = deferred(); + const disposed = deferred(); + const fake = createFakeWorkerSession({ onDispose: disposed.resolve }); + vi.spyOn(executorModule, "runSubprocess").mockImplementation(async options => { + AgentRegistry.global().register({ + id: options.id, + displayName: options.id, + kind: "sub", + parentId: "Main", + session: fake.session, + status: "running", + }); + started.resolve(); + await gate.promise; + return makeResult(options.id); + }); + + const manager = createManager(); + const session = createSession({ manager }); + const registry = VibeSessionRegistry.global(); + const { jobId } = await registry.spawn(session, { + cli: "fast", + name: "IgnoresSuspendAbort", + prompt: "Keep working through a parent-session switch.", + }); + await started.promise; + + vi.useFakeTimers(); + try { + const suspension = registry.suspendScope(registry.ownerScope(session), manager); + await disposed.promise; + await flushMicrotasks(); + expect(vi.getTimerCount()).toBeGreaterThan(0); + vi.advanceTimersByTime(250); + + expect(await suspension).toBe(1); + expect(manager.getJob(jobId)!.status).toBe("cancelled"); + expect(fake.isDisposed()).toBe(true); + expect(AgentRegistry.global().get("IgnoresSuspendAbort")).toBeUndefined(); + expect(registry.listIds(session)).toEqual([]); + + gate.resolve(); + await manager.getJob(jobId)!.promise; + expect(manager.getJob(jobId)!.status).toBe("cancelled"); + expect(fake.isDisposed()).toBe(true); + expect(AgentRegistry.global().get("IgnoresSuspendAbort")).toBeUndefined(); + expect(registry.listIds(session)).toEqual([]); + } finally { + gate.resolve(); + vi.useRealTimers(); + } + }); + it("restores an interrupted turn as idle without replay and continues only after send", async () => { installPersistedSpawnMock(); const parentManager = await createPersistedParent(); @@ -1835,9 +1890,11 @@ describe("vibe session registry", () => { await manager.getJob("Fast-t2")!.promise; }); - it("kill cancels the in-flight turn and releases the worker session", async () => { + it("bounds kill teardown when a cancelled turn ignores abort and settles late", async () => { const gate = deferred(); - const fake = createFakeWorkerSession(); + const started = deferred(); + const disposed = deferred(); + const fake = createFakeWorkerSession({ onDispose: disposed.resolve }); vi.spyOn(executorModule, "runSubprocess").mockImplementation(async options => { AgentRegistry.global().register({ id: options.id, @@ -1847,6 +1904,7 @@ describe("vibe session registry", () => { session: fake.session, status: "running", }); + started.resolve(); await gate.promise; return makeResult(options.id); }); @@ -1854,19 +1912,44 @@ describe("vibe session registry", () => { const manager = createManager(); const session = createSession({ manager }); const registry = VibeSessionRegistry.global(); - const { jobId } = await registry.spawn(session, { cli: "fast", name: "Doomed", prompt: "Never mind." }); - await pollUntil(() => AgentRegistry.global().get("Doomed") !== undefined); + const { jobId } = await registry.spawn(session, { + cli: "fast", + name: "IgnoresKillAbort", + prompt: "Keep working through explicit termination.", + }); + await started.promise; - const killPromise = registry.kill(session, "Doomed"); - await pollUntil(() => manager.getJob(jobId)?.status === "cancelled"); - gate.resolve(); - const outcome = await killPromise; - expect(outcome.cancelledTurn).toBe(true); - expect(manager.getJob(jobId)!.status).toBe("cancelled"); - expect(fake.isDisposed()).toBe(true); - expect(AgentRegistry.global().get("Doomed")).toBeUndefined(); - expect(registry.screens(session)[0]?.state).toBe("dead"); - await expect(registry.send(session, { session: "Doomed", message: "hello?" })).rejects.toThrow("dead"); + vi.useFakeTimers(); + try { + const kill = registry.kill(session, "IgnoresKillAbort"); + await disposed.promise; + await flushMicrotasks(); + expect(vi.getTimerCount()).toBeGreaterThan(0); + vi.advanceTimersByTime(250); + + const outcome = await kill; + expect(outcome.cancelledTurn).toBe(true); + expect(manager.getJob(jobId)!.status).toBe("cancelled"); + expect(fake.isDisposed()).toBe(true); + expect(AgentRegistry.global().get("IgnoresKillAbort")).toBeUndefined(); + expect(registry.screens(session)[0]?.state).toBe("dead"); + await expect(registry.send(session, { session: "IgnoresKillAbort", message: "hello?" })).rejects.toThrow( + "dead", + ); + + gate.resolve(); + await manager.getJob(jobId)!.promise; + expect(manager.getJob(jobId)!.status).toBe("cancelled"); + expect(fake.isDisposed()).toBe(true); + expect(AgentRegistry.global().get("IgnoresKillAbort")).toBeUndefined(); + expect(registry.screens(session)[0]?.state).toBe("dead"); + await expect(registry.send(session, { session: "IgnoresKillAbort", message: "still there?" })).rejects.toThrow( + "dead", + ); + } finally { + gate.resolve(); + vi.useRealTimers(); + } }); it("keeps a persisted in-flight kill terminal when the old executor finalizes late", async () => {