diff --git a/.claude/docs/landing-prs.md b/.claude/docs/landing-prs.md index d9643b5f42d4..adedad99deb4 100644 --- a/.claude/docs/landing-prs.md +++ b/.claude/docs/landing-prs.md @@ -38,7 +38,7 @@ Companion to the "Landing PRs: What Bun Reviewers Catch" section in CLAUDE.md. T - **Never assume OS/ABI facts are portable.** errno meanings differ (EPERM is a sharing violation on Windows), event flags differ, Windows env vars are case-insensitive, Windows has no POSIX signals, blocking syscalls retry on EINTR. FFI/ABI: explicit calling conventions on BOTH sides; fixed-width or C-ABI types, never bare int read from c_ulong (LLP64 garbage on Windows); extern declarations diff'd parameter-by-parameter against definitions — they compile cleanly per side and crash only on the platform you didn't build; complete Windows error-translation tables with fallbacks instead of force-unwraps. - **Platform parity is part of every fix.** When you fix one platform backend (POSIX vs kqueue vs epoll vs libuv), audit every sibling backend for the same defect and apply symmetrically — or state why a backend is unaffected ("kqueue register path is unhandled. You only patched unregister."). A POSIX-only API addition ships its Windows equivalent in the same PR. Enabling a feature on a new platform means grepping every gate, dispatch chain, parallel platform script, test skip, and allowlist. Platform-specific CI failures in files you touched are real merge-blocking bugs, never flakes. Comment WHY on every new platform exclusion. -- **Write tests to pass on every CI platform** (Windows, macOS x64+arm64, Linux glibc and musl). Split on `/\r?\n/` (Windows CRLF); normalize separators in path assertions; never spawn shell builtins (echo, sleep are not programs on Windows — use bunExe() -e); no hardcoded /tmp or /bin; incidental servers bind 127.0.0.1, never ::1 (CI Linux may lack IPv6 — IPv6-specific tests gate on the harness IPv6 helper); exit codes not signal names for "did not crash" (Windows has no signals); when probing a limit, exceed the LARGEST platform limit to trip the guard and stay under the SMALLEST when constructing inputs (macOS PATH_MAX is 1024). Before skipping a platform, verify it genuinely lacks the capability (Windows supports AF_UNIX). Skip narrowly via test.skipIf with a reason; never fix one platform by loosening assertions for all. +- **Write tests to pass on every CI platform** (Windows, macOS x64+arm64, Linux glibc and musl). Split on `/\r?\n/` (Windows CRLF); normalize separators in path assertions; never spawn shell builtins (echo, sleep are not programs on Windows — use bunExe() -e); no hardcoded /tmp or /bin; incidental servers bind 127.0.0.1, never ::1 (CI Linux may lack IPv6 — IPv6-specific tests gate on the harness IPv6 helper); exit codes not signal names for "did not crash" (Windows has no signals); when probing a limit, exceed the LARGEST platform limit to trip the guard and stay under the SMALLEST when constructing inputs (macOS PATH_MAX is 1024); a test that asserts a peer's RST is reported must send the RST to a socket whose receive buffer is empty (or that reads before the reset): macOS answers an RST with an ACK instead of resetting while unread bytes sit in the buffer, depending on timing, and a socket that never reads gets no second chance (see the "server socket whose peer resets the connection" tests in test/js/node/tls/node-tls-server.test.ts). Before skipping a platform, verify it genuinely lacks the capability (Windows supports AF_UNIX). Skip narrowly via test.skipIf with a reason; never fix one platform by loosening assertions for all. - **Decide explicitly: filesystem path or URL-like identifier.** Module specifiers, cache keys, sourcemap paths use forward slashes everywhere (posix path helpers). Filesystem paths use platform path APIs, never literal '/' concatenation. Windows: accept BOTH separators; drive-relative (C:foo) and UNC forms exist; PATH splits on ';'. On POSIX, backslash is a legal filename character. Splitting a posix-normalized string with the platform separator silently no-ops on Windows — feed the other separator style through every new API in tests. - **Beyond `rust:check-all` (required by CLAUDE.md) for platform-gated code:** verify link-time symbol resolution (a POSIX extern must still resolve on Windows even if runtime-gated); audit enum switches duplicated across platform arms; distrust lint sweeps — a cast redundant on your host may be load-bearing on another target. Trick: flip the platform condition locally to force the other branch through the type-checker. diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 0b5f1358f746..0dcf1caa8224 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -4171,9 +4171,9 @@ describe.concurrent("close() error after the peer resets the connection", () => describe.each(["tcp", "tls"] as const)("%s", transport => { // The peer lives in a child process that is killed while data it never read sits // in its receive buffer: the kernel then closes its socket with an RST, and - // nothing (no FIN, and for TLS no close_notify) is queued ahead of the reset. An - // in-process terminate() is not usable for the TLS case: it writes a close_notify - // first, and on POSIX the reading side consumes that as a clean end. + // nothing (no FIN, and for TLS no close_notify) is queued ahead of the reset, + // whatever Bun's own terminate() sends (until #39632 it sent a close_notify first + // for TLS, which the reading side consumed as a clean end). const peerSource = ` await Bun.connect({ hostname: "127.0.0.1", diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index 32f369386c1c..13b4b86cdacc 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -2458,12 +2458,19 @@ describe("deferred spill-close", () => { }); }); -describe.each(["tls", "net"])("%s server socket whose peer resets the connection behind unread data", transport => { - // The peer sends data while the accepted socket is paused, then resets the - // connection (RST) instead of ending it. Node reports that as a read - // error and never emits 'end'. allowHalfOpen keeps the accepted socket open - // after an 'end', so a reset misreported as an orderly end strands it. - async function acceptPausedSocketAndFill() { +describe.each(["tls", "net"])("%s server socket whose peer resets the connection", transport => { + // The peer resets the connection (RST) instead of ending it while the accepted + // socket is paused. Node reports that as a read error and never emits 'end'. + // allowHalfOpen keeps the accepted socket open after an 'end', so a reset + // misreported as an orderly end strands it. + // + // Whether macOS delivers a reset to a socket that holds unread bytes depends on + // timing: filling the buffers first (the original shape of these tests) lost it + // nearly every time, and even 296 unread bytes lost it in up to 17 of 40 attempts + // when the RST came a few milliseconds later. With an empty buffer it is never + // lost, so only the test that is about the unread bytes sends any, and it sends + // them right before the reset. Linux and Windows deliver it either way. + async function acceptPausedSocket() { const accepted = Promise.withResolvers(); const onConnection = (socket: net.Socket) => { socket.pause(); @@ -2476,6 +2483,7 @@ describe.each(["tls", "net"])("%s server socket whose peer resets the connection await once(server.listen(0, "127.0.0.1"), "listening"); const peerReady = Promise.withResolvers(); + const peerGotData = Promise.withResolvers(); const peer = await Bun.connect({ hostname: "127.0.0.1", port: (server.address() as AddressInfo).port, @@ -2488,7 +2496,9 @@ describe.each(["tls", "net"])("%s server socket whose peer resets the connection if (success) peerReady.resolve(); else peerReady.reject(verifyError ?? new Error("handshake failed")); }, - data() {}, + data() { + peerGotData.resolve(); + }, close() {}, error(_peer, error) { peerReady.reject(error); @@ -2519,10 +2529,11 @@ describe.each(["tls", "net"])("%s server socket whose peer resets the connection // Paused again: the 'data' listener above switched the stream to flowing. socket.pause(); - const t = { + return { socket, peer, events, + peerGotData: peerGotData.promise, bytesRead: () => bytes, settled: settled.promise, [Symbol.dispose]() { @@ -2530,36 +2541,36 @@ describe.each(["tls", "net"])("%s server socket whose peer resets the connection server.close(); }, }; - // One chunk that fits: it is queued ahead of the reset and the receive window - // stays open. macOS drops an RST that arrives at a zero window, so filling - // the buffers would strand the socket there. - const chunk = Buffer.alloc(64 * 1024, "r"); - const written = peer.write(chunk); - if (written !== chunk.length) { - t[Symbol.dispose](); - throw new Error(`short write: ${written} of ${chunk.length}`); - } - return t; } it("reports the reset that arrives while the socket is paused as ECONNRESET, not 'end'", async () => { - using t = await acceptPausedSocketAndFill(); + using t = await acceptPausedSocket(); + // A round trip to the peer after the pause: this process has polled since, so + // the reset has to reach the paused socket on its own and cannot ride on the + // writable event that a pause leaves behind. The peer sends nothing back. + t.socket.write("still here"); + await t.peerGotData; t.peer.terminate(); await t.settled; expect(t.events).toEqual(["error ECONNRESET", "close hadError=true"]); }); it("delivers the data queued ahead of the reset and then reports ECONNRESET, not 'end'", async () => { - using t = await acceptPausedSocketAndFill(); - // Both happen before the event loop runs again, so the socket's next read - // event carries the queued data and the reset together: the read loop drains - // the data and then gets the error from recv(). + using t = await acceptPausedSocket(); + // One chunk that fits in the buffers, then all three happen before the event + // loop runs again: the socket's next read event carries the queued data and the + // reset together, the read loop drains the data and then gets the error from + // recv(). + const chunk = Buffer.alloc(64 * 1024, "r"); + expect(t.peer.write(chunk)).toBe(chunk.length); t.socket.resume(); t.peer.terminate(); await t.settled; - expect(t.events).toEqual(["error ECONNRESET", "close hadError=true"]); // Windows discards the receive queue on a reset. Linux and macOS keep it, and // like node, the data is delivered before the error. - if (!isWindows) expect(t.bytesRead()).toBeGreaterThan(0); + expect({ events: t.events, dataDelivered: t.bytesRead() > 0 }).toEqual({ + events: ["error ECONNRESET", "close hadError=true"], + dataDelivered: !isWindows, + }); }); });