morluto/rea

Fork 0

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

[PR #2014] [MERGED] fix(ghidra): recover stale clients on same-target reopen #4967

New issue

Closed

opened 2026-10-31 00:57:29 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:57:29 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2014
Author: @lyly-bonbon
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/12/2026
Merged by: @morluto

Base: main ← Head: fix/ghidra-reopen-health

📝 Commits (5)

007a349 fix(ghidra): recover stale clients on same-target reopen
ad717f5 test(ghidra): allow supervised shutdown in health regression
2ecd9df fix(ghidra): read process exit without diagnostic snapshots
26223d5 fix(cli): retain import-map option help after schema reuse
2f325ae test(adb): align fixtures with literal output and directory commands

📊 Changes

9 files changed (+498 additions, -24 deletions)

View changed files

📝 src/adb/AdbProvider.test.ts (+12 -9)
📝 src/cli/webModuleCommands.ts (+4 -4)
📝 src/ghidra/GhidraClient.ts (+31 -1)
📝 src/ghidra/GhidraProviderClient.ts (+5 -1)
📝 src/process/ProviderProcess.ts (+21 -7)
📝 src/process/ProviderProcessOutput.test.ts (+52 -1)
➕ tests/boundary/mcp/ghidraReopen.test.ts (+346 -0)
📝 tests/boundary/providers/ghidra/ghidraClient.test.ts (+19 -1)
📝 tests/fixtures/fakeGhidra.mjs (+8 -0)

📄 Description

Summary

Recover a retained Ghidra session when its process has exited or its bridge connection has closed. Same-target open_binary now retires the stale client; the next query starts a replacement after cleanup succeeds.

Refs #1719.

Problem and expected behavior

After a successful query, killing the Ghidra child or disconnecting its socket left the successful startup promise cached. Ghidra exposed no operational health snapshot, so the composite provider's health response let same-target reopen reuse that client. The next query still failed against the old bridge.

Reopen should release the stale session and permit a fresh lazy start. If cleanup cannot be confirmed, it must return cleanup_incomplete, retain ownership, and retry cleanup on the next open without starting another child.

Change and scope

Report observed process exit, socket disconnection, and resources retained after failed cleanup from GhidraClient.
Forward that snapshot through the Ghidra provider's existing optional health hook, using the shared session recovery path.
Preserve lazy startup and reuse of a live client. Historical exit diagnostics after successful cleanup do not mark a new session unhealthy.

This addresses stale-client recovery. It does not change startup deadlines, snapshot semantics, or detection of a connected but unresponsive bridge.

Contract and boundary impact

Semantic owner and earliest changed stage: Ghidra client lifecycle observation, before retained-client reuse.
CLI and MCP/tool-catalog contract: existing shapes and tools; same-target reopen now recovers from observed exit/disconnection.
Provider, bridge, target-format, or platform compatibility: no bridge protocol or target-format changes.
Evidence, artifact, provenance, or reconstruction contract: unchanged; tests verify the input bytes remain intact.
Process execution, authorization, cleanup, or containment impact: existing ownership and cleanup gates remain authoritative; replacement stays blocked while cleanup is incomplete.
Generated metadata, package, or installation impact: none.

Evidence and regression coverage

Added three public MCP scenarios covering lazy start/live reuse, signal exit, socket loss with a live child, and two failed cleanup attempts followed by recovery. They use the production client/provider/session, real Node children and Unix sockets, with installation discovery and the Java bridge simulated.
All three fail on base b8fab742 and pass with this change: old resources survive reopen, a disconnected child remains alive, and unconfirmed cleanup is incorrectly bypassed on the base.
Extended client boundary tests for retained file descriptors and runtime directories after cleanup failure, successful retries, and historical exit diagnostics.
Representative MCP behavior: open_binary returns success after stale resources are released; list_strings succeeds through the replacement. Unconfirmed cleanup returns error code cleanup_incomplete and launches no replacement.
Remaining proof gaps: no real Ghidra/JDK installation on this host; the new Unix-socket MCP scenarios explicitly skip Windows.

Validation performed

On macOS arm64 with Node 24.18.0 and npm 11.16.0:

npm run test:focused -- tests/boundary/mcp/ghidraReopen.test.ts tests/boundary/providers/ghidra/ghidraClient.test.ts src/ghidra/GhidraProvider.behavior.test.ts src/application/binary/BinarySessionLiveness.test.ts tests/composition/analysis-sessions/binarySession.test.ts tests/composition/analysis-sessions/providerBinding.test.ts — 124 tests passed across six files.
npm run check — passed; lint reported warnings and no errors.
Re-ran the 33 client boundary tests and npm run check after making the shutdown diagnostic assertion compare against the child's observed exit code; both passed.
npm run verify:test-discovery — all 895 test files discovered exactly once.
npm run jscpd — passed.
git diff --check — passed.
npm run verify:ghidra — build passed; real-provider verification could not start because GHIDRA_INSTALL_DIR is unavailable. No real-engine result is claimed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: controlled macOS process/socket coverage only; real Ghidra and Windows execution remain unverified.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new launch authority or transport data; recovery uses existing owned-resource cleanup and preserves failed cleanup for retry.

Review checklist

This PR addresses a concrete problem and links the related issue.
The change has one focused outcome and a Conventional Commit title.
Tests cover the changed behavior and cleanup failure paths.
Public schemas and generated metadata require no changes.
Representative MCP behavior and verification limits are recorded above.
The final diff contains no unrelated changes or unsupported claims.

_{🔄 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/2014 **Author:** [@lyly-bonbon](https://github.com/lyly-bonbon) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/ghidra-reopen-health` --- ### 📝 Commits (5) - [`007a349`](https://github.com/morluto/rea/commit/007a34940f419a8e6a5933ae2e53b6ed02effa11) fix(ghidra): recover stale clients on same-target reopen - [`ad717f5`](https://github.com/morluto/rea/commit/ad717f5660663e51d32572e05a55709d00e4c319) test(ghidra): allow supervised shutdown in health regression - [`2ecd9df`](https://github.com/morluto/rea/commit/2ecd9df83b8eb2fd2aeab5a07fcd8a6ec7d5e7c6) fix(ghidra): read process exit without diagnostic snapshots - [`26223d5`](https://github.com/morluto/rea/commit/26223d5aa836f2320e833a8b7c9bf6e10c83bf50) fix(cli): retain import-map option help after schema reuse - [`2f325ae`](https://github.com/morluto/rea/commit/2f325ae34752fe818db98a475657b385babe14af) test(adb): align fixtures with literal output and directory commands ### 📊 Changes **9 files changed** (+498 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `src/adb/AdbProvider.test.ts` (+12 -9) 📝 `src/cli/webModuleCommands.ts` (+4 -4) 📝 `src/ghidra/GhidraClient.ts` (+31 -1) 📝 `src/ghidra/GhidraProviderClient.ts` (+5 -1) 📝 `src/process/ProviderProcess.ts` (+21 -7) 📝 `src/process/ProviderProcessOutput.test.ts` (+52 -1) ➕ `tests/boundary/mcp/ghidraReopen.test.ts` (+346 -0) 📝 `tests/boundary/providers/ghidra/ghidraClient.test.ts` (+19 -1) 📝 `tests/fixtures/fakeGhidra.mjs` (+8 -0) </details> ### 📄 Description ## Summary Recover a retained Ghidra session when its process has exited or its bridge connection has closed. Same-target `open_binary` now retires the stale client; the next query starts a replacement after cleanup succeeds. Refs #1719. ## Problem and expected behavior After a successful query, killing the Ghidra child or disconnecting its socket left the successful startup promise cached. Ghidra exposed no operational health snapshot, so the composite provider's health response let same-target reopen reuse that client. The next query still failed against the old bridge. Reopen should release the stale session and permit a fresh lazy start. If cleanup cannot be confirmed, it must return `cleanup_incomplete`, retain ownership, and retry cleanup on the next open without starting another child. ## Change and scope - Report observed process exit, socket disconnection, and resources retained after failed cleanup from `GhidraClient`. - Forward that snapshot through the Ghidra provider's existing optional health hook, using the shared session recovery path. - Preserve lazy startup and reuse of a live client. Historical exit diagnostics after successful cleanup do not mark a new session unhealthy. This addresses stale-client recovery. It does not change startup deadlines, snapshot semantics, or detection of a connected but unresponsive bridge. ## Contract and boundary impact - Semantic owner and earliest changed stage: Ghidra client lifecycle observation, before retained-client reuse. - CLI and MCP/tool-catalog contract: existing shapes and tools; same-target reopen now recovers from observed exit/disconnection. - Provider, bridge, target-format, or platform compatibility: no bridge protocol or target-format changes. - Evidence, artifact, provenance, or reconstruction contract: unchanged; tests verify the input bytes remain intact. - Process execution, authorization, cleanup, or containment impact: existing ownership and cleanup gates remain authoritative; replacement stays blocked while cleanup is incomplete. - Generated metadata, package, or installation impact: none. ## Evidence and regression coverage - Added three public MCP scenarios covering lazy start/live reuse, signal exit, socket loss with a live child, and two failed cleanup attempts followed by recovery. They use the production client/provider/session, real Node children and Unix sockets, with installation discovery and the Java bridge simulated. - All three fail on base `b8fab742` and pass with this change: old resources survive reopen, a disconnected child remains alive, and unconfirmed cleanup is incorrectly bypassed on the base. - Extended client boundary tests for retained file descriptors and runtime directories after cleanup failure, successful retries, and historical exit diagnostics. - Representative MCP behavior: `open_binary` returns success after stale resources are released; `list_strings` succeeds through the replacement. Unconfirmed cleanup returns error code `cleanup_incomplete` and launches no replacement. - Remaining proof gaps: no real Ghidra/JDK installation on this host; the new Unix-socket MCP scenarios explicitly skip Windows. ## Validation performed On macOS arm64 with Node 24.18.0 and npm 11.16.0: - `npm run test:focused -- tests/boundary/mcp/ghidraReopen.test.ts tests/boundary/providers/ghidra/ghidraClient.test.ts src/ghidra/GhidraProvider.behavior.test.ts src/application/binary/BinarySessionLiveness.test.ts tests/composition/analysis-sessions/binarySession.test.ts tests/composition/analysis-sessions/providerBinding.test.ts` — 124 tests passed across six files. - `npm run check` — passed; lint reported warnings and no errors. - Re-ran the 33 client boundary tests and `npm run check` after making the shutdown diagnostic assertion compare against the child's observed exit code; both passed. - `npm run verify:test-discovery` — all 895 test files discovered exactly once. - `npm run jscpd` — passed. - `git diff --check` — passed. - `npm run verify:ghidra` — build passed; real-provider verification could not start because `GHIDRA_INSTALL_DIR` is unavailable. No real-engine result is claimed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: controlled macOS process/socket coverage only; real Ghidra and Windows execution remain unverified. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new launch authority or transport data; recovery uses existing owned-resource cleanup and preserves failed cleanup for retry. ## Review checklist - [x] This PR addresses a concrete problem and links the related issue. - [x] The change has one focused outcome and a Conventional Commit title. - [x] Tests cover the changed behavior and cleanup failure paths. - [x] Public schemas and generated metadata require no changes. - [x] Representative MCP behavior and verification limits are recorded above. - [x] The final diff contains no unrelated changes or unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:57:29 +00:00

closed this issue
added the
pull-request
label

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#4967

No description provided.

Rows
Columns