redis: close the socket when the connection fails - #33479
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:17 PM PT - Jul 6th, 2026
❌ @robobun, your commit b20c3d5 has some failures in 🧪 To try this PR locally: bunx bun-pr 33479That installs a local version of the PR into your bun-33479 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
83cb50d to
b5c4ae4
Compare
|
Checked the four issues the find-issues bot suggested. None of them are fixed by this PR — please don't paste that
The bot seems to have matched on the words "onclose never fires" and "connection closed" rather than on the code path. |
There was a problem hiding this comment.
Thanks — making is_manually_closed unconditional resolves both issues I raised (the SELECT reconnect loop and the unarmed timeout). Leaving the final call to a human since this makes every fail() terminal even with autoReconnect: true (the PR description's "the client reconnects and is usable again" bullet is now stale), which is a reasonable design but a user-visible semantics change worth a maintainer nod.
Extended reasoning...
Overview
The PR changes ValkeyClient::fail_with_js_value() in src/runtime/valkey_jsc/valkey.rs to unconditionally set is_manually_closed = true and call close(), replacing the previous if !connection_ready() guard that only closed during the handshake. A new test file test/js/valkey/reliability/connection-teardown.test.ts spins up an in-process RESP3 mock server and asserts that a post-auth protocol error (a) closes the socket and fires onclose with autoReconnect: false, and (b) is terminal (no re-dial) with autoReconnect: true, with connect() able to revive the client.
Change since my last review
My earlier 🔴 comment was against a prior revision where is_manually_closed stayed guarded by !connection_ready() and only close() was hoisted out — that version would have reconnect-looped forever on a bad DB index. The current revision sets is_manually_closed = true unconditionally, which resolves that (no reconnect ever runs from fail()), and also moots the 🟡 nit about the unarmed connection-timeout on the reconnect path. The commit message explicitly cites the retry_attempts reset in handle_hello_response() as motivation, matching my earlier analysis.
Security risks
None. No new input parsing, no auth/crypto surface, no untrusted data handling changed. The mock RESP server in the test is bound to 127.0.0.1 on port 0.
Level of scrutiny
Medium. The Rust change is 9 lines in one function, but it sits at the heart of the Redis client's connection-lifecycle state machine and encodes a design decision: any failure that reaches fail() (parser error, idle timeout, bad SELECT, unrecognised push) is now terminal regardless of autoReconnect, and only peer-initiated socket closes still auto-reconnect. That's defensible and well-argued in the code comment and commit message, and it's a strict improvement over main (stranded → terminal + revivable), but it is a user-visible semantics change to autoReconnect that a maintainer should sign off on.
Other factors
- The PR description still describes the earlier revision's behavior ("
autoReconnect: true→ the client reconnects and is usable again"), which now contradicts both the code and the second test ("a protocol error is terminal even with autoReconnect enabled"). Worth updating before merge to avoid confusing future readers. - Idle timeout going through
fail()means an idled connection is now terminal rather than auto-reconnected — probably fine (it was stranded before anyway), but worth a maintainer confirming that's the intended semantics for users who set bothidleTimeoutandautoReconnect: true. - Tests are well-constructed: hermetic (in-process TCP server, no real Redis), await conditions rather than sleeping, and cover both branches of the
on_closepolicy.
|
Thanks. On the stale description: it was rewritten in b5c4ae4 and no longer claims the client reconnects, so the review just caught an older snapshot of the body. The current text spells out the terminal policy and tabulates what each You're right that the semantics change is the thing worth a maintainer's nod, so to make that call cheap, here is the whole decision: Option A (this PR). Every Option B. Let post-handshake failures fall into auto-reconnect. This needs two more fixes before it is even safe:
So Option B is strictly more machinery for a behaviour that is wrong for at least one of its callers. That's why I went with A. On your Happy to flip to B (plus the two fixes above) if a maintainer prefers it. |
fail_with_js_value() only closed the socket when the handshake had not completed. A failure on an established connection set flags.failed, rejected every pending command, and then left the socket open: .connected kept reporting true, onclose never fired, and every later command rejected with ERR_REDIS_CONNECTION_CLOSED forever, with nothing to tell the app. Close the socket, and mark the close as deliberate. The failures that reach here are the client's own (a frame the parser cannot read, a non-OK SELECT, an idle timeout, a rejected handshake), so they recur on reconnect; worse, handle_hello_response() resets retry_attempts on every successful HELLO, so a post-handshake failure would never reach maxRetries and would re-dial forever. A socket the peer drops never goes through fail(), so autoReconnect still covers it, and connect() revives a failed client. close() is idempotent, so the callers that had already closed the socket are unaffected.
b5c4ae4 to
b20c3d5
Compare
CI status (build 69222, sha b20c3d5)Diff is green. The rebase onto The only red is darwin infra, unrelated to this change:
I'm not pushing a The diff itself is ready for review; the one open call is the design question above (should |
## What
A RESP simple string (`+`) or error (`-`) reply longer than 512KB is
rejected with `ERR_REDIS_INVALID_RESPONSE "Failed to read data (buffer
path)"`, after which the client rejects every subsequent command with
`ERR_REDIS_CONNECTION_CLOSED` while still reporting `connected: true`.
1.3.14 resolved the same reply fine.
## Repro
```js
// against redis-server 8.4.0
await client.send("EVAL", ["return redis.status_reply(string.rep('x', 600000))", "0"]);
// 1.3.14: resolves, length 600000; subsequent ping: PONG
// 1.4: rejects ERR_REDIS_INVALID_RESPONSE "Failed to read data (buffer path)"
// subsequent ping: rejects ERR_REDIS_CONNECTION_CLOSED
// boundary: 524288 OK, 524289 fails
```
## Cause
`read_until_crlf` in `src/valkey/valkey_protocol.rs` was capped at
`MAX_LINE_LEN = 512 * 1024` by a later hardening pass; the 1.3.14 Zig
parser (`readUntilCRLF`) scanned the whole buffer. The cap backs every
line-terminated RESP type (`+ - : _ , # (`). The RESP spec places no
length limit on these, and a real Redis server emits arbitrary-length
`+` / `-` lines for Lua `redis.status_reply()` / `redis.error_reply()`.
Bulk strings (`$`) carry a length prefix and were never affected.
## Fix
Raise `MAX_LINE_LEN` to match `MAX_BULK_LEN` (512MB, the server-side
`proto-max-bulk-len` default), so line-terminated replies get the same
buffer-growth bound as length-prefixed blobs. The scan window and
`LineTooLong` check are kept so a server that streams an unterminated
line still cannot grow the read buffer without bound. The incremental
`ReplyScanner`'s `crlf_skip` bookkeeping keeps the scan linear across
socket reads.
The second half of the report (a protocol error latches `flags.failed`
without closing the socket, leaving a zombie client) is #33479; this PR
only removes the trigger that made valid replies hit that path.
## Verification
`test/js/valkey/reliability/resp-nesting-depth.test.ts` gains three
cases driven by an in-process mock server (no Docker): a 600KB `+` reply
resolves and the client serves a follow-up `PING`; the 524289-byte
boundary resolves; a 600KB `-` reply rejects with the server's original
error text and the client survives. All three fail on the released bun
with `ERR_REDIS_INVALID_RESPONSE "Failed to read data (buffer path)"`
and pass with the fix.
`test/js/valkey/valkey-gc.test.ts`'s "line terminator never arrives"
case is updated: 600KB unterminated followed by socket close is now
under the cap, so the pending command is rejected with
`ERR_REDIS_CONNECTION_CLOSED` rather than `ERR_REDIS_INVALID_RESPONSE`.
<!-- robobun:evidence:begin -->
---
**[review]** gate passed · iteration 1 · 4 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: 4 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/reliability/resp-nesting-depth.test.ts test/js/valkey/valkey-gc.test.ts test/js/valkey/valkey-incremental-scan.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (0ea8158)
test/js/valkey/valkey-gc.test.ts:
(pass) RedisClient survives GC after a command throws during argument validation [692.15ms]
173 | });
174 | try {
175 | await client.send("PING", []);
176 | expect.unreachable();
177 | } catch (error: any) {
178 | expect(error.code).toBe("ERR_REDIS_CONNECTION_CLOSED");
^
error: expect(received).toBe(expected)
Expected: "ERR_REDIS_CONNECTION_CLOSED"
Received: "ERR_REDIS_INVALID_RESPONSE"
at <anonymous> (/workspace/bun/test/js/valkey/valkey-gc.test.ts:178:28)
(fail) rejects a RESP simple-string reply whose lin
... (truncated)
release without fix: all passed
bun test v1.4.0-canary.1 (af1fbb4)
test/js/valkey/valkey-gc.test.ts:
(pass) RedisClient survives GC after a command throws during argument validation [29.77ms]
(pass) custom setter with a foreign receiver throws instead of corrupting the heap [19.23ms]
(pass) RedisClient survives GC across many short-lived instances [18.26ms]
(pass) rejects a RESP simple-string reply whose line terminator never arrives [27.35ms]
(pass) getBuffer replies survive GC with adopted backing stores intact [64.13ms]
test/js/valkey/valkey-incremental-scan.test.ts:
(pass) Valkey reply torn across socket reads > BulkString ($) torn at byte 5 decodes (baseline) [5.31ms]
(pass) Valkey reply torn across socket reads > BulkString ($) torn at byte 10 decodes (baseline) [4.22ms]
(pass) Valkey reply torn across socket reads > BulkString ($) torn at byte 19 decodes (baseline) [4.12ms]
(pass) Valkey reply torn across socket reads > BulkString ($) torn at byte 21 decodes (baseline) [4.06ms]
(pass) Valkey reply torn across socket reads > VerbatimString (=) torn at byte 5 decodes instead of failing the connection [4.01ms]
(pass) Valkey reply torn across socket reads > VerbatimString (=) torn at byte 1
... (truncated)
```
</details>
<details><summary>passes on PR (with fix)</summary>
```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/reliability/resp-nesting-depth.test.ts test/js/valkey/valkey-gc.test.ts test/js/valkey/valkey-incremental-scan.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (0ea8158)
test/js/valkey/valkey-gc.test.ts:
(pass) custom setter with a foreign receiver throws instead of corrupting the heap [536.19ms]
(pass) rejects a RESP simple-string reply whose line terminator never arrives [530.92ms]
(pass) RedisClient survives GC after a command throws during argument validation [636.70ms]
(pass) RedisClient survives GC across many short-lived instances [799.75ms]
(pass) getBuffer replies survive GC with adopted backing stores intact [2322.86ms]
test/js/valkey/valkey-incremental-scan.test.ts:
(pass) Valkey reply torn across socket reads > BulkString ($) torn at byte 5 decodes (baseline) [104.02ms]
(pass)
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped) in 13660ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/90] gen ErrorCode+*.h
[2/47] gen cpp.rs (cppbind)
[3/47] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
- ResolveMessage (13 fields)
- BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
- Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
- ResourceUsage (8 fields)
- Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
- CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
- FileSystemRouter (5 fields)
- FrameworkFileSystemRouter (2 fields)
- MatchedRoute (8 fields)
Found 1 classes from /w
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
src/valkey/valkey_protocol.rs | 6 +-
.../valkey/reliability/resp-nesting-depth.test.ts | 70 ++++++++++++++++++++++
test/js/valkey/valkey-gc.test.ts | 21 +++----
test/js/valkey/valkey-incremental-scan.test.ts | 5 +-
4 files changed, 84 insertions(+), 18 deletions(-)
```
</details>
**gate history** · 1 passed · 1 rejected · iteration 1
<details><summary>evidence per changed file</summary>
```
file reads edits tests
src/valkey/valkey_protocol.rs 2 3 0
test/js/valkey/reliability/resp-nesting-depth.test.ts 1 1 0
test/js/valkey/valkey-gc.test.ts 3 3 0
test/js/valkey/valkey-incremental-scan.test.ts 1 1 0
```
</details>
<!-- robobun:evidence:end -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Superseded by #37993, which makes the same fail() change together with the related onclose/status fixes; this one can be closed once that lands. |
1 participant
What
Bun.RedisClientcan end up permanently half-alive:flags.failedis set, every command rejects withERR_REDIS_CONNECTION_CLOSED, but the socket is still open,.connectedstill reportstrue, andonclosenever fires. The application is never told the connection is dead.ValkeyClient::fail()is the single entry point for every connection-level failure: a RESP frame the parser can't read, a non-OKSELECT, the idle timeout, an unrecognised RESP3 push. On an established connection none of them closed the socket.Repro
A scripted RESP3 server that answers
HELLOand then sends a byte that is not a RESP type:Every later command rejects, forever, and nothing ever tells the app.
Cause
The socket was only closed while the handshake was still in flight. Once
is_authenticatedflipped,fail()rejected the queues and returned, leaving a live socket attached to a client that refuses to use it.statusstaysConnectedbecause only the socket'son_closecallback moves it toDisconnected, and that callback is the one thing that runs theonclose/ reconnect policy.Fix
Close the socket, and mark the close as deliberate:
Deliberate, rather than letting it fall into auto-reconnect, because every failure that reaches
fail()is the client's own and recurs on reconnect.handle_hello_response()resetsretry_attempts = 0on each successfulHELLO, so a post-handshake failure can never reachmaxRetries— it would re-dial forever. Measured, with the reconnecting variant of this patch andredis://host/99(bad DB index): 22 handshakes in 1.5s,onclosenever fired. With the patch as it stands: one handshake, oneonclose,connected === false.What does not change:
fail()—on_close()'s reconnect branch handles it — soautoReconnectstill covers the case it exists for. Verified against a scripted server that destroys the connection: 2 handshakes, client reconnects, identical tomain.connect()revives a failed client (it clearsfailedandis_manually_closed), which is whattest/regression/issue/29925.test.tsalready pins.close()is idempotent (it early-returns on a detached or already-closed socket), so the callers that had closed the socket before reachingfail()(on_closeitself,on_connect_error,fail_handshake, the TLS-context failure) are unaffected.fail() callers, and what each does now
handle_hello_response(bad auth, RESP2 server)authenticate(OOM writing HELLO/SELECT)on_close(3 branches),on_connect_errorfail_handshake, TLS-context failureon_timer, pre-auth)on_dataparse error (post-auth)onclose, terminalSELECTfailure (error / non-OK/ wrong type)onclose, terminalon_timer, post-auth)onclose, terminalonclose, terminalVerification
test/js/valkey/reliability/connection-teardown.test.tsdrives both branches of the policy against an in-process RESP3 server (no Redis needed). Both tests hang onmain(the close never arrives) and pass with the fix. TheautoReconnect: truetest assertsonclosefires — which only happens on the terminal branch ofon_close, never on the reconnecting one — and that exactly 2 handshakes are seen, pinning the absence of a re-dial storm.test/js/valkey/is green, andtest/regression/issue/29925.test.ts(which spawns a realredis-serverand exercisesflags.failedthroughclose()/connect()) still passes.Relationship to #32858
This is the other half of the same report. #32858 fixes the trigger in the subscriber path — a
pmessage/smessage/invalidatepush made the client callfail()on itself — and rewrites the push-kind dispatch so those frames stop failing anything. It deliberately leavesfail()alone.This PR fixes what
fail()does once it is reached, from any of its callers, so no code path can strand a client infailed && connected. The two touch different functions and do not conflict.