From eee83d782abc1169d2f5143398ceb98fa41bddb4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 3 Oct 2026 21:37:13 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=A4=96=20fix:=20session=20disposal=20?= =?UTF-8?q?does=20not=20wait=20for=20a=20stopping=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #5589 --- src/node/services/backgroundProcessManager.ts | 32 ++++++++++++++++--- .../backgroundProcessesFormalRepro.test.ts | 23 +++++++++---- src/node/services/tools/bash.ts | 4 ++- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/node/services/backgroundProcessManager.ts b/src/node/services/backgroundProcessManager.ts index cd27ad391fa..b5d400ebb9c 100644 --- a/src/node/services/backgroundProcessManager.ts +++ b/src/node/services/backgroundProcessManager.ts @@ -516,6 +516,12 @@ export class BackgroundProcessManager extends EventEmitter>>(); + /** + * Pending entries of migrations whose command is being terminated (refused or failed): they + * never register, and they stay pending until the command's exit is confirmed, which may be + * never (#5589). Only cleanups that delete the checkout afterwards wait for them. + */ + private readonly stoppingAdmissions = new WeakSet>(); /** * Open admission seals per workspace (#4967), counted: while any is held, beginMigration() and * spawn() refuse. cleanup() holds one for its own duration; a removal or archive holds one @@ -1456,11 +1462,21 @@ export class BackgroundProcessManager extends EventEmitter pending[Symbol.dispose]() }; + return { + admitted, + markStopping: () => this.stoppingAdmissions.add(pending.settled), + [Symbol.dispose]: () => pending[Symbol.dispose](), + }; } /** @@ -1475,7 +1491,9 @@ export class BackgroundProcessManager extends EventEmitter } { const settled = Promise.withResolvers(); let pending = this.pendingAdmissions.get(workspaceId); if (pending === undefined) { @@ -1484,6 +1502,7 @@ export class BackgroundProcessManager extends EventEmitter { const current = this.pendingAdmissions.get(workspaceId); current?.delete(settled.promise); @@ -1518,6 +1537,9 @@ export class BackgroundProcessManager extends EventEmitter !this.stoppingAdmissions.has(entry)); + if (registering.length === 0) return; + await Promise.all(registering); } return; } diff --git a/src/node/services/backgroundProcessesFormalRepro.test.ts b/src/node/services/backgroundProcessesFormalRepro.test.ts index 720eacae963..497be42eafb 100644 --- a/src/node/services/backgroundProcessesFormalRepro.test.ts +++ b/src/node/services/backgroundProcessesFormalRepro.test.ts @@ -428,8 +428,7 @@ describe("#5465 case 1: a refused migration whose command outlives the kill join ) { const ws = uniqueWorkspace(tag); const manager = new BackgroundProcessManager(path.dirname(localBgWorkspaceDir(ws))); - // A rejected exit observation keeps the migration pending for good, so cleanup() would hang. - if (!rejectExit) cleanups.push(() => manager.cleanup(ws)); + cleanups.push(() => manager.cleanup(ws)); const dir = await tempDir(tag); const pidFile = path.join(dir, "pid"); const releaseKill = Promise.withResolvers(); @@ -527,12 +526,24 @@ describe("#5465 case 1: a refused migration whose command outlives the kill join }, 20_000); } - test("a migration whose exit observation fails stays pending", async () => { + test("a migration whose exit is never confirmed blocks removal but not session disposal", async () => { const { manager, ws, releaseKill } = await refuseMigration("join-reject", true, false, true); // The kill takes effect, but the runtime reports an error instead of the exit: that does not - // confirm the stop, so a removal's cleanup must not finish. + // confirm the stop, so the migration stays pending. releaseKill(); - const cleanup = manager.cleanup(ws).then(() => "finished"); - expect(await Promise.race([cleanup, Bun.sleep(500).then(() => "waiting")])).toBe("waiting"); + // Session disposal (agentSession.ts) deletes nothing: it finishes and releases its admission + // seal, so the workspace can send commands to the background again (#5589). + const disposal = manager.cleanup(ws).then(() => "finished"); + expect(await Promise.race([disposal, Bun.sleep(2_000).then(() => "waiting")])).toBe("finished"); + const later = manager.beginMigration(ws); + expect(later.admitted).toBe(true); + later[Symbol.dispose](); + // A removal's cleanup (workspaceService.ts) deletes the checkout once it returns, so it keeps + // waiting for the unconfirmed stop (and fails closed at its drain deadline). + const removal = manager.cleanup(ws, { failClosedAfterDrainTimeout: true }).then( + () => "finished", + () => "failed closed" + ); + expect(await Promise.race([removal, Bun.sleep(500).then(() => "waiting")])).toBe("waiting"); }, 20_000); }); diff --git a/src/node/services/tools/bash.ts b/src/node/services/tools/bash.ts index 6a9f6fd9415..7585060a3c2 100644 --- a/src/node/services/tools/bash.ts +++ b/src/node/services/tools/bash.ts @@ -1587,8 +1587,10 @@ ${scriptWithEnv}`; // has not taken effect yet (#5522). // A rejected exitCode (e.g. a remote transport error) does not confirm the stop, so the // migration then stays pending for the session: removal and archive keep failing - // closed rather than deleting the checkout under a command that may still run. + // closed rather than deleting the checkout under a command that may still run. Session + // disposal, which deletes nothing, does not wait for a stopping migration (#5589). migrationHandedToExit = true; + migration?.markStopping(); void execStream.exitCode.then( () => migration?.[Symbol.dispose](), () => undefined From 450bb835d6492fd6fffcb88d0d465d1bf8eef8d1 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 3 Oct 2026 21:50:48 +0000 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=A4=96=20fix:=20wake=20a=20waiting=20?= =?UTF-8?q?disposal=20drain=20when=20a=20migration=20becomes=20stopping?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../services/backgroundProcessManager.test.ts | 21 +++++++++++++++++++ src/node/services/backgroundProcessManager.ts | 12 +++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/node/services/backgroundProcessManager.test.ts b/src/node/services/backgroundProcessManager.test.ts index 524feedd845..fabf9d5d51e 100644 --- a/src/node/services/backgroundProcessManager.test.ts +++ b/src/node/services/backgroundProcessManager.test.ts @@ -2998,6 +2998,27 @@ describe("BackgroundProcessManager", () => { expect(ws2Processes.length).toBeGreaterThanOrEqual(1); expect(ws2Processes.some((p) => p.status === "running")).toBe(true); }); + + it("lets session disposal finish once a migration it waits for becomes stopping", async () => { + // #5589: disposal can start while a migration is still on its way to registering. If the + // migration then fails and its command's exit is never confirmed, it never settles; the + // disposal drain must notice the stopping transition instead of waiting on it forever. + const migration = manager.beginMigration(testWorkspaceId); + expect(migration.admitted).toBe(true); + const disposal = manager.cleanup(testWorkspaceId).then(() => "finished"); + await Bun.sleep(50); + migration.markStopping(); + expect(await Promise.race([disposal, Bun.sleep(2_000).then(() => "waiting")])).toBe( + "finished" + ); + // Removal still waits for it (and fails closed at its drain deadline). + const removal = manager + .cleanup(testWorkspaceId, { failClosedAfterDrainTimeout: true }) + .then(() => "finished"); + expect(await Promise.race([removal, Bun.sleep(200).then(() => "waiting")])).toBe("waiting"); + migration[Symbol.dispose](); + expect(await removal).toBe("finished"); + }); }); describe("cleanup with a hung spawn", () => { diff --git a/src/node/services/backgroundProcessManager.ts b/src/node/services/backgroundProcessManager.ts index b5d400ebb9c..cccaf40120c 100644 --- a/src/node/services/backgroundProcessManager.ts +++ b/src/node/services/backgroundProcessManager.ts @@ -522,6 +522,8 @@ export class BackgroundProcessManager extends EventEmitter>(); + /** Resolved and replaced on each markStopping(), so a disposal drain already waiting re-checks. */ + private stoppingMarked = Promise.withResolvers(); /** * Open admission seals per workspace (#4967), counted: while any is held, beginMigration() and * spawn() refuse. cleanup() holds one for its own duration; a removal or archive holds one @@ -1474,7 +1476,11 @@ export class BackgroundProcessManager extends EventEmitter this.stoppingAdmissions.add(pending.settled), + markStopping: () => { + this.stoppingAdmissions.add(pending.settled); + this.stoppingMarked.resolve(); + this.stoppingMarked = Promise.withResolvers(); + }, [Symbol.dispose]: () => pending[Symbol.dispose](), }; } @@ -1553,7 +1559,9 @@ export class BackgroundProcessManager extends EventEmitter !this.stoppingAdmissions.has(entry)); if (registering.length === 0) return; - await Promise.all(registering); + // A migration may become stopping while this waits (disposal can start before a migration + // fails): wake up and re-check rather than wait on an entry that may never settle. + await Promise.race([Promise.all(registering), this.stoppingMarked.promise]); } return; }