[PR #2321] [MERGED] fix(process): release handles and close worker on incomplete cleanup #5162

Closed
opened 2026-10-31 01:04:27 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2321
Author: @viniciusdsandrade
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/cli-worker-incomplete-cleanup


📝 Commits (5)

  • a474837 fix(process): unreference handles on incomplete provider process stop
  • 56a45d7 fix(javascript): close worker and release handles on incomplete cleanup
  • e61e5a6 fix(javascript): defer worker stdin close until incomplete cleanup
  • 1c6d73a test(javascript): assert cleanup error instance on inspection uncertainty
  • e668102 fix(process): bound retry listeners and verify incomplete host cleanup

📊 Changes

5 files changed (+217 additions, -2 deletions)

View changed files

📝 src/javascript/analysis/JavaScriptAnalysisWorker.ts (+23 -2)
📝 src/process/ProviderProcess.ts (+22 -0)
📝 src/process/ProviderProcessOutput.test.ts (+37 -0)
📝 src/windows/WindowsOwnedProcess.ts (+14 -0)
📝 tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts (+121 -0)

📄 Description

Summary

Fixes #2290

Release event-loop handles in ProviderProcessSupervisor when process group stop reports incomplete cleanup, and close cooperative protocol stdin, detach listeners, and reset internal state in JavaScriptAnalysisWorker.#stop. This ensures that when process inspection fails closed under restrictive environments (such as denied /bin/ps process table access), the public CLI completes boundedly rather than hanging alive indefinitely while keeping cleanup uncertainty and retained resources intact.

Problem and expected behavior

Under restrictive sandbox policies where process inspection is rejected (e.g. setuid /bin/ps failing with spawn EPERM), cleanupOwnedProcessGroup fails closed and reports { cleaned: false, reason: "process table could not be inspected: spawn EPERM" }. REA correctly preserves honest uncertainty and emits cleanup_incomplete.

However:

  1. JavaScriptAnalysisWorker.#stop never closed cooperative protocol stdin (supervisor.launch.process.stdin?.end()), leaving the worker process javascriptAnalysisWorkerProcess.js alive indefinitely awaiting requests on process.stdin.
  2. ProviderProcessSupervisor did not unreference the child process or its stdio stream handles (stdin, stdout, stderr) when #stopWithin returned incomplete.
  3. In JavaScriptAnalysisWorker.#stop, stopped.status === "incomplete" returned before detaching the stdout data listener or resetting worker state.

Because active process handles and open stdio pipes remained referenced in Node's libuv event loop, the public CLI remained alive for minutes after outputting the cleanup_incomplete error report.

Expected behavior: the CLI completes within a bounded lifecycle after reporting cleanup_incomplete, without signaling unverified process groups or discarding retained ownership records.

Change and scope

  • In src/process/ProviderProcess.ts:
    • Add optional unref?() and ref?() to ProviderProcessHandle.
    • Add unref() and ref() methods to ProviderProcessSupervisor that propagate to the underlying child process handle and open stdio streams (stdin, stdout, stderr).
    • In ProviderProcessSupervisor.#stop: re-arm references during #stopWithin and unreference process and stream handles when cleanup reports status === "incomplete".
  • In src/windows/WindowsOwnedProcess.ts:
    • Add unref() and ref() methods and honor #unreferenced in #schedule(), ensuring unsettled job timers do not hold the event loop open.
  • In src/javascript/analysis/JavaScriptAnalysisWorker.ts:
    • In #stop: end cooperative protocol stdin (supervisor.launch.process.stdin?.end()) so the worker process can exit cleanly on EOF.
    • Detach the stdout data listener and reset internal worker state (#ready, #frames, #pending, #decoder, #protocolFailure) regardless of whether cleanup completed or reported incomplete.
    • Retain pendingCleanup and return honest ProviderCleanupError when stopped.status === "incomplete".
  • In src/process/ProviderProcessOutput.test.ts:
    • Add unit test verifying that ProviderProcessSupervisor.#stop unreferences process and stream handles when cleanup reports incomplete.
  • In tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts:
    • Add boundary test verifying that JavaScriptAnalysisWorker ends worker stdin, unreferences process handles, emits ProviderCleanupError when cleanup reports process inspection uncertainty, and allows subsequent clean retry once permitted.

Contract and boundary impact

  • Semantic owner and earliest changed stage: process lifecycle supervision in src/process/ProviderProcess.ts and worker lifecycle in src/javascript/analysis/JavaScriptAnalysisWorker.ts.
  • Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing process supervision and worker boundaries).
  • CLI and MCP/tool-catalog contract: unchanged schemas. Retains existing cleanup_incomplete error code and uncertainty details.
  • Provider, bridge, target-format, or platform compatibility: POSIX child processes and Windows owned jobs.
  • Evidence, artifact, provenance, or reconstruction contract: retained ownership records and partial evidence remain unmodified.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): preserves fail-closed inspection rule; unverified process groups are never signaled.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated:
    • src/process/ProviderProcessOutput.test.ts: verifies ProviderProcessSupervisor unreferences process and stdio handles on incomplete cleanup.
    • tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts: verifies JavaScriptAnalysisWorker ends stdin, unreferences process handles, and preserves ProviderCleanupError when cleanup reports process inspection uncertainty.
  • Base reproduction or other evidence: on the base revision, denied process table inspection leaves stdin open and stdio/process handles referenced, keeping the event loop alive indefinitely.
  • User-visible CLI/MCP output (if applicable): bounded CLI process completion after emitting cleanup_incomplete JSON report.
  • Remaining proof gaps: tests were verified by code review and static analysis; CI executes the test suite.

