Skip to content

bun test: cover child-process behavior after directory scans - #43647

Open
lorenzozanee wants to merge 1 commit into
oven-sh:mainfrom
lorenzozanee:claude/asset-04-directory-fd-retention
Open

lorenzozanee wants to merge 1 commit into
oven-sh:mainfrom
lorenzozanee:claude/asset-04-directory-fd-retention

Conversation

@lorenzozanee

Copy link
Copy Markdown

What does PR do?

Extends the existing POSIX scanner directory-fd regression test to walk a near-OPEN_MAX tree and verify that node:child_process.spawnSync still launches /bin/echo with status 0 and ok\n. It adds coverage for the directory-fd retention scenario in #42077 while leaving the production repair in #40016 unchanged.

Fixes #42077

How did you verify code works?

  • git diff --check
  • bun test test/cli/test/bun-test.test.ts -t 'does not retain directory fds needed by child processes' (expected failure on released Bun 1.4.0: OPEN_FDS=10420; this confirms the regression guard catches the unfixed behavior)
  • bun bd could not start because this host has LLVM 21 and the repository requires Clang/LLVM 23.1.x; current-source execution remains for upstream CI.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6a476d18-b285-443b-9527-a435c0c08714

📥 Commits

Reviewing files that changed from the base of the PR and between 4353c91 and e70399d.

📒 Files selected for processing (1)
  • test/cli/test/bun-test.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

The scanner directory-fd test now creates 2,600 directories, runs unfiltered bun test, and verifies that spawnSync executes /bin/echo successfully while open file descriptors remain below 256.

Changes

Scanner descriptor regression

Layer / File(s) Summary
Large-tree spawn validation
test/cli/test/bun-test.test.ts
The test creates 2,600 directories and runs unfiltered bun test. The probe logs open descriptors and /bin/echo results. Assertions verify OPEN_FDS < 256, status 0, stdout "ok\n", and error null.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: testing child-process behavior after directory scans.
Description check ✅ Passed The description explains the change, linked issue, verification steps, and build limitation. The first heading differs slightly from the template, but the required information is present.
Linked Issues check ✅ Passed The change updates test/cli/test/bun-test.test.ts for #42077. It creates 2,600 nested directory paths, runs unfiltered bun test, checks that OPEN_FDS stays below 256, and verifies `spawnSync("/b…
Out of Scope Changes check ✅ Passed The diff changes only the targeted scanner regression test in test/cli/test/bun-test.test.ts. The added directory tree, child-process probe, and assertions directly support #42077. No unrelated prod…

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR. #42077 is now closed as already fixed: #40016 fixed it, and the fix shipped in Bun v1.4.1. So this PR is a test-only addition, and the Fixes #42077 line in the description no longer applies. The maintainers decide if they want the extra test coverage.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bun test retains one open directory fd per walked directory (part 2 of #32067)

2 participants