redis: propagate the real failure reason to the connect() promise - #35509
redis: propagate the real failure reason to the connect() promise#35509robobun wants to merge 7 commits into
Conversation
WalkthroughChangesValkey connection error identity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:11 PM PT - Jul 24th, 2026
❌ @robobun, your commit fb01fcc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35509That installs a local version of the PR into your bun-35509 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![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: 4
🤖 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 `@test/js/valkey/reliability/connection-failures.test.ts`:
- Line 341: Remove the implementation-history comment near the reliability
regression test; retain the test name and existing behavior unchanged, since
only the issue URL comment should remain for this regression test.
- Around line 408-409: Update the timeout error assertion in the
connection-failure test to require the exact configured message “Connection
timeout reached after 200ms” instead of using a substring assertion. Keep the
existing error-code assertion unchanged.
- Around line 344-354: The stubServer helper currently treats arbitrary TCP
chunks as complete requests, suppresses socket errors, and exposes non-awaitable
cleanup. Update stubServer to frame requests or respond only after the initial
request is complete, propagate server and socket lifecycle failures through the
relevant promises, and return an async close() that awaits server shutdown.
Update every finally block using stubServer to await close() so all fixture
resources are released.
- Around line 415-428: Add a two-attempt reliability test alongside the existing
connectError test that enables autoReconnect, causes the first connection
attempt to fail with an auth or timeout reason, then causes the reconnect
attempt to end with a different terminal failure. Assert the final connectError
result uses the second attempt’s error code and message, covering stale-reason
reset behavior while preserving the existing single-failure coverage.
🪄 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: eb87f14b-694d-4f7e-ab2c-c994b3f2828d
📒 Files selected for processing (3)
src/runtime/valkey_jsc/js_valkey.rssrc/runtime/valkey_jsc/valkey.rstest/js/valkey/reliability/connection-failures.test.ts
A -WRONGPASS or -NOAUTH reply to HELLO, a connectionTimeout expiry, and a TCP connect error all surfaced as the same ERR_REDIS_CONNECTION_CLOSED / "Connection closed" when awaited via client.connect(). The server's actual error text was discarded. The failure was already passed to fail() which rejected queued commands with the right message, but on_valkey_close() then built a fresh generic error for the connection_promise and the onclose callback. Store the first failure in close_reason and use it in on_valkey_close() so connect() rejects with ERR_REDIS_AUTHENTICATION_FAILED + the server's WRONGPASS/NOAUTH text, ERR_REDIS_CONNECTION_TIMEOUT for a connectionTimeout, and "Failed to connect" for a TCP connect error. Fixes #3064
on_connect_error under autoReconnect left the stale "Failed to connect" close_reason alive into the next attempt; if that attempt then timed out before on_open(), the connect() promise reported the stale reason instead of ERR_REDIS_CONNECTION_TIMEOUT.
The stub only answers the initial HELLO so it now buffers until the full CRLF-terminated command is received and replies exactly once, avoiding any reliance on a single-write TCP read boundary.
a904270 to
fb01fcc
Compare
There was a problem hiding this comment.
LGTM — the stale-close_reason reset I flagged earlier is now handled in connect(), and the two remaining items (TLS handshake sibling, errno in "Failed to connect") are reasonable follow-ups that match the Postgres/MySQL sibling convention.
What was reviewed:
close_reasonset/clear points cover every attempt entry (do_connect,connect,on_open) and every terminal exit (failfirst-writer-wins →on_valkey_close().take()); traced the auto-reconnect path to confirm no stale-reason carryover.Box<[u8]>inclose_reasonis freed by.take()or field Drop — no leak.- New tests are hermetic (local
net.createServer, not gated on Docker); the stub now buffers untilHELLO+CRLF before replying, and the timeout test asserts the exact message.
Extended reasoning...
Overview
Adds a close_reason: Option<(Box<[u8]>, RedisError)> field to ValkeyClient so the connect() promise (and onclose callback) reject with the actual failure cause instead of a generic "Connection closed". fail() records the first failure; on_valkey_close() consumes it via .take(); on_connect_error() records "Failed to connect" for the TCP-level case. The field is reset alongside other per-attempt state in do_connect(), the private connect(), and on_open(). Four new hermetic tests exercise WRONGPASS, NOAUTH, connectionTimeout, and ECONNREFUSED using local net.createServer stubs.
Security risks
None. This is error-message plumbing on failure paths; no new parsing of untrusted input, no auth/crypto/permission changes. The recorded message bytes originate from the server's HELLO error line (already displayed to users via queued-command rejections before this PR) or from static literals.
Level of scrutiny
Moderate. The valkey client has intrusive refcounting and re-entrant close/fail paths, so any new mutable per-connection state needs its reset points enumerated. I traced them across two prior review passes: my earlier 🔴 (stale reason surviving into an auto-reconnect attempt because reconnect() → connect() didn't clear it) was fixed in 8610e6e / 643a531. The remaining lifecycle is: fail() guards on is_none() (first-writer wins), fail_with_js_value() sets is_manually_closed and closes the socket, on_close() re-enters fail() which now early-returns on flags.failed, and on_valkey_close() .take()s the reason exactly once. The Box<[u8]> drops naturally with the struct if never taken.
Other factors
- All prior review threads (mine and CodeRabbit's) are resolved. The two acknowledged follow-ups — TLS handshake failure still surfaces as generic (needs
close_reasonto hold a JSValue, not just bytes) andon_connect_errordiscarding the errno — are unchanged from pre-PR behavior and match the Postgres/MySQL siblings exactly. - The new
describeblock is not gated onisEnabled, so the tests run on every CI lane without Docker; the gate evidence confirms 4 fail on the released binary and pass with the fix. - The stub-server framing concern was addressed (buffers until
HELLO+ trailing CRLF, replies once); the timeout assertion is now exact. - No changes since my last inline comment other than the CI-retrigger commit.
|
CI build 80065 failed on agent availability, not on this diff:
This diff touches |
2 participants
Problem
Four distinct connect-time failures all surfaced as the same
ERR_REDIS_CONNECTION_CLOSED/"Connection closed"when awaited viaclient.connect():A misconfigured password read exactly like a network blip, which leads to retry storms against a permanent auth failure and makes it impossible to tell "wrong password" from "server down" from "timeout".
Cause
handle_hello_response()andon_connection_timeout()already callfail()with the right message andRedisError(queued commands were rejected with the correct error). Butfail()only rejects in-flight and queued commands; theconnect()promise is held inconnection_promiseand is rejected later byon_valkey_close(), which always built a fresh generic"Connection closed"error regardless of what caused the close.Fix
Store the first failure passed to
fail()in a newclose_reasonfield onValkeyClient, and haveon_valkey_close()use it (falling back to the generic error when none was recorded).on_connect_error()now records"Failed to connect"to distinguish a TCP-level connect failure. The field is reset alongsideflags.failedindo_connect()andon_open().After:
The
onclosecallback now also receives the specific error instead of the generic one.Verification
New tests in
test/js/valkey/reliability/connection-failures.test.tsuse localnet.createServerstubs for each case (no Docker needed). All four fail on the released binary and pass with this change.Related: #23467 (this change will surface the real reason that connection fails instead of the generic
ERR_REDIS_CONNECTION_CLOSED).[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file