morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #1841] [MERGED] fix(ida): let one-shot commands exit after retaining cleanup #4851

New issue

Closed

opened 2026-10-31 00:56:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:56:02 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1841
Author: @AS9929
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/ida-one-shot-exit-after-retained-cleanup

📝 Commits (10+)

5fd536c fix(ida): let one-shot commands exit after retaining cleanup
66a6ed8 fix(ida): report an unverified upstream worker after failed cleanup
256f4c7 Merge branch 'main' of https://github.com/morluto/rea into fix/ida-one-shot-exit-after-retained-cleanup
cc54f30 fix(ida): name the inventory sessions that block worker release
1a26388 fix(ida): keep blocker evidence and backend in cleanup diagnostics
b354111 fix(ida): stop naming a worker after its close was confirmed
d782961 test(ida): group cleanup diagnostic cases in their own suite
9ac858d fix(ida): keep ownership and pre-close peers in cleanup diagnostics
bed0f46 fix(ida): drop a confirmed worker from retained cleanup blockers
e84cd8f test(ida): retry cleanup through a detached stdio connection

📊 Changes

9 files changed (+596 additions, -17 deletions)

View changed files

📝 src/ida/IdaBusyHealth.test.ts (+1 -0)
📝 src/ida/IdaMcpConnection.ts (+17 -0)
📝 src/ida/IdaNativeProtocol.test.ts (+1 -0)
📝 src/ida/IdaSessionClient.test.ts (+251 -0)
📝 src/ida/IdaSessionClient.ts (+110 -17)
📝 src/ida/IdaWorkspaceOwnership.test.ts (+6 -0)
➕ tests/boundary/cli/idaRetainedCleanupCli.test.ts (+92 -0)
➕ tests/boundary/providers/ida/retainedConnectionRetry.test.ts (+114 -0)
📝 tests/fixtures/idaMcp.ts (+4 -0)

📄 Description

Summary

A one-shot CLI command using the headless IDA provider never exits after a request outlives timeoutMs. REA prints the cleanup_incomplete result and then hangs indefinitely, keeping the upstream supervisor, its worker and the private database alive. This is the IDA counterpart of #1751 / #1763 (Hopper). No issue is linked; it was found while analyzing a large binary on a real IDA installation.

Problem and expected behavior

Trigger: any headless IDA request that does not complete within timeoutMs. The realistic case is idb_open on a large binary that is still running auto-analysis past the default 300 s, e.g. a 119 MB universal Mach-O:

REA_ANALYSIS_PROVIDER=ida REA_IDA_MCP_CONFIG=headless.json rea decompile /Applications/iTerm.app/Contents/MacOS/iTerm2 _main --json

REA correctly cannot prove the worker stopped, so it retains the upstream connection and private workspace (Worker inventory cannot prove that the request has stopped; the private workspace was retained.). But the retained StdioClientTransport child process and its pipes stay referenced, so the Node event loop never drains. Observed: the CLI wrote its final JSON and was still running 15 h later, with the supervisor child and a 650 MB .i64 held open.

Expected: like Hopper after #1763, the CLI exits with status 1 and the same structured cleanup_incomplete result; a long-lived MCP server keeps the retained connection for a later close retry. The result should also say that the upstream worker may still be running, because nothing in REA can currently prove otherwise.

Change and scope

IdaMcpConnection gains detach(): for a stdio registration it unrefs the child process and its stdin/stdout/stderr sockets. The SDK exposes only the child pid, so the handle is read from the transport and checked with instanceof ChildProcess before use. HTTP registrations hold no child and are unchanged.
IdaSessionClient.close() calls detach() when cleanup fails, the point where the connection is retained.
When cleanup fails before the owned worker's release is confirmed, the cleanup diagnostics gain upstream_sessions: { state: "unverified", sessions: [{ session_id, backend, owned }], reason }. It names every same-input session the latest successful inventory reported, including peers seen beside the requested worker, and keeps them across retries whose inventory is unavailable. Without inventory evidence it names the requested private worker with owned: null; after a confirmed close it reports an empty list, meaning the remaining identities are unknown. Upstream spawns workers in their own session and shutdown() deliberately leaves them running, so supervisor EOF does not stop a busy worker (thanks @dandgabr). Failures after a confirmed release, or before any open was attempted, do not report a worker.

