fix: reap test writers before buffer-overrun teardown - #110
Merged
Conversation
Reviewer's GuideThe PR fixes a delegated-writer race by making the fake pm wrapper reap and fully capture its real process before exposing output to an outer buffer limit, adds a real-process regression, isolates negative fixtures from global settings and telemetry, and updates cleanup documentation and release tracking. Sequence diagram for delegated output capture before teardownsequenceDiagram
participant Outer as Outer command
participant Wrapper as Fake pm wrapper
participant Child as Delegated process
participant Tracker as Tracker history
Outer->>Wrapper: Request delegated command
Wrapper->>Child: Start delegated process
Child->>Tracker: Write tracker history
Outer->>Wrapper: Enforce output buffer limit
Wrapper->>Child: Wait for process exit
Child-->>Wrapper: Exit after final write
Wrapper-->>Outer: Forward fully captured output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Summary by CodeRabbit
WalkthroughThe test wrapper now captures delegated ChangesDelegated writer race
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The test wrapper now prevents delegated writers from surviving buffer-overrun handling, with regression coverage for completion and process reaping. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant Test
participant FakePmWrapper
participant RealPm
participant WriterProgram
Test->>FakePmWrapper: Spawn with 16-byte maxBuffer
FakePmWrapper->>RealPm: Spawn with captured output
RealPm->>WriterProgram: Run delegated writer
WriterProgram-->>RealPm: Write chunk and completion marker
RealPm-->>FakePmWrapper: Exit with captured output
FakePmWrapper->>FakePmWrapper: Forward output with writeSync
Test-->>FakePmWrapper: Observe ENOBUFS after writer exit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
The guide correctly identifies the delegated-process lifetime boundary and test isolation. The real-process regression verifies completion and ESRCH before cleanup. |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/atomic.test.ts" line_range="298" />
<code_context>
- "const r = spawnSync(realPm, args, { stdio: 'inherit' });",
+ // Do not expose output until the real process exits. Otherwise an outer
+ // capture overrun kills this wrapper while real pm is still writing history.
+ "const r = spawnSync(realPm, args, { encoding: 'utf-8', maxBuffer: 64 * 1024 * 1024 });",
+ "if (r.stdout) writeSync(1, r.stdout);",
+ "if (r.stderr) writeSync(2, r.stderr);",
"process.exit(r.status == null ? 1 : r.status);",
</code_context>
<issue_to_address>
**issue (bug_risk):** The fake wrapper still terminates the delegated `pm` process when either stream exceeds its own 64 MiB `maxBuffer`; it then forwards only the truncated output and exits with a synthetic status. A real command producing more than 64 MiB therefore remains vulnerable to the same delegated-writer teardown race and can be misclassified by the outer test capture.
**Triggers:** When the delegated command emits more than 64 MiB on stdout or stderr.
**Suggested fix:** Use a sufficiently unbounded capture strategy, or explicitly drain the delegated process streams without imposing a smaller buffer cap before forwarding the bytes.
```suggestion
"const r = spawnSync(realPm, args, { encoding: 'utf-8', maxBuffer: Infinity });",
```
</issue_to_address>
### Comment 2
<location path="test/atomic.test.ts" line_range="1" />
<code_context>
+import "./support/isolated-environment.ts";
+
import assert from "node:assert/strict";
</code_context>
<issue_to_address>
**issue (bug_risk):** The isolation module is imported with a static ESM import, but static dependencies such as `../index.ts` and `@unbrained/pm-cli/sdk` are evaluated before the imported module's top-level body runs. Any host SDK initialization that reads global tracker or observability settings during module evaluation therefore sees the developer's original environment, not the values assigned by `isolated-environment.ts`.
**Triggers:** When the host SDK reads `PM_GLOBAL_PATH` or telemetry/Sentry settings during module initialization.
**Suggested fix:** Set the environment in the test runner before loading static dependencies, or replace the host SDK imports with dynamic imports performed after the isolation module has initialized.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: test/atomic.test.ts:298, test/atomic.test.ts:1
Greptile SummaryThe PR prevents the fake
Confidence Score: 5/5The PR appears safe to merge; the delegated-writer lifetime race is covered without introducing an actionable regression. The wrapper now waits for delegated process completion before exposing output to the outer capture limit, environment mutations are restored, and teardown occurs only after relevant child processes have exited.
|
| Filename | Overview |
|---|---|
| test/atomic.test.ts | Buffers delegated output until process completion and adds a regression proving the delegated writer finishes and is reaped before teardown. |
| test/support/isolated-environment.ts | Creates disposable global tracker state and disables inherited production observability for negative subprocess fixtures. |
| test/buffer-regression.test.ts | Imports the isolated test environment before executing buffer-overrun fixtures. |
| test/support/remove-tree.ts | Clarifies that retrying filesystem removal does not itself establish process termination. |
Sequence Diagram
sequenceDiagram
participant Test as Outer test capture
participant Wrapper as Fake pm wrapper
participant PM as Delegated process
participant FS as Tracker history
Test->>Wrapper: spawnSync(pm), maxBuffer 16
Wrapper->>PM: spawnSync(real command), capture output
PM->>FS: complete history writes
PM-->>Wrapper: exit and captured output
Wrapper-->>Test: forward captured output
Note over Test,Wrapper: Outer ENOBUFS can occur only after PM exits
Test->>FS: safely remove fixture
Reviews (1): Last reviewed commit: "fix: reap test writers before buffer-ove..." | Re-trigger Greptile
Owner
Author
|
Both findings were checked and answered inline. The inner synchronous call waits for its direct child even on its own overrun, and the separate first ESM dependency executes before later sibling dependencies. Neither finding demonstrates a regression in this patch. |
Owner
Author
|
Completed review for f9c13a7 acknowledged. No actionable findings remain. Both hosted Node matrix jobs passed all 225 tests with exact 100% coverage, including the process-lifetime regression. |
Owner
Author
|
The requested review completed and its current-head result has been checked. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an outer command capture exceeds its output limit, the fake pm wrapper could exit while its delegated process was still writing tracker history. Capture the delegated output until that process has exited, then forward it. Isolate negative fixtures from global settings and production observability.
A real-process regression fails with the old wrapper and passes with the fix on Node 22.23.2 and Node 26.7.0. Full coverage passes all 225 tests with exact 100% line, branch, and function coverage; static checks, all three linked verification plans, production audit, and package dry run pass.
Tracked in pm-csv-yriu. This fixes a demonstrated test-writer race relevant to the CI history-write error discussed in unbraind/pm-cli#1209. The precise historical ENOENT was not reproduced, so that attribution remains an inference.
Final head f9c13a7: both hosted Node matrix jobs, CodeQL, Semgrep, DeepScan, and static checks pass. CodeRabbit completed with no actionable findings; Greptile reviewed all seven files with no comments. Sourcery findings about inner-child reaping and ESM dependency evaluation were disproved with real-process checks and answered inline; all threads are resolved. Cubic skipped review and is not counted as approval.