morluto/rea

Fork 0

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

[PR #1708] [MERGED] fix(native): retain failed pwntools process and runtime cleanup #4770

New issue

Closed

opened 2026-10-31 00:54:51 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:54:51 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1708
Author: @1NormalGuy
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/12/2026
Merged by: @morluto

Base: main ← Head: fix/pwntools-cleanup-ownership

📝 Commits (6)

0207e3b fix(native): retain failed pwntools process and runtime cleanup
1b565a4 test(native): verify retained recorded-crash cleanup lifecycle
7d8331e Merge remote-tracking branch 'origin/main' into fix/pr-1708-review
66a32a7 fix(native): finalize diagnostic cleanup before publishing results
a357aa4 fix(native): retire decoder stages before reading runtime evidence
433a744 Merge remote-tracking branch 'origin/main' into fix/pr-1708-review

📊 Changes

26 files changed (+1716 additions, -135 deletions)

View changed files

📝 src/application/binaryDiagnostics/BinaryLayoutPort.ts (+5 -0)
📝 src/application/binaryDiagnostics/BinaryLayoutService.test.ts (+18 -0)
📝 src/application/binaryDiagnostics/BinaryLayoutService.ts (+27 -1)
📝 src/application/binaryDiagnostics/RecordedCrashPort.ts (+5 -0)
📝 src/application/binaryDiagnostics/RecordedCrashService.test.ts (+18 -0)
📝 src/application/binaryDiagnostics/RecordedCrashService.ts (+27 -1)
📝 src/cli/binaryDiagnosticsCommands.ts (+2 -2)
📝 src/domain/analysisErrorBase.ts (+8 -0)
📝 src/domain/native/recordedCrash.ts (+11 -0)
📝 src/native/pwndbg/PwndbgCoreContext.ts (+22 -18)
📝 src/native/pwntools/PwntoolsDecoder.ts (+283 -41)
📝 src/native/pwntools/PwntoolsRecordedCrashProvider.ts (+34 -20)
📝 src/process/OwnedCommand.ts (+7 -1)
📝 src/server/createServer.ts (+8 -0)
📝 tests/boundary/binaryDiagnostics/pwntoolsLayoutProvider.test.ts (+26 -6)
📝 tests/boundary/binaryDiagnostics/recordedCrashProvider.test.ts (+111 -45)
📝 tests/boundary/mcp/analysisSummaryDetailMcp.test.ts (+3 -0)
📝 tests/boundary/mcp/analysisViewMcp.test.ts (+3 -0)
📝 tests/boundary/mcp/binaryDiagnosticsMcp.test.ts (+8 -0)
📝 tests/boundary/mcp/recordedCrashMcp.test.ts (+8 -0)

...and 6 more files

📄 Description

Summary

Retain owned decoder/debugger processes and runtime files until cleanup is confirmed. Finalize one-shot CLI inspections before publishing their original outcome, so a successful cleanup retry returns the decoded report and a genuine decoder failure retains its original diagnostics. These defects were reproduced during repository review; there is no related issue.

Problem and expected behavior

Previously, failed cleanup could discard its supervisor and delete files still belonging to a worker. A later CLI cleanup retry could also succeed while the command still reported stale cleanup_incomplete resources and hid its decoded report. Ownership must survive failed cleanup, and published outcomes must reflect the final cleanup attempt.

Change and scope

Retain worker/debugger supervisors and runtime roots, serialize admission, preserve prompt queued cancellation, block acquisition while cleanup remains unconfirmed, and allow close() to retry.
Execute both decoder stages through one owner-bound command runner. Retire each process group before returning its result or original failure, reading replies or failure markers, or acquiring the next stage. Pwndbg receives the executor instead of launcher/cleanup-transfer callbacks.
Add an explicit terminal inspection operation at the provider/service boundary and remove CLI-side result reconstruction. Successful retries restore the original success or typed primary failure.
Report only resources still owned after partial cleanup. Preserve validated reports through the existing partial_observation error field when cleanup remains incomplete, including artifact identity and diagnostics. Failed debugger enrichment retains validated basic core evidence with explicit requested: true and status: unavailable.
Close shared diagnostic services during MCP shutdown and register failure-test teardown before assertions.

Contract and boundary impact

Semantic owner and earliest changed stage: owned process execution and decoder lifecycle.
CLI and MCP/tool-catalog contract: advertised schemas unchanged; existing typed cleanup and partial-observation fields carry failures and retained reports.
Provider/platform scope: pwntools binary layout, recorded-crash decoding, and optional Pwndbg enrichment. No new target execution authority.
Evidence/provenance: caller-selected artifact paths, snapshot digests, byte counts, validated reports, primary failures, output, and cleanup reasons survive finalization.
Internal terminal-operation contract: an admitted inspection finalizes before publication; input validation or cancelled admission acquires nothing and does not close an existing provider. Reusable callers retain explicit close() responsibility.
Generated metadata, installation, package, and release impact: none; no public migration required.

Evidence and regression coverage

Actual owned Node workers and filesystem boundaries cover retained live children, retryable shutdown, admission blocking, queued cancellation, primary-decoder retirement before debugger acquisition, and current resource lists after partial cleanup. Public CLI regressions cover restored success, preserved command failure, recovered process cleanup, persistent runtime-removal failure with a retained report, and cancellation during cleanup. CLI/MCP service checks cover shared workflow projection and shutdown.

Representative final outcomes:

A successful runtime-removal retry returns normal Evidence with exit status 0.
A real decoder exit failure remains execution_failure after cleanup recovers.
Persistent cleanup returns cleanup_incomplete with current resources and a validated partial_observation when available.
Combined analysis/cleanup failures preserve both primary_error and cleanup_error.

Validation performed

On fix commit a357aa41a, with Node 24.18.0 and npm 11.16.0:

Focused provider/process/service/public CLI/MCP suite: 95 passed, 68 Linux-only cases skipped on macOS, across 15 files. Required runtime artifacts built successfully.
npm run check:ci: passed, including typecheck, lint, formatting, unused-code/metadata checks, duplication checks, module boundaries, and architecture guards.
npm run docs:check: passed, including README consistency.
npm run verify:test-discovery: all 949 test files discovered exactly once.
git diff --check and the pre-push check: passed.

After merging current upstream ad08510a1 into 433a744d1, the affected EVM/layout/recorded-crash MCP and retirement suite passed all 15 tests across four files, and npm run check:ci passed again. The server conflict preserves upstream static EVM binding and this PR's shared diagnostic cleanup.

The local Node-worker regressions establish actual POSIX ownership/filesystem behavior, with only the decoder availability gate simulated. They do not establish real pwntools/GDB behavior. Previous reviewed commit 66a32a734 passed all CI, including real Linux pwntools and recorded-crash workflows. CI on final merge head 433a744d1 is pending; real-provider verification will be recorded separately when it completes.

Review follow-up

Merged current upstream main cleanly. Addressed the cleanup-retry/result-reconciliation thread at the owning process/provider boundary. Separate standards and specification reviews also found and fixed stale resource lists after partial cleanup, lost decoded observations on persistent cleanup failure, and missing failure-test teardown. Follow-up review also fixed both worker-file races: failure markers and debugger replies are consumed only after confirmed retirement. Eight real-worker regressions cover surviving descendants rewriting files, unresolved ownership in both stages, and debugger output overflow. Optional debugger validation retains truthful partial evidence. No remaining findings from those reviews.

Review checklist

Concrete problem and focused Conventional Commit title.
Meaningful cleanup, cancellation, malformed-output, and ownership regressions.
Caller identity, provenance, failed attempts, and explicit uncertainty preserved.
Contracts and generated documentation checked.
Final diff checked for unrelated changes and unsupported provider 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/1708 **Author:** [@1NormalGuy](https://github.com/1NormalGuy) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/pwntools-cleanup-ownership` --- ### 📝 Commits (6) - [`0207e3b`](https://github.com/morluto/rea/commit/0207e3bf49d78851129f0cd807d0a4040a66d0fd) fix(native): retain failed pwntools process and runtime cleanup - [`1b565a4`](https://github.com/morluto/rea/commit/1b565a44f74e5615e3b9d717868ae87f844edc01) test(native): verify retained recorded-crash cleanup lifecycle - [`7d8331e`](https://github.com/morluto/rea/commit/7d8331ebedf84936de3ccf8febe4507d366ee826) Merge remote-tracking branch 'origin/main' into fix/pr-1708-review - [`66a32a7`](https://github.com/morluto/rea/commit/66a32a734874a81eddb76cd701e94baafecb50df) fix(native): finalize diagnostic cleanup before publishing results - [`a357aa4`](https://github.com/morluto/rea/commit/a357aa41aeca17a7a65dccdd316666c332cf1fd9) fix(native): retire decoder stages before reading runtime evidence - [`433a744`](https://github.com/morluto/rea/commit/433a744d155d30adbc3455a39e8b9ded8a1e4445) Merge remote-tracking branch 'origin/main' into fix/pr-1708-review ### 📊 Changes **26 files changed** (+1716 additions, -135 deletions) <details> <summary>View changed files</summary> 📝 `src/application/binaryDiagnostics/BinaryLayoutPort.ts` (+5 -0) 📝 `src/application/binaryDiagnostics/BinaryLayoutService.test.ts` (+18 -0) 📝 `src/application/binaryDiagnostics/BinaryLayoutService.ts` (+27 -1) 📝 `src/application/binaryDiagnostics/RecordedCrashPort.ts` (+5 -0) 📝 `src/application/binaryDiagnostics/RecordedCrashService.test.ts` (+18 -0) 📝 `src/application/binaryDiagnostics/RecordedCrashService.ts` (+27 -1) 📝 `src/cli/binaryDiagnosticsCommands.ts` (+2 -2) 📝 `src/domain/analysisErrorBase.ts` (+8 -0) 📝 `src/domain/native/recordedCrash.ts` (+11 -0) 📝 `src/native/pwndbg/PwndbgCoreContext.ts` (+22 -18) 📝 `src/native/pwntools/PwntoolsDecoder.ts` (+283 -41) 📝 `src/native/pwntools/PwntoolsRecordedCrashProvider.ts` (+34 -20) 📝 `src/process/OwnedCommand.ts` (+7 -1) 📝 `src/server/createServer.ts` (+8 -0) 📝 `tests/boundary/binaryDiagnostics/pwntoolsLayoutProvider.test.ts` (+26 -6) 📝 `tests/boundary/binaryDiagnostics/recordedCrashProvider.test.ts` (+111 -45) 📝 `tests/boundary/mcp/analysisSummaryDetailMcp.test.ts` (+3 -0) 📝 `tests/boundary/mcp/analysisViewMcp.test.ts` (+3 -0) 📝 `tests/boundary/mcp/binaryDiagnosticsMcp.test.ts` (+8 -0) 📝 `tests/boundary/mcp/recordedCrashMcp.test.ts` (+8 -0) _...and 6 more files_ </details> ### 📄 Description ## Summary Retain owned decoder/debugger processes and runtime files until cleanup is confirmed. Finalize one-shot CLI inspections before publishing their original outcome, so a successful cleanup retry returns the decoded report and a genuine decoder failure retains its original diagnostics. These defects were reproduced during repository review; there is no related issue. ## Problem and expected behavior Previously, failed cleanup could discard its supervisor and delete files still belonging to a worker. A later CLI cleanup retry could also succeed while the command still reported stale `cleanup_incomplete` resources and hid its decoded report. Ownership must survive failed cleanup, and published outcomes must reflect the final cleanup attempt. ## Change and scope - Retain worker/debugger supervisors and runtime roots, serialize admission, preserve prompt queued cancellation, block acquisition while cleanup remains unconfirmed, and allow `close()` to retry. - Execute both decoder stages through one owner-bound command runner. Retire each process group before returning its result or original failure, reading replies or failure markers, or acquiring the next stage. Pwndbg receives the executor instead of launcher/cleanup-transfer callbacks. - Add an explicit terminal inspection operation at the provider/service boundary and remove CLI-side result reconstruction. Successful retries restore the original success or typed primary failure. - Report only resources still owned after partial cleanup. Preserve validated reports through the existing `partial_observation` error field when cleanup remains incomplete, including artifact identity and diagnostics. Failed debugger enrichment retains validated basic core evidence with explicit `requested: true` and `status: unavailable`. - Close shared diagnostic services during MCP shutdown and register failure-test teardown before assertions. ## Contract and boundary impact - Semantic owner and earliest changed stage: owned process execution and decoder lifecycle. - CLI and MCP/tool-catalog contract: advertised schemas unchanged; existing typed cleanup and partial-observation fields carry failures and retained reports. - Provider/platform scope: pwntools binary layout, recorded-crash decoding, and optional Pwndbg enrichment. No new target execution authority. - Evidence/provenance: caller-selected artifact paths, snapshot digests, byte counts, validated reports, primary failures, output, and cleanup reasons survive finalization. - Internal terminal-operation contract: an admitted inspection finalizes before publication; input validation or cancelled admission acquires nothing and does not close an existing provider. Reusable callers retain explicit `close()` responsibility. - Generated metadata, installation, package, and release impact: none; no public migration required. ## Evidence and regression coverage Actual owned Node workers and filesystem boundaries cover retained live children, retryable shutdown, admission blocking, queued cancellation, primary-decoder retirement before debugger acquisition, and current resource lists after partial cleanup. Public CLI regressions cover restored success, preserved command failure, recovered process cleanup, persistent runtime-removal failure with a retained report, and cancellation during cleanup. CLI/MCP service checks cover shared workflow projection and shutdown. Representative final outcomes: - A successful runtime-removal retry returns normal Evidence with exit status 0. - A real decoder exit failure remains `execution_failure` after cleanup recovers. - Persistent cleanup returns `cleanup_incomplete` with current resources and a validated `partial_observation` when available. - Combined analysis/cleanup failures preserve both `primary_error` and `cleanup_error`. ## Validation performed On fix commit `a357aa41a`, with Node 24.18.0 and npm 11.16.0: - Focused provider/process/service/public CLI/MCP suite: 95 passed, 68 Linux-only cases skipped on macOS, across 15 files. Required runtime artifacts built successfully. - `npm run check:ci`: passed, including typecheck, lint, formatting, unused-code/metadata checks, duplication checks, module boundaries, and architecture guards. - `npm run docs:check`: passed, including README consistency. - `npm run verify:test-discovery`: all 949 test files discovered exactly once. - `git diff --check` and the pre-push check: passed. After merging current upstream `ad08510a1` into `433a744d1`, the affected EVM/layout/recorded-crash MCP and retirement suite passed all 15 tests across four files, and `npm run check:ci` passed again. The server conflict preserves upstream static EVM binding and this PR's shared diagnostic cleanup. The local Node-worker regressions establish actual POSIX ownership/filesystem behavior, with only the decoder availability gate simulated. They do not establish real pwntools/GDB behavior. Previous reviewed commit `66a32a734` passed all CI, including real Linux pwntools and recorded-crash workflows. CI on final merge head `433a744d1` is pending; real-provider verification will be recorded separately when it completes. ## Review follow-up Merged current upstream `main` cleanly. Addressed the cleanup-retry/result-reconciliation thread at the owning process/provider boundary. Separate standards and specification reviews also found and fixed stale resource lists after partial cleanup, lost decoded observations on persistent cleanup failure, and missing failure-test teardown. Follow-up review also fixed both worker-file races: failure markers and debugger replies are consumed only after confirmed retirement. Eight real-worker regressions cover surviving descendants rewriting files, unresolved ownership in both stages, and debugger output overflow. Optional debugger validation retains truthful partial evidence. No remaining findings from those reviews. ## Review checklist - [x] Concrete problem and focused Conventional Commit title. - [x] Meaningful cleanup, cancellation, malformed-output, and ownership regressions. - [x] Caller identity, provenance, failed attempts, and explicit uncertainty preserved. - [x] Contracts and generated documentation checked. - [x] Final diff checked for unrelated changes and unsupported provider 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:54:51 +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#4770

No description provided.

Rows
Columns