Nothing is closed or killed, ownership is unchanged, and the retained workspace is still reported.

Worker lifetime is not changed by this PR. A one-shot CLI never retries close, so on main the hang does not advance cleanup either. Proving that the worker stopped needs an upstream cancellation or release receipt (the constraint discussed in #1710).

Not included: #1710 (recovery after the supervisor reaps a worker) and #1769 / #1785 (missing procedure reporting).

Contract and boundary impact

Semantic owner and earliest changed stage: src/ida/IdaMcpConnection.ts (transport handles), invoked from IdaSessionClient.close() on failure.
CLI and MCP/tool-catalog contract: none; the CLI now exits with the result it already printed.
Provider, bridge, target-format, or platform compatibility: headless and attached stdio IDA registrations; HTTP unchanged.
Evidence, artifact, provenance, or reconstruction contract: none.
Process execution, authorization, cleanup, or containment impact: retained resources no longer hold a one-shot process open; MCP retry behavior is unchanged; an unconfirmed worker release is now reported explicitly instead of implied.
Generated metadata, package, or installation impact: none.

Evidence and regression coverage

Tests added or updated:
- tests/boundary/cli/idaRetainedCleanupCli.test.ts runs the compiled CLI against a stdio IDA registration whose supervisor completes the handshake and never answers a tool call (timeoutMs: 500). It requires exit 1 with cleanup_incomplete and a retained workspace under the configured workspaceRoot.
- The CLI test also requires cleanup_error.details.diagnostics.upstream_sessions with state: "unverified" and the private rea- worker session.
- src/ida/IdaWorkspaceOwnership.test.ts asserts detach() runs once when cleanup fails and not again when the retry succeeds, and that no upstream_sessions is reported when no open was attempted (this assertion fails if the diagnostic is emitted unconditionally).
Base reproduction: the new CLI test times out at 30 s on origin/main (0b4ce836) with the source fix reverted, and passes in ~2 s with it.
User-visible CLI output: unchanged cleanup_incomplete JSON; exit status 1 instead of hanging.
Remaining proof gaps: no Windows or Linux run (the test is not platform-gated, so CI covers them); HTTP registrations not exercised.

Validation performed

npm run test:focused -- src/ida/*.test.ts tests/boundary/cli/idaRetainedCleanupCli.test.ts: 9 files, 54 tests passed
Same CLI test with only the src/ida source changes reverted: Test timed out in 30000ms
npm run check: passed (typecheck, lint, format:check, knip, metadata:check, architecture guards); no new lint warnings in touched files
npm run verify:test-discovery: all 837 test files discovered once
Real provider, macOS 15 arm64, IDA Pro 9.4.260610, ida-pro-mcp c133c385, headless idalib-mcp --stdio, registration timeoutMs: 1500 on a small arm64 binary. Before: final JSON printed, then killed by a 60 s alarm (exit 142). After: exit 1 in about 5 s with cleanup_incomplete and upstream_sessions.state: "unverified".
Worker lifetime, real provider, 119 MB universal Mach-O (iTerm2), timeoutMs: 20000, with rea-agents@6.4.0 and this branch run side by side for 12 minutes. In both runs the worker analyzed at ~100% CPU for ~5.5 minutes and then stayed idle until stopped manually. 6.4.0's CLI was still running at 12 minutes; this branch's CLI exited after ~60 s. The worker behaved the same in both.

Compatibility, safety, and release

Breaking changes or migration steps: none. IdaMcpConnection is internal; its test doubles implement the new no-op method.
Real Hopper/Ghidra, browser, or OS coverage: real IDA on macOS arm64 only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no process is killed or detached from its parent; only event-loop references change. A retained connection in a live MCP server keeps working.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1841 **Author:** [@AS9929](https://github.com/AS9929) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/ida-one-shot-exit-after-retained-cleanup` --- ### 📝 Commits (10+) - [`5fd536c`](https://github.com/morluto/rea/commit/5fd536c09efbdc5f9d26eece177b6acf12ad5018) fix(ida): let one-shot commands exit after retaining cleanup - [`66a6ed8`](https://github.com/morluto/rea/commit/66a6ed84d370eb2ba5c6c3024b1d8c39383386bf) fix(ida): report an unverified upstream worker after failed cleanup - [`256f4c7`](https://github.com/morluto/rea/commit/256f4c7d0b3aa811df9e93b484b52f18c65e0904) Merge branch 'main' of https://github.com/morluto/rea into fix/ida-one-shot-exit-after-retained-cleanup - [`cc54f30`](https://github.com/morluto/rea/commit/cc54f30c103fa5a93be064fa1c7119f08ab9c6ee) fix(ida): name the inventory sessions that block worker release - [`1a26388`](https://github.com/morluto/rea/commit/1a263883c150d344236def31004a5b0ade85bfc2) fix(ida): keep blocker evidence and backend in cleanup diagnostics - [`b354111`](https://github.com/morluto/rea/commit/b3541115404e352cf6592bb9dfd7c303c08946fc) fix(ida): stop naming a worker after its close was confirmed - [`d782961`](https://github.com/morluto/rea/commit/d7829613ac48ae10b61650b81e0a4707fc8863d4) test(ida): group cleanup diagnostic cases in their own suite - [`9ac858d`](https://github.com/morluto/rea/commit/9ac858df044f414aea142ef00ffcc5321e119cf5) fix(ida): keep ownership and pre-close peers in cleanup diagnostics - [`bed0f46`](https://github.com/morluto/rea/commit/bed0f46ea846a9ae774f661d875a61dcaf1b6fcd) fix(ida): drop a confirmed worker from retained cleanup blockers - [`e84cd8f`](https://github.com/morluto/rea/commit/e84cd8fd907e60b5fc69d53dc5db4df96b806555) test(ida): retry cleanup through a detached stdio connection ### 📊 Changes **9 files changed** (+596 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `src/ida/IdaBusyHealth.test.ts` (+1 -0) 📝 `src/ida/IdaMcpConnection.ts` (+17 -0) 📝 `src/ida/IdaNativeProtocol.test.ts` (+1 -0) 📝 `src/ida/IdaSessionClient.test.ts` (+251 -0) 📝 `src/ida/IdaSessionClient.ts` (+110 -17) 📝 `src/ida/IdaWorkspaceOwnership.test.ts` (+6 -0) ➕ `tests/boundary/cli/idaRetainedCleanupCli.test.ts` (+92 -0) ➕ `tests/boundary/providers/ida/retainedConnectionRetry.test.ts` (+114 -0) 📝 `tests/fixtures/idaMcp.ts` (+4 -0) </details> ### 📄 Description ## Summary A one-shot CLI command using the headless IDA provider never exits after a request outlives `timeoutMs`. REA prints the `cleanup_incomplete` result and then hangs indefinitely, keeping the upstream supervisor, its worker and the private database alive. This is the IDA counterpart of #1751 / #1763 (Hopper). No issue is linked; it was found while analyzing a large binary on a real IDA installation. ## Problem and expected behavior Trigger: any headless IDA request that does not complete within `timeoutMs`. The realistic case is `idb_open` on a large binary that is still running auto-analysis past the default 300 s, e.g. a 119 MB universal Mach-O: ```bash REA_ANALYSIS_PROVIDER=ida REA_IDA_MCP_CONFIG=headless.json rea decompile /Applications/iTerm.app/Contents/MacOS/iTerm2 _main --json ``` REA correctly cannot prove the worker stopped, so it retains the upstream connection and private workspace (`Worker inventory cannot prove that the request has stopped; the private workspace was retained.`). But the retained `StdioClientTransport` child process and its pipes stay referenced, so the Node event loop never drains. Observed: the CLI wrote its final JSON and was still running 15 h later, with the supervisor child and a 650 MB `.i64` held open. Expected: like Hopper after #1763, the CLI exits with status `1` and the same structured `cleanup_incomplete` result; a long-lived MCP server keeps the retained connection for a later close retry. The result should also say that the upstream worker may still be running, because nothing in REA can currently prove otherwise. ## Change and scope - `IdaMcpConnection` gains `detach()`: for a stdio registration it unrefs the child process and its stdin/stdout/stderr sockets. The SDK exposes only the child pid, so the handle is read from the transport and checked with `instanceof ChildProcess` before use. HTTP registrations hold no child and are unchanged. - `IdaSessionClient.close()` calls `detach()` when cleanup fails, the point where the connection is retained. - When cleanup fails before the owned worker's release is confirmed, the cleanup diagnostics gain `upstream_sessions: { state: "unverified", sessions: [{ session_id, backend, owned }], reason }`. It names every same-input session the latest successful inventory reported, including peers seen beside the requested worker, and keeps them across retries whose inventory is unavailable. Without inventory evidence it names the requested private worker with `owned: null`; after a confirmed close it reports an empty list, meaning the remaining identities are unknown. Upstream spawns workers in their own session and `shutdown()` deliberately leaves them running, so supervisor EOF does **not** stop a busy worker (thanks @dandgabr). Failures after a confirmed release, or before any open was attempted, do not report a worker. Nothing is closed or killed, ownership is unchanged, and the retained workspace is still reported. Worker lifetime is not changed by this PR. A one-shot CLI never retries `close`, so on `main` the hang does not advance cleanup either. Proving that the worker stopped needs an upstream cancellation or release receipt (the constraint discussed in #1710). Not included: #1710 (recovery after the supervisor reaps a worker) and #1769 / #1785 (missing procedure reporting). ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/ida/IdaMcpConnection.ts` (transport handles), invoked from `IdaSessionClient.close()` on failure. - CLI and MCP/tool-catalog contract: none; the CLI now exits with the result it already printed. - Provider, bridge, target-format, or platform compatibility: headless and attached stdio IDA registrations; HTTP unchanged. - Evidence, artifact, provenance, or reconstruction contract: none. - Process execution, authorization, cleanup, or containment impact: retained resources no longer hold a one-shot process open; MCP retry behavior is unchanged; an unconfirmed worker release is now reported explicitly instead of implied. - Generated metadata, package, or installation impact: none. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/cli/idaRetainedCleanupCli.test.ts` runs the compiled CLI against a stdio IDA registration whose supervisor completes the handshake and never answers a tool call (`timeoutMs: 500`). It requires exit `1` with `cleanup_incomplete` and a retained workspace under the configured `workspaceRoot`. - The CLI test also requires `cleanup_error.details.diagnostics.upstream_sessions` with `state: "unverified"` and the private `rea-` worker session. - `src/ida/IdaWorkspaceOwnership.test.ts` asserts `detach()` runs once when cleanup fails and not again when the retry succeeds, and that no `upstream_sessions` is reported when no open was attempted (this assertion fails if the diagnostic is emitted unconditionally). - Base reproduction: the new CLI test times out at 30 s on `origin/main` (`0b4ce836`) with the source fix reverted, and passes in ~2 s with it. - User-visible CLI output: unchanged `cleanup_incomplete` JSON; exit status `1` instead of hanging. - Remaining proof gaps: no Windows or Linux run (the test is not platform-gated, so CI covers them); HTTP registrations not exercised. ## Validation performed - `npm run test:focused -- src/ida/*.test.ts tests/boundary/cli/idaRetainedCleanupCli.test.ts`: 9 files, 54 tests passed - Same CLI test with only the `src/ida` source changes reverted: `Test timed out in 30000ms` - `npm run check`: passed (typecheck, lint, format:check, knip, metadata:check, architecture guards); no new lint warnings in touched files - `npm run verify:test-discovery`: all 837 test files discovered once - Real provider, macOS 15 arm64, IDA Pro 9.4.260610, ida-pro-mcp `c133c385`, headless `idalib-mcp --stdio`, registration `timeoutMs: 1500` on a small arm64 binary. Before: final JSON printed, then killed by a 60 s alarm (exit 142). After: exit `1` in about 5 s with `cleanup_incomplete` and `upstream_sessions.state: "unverified"`. - Worker lifetime, real provider, 119 MB universal Mach-O (iTerm2), `timeoutMs: 20000`, with `rea-agents@6.4.0` and this branch run side by side for 12 minutes. In both runs the worker analyzed at ~100% CPU for ~5.5 minutes and then stayed idle until stopped manually. 6.4.0's CLI was still running at 12 minutes; this branch's CLI exited after ~60 s. The worker behaved the same in both. ## Compatibility, safety, and release - Breaking changes or migration steps: none. `IdaMcpConnection` is internal; its test doubles implement the new no-op method. - Real Hopper/Ghidra, browser, or OS coverage: real IDA on macOS arm64 only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no process is killed or detached from its parent; only event-loop references change. A retained connection in a live MCP server keeps working. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:56:02 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:14:30 +00:00

[PR #4851] [CLOSED] fix(native): keep plutil's decode diagnostic when inspect_plist refuses a file #6442

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#4851

No description provided.

Rows
Columns