Repository navigation
Conversation
expired(headersTimeout, requestTimeout) takes milliseconds and compares uv_hrtime() nanoseconds. The product toUInt32() * 1000000 was a 32-bit product, so a timeout above 4294 ms wrapped: 4295 ms became 32,704 ns. Cast each value to uint64_t before the multiplication, as ConnectionsList::Expired does in Node (src/node_http_parser.cc).
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updated 10:07 PM PT - Oct 21st, 2026
❌ @robobun, your commit aece17b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46411That installs a local version of the PR into your bun-46411 --bun |
|
Status
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — the two-line widening in jsConnectionsList_expired is correct and matches Node's uint64_t cast; the inline note on the test's unbounded spin is a robustness nit, not a blocker.
Extended reasoning...
The change casts each toUInt32 result to uint64_t before the ms-to-ns multiplication at src/jsc/bindings/node/http/JSConnectionsListPrototype.cpp:94 and :96, so timeouts above 4294 ms no longer wrap; the surrounding swap/deadline logic is unchanged and the RETURN_IF_EXCEPTION checks remain after each coercion. A grep of src/jsc/bindings for other * 1000000 or toUInt32(globalObject) * products found no sibling sites sharing the pattern. The test adds a test.each matrix with values from Node v26.3.0 that fails without either cast. No security-sensitive surface is touched (expired() is only reachable via process.binding("http_parser") from user code), and no CODEOWNERS entry covers the changed files. The one inline finding concerns the deadline-less busy-wait in the new test, which exits in about 2 ms today and is a quality nit rather than a correctness issue in the fix.
| idle: list.idle(), | ||
| }).toEqual({ headersCompleted: [false, true], idle: [] }); | ||
| // Both parsers are older than 1 ms. | ||
| while (incomplete.duration() < 2 || complete.duration() < 2) {} |
There was a problem hiding this comment.
🟡 (optional) CI runners get a silent whole-file hang instead of a test failure if duration() ever stops advancing for one of the two parsers. The spin at node-http-parser.test.ts:480 has no deadline, and because it is synchronous the test runner's per-test timeout cannot fire. Any future regression in HTTPParser::duration() or in initialize() setting m_lastMessageStart (NodeHTTPParser.cpp:278) turns this test from red into a stuck job. Fix: bound the poll with a wall-clock deadline (for example a performance.now() limit of a few hundred ms) and fail with a message naming which parser's duration() stayed below 2, keeping the condition-polled shape.
Why this was flagged
The loop while (incomplete.duration() < 2 || complete.duration() < 2) {} at test/js/node/http/node-http-parser.test.ts:480 runs on every bun test invocation of this file, seven times per run via test.each. Today it exits in about 2 ms because NodeHTTPParser.cpp:278 and :358 set m_lastMessageStart and NodeHTTPParser.cpp:325 computes uv_hrtime() minus that value. The dismissal assumed the preceding idle: [] assertion at :475-:478 is a sufficient guard, but that assertion only proves lastMessageStart is non-zero at that instant; it does not bound the loop if duration() later returns a constant (for example a regression that returns 0 for parsers in a list, or reads a cached value). Being synchronous, no setTimeout-based test timeout can interrupt it, so the file hangs until the CI job limit. Root CLAUDE.md says to poll with a deadline. Remedy: add a deadline to the poll and throw with the observed durations when it is crossed.
Verification: nit — triggers only if a future regression makes HTTPParser::duration() stop advancing. test/js/node/http/node-http-parser.test.ts:480 is a synchronous spin with no wall-clock bound. Per-test timeout cannot fire: bun test's timeout is enforced by the event loop (src/runtime/test_runner/Execution.rs:298-335). So a sync spin turns a would-be assertion failure into a hung bun test process.
Problem
ConnectionsList#expired()ofprocess.binding("http_parser")wraps a timeout above 4294 ms.expired(4295, 0)reports a 3 ms old parser. Node v26.3.0 reports none.toUInt32(globalObject) * 1000000atsrc/jsc/bindings/node/http/JSConnectionsListPrototype.cpp:93and:95. 4295 ms becomes 32,704 ns.src/jscallsexpired(), andnode:httptimeouts do not use this list.Fix
toUInt32()result touint64_tbefore the multiplication, as Node does.test/js/node/http/node-http-parser.test.ts, 7 new cases with the results of Node v26.3.0. Five fail on bun 1.4.3 and on a debug build of main. Also rantest-http-parser*.js.ConnectionsList. The fix does not depend on it.Background
ConnectionsListis a set ofHTTPParserobjects in bun's port of Node'shttp_parserbinding.expired()returns each parser whose message started beforeuv_hrtime()(nanoseconds) minus a timeout (milliseconds).toUInt32()result.seconds * 1000. socket: compute the Duplex and named pipe timeout in 64 bits, sosocket.timeout()above 49.7 days does not wrap #46217 owns them and asks three design questions.Downsides
imulbecome 64-bit, and the text grows from 4055 to 4062 bytes. Compatibility: before bun 1.4.0 the call did not return, so only 1.4.0 to 1.4.3 wrap.Notes
Reproduction. The script runs on bun and on Node with no flags.
Each list holds two parsers that are 3 ms old: one with incomplete headers, one with complete headers and a pending body.
expired(1, 0)["incomplete"]["incomplete"]expired(0, 1)["incomplete","complete"]["incomplete","complete"]expired(4227858433, 0)[]["incomplete"]expired(0, 4227858433)[]["incomplete","complete"]expired(4227858433, 4227858433)[]["incomplete","complete"]expired(4227858433, 1)["incomplete"]["incomplete","complete"]expired(1, 4227858433)["incomplete"]["incomplete","complete"]expired(4294, 0)[][]expired(4295, 0)[]["incomplete"]The first seven rows are the cases of the test. "This branch" is a debug build.
Wrapped values
60,000 ms and 300,000 ms are the defaults of
headersTimeoutandrequestTimeoutinnode:http. Withexpired(60000, 300000), bun 1.4.3 reports a parser with incomplete headers after 3.65 s and a parser with complete headers after 4.17 s. This branch reports neither in 6 s.The test
expired(4227858433, 0)needs the first cast andexpired(0, 4227858433)needs the second. With only the first cast, the rows(0, long),(long, long)and(1, long)fail. With only the second cast, the rows(long, 0),(long, long)and(long, 1)fail. I built both variants.uv_hrtime()is below the timeout.expired()then uses no deadline for that timeout, as Node does. On a host with a longer uptime, the deadline is 48.9 days in the past. The expected result is the same.(1, 0)and(0, 1)are controls. They pass without the fix.expired(4295, 4295)andexpired(60_000, 300_000). The second passes without the fix for a young parser, because the wrapped values are 4.17 s and 3.65 s.Reach
process.binding("http_parser")returns the class. No built-in module passes a list toparser.initialize()or callsexpired().checkConnectionsinsrc/js/node/_http_server.tscallsisRequestTimedOut(src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp), which compares milliseconds in 64 bits with no product. node:http, node:http2: give emit('connection') and allowHTTP1 connections the server's timers and error path #46185 rewrites that sweep.ConnectionsList.expired()with a nonzero timeout did not return. I ran 1.2.16, 1.3.7 and 1.3.14: the call hangs. On 1.3.14 it also hangs for an empty list. Hardening: input validation and bounds tightening across 26 subsystems #31129 repaired the loop, first released in 1.4.0.process.binding("http_parser")throwsNo such module: http_parser. A listeninghttp.Serverholds aConnectionsListatSymbol(http.server.connections)(lib/_http_server.js:613), and the sweep of Node callsexpired(this.headersTimeout, this.requestTimeout)on it (:746). Thehttp.Serverof bun has no such symbol.process.binding("http_parser")returns the binding with the warning DEP0111 (lib/internal/bootstrap/realm.js). Node v24.0.0 removeshttp_parserfrom that list.Other sites
bugprone-implicit-widening-of-multiplication-resultreports lines 93 and 95 of this file on main and nothing in this file on this branch. Over the 221 C and C++ translation units that the linux-x64 debug build compiles fromsrc/andpackages/, it reports 16 other sites outside the vendoredsqlite3.c. All 16 are index, offset or size products. The.clang-tidyfile of the repository does not enable the check, and this PR does not add it.socket.timeout()above 49.7 days does not wrap #46217:UpgradedDuplex::set_timeout(src/runtime/socket/UpgradedDuplex.rs) andWindowsNamedPipe::set_timeout(src/runtime/socket/WindowsNamedPipe.rs) computeseconds * 1000in ac_uint. The two PRs share no file and neither depends on the other.Bun.spawn), Bun.serve: validate server.timeout() seconds argument #34958 (au8inserver.timeout()), usockets: store the socket timeout as a 32-bit due tick, sosocket.timeout()above 240 s fires #45614 (a modulo in uSockets).What this PR does not take from #37739
JSConnectionsList.cpp. They are on main since Add jsc-exception-lint and fix the missing exception checks it finds #40410.ConnectionsList.Verification
bun test test/js/node/http/node-http-parser.test.ts -t ConnectionsList: 6 pass, 5 fail.--rerun-each 100. They also pass withBUN_JSC_validateExceptionChecks=1.test-http-parser*.jsfiles andtest-http-server-connections-checking-leak.jsintest/js/node/test/parallel/. Also the 3process.bindingtests oftest/js/node/process/process.test.js.Size measurement
JSConnectionsListPrototype.cppalone from main and from this branch, with the release flags (-O3) and without-flto=thin.sizereports 4055 and 4062 bytes of text.llvm-objdumpshows the change:imul ebx, ebx, 0xf4240becomesimul rbx, rax, 0xf4240, and the same for the second timeout.