Repository navigation
Conversation
…nected With a database in the URL the client sends HELLO and SELECT together but became Connected on the HELLO reply. connect() resolved, onconnect ran and the auto-flusher drained the queue while the SELECT reply was still due, so a command sent from there was pipelined behind the SELECT. When the server rejected the SELECT (index out of range, cluster mode) that command had already run against database 0, and its promise was rejected with the SELECT's error. The client now stays Connecting until the SELECT reply arrives. The accepted HELLO reply is held in pending_hello so connect() still resolves with it. A rejected SELECT fails the handshake with ERR_REDIS_SERVER_ERROR and the server's text, and a SELECT reply that never comes is bounded by connectionTimeout like the rest of the handshake.
…y on the held HELLO authenticate() is the one place that decides whether a SELECT goes out with the handshake, so it sets is_selecting_db_internal there again and hello_accepted() tests the flag instead of re-deriving it from the database index. handle_response() tells the two handshake replies apart by whether a HELLO reply is held.
|
Reproduced on 1.4.3 (and main at a3e0ab6) against redis 8.0.2 before writing the fix: import { RedisClient } from "bun";
const db0 = new RedisClient("redis://127.0.0.1:6379");
const client = new RedisClient("redis://127.0.0.1:6379/99", { autoReconnect: false });
console.log(await client.connect().then(() => "resolved", e => e.code), client.connected); // resolved true
console.log(await client.set("k", "v").then(() => "resolved", e => e.message)); // ERR DB index is out of range
console.log(await db0.get("k")); // "v" <- written to db 0With this branch: CI (build 112832): the valkey suites pass on every lane. Three jobs are red, none touch this diff:
The other entries in the annotation passed on retry. Ready for review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesValkey connections remain in Valkey handshake completion
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Valkey clients now wait for database selection to complete before reporting connection readiness, preventing commands from running against an unselected database. The updated behavior and failure paths are covered without an identified remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Deferring to a human because it changes the user-visible connect() contract (resolve → reject on a failed SELECT, ERR_REDIS_INVALID_COMMAND → ERR_REDIS_SERVER_ERROR) and sits in the middle of several adjacent handshake PRs the description names.
What was reviewed:
pending_hellolifecycle — initialized at bothJSValkeyClientconstruction sites and reset in bothon_close()andconnect(), matching every existingis_selecting_db_internalreset.handle_responsedispatch —pending_hello.take()correctly routes the second handshake reply tohandle_select_response, andhandshake_completeis now the onlyStatus::Connectedtransition;get_timeout_intervalkeying on!= Connectedis what makesconnectionTimeoutbound a hung SELECT.- New tests — buffered framing via
readCommands,port: 0, control-connection round trips instead of sleep, resources released infinally, combined-object.toEqualassertions.
Extended reasoning...
Overview
The PR reorders the Valkey client's handshake state machine so that when the URL specifies a database, the client remains Connecting until the SELECT reply arrives (previously it flipped to Connected on the HELLO reply and handled SELECT afterward). A new pending_hello: Option<RESPValue> field stashes the accepted HELLO while waiting for SELECT, handle_response dispatches handshake replies based on it, and a single handshake_complete() is the one place that sets Connected, resets reconnect state, and calls on_valkey_connect. connection_ready() drops its now-redundant is_selecting_db_internal check, and a rejected SELECT surfaces as RedisError::ServerError instead of InvalidCommand. Tests are updated in test/js/valkey/reliability/connection-failures.test.ts: the existing rejected-SELECT test now asserts connect() rejects and onconnect doesn't fire on the failed handshake, plus two new tests with a stub server that withholds the SELECT reply.
Security risks
None identified. This is internal connection-state sequencing for a database client; no auth, crypto, path, or untrusted-input parsing is touched. The RESP parsing path is unchanged — only which handler a reply is routed to during the handshake.
Level of scrutiny
Medium-high. The mechanical change is small and the state resets are complete (both construction sites, on_close, connect()), but it alters user-observable semantics: client.connected stays false longer, onconnect fires later, connect() now rejects where it previously resolved on a failed SELECT, the error code changes, and with enableOfflineQueue: false a command in the SELECT window is now rejected instead of accepted. The PR description also names four adjacent in-flight PRs (#39542, #41829, #41645, #36071) touching the same functions, so a maintainer should confirm the composition and the intended error-code contract.
Other factors
The lifecycle audit checks out: pending_hello is reset alongside is_selecting_db_internal at every site that resets the latter, and get_timeout_interval already keys on status != Connected, so extending the Connecting window is what makes connectionTimeout bound a never-answered SELECT — no new timer wiring needed. Tests follow repo conventions closely (buffered framing, port: 0, control-connection round trips to turn the event loop rather than sleep, Promise.withResolvers for the SELECT-arrived signal, finally cleanup, combined .toEqual objects). No CODEOWNERS entry covers src/runtime/valkey_jsc/. The exit reason was dry_streak, so the hunt ran to completion.
|
Updated 6:27 AM PT - Sep 8th, 2026
❌ @robobun, your commit 31150ad has 1 failures in 🧪 To try this PR locally: bunx bun-pr 41967That installs a local version of the PR into your bun-41967 --bun |
Problem
redis://host/3) the client writesHELLOandSELECT 3together, but becameConnectedon the HELLO reply:connect()resolved,onconnectran and the auto-flusher drained the queue with the SELECT reply still due.ERR DB index is out of range, cluster mode), the command had already run on database 0 and got its error.handle_hello_response(src/runtime/valkey_jsc/valkey.rs) setStatus::Connectedbefore the SELECT reply, andon_auto_flushgates on that status.Fix
Connectinguntil the SELECT reply.handshake_completeis the one place that setsConnected, soconnected,onconnect,connect()and the auto-flusher wait for it.pending_hellokeeps the HELLO reply forconnect(). This is the rule @alii set when closing redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) #34829.ERR_REDIS_SERVER_ERRORand the server's text (wasERR_REDIS_INVALID_COMMAND, valkey: add ERR_REDIS_SERVER_ERROR for server error replies #39573 moved the other server error replies).connectionTimeoutnow bounds a SELECT reply that never comes.test/js/valkey/reliability/connection-failures.test.ts(two new tests, one updated, all fail on stock bun), the docker-freetest/js/valkeysuites, redis 8.0.2.Background
HELLO 3 [AUTH user pass], thenSELECT nwhen the URL names a database, in one flush. Replies pair with commands by order.onconnectsent went out before the SELECT reply was handled.Notes
redis://127.0.0.1:6379/99:await client.connect()resolves withconnected === true,client.set(k, v)rejects withERR DB index is out of range, and a second client on db 0 readsvback fromk.Status(redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) #34829's closing note).authenticate()is the one place that decides whether a SELECT goes out with the handshake and marks it in the existingis_selecting_db_internal.pending_hellois the held HELLO reply, and while it is held the next reply is the SELECT's. The planned fold of the handshake phases into one connection enum can move both into its SELECT-phase variant.handshake_complete()is the singleConnectedtransition for it to absorb.connect()andonclosestill report the genericERR_REDIS_CONNECTION_CLOSED"Connection closed" for a failed handshake. valkey: reject connect() and call onclose with the real failure reason #39542 makes them carry the real reason (the server's SELECT error here) and will want a small rebase over the updated SELECT test.Connecting. With both changes, a rejected handshake SELECT is retried underautoReconnectlike a rejected HELLO, and is terminal without it. The "no retry" comment in the updated test describes main and moves with redis: count a retry's failed handshake as one attempt instead of ending the client #41829.SELECTacross reconnect) and redis: drop unsolicited frames that arrive before the HELLO reply #36071 (stray frames before HELLO) touch the same functions. Neither changes when the client becomesConnected. redis: remember the database a SELECT chose across reconnect and duplicate() #41645 changes the SELECT predicate inauthenticate()only, which is the one place this PR reads it.connect()settled on HELLO before the SELECT reply. It now asserts thatconnect()rejects, thatonconnectdoes not run for the failed handshake, and that aping()sent fromonconnecton the next connection resolves.SET arrived before SELECT was answeredandconnected === truebefore the SELECT reply.Connectedwith its commands queued for good (the idle timer, off by default, was the only bound).enableOfflineQueue: false, a command issued while the SELECT reply is pending is now rejected like any other command on a client that is not connected yet. Before, it was accepted and flushed early.valkey-gc.test.ts"RESP map keys are not leaked" times out at 5 s on this container's debug build with and without the change.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/valkey/reliability/connection-failures.test.ts