usockets: stop pause() from arming writable interest it never had - #37099
Open
robobun wants to merge 9 commits into
Open
usockets: stop pause() from arming writable interest it never had#37099robobun wants to merge 9 commits into
robobun wants to merge 9 commits into
Claude / Claude Code Review
completed
Aug 7, 2026 in 17m 29s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | packages/bun-usockets/src/eventing/epoll_kqueue.c:552-555 |
kqueue: paused+shutdown socket has zero filters — peer FIN/RST never detected (diverges from epoll) |
| 🟡 Nit | test/js/bun/net/socket.test.ts:430 |
Test sleeps 1000ms for a bug that fires in ~2ms |
| 🟡 Nit | test/js/bun/net/socket.test.ts:431-432 |
Tests: terminate() after expect() leaks the client socket on assertion failure |
Annotations
Check failure on line 555 in packages/bun-usockets/src/eventing/epoll_kqueue.c
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
Check warning on line 430 in test/js/bun/net/socket.test.ts
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`,
Check warning on line 432 in test/js/bun/net/socket.test.ts
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.
Loading