Repository navigation
fix(worker): close owned file handles and descriptors on exit - #184
Conversation
Track FileHandles and opt-in raw fs descriptors per Worker, then close them after pending filesystem work returns. Preserve parent ownership and unsupported postMessage transfer behavior. Adapts oven-sh#34260 by robobun.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db8e84ba88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| w.on("message", async () => { | ||
| // Wait for the pool to have opened them, then end the worker. If the | ||
| // worker's busy loop ends first, report that instead of polling forever. | ||
| while (openFds(ino) < 50 && !exited) await new Promise(r => setTimeout(r, 5)); |
There was a problem hiding this comment.
When filesystem jobs take longer than the worker's two-second busy loop, this polling loop stops as soon as exit fires and records fewer than 50 descriptors, causing the test to fail even if cleanup is correct. This makes the result depend on thread-pool scheduling and machine load; synchronize the worker with the parent or use a bounded condition mechanism instead of repeated setTimeout sleeps.
AGENTS.md reference: AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
|
|
||
| ### [`node:worker_threads`](https://nodejs.org/api/worker_threads.html) | ||
|
|
||
| Worker termination and natural exit close FileHandles opened by the Worker. Raw descriptors from `fs.open()` and `fs.openSync()` close by default; `trackUnmanagedFds: false` preserves them. Parent-owned descriptors remain open. FileHandle transfer through `postMessage()` remains unsupported. |
There was a problem hiding this comment.
This newly added paragraph says trackUnmanagedFds is implemented, but the same node:worker_threads section still says at line 388 that Bun ignores the option. Users therefore receive mutually exclusive compatibility guidance on one page; update the existing limitation entry as part of this behavior change.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
What does this PR do?
Close descriptors owned by a
node:worker_threadsWorker when it terminates or exits naturally. Without cleanup, repeated worker restarts retain file descriptors and can eventually exhaust the process descriptor limit.A worker-local registry records successful
fs.openresults and FileHandle ownership. It records the JavaScript-visible descriptor after native-to-CRT conversion on Windows, so explicit close and teardown use the same identity. Raw descriptors follow Node'strackUnmanagedFdsoption (default true, inherited by nested workers); FileHandles close even when raw tracking is disabled. Explicit close removes ownership, and canceled close work restores it only when the descriptor was previously owned. The teardown sweep runs after off-thread filesystem work returns, so an outstanding operation cannot use a descriptor number after cleanup reuses it. Parent descriptors and inherited stdio are never registered merely because a worker uses them.This adapts the open proposal oven-sh#34260 by robobun to the fork's current VM teardown and filesystem task ownership. Existing constructor-transfer bookkeeping is preserved; FileHandle transfer through
postMessage()remains unsupported. A regression asserts its existingDataCloneErrorand verifies the sender can still use and close the handle.How did you verify your code works?
Node.js 24.22.0 passes the portable oracle: termination and natural exit; raw sync/callback opens; FileHandles with tracking enabled or disabled; parent descriptors; multiple handles; explicit double close and subsequent parent allocation; nested tracking inheritance. These held-handle exits emit no warning in Node. Existing garbage-collection diagnostics are unchanged.
Native Linux before/after proof on the same base:
test/js/node/worker_threads/worker-track-unmanaged-fds.test.ts: 4 pass / 11 fail before; all 15 pass after. This includes raw-fd options, FileHandles, multiple/closed handles, natural-exit writes, pending closes/opens, and ReadStreams.test/js/node/worker_threadsandtest/js/node/fs: 1,259 pass / 115 skip. Four files required a separate run with--expose-internalsafter initial harness-import errors: 18 pass / 5 skip / 0 fail. Combined: 1,277 pass / 120 skip; no unresolved test failure.8c4133df4after correcting the libuv cancellation comparison. The final descriptor normalization passes both Linux x64 and macOS arm64 native build and compatibility CI. Native Windows runtime behavior was not exercised.The built-in TypeScript project typechecks, and all 26 CI-selection tests pass. Independent Codex and Claude reviews of the complete change are scoped-clean at P2. The final main-branch integration also has a clean Codex P2 review.
Final head
db8e84ba88ff6c0b55855ec38664e5b0f1d4dcd8passes Linux x64 and macOS arm64 native CI, including the new regression file on both platforms. The reports contain 51 successful Linux execution entries and 48 successful macOS entries. All final-head formatting, lint, type, and Rust checks pass. The opened-onlyclaude-find-issuesrun encountered the known missing CI credential on the initial head; it was not rerun.