From 7369b5ee63c69af8af18601bb1de92dd41c6a250 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:27:26 +0800 Subject: [PATCH] fix(ida): retain cleanup facts and let one-shot commands exit (#2814) * fix(ida): let one-shot commands exit after retaining cleanup When a headless IDA request outlives timeoutMs (for example idb_open on a large binary still running auto-analysis), REA cannot prove the worker stopped and correctly retains the upstream connection and private workspace. The retained stdio child and its pipes stayed referenced, so a one-shot CLI printed its cleanup_incomplete result and then never exited, keeping the supervisor, worker and database alive. Detach the retained connection's child process and pipes from the event loop when cleanup fails. A long-lived MCP server can still retry through the retained connection; a one-shot CLI exits with status 1, and the supervisor observes end of input. Co-Authored-By: Claude Opus 5.5 * fix(ida): report an unverified upstream worker after failed cleanup Upstream ida-pro-mcp spawns headless workers in their own session and deliberately leaves them running when its supervisor exits, so neither a retained connection nor REA exiting proves that a timed-out worker stopped. When cleanup fails before the owned worker's release is confirmed, the cleanup diagnostics now name its private session and state that it may still be running until it finishes and reaches its idle timeout. Failures after a confirmed release, or before any open was attempted, do not report a worker. Co-Authored-By: Claude Opus 5.5 * fix(ida): name the inventory sessions that block worker release When idb_close confirmed the requested worker gone but the follow-up inventory still found a different peer on the private input, the upstream_worker diagnostic named the released session and could misdirect manual cleanup. Record the session ids of the inventory entries that actually blocked release and report those; fall back to the requested private session only when no inventory evidence exists (an incomplete open or an unanswered request). The field becomes session_ids, since more than one entry can block release. Co-Authored-By: Claude Opus 5.5 * fix(ida): keep blocker evidence and backend in cleanup diagnostics The release diagnostic reset its observed blockers on every close, so a retry whose inventory was unavailable fell back to the already-released session; and it kept only session ids, describing a blocking GUI session with detached-worker guidance. Blocker evidence is now replaced only by a successful inventory, and the diagnostic becomes upstream_sessions with each session's id and backend, with guidance that distinguishes workers from GUI sessions. Co-Authored-By: Claude Opus 5.5 * fix(ida): stop naming a worker after its close was confirmed When idb_close confirmed the requested worker's release but the following inventory failed, no blocker evidence existed yet and the diagnostic fell back to the confirmed-absent worker. After a confirmed close the diagnostic now reports an empty session list, meaning any remaining same-input sessions are unknown. Co-Authored-By: Claude Opus 5.5 * test(ida): group cleanup diagnostic cases in their own suite The added cases pushed the reaped-worker describe block past the 150-line limit enforced by lint:built. Co-Authored-By: Claude Opus 5.5 * fix(ida): keep ownership and pre-close peers in cleanup diagnostics Blocking sessions dropped the parsed ownership fact, so an external unowned worker looked the same as REA's own; and recovery inventory recorded same-input peers only when the requested worker was missing, so a peer seen beside it was lost if the following close failed. Each reported session now carries owned (null when no inventory has reported it), and recovery records every same-input session until a later successful inventory replaces that evidence. Co-Authored-By: Claude Opus 5.5 * fix(ida): drop a confirmed worker from retained cleanup blockers Recovery inventory now records the requested worker beside any peers, so once idb_close confirmed its release a failing follow-up inventory still reported it as unreleased. Remove the confirmed session from the retained blockers at close confirmation and keep the remaining peers. Co-Authored-By: Claude Opus 5.5 * test(ida): retry cleanup through a detached stdio connection The retry coverage used a recording connection whose detach() only counts calls, so it could not show that detaching keeps a retained connection usable. Run a real stdio supervisor in a long-lived host, fail the first post-close inventory, and require the retry through the same detached connection to confirm release and remove the workspace. The test fails if detach() destroys the pipes instead of unref-ing them. Co-Authored-By: Claude Opus 5.5 * fix(ida): close the stdio supervisor in failed retry tests Merge main and give its new IdaNativeProtocol connection double the required detach() method. The real-stdio retry test now closes its connection in an unconditional teardown, so a failing assertion cannot leave the supervisor waiting on stdin. Co-Authored-By: Claude Opus 5.5 * test(ida): share the cleanup diagnostic expectation in session tests Repeated upstream_sessions assertions and the unrelated live peer literal pushed repository duplication over the jscpd threshold. Build the expected diagnostic with one helper and reuse one peer constant. Co-Authored-By: Claude Opus 5.5 * test(ida): distinguish observed ownership from unreported input bindings * fix(ida): keep requested-ID conflicts in recovery cleanup Recovery inventory kept only same-input entries, unlike the startup and post-close filters. After an explicitly rejected open, an entry under the requested session ID on another input was dropped, so cleanup could close the connection, remove the private workspace and report success while that session stayed registered. Match the requested ID as well so the entry is retained as a blocker and cleanup stays incomplete. Also split the cleanup diagnostics suite under the lint:built function length limit, and make two startup-inventory fixtures pass a defined session id so the test file typechecks. Co-Authored-By: Claude Opus 5.5 * fix(ida): preserve retained session inventory facts --------- Co-authored-by: Xingchuan Ma Co-authored-by: Claude Opus 5.5 Co-authored-by: morluto --- src/ida/IdaSessionClient.test.ts | 57 +++++++++++++++++++ src/ida/IdaSessionClient.ts | 32 +++++++---- .../cli/idaRetainedCleanupCli.test.ts | 1 + 3 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/ida/IdaSessionClient.test.ts b/src/ida/IdaSessionClient.test.ts index 4de487689..d126cfbfd 100644 --- a/src/ida/IdaSessionClient.test.ts +++ b/src/ida/IdaSessionClient.test.ts @@ -1077,6 +1077,7 @@ describe("headless IDA cleanup diagnostics", () => { backend: "worker", owned: false, input_path: producer.inputPath, + is_active: false, }, ), ); @@ -1368,6 +1369,62 @@ describe("headless IDA cleanup verification", () => { }); }); +it("retains a conflicting requested session ID during pre-close recovery", async () => { + const { producer, client, root } = await fixture("headless"); + producer.overrideCall = (name) => + name === "idb_list" + ? { + sessions: [ + { + session_id: String(producer.database), + input_path: "/conflicting/input", + owned: true, + backend: "worker", + is_active: false, + }, + ], + } + : undefined; + expect((await client.execute("health", {})).ok).toBe(false); + const requested = String(producer.database); + const result = await client.close(); + expect(result).toMatchObject( + reportsSessions({ + session_id: requested, + input_path: "/conflicting/input", + owned: true, + backend: "worker", + is_active: false, + }), + ); + expect(producer.calls.some(({ name }) => name === "idb_close")).toBe(false); + expect( + (await readdir(root)).some((name) => name.startsWith("rea-ida-")), + ).toBe(true); +}); + +it.each(["unavailable", "malformed"] as const)( + "does not invent a worker after explicit rejection and %s inventory", + async (kind) => { + const { producer, client } = await fixture("headless"); + producer.overrideCall = (name) => + name === "idb_open" + ? { error: "rejected before worker creation" } + : name === "idb_list" && kind === "malformed" + ? {} + : undefined; + expect((await client.execute("health", {})).ok).toBe(false); + producer.beforeCall = async (name) => { + if (name === "idb_list" && kind === "unavailable") + throw new Error("inventory unavailable"); + }; + const result = await client.close(); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("Expected retained cleanup"); + expect(result.error).not.toHaveProperty("diagnostics.upstream_sessions"); + }, +); + it.each(["idb_close", "idb_list", "connection_close"])( "preserves transport stderr through cleanup at %s", async (stage) => { diff --git a/src/ida/IdaSessionClient.ts b/src/ida/IdaSessionClient.ts index cff968132..d157b5209 100644 --- a/src/ida/IdaSessionClient.ts +++ b/src/ida/IdaSessionClient.ts @@ -98,6 +98,8 @@ interface UpstreamSession { readonly owned: boolean | null; /** Last inventoried input binding; null until upstream reports it. */ readonly input_path: string | null; + /** Last inventoried activity; null before upstream reports it. */ + readonly is_active: boolean | null; } /** A failed cleanup retains its private workspace and reports the remaining resource. */ @@ -130,15 +132,16 @@ class IdaCleanupError extends ProviderAdapterError { upstream_sessions: { state: "unverified", sessions: sessions.map( - ({ session_id, backend, owned, input_path }) => ({ + ({ session_id, backend, owned, input_path, is_active }) => ({ session_id, backend, owned, input_path, + is_active, }), ), reason: - "Release of upstream sessions matching the requested session ID or private input was not confirmed; input_path preserves the last observed binding and is null when no inventory has reported it. A worker outlives its supervisor and may keep running until it finishes and reaches its idle timeout; exiting REA does not stop it. A gui session belongs to an IDA instance REA does not control. An empty session_id means upstream reported none; owned is null when no inventory has reported ownership; an empty list means the requested worker's close was confirmed but no inventory has identified any remaining session.", + "Release of upstream sessions matching the requested session ID or private input was not confirmed; input_path preserves the last observed binding and is null when no inventory has reported it. is_active preserves registry activity and is null when unknown; it does not establish process liveness. A worker outlives its supervisor and may keep running until it finishes and reaches its idle timeout; exiting REA does not stop it. A gui session belongs to an IDA instance REA does not control. An empty session_id means upstream reported none; owned is null when no inventory has reported ownership; an empty list means the requested worker's close was confirmed but no inventory has identified any remaining session.", }, }), }, @@ -691,7 +694,12 @@ export class IdaSessionClient implements AnalysisClient { */ #unreleasedSessions(): readonly UpstreamSession[] | undefined { if (this.#blockingSessions !== undefined) return this.#blockingSessions; - if (!this.#openAttempted || this.#workspace === undefined) return undefined; + if ( + !this.#openAttempted || + this.#workspace === undefined || + this.#openRejected + ) + return undefined; return this.#databaseCloseCompleted ? [] : [ @@ -700,6 +708,7 @@ export class IdaSessionClient implements AnalysisClient { backend: "worker", owned: null, input_path: null, + is_active: null, }, ]; } @@ -719,12 +728,15 @@ export class IdaSessionClient implements AnalysisClient { ): boolean { this.#blockingSessions = entries.length > 0 - ? entries.map(({ session_id, backend, owned, input_path }) => ({ - session_id, - backend, - owned, - input_path, - })) + ? entries.map( + ({ session_id, backend, owned, input_path, is_active }) => ({ + session_id, + backend, + owned, + input_path, + is_active, + }), + ) : undefined; return entries.length > 0; } @@ -781,7 +793,7 @@ export class IdaSessionClient implements AnalysisClient { ); if (this.#database === undefined && blocked) throw new AnalysisProtocolError( - "An unowned worker still uses the private workspace; it was retained.", + "A registered session still matches the requested ID or private input without a verified private worker binding; the workspace was retained.", ); if ( this.#database === undefined && diff --git a/tests/boundary/cli/idaRetainedCleanupCli.test.ts b/tests/boundary/cli/idaRetainedCleanupCli.test.ts index b31903489..826bb7f1d 100644 --- a/tests/boundary/cli/idaRetainedCleanupCli.test.ts +++ b/tests/boundary/cli/idaRetainedCleanupCli.test.ts @@ -79,6 +79,7 @@ describe("headless IDA timeout through the one-shot CLI", () => { backend: "worker", owned: null, input_path: null, + is_active: null, }, ], },