For evidence-bearing changes:

  • 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

  • No local test commands were run: verification is source-derived; CI executes the deterministic suite.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: none run.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: preserves fail-closed security rule (no signaling unverified PIDs) while ensuring bounded CLI completion.

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.

🔄 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/2321 **Author:** [@viniciusdsandrade](https://github.com/viniciusdsandrade) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/cli-worker-incomplete-cleanup` --- ### 📝 Commits (5) - [`a474837`](https://github.com/morluto/rea/commit/a4748375893832f93b4a5ace3db97bd775f485b1) fix(process): unreference handles on incomplete provider process stop - [`56a45d7`](https://github.com/morluto/rea/commit/56a45d7859331af2a461348127aed0c8dae5983c) fix(javascript): close worker and release handles on incomplete cleanup - [`e61e5a6`](https://github.com/morluto/rea/commit/e61e5a60b4943c64535252dbcbb4c02c3eae501e) fix(javascript): defer worker stdin close until incomplete cleanup - [`1c6d73a`](https://github.com/morluto/rea/commit/1c6d73a0a80b04599cd3fd21fc5bfc972cb1cec9) test(javascript): assert cleanup error instance on inspection uncertainty - [`e668102`](https://github.com/morluto/rea/commit/e668102dcb3fee6410f178ee0fffa270cbaf442a) fix(process): bound retry listeners and verify incomplete host cleanup ### 📊 Changes **5 files changed** (+217 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/javascript/analysis/JavaScriptAnalysisWorker.ts` (+23 -2) 📝 `src/process/ProviderProcess.ts` (+22 -0) 📝 `src/process/ProviderProcessOutput.test.ts` (+37 -0) 📝 `src/windows/WindowsOwnedProcess.ts` (+14 -0) 📝 `tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts` (+121 -0) </details> ### 📄 Description ## Summary Fixes #2290 Release event-loop handles in `ProviderProcessSupervisor` when process group stop reports incomplete cleanup, and close cooperative protocol `stdin`, detach listeners, and reset internal state in `JavaScriptAnalysisWorker.#stop`. This ensures that when process inspection fails closed under restrictive environments (such as denied `/bin/ps` process table access), the public CLI completes boundedly rather than hanging alive indefinitely while keeping cleanup uncertainty and retained resources intact. ## Problem and expected behavior Under restrictive sandbox policies where process inspection is rejected (e.g. setuid `/bin/ps` failing with `spawn EPERM`), `cleanupOwnedProcessGroup` fails closed and reports `{ cleaned: false, reason: "process table could not be inspected: spawn EPERM" }`. REA correctly preserves honest uncertainty and emits `cleanup_incomplete`. However: 1. `JavaScriptAnalysisWorker.#stop` never closed cooperative protocol `stdin` (`supervisor.launch.process.stdin?.end()`), leaving the worker process `javascriptAnalysisWorkerProcess.js` alive indefinitely awaiting requests on `process.stdin`. 2. `ProviderProcessSupervisor` did not unreference the child process or its stdio stream handles (`stdin`, `stdout`, `stderr`) when `#stopWithin` returned `incomplete`. 3. In `JavaScriptAnalysisWorker.#stop`, `stopped.status === "incomplete"` returned before detaching the `stdout` data listener or resetting worker state. Because active process handles and open stdio pipes remained referenced in Node's libuv event loop, the public CLI remained alive for minutes after outputting the `cleanup_incomplete` error report. Expected behavior: the CLI completes within a bounded lifecycle after reporting `cleanup_incomplete`, without signaling unverified process groups or discarding retained ownership records. ## Change and scope - In `src/process/ProviderProcess.ts`: - Add optional `unref?()` and `ref?()` to `ProviderProcessHandle`. - Add `unref()` and `ref()` methods to `ProviderProcessSupervisor` that propagate to the underlying child process handle and open stdio streams (`stdin`, `stdout`, `stderr`). - In `ProviderProcessSupervisor.#stop`: re-arm references during `#stopWithin` and unreference process and stream handles when cleanup reports `status === "incomplete"`. - In `src/windows/WindowsOwnedProcess.ts`: - Add `unref()` and `ref()` methods and honor `#unreferenced` in `#schedule()`, ensuring unsettled job timers do not hold the event loop open. - In `src/javascript/analysis/JavaScriptAnalysisWorker.ts`: - In `#stop`: end cooperative protocol `stdin` (`supervisor.launch.process.stdin?.end()`) so the worker process can exit cleanly on EOF. - Detach the `stdout` data listener and reset internal worker state (`#ready`, `#frames`, `#pending`, `#decoder`, `#protocolFailure`) regardless of whether cleanup completed or reported incomplete. - Retain `pendingCleanup` and return honest `ProviderCleanupError` when `stopped.status === "incomplete"`. - In `src/process/ProviderProcessOutput.test.ts`: - Add unit test verifying that `ProviderProcessSupervisor.#stop` unreferences process and stream handles when cleanup reports incomplete. - In `tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts`: - Add boundary test verifying that `JavaScriptAnalysisWorker` ends worker `stdin`, unreferences process handles, emits `ProviderCleanupError` when cleanup reports process inspection uncertainty, and allows subsequent clean retry once permitted. ## Contract and boundary impact - Semantic owner and earliest changed stage: process lifecycle supervision in `src/process/ProviderProcess.ts` and worker lifecycle in `src/javascript/analysis/JavaScriptAnalysisWorker.ts`. - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing process supervision and worker boundaries). - CLI and MCP/tool-catalog contract: unchanged schemas. Retains existing `cleanup_incomplete` error code and uncertainty details. - Provider, bridge, target-format, or platform compatibility: POSIX child processes and Windows owned jobs. - Evidence, artifact, provenance, or reconstruction contract: retained ownership records and partial evidence remain unmodified. - Target selection, authority, upstream behavior, and cleanup details (if applicable): preserves fail-closed inspection rule; unverified process groups are never signaled. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects: - [ ] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: - `src/process/ProviderProcessOutput.test.ts`: verifies `ProviderProcessSupervisor` unreferences process and stdio handles on incomplete cleanup. - `tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts`: verifies `JavaScriptAnalysisWorker` ends `stdin`, unreferences process handles, and preserves `ProviderCleanupError` when cleanup reports process inspection uncertainty. - Base reproduction or other evidence: on the base revision, denied process table inspection leaves `stdin` open and stdio/process handles referenced, keeping the event loop alive indefinitely. - User-visible CLI/MCP output (if applicable): bounded CLI process completion after emitting `cleanup_incomplete` JSON report. - Remaining proof gaps: tests were verified by code review and static analysis; CI executes the test suite. For evidence-bearing changes: - [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 - No local test commands were run: verification is source-derived; CI executes the deterministic suite. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: preserves fail-closed security rule (no signaling unverified PIDs) while ensuring bounded CLI completion. ## 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. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and 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 01:04:27 +00:00
Sign in to join this conversation.
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#5162
No description provided.