Skip to content

Bun.serve: make idleTimeout a floor so it never fires early - #33610

Closed
robobun wants to merge 3 commits into
mainfrom
farm/7ca1cfcb/usockets-timeout-phase
Closed

robobun wants to merge 3 commits into
mainfrom
farm/7ca1cfcb/usockets-timeout-phase

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Reproduction

// idleTimeout: 4, handler sleeps 150 ms, 32 requests spaced 130 ms so
// their arrival phase sweeps one full 4 s wheel period.
const server = Bun.serve({
  port: 0, idleTimeout: 4,
  async fetch(req) { await Bun.sleep(150); return new Response("done"); },
});

On 1.4.0 one of those requests is closed with zero response bytes and req.signal aborts ~80 ms after the request arrived, even though nothing was idle for anywhere near 4 s:

requests killed with NO response: 1/32
earliest req.signal abort after request arrival: 83 ms (configured idleTimeout: 4000 ms)

Cause

us_socket_timeout stores s->timeout = (group->timestamp + ceil(seconds/4)) % 240 and the sweep in loop.c fires when timestamp == s->timeout. But timestamp advances on an absolute LIBUS_TIMEOUT_GRANULARITY (4 s) grid that is unrelated to when the socket was armed, so a socket armed t ms before a grid boundary gets its first tick after only t ms. For idleTimeout: N the actual fire window was (4*(k-1), 4*k] with k = ceil(N/4), i.e. idleTimeout: 4 could fire after ~0 ms and every value N could fire up to 4 s early. server.timeout(req, N), Bun.connect socket timeouts and the WebSocket maxLifetime (minute wheel) arm through the same helpers.

Fix

Arm with one extra tick, clamped to the 240-slot wheel, so a timeout of N seconds fires in (ceil(N/4)*4, ceil(N/4)*4 + 4] s and never before N (packages/bun-usockets/src/socket.c). The same adjustment is applied to us_connecting_socket_timeout and the minute-granularity us_socket_long_timeout / us_connecting_socket_long_timeout.

The existing should allow use of custom timeout test stalled for 10 s under an 8 s idleTimeout; that now fires in (8, 12] s so the stall is bumped to 14 s, and the .resolves / .rejects assertions in that helper are now awaited.

Verification

# without packages/ change
(fail) idleTimeout never fires before the configured number of seconds
  { earliestAbort: 31.72, killed: 1 }

# with packages/ change
(pass) idleTimeout never fires before the configured number of seconds [4848ms]

test/js/bun/http/serve.test.ts: 239 pass / 4 pre-existing container-env failures (localhost/IPv6/proxy) that reproduce on main.

The uSockets sweep timer advances group->timestamp on an absolute
LIBUS_TIMEOUT_GRANULARITY (4 s) grid that is unrelated to when a socket
is armed. us_socket_timeout stored timestamp + ceil(seconds/4), so a
socket armed t ms before a grid boundary got its first tick after only
t ms: idleTimeout: 4 could reap a socket ~0 ms after the request
arrived, aborting a handler that was still running.

Arm with one extra tick (clamped to the 240-slot wheel) so a timeout of
N seconds fires in (ceil(N/4)*4, ceil(N/4)*4 + 4] s and never before N.
The same adjustment is applied to us_connecting_socket_timeout and the
minute-granularity long-timeout path.

The existing 'should allow use of custom timeout' test stalled for 10 s
under an 8 s idleTimeout that now fires in (8, 12] s, so it would race
the timer; bump the stall to 14 s and await the .resolves/.rejects
assertions that were previously fire-and-forget.
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR changes how uSockets computes timeout wheel slots, introducing a bounded tick-rounding helper used by socket and connecting-socket timeout/long-timeout setters, updates the related documentation comment, and adjusts/extends HTTP server idle-timeout tests to match the new timing behavior.

Changes

Timeout Wheel Rounding Fix

