morluto/rea

Fork 0

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

[PR #1260] [MERGED] fix(session): retain failed provider cleanup for lifecycle retries #2746

New issue

Closed

opened 2026-10-17 21:09:46 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-17 21:09:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1260
Author: @N0zoM1z0
Created: 10/9/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/1190-session-cleanup-retry

📝 Commits (1)

2d239ee fix(session): retain failed provider cleanup for lifecycle retries

📊 Changes

4 files changed (+441 additions, -37 deletions)

View changed files

📝 src/application/DirectAnalysisCleanup.ts (+5 -4)
📝 src/application/binary/AnalysisClientCleanup.ts (+26 -0)
📝 src/application/binary/BinarySession.ts (+137 -33)
➕ tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts (+273 -0)

📄 Description

Summary

After provider cleanup failed, BinarySession discarded the client. A later close_binary returned success without retrying and a new target could start while the old runtime remained. Retain the retired client until cleanup succeeds, keep it unavailable for analysis, and gate replacement startup on the retry.

Follow-up to #1190 and #1201.

Problem and expected behavior

The smallest trigger is an owned runtime directory that cannot be removed. After the caller resolves the constraint, a successful second close must actually remove it. Repeated failed cleanup must still report its typed error and resource paths.

Change and scope

One pending cleanup binding covers close, target replacement, failed/cancelled startup, snapshot import failure and failed restoration. Successful cleanup releases the binding. Document retention persists across retries unless the caller explicitly changes it. Cancellation is rechecked after awaited cleanup before replacement admission.

The shared error helper preserves the primary lifecycle failure and cleanup diagnostics; one-shot managed cleanup reuses the same projection.

Contract and boundary impact

Semantic owner and earliest changed stage: shared application session lifecycle.
CLI and MCP/tool-catalog contract: existing schemas/inventory unchanged; failed opens return typed cleanup diagnostics rather than losing the primary error in a rejection.
Provider, bridge, target-format, or platform compatibility: provider-neutral; no provider protocol changes.
Evidence, artifact, provenance, or reconstruction contract: primary errors and existing one-shot observations remain preserved.
Process execution, authorization, cleanup, or containment impact: no replacement starts until the retired client confirms cleanup; no new authority.
Generated metadata, package, or installation impact: none; ignored build outputs are not committed.

Evidence and regression coverage

Six regression cases fail on the base: close/switch/reopen retries, failed startup, cancelled startup and retained documents.
Additional coverage verifies failed restoration, explicit retention changes and cancellation during pending cleanup. The cancellation test failed with the first implementation, then passed after the admission check was added.
Real filesystem fault probe: first close reports the retained path; after restoring permissions, the second close invokes provider cleanup and removes the runtime.
Real Linux Ghidra 12.1.4 + JDK 21, actual stdio MCP: import/inventory of a source-built x64 ELF, chmod on its owned runtime, close_binary reports cleanup_incomplete, permissions restored, second close_binary removes the runtime, then ping succeeds on the same connection. Java heap 512 MiB, Node heap 768 MiB, one CPU.
Representative MCP failure: {"error":{"code":"cleanup_incomplete","details":{"operation":"close_binary","resources":["/tmp/owned-runtime"]}}}; successful retry: {"result":null} with the directory absent.
Remaining proof gaps: no new real Hopper/IDA or Windows/macOS provider run; portable lifecycle regressions cover the shared application behavior.

Validation performed

npm run build:cached — passed on the committed tree.
npm run test:focused -- tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts tests/composition/analysis-sessions/binarySession.test.ts src/application/binary/AnalysisClientCleanup.test.ts tests/composition/directAnalysisCleanup.test.ts tests/composition/directAnalysisSnapshotReplay.test.ts — 61 passed.
npm run verify:test-discovery — passed; new test discovered once.
npm run check:fast and the normal pre-push hook — passed.
Both owned-filesystem probes above — passed on the committed tree.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: real Linux Ghidra verified as above; other real providers not rerun.
Package or release metadata impact: none; no release performed.
Security, privacy, process, or containment review: pending ownership is retained; filesystem faults affect only isolated test-owned directories; no host permission defaults changed.

Review checklist

Focused bug fix with linked prior issue and Conventional Commit title.
Fail-before/pass-after regressions and real provider workflow verified.
CLI/MCP diagnostics, cancellation, ownership and caller retention reviewed.
No generated files, binaries, local logs or unrelated changes committed.

_{🔄 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/1260 **Author:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Created:** 10/9/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/1190-session-cleanup-retry` --- ### 📝 Commits (1) - [`2d239ee`](https://github.com/morluto/rea/commit/2d239eeabd791fda2956b6bfce3fd17959039610) fix(session): retain failed provider cleanup for lifecycle retries ### 📊 Changes **4 files changed** (+441 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `src/application/DirectAnalysisCleanup.ts` (+5 -4) 📝 `src/application/binary/AnalysisClientCleanup.ts` (+26 -0) 📝 `src/application/binary/BinarySession.ts` (+137 -33) ➕ `tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts` (+273 -0) </details> ### 📄 Description ## Summary After provider cleanup failed, BinarySession discarded the client. A later `close_binary` returned success without retrying and a new target could start while the old runtime remained. Retain the retired client until cleanup succeeds, keep it unavailable for analysis, and gate replacement startup on the retry. Follow-up to #1190 and #1201. ## Problem and expected behavior The smallest trigger is an owned runtime directory that cannot be removed. After the caller resolves the constraint, a successful second close must actually remove it. Repeated failed cleanup must still report its typed error and resource paths. ## Change and scope One pending cleanup binding covers close, target replacement, failed/cancelled startup, snapshot import failure and failed restoration. Successful cleanup releases the binding. Document retention persists across retries unless the caller explicitly changes it. Cancellation is rechecked after awaited cleanup before replacement admission. The shared error helper preserves the primary lifecycle failure and cleanup diagnostics; one-shot managed cleanup reuses the same projection. ## Contract and boundary impact - Semantic owner and earliest changed stage: shared application session lifecycle. - CLI and MCP/tool-catalog contract: existing schemas/inventory unchanged; failed opens return typed cleanup diagnostics rather than losing the primary error in a rejection. - Provider, bridge, target-format, or platform compatibility: provider-neutral; no provider protocol changes. - Evidence, artifact, provenance, or reconstruction contract: primary errors and existing one-shot observations remain preserved. - Process execution, authorization, cleanup, or containment impact: no replacement starts until the retired client confirms cleanup; no new authority. - Generated metadata, package, or installation impact: none; ignored build outputs are not committed. ## Evidence and regression coverage - Six regression cases fail on the base: close/switch/reopen retries, failed startup, cancelled startup and retained documents. - Additional coverage verifies failed restoration, explicit retention changes and cancellation during pending cleanup. The cancellation test failed with the first implementation, then passed after the admission check was added. - Real filesystem fault probe: first close reports the retained path; after restoring permissions, the second close invokes provider cleanup and removes the runtime. - **Real Linux Ghidra 12.1.4 + JDK 21, actual stdio MCP:** import/inventory of a source-built x64 ELF, chmod on its owned runtime, `close_binary` reports `cleanup_incomplete`, permissions restored, second `close_binary` removes the runtime, then ping succeeds on the same connection. Java heap 512 MiB, Node heap 768 MiB, one CPU. - Representative MCP failure: `{"error":{"code":"cleanup_incomplete","details":{"operation":"close_binary","resources":["/tmp/owned-runtime"]}}}`; successful retry: `{"result":null}` with the directory absent. - Remaining proof gaps: no new real Hopper/IDA or Windows/macOS provider run; portable lifecycle regressions cover the shared application behavior. ## Validation performed - `npm run build:cached` — passed on the committed tree. - `npm run test:focused -- tests/composition/analysis-sessions/binarySessionCleanupRetry.test.ts tests/composition/analysis-sessions/binarySession.test.ts src/application/binary/AnalysisClientCleanup.test.ts tests/composition/directAnalysisCleanup.test.ts tests/composition/directAnalysisSnapshotReplay.test.ts` — 61 passed. - `npm run verify:test-discovery` — passed; new test discovered once. - `npm run check:fast` and the normal pre-push hook — passed. - Both owned-filesystem probes above — passed on the committed tree. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: real Linux Ghidra verified as above; other real providers not rerun. - Package or release metadata impact: none; no release performed. - Security, privacy, process, or containment review: pending ownership is retained; filesystem faults affect only isolated test-owned directories; no host permission defaults changed. ## Review checklist - [x] Focused bug fix with linked prior issue and Conventional Commit title. - [x] Fail-before/pass-after regressions and real provider workflow verified. - [x] CLI/MCP diagnostics, cancellation, ownership and caller retention reviewed. - [x] No generated files, binaries, local logs or unrelated changes committed. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-17 21:09:46 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:06:03 +00:00

[PR #2748] [MERGED] fix(javascript): keep spread child argv counts unknown #5362

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

No description provided.

Rows
Columns