Repository navigation
Conversation
4d71562 to
8481762
Compare
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Use close-on-exec for standard descriptor duplicates crates/perry-runtime/src/child_process/options.rs, crates/perry-runtime/src/child_process/reactor/stdin.rs, crates/perry-runtime/src/fs/mod.rs |
These Unix code paths now duplicate descriptors with F_DUPFD_CLOEXEC instead of dup. |
Probe descriptors in child processes test-files/test_gap_std_fd_dup_cloexec.ts, changelog.d/13613-std-fd-dup-cloexec.md |
The test probes descriptors 3–9 in piped and detached children. The changelog describes standard-stream inheritance and its effect on EOF detection. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: proggeramlug
Merge Risk: 
84817
-
No actionable merge-blocking risk is established for this change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the main change: the std fd registry duplicates are now created close-on-exec. It matches the raw summary and the objectives, which describe switching the duplicates to F_DUPFD_CLOEXEC… Description check The description follows the repository template. It has a Summary that states the problem and the fix, a Changes list with file paths, and a Related issue section that states n/a with a reason. The Te… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: the std fd registry duplicates are now created close-on-exec. It matches the raw summary and the objectives, which describe switching the duplicates to F_DUPFD_CLOEXEC. It uses the conventional fix(scope): prefix and is specific enough for a reader scanning history.
Full details: Description check
-
Explanation
The description follows the repository template. It has a Summary that states the problem and the fix, a Changes list with file paths, and a Related issue section that states n/a with a reason. The Test plan lists the checkboxes and reports the gap test results and the one failing existing test. The Checklist is complete. The two unchecked build and test boxes are explained as deferred to CI. The Screenshots section is optional and omitted, which is acceptable.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Summary
The runtime kept duplicates of the process's stdin, stdout and stderr (and of a child's stdin pipe while draining it) created with plain
dup(), without close-on-exec. Every child process spawned afterwards inherited those extra descriptors, including detached children started withstdio: "ignore". A detached daemon then held the parent's stdout and stderr pipes open, so whoever reads the parent's output never saw EOF after the parent exited. Node never leaks these descriptors into children. The duplicates are now created withF_DUPFD_CLOEXEC.Changes
crates/perry-runtime/src/fs/mod.rs(std_fd_registry): duplicate fds 0-2 withfcntl(F_DUPFD_CLOEXEC).crates/perry-runtime/src/child_process/reactor/stdin.rs(cp_spawn_stdin_drain): same for the long-lived duplicate of a child's stdin pipe.crates/perry-runtime/src/child_process/options.rs(cp_stdio_from_fd): same for the transient duplicate made while building a child's stdio, which a concurrent spawn could inherit.test-files/test_gap_std_fd_dup_cloexec.ts. It writes to fds 1 and 2 first, then checks that fds 3-9 are closed in a piped child and in a detachedstdio: "ignore"child.changelog.d/13613-std-fd-dup-cloexec.md.Related issue
n/a. Found while compiling the Supabase CLI with Perry.
Test plan
The new
test-files/test runs in the gap suite, which compares its output against Node; it passes on my fork, along with the existing child_process and fs fd gap tests (run).test_issue_1934_spawn_reactor.tsfails in that run with anexit/endorder difference; it fails the same way on the base commit without this change.The full
cargo build --releaseandcargo test --workspaceruns are left to this PR's CI, so those two boxes are not ticked.cargo fmton the touched Rust files is clean.cargo build --releasecleancargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowspassestest-files/or a#[test]in the affected cratedocs/src/-p perry-ui-<backend>locally on that platformChecklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit