Skip to content
Open
11 changes: 9 additions & 2 deletions packages/bun-usockets/src/eventing/epoll_kqueue.c
Original file line number Diff line number Diff line change
Expand Up @@ -543,9 +543,16 @@
}

if(!is_readable && !is_writable) {
if(!(old_events & LIBUS_SOCKET_WRITABLE)) {
// if we are not reading or writing, we need to add writable to receive FIN
/* 0-event poll: arm a one-shot write filter so a peer teardown still
* has an event to ride (EV_EOF on EVFILT_WRITE; epoll gets this for
* free via the implicit EPOLLHUP|EPOLLERR). Never for a socket whose
* write side WE shut down: our own SS_CANTSENDMORE makes any write
* filter report EV_EOF instantly, which read as the connection being
* over and closed a paused half-closed socket whose peer was alive. */
int own_shutdown = user_data &&
us_internal_poll_type((struct us_poll_t *) user_data) == POLL_TYPE_SOCKET_SHUT_DOWN;
if(!(old_events & LIBUS_SOCKET_WRITABLE) && !own_shutdown) {
EV_SET64(&change_list[change_length++], fd, EVFILT_WRITE, EV_ADD | EV_ONESHOT, 0, 0, (uint64_t)(void*)user_data, 0, 0);

Check failure on line 555 in packages/bun-usockets/src/eventing/epoll_kqueue.c

View check run for this annotation

Claude / Claude Code Review

kqueue: paused+shutdown socket has zero filters — peer FIN/RST never detected (diverges from epoll)

On kqueue, a paused shut-down socket now has **zero** filters registered — when the peer subsequently sends FIN or RST, no kevent can fire and the socket is never closed (fd leak until the app explicitly resumes/closes). The epoll path for the same 0-event `us_poll_change` explicitly registers `EPOLLHUP|EPOLLERR` so Linux still detects peer teardown and hits the "We got FIN back after sending it" close in loop.c; macOS silently diverges. The PR trades pre-PR's "closes too early" for "never close
Comment thread
robobun marked this conversation as resolved.
Outdated
}
} else if ((new_events & LIBUS_SOCKET_WRITABLE) != (old_events & LIBUS_SOCKET_WRITABLE)) {
/* Do they differ in writable? */
Expand Down
10 changes: 8 additions & 2 deletions packages/bun-usockets/src/socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -840,8 +840,14 @@ void us_socket_pause(struct us_socket_t *s) {
if (s->flags.is_paused) return;
// closed cannot be paused because it is already closed
if (us_socket_is_closed(s)) return;
// we are readable and writable so we can just pause readable side
us_poll_change(&s->p, s->group->loop, LIBUS_SOCKET_WRITABLE);
/* Drop readable interest but only KEEP writable interest, never add it:
* forcing WRITABLE here dispatched a bogus writable (a JS drain event
* with nothing buffered) on every pause, and on a shut-down socket the
* fresh kqueue EVFILT_WRITE one-shot reported our own SS_CANTSENDMORE
* as EV_EOF immediately, closing a half-closed socket whose peer was
* still alive (libuv's uv_read_stop only removes read interest). A
* backpressured write keeps its interest; none means nothing to drain. */
us_poll_change(&s->p, s->group->loop, us_poll_events(&s->p) & LIBUS_SOCKET_WRITABLE);
Comment thread
robobun marked this conversation as resolved.
s->flags.is_paused = 1;
}

Expand Down
70 changes: 70 additions & 0 deletions test/js/bun/net/socket.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,76 @@
expect(await bunRun(fileURLToPath(new URL("./kqueue-filter-coalesce-fixture.ts", import.meta.url)))).toSpawn();
});

// us_socket_pause armed WRITABLE unconditionally; the always-writable socket
// then dispatched a bogus drain with nothing buffered.
it("pause() with nothing buffered must not fire a drain event", async () => {
using server = Bun.listen({
hostname: "127.0.0.1",
port: 0,
socket: { open() {}, data() {}, end() {}, error() {}, close() {} },
});
let drains = 0;
const opened = Promise.withResolvers<any>();
await Bun.connect({
hostname: "127.0.0.1",
port: server.port,
socket: {
open: s => opened.resolve(s),
drain() {
drains++;
},
data() {},
end() {},
error() {},
close() {},
},
});
const s = await opened.promise;
await Bun.sleep(50); // let any connect-time writable settle
const before = drains;
s.pause();
await Bun.sleep(100); // window in which the buggy pause-armed writable fired
expect(drains - before).toBe(0);
s.terminate();
});

// On kqueue, pause() after shutdown() armed an EVFILT_WRITE one-shot that
// reported our own SS_CANTSENDMORE as EV_EOF immediately: the half-closed
// socket closed within milliseconds even though the peer was alive and
// silent. Linux always kept it open; this pins the behavior on both.
it("shutdown() then pause() keeps a half-closed socket open while the peer is silent", async () => {
let closedEarly = false;
const opened = Promise.withResolvers<void>();
using server = Bun.listen({
hostname: "127.0.0.1",
port: 0,
socket: {
open(s) {
s.shutdown();
s.pause();
opened.resolve();
},
data() {},
end() {},
error() {},
close() {
closedEarly = true;
},
},
});
// allowHalfOpen peer ignores our FIN and stays silently connected.
const peer = await Bun.connect({
hostname: "127.0.0.1",
port: server.port,
allowHalfOpen: true,
socket: { open() {}, data() {}, end() {}, error() {}, close() {} },
});
await opened.promise;
await Bun.sleep(1000);

Check warning on line 430 in test/js/bun/net/socket.test.ts

View check run for this annotation

Claude / Claude Code Review

Test sleeps 1000ms for a bug that fires in ~2ms

The 1000ms `Bun.sleep` window here is ~500x the observed failure latency — the PR description says the buggy pause() closed the socket "within ~2ms", so ~150-200ms would still give >>50x margin while not spending the whole ~1s per-test budget on a happy-path sleep. REVIEW.md also asks that any literal sleep ≥50ms outside a poll loop carry a comment naming why no observable signal exists (the 50ms/100ms sleeps in the first new test have one; this one doesn't). Mitigated by `describe.concurrent`,
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(closedEarly).toBe(false);
peer.terminate();

Check warning on line 432 in test/js/bun/net/socket.test.ts

View check run for this annotation

Claude / Claude Code Review

Tests: terminate() after expect() leaks the client socket on assertion failure

Both new tests call `terminate()` only *after* the `expect()` assertion (`s.terminate()` at line 395 and `peer.terminate()` at line 432), so on the exact regression each test guards against the assertion throws and the `Bun.connect` client socket is never released — only the server is `using`-scoped. Wrap the assertion in `try { expect(...) } finally { s.terminate() }` (or terminate before asserting) so cleanup runs on failure too.
Comment thread
robobun marked this conversation as resolved.
Outdated
});

it("reload() should preserve active_connections (no UAF / counter underflow)", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), fileURLToPath(new URL("./socket-reload-fixture.ts", import.meta.url))],
Expand Down
Loading