diff --git a/src/application/binary/BinarySession.ts b/src/application/binary/BinarySession.ts index 0ec43983f4..0cc232ac69 100644 --- a/src/application/binary/BinarySession.ts +++ b/src/application/binary/BinarySession.ts @@ -154,6 +154,8 @@ export class BinarySession #transition: Promise = Promise.resolve(); #transitionGeneration = 0; #transitionPending = 0; + /** Process shutdown has begun; later target acquisition must not outlive it. */ + #shutdownStarted = false; readonly #calls = new Map, number>(); readonly #promptCompletionControllers = new Set(); readonly #providerRouter: SessionProviderRouter; @@ -182,6 +184,8 @@ export class BinarySession options: Pick = {}, ): Promise> { return this.#serialize(async (admittedThrough) => { + const refused = this.#targetAcquisitionRefused("open_bundle_resources"); + if (refused !== undefined) return refused; await this.#drainCalls(admittedThrough); if (isAborted(options.signal)) return err( @@ -498,6 +502,8 @@ export class BinarySession options: BinarySessionOpenOptions, ): Promise> { return this.#serialize(async (admittedThrough) => { + const refused = this.#targetAcquisitionRefused("open_binary"); + if (refused !== undefined) return refused; const admitted = await this.#withLoadedSnapshot(options); if (!admitted.ok) return admitted; return this.#openWithinTransition( @@ -757,6 +763,22 @@ export class BinarySession }); } + /** + * Reject target acquisition once process shutdown has begun. + * Acquisition already inside its transition still finishes; the shutdown + * close queued behind that transition retires it. + */ + beginShutdown(): void { + this.#shutdownStarted = true; + } + + #targetAcquisitionRefused( + operation: "open_binary" | "open_bundle_resources", + ): Result | undefined { + if (!this.#shutdownStarted) return undefined; + return err(new AnalysisCancelledError(operation)); + } + /** Close the active target, if any. */ close( options: Pick & { diff --git a/src/gdb/GdbSessionManager.ts b/src/gdb/GdbSessionManager.ts index 0671ea895e..6de5042460 100644 --- a/src/gdb/GdbSessionManager.ts +++ b/src/gdb/GdbSessionManager.ts @@ -494,6 +494,8 @@ interface GdbSession { readonly settleStdout: () => void; inferiorTtyVerified: boolean; noInferiorVerified: boolean; + /** Sticky admission seal once destructive cleanup starts, including failed stops. */ + cleanupStarted: boolean; stopAttempt: ReturnType | undefined; readonly pending: Map; readonly cleanupFailures: Set; @@ -517,6 +519,18 @@ interface GdbSession { recentRecordsTruncated: boolean; } +const gdbConsoleUnavailable = (): AnalysisInputError => + new AnalysisInputError("gdb_console", undefined, [ + { + path: ["session_id"], + reason: "invalid_value", + message: "GDB session is unavailable or closed.", + }, + ]); + +const gdbCloseUnavailable = (): AnalysisInputError => + new AnalysisInputError("close_gdb_session"); + /** Owns persistent GDB/MI sessions independently from the active binary target. */ export class GdbSessionManager { readonly #sessions = new Map(); @@ -794,6 +808,7 @@ export class GdbSessionManager { // GDB is launched with --nx and receives no target-execution commands // during startup, so no inferior can exist until execute() is called. noInferiorVerified: true, + cleanupStarted: false, stopAttempt: undefined, pending: new Map(), cleanupFailures: new Set(), @@ -1140,26 +1155,27 @@ export class GdbSessionManager { this.#closing || session === undefined || !session.ready || + session.cleanupStarted || session.exited || session.protocolError !== undefined ) - return err( - new AnalysisInputError("gdb_console", undefined, [ - { - path: ["session_id"], - reason: "invalid_value", - message: "GDB session is unavailable or closed.", - }, - ]), - ); + return err(gdbConsoleUnavailable()); if (signal?.aborted) return err(cancelledWithCallerReason("gdb_console", signal)); try { const completed = await this.#serialize( session, async () => { - if (session.exited || session.protocolError !== undefined) - return err(new AnalysisInputError("gdb_console")); + // Retained cleanup ownership and a delayed exit event do not imply + // command readiness. Recheck admission when this queued turn starts. + if ( + this.#closing || + this.#sessions.get(sessionId) !== session || + session.cleanupStarted || + session.exited || + session.protocolError !== undefined + ) + return err(gdbConsoleUnavailable()); session.noInferiorVerified = false; session.knownInferiorIdentities.clear(); if (commandMayChangeInferiorOutput(command)) @@ -1324,7 +1340,10 @@ export class GdbSessionManager { session_id: session.id, mi_version: session.miVersion, state: - session.exited || session.protocolError !== undefined || !session.ready + session.cleanupStarted || + session.exited || + session.protocolError !== undefined || + !session.ready ? "closed" : "ready", recent_mi_records: [...session.lines], @@ -1352,38 +1371,17 @@ export class GdbSessionManager { if (this.#closing) return err(new AnalysisCancelledError("close_gdb_session")); const session = this.#sessions.get(sessionId); - if (session === undefined) - return err(new AnalysisInputError("close_gdb_session")); - if (session.exited) { - const stopped = await this.#stopDebuggerOnly(session); - if (stopped.status === "incomplete") - return err( - this.#retainCleanupFailure( - session, - "close_gdb_session", - stopped.reason, - ), - ); - this.#sessions.delete(sessionId); - return ok({ target_state: "unknown" }); - } + if (session === undefined) return err(gdbCloseUnavailable()); + if (session.exited) return this.#finishSessionCleanup(session); try { return await this.#serialize( session, async () => { - if (session.exited) { - const stopped = await this.#stopDebuggerOnly(session); - if (stopped.status === "incomplete") - return err( - this.#retainCleanupFailure( - session, - "close_gdb_session", - stopped.reason, - ), - ); - this.#sessions.delete(sessionId); - return ok({ target_state: "unknown" }); - } + // An earlier close can remove this session while this close waits. + if (this.#sessions.get(sessionId) !== session) + return err(gdbCloseUnavailable()); + if (session.exited || session.cleanupStarted) + return this.#finishSessionCleanup(session); let targetState: "none_observed" | "unknown" = "unknown"; try { const groups = await this.#request( @@ -1418,6 +1416,9 @@ export class GdbSessionManager { }, ), ); + // Refusing inferior disposition above leaves console remediation + // available; from here onward only owned cleanup may be retried. + session.cleanupStarted = true; if (groupCount === 0) { session.noInferiorVerified = true; targetState = "none_observed"; @@ -1454,6 +1455,22 @@ export class GdbSessionManager { } } + async #finishSessionCleanup( + session: GdbSession, + ): Promise> { + const stopped = await this.#stopDebuggerOnly(session); + if (stopped.status === "incomplete") + return err( + this.#retainCleanupFailure( + session, + "close_gdb_session", + stopped.reason, + ), + ); + this.#sessions.delete(session.id); + return ok({ target_state: "unknown" }); + } + async retryCleanupFailures(): Promise> { for (const session of this.#sessions.values()) { if (session.cleanupFailures.size === 0) continue; @@ -1764,6 +1781,7 @@ export class GdbSessionManager { #stopDebuggerOnly( session: GdbSession, ): ReturnType { + session.cleanupStarted = true; if (session.stopAttempt !== undefined) return session.stopAttempt; const attempt = this.#stopProcess(session, "GDB") .then((stopped) => { diff --git a/src/main/shutdown.ts b/src/main/shutdown.ts index 075faacde1..039f38b8c8 100644 --- a/src/main/shutdown.ts +++ b/src/main/shutdown.ts @@ -47,8 +47,11 @@ export const createShutdown = (input: { if (shutdownAttempt !== undefined) return shutdownAttempt; const pending = cleanupOwners.filter(({ closed }) => !closed); const attempt = (async () => { - // HTTP handlers can still own provider resources after cancellation. - // A failed drain keeps every shared owner available for a later retry. + // Seal before the drain and owner closes. A tool call admitted before + // shutdown can reach session.open() after that close and would otherwise + // start a provider the completed shutdown never retires. A failed drain + // still keeps every shared owner available for a later cleanup retry. + session.beginShutdown(); await handle.prepareShutdown?.(); const results = await Promise.allSettled( pending.map(async (owner) => { diff --git a/tests/boundary/mcp/shutdown.test.ts b/tests/boundary/mcp/shutdown.test.ts index 7288ec71ba..73925a860b 100644 --- a/tests/boundary/mcp/shutdown.test.ts +++ b/tests/boundary/mcp/shutdown.test.ts @@ -280,3 +280,127 @@ it("retries failed MCP cleanup owners on a repeated signal and unregisters after 2, 2, 1, 1, ]); }); + +const idleShutdownDependencies = (): RuntimeDependencies => ({ + env: {}, + serve: () => { + throw new Error("unused fixture transport"); + }, + writeStderr: () => undefined, + setExitCode: () => undefined, + registerShutdown: () => () => undefined, +}); + +it("does not leave a provider running when a target open is queued behind shutdown", async () => { + const [target] = await createBinarySessionTargets(); + if (target === undefined) throw new Error("Missing fixture target"); + let created = 0; + let closed = 0; + let releaseHealth: (() => void) | undefined; + const session = createTestBinarySession(() => { + created += 1; + return { + execute: async (operation) => { + if (operation === "health" && created === 1) + await new Promise((resolve) => { + releaseHealth = resolve; + }); + return observed(null); + }, + close: async () => { + closed += 1; + return ok(null); + }, + }; + }); + const opening = session.open(target); + await expect.poll(() => releaseHealth).toBeTypeOf("function"); + let closeQueued = false; + const originalClose = session.close.bind(session); + session.close = (...args) => { + closeQueued = true; + return originalClose(...args); + }; + const lifecycle = createShutdown({ + handle: { close: async () => undefined }, + session, + dependencies: idleShutdownDependencies(), + serverLogger: pino({ enabled: false }), + }); + + const shuttingDown = lifecycle.shutdown(); + await expect.poll(() => closeQueued).toBe(true); + const reopening = session.open(target); + const resources = session.openBundleResources(target); + releaseHealth?.(); + + expect((await opening).ok).toBe(true); + await shuttingDown; + await expect(reopening).resolves.toMatchObject({ + ok: false, + error: { _tag: "AnalysisCancelledError", operation: "open_binary" }, + }); + await expect(resources).resolves.toMatchObject({ + ok: false, + error: { + _tag: "AnalysisCancelledError", + operation: "open_bundle_resources", + }, + }); + expect(session.activeTarget()).toBeUndefined(); + expect(session.activeBundleResourceRoot()).toBeUndefined(); + expect(created).toBe(closed); +}); + +it("rejects target acquisition after shutdown has settled", async () => { + const [target] = await createBinarySessionTargets(); + if (target === undefined) throw new Error("Missing fixture target"); + let created = 0; + const session = createTestBinarySession(() => { + created += 1; + return { + execute: async () => observed(null), + close: async () => ok(null), + }; + }); + const lifecycle = createShutdown({ + handle: { close: async () => undefined }, + session, + dependencies: idleShutdownDependencies(), + serverLogger: pino({ enabled: false }), + }); + + await lifecycle.shutdown(); + await expect(session.open(target)).resolves.toMatchObject({ + ok: false, + error: { _tag: "AnalysisCancelledError", operation: "open_binary" }, + }); + await expect(session.openBundleResources(target)).resolves.toMatchObject({ + ok: false, + error: { + _tag: "AnalysisCancelledError", + operation: "open_bundle_resources", + }, + }); + expect(created).toBe(0); + expect(session.activeTarget()).toBeUndefined(); +}); + +it("opens a target again after an ordinary close", async () => { + const [target] = await createBinarySessionTargets(); + if (target === undefined) throw new Error("Missing fixture target"); + let created = 0; + const session = createTestBinarySession(() => { + created += 1; + return { + execute: async () => observed(null), + close: async () => ok(null), + }; + }); + + expect((await session.open(target)).ok).toBe(true); + expect((await session.close()).ok).toBe(true); + expect((await session.open(target)).ok).toBe(true); + expect(created).toBe(2); + expect(session.activeTarget()?.path).toBe(target); +}); diff --git a/tests/boundary/providers/GdbSessionManagerCoverage.test.ts b/tests/boundary/providers/GdbSessionManagerCoverage.test.ts index cdc6480246..5e010c0cb9 100644 --- a/tests/boundary/providers/GdbSessionManagerCoverage.test.ts +++ b/tests/boundary/providers/GdbSessionManagerCoverage.test.ts @@ -614,6 +614,231 @@ it("drains active GDB commands and rejects new commands before closeAll cleanup" expect(fake.children[0]?.killSignals).toEqual(["SIGTERM"]); }); +it("rejects GDB commands queued before closeAll when their turn arrives", async () => { + let heldToken = ""; + const commands: string[] = []; + const fake = createFakeSpawner((child, token, command) => { + commands.push(command); + if (command.includes("held command")) heldToken = token; + else + child.stdout.write( + command.includes("list-thread-groups") + ? `${token}^done,groups=[]\n` + : `${token}^done\n`, + ); + }); + const manager = managerWithFakeSpawner(fake.spawnProcess, { + stopProcess: async () => { + fake.children[0]?.exit(null, "SIGTERM"); + return { status: "stopped" }; + }, + }); + const started = await expectGdbSessionStarted(manager); + const active = manager.execute(started.value.session_id, "held command"); + await expect.poll(() => heldToken).not.toBe(""); + const queued = manager.execute(started.value.session_id, "run"); + const closing = manager.closeAll(); + fake.children[0]?.stdout.write(`${heldToken}^done\n`); + expect((await active).ok).toBe(true); + await expect(queued).resolves.toMatchObject({ + ok: false, + error: { + _tag: "AnalysisInputError", + operation: "gdb_console", + issues: [{ path: ["session_id"], reason: "invalid_value" }], + }, + }); + await closing; + expect(commands.some((command) => command.includes('"run"'))).toBe(false); +}); + +it("rejects GDB commands queued behind a session close", async () => { + let heldToken = ""; + const commands: string[] = []; + const fake = createFakeSpawner((child, token, command) => { + commands.push(command); + if (command.includes("held command")) heldToken = token; + else + child.stdout.write( + command.includes("list-thread-groups") + ? `${token}^done,groups=[]\n` + : `${token}^done\n`, + ); + }); + const manager = managerWithFakeSpawner(fake.spawnProcess, { + // The child exit event can arrive after stop reports success. + stopProcess: async () => ({ status: "stopped" }), + }); + const started = await expectGdbSessionStarted(manager); + const active = manager.execute(started.value.session_id, "held command"); + await expect.poll(() => heldToken).not.toBe(""); + const closed = manager.close(started.value.session_id); + const queued = manager.execute(started.value.session_id, "run"); + fake.children[0]?.stdout.write(`${heldToken}^done\n`); + expect((await active).ok).toBe(true); + expect((await closed).ok).toBe(true); + await expect(queued).resolves.toMatchObject({ + ok: false, + error: { + _tag: "AnalysisInputError", + operation: "gdb_console", + issues: [{ path: ["session_id"], reason: "invalid_value" }], + }, + }); + expect(commands.some((command) => command.includes('"run"'))).toBe(false); +}); + +it.each(["close", "retryCleanupFailures"] as const)( + "blocks GDB commands during failed cleanup and retries through %s", + async (retry) => { + let heldToken = ""; + const commands: string[] = []; + const fake = createFakeSpawner((child, token, command) => { + commands.push(command); + if (command.includes("held command")) heldToken = token; + else + child.stdout.write( + command.includes("list-thread-groups") + ? `${token}^done,groups=[]\n` + : `${token}^done\n`, + ); + }); + let finishStop!: () => void; + const stopHeld = new Promise((resolve) => { + finishStop = resolve; + }); + let stopCalls = 0; + const manager = managerWithFakeSpawner(fake.spawnProcess, { + stopProcess: async () => { + stopCalls += 1; + if (stopCalls === 1) { + await stopHeld; + return { + status: "incomplete", + reason: "simulated cleanup uncertainty", + }; + } + return { status: "stopped" }; + }, + }); + const started = await expectGdbSessionStarted(manager); + const id = started.value.session_id; + const active = manager.execute(id, "held command"); + await expect.poll(() => heldToken).not.toBe(""); + const closing = manager.close(id); + const queued = manager.execute(id, "run"); + fake.children[0]?.stdout.write(`${heldToken}^done\n`); + expect((await active).ok).toBe(true); + await expect.poll(() => stopCalls).toBe(1); + const unavailable = { + ok: false, + error: { + _tag: "AnalysisInputError", + operation: "gdb_console", + issues: [{ path: ["session_id"], reason: "invalid_value" }], + }, + }; + // Finish the barrier before assertions so a regression cannot strand teardown. + const duringStop = manager.execute(id, "run"); + finishStop(); + const closed = await closing; + expect(closed).toMatchObject({ + ok: false, + error: { cleanupIncomplete: true }, + }); + await expect(queued).resolves.toMatchObject(unavailable); + await expect(duringStop).resolves.toMatchObject(unavailable); + await expect(manager.execute(id, "run")).resolves.toMatchObject( + unavailable, + ); + expect(manager.status(id)?.state).toBe("closed"); + const commandsBeforeRetry = [...commands]; + expect( + ( + await (retry === "close" + ? manager.close(id) + : manager.retryCleanupFailures()) + ).ok, + ).toBe(true); + expect(stopCalls).toBe(2); + expect(commands).toEqual(commandsBeforeRetry); + expect(commands.some((command) => command.includes('"run"'))).toBe(false); + expect(manager.status(id)).toBeUndefined(); + if (!closed.ok) expect(closed.error.cleanupIncomplete).toBe(false); + }, +); + +it("allows explicit inferior remediation when close refuses before destructive cleanup", async () => { + let detached = false; + let stopCalls = 0; + const fake = createFakeSpawner((child, token, command) => { + if (command.includes('"detach"')) detached = true; + child.stdout.write( + command.includes("list-thread-groups") + ? `${token}^done,groups=${detached ? "[]" : '[{id="i1",pid="123"}]'}\n` + : `${token}^done\n`, + ); + }); + const manager = managerWithFakeSpawner(fake.spawnProcess, { + stopProcess: async () => { + stopCalls += 1; + return { status: "stopped" }; + }, + }); + const started = await expectGdbSessionStarted(manager); + const id = started.value.session_id; + await expect(manager.close(id)).resolves.toMatchObject({ + ok: false, + error: { _tag: "AnalysisCapabilityUnavailableError", retryable: true }, + }); + expect(stopCalls).toBe(0); + expect(manager.status(id)?.state).toBe("ready"); + expect((await manager.execute(id, "detach")).ok).toBe(true); + expect((await manager.close(id)).ok).toBe(true); + expect(stopCalls).toBe(1); +}); + +it("rejects a duplicate GDB close queued behind a session close", async () => { + let heldToken = ""; + const commands: string[] = []; + let stopCalls = 0; + const fake = createFakeSpawner((child, token, command) => { + commands.push(command); + if (command.includes("held command")) heldToken = token; + else + child.stdout.write( + command.includes("list-thread-groups") + ? `${token}^done,groups=[]\n` + : `${token}^done\n`, + ); + }); + const manager = managerWithFakeSpawner(fake.spawnProcess, { + stopProcess: async () => { + stopCalls += 1; + return { status: "stopped" }; + }, + }); + const started = await expectGdbSessionStarted(manager); + const active = manager.execute(started.value.session_id, "held command"); + await expect.poll(() => heldToken).not.toBe(""); + const closed = manager.close(started.value.session_id); + const duplicate = manager.close(started.value.session_id); + fake.children[0]?.stdout.write(`${heldToken}^done\n`); + expect((await active).ok).toBe(true); + await expect(closed).resolves.toMatchObject({ + ok: true, + value: { target_state: "none_observed" }, + }); + await expect(duplicate).resolves.toMatchObject({ + ok: false, + error: { _tag: "AnalysisInputError", operation: "close_gdb_session" }, + }); + expect( + commands.filter((command) => command.includes("-gdb-exit")), + ).toHaveLength(1); + expect(stopCalls).toBe(1); +}); + it("enforces the per-session queue capacity while a GDB command is active", async () => { let heldToken = ""; const fake = createFakeSpawner((child, token, command) => { @@ -2500,7 +2725,7 @@ it("retains ownership without signaling when detach confirmation fails", async ( await expect(manager.closeAll()).rejects.toThrow(/did not stop cleanly/i); - expect(manager.status(started.value.session_id)?.state).toBe("ready"); + expect(manager.status(started.value.session_id)?.state).toBe("closed"); expect(fake.children[0]?.killSignals).toEqual([]); }); @@ -2608,7 +2833,7 @@ it("retains a session when closing cannot confirm the debugger process stopped", operation: "close_gdb_session", }); } - expect(manager.status(started.value.session_id)?.state).toBe("ready"); + expect(manager.status(started.value.session_id)?.state).toBe("closed"); }); it("retains an exited GDB session when provider stop verification is incomplete", async () => {