Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude/docs/landing-prs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
6 changes: 3 additions & 3 deletions test/js/bun/net/socket.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4169,11 +4169,11 @@
const readReset = { reported: true, code: "ECONNRESET", syscall: "read" };

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).

Check failure on line 4176 in test/js/bun/net/socket.test.ts

View check run for this annotation

Claude / Claude Code Review

socket.test.ts paused test rework described in PR is missing from the diff

The PR description says "In socket.test.ts the paused case becomes a second case of the child-peer test from #39615", but the only change to this file is this comment update — the paused test at lines 4097-4156 is untouched, and line 4140 still does `peer.write("queued behind the pause")` (23 bytes into the paused accepted socket's buffer) immediately before `peer.terminate()`. That is the "24 bytes then RST back to back: 37/40" shape from this PR's own Notes and directly violates the guidance
Comment thread
robobun marked this conversation as resolved.
const peerSource = `
await Bun.connect({
hostname: "127.0.0.1",
Expand Down
61 changes: 36 additions & 25 deletions test/js/node/tls/node-tls-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2458,12 +2458,19 @@
});
});

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<net.Socket>();
const onConnection = (socket: net.Socket) => {
socket.pause();
Expand All @@ -2476,6 +2483,7 @@
await once(server.listen(0, "127.0.0.1"), "listening");

const peerReady = Promise.withResolvers<void>();
const peerGotData = Promise.withResolvers<void>();
const peer = await Bun.connect({
hostname: "127.0.0.1",
port: (server.address() as AddressInfo).port,
Expand All @@ -2488,7 +2496,9 @@
if (success) peerReady.resolve();
else peerReady.reject(verifyError ?? new Error("handshake failed"));
},
data() {},
data() {
peerGotData.resolve();
},
close() {},
error(_peer, error) {
peerReady.reject(error);
Expand Down Expand Up @@ -2519,47 +2529,48 @@
// 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]() {
socket.destroy();
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;

Check warning on line 2552 in test/js/node/tls/node-tls-server.test.ts

View check run for this annotation

Claude / Claude Code Review

peerGotData promise has no rejection path

The new `peerGotData` promise is only resolved from the peer's `data()` handler — nothing rejects it (peer `close()` is a no-op, peer `error`/`connectError` reject only `peerReady`, which is already settled by this point). If a regression breaks the write from the paused socket or the peer errors/closes before receiving "still here", `await t.peerGotData` hangs until the harness timeout instead of failing with a diagnosable message. Consider rejecting `peerGotData` from the peer's `error` and
Comment thread
robobun marked this conversation as resolved.
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,
});
});
});