Layer / File(s) Summary
Timeout tick helper and doc comment
packages/bun-usockets/src/libusockets.h, packages/bun-usockets/src/socket.c
Adds us_internal_timeout_ticks, a shift-based rounding/capping helper for wheel tick offsets, and updates the header comment to describe the exact firing window using ceil(N/4)*4.
Apply helper across timeout setters
packages/bun-usockets/src/socket.c
Updates us_socket_timeout, us_connecting_socket_timeout, us_socket_long_timeout, and us_connecting_socket_long_timeout to compute stored slots as (group timestamp + ticks) % 240 via the new helper, replacing prior direct arithmetic.
HTTP server timeout test updates
test/js/bun/http/serve.test.ts
Increases handler sleep and test timeouts, tightens response assertions for success/failure paths, and adds a new concurrent test confirming idleTimeout never fires before the configured threshold.

Possibly related PRs

  • oven-sh/bun#30376: Modifies HTTP client logic that arms/re-arms idle timers using the same uSockets timeout scheduling mechanism changed in this PR.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Clearly states the main behavior change: idleTimeout now acts as a floor and won't fire early.
Description check ✅ Passed It covers the bug, cause, fix, and verification, though it uses different headings than the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 7, 2026
@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:40 AM PT - Jul 7th, 2026

❌ @robobun, your commit f1ba741 has 3 failures in Build #69692 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33610

That installs a local version of the PR into your bun-33610 executable, so you can run:

bun-33610 --bun

Comment thread packages/bun-usockets/src/socket.c
Comment thread test/js/bun/http/serve.test.ts
robobun added 2 commits July 7, 2026 04:56
…iately

The sweep compares long_ticks == s->long_timeout on every 4 s tick but
long_ticks = (global_tick / 15) % 240 only advances once per 15 ticks,
so storing (long_timestamp + 240) % 240 == long_timestamp matches on the
very next sweep. Capping at 239 keeps the stored slot distinct from the
arm-time slot on both wheels.

Also await the .resolves assertion in the adjacent per-request timeout
test.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/socket.c`:
- Around line 104-111: The timeout helper in us_internal_timeout_ticks currently
clamps the computed value to 239, which makes the documented maxLifetime value
of 240 fire one slot early. Update the logic so the public cap in App.h and the
wheel-slot handling stay consistent: either reduce the exposed maxLifetime limit
to 239 or allow the 240th slot to be represented explicitly without being
clamped away.

In `@test/js/bun/http/serve.test.ts`:
- Around line 2514-2568: This timing-sensitive idleTimeout test is flaky under
it.concurrent because sibling test contention can delay the event loop and make
a valid request exceed the 4s idle window, causing false failures in the killed
assertion. Move the test to sequential execution with it(...) (or equivalent
non-concurrent form) and set an explicit timeout aligned with the rest of the
serve tests, while keeping the existing idleTimeout/earliestAbort checks in the
same test body.
🪄 Autofix (Beta)

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: 1ab98b36-3aa5-406c-a666-1425b469a89d

📥 Commits

Reviewing files that changed from the base of the PR and between 3f67971 and f1ba741.

📒 Files selected for processing (3)
  • packages/bun-usockets/src/libusockets.h
  • packages/bun-usockets/src/socket.c
  • test/js/bun/http/serve.test.ts

Comment thread packages/bun-usockets/src/socket.c
Comment thread test/js/bun/http/serve.test.ts
@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is green on every lane that ran tests; serve.test.ts (which carries the new idleTimeout never fires before test) passes everywhere.

CI is red only on infra and unrelated flakes across two consecutive builds (69673, 69692):

  • darwin 26 aarch64 x2: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun' on both builds. The test shard never downloads the binary, so no tests run.
  • Flaky-annotated 1-retry failures on Windows / misc lanes (hot.test.ts, bun-install.test.ts, s3.test.ts, postgres-*, bundler_compile.test.ts), none touching uSockets timeouts or serve.test.ts.

Ready for review; needs a maintainer to merge past the darwin artifact-download infra timeout.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Closing this PR because it has been inactive for more than 90 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant