Repository navigation
Conversation
…ies behind it A pub/sub message that did not become a JS value was dropped in on_valkey_message and its exception stayed pending. The messages behind it in the same read were lost, and the next callback that the event loop ran was skipped and got that exception. Since #46021 a string copy that the allocator refuses is such a message, so a subscriber near its memory limit lost messages with no error. on_valkey_message returns the error. ValkeyClient::on_data reports it as an uncaught exception, once for that reply, and handles the rest of the read. A psubscribe or punsubscribe confirmation that does not convert rejects its own request. Before, that request never settled, the replies behind it in the same read were lost, and later replies resolved the wrong commands.
… not the 'error' event emit_error returned early when the message or the code string was not created, and the stream's 'error' event was never delivered. Both are a few tens of bytes, so the only failure is a refused allocation. Before #46021 that allocation crashed. It is the out-of-memory report now.
|
Status How this was reproduced (release builds, Linux x64):
The tests are in |
|
Updated 12:26 AM PT - Oct 22nd, 2026
❌ @robobun, your commit f9a17c5 has some failures in 🧪 To try this PR locally: bunx bun-pr 46430That installs a local version of the PR into your bun-46430 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes route JavaScript allocation and Valkey RESP conversion errors through runtime error handling. Valkey message and subscription tests cover error reporting, reply handling, reconnects, and allocation failures. ChangesValkey RESP error handling
Zlib allocation errors
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: reporting failed conversions in Redis and node:zlib instead of dropping errors. |
| Description check | |
The description explains the problem, implementation, verification, limitations, and performance impact. It does not use the exact template headings, but it provides the required change summary and ve… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, implementation, verification, limitations, and performance impact. It does not use the exact template headings, but it provides the required change summary and verification details.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Beyond the inline findings, I also checked two things the diff relies on: JSPromiseStrong::settle with an Err goes through reject, which takes the pending exception (and maps OutOfMemory to a fresh error), so a failed psubscribe/punsubscribe confirmation rejects its promise rather than leaving it pending; and the two handle_oom sites in emit_error only ever see zlib/brotli/zstd static message and code strings, so an Err from create_utf8_for_js there can only be a refused allocation, not a JS exception that needed propagating.
Extended reasoning...
The change turns a dropped conversion error in the Valkey subscriber read loop into a report-and-continue via report_error_or_terminate, routes failing subscribe confirmations through settle(Result), and converts two swallowed string-allocation failures in node:zlib emit_error into handle_oom crashes. The settle-rejects and zlib-OOM-only concerns were verified from JSPromise.rs and node_zlib_binding.rs and are not issues; the inline findings cover what remains.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/valkey_jsc/valkey.rs— A subscriber whose server answers a subscribe request with an error reply gets that request's promise hung forever while every other in-flight command is rejected. valkey.rs:1148 pops the request before valkey.rs:1163 calls fail(), and fail_with_js_value at valkey.rs:600 rejects only what is still in in_flight and queue, so the popped pair is dropped unsettled. Eight finders saw it; the PR text names it as left for #32858 but the new report-and-continue loop at valkey.rs:818-827 now sits directly above this arm and leaves it the one Err path that still loses a promise. Fix: settle the popped pair (reject it with the same error) before or inside fail() on this arm, or push it back before calling fail().Why this was flagged
Trigger: the client is a subscriber (is_subscriber() true) and the server sends a RESP error reply. At valkey.rs:1113 the value is not a Push, so should_consume_promise_pair stays true and valkey.rs:1148 pops the front request into pair_maybe. valkey.rs:1163 calls fail(err, ServerError), which at valkey.rs:600 rejects the remaining in_flight and queue and closes the socket; pair_maybe is dropped at the return on valkey.rs:1164 with its promise never settled. The caller of that subscribe awaits forever and the objects it pins never collect. The base branch behaves the same, but the PR's stated purpose is that 'leaf code never drops an exception' and that every reply's request is settled; this arm is the remaining one that drops a popped request, and the new loop (valkey.rs:818-827) no longer aborts the read around it, so the fix belongs in the same change. Remedy: reject pair_maybe with the server error before fail(), or let fail() take it.
Verification: pre-existing; acknowledged in diff: the PR description's "What this PR leaves" names
valkey.rs:1153as dropping the popped request unsettled and defers it to #32858; the base branch fails the same way, and the diff does not change this arm. Line 1148 pops the request; line 1163self.fail(err, RedisError::ServerError)?only rejectsin_flightandqueue, so that promise stays pending forever.
| if let Err(err) = self.handle_response(&mut value_to_handle) | ||
| && bun_jsc::task::report_error_or_terminate(&self.global_object(), err).is_err() | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get the same five-line report-and-continue block pasted into both reply loops, so the two read paths can drift apart on the next edit. The block at valkey.rs:818-822 and the one at valkey.rs:881-885 are byte-identical, and each is followed by the same socket-identity check and send_next_command call. Fix: move the handle-report-check tail into one helper that returns whether the loop may go on, and call it from both the buffered path and the stack path, so that the two paths stay symmetric by construction.
Why this was flagged
The diff adds if let Err(err) = self.handle_response(..) && bun_jsc::task::report_error_or_terminate(&self.global_object(), err).is_err() { return Ok(()); } at valkey.rs:818-822 and again at valkey.rs:881-885. Both sites are followed by if *self.socket.socket() != socket { return Ok(()); } and self.send_next_command();. The base branch had the same duplication with a single ? line, so this is a pre-existing shape the diff grows from one line to five. No runtime failure follows today; the cost is that a later change to one loop's error handling (for example a liveness check after the uncaughtException handler) can miss the other, which is exactly the asymmetry the two-path read loop already invites.
Verification: nit. Triggering condition: any future edit to the report-and-continue tail of one reply loop that is not mirrored in the other. Verified in /home/claude/bun/src/runtime/valkey_jsc/valkey.rs: the buffered path at lines 818-822 and the stack path at lines 881-885 contain the identical block. The pre-existing two-path duplication grows from one line to five within this diff.
| if let Err(err) = self.handle_response(&mut value_to_handle) | ||
| && bun_jsc::task::report_error_or_terminate(&self.global_object(), err).is_err() | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: A Bun.ModuleGraph whose RedisClient receives an unconvertible reply now gets the error on the host's process.on('uncaughtException') (or the host exits 1), not the graph's onError. The new report at valkey.rs:819 runs in the uws read callback, which is outside the client's context: nothing enters self.context there. Fix: route a reply's error to the context that owns the client at every report site — enter self.parent().context (as ValkeyDeferredClose::run does at js_valkey.rs:2083) around handle_response and the report at both loops (valkey.rs:819 and valkey.rs:882), so the thrown exception's async frame and the report are the graph's.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A RedisClient created by a Bun.ModuleGraph module (js_valkey.rs:755 stores context: cx.context().id()) is subscribed and the server sends a message whose conversion fails. The uws data callback reaches ValkeyClient::on_data (valkey.rs:743) with no context entered: the trampoline NsHandler::on_data (uws_handlers.rs:549) and JSValkeyClient::on_data (js_valkey.rs:1945) never call enter_context. resp_value_to_js throws there, so the Exception's async frame is the host's, and the new report_error_or_terminate(&self.global_object(), err) at valkey.rs:819 / valkey.rs:882 calls uncaught_exception (VirtualMachine.rs:2144), whose Bun__ModuleGraph__handleUncaughtException (ModuleGraph.cpp:229-242) resolves the graph from that frame, finds none, and falls through to the process-wide handler at VirtualMachine.rs:2188: the host's uncaughtException listeners run, or with none the host process exits 1. On the base branch the Message path dropped the exception, so this host-level report is new behaviour introduced by the PR's report site.
Verification: The new report sites valkey.rs:818-822 and :881-885 call bun_jsc::task::report_error_or_terminate(&self.global_object(), err) from inside ValkeyClient::on_data, which is reached from NsHandler::on_data (src/runtime/socket/uws_handlers.rs:549-552) -> JSValkeyClient::on_data (js_valkey.rs:1945-1960). Neither frame calls enter_context.
This branch has not been deployed
2 participants
Problem
create_utf8_for_js, not a crash. ARedisClientsubscriber drops it (js_valkey.rs:1370). Near a 256 MiB limit, 18 of 135 runs lose the message with no error. 1.4.3 crashes instead: 0 of 135.node:zlibemit_errordrops the error too (node_zlib_binding.rs:843): no'error'event.Fix
ValkeyClient::on_data) reports a reply's error as an uncaught exception. It then handles the rest of the read: 0 of 135 silent. Apsubscribeconfirmation that does not convert rejects its request.emit_errorends the process with the out-of-memory report.test/js/valkey/reliability/resp-nesting-depth.test.ts(8 new rows), the other mock-server valkey suites,valkey.test.tsover TCP,node:zlib.Background
on_dataparses each reply of a socket read. Amessagepush goes to the listeners. Any other reply settles the request at the front ofin_flight.on_valkey_message:psubscribestays broken. Considered closing the connection: a subscriber does not redial.Downsides
uncaughtExceptionhandler the process exits with code 1. On main it stays alive and silent..text+48 bytes. At most 1 more branch instruction of 1,033 per delivered message (Notes).Notes
Where the error comes from
#46021 made
BunString__createUTF8ForJS(src/jsc/bindings/BunString.cpp:92-111) throwRangeError: Out of memorywhen the allocation for the JS string is refused: for ASCII input of any length, and for valid non-ASCII input of 64 KiB or more. Rust sees it asErrfrombun_string_jsc::create_utf8_for_js, which has about 180 call sites. Before, the same refusal crashed inside WTF (Segmentation fault at address 0xBBADBEEF).Most call sites return the error or use
?. Two dropped it with the exception left pending, and this PR changes both:JSValkeyClient::on_valkey_message(js_valkey.rs:1370-1391):let Ok(..) = .. else { return; }for the channel and for the message, and.is_err()thenreturnfor the listener call.CompressionStream::emit_error(node_zlib_binding.rs:843,:855):Err(_) => return.Reproduction with a Redis server
A subscriber process gets an allocator limit (
MIMALLOC_RESERVE_OS_MEMORY=268435456 MIMALLOC_DISALLOW_OS_ALLOC=1), fills it with 170 to 214 MiB of ballast, and receives one 6.2 MB message and then a small one from a publisher that has no limit. Release builds, Linux x64. "Silent" means: no message arrives, no error is reported, and the process is alive 10 s later.0793250a5b)uncaughtException, 54 deliver the message, 67 end with a crash reportuncaughtExceptionWith the fix the refused copy is an
uncaughtExceptionwithRangeError: Out of memory. The runs that still end with a crash report are refusals in other places, for example the client's read buffer (valkey.rs:757), which abort as before. The official canary build963452075egives the same counts as the local build of main: 18 of 135 silent in each of two sweeps, and 8 of 78 for ASCII text.Without a memory limit
The swallow does not need the allocator. A mock server sends a reply that cannot become a JS value: a map whose key is a map. The client makes a null-prototype object of a map and uses each converted key as a property key, and a null-prototype object has no primitive value (
TypeError: No default value). No Redis server sends that reply, so on 1.4.3 this is the only way to reach the arm.TypeErroris reported at once, then 2 of 2 deliveredTypeErrorTypeErrorpsubscribe,punsubscribe,get a,get bwith two bad confirmationspsubscribenever settles,punsubscriberesolves"value-of-b", bothgetnever settleTypeError, eachgetresolves its own valueThe wrong value has one cause.
handle_responsepops the request fromin_flight, the conversion fails, and?returns throughon_data, which drops the rest of the socket read. The replies in that rest never meet their requests, so each later reply resolves the request before its own.What the fix does
on_valkey_messageuses?for the two conversions and for the listener call.on_datacallsreport_error_or_terminatefor theErrof one reply, at both of its loops, and goes on with the next reply. That is the drain ofnode/quic/session.rs:955. A VM that stops ends the read.JSPromiseStrong::settle, which rejects with the pending exception and lets a termination pass.emit_errorpasses both strings throughbun_core::handle_oom. They are error texts of zlib, brotli and zstd, a few tens of bytes, so anErris a refused allocation only.With no
uncaughtExceptionhandler the process prints the error and exits with code 1, as for any uncaught exception. Measured with a mock server and no handler: the two messages behind the bad one arrive, then the process exits 1. On 1.4.3 the same process exits 1 when its next timer fires, with no message delivered.What this PR leaves
valkey.rs:1153(an error reply in subscriber mode) and:1168(an unknown push kind) fail the connection and drop the popped request unsettled.valkey-incremental-scan.test.ts:245documents the first one and names redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands #32858.pmessageandsmessagepayloads are dropped by design (valkey.rs:926). Bun.RedisClient: subscribe() handler silently drops messages after HELLO 2 on the subscriber connection #29042 (messages lost afterHELLO 2) is a different drop and is not changed.src/runtime/socket/udp_socket.rs:280-292returns from its packet loop with the exception pending when a payload or a hostname cannot be created. It has no deterministic test, so it is not in this PR.valkey.rs:757,OffsetByteList::write).The tests
All use a mock server in the test process that answers a batch of commands with one write, so the replies are in the same read of the client.
uncaughtExceptionhandler closes the client and connects it again. The old read must stop there.psubscribeandpunsubscribewith a bad confirmation and aPINGbehind it.BUN_JSC_maxSingleAllocationSize, which makes WTF refuse one allocation above a cap: an ASCII message and a non-ASCII message. The variable exists in debug builds only, as intest/js/node/buffer-oom.test.ts. ASAN's per-allocation cap cannot replace it here: the client's read buffer doubles, so it can need as many bytes as the UTF-16 copy of the message.Without the fix, the 6 rows that need no allocator fail on release builds of 1.4.3 and of main. On a debug build the children of the subscriber rows abort: the exception that the swallow leaves pending trips
releaseAssertNoException.emit_errorhas no test. No cap refuses one 42-byte allocation and nothing else. The evidence is a probe on release builds that exhausts the allocator, then lets an inflate of garbage fail: 1.4.3 ends with a crash report in 6 of 6 runs. On main the event is never delivered in 6 of 6, and the process exits 0 in 5 of them. With this PR 6 of 6 end withBun has run out of memory.Other suites. Debug build:
valkey-incremental-scan(41 pass),reliability/connection-failures(46 pass), thenode:zlibsuites (527 pass),test/internal/source-lints/jsresult-swallow.test.ts. Release builds of main and of the PR against a local Redis server: the TCP half ofvalkey.test.ts, 494 pass on both.Costs
.text: +48 bytes, 102,123,360 to 102,123,408 (size, release builds of0793250a5band of the PR, both with--lto=off). Per symbol (nm -S):ValkeyClient::handle_response4,911 to 4,985,ValkeyClient::on_data1,236 to 1,254,emit_error9 bytes less in each of its 3 instances.handle_responseinon_databoth builds have one compare and one conditional jump (objdump).handle_responsehas 156 conditional jumps, on main 158. Counters for 1,000,000 delivered 16-byte messages (perf_event_open, user mode, medians of 5 interleaved runs): 4,931 instructions per message on main and 4,927 with the PR, 1,032.3 branch instructions and 1,033.6. The same-binary spread is 8 instructions and 1.5 branch instructions.BunString__createUTF8ForJSruns 2 times per delivered message on both builds, andmi_malloc7.4 and 7.3 times (gdb breakpoint counts over 1,000 messages).