Repository navigation
Conversation
…ime unless the client sets enableAutoPipelining BUN_FEATURE_FLAG_DISABLE_REDIS_AUTO_PIPELINING had no effect on Bun.redis, new RedisClient() and new RedisClient(url). Only the options parser read it, and a client with no options object took a default that hardcoded auto-pipelining on. Where the flag was read, the client still sent batches: the first command of a burst went out alone and the rest in one write. Options::from_js now builds the options of every client, with or without an options object, so impl Default for valkey::Options is gone. With the flag set, a client that does not set enableAutoPipelining sends one command at a time: Meta::check gives none of its commands SUPPORTS_AUTO_PIPELINING, so the client writes each one only when no command is in flight, as it does for INFO. A client that sets enableAutoPipelining, to either value, behaves as before. So does a process without the flag. js_send no longer calls Meta::check. ValkeyClient::send already checks every command.
…on drops When a connection drops and the client is going to reconnect, it rejects the commands in flight and keeps its queue for the next connection. A client under BUN_FEATURE_FLAG_DISABLE_REDIS_AUTO_PIPELINING has at most one command in flight and queues the rest of a burst. It sent that rest on the next connection, where the body of a MULTI runs outside its transaction. ValkeyClient::on_close now gets the status that the connection had. If the connection was ready (HELLO and SELECT answered), such a client rejects its queue too. It keeps the queue across a dial that fails and across a drop in the handshake, as before. A client that sets enableAutoPipelining and a process without the flag do not change: they keep a command that waits, for the next connection.
…lining off Six sentences said that subscribe() disables auto-pipelining and that unsubscribe() enables it again. No build does that. A subscribed client still writes four ping() calls of one tick as one batch.
|
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 9 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 |
StatusHow to reproduce. Save the script as import { RedisClient } from "bun";
// A mock server. It answers HELLO and every GET, and counts the GETs in each read.
const reads = [];
const server = Bun.listen({
hostname: "127.0.0.1",
port: 0,
socket: {
data(socket, chunk) {
const text = chunk.toString();
if (text.includes("HELLO")) return void socket.write("+OK\r\n");
const gets = text.split("GET").length - 1;
reads.push(gets);
socket.write("$1\r\nx\r\n".repeat(gets));
},
},
});
const client = new RedisClient(`redis://127.0.0.1:${server.port}`);
await client.connect();
await new Promise(resolve => setImmediate(resolve));
await Promise.all(["a", "b", "c", "d"].map(key => client.get(key)));
console.log("GETs per server read:", reads);
client.close();
server.stop(true);
The flag has no effect on 1.4.2 and on main: the four GETs go out as one batch. With PR. #46382 |
|
Updated 5:43 PM PT - Oct 21st, 2026
✅ @robobun, your commit 567f9091ce1d80035ad992677f7fc636fadc56b8 passed in 🧪 To try this PR locally: bunx bun-pr 46382That installs a local version of the PR into your bun-46382 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the two refactors folded into this PR: Options::from_js taking Option<JSValue> keeps the old null/undefined handling (JSValue::is_object requires a cell, so null still falls through to the defaults), and dropping the meta.check call in js_send is behavior-preserving because ValkeyClient::send recomputes checked_command.meta from scratch before routing. The fake-server batch-boundary concern (one socket read treated as one batch) was also looked at and ruled out as a product bug.
Extended reasoning...
The change touches the Rust Redis/Valkey client (option parsing, command metadata, close/reconnect queue handling), a types file, and adds a ~380-line subprocess test block; it touches no auth, crypto, or injection surface. Two confirmed findings on the flag-on drop/subscribe path and the partial-write hang are posted inline, so a human look is already signaled; this note only records what else was examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/valkey_jsc/valkey.rs— With the env flag set, a client whose peer answers a command before its last bytes are written can hang every later command forever, with no error and no timeout. In one-at-a-time mode only one command is ever in flight, so when its reply arrives while write_buffer still holds its tail, send_next_command at valkey.rs:714 skips the queue and valkey.rs:744 only flushes the tail; nothing re-runs the queue once the flush completes, and no later reply will come because in_flight is empty. Fix: after flush_data at valkey.rs:744 drains write_buffer, re-run the queue step (or register the flusher) whenever queue is non-empty and in_flight is empty, so a one-at-a-time client cannot be left with an unsent head.Why this was flagged
Trigger: a one-at-a-time client (Bun.redis or new RedisClient without enableAutoPipelining, in a process with BUN_FEATURE_FLAG_DISABLE_REDIS_AUTO_PIPELINING) sends a command larger than the socket buffer, then more commands. drain at valkey.rs:1343-1353 writes part of it and keeps the tail in write_buffer. A peer that replies before reading the full command and keeps the socket open pops the only in-flight entry at valkey.rs:1146. send_next_command at valkey.rs:714 requires an empty write_buffer, so it does nothing; valkey.rs:744 flushes the tail and returns. On the base branch a default client batches the following commands into the same write, so a later reply to one of them re-enters send_next_command; in one-at-a-time mode there is no later reply. The only remaining trigger is uws on_writable at valkey.rs:1365, which fires only if the kernel reported a full buffer. Population: every flag user by default after this PR; before it only enableAutoPipelining:false clients. Remedy: re-run the queue after the flush at valkey.rs:744 when in_flight is empty and queue is not.
Verification: pre-existing (route untouched by the diff). When a reply pops the in-flight pair (valkey.rs:1146), line 714 is false so the queue block is skipped and line 744 only drains the tail. Once the tail is gone, queue still holds the next command, in_flight is empty, nothing re-enters send_next_command. The base branch already hangs this way for enableAutoPipelining: false clients.
| let waiting = if self.flags.one_command_at_a_time | ||
| && closed == Status::Connected | ||
| && !self.flags.is_selecting_db_internal | ||
| { | ||
| core::mem::take(&mut self.queue) | ||
| } else { | ||
| command::entry::Queue::new() | ||
| }; |
There was a problem hiding this comment.
🔴 With the flag on, a user whose subscribe() was waiting behind an in-flight command when the connection drops gets a rejected promise but a listener it can never remove. The new queue rejection at valkey.rs:691-698 rejects the queued SUBSCRIBE entry, yet the handler stored by upsert_receive_handler at js_valkey_functions.rs:1894/1903 stays in the map. Since is_subscriber is only set on the server's confirmation (valkey.rs:938), unsubscribe() throws REDIS_INVALID_STATE, and has_subscriptions() keeps the reconnected client's poll ref alive (js_valkey.rs:1659-1671). Fix: when a queued entry carrying SUBSCRIPTION_REQUEST is rejected on a client that is not yet a subscriber, remove its receive handlers, or keep such entries in the queue for the next connection.
Why this was flagged
Process runs with BUN_FEATURE_FLAG_DISABLE_REDIS_AUTO_PIPELINING=1 and a client that does not set enableAutoPipelining. The script calls client.get(k) and then client.subscribe("ch", fn) in the same tick; every command is non-pipelineable so the SUBSCRIBE is pushed to self.queue behind the in-flight GET (valkey.rs:1391, 1400). Before the GET reply the server closes the connection. on_close takes self.queue at valkey.rs:695 and reject_all_pending_commands rejects the SUBSCRIBE promise with ERR_REDIS_CONNECTION_CLOSED (valkey.rs:502-507). The listener added at js_valkey_functions.rs:1903 remains in subscriptionCallbackMap; add_subscription (valkey.rs:938) never ran so is_subscriber is false. After the reconnect, update_poll_ref computes listening = has_subscriptions() && Connected (js_valkey.rs:1659-1665) and refs the event loop, so the process does not exit until close(). On the base branch the queued SUBSCRIBE is kept (reject_in_flight_commands passed an empty Queue) and sent on the next connection, so the subscription is established and unsubscribe() works.
Verification: ValkeyClient::on_close takes the queue (valkey.rs:691-698), then reject_all_pending_commands only does cmd.promise.reject(...) for queue entries (valkey.rs:502-507). is_subscriber is set only from the server's subscribe push at valkey.rs:938, so unsubscribe() throws REDIS_INVALID_STATE via require_subscriber (js_valkey_functions.rs:45-57). On base the same script keeps the queued SUBSCRIBE across the reconnect.
Problem
BUN_FEATURE_FLAG_DISABLE_REDIS_AUTO_PIPELINING=1does nothing forBun.redis,new RedisClient()andnew RedisClient(url). Without an options object a client takesOptions::default(), which hardcodestrue(src/runtime/valkey_jsc/js_valkey.rs:640).send_next_command,src/runtime/valkey_jsc/valkey.rs:706).Fix
Options::from_jsnow builds the options of every client. With the flag, a client that does not setenableAutoPipelininggetsone_command_at_a_time.Meta::checkthen marks none of its commands pipelineable, so each goes out only whenin_flightis empty, likeINFO.test/js/valkey/reliability/connection-failures.test.ts(3 new tests, 2 fail on main). Other suites: Notes.Background
Options::default()alone (redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) #34829): the flag then selects theenableAutoPipelining: falsemode, which batches.Downsides
enableAutoPipeliningstill overrides the flag, andfalsestill batches. Notes has two questions for a maintainer.ValkeyClientgrows from 320 to 328 bytes.Notes
Origin. There is no issue. I found the defect by reading the code. The flag has had this split since #18812 added the client. Closed #34829 carried a flag read in
Options::default()(3877aff) with no test.Who changes. Only a client that does not set
enableAutoPipelining, in a process with the flag. What the server reads for four GETs issued in one tick on an idle connection:Bun.redis,new RedisClient(),(url),(url, undefined),(url, null),.duplicate()(url, {}),(url, { enableAutoPipelining: undefined })(url, { enableAutoPipelining: false }),(url, { enableAutoPipelining: null })(url, { enableAutoPipelining: true })Questions for a maintainer.
enableAutoPipelining: falsechange too? Three meanings are possible.in_flightto at most one entry". Userland can do the same today with a promise chain:false.enableAutoPipelining? On main the option wins, and this PR keeps that. So a client withtruedoes not see the flag, and a client withfalsekeeps the mode of main.Also in this PR.
impl Default for valkey::Optionsis gone.Options::from_jsholds the defaults and is the only builder.js_sendcalledMeta::checka second time, with the same result. That call is gone.ValkeyClient::sendis the only caller.packages/bun-types/redis.d.ts. They said thatsubscribe()turns auto-pipelining off and thatunsubscribe()turns it on again. No build does that: a subscribed client writes fourping()calls of one tick as one batch, on main, on this PR and on 1.4.2.One command at a time, in detail.
SUPPORTS_AUTO_PIPELININGis a bit on each command, andMeta::checksets it. The queue writes a command without the bit only whenin_flightis empty.INFO,MULTI,EXECandWATCHtake that path on main.HELLOandSELECTin one write, with and without the flag.subscribe()of several channels leavesin_flightat its first confirmation. The next command goes out then, before the other confirmations. Main does the same for a never-batched command.CLIENT REPLY OFF) stops the client untilclose(), a connection drop, or the idle timeout ifidleTimeoutis set. Main does the same forINFOand for a client withenableAutoPipelining: false.enableAutoPipelining: false.The connection drop.
MULTIran outside its transaction.HELLOandSELECTanswered).ValkeyClient::on_closegets the status that the connection had. The client keeps its queue across a dial that fails and across a drop in the handshake, as on main.INFO,EXEC), and what follows it, for the next connection. A client withenableAutoPipelining: falsekeeps everything behind the first command of a burst. At a drop before the reply to theSELECTof a URL database, a default client rejects the burst that it has in flight, and a one-at-a-time client keeps its burst.Left alone on purpose.
Entry::createabout acheckcall there. Remove dead code from the bundler, the resolver, bun_css, the Valkey client, and six smaller areas #45723 edits the same lines.ValkeyClient::send, and thedrain()call with an empty queue insend_next_command.ConnectionFlags::default()still hasenable_auto_pipelining: true. Both construction sites set the field.SubscriptionCtxsaves and restoresenable_auto_pipelining, and nothing changes the value between the two.enableOfflineQueue: false, as on main.src/bun_core/env_var.rs. It is still true, and an edit there rebuilds every crate that depends onbun_core.Tests.
describe("Valkey: Auto-Pipelining"). Each runs the client in a child process (two with the flag, one without) against a mock RESP server that logs the order of commands and replies.connect(), the offline queue, a database in the URL,onconnect, a never-batched command, an error reply, an 8 MB command, a command issued from the continuation of a reply, and 6 connection drops (under a burst, withenableOfflineQueue: false, in the body of aMULTI, after an early error reply to a large command, before the connection is ready, and for a client that sets the option).test/js/valkey/(15 files) on the debug build of this PR with a 120 s timeout, against a local redis-server 8.0.2 with the TLS and ACL files of the repo: 1341 pass, 3 skip, 1 todo, 2 fail. The 2 failures assert the error text of another server version. They fail on main in the same way.test/integration/bun-types/bun-types.test.ts: 24 pass.Measurements. Release builds of main ea6a2c7 and of this change on top of it, linux-x64. Tools: gdb, nm, size, llvm-objdump. perf, valgrind, strace and bloaty are not installed here. "Option-less" means a client that does not set
enableAutoPipelining.getenvper process (gdb hit counts, 1000 constructions and 10000 GETs).JSValkeyClient::send, default client: 2297 -> 2291. First in a tick: 2434 -> 2428 (gdb stepi, 3 runs agree).mi_mallocinsidesend: 1 -> 1.send_next_command55 -> 55 instructions per batch-final reply and 30 -> 30 per mid-batch reply. Flush of 4 / 8 commands: 643 / 1059 -> 643 / 1059.send_next_command,drain, the flusher trampoline,handle_responseandon_data: the same instructions in the same order on both builds (120 / 183 / 216 / 1216 / 294). With addresses masked, 6 / 1 / 4 / 26 / 0 lines differ, each in one field offset. The new bool moved the fields behind it.estimateShallowMemoryUsageOf(client)after a burst of 8: 1120 -> 1128 bytes.sendtoper 4-GET burst (gdb catch syscall): default 1 -> 1.enableAutoPipelining: false2 -> 2.Bun.rediswith the flag 1 -> 4. Explicittruewith the flag 1 -> 1.enableAutoPipelining: false: 4.56 ms -> 4.26 ms (second main series: 3.40 ms). One round trip costs about 0.34 ms in this container.Promise.allof 100 / 1000 GETs, medians of 10 interleaved runs: 392.45 / 970.64 ms -> 383.34 / 986.93 ms. Second main series: 400.26 / 966.27 ms. No difference outside the spread of the runs.size).JSValkeyClient::send4602 -> 5093 bytes, and the standaloneMeta::check(605 bytes) is gone.create_no_js_no_pubsub7624 -> 7755.on_close1508 -> 1716 (nm -S).sizeof:ValkeyClient320 -> 328,ConnectionFlags9 -> 10 (gdb, debug builds).enableAutoPipeliningoption become one command per server read. The other 34 rows do not change.proto-max-bulk-len 1mb, thenMULTI,SETof 32 MB,INCR,EXECin one tick. The server answers theSETwith an error and closes. With the flag and without it, theINCRrejects and the counter stays unset.MULTI, 3INCR,EXECwith a whole-transaction retry and a drop at the 1st, 2nd or 3rdINCR, flag set: the counter ends at 3 in 9 of 9 rows.Self-review. Two rounds.
enableAutoPipelining: falsesend one command at a time. The first round asked to take that out: nobody asked for it, it costs users of the option one round trip per command, and the same queue then sent commands again after a drop. That is question 1.in_flightbefore the close let the queue through.on_closeto reject the queue of every client withenableOfflineQueue: false, and this PR does that for one-at-a-time clients only.Entry::create.Open PRs in the same functions. #41418 and #42054 conflict with this branch in
js_valkey.rs. Both conflict with main already. #38897, #45723, #41967 and #41645 merge cleanly. None of them fixes this.