redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) - #34829
redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC)#34829robobun wants to merge 93 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/valkey_jsc/valkey.rs (1)
1234-1244: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not complete the connection before
SELECTsucceeds.
mark_connected()resolves the JS connection promise whileSELECTis pending, and the SELECT path marks the handshake ready before validating its response. A command queued from that resolved promise can auto-flush after a failingSELECT(for example,SELECT 99) and execute against the default database before its promise is rejected.
src/runtime/valkey_jsc/valkey.rs#L1234-L1244: deferStatus::Connectedandon_valkey_connect()until database selection has succeeded.src/runtime/valkey_jsc/valkey.rs#L1003-L1025: validate+OKbefore settingHandshake::Ready, then complete the deferred connect flow.Add a failing-SELECT-followed-by-write regression test. As per coding guidelines, “Every behavioral change must include an automated regression test in the same change.”
🤖 Prompt for 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. In `@src/runtime/valkey_jsc/valkey.rs` around lines 1234 - 1244, The connection lifecycle currently resolves before database selection is confirmed. In src/runtime/valkey_jsc/valkey.rs#L1234-L1244, update mark_connected so Status::Connected and on_valkey_connect are deferred when selecting a database; in src/runtime/valkey_jsc/valkey.rs#L1003-L1025, validate the SELECT response is +OK before setting Handshake::Ready, then complete the deferred connection flow only on success. Add a regression test covering a failing SELECT followed by a write.Source: Coding guidelines
🤖 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 `@src/runtime/valkey_jsc/valkey.rs`:
- Around line 905-908: Condense the invariant comment above the subscription
acknowledgment handling to three lines or fewer, while preserving the
distinction that only plain subscribe acknowledgments enter subscriber mode and
pattern/shard acknowledgments merely settle their promises.
In `@test/js/valkey/reliability/resp-nesting-depth.test.ts`:
- Around line 364-386: Extend the reliability tests beside the existing
psubscribe() case with an equivalent ssubscribe() acknowledgment followed by
client.get("k"). Use the SSUBSCRIBE RESP acknowledgment and assert it returns 0,
then verify get still returns "value", preserving the same setup and cleanup
pattern to cover the sibling entry point.
---
Outside diff comments:
In `@src/runtime/valkey_jsc/valkey.rs`:
- Around line 1234-1244: The connection lifecycle currently resolves before
database selection is confirmed. In
src/runtime/valkey_jsc/valkey.rs#L1234-L1244, update mark_connected so
Status::Connected and on_valkey_connect are deferred when selecting a database;
in src/runtime/valkey_jsc/valkey.rs#L1003-L1025, validate the SELECT response is
+OK before setting Handshake::Ready, then complete the deferred connection flow
only on success. Add a regression test covering a failing SELECT followed by a
write.
🪄 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: 7725609b-f0e6-4574-81d2-4353b96e294f
📒 Files selected for processing (4)
src/runtime/valkey_jsc/command.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/valkey_jsc/valkey.rstest/js/valkey/reliability/resp-nesting-depth.test.ts
…on-subscriber test
|
Re the outside-diff finding about |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/valkey_jsc/valkey.rs (1)
419-424: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate auto-flushing on
connection_ready().
mark_connected()setsStatus::ConnectedbeforeSELECTsucceeds. A command queued from the connection callback can therefore be auto-flushed whileHandshake::SelectingDb; ifSELECTfails, that command may execute against the server’s previous database. Keep it queued untilHandshake::Ready, and add a regression covering a failed SELECT plus an immediately queued pipelineable write.Proposed fix
- if self.status != Status::Connected { + if !self.connection_ready() { self.auto_flusher.registered.set(false); return false; }🤖 Prompt for 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. In `@src/runtime/valkey_jsc/valkey.rs` around lines 419 - 424, Update the auto-flusher registration decision in the surrounding flush logic to require connection_ready() in addition to pending pipelineable work and remaining queue entries, so commands remain queued throughout Handshake::SelectingDb and only flush after Handshake::Ready. Add a regression test covering a failed SELECT followed by an immediately queued pipelineable write, verifying the write is not executed against the prior database.
🤖 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.
Outside diff comments:
In `@src/runtime/valkey_jsc/valkey.rs`:
- Around line 419-424: Update the auto-flusher registration decision in the
surrounding flush logic to require connection_ready() in addition to pending
pipelineable work and remaining queue entries, so commands remain queued
throughout Handshake::SelectingDb and only flush after Handshake::Ready. Add a
regression test covering a failed SELECT followed by an immediately queued
pipelineable write, verifying the write is not executed against the prior
database.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d6362e7-bed2-4878-bc48-580d9c3cf76d
📒 Files selected for processing (2)
src/runtime/valkey_jsc/valkey.rstest/js/valkey/reliability/resp-nesting-depth.test.ts
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
@robobun fix conflicts
Resolve conflicts in js_valkey.rs: - new_disconnected() now uses RefCountedTimer::new() from #34760 - clone_without_connecting keeps the new_disconnected() form - on_valkey_reconnect uses reconnect_timer.arm() from #34760 - destroy path keeps timer ref_held asserts from #34760; ssl_ctx is OwnedSslCtx so the manual SSL_CTX_free block is dropped - restore core::cell::Cell import for RefCountedTimer.ref_held
|
Merged |
There was a problem hiding this comment.
All three regressions I flagged earlier (psubscribe subscriber-mode, unsettled connectionPromise on reconnect error, send('SUBSCRIBE') leaky-proxy gate) are addressed in a1df82d / 77fc987 / edb5524, and no new issues surfaced after the main merge. Deferring to a human for the explicitly-listed user-facing behaviour changes (psubscribe return shape, blob-error reject, undefined/null varargs now throw, PUNSUBSCRIBE auto-pipeline fix) and because the docker-gated integration suite (~1065 tests) doesn't run locally.
What was reviewed:
- Handshake enum vs. old bool pair —
connection_ready()/mark_connected()ordering preserved from main; SELECT-before-resolve timing unchanged. reject_all_pending_commandsfull-drain andLinearFifo::iter()ring-wrap coverage.sub_count > 0gate foradd_subscription()covers all three subscribe entry points (psubscribe / send SUBSCRIBE / send SSUBSCRIBE), with mock-server tests.OwnedSslCtxRAII replacing manualSSL_CTX_free;on_closesocket-ref release ordering.
Extended reasoning...
Overview
Net -1251 LOC hardening pass over the Redis/Valkey client: replaces is_authenticated/is_selecting_db_internal bools with a Handshake enum, fixes several promise-settlement leaks (reject_all_pending_commands early-return, wrapped-ring readable_slice(0) scans, subscribe-ack popping unrelated promises), reworks reconnect failure to actually fail the client, and collapses ~1400 lines of duplicated per-command boilerplate in js_valkey_functions.rs into shared require_arg/collect_varargs helpers plus meta-parametrized macros. Also flattens the mod.rs alias maze, prunes dead RedisError variants and unused Cargo deps, moves _secure to RAII OwnedSslCtx, and rewrites valkey.classes.ts as a table.
Security risks
None identified. The RESP parser changes (read_blob, read_aggregate_header, expect_crlf) tighten bounds handling — the buffer.len() - pos < len check avoids the pos + len wrap the old code had. TLS context handling moves from a raw *mut SslCtx + manual SSL_CTX_free to OwnedSslCtx RAII, and the reject_unauthorized path is unchanged.
Level of scrutiny
High. This is a large structural refactor of a network client's connection state machine with multiple deliberate user-facing behaviour changes that the PR description explicitly flags for review. The memory-safety surface (JSC promise strong refs, SSL_CTX ownership, ring-buffer iteration, socket keep-alive refs across reconnect) is non-trivial, and the primary test coverage (~1065 docker-gated integration tests) only runs in CI, not locally.
Other factors
- I flagged three regressions across earlier revisions; all were fixed with targeted commits and mock-server tests. This run found nothing new.
- The PR textually conflicts with 7+ open lifecycle-adjacent PRs; author has already rebased over #34760.
- Several open issues (#27861, #18895, #22812, #33103) plausibly overlap but are not claimed as fixed with dedicated regression tests — a maintainer should decide whether to require those before merge.
- The
get_reconnect_delay()computation is now dead inon_socket_closed()(delay is applied viaon_valkey_reconnect's timer, but thedelay_ms == 0early-bail from the old path is gone) — worth a quick sanity check that reconnect backoff still behaves as before, though this looks intentional.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/valkey_jsc/js_valkey.rs (1)
306-314: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep this ownership comment within three lines.
The nine-line doc block exceeds the repository comment limit; retain only the ref-count and offset invariants.
Proposed reduction
-/// Intrusive [`EventLoopTimer`] slot that owns one strong ref on -/// [`JSValkeyClient`] while armed. `ref_held` mirrors the `ref_()` taken in -/// [`arm`] so [`disarm`] and [`take_fire_ref`] release it exactly once even -/// when the fire/close/reconnect paths re-enter each other. -/// -/// [`EventLoopTimer`]: Timer::EventLoopTimer -/// [`arm`]: Self::arm -/// [`disarm`]: Self::disarm -/// [`take_fire_ref`]: Self::take_fire_ref +/// Owns one client ref while armed; `ref_held` balances `arm`, `disarm`, and firing. +/// The timer must remain first so dispatch can recover the enclosing client.As per coding guidelines, “Keep code comments to three lines or fewer.”
🤖 Prompt for 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. In `@src/runtime/valkey_jsc/js_valkey.rs` around lines 306 - 314, Shorten the documentation above EventLoopTimer to no more than three lines, retaining only the invariant that the armed slot owns one strong JSValkeyClient reference and that ref_held ensures disarm and take_fire_ref release it exactly once.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/valkey_jsc/js_valkey.rs`:
- Around line 306-314: Shorten the documentation above EventLoopTimer to no more
than three lines, retaining only the invariant that the armed slot owns one
strong JSValkeyClient reference and that ref_held ensures disarm and
take_fire_ref release it exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba04fa8f-ec76-471b-a440-f5f491dc1c28
📒 Files selected for processing (2)
src/jsc/bindings/ErrorCode.tssrc/runtime/valkey_jsc/js_valkey.rs
There was a problem hiding this comment.
No new issues found this pass — the earlier inline findings (psubscribe/send-SUBSCRIBE subscriber-mode regression, unrejected connectionPromise on reconnect-connect error, varargs throw semantics) are all addressed. Deferring to a human reviewer given the scope: this is a ~-1250 LOC structural rewrite of the Valkey client's connection lifecycle, promise-settlement paths, and RESP parser, plus several user-facing behaviour changes (psubscribe() return shape, blob-error reject-vs-resolve, undefined/null varargs now throw, PUNSUBSCRIBE auto-pipeline gate) that warrant a maintainer's sign-off.
What was reviewed:
- Handshake enum /
connection_ready()gating vs. the old bool pair — queued user commands still wait forReady. reject_all_pending_commandsfull-drain andLinearFifo::iter()covering both ring halves.- Subscribe-ack routing after the
sub_count > 0gate;send('SUBSCRIBE'/'PSUBSCRIBE'/'SSUBSCRIBE')no longer flips subscriber mode (mock-server tests cover all three). on_socket_closedreconnect predicate and the reconnect-error path now rejecting the cachedconnectionPromise.
Extended reasoning...
Overview
This PR restructures src/runtime/valkey_jsc/ and src/valkey/: replaces the is_authenticated/is_selecting_db_internal bools with a Handshake enum, rewrites on_close/on_connect_error into a shared on_socket_closed, fixes reject_all_pending_commands to drain fully, adds LinearFifo::iter() for wrapped-ring scans, changes RESPValue::read_value to return Option so Err is always a real protocol error, collapses ~140 hand-written prototype methods onto shared cmd_*! macros / collect_varargs, table-drives valkey.classes.ts, and prunes ~190 lines of dead code plus six unused Cargo deps. It also introduces ERR_REDIS_SERVER_ERROR, wraps _secure in OwnedSslCtx, and reorders subscribe/unsubscribe rollback so handler-map mutation happens only after send() succeeds.
Security risks
Low. TLS handling is touched (SSL_CTX moved to RAII OwnedSslCtx, TLS-context build errors now surfaced), but the verification/hostname-check path in on_handshake_ is unchanged in substance. URL parsing is extracted verbatim into parse_valkey_url. No new untrusted-input parsing beyond the existing RESP parser, whose bounds checks are tightened (read_blob subtracts instead of adding to avoid pos + len wrap).
Level of scrutiny
High. The Valkey client's connection lifecycle and in-flight promise bookkeeping are exactly the class of code where a subtle ordering bug produces forever-pending promises or response-mismatch (issue #27861). Three earlier review rounds on this PR each surfaced a real regression that was then fixed, which is evidence the change surface is risk-bearing. The docker-gated integration suite skips locally; CI must be green on those lanes before merge.
Other factors
- User-facing behaviour changes are the main reason to defer:
psubscribe()now resolves to a number instead of the raw push frame; RESP3 blob-errors now reject instead of resolving with anError;undefined/nullin varargs positions now throw where they previously skipped/truncated;PUNSUBSCRIBEis now correctly excluded from auto-pipelining. These are defensible but need a maintainer's call. - All prior inline threads (mine and CodeRabbit's) are resolved; the
mark_connected-before-SELECTordering was confirmed as pre-existing behaviour and left unchanged. - The PR conflicts textually with several open targeted-fix PRs (#33306, #32779, etc.) — merge ordering is a human decision.
coerce_redis_argnow passesallow_file = true(which rejects file-backed blobs) for the generic path, changing e.g.set(key, Bun.file(...))behaviour; the comment explains why, but it's another semantic shift worth a glance.
|
CI build 76661: all valkey/redis test files are green across every lane. The only |
2 participants
Hardening pass over the Redis/Valkey client (
src/runtime/valkey_jsc/+src/valkey/), focused on connection lifecycle correctness. Net -1251 LOC across 20 files.Connection lifecycle / state machine
is_authenticated/is_selecting_db_internalbooleans onConnectionFlagswith aHandshake { AwaitingHello, SelectingDb, Ready }enum.connection_ready()now requiresHandshake::Ready, and a previously reachableunreachable!()in the response dispatcher is gone.reconnect()previously called the outerJSValkeyClientwrapper that only fires theonclosecallback; it never setflags.failed, never rejected queued promises, never closed the socket. Now routes throughValkeyClient::fail_with_js_valueso a failed reconnect is a real client failure. The outer wrapper is renamedcall_onclose_handlerso the two are no longer confusable.oncloseon connect error: a synchronous error fromconnect()during auto-reconnect now firesoncloseinstead of being dropped.on_closesocket release: the event-loop socket ref is now released even whenfail()throws mid-teardown.on_closereconnect predicate: simplified and made the reconnect decision readable.on_connect_errornow reports the actual errno (e.g.ECONNREFUSED) instead of a generic "Connection closed".JsResultout ofconnect().on_auto_flushmade no progress, and the dead post-enqueue re-register is removed.Promise settlement correctness
reject_all_pending_commandsdrains fully: previously used?mid-drain, so the firstErr(Terminated)returned early and dropped the already-mem::replace'd queues, leaving remaining promises pending forever. Now runs both loops to completion and returns the first error afterwards.has_any_pending_commands/ queue scans usedreadable_slice(0)alone, which only yields the first contiguous segment of a wrapped ring buffer. AddedLinearFifo::iter()(with a unit test) and moved the scan onto it.SUBSCRIPTION_REQUEST.PromisePair/Entryeach carried a duplicateMetathat could diverge fromPromise.meta. The duplicates are deleted; divergence is now a compile error.send()succeeds, and the subscribe rollback only removes the handler that was just added.Protocol / parser
RESPValue::read_valuereturnsOptionsoErris always a real protocol error and "need more bytes" isNone. Extractedread_aggregate_header/read_n_values/read_n_entriesto collapse six near-identical loops.*-1→Null,_\r\nvalidated, andBigNumberconsistently decodes as string.SubscriptionPushMessagematch: dropped theis_reply_kindstring-compare hack;pmessagepattern is skipped before dispatch so the payload shape matchesmessage.ERR_REDIS_SERVER_ERROR: new error code for server-ERR/!blob-error replies (previously mislabelledERR_REDIS_INVALID_RESPONSE). Documented indocs/runtime/redis.mdx.RedisErrorvariants including the JSC-layerJSError/JSTerminatedthat don't belong in the protocol crate.User-facing behaviour changes (for review)
psubscribe()return value: now resolves to the handler count (a number), matchingsubscribe()'s documentedPromise<number>shape. Previously it resolved to the raw push frame{type:"psubscribe", data:[...]}.psubscribehas no.d.tsentry and is undocumented, so nothing published breaks, but it is a change.!blob-error now rejects the command promise (as simple-ERRreplies already do) instead of resolving with anErrorinstance.PUNSUBSCRIBEtypo: the auto-pipeline disallow set hadUNPSUBSCRIBE, sopunsubscribe()was always auto-pipelined. Now matches correctly, and the check is case-insensitive soclient.send("multi", ...)is caught too.undefined/nullnow throw: previouslydel/mget/unlink/touch/zrem/lpush/rpushetc. silently skippedundefined/nullin trailing arguments, andset/sadd/srem/hmgetsilently truncated at the first one (e.g.client.del('a', undefined, 'b')sentDEL a b). These now throwERR_INVALID_ARG_TYPEsynchronously. The published.d.tsnever acceptedundefinedin these positions, so type-correct callers are unaffected.throw_invalid_argument_type/ERR_MISSING_ARGSwith consistent codes and labels (e.g. the first arg is now labelledkey, notadditional arguments).Resource ownership
_securewrapped in the existingboringssl::c::OwnedSslCtxRAII type; manualSSL_CTX_freedropped.Address/TLSderiveClone; theunreachable!()reconstruction indisconnect()is gone.hset_implusesJSValue::to_slicefor RAII string release.WTFStringImplleak and swallowed throw in theinvoke_callbacksdebug-log path.SubscriptionCtxsaved-flags bools folded intoOption<SavedFlags>.Cleanup
mod.rsflattened: the 87-line alias maze (four paths toCommand, a deadindex.rs, an emptyValkeyContextZST, self-referential re-export loop) is now 5pub modlines + onepub use.ValkeyCommand.rsis renamedcommand.rs.connection_strings,Status::is_active,on_valkey_timeout, a duplicateUnwrapOrOom,reader_pos, unreachable subscriber branches, stale Zig-port comments.JsTerminated<T>alias deleted: it wastype JsTerminated<T> = JsResult<T>, which lied about narrowing and came with an identitynarrow_terminatedshim. Callers now see honest-> JsResult<()>.on_valkey_*forwarders inlined;global_objecttakes&self.valkey.classes.tstable-driven: 55 command prototypes hadlength: 0; now derived from a table.bun_valkeyCargo.toml: dropped six unused deps (bitflags,const_format,enum-map,enumset,libc,scopeguard).parse_valkey_url+new_disconnectedto dedupe the two constructor sites (Bun.redisdefault +new RedisClient); theBunObject.rscall site is now a one-liner.Verification
The docker-gated integration suites skip locally (no docker daemon); CI exercises them.
Possibly related issues
The lifecycle fixes here overlap with the symptoms in several open issues, but this PR does not include dedicated regression tests for them, so they are linked here rather than marked as fixed:
reject_all_pending_commandsdrain fix and wrapped-ringLinearFifo::iter()scan both touch in-flight promise bookkeeping across reconnects.connect()behaviour on failure): failed reconnects now reject queued promises and close the socket, andon_connect_errorsurfaces the real errno.RedisClient.onClosedoes not get invoked #22812 (onclosenot invoked):onclosenow fires whenconnect()errors during auto-reconnect.on_closenow releases the socket ref even whenfail()throws. See also redis: release the event loop when close() is called while subscribed #33104.nulltoonclosepanics duringclose()#29145: tangentially related at best; Fix crash when RedisClient onclose/onconnect is set to a non-callable #32085 is the targeted fix for RedisClient: assigningnulltoonclosepanics duringclose()#29145.Related open PRs that touch nearby lifecycle code
This is a structural pass and may textually conflict with several targeted fixes already open: #33306, #32779, #33479, #32803, #32858, #33450, #34760. Happy to rebase over whichever land first.
[review] gate passed · iteration 3 · 21 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 1 rejected · iteration 3
evidence per changed file