Create sockets with WSA_FLAG_NO_HANDLE_INHERIT on Windows - #36938
Conversation
On Windows, plain socket() and accept() return inheritable handles, so a child spawned with bInheritHandles=TRUE (any node:child_process spawn with stdio) duplicated every live socket, including listen sockets. A detached child that outlived the parent kept the parent's port in LISTENING state even after server.close() and parent exit. Create sockets with WSASocketW(..., WSA_FLAG_OVERLAPPED | WSA_FLAG_NO_HANDLE_INHERIT) and clear HANDLE_FLAG_INHERIT on accepted sockets, matching libuv. Fixes #36936
WalkthroughChangesThe Windows socket implementation now creates non-inheritable sockets and clears inheritance on accepted sockets. A Windows-only regression test verifies that a detached child does not keep the parent HTTP server’s port open. Windows socket inheritance
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-usockets/src/bsd.c`:
- Around line 890-894: Update the _WIN32 handling after accept() to check the
result of SetHandleInformation; on failure, save GetLastError(), close
accepted_fd, restore the saved error with SetLastError(), and return
LIBUS_SOCKET_ERROR so an inheritable socket is never passed onward.
In `@test/js/node/child_process/child-process-socket-inherit.test.ts`:
- Around line 41-44: Reorder the assertions in the subprocess test so the JSON
parsing and validation of stdout via JSON.parse occur after the stderr assertion
but before expect(exitCode).toBe(0). Keep the existing stdout fixture validation
and exit-code expectation unchanged apart from their ordering.
- Around line 59-61: Update the child cleanup try/catch around
process.kill(childPid) to ignore only ESRCH, and rethrow any other error so
unexpected cleanup failures are reported.
- Around line 22-30: Extend the socket inheritance test around the server
connection flow to add a Windows-specific case that accepts a client connection
before spawning the detached child. After the parent exits, verify the detached
child does not retain or keep the accepted socket usable, while preserving the
existing listener-inheritance coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80354e4f-88cd-4bdf-a0e7-e49af47a0924
📒 Files selected for processing (2)
packages/bun-usockets/src/bsd.ctest/js/node/child_process/child-process-socket-inherit.test.ts
| #ifdef _WIN32 | ||
| /* accept() returns an inheritable handle regardless of the listening | ||
| * socket's flags; keep it out of spawned children. */ | ||
| SetHandleInformation((HANDLE) accepted_fd, HANDLE_FLAG_INHERIT, 0); | ||
| #endif |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target function context ---'
sed -n '820,925p' packages/bun-usockets/src/bsd.c
printf '%s\n' '--- accepted_fd and LIBUS_SOCKET_ERROR references ---'
rg -n -C 3 'accepted_fd|LIBUS_SOCKET_ERROR|SetHandleInformation|accept\(' packages/bun-usockets/src/bsd.c packages/bun-usockets -g '*.{c,h,cc,cpp,hpp}'
printf '%s\n' '--- repository guidance ---'
find .. -name REVIEW.md -o -name '*landing*guidance*' 2>/dev/null | head -50Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- socket close and Windows error helpers ---'
rg -n -C 8 'bsd_close_socket|`#define`.*LIBUS_SOCKET_ERROR|typedef.*LIBUS_SOCKET_DESCRIPTOR|LIBUS_SOCKET_DESCRIPTOR' packages/bun-usockets/src/bsd.c packages/bun-usockets/src/libusockets.h | head -240
printf '%s\n' '--- callers of bsd_accept_socket ---'
rg -n -C 12 'bsd_accept_socket\s*\(' packages/bun-usockets/src -g '*.{c,h}'
printf '%s\n' '--- relevant Windows includes and error handling ---'
sed -n '1,120p' packages/bun-usockets/src/bsd.c
sed -n '320,390p' packages/bun-usockets/src/bsd.c
sed -n '700,770p' packages/bun-usockets/src/bsd.cRepository: oven-sh/bun
Length of output: 35768
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- close implementation and platform error conventions ---'
sed -n '430,475p' packages/bun-usockets/src/bsd.c
printf '%s\n' '--- accept-loop ownership and error handling ---'
sed -n '480,585p' packages/bun-usockets/src/loop.c
printf '%s\n' '--- Windows-specific socket creation and inheritance references ---'
rg -n -C 5 'NO_HANDLE_INHERIT|HANDLE_INHERIT|SetHandleInformation|GetLastError|WSASetLastError|bsd_close_socket\(' packages/bun-usockets/src -g '*.{c,h}'
printf '%s\n' '--- git metadata for the target lines ---'
git blame -L 885,905 -- packages/bun-usockets/src/bsd.cRepository: oven-sh/bun
Length of output: 50367
🌐 Web query:
Microsoft documentation SetHandleInformation HANDLE_FLAG_INHERIT accepted socket accept Windows handle inheritance closesocket GetLastError
💡 Result:
While Windows documentation lists sockets among the objects supported by SetHandleInformation, attempting to manage socket handle inheritance via SetHandleInformation with the HANDLE_FLAG_INHERIT flag is often unreliable and frequently ineffective due to the architecture of Windows Sockets (Winsock) [1][2][3][4]. The primary issue is the involvement of Layered Service Providers (LSPs), which sit between your application and the underlying Winsock stack [2][5][6]. Even if you successfully mark a socket handle as non-inheritable using SetHandleInformation, the underlying handle used by the LSP may still be marked inheritable, causing child processes to inadvertently receive access to the socket [2]. Furthermore, even when inheritance is technically avoided, inherited socket handles often cannot be used in a child process because the LSP was never informed of the handle duplication, leading to errors such as WSAENOTSOCK (10038) or invalid handle errors [5]. To reliably prevent socket handle inheritance, consider the following approaches: 1. Use WSA_FLAG_NO_HANDLE_INHERIT: When creating a socket, use the WSASocket function with the WSA_FLAG_NO_HANDLE_INHERIT flag (available on Windows Vista/Server 2008 and later). This is the most reliable way to ensure a socket is not inherited [2]. 2. Use Explicit Handle Lists: Instead of setting bInheritHandles to TRUE in CreateProcess (which exposes all inheritable handles), you can use the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute with UpdateProcThreadAttribute. This allows you to provide an explicit list of specific handles to be inherited, ensuring only the intended handles are passed to the child process [7][8]. 3. Avoid Relying on Inheritance for IPC: Because of the complexities surrounding socket inheritance, it is generally recommended to avoid using socket handle inheritance for interprocess communication. Instead, use explicit IPC mechanisms such as Named Pipes or sockets created with explicit non-inheritance flags [5]. Regarding closesocket and GetLastError: If you encounter an error with a socket handle—potentially because an inherited handle has become invalid—calling closesocket will return SOCKET_ERROR, and you can retrieve the specific error code using WSAGetLastError (or GetLastError, though WSAGetLastError is preferred for Winsock-specific errors) [5][6]. However, if the handle has been closed or invalidated by the system due to the inheritance issues described above, the error returned will typically indicate that the handle is no longer valid [5].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-sethandleinformation
- 2: https://stackoverflow.com/questions/12058911/can-tcp-socket-handles-be-set-not-inheritable
- 3: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/handleapi/nf-handleapi-sethandleinformation.md
- 4: https://learn.microsoft.com/en-us/windows/win32/sysinfo/handle-inheritance
- 5: https://stackoverflow.com/questions/11847793/are-tcp-socket-handles-inheritable
- 6: https://learn.microsoft.com/en-us/windows/win32/winsock/socket-handles-2
- 7: https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
- 8: https://learn.microsoft.com/en-us/windows/win32/procthread/inheritance
🌐 Web query:
site:learn.microsoft.com Winsock accept accepted socket handle inheritance WSA_FLAG_NO_HANDLE_INHERIT accept
💡 Result:
In Winsock, socket handles are inheritable by default when created via the socket or WSASocket functions [1][2]. The WSA_FLAG_NO_HANDLE_INHERIT flag (0x80) can be passed to the WSASocket function to create a socket that is explicitly non-inheritable [1][2]. Regarding connection acceptance: 1. The standard accept function creates and returns a new socket handle for an incoming connection [3][4]. This newly created handle generally inherits the inheritance property of the listening socket or follows the default behavior of being inheritable [1][2]. 2. The WSAAccept function, which allows for conditional acceptance, also returns a new socket handle [5][6]. 3. If you require a non-inheritable socket handle for a new connection accepted via these functions, you cannot directly pass a "non-inheritance" flag to accept or WSAAccept. Instead, you must use the SetHandleInformation function from the Windows API after the socket has been accepted to modify the handle's inheritance attribute (specifically, by clearing the HANDLE_FLAG_INHERIT flag). 4. For high-performance scenarios using AcceptEx, the socket passed into the sAcceptSocket parameter must be created beforehand using WSASocket [7]. You can (and should) use the WSA_FLAG_NO_HANDLE_INHERIT flag during the creation of this sAcceptSocket to ensure it is non-inheritable before it is even used in the AcceptEx operation [1][7][2].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasocketw
- 2: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasocketa
- 3: https://learn.microsoft.com/en-us/windows/win32/api/Winsock2/nf-winsock2-accept
- 4: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-accept
- 5: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsaaccept
- 6: https://learn.microsoft.com/en-us/windows/win32/winsock/connection-setup-and-teardown-2
- 7: https://learn.microsoft.com/en-us/windows/win32/api/mswsock/nf-mswsock-acceptex
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("packages/bun-usockets/src/bsd.c").read_text()
start = source.index("LIBUS_SOCKET_DESCRIPTOR bsd_accept_socket")
end = source.index("\nssize_t bsd_recv", start)
function = source[start:end]
checks = {
"SetHandleInformation is unchecked": "SetHandleInformation((HANDLE) accepted_fd, HANDLE_FLAG_INHERIT, 0);" in function
and "!SetHandleInformation" not in function,
"accepted socket is returned after the unchecked call": "return accepted_fd;" in function[function.index("SetHandleInformation"):],
"Windows cleanup uses closesocket": "void bsd_close_socket" in source
and "closesocket(fd);" in source[source.index("void bsd_close_socket"):source.index("void bsd_close_socket") + 180],
"accepted socket is closed before retry only for the unrelated Apple path": "bsd_close_socket(accepted_fd);" in function
and function.count("bsd_close_socket(accepted_fd);") == 1,
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
print("CALLER_SENTINEL_ONLY:")
loop = Path("packages/bun-usockets/src/loop.c").read_text()
call_start = loop.index("LIBUS_SOCKET_DESCRIPTOR client_fd = bsd_accept_socket")
call_end = loop.index("case POLL_TYPE_SOCKET_SHUT_DOWN", call_start)
print(loop[call_start:call_end].splitlines()[0])
print(next(line.strip() for line in loop[call_start:call_end].splitlines()
if "if (client_fd == LIBUS_SOCKET_ERROR)" in line))
print("No bsd_accept_socket error out-parameter is present in its declaration.")
PYRepository: oven-sh/bun
Length of output: 576
Fail closed when SetHandleInformation fails. Save GetLastError(), close accepted_fd, restore the error after closesocket, and return LIBUS_SOCKET_ERROR; do not pass an accepted socket with inheritance still enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bun-usockets/src/bsd.c` around lines 890 - 894, Update the _WIN32
handling after accept() to check the result of SetHandleInformation; on failure,
save GetLastError(), close accepted_fd, restore the saved error with
SetLastError(), and return LIBUS_SOCKET_ERROR so an inheritable socket is never
passed onward.
Source: Coding guidelines
| // Spawn a detached child while the listen socket is open, then exit. | ||
| const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 100000)"], { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| windowsHide: true, | ||
| }); | ||
| child.unref(); | ||
|
|
||
| await new Promise((resolve, reject) => server.close(error => (error ? reject(error) : resolve()))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate test files ---'
git ls-files 'test/js/node/child_process/child-process-socket-inherit.test.ts' 'packages/bun-usockets/src/bsd.c' 'REVIEW.md' '*landing*' | sed -n '1,80p'
printf '%s\n' '--- test outline ---'
ast-grep outline test/js/node/child_process/child-process-socket-inherit.test.ts --view expanded
printf '%s\n' '--- test source ---'
cat -n test/js/node/child_process/child-process-socket-inherit.test.ts
printf '%s\n' '--- bsd.c target area ---'
sed -n '850,930p' packages/bun-usockets/src/bsd.c
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C 4 'bsd_accept_socket|accept_socket|child-process-socket-inherit|socket.inherit|inherit' \
test/js/node/child_process packages/bun-usockets/src packages/bun-usockets 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- repository guidance ---'
if [ -f REVIEW.md ]; then cat REVIEW.md; fiRepository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant history and diff metadata ---'
git status --short
git diff --stat
git diff -- test/js/node/child_process/child-process-socket-inherit.test.ts packages/bun-usockets/src/bsd.c | sed -n '1,260p'
printf '%s\n' '--- adjacent child-process socket tests ---'
rg -n -C 8 'server\.listen|server\.close|spawn\(|ECONNREFUSED|accept|connection' \
test/js/node/child_process -g '*.test.ts' | sed -n '1,320p'Repository: oven-sh/bun
Length of output: 27761
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
test = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
bsd = Path("packages/bun-usockets/src/bsd.c").read_text()
parent = test.split('"parent.mjs": `', 1)[1].split('`,', 1)[0]
spawn_at = parent.index("spawn(process.execPath")
close_at = parent.index("server.close")
parent_connects = bool(re.search(r"\b(?:connect|fetch|http\.request|https\.request)\s*\(", parent[:spawn_at]))
test_connect_at = test.index("const socket = connect")
parent_exit_marker = test.index("const { port, childPid }")
print({
"parent_connects_before_spawn": parent_connects,
"parent_closes_server_after_spawn": close_at > spawn_at,
"external_connect_occurs_after_parent_output_is_read": test_connect_at > parent_exit_marker,
"windows_accepted_handle_guard_present": bool(
re.search(
r"`#ifdef` _WIN32\s*/\* accept\(\).*?SetHandleInformation"
r"\(\(HANDLE\) accepted_fd,\s*HANDLE_FLAG_INHERIT,\s*0\);",
bsd,
re.S,
)
),
"cleanup_swallows_process_kill_errors": bool(
re.search(r"try\s*\{\s*process\.kill\(childPid\);\s*\}\s*catch\s*\{\s*\}", test, re.S)
),
})
PY
printf '%s\n' '--- cross-platform landing guidance ---'
rg -n -A 45 -B 5 'Cross-platform|platform-sensitive|Windows' .claude/docs/landing-prs.md | sed -n '1,180p'Repository: oven-sh/bun
Length of output: 24602
🌐 Web query:
site:github.com/oven-sh/bun/issues/36936 socket inherit Windows accepted socket
💡 Result:
GitHub Issue #36936: Windows: detached child inherits listening socket and keeps port open after parent exits.
- Status: Open; no assignee, labels, or linked PRs yet.
- Reported: August 5, 2026.
- Affected: Bun
1.4.0-canary.1+81dac417bon Windows. - Problem: A detached child process inherits the parent’s HTTP listening socket, so the port remains in
LISTENINGstate after the parent callsserver.close()and exits. - Likely cause: The Windows socket/handle is incorrectly marked inheritable.
- Comparison: The same reproduction reportedly works correctly under Node.js 25.8.1. [1]
[1] (github.com)
Citations:
Add coverage for accepted socket inheritance.
The current test connects only after the parent exits, so it does not execute bsd_accept_socket. Add a Windows case that accepts a connection before spawning the detached child and asserts that the child does not retain the accepted socket after the parent exits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/child_process/child-process-socket-inherit.test.ts` around lines
22 - 30, Extend the socket inheritance test around the server connection flow to
add a Windows-specific case that accepts a client connection before spawning the
detached child. After the parent exits, verify the detached child does not
retain or keep the accepted socket usable, while preserving the existing
listener-inheritance coverage.
Source: Coding guidelines
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| const { port, childPid } = JSON.parse(stdout.trim()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate stdout before the exit code.
Line 43 asserts the exit code before Line 44 validates the fixture output. Parse and validate stdout after the stderr assertion and before the exit-code assertion.
Based on learnings, subprocess tests must assert stdout and stderr before the exit code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/child_process/child-process-socket-inherit.test.ts` around lines
41 - 44, Reorder the assertions in the subprocess test so the JSON parsing and
validation of stdout via JSON.parse occur after the stderr assertion but before
expect(exitCode).toBe(0). Keep the existing stdout fixture validation and
exit-code expectation unchanged apart from their ordering.
Sources: Coding guidelines, Learnings
| try { | ||
| process.kill(childPid); | ||
| } catch {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
target='test/js/node/child_process/child-process-socket-inherit.test.ts'
printf '%s\n' '--- target outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline "$target" --view expanded || true
fi
printf '%s\n' '--- target context ---'
cat -n "$target" | sed -n '1,130p'
printf '%s\n' '--- nearby files ---'
fd -i 'child.*process.*\.test\.(ts|tsx|js|jsx)$' test/js/node/child_process 2>/dev/null | head -40 || true
printf '%s\n' '--- kill cleanup patterns ---'
rg -n -C 5 'process\.kill|catch\s*\{\s*\}' test/js/node/child_process test 2>/dev/null | head -240 || true
printf '%s\n' '--- review guidance ---'
for f in REVIEW.md test/REVIEW.md; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: oven-sh/bun
Length of output: 44502
🏁 Script executed:
#!/bin/bash
set -eu
target='test/js/node/child_process/child-process-socket-inherit.test.ts'
cat -n "$target" | sed -n '1,130p'
rg -n -C 5 'process\.kill|childPid|detached|unref' test/js/node/child_process test 2>/dev/null | head -260Repository: oven-sh/bun
Length of output: 21105
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- process.kill implementations and bindings ---'
rg -n -C 8 'process\.kill|kill\(.*pid|ERRNO.*ESRCH|ESRCH|EPERM' src packages test/js | head -400
printf '%s\n' '--- platform-specific kill helpers ---'
rg -n -C 6 'killProcess|terminateProcess|TerminateProcess|kill.*process|process.*kill' src | head -400
printf '%s\n' '--- process.kill type/docs references ---'
rg -n -C 5 'kill\s*\(' packages/bun-types src/js test/js/node/child_process | head -300Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 8 'process\.kill|kill\(.*pid|ERRNO.*ESRCH|ESRCH|EPERM' src packages test/js | head -400
rg -n -C 6 'killProcess|terminateProcess|TerminateProcess|kill.*process|process.*kill' src | head -400Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- child-process kill implementation ---'
sed -n '1535,1605p' src/js/node/child_process.ts
printf '%s\n' '--- global process.kill references ---'
rg -n -C 12 'kill\s*:\s*|kill\(pid|fn.*kill|process_kill|uv_kill' src/js src | head -260
printf '%s\n' '--- direct uv_kill implementation references ---'
rg -n -C 10 'uv_kill|pub fn kill|fn kill' src/spawn src/js src/bun.js src | head -260Repository: oven-sh/bun
Length of output: 35116
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1535,1605p' src/js/node/child_process.ts
rg -n -C 12 'kill\s*:\s*|kill\(pid|process_kill|uv_kill' src/js src | head -260
rg -n -C 10 'uv_kill|pub fn kill|fn kill' src/spawn src/js src | head -260Repository: oven-sh/bun
Length of output: 32407
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- process module files ---'
fd -i 'process' src/js src | head -120
printf '%s\n' '--- kill declarations in likely process modules ---'
for f in $(fd -i 'process' src/js src | head -80); do
if rg -q 'kill|Process' "$f" 2>/dev/null; then
printf '%s\n' "--- $f ---"
rg -n -C 5 'kill|Process' "$f" | head -100
fi
done
printf '%s\n' '--- read-only policy verifier ---'
python3 - <<'PY'
from pathlib import Path
kill_impl = Path("src/spawn/process.rs").read_text()
target = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
errno_map = Path("src/errno/windows_errno.rs").read_text()
assert 'if errno_ != bun_sys::E::ESRCH' in kill_impl
assert 'if err.errno != bun_sys::E::ESRCH as u16' in kill_impl
assert 'process.kill(childPid)' in target
assert 'catch {}' in target
assert 'EPERM = 1' in errno_map
print("target uses broad cleanup suppression: yes")
print("implementation treats only ESRCH as benign in POSIX and Windows child kill paths: yes")
print("Windows errno table defines EPERM: yes")
PYRepository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
fd -i 'process' src/js src | head -120
for f in $(fd -i 'process' src/js src | head -80); do
if rg -q 'kill|Process' "$f" 2>/dev/null; then
printf '%s\n' "--- $f ---"
rg -n -C 5 'kill|Process' "$f" | head -100
fi
done
python3 - <<'PY'
from pathlib import Path
kill_impl = Path("src/spawn/process.rs").read_text()
target = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
errno_map = Path("src/errno/windows_errno.rs").read_text()
assert 'if errno_ != bun_sys::E::ESRCH' in kill_impl
assert 'if err.errno != bun_sys::E::ESRCH as u16' in kill_impl
assert 'process.kill(childPid)' in target
assert 'catch {}' in target
assert 'EPERM = 1' in errno_map
print("cleanup policy evidence is consistent")
PYRepository: oven-sh/bun
Length of output: 50367
Report unexpected child cleanup failures.
If process.kill(childPid) fails with an error other than ESRCH, rethrow it so failed cleanup cannot leave the detached child alive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/child_process/child-process-socket-inherit.test.ts` around lines
59 - 61, Update the child cleanup try/catch around process.kill(childPid) to
ignore only ESRCH, and rethrow any other error so unexpected cleanup failures
are reported.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| const { port, childPid } = JSON.parse(stdout.trim()); | ||
|
|
||
| try { | ||
| // The parent has exited and closed its server, so nothing may be listening | ||
| // on the port even though the detached child is still alive. | ||
| const result = await new Promise<string>(resolve => { | ||
| const socket = connect({ port, host: "127.0.0.1" }); | ||
| socket.on("connect", () => { | ||
| socket.destroy(); | ||
| resolve("connected"); | ||
| }); | ||
| socket.on("error", error => resolve((error as NodeJS.ErrnoException).code ?? "error")); | ||
| }); | ||
| expect(result).toBe("ECONNREFUSED"); | ||
| } finally { | ||
| try { | ||
| process.kill(childPid); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
🟡 The expect(stderr), expect(exitCode), and JSON.parse calls at lines 42-44 run before the try/finally that kills the detached grandchild, so if the parent emits any stderr, exits non-zero, or prints malformed stdout after spawning, the assertion throws and the detached child is orphaned on the persistent Windows runner for 100s. Consider parsing childPid best-effort first and wrapping all assertions inside the try, and/or shortening the grandchild's setTimeout(() => {}, 100000) to ~5-10s so a leak self-heals quickly.
Extended reasoning...
What the bug is
The test spawns a parent process that in turn spawns a detached grandchild running setTimeout(() => {}, 100000) — a process that will live for 100 seconds unless explicitly killed. Cleanup for this grandchild is process.kill(childPid) inside a finally block at lines 58-62. However, three fallible statements execute before that try block is entered:
expect(stderr).toBe(""); // line 42
expect(exitCode).toBe(0); // line 43
const { port, childPid } = JSON.parse(stdout.trim()); // line 44
try { ... } finally { process.kill(childPid); } // lines 46-62If any of lines 42-44 throw, control never reaches the try, childPid is never extracted, and the detached grandchild survives on the CI runner for the full 100 seconds.
Concrete trigger path (step-by-step)
parent.mjsstarts, listens on port 0, and spawns the detached grandchild (child.pid = 12345), which begins its 100-second sleep.parent.mjscallsserver.close()and prints{"port":54321,"childPid":12345}to stdout — but also emits something to stderr (a deprecation warning, a debug-build log line that leaked pastbunEnv, or any future runtime notice).- The parent exits with code 0.
- Back in the test,
stderris"[some warning]\n", soexpect(stderr).toBe("")at line 42 throws. - Line 44 (
JSON.parse) never runs →childPidis never bound → thetry/finallyis never entered →process.kill(childPid)never fires. - Process 12345 remains alive for 100 seconds on the persistent Windows CI runner. If the test is retried, another orphan accumulates.
The same applies if the parent crashes after spawn() but before console.log(...) (e.g., server.close() errors): exitCode !== 0 or stdout is not valid JSON, one of lines 42-44 throws, and the grandchild leaks.
Why existing code doesn't prevent it
The try/finally is correctly structured for the connection-check assertion inside it, but the three preceding lines are outside its guard. The await using proc on line 35 only cleans up the direct subprocess (the parent), which has already exited; it does nothing for the detached, unref'd grandchild. The only backstop is the grandchild's own setTimeout(..., 100000) — which is 100 seconds, long enough for retries to stack multiple orphans on a persistent runner.
Why this matters (REVIEW.md)
REVIEW.md's test-hygiene section states:
Release every resource via
using/await usingor try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners)
This is a direct instance of that rule: cleanup is registered after three assertions.
Impact
Test-hygiene only. With this PR's fix applied, the leaked grandchild holds no inherited socket, port, or stdio pipe — it's just an idle process that self-terminates after 100s. Nothing user-facing breaks; the concern is orphan accumulation on persistent Windows CI runners across retries when the test fails.
Fix
Either or both of:
- Restructure: parse
childPidbest-effort first (e.g.,let childPid; try { ({ port, childPid } = JSON.parse(stdout.trim())); } catch {}), then wrap all assertions — includingexpect(stderr)/expect(exitCode)— inside thetry { ... } finally { if (childPid) try { process.kill(childPid); } catch {} }. - Shorten the self-timeout: change
setTimeout(() => {}, 100000)to ~5000-10000. The test only needs the grandchild alive long enough to attempt oneconnect(), so a few seconds is plenty and any leak self-heals before it can accumulate. This also covers the case where the parent dies before printingchildPid(where restructuring alone can't help).
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The test added here fails its temp dir cleanup with EBUSY on the Windows 11 aarch64 lane (the detached child inherits the temp dir as cwd and outlives the rm). Fix in #36971. |
…est (#36971) Fixes the Windows CI failure in `test/js/node/child_process/child-process-socket-inherit.test.ts`, which has been failing on the Windows 11 aarch64 lane since the test landed in #36938 (for example in build 89207): ``` EBUSY: resource busy or locked, rm 'C:\Windows\Temp\buntmp-Fbshlf\socket-inherit_Ev5bLB' at [Symbol.dispose] (C:\buildkite-agent\build\test\harness.ts:462:8) ``` ### Cause The fixture spawns a detached child (`setTimeout(() => {}, 100000)`) that inherits the parent's cwd, which is the test's temp dir. The test kills the child in its `finally`, but on Windows `process.kill` only initiates `TerminateProcess`: the process object, including its working directory handle, is released asynchronously. `using dir` disposal runs `fs.rmSync` immediately after, so cleanup races the child's teardown and fails with EBUSY whenever the child has not finished dying. The socket assertions themselves pass (all 3 expect calls run); only cleanup fails. On the slower aarch64 runners the rm loses the race most of the time. It is worse on the failure paths: if the parent fixture exits non-zero, `childPid` is never parsed, the kill never runs, and the child holds the temp dir for the full 100 seconds. ### Fix Spawn the detached child with an explicit `cwd` outside the temp dir (`os.tmpdir()`). The child then never holds the directory that disposal removes, in every path, with no timing dependence. `cwd` is independent of handle inheritance (`lpCurrentDirectory` vs `bInheritHandles` in `CreateProcess`), so the code path exercised by the test is unchanged: the child is still spawned with inheritable-handle semantics while the listen socket is open. I considered retrying EBUSY in the harness `tempDir` disposal instead, but that would mask genuine process and handle leaks in every other Windows test. ### Verification (Windows 11 aarch64, the failing lane) - Unmodified test, release build with the #36938 fix (same setup as CI): EBUSY in 3 of 5 runs. - Fixed test, same build: 20 of 20 runs pass. - Fixed test, debug build with `packages/bun-usockets/src/bsd.c` reverted to its pre-#36938 state: fails with `Expected: "ECONNREFUSED", Received: "connected"`, so the test still detects the original inherited-socket bug (#36936). - Fixed test, debug build with current `bsd.c`: passes. The test is `skipIf(!isWindows)`, so Linux lanes are unaffected.
2 participants
Fixes #36936
Problem
On Windows, a detached child spawned while an HTTP server is listening inherits the server's listening socket. If the child outlives the parent, the port stays in LISTENING state (attributed to the dead parent's PID) even after
server.close()completes and the parent exits normally:Cause
uSockets creates sockets with plain
socket(). On Windows that returns an inheritable handle (theSOCK_CLOEXECbranch inbsd_create_socketis POSIX-only). Whennode:child_process.spawncreates the child via CreateProcess withbInheritHandles=TRUE(needed for stdio), every live socket handle is duplicated into the child, so the kernel keeps listen sockets open for as long as the child lives.accept()likewise returns inheritable handles.Fix
In
packages/bun-usockets/src/bsd.c, matching what libuv does:bsd_create_socket: on Windows, create sockets withWSASocketW(..., WSA_FLAG_OVERLAPPED | WSA_FLAG_NO_HANDLE_INHERIT)instead ofsocket().bsd_accept_socket: clearHANDLE_FLAG_INHERITon accepted sockets viaSetHandleInformation.All TCP, UDP, and connect sockets funnel through
bsd_create_socket, so this coversBun.serve,node:http,node:net, and dgram.Verification (on Windows)
The new test (
test/js/node/child_process/child-process-socket-inherit.test.ts) spawns a parent that listens on port 0, spawns a detached child, closes the server, and exits; the test then asserts connecting to the port yields ECONNREFUSED while the detached child is still alive.USE_SYSTEM_BUN=1): fails, the connection succeeds because the inherited listen socket still accepts.bun bd test): passes, and the issue's original repro now matches Node: parent exited, child alive,Get-NetTCPConnection -LocalPort 43123 -State Listenreturns nothing.test/js/bun/net/tcp-server.test.ts,test/js/node/net/node-net.test.ts,test/js/bun/udp/udp_socket.test.tsall pass on Windows with the fix (272 pass, 0 fail).Note: the bug is Windows-only (POSIX uses
SOCK_CLOEXEC), so the test istest.skipIf(!isWindows)and only exercises the fix on Windows lanes.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/child_process/child-process-socket-inherit.test.ts