morluto/rea

Fork 0

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

[PR #6205] [MERGED] fix(process): settle owned cleanup with the launcher's own exit proof #7202

New issue

Closed

opened 2026-11-01 21:46:06 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:06 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6205
Author: @kaoru0822-kitauji
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/6134-launcher-exit-settle

📝 Commits (2)

3b43ee3 fix(process): settle owned cleanup with the launcher's own exit proof
9117196 test(process): bound Hopper session cleanup attempts by exit evidence

📊 Changes

3 files changed (+271 additions, -16 deletions)

View changed files

➕ src/process/ProviderProcess.launcherExit.test.ts (+240 -0)
📝 src/process/ProviderProcess.ts (+22 -13)
📝 tests/boundary/process/binarySessionCleanupRetry.test.ts (+9 -3)

📄 Description

What broke: When the supervised launcher was still in kernel teardown at cleanup time (state R, PF_EXITING set, empty cmdline), owned cleanup failed closed. ProviderProcess.stop() then reported incomplete immediately and never used its own child-exit observation, so a run whose analysis had succeeded ended as cleanup_incomplete.
Fix: If ownership verification fails before the supervisor has seen the launcher exit, stop() waits up to the kill grace (default 1 s) for that exit, which is positive proof, and then verifies once more. If the exit was already seen before cleanup started, there is no new evidence, so the original failure is returned unchanged.
Risk: Low. The wait never triggers a signal, and the second attempt runs the full fail-closed verification. A failing stop() whose launcher stays alive now returns up to one kill grace later.

Summary

Fixes #6134.

The issue reports owned launcher command identity did not match (observed=[java]; ...) on 6.4.0. That came from settleLauncherExit refreshing the process table only once. On current main, #5249 (not yet in a release) settles native exit-in-progress before comparing the command, so that exact message no longer appears on Linux or macOS native hosts. The failure itself still occurs with a different reason. The native exit wait is limited to a shared 1 s budget. When kernel teardown takes longer, cleanup returns owned launcher exit could not be confirmed, and the supervisor reports incomplete without waiting for the child exit it already observes.

Problem and expected behavior

Smallest trigger: an owned launcher with a large heap closes and enters kernel exit on a slow host, and stop() runs during that teardown. This is the Ghidra path: the shutdown ack, then waitForExit(500), then stop().

Violated invariant: docs/testing.md says "Positive owned exit proof resolves target lifetime." The supervisor holds that proof, because its unreaped child keeps the PID, and exit means the child was reaped. Before this change it ignored the proof whenever process-table verification could not settle the launcher.

Expected: cleanup succeeds once the launcher really exits within the grace. A still-live launcher whose identity does not match stays incomplete and is never signaled.

Change and scope

src/process/ProviderProcess.ts #stopWithin: the flow is cleanup(). If that fails and the exit had not been seen when cleanup began, the supervisor waits up to killGraceMs for its own exit, then runs cleanup() once more. Every other path keeps its previous order. A small incompleteCleanup helper replaces three copies of the same result construction.
Shared boundary: this covers every ProviderProcessSupervisor owner that passes cleanup. That includes Ghidra, OwnedCommand, OwnedWorkerRuntime, Hopper, Jadx MCP, Firmware, Wakaru and SourceMap.
Other callers I checked:
- ProcessCaptureLifecycle releases only after a terminal (exit) observation.
- Electron cleanup runs after Playwright close(). I inspected its source and did not change it.
- observeOwnedProcessLineage returns a point-in-time unavailable result, not a failure.
Intentionally unchanged:
- The ownership rules in ProcessOwnership*. Bracketed [comm] text is not treated as exit proof, because a live process can set its argv to [java]. Native lifecycle stays the owner of exit-in-progress, and a mismatch on a live non-exiting launcher still fails.
- The 1 s native settlement budget.
- The identityless single refresh. The supervisor retry covers it when a supervisor owns the launcher. A direct cleanupOwnedProcessGroup call without a supervisor still fails closed, because no positive exit evidence exists there.
Cancellation: stop() takes no AbortSignal, because final cleanup is designed to finish. The added wait is bounded by killGraceMs and uses the existing waitForExit primitive. Cancellation of the cancellable native settle (settleExitingProcessIdentities) is still covered by the existing test in ProcessOwnership.exiting.test.ts.

Contribution scope: not applicable. This is an ordinary fix within existing process-cleanup boundaries.

Contract and boundary impact

Semantic owner and earliest changed stage: ProviderProcessSupervisor stop, the owner of the launcher child handle and its exit observation.
Contribution scope fit and linked maintainer decision: not applicable (ordinary fix).
CLI and MCP/tool-catalog contract: no change. A run that used to fail with cleanup_incomplete because of a late launcher exit now completes.
Provider, bridge, target-format, or platform compatibility: POSIX supervised providers. The Windows Job Object path is unchanged.
Evidence, artifact, provenance, or reconstruction contract: no change. On failure, cleanupResult keeps the latest verification failure.
Target selection, authority, upstream behavior, and cleanup details: the wait only observes. It sends no signal and never kills a launcher that is not verified.
Generated metadata, package, or installation impact: none.
Launch or attach to a process.

(This is what the existing supervised providers already do. This PR adds no new effects.)

Evidence and regression coverage

Tests added: src/process/ProviderProcess.launcherExit.test.ts. These run the real cleanupOwnedProcessGroup against a controlled host.
- Identityless host with a procps [java] row that exits just after the single refresh. The first attempt fails with the issue's literal mismatch reason, the second attempt cleans, and nothing is signaled.
- Native host with exiting lasting past the 1 s settlement deadline. The first attempt fails with owned launcher exit could not be confirmed, and after the child exit the result is verified-cleanup.
- A still-live mismatched /usr/bin/other launcher, on an identityless host and on a native non-zombie host. Both stay incomplete with the mismatch reason. Verification runs once, the wait is bounded by killGraceMs, and there is no signal or kill.
- Exit already observed before cleanup. There is no retry and no wait.
- The post-exit verification also fails. That failure is returned, and a later stop() retry cleans.
Test updated: tests/boundary/process/binarySessionCleanupRetry.test.ts. The fake Hopper launcher exits after its shutdown acknowledgement. If the supervisor first observes that exit during a failed verification, it verifies once more, so the first close makes one or two attempts depending on timing. The test used to pin exactly one attempt; it now asserts that bound. It still requires exactly one more attempt for each later session retry, because the exit was already observed by then, and it still requires resources to be retained until a successful cleanup.
With the ProviderProcess.ts change reverted, 5 of the 6 new tests fail. "No retry without new evidence" passes on both, as expected.

Base reproduction on real Linux (node:24.18.0-trixie container in a colima VM, procps-ng 4.0.4, built dist/). An owned python3 launcher with a 2 GiB heap (THP disabled) has its stdin closed, and stop() is entered once /proc/<pid>/stat shows PF_EXITING with an empty cmdline:

build	unthrottled	`--cpus=0.04` (slow guest)
base `c465ff5a8`	`verified-cleanup`, teardown 70 ms	`incomplete`: `owned launcher exit could not be confirmed`, 2/2 runs (1.4–1.9 s)
this PR	`verified-cleanup`, teardown 63 ms	`verified-cleanup`, 2/2 runs (stop returned after 1.6 s and 2.5 s)

The mid-exit row was state=R, PF_EXITING set and cmdline 0 bytes in every run. Repro evidence is also posted on the issue.

Remaining proof gaps:
- No real Ghidra/JVM run. The Linux check used a large-heap python3 launcher.
- No macOS mid-exit run.
- A teardown longer than the 1 s native budget plus the 1 s kill grace still fails closed by design.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:local -- src/process/ProviderProcess.launcherExit.test.ts: 6 passed. Without the fix, 5 fail.
npm run test:local -- all 216 test files under src/{process,ghidra,hopper,android,firmware,javascript,kuna,native,apktool,sqlite,evm,providers,browser,wasm,signals,pcap,captures}: 3211 passed, 14 skipped.
npm run test:local -- the 4 src/application/src/cli/src/mcp test files that exercise stop() or cleanup_incomplete, plus the new file: 49 passed.
npm run test:focused -- 40 boundary files: tests/boundary/process/{binarySessionCleanupRetry,ghidraJavaLifecycle,jadxJavaLifecycle,jadxHashCleanup,ownedCommand,ownedCommandCancellation,ownedDebuggerSession,ghidraDecompilerVerifierCleanup}, all tests/boundary/providers/hopper/*, the ghidra/native/kuna provider tests, mcp/ghidraReopen, sqlite and the javascript worker. Result: 455 passed, 6 skipped.
npm run check:fast: passed.
npm run format:check: passed.
The real Linux container repro above, against base and fix builds.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: real Linux kernel mid-exit was tested with python3. Ghidra, a real JVM and macOS were not run.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new signal path. The second verification runs only after the supervisor sees its own child exit. Before that reap, the child holds the PID, so it cannot be reused.

Author readiness

Human owner: @kaoru0822-kitauji

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
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/6205 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6134-launcher-exit-settle` --- ### 📝 Commits (2) - [`3b43ee3`](https://github.com/morluto/rea/commit/3b43ee38a3cdb2cf9fc27266a086e589a076c625) fix(process): settle owned cleanup with the launcher's own exit proof - [`9117196`](https://github.com/morluto/rea/commit/9117196dd86ab558417d425a4d08cbb8672e5277) test(process): bound Hopper session cleanup attempts by exit evidence ### 📊 Changes **3 files changed** (+271 additions, -16 deletions) <details> <summary>View changed files</summary> ➕ `src/process/ProviderProcess.launcherExit.test.ts` (+240 -0) 📝 `src/process/ProviderProcess.ts` (+22 -13) 📝 `tests/boundary/process/binarySessionCleanupRetry.test.ts` (+9 -3) </details> ### 📄 Description **What broke:** When the supervised launcher was still in kernel teardown at cleanup time (state `R`, `PF_EXITING` set, empty cmdline), owned cleanup failed closed. `ProviderProcess.stop()` then reported `incomplete` immediately and never used its own child-exit observation, so a run whose analysis had succeeded ended as `cleanup_incomplete`. **Fix:** If ownership verification fails before the supervisor has seen the launcher exit, `stop()` waits up to the kill grace (default 1 s) for that exit, which is positive proof, and then verifies once more. If the exit was already seen before cleanup started, there is no new evidence, so the original failure is returned unchanged. **Risk:** Low. The wait never triggers a signal, and the second attempt runs the full fail-closed verification. A failing `stop()` whose launcher stays alive now returns up to one kill grace later. ## Summary Fixes #6134. The issue reports `owned launcher command identity did not match (observed=[java]; ...)` on 6.4.0. That came from `settleLauncherExit` refreshing the process table only once. On current `main`, #5249 (not yet in a release) settles native exit-in-progress before comparing the command, so that exact message no longer appears on Linux or macOS native hosts. The failure itself still occurs with a different reason. The native exit wait is limited to a shared 1 s budget. When kernel teardown takes longer, cleanup returns `owned launcher exit could not be confirmed`, and the supervisor reports `incomplete` without waiting for the child exit it already observes. ## Problem and expected behavior Smallest trigger: an owned launcher with a large heap closes and enters kernel exit on a slow host, and `stop()` runs during that teardown. This is the Ghidra path: the shutdown ack, then `waitForExit(500)`, then `stop()`. Violated invariant: docs/testing.md says "Positive owned exit proof resolves target lifetime." The supervisor holds that proof, because its unreaped child keeps the PID, and `exit` means the child was reaped. Before this change it ignored the proof whenever process-table verification could not settle the launcher. Expected: cleanup succeeds once the launcher really exits within the grace. A still-live launcher whose identity does not match stays `incomplete` and is never signaled. ## Change and scope - `src/process/ProviderProcess.ts` `#stopWithin`: the flow is `cleanup()`. If that fails and the exit had not been seen when cleanup began, the supervisor waits up to `killGraceMs` for its own exit, then runs `cleanup()` once more. Every other path keeps its previous order. A small `incompleteCleanup` helper replaces three copies of the same result construction. - Shared boundary: this covers every `ProviderProcessSupervisor` owner that passes `cleanup`. That includes Ghidra, `OwnedCommand`, `OwnedWorkerRuntime`, Hopper, Jadx MCP, Firmware, Wakaru and SourceMap. - Other callers I checked: - `ProcessCaptureLifecycle` releases only after a terminal (exit) observation. - Electron cleanup runs after Playwright `close()`. I inspected its source and did not change it. - `observeOwnedProcessLineage` returns a point-in-time `unavailable` result, not a failure. - Intentionally unchanged: - The ownership rules in `ProcessOwnership*`. Bracketed `[comm]` text is not treated as exit proof, because a live process can set its argv to `[java]`. Native lifecycle stays the owner of exit-in-progress, and a mismatch on a live non-exiting launcher still fails. - The 1 s native settlement budget. - The identityless single refresh. The supervisor retry covers it when a supervisor owns the launcher. A direct `cleanupOwnedProcessGroup` call without a supervisor still fails closed, because no positive exit evidence exists there. - Cancellation: `stop()` takes no `AbortSignal`, because final cleanup is designed to finish. The added wait is bounded by `killGraceMs` and uses the existing `waitForExit` primitive. Cancellation of the cancellable native settle (`settleExitingProcessIdentities`) is still covered by the existing test in `ProcessOwnership.exiting.test.ts`. Contribution scope: not applicable. This is an ordinary fix within existing process-cleanup boundaries. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ProviderProcessSupervisor` stop, the owner of the launcher child handle and its exit observation. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix). - CLI and MCP/tool-catalog contract: no change. A run that used to fail with `cleanup_incomplete` because of a late launcher exit now completes. - Provider, bridge, target-format, or platform compatibility: POSIX supervised providers. The Windows Job Object path is unchanged. - Evidence, artifact, provenance, or reconstruction contract: no change. On failure, `cleanupResult` keeps the latest verification failure. - Target selection, authority, upstream behavior, and cleanup details: the wait only observes. It sends no signal and never kills a launcher that is not verified. - Generated metadata, package, or installation impact: none. - [x] Launch or attach to a process. (This is what the existing supervised providers already do. This PR adds no new effects.) ## Evidence and regression coverage - Tests added: `src/process/ProviderProcess.launcherExit.test.ts`. These run the real `cleanupOwnedProcessGroup` against a controlled host. - Identityless host with a procps `[java]` row that exits just after the single refresh. The first attempt fails with the issue's literal mismatch reason, the second attempt cleans, and nothing is signaled. - Native host with `exiting` lasting past the 1 s settlement deadline. The first attempt fails with `owned launcher exit could not be confirmed`, and after the child exit the result is `verified-cleanup`. - A still-live mismatched `/usr/bin/other` launcher, on an identityless host and on a native `non-zombie` host. Both stay `incomplete` with the mismatch reason. Verification runs once, the wait is bounded by `killGraceMs`, and there is no signal or kill. - Exit already observed before cleanup. There is no retry and no wait. - The post-exit verification also fails. That failure is returned, and a later `stop()` retry cleans. - Test updated: `tests/boundary/process/binarySessionCleanupRetry.test.ts`. The fake Hopper launcher exits after its shutdown acknowledgement. If the supervisor first observes that exit during a failed verification, it verifies once more, so the first close makes one or two attempts depending on timing. The test used to pin exactly one attempt; it now asserts that bound. It still requires exactly one more attempt for each later session retry, because the exit was already observed by then, and it still requires resources to be retained until a successful cleanup. - With the `ProviderProcess.ts` change reverted, 5 of the 6 new tests fail. "No retry without new evidence" passes on both, as expected. - Base reproduction on real Linux (`node:24.18.0-trixie` container in a colima VM, procps-ng 4.0.4, built `dist/`). An owned `python3` launcher with a 2 GiB heap (THP disabled) has its stdin closed, and `stop()` is entered once `/proc/<pid>/stat` shows `PF_EXITING` with an empty `cmdline`: | build | unthrottled | `--cpus=0.04` (slow guest) | |---|---|---| | base `c465ff5a8` | `verified-cleanup`, teardown 70 ms | `incomplete`: `owned launcher exit could not be confirmed`, 2/2 runs (1.4–1.9 s) | | this PR | `verified-cleanup`, teardown 63 ms | `verified-cleanup`, 2/2 runs (stop returned after 1.6 s and 2.5 s) | The mid-exit row was `state=R`, `PF_EXITING` set and `cmdline` 0 bytes in every run. Repro evidence is also posted on the issue. - Remaining proof gaps: - No real Ghidra/JVM run. The Linux check used a large-heap `python3` launcher. - No macOS mid-exit run. - A teardown longer than the 1 s native budget plus the 1 s kill grace still fails closed by design. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:local -- src/process/ProviderProcess.launcherExit.test.ts`: 6 passed. Without the fix, 5 fail. - `npm run test:local --` all 216 test files under `src/{process,ghidra,hopper,android,firmware,javascript,kuna,native,apktool,sqlite,evm,providers,browser,wasm,signals,pcap,captures}`: 3211 passed, 14 skipped. - `npm run test:local --` the 4 `src/application`/`src/cli`/`src/mcp` test files that exercise `stop()` or `cleanup_incomplete`, plus the new file: 49 passed. - `npm run test:focused --` 40 boundary files: `tests/boundary/process/{binarySessionCleanupRetry,ghidraJavaLifecycle,jadxJavaLifecycle,jadxHashCleanup,ownedCommand,ownedCommandCancellation,ownedDebuggerSession,ghidraDecompilerVerifierCleanup}`, all `tests/boundary/providers/hopper/*`, the ghidra/native/kuna provider tests, `mcp/ghidraReopen`, sqlite and the javascript worker. Result: 455 passed, 6 skipped. - `npm run check:fast`: passed. - `npm run format:check`: passed. - The real Linux container repro above, against base and fix builds. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: real Linux kernel mid-exit was tested with `python3`. Ghidra, a real JVM and macOS were not run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new signal path. The second verification runs only after the supervisor sees its own child exit. Before that reap, the child holds the PID, so it cannot be reused. ## Author readiness Human owner: @kaoru0822-kitauji - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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. - [ ] 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-11-01 21:46:06 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:49 +00:00

[PR #7202] [CLOSED] docs(mcp): describe compact input advertisements as the default #7712

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

No description provided.

Rows
Columns