From 5f0d18a534bbe7656a5b0981deb145bdbbb11ecb Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Fri, 16 Oct 2026 08:26:18 +0800 Subject: [PATCH] fix(ghidra): retain request limits and startup cleanup Preserve the full request options through the FIFO and settle interrupted or rejected final lineage observation through owned startup cleanup. Keep the startup diagnostic checkpoint until finalization succeeds. Validate typed expired ping, cancellation, child exit, runtime removal, retry, and MCP recovery without running a native Ghidra instance. Refs: #4289, #4274 --- src/ghidra/GhidraClient.ts | 14 ++- src/ghidra/GhidraRequestQueue.test.ts | 31 +++++++ src/ghidra/GhidraRequestQueue.ts | 24 +++-- tests/boundary/mcp/ghidraReopen.test.ts | 5 +- .../providers/ghidra/ghidraClient.test.ts | 88 +++++++++++++++++++ 5 files changed, 144 insertions(+), 18 deletions(-) diff --git a/src/ghidra/GhidraClient.ts b/src/ghidra/GhidraClient.ts index 66b09caae9..214ed73a8b 100644 --- a/src/ghidra/GhidraClient.ts +++ b/src/ghidra/GhidraClient.ts @@ -731,8 +731,20 @@ export class GhidraClient { this.#startupOutputCheckpoint = this.#process.checkpointOutput(); const completed = await this.#connectAndHandshake(endpoint, deadline); if (!completed.ok) return this.#startupFailure(completed.error, deadline); + try { + await this.#lineage.observe(this.#launch); + } catch (cause: unknown) { + return this.#startupFailure( + this.#failure( + "start", + "Ghidra process lineage observation failed", + cause, + ), + deadline, + ); + } + if (deadline.signal.aborted) return this.#startupInterrupted(deadline); this.#startupOutputCheckpoint = undefined; - await this.#lineage.observe(this.#launch); this.#requestQueue.reopen(); return completed; } diff --git a/src/ghidra/GhidraRequestQueue.test.ts b/src/ghidra/GhidraRequestQueue.test.ts index c529295e7b..1be5007e23 100644 --- a/src/ghidra/GhidraRequestQueue.test.ts +++ b/src/ghidra/GhidraRequestQueue.test.ts @@ -10,6 +10,37 @@ const failure = bindGhidraSessionFailure(() => ({ })); describe("Ghidra request queue failure settlement", () => { + it("forwards the request deadline to the executor", async () => { + const optionsSeen: { timeoutMs?: number }[] = []; + const queue = new GhidraRequestQueue( + (_method, _parameters, options) => { + optionsSeen.push(options); + return Promise.resolve( + options.timeoutMs === 0 + ? err( + failure( + "timeout", + "Ghidra request deadline elapsed", + undefined, + { timeoutMs: 0 }, + ), + ) + : ok(null), + ); + }, + failure, + () => Promise.resolve(ok(null)), + ); + + await expect( + queue.run("ping", {}, { timeoutMs: 0 }), + ).resolves.toMatchObject({ + ok: false, + error: { kind: "timeout", timeoutMs: 0 }, + }); + expect(optionsSeen).toEqual([{ timeoutMs: 0 }]); + }); + it.each([new TypeError("Decoder rejected"), "decoder rejected", null])( "preserves a rejected executor cause (%j) and drains the next request", async (cause) => { diff --git a/src/ghidra/GhidraRequestQueue.ts b/src/ghidra/GhidraRequestQueue.ts index e3072dab27..0b03d448ae 100644 --- a/src/ghidra/GhidraRequestQueue.ts +++ b/src/ghidra/GhidraRequestQueue.ts @@ -30,7 +30,7 @@ export type GhidraActiveCancellationHandler = () => Promise< interface QueuedRequest { readonly method: string; readonly parameters: JsonValue; - readonly signal: AbortSignal | undefined; + readonly options: GhidraRequestOptions; readonly resolve: (result: GhidraQueuedRequestResult) => void; readonly onAbort: (() => void) | undefined; } @@ -72,7 +72,7 @@ export class GhidraRequestQueue { entry = { method, parameters, - signal: options.signal, + options, resolve, onAbort, }; @@ -102,7 +102,7 @@ export class GhidraRequestQueue { const entry = this.#queue.shift(); if (entry === undefined) return; this.#release(entry); - if (entry.signal?.aborted === true) { + if (entry.options.signal?.aborted === true) { entry.resolve( err(this.failure("cancelled", "Ghidra request was cancelled")), ); @@ -113,7 +113,7 @@ export class GhidraRequestQueue { let executionSettled = false; let activeCancellation: Promise> | undefined; const onAbort = - entry.signal === undefined + entry.options.signal === undefined ? undefined : () => { if (executionSettled) return; @@ -126,12 +126,8 @@ export class GhidraRequestQueue { activeCancellation = this.onActiveCancellation(); }; if (onAbort !== undefined) - entry.signal?.addEventListener("abort", onAbort, { once: true }); - void this.execute( - entry.method, - entry.parameters, - entry.signal === undefined ? {} : { signal: entry.signal }, - ) + entry.options.signal?.addEventListener("abort", onAbort, { once: true }); + void this.execute(entry.method, entry.parameters, entry.options) .then(async (result) => { if (activeCancellation !== undefined) { const primary = result.ok @@ -161,8 +157,8 @@ export class GhidraRequestQueue { ); }) .finally(() => { - if (entry.signal !== undefined && onAbort !== undefined) - entry.signal.removeEventListener("abort", onAbort); + if (entry.options.signal !== undefined && onAbort !== undefined) + entry.options.signal.removeEventListener("abort", onAbort); this.#active = false; if (this.#closedFailure === undefined) this.#drain(); }); @@ -223,7 +219,7 @@ export class GhidraRequestQueue { } #release(entry: QueuedRequest): void { - if (entry.signal !== undefined && entry.onAbort !== undefined) - entry.signal.removeEventListener("abort", entry.onAbort); + if (entry.options.signal !== undefined && entry.onAbort !== undefined) + entry.options.signal.removeEventListener("abort", entry.onAbort); } } diff --git a/tests/boundary/mcp/ghidraReopen.test.ts b/tests/boundary/mcp/ghidraReopen.test.ts index 7af55924af..a69b41d65a 100644 --- a/tests/boundary/mcp/ghidraReopen.test.ts +++ b/tests/boundary/mcp/ghidraReopen.test.ts @@ -248,9 +248,8 @@ const disconnect = async ( child.on("message", received); child.send("disconnect"); }); - await expect - .poll(async () => (await client.ping({ timeoutMs: 200 })).ok) - .toBe(false); + const disconnectedPing = await client.ping({ timeoutMs: 200 }); + expect(disconnectedPing.ok).toBe(false); expect(child.exitCode).toBeNull(); expect(child.signalCode).toBeNull(); }; diff --git a/tests/boundary/providers/ghidra/ghidraClient.test.ts b/tests/boundary/providers/ghidra/ghidraClient.test.ts index 452c122f7a..5cc3d20756 100644 --- a/tests/boundary/providers/ghidra/ghidraClient.test.ts +++ b/tests/boundary/providers/ghidra/ghidraClient.test.ts @@ -14,6 +14,7 @@ import type { ProgressUpdate } from "../../../../src/application/ProgressReporte import { ok } from "../../../../src/domain/result.js"; import { GhidraClient } from "../../../../src/ghidra/GhidraClient.js"; +import { ProviderRunLineage } from "../../../../src/process/ProviderRunLineage.js"; import { OwnedFileHandle } from "../../../../src/filesystem/OwnedFileHandle.js"; import { GHIDRA_PROCESS_DIAGNOSTIC_BYTES } from "../../../../src/ghidra/GhidraDiagnostics.js"; import { GhidraHeadlessLauncher } from "../../../../src/ghidra/GhidraLauncher.js"; @@ -561,6 +562,83 @@ describe("GhidraClient", () => { }); describe("GhidraClient startup lifecycle", () => { + it("retains startup failure and cleans up when lineage observation rejects", async () => { + const cause = new TypeError("Lineage reader rejected"); + const observe = vi.spyOn(ProviderRunLineage.prototype, "observe"); + onTestFinished(() => observe.mockRestore()); + observe.mockRejectedValueOnce(cause); + const launcher = new FixtureLauncher(); + const client = clientFor(launcher); + + const result = await client.start(); + expect(result).toMatchObject({ + ok: false, + error: { + kind: "start", + message: "Ghidra process lineage observation failed", + }, + }); + if (result.ok) throw new Error("Expected lineage observation failure"); + expect(result.error.cause).toBe(cause); + const child = launcher.processes[0]; + if (child === undefined) throw new Error("fixture child was not launched"); + await expect(waitForChildExit(child, 5_000)).resolves.toBe(true); + await expect(access(launcher.runtimeRoots[0] ?? "")).rejects.toMatchObject({ + code: "ENOENT", + }); + expect(client.isReady()).toBe(false); + await expect(client.start()).resolves.toMatchObject({ ok: true }); + expect(launcher.processes).toHaveLength(2); + }); + + it("cleans up when the final waiter cancels during lineage observation", async () => { + let markObservationPending: (() => void) | undefined; + const observationPending = new Promise((resolve) => { + markObservationPending = resolve; + }); + let releaseObservation: (() => void) | undefined; + const observationGate = new Promise((resolve) => { + releaseObservation = resolve; + }); + const originalObserve = ProviderRunLineage.prototype.observe; + const observe = vi.spyOn(ProviderRunLineage.prototype, "observe"); + onTestFinished(() => observe.mockRestore()); + observe.mockImplementationOnce(async function ( + this: ProviderRunLineage, + launch, + ) { + markObservationPending?.(); + await observationGate; + return await originalObserve.call(this, launch); + }); + + const launcher = new FixtureLauncher(); + const client = clientFor(launcher); + const controller = new AbortController(); + const starting = client.start(controller.signal); + await observationPending; + const child = launcher.processes[0]; + if (child === undefined) throw new Error("fixture child was not launched"); + + controller.abort(); + releaseObservation?.(); + + await expect(starting).resolves.toMatchObject({ + ok: false, + error: { kind: "cancelled" }, + }); + await expect(waitForChildExit(child, 5_000)).resolves.toBe(true); + await expect(access(launcher.runtimeRoots[0] ?? "")).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(client.callToolIfReady("list_strings", {})).resolves.toBe( + undefined, + ); + + await expect(client.start()).resolves.toMatchObject({ ok: true }); + expect(launcher.processes).toHaveLength(2); + }); + it("applies one startup deadline and removes the private runtime", async () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }); const launcher = new FixtureLauncher("silent"); @@ -603,6 +681,16 @@ describe("GhidraClient startup lifecycle", () => { }); describe("GhidraClient established requests", () => { + it("preserves an expired ping deadline through the request queue", async () => { + const client = clientFor(new FixtureLauncher()); + await expect(client.start()).resolves.toMatchObject({ ok: true }); + + await expect(client.ping({ timeoutMs: 0 })).resolves.toMatchObject({ + ok: false, + error: { kind: "timeout", timeoutMs: 0 }, + }); + }); + it("fails an established function request when the provider process exits", async () => { const client = clientFor(new FixtureLauncher("exit_tools")); await expect(client.start()).resolves.toMatchObject({ ok: true });