Conversation
… closed The Rust callers read errno after bsd_create_connect_socket and bsd_create_connect_socket_unix fail. Until now the POSIX path relied on close(2) leaving errno alone. Set it from the connect result the same way the Windows path and the bind failure above it already do, and keep it across the close of the long path dirfd on Linux.
|
Warning Review limit reached
Next review available in: 27 seconds Limit details: You’ve used all 5 included reviews currently available. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (7)
Comment |
|
which is better 39569 or 39542 ? keep one and close the other plz |
|
oh i see - just maintain the original pr. not a new one |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I also checked whether the TLS-context-failure path (which no longer calls fail() early) leaves the connection-timeout timer armed — it doesn't: close_without_socket_next_tick is entered before reset_connection_timeout, and the deferred close's on_close → on_valkey_close path releases the socket ref without needing the timer. The Strong in failure is cleared on every path that would otherwise leak it (connect(), on_open, and struct drop).
Extended reasoning...
This PR replaces the valkey client's flags.failed: bool with failure: Option<Strong> and threads the actual failure reason through on_close/on_connect_error/on_valkey_close. It also touches packages/bun-usockets/src/bsd.c (errno preservation across close(2)), extracts bun_errno::connect_errno for reuse between Bun.connect and the redis client, and adds an errno field to uws::ConnectError. The two inline findings are message-accuracy and test-hygiene nits; neither is blocking. Given the scope (GC-rooted state on a struct with re-entrant close paths, C-level errno handling, cross-PR merge-order notes for #39546/#39547, and user-visible error-message changes), a human maintainer should still review this rather than have it auto-approved.
| } | ||
| None => JSValkeyClient::connect_error_message(address, code), | ||
| }; | ||
| this.client_mut() | ||
| .on_close(valkey::CloseReason::DialFailed(&message)) |
There was a problem hiding this comment.
🟡 Calling close() while an InternalSocket::Connecting is in flight (async DNS / happy-eyeballs) now surfaces connect ECONNREFUSED <host>:<port> instead of the previous neutral Connection closed, because us_connecting_socket_close synthesises ECONNABORTED and connect_errno maps that to ECONNREFUSED. Nothing was refused — the user aborted; consider checking is_manually_closed (or special-casing ECONNABORTED) in on_connect_error and passing CloseReason::SocketClosed in that case. Nit: error code stays ERR_REDIS_CONNECTION_CLOSED and the window is narrow.
Extended reasoning...
What the bug is
When a user calls client.close() while the client's socket is an InternalSocket::Connecting (a hostname that did not parse as an IP literal and had no single cached DNS result — i.e. async DNS or happy-eyeballs is in flight), the connect() promise and onclose now receive the message connect ECONNREFUSED <host>:<port>. Before this PR they received the neutral Connection closed. Nothing was refused; the user aborted the dial themselves.
The specific code path
Step-by-step:
disconnect()→ValkeyClient::close(FastShutdown)→AnySocket::close→ConnectingSocket::close→us_connecting_socket_close(c).us_connecting_socket_close(socket.c:215-218):if (!c->error) c->error = ECONNABORTED;, then dispatchesus_dispatch_connecting_error(c, c->error).- That reaches
SocketHandler::on_connect_error(this, from_connecting(c), ECONNABORTED). socket.dns_error()readsc->error_is_dns ? c->error : 0;error_is_dnswas never set for a manual abort → returns0.- The DNS branch of
on_connect_erroris skipped (dns_error == 0), so it falls toJSValkeyClient::connect_error_message(address, ECONNABORTED). bun_errno::connect_errno(ECONNABORTED):ECONNABORTEDis not in theKEPTlist (ENOENT,ENOTSOCK,EACCES,EINVAL,ECONNRESET,EADDRINUSE,EADDRNOTAVAIL) → returnsSystemErrno::ECONNREFUSED.- Message becomes
b"connect ECONNREFUSED <host>:<port>";on_close(CloseReason::DialFailed(&message))is called. - In
ValkeyClient::on_close,is_manually_closed == trueandfailure.is_none(), so it takes the first branch and callsself.fail(message, ConnectionClosed), which records that string asself.failure. on_valkey_closerejects the pendingconnect()promise and callsonclosewith that recorded error.
Why existing code doesn't prevent it
Before this PR, on_connect_error ignored code entirely and called on_close() with no argument, which unconditionally used b"Connection closed". Now on_close takes the reason from the caller, and on_connect_error builds one from the errno without distinguishing a caller-initiated abort from a dial that actually failed.
The other close-while-connecting shapes are unaffected: a Connected semi-socket goes through the is_semi_socket branch of ValkeyClient::close (CloseReason::SocketClosed), and a fail()-initiated close of a Connecting socket already has failure.is_some(), so the second fail() inside on_close is a no-op and on_valkey_close reports the previously-recorded failure. Only the manual disconnect() while InternalSocket::Connecting regresses.
Concrete example
const c = new RedisClient('redis://some-hostname-not-in-cache:6379', { autoReconnect: false });
c.onclose = e => console.log(e.message);
const p = c.connect();
c.close();
// onclose and p now carry: "connect ECONNREFUSED some-hostname-not-in-cache:6379"
// before this PR: "Connection closed"Impact
Message-accuracy regression only. The error code stays ERR_REDIS_CONNECTION_CLOSED, the trigger window is narrow (manual close() during the brief async-DNS/happy-eyeballs window; hostname must not be an IP literal and not in the DNS cache), and the user called close() themselves so they already know why the connection ended. No functional breakage. Still worth noting since the PR's stated purpose is accurate failure messages, and per REVIEW.md "error messages are reviewed word-for-word as code."
How to fix
Either check this.client.get().flags.is_manually_closed in on_connect_error and pass CloseReason::SocketClosed in that case, or special-case code == ECONNABORTED (which uSockets uses specifically as its "caller aborted" sentinel) to CloseReason::SocketClosed. The former is more direct since is_manually_closed is exactly the signal that the user initiated this.
| } | ||
| }); | ||
| }); | ||
| await new Promise<void>(resolve => server.listen(0, "127.0.0.1", resolve)); |
There was a problem hiding this comment.
🟡 nit: stubServer(), closedPort(), and the two inline server setups in this describe block await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve)) without wiring server.on('error', reject), so a listen failure (e.g. EMFILE on a busy runner) would hang to the file timeout instead of surfacing the real error. The pre-existing helloServer() in this file already uses await once(server, 'listening') and comments why ("events.once() rejects if the listen fails instead of leaving the test to time out"); the new helpers should follow that convention. Also affects the listen calls at ~380 (closedPort), ~464 (tls.createServer) and ~500 (net.createServer).
Extended reasoning...
What the issue is
The new connect() error identity describe block adds several test helpers and inline server setups that await server.listen() via a resolve-only Promise:
stubServer()at line 373:await new Promise<void>(resolve => server.listen(0, "127.0.0.1", resolve))closedPort()at ~380: same pattern for bothlistener.listen()andlistener.close()- The inline
tls.createServerlisten at ~464 (certificate-verify test) - The inline
net.createServerlisten at ~500 (connect-after-auth-failure test)
None of these wire server.on('error', reject). In Node semantics the listen(port, host, callback) callback is registered as a one-shot 'listening' listener, so if listen emits 'error' instead (EMFILE, ENFILE, etc.), the promise never settles.
Why REVIEW.md flags this
REVIEW.md's "Tests reviewers reject" section is explicit: "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks." This is a repo-level check, not a subjective style preference.
The in-file convention it diverges from
The pre-existing helloServer() helper in the very same file already handles this correctly and documents why:
// events.once() rejects if the listen fails instead of leaving the test to time out.
listen: async () => {
server.listen(0, "127.0.0.1");
await once(server, "listening");
...
},events.once() automatically rejects when the emitter emits 'error', which is exactly the behaviour the new helpers are missing.
Step-by-step: how it manifests
- A CI runner is fd-exhausted (many parallel tests, ASAN quarantine holding fds, etc.).
net.createServer().listen(0, '127.0.0.1', resolve)is called insidestubServer().- The kernel returns
EMFILEfromsocket(2); Node emits'error'on the server with anEMFILEerror. - No
'error'handler is registered on the server, and theresolvecallback (a'listening'listener) never fires. - The
await new Promise<void>(...)never settles; the test hangs until the file-level timeout kills it. - CI reports a timeout with no indication that
EMFILEwas the actual cause, making the failure hard to diagnose.
With await once(server, 'listening') (or an explicit server.on('error', reject)), step 5 would instead reject with the EMFILE error and the test would fail immediately with a useful message.
Why this is a nit, not blocking
listen(0, '127.0.0.1')on an ephemeral loopback port essentially cannot hitEADDRINUSE(port 0 = OS-assigned) or address-unavailable; the only realistic failure is fd exhaustion, which is rare.- The identical resolve-only pattern already exists in pre-existing tests in this file (the "Auto-Reconnect In-Flight Commands" test and the "failed TLS handshake" test both do
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve))), so the PR is copying one of two existing local conventions rather than introducing a new anti-pattern. - Impact is diagnosability (hang vs. clear failure), not correctness of what the tests assert.
Fix
Swap each await new Promise<void>(resolve => server.listen(0, '127.0.0.1', resolve)) for:
server.listen(0, "127.0.0.1");
await once(server, "listening");(once from events is already imported at the top of the file.) This matches helloServer() and satisfies the REVIEW.md rule.
2 participants
Adopted from #39542 (@alii). The four commits from that branch are unchanged. Two commits are added on top, see "Added in this PR" below. This PR supersedes #39542.
Problem
connect()rejects withERR_REDIS_CONNECTION_CLOSED"Connection closed" for every failure.onclosegets the same generic error. A wrong password, a refused port, a missing unix socket path, a rejected certificate and an idle timeout all look the same (Connection to redis fails on prod 7.4 redis redislabs server (local redis works fine) #23467).on_valkey_close(src/runtime/valkey_jsc/js_valkey.rs) builds a new generic error for the promise and foronclose. The code that closed the socket already had the real one.Fix
ConnectionFlags.failed(a bool) is replaced byValkeyClient.failure: Option<Strong>. It holds the JS error that rejected the queued commands.fail_with_js_valuesets it once.connect()andon_openclear it. Every reader of the old bool readsis_some().on_valkey_closerejects theconnect()promise and callsonclosewith that recorded error. The commands, the promise andoncloseget one object.ValkeyClient::on_closetakes aCloseReason. The socket close callback passesSocketClosed("Connection closed").on_connect_errorpasses the errno or the resolver text in thenode:netshape:connect ECONNREFUSED 127.0.0.1:6379,getaddrinfo ENOTFOUND host. A dial that fails insideconnect()passes the errno it left, for exampleconnect ENOENT /run/redis.sock.uws::ConnectErrornow carries that errno.Bun.connect(WSA codes mapped on Windows, unix path errors kept, the restECONNREFUSED) moves to one function,bun_errno::connect_errno.Bun.connectand the redis client use it.connect()no longer callsfail()early. It hands its reason to the deferred close and goes throughon_close()like every other dial failure.connect()and when it failed from the event loop.Strongbecause the TLS context failure crosses one event loop turn beforeon_valkey_closeruns.ERR_REDIS_AUTHENTICATION_FAILED. Timeouts keepERR_REDIS_CONNECTION_TIMEOUTandERR_REDIS_IDLE_TIMEOUT. TLS verify errors keep their OpenSSL code. Every other close keepsERR_REDIS_CONNECTION_CLOSED.Added in this PR
bsd_create_connect_socketandbsd_create_connect_socket_unixseterrnofrom the connect result after they close the failed socket. The Windows path and thebind()failure above them already do this. Before, the POSIX path relied onclose(2)not touching errno. The Rust side reads errno right after these calls. The Linux close of the long path dirfd keeps errno for the same reason.on_connect_error. The host has a 64 byte label, so the lookup fails locally. It expectsgetaddrinfo ENOTFOUND <host>on one object.Verification
bun bd test test/js/valkey/reliability/connection-failures.test.ts: 38 pass, 13 skip (the skipped tests need the redis_unified container, they ran in the CI of valkey: reject connect() and call onclose with the real failure reason #39542).USE_SYSTEM_BUN=1 bun test ...connection-failures.test.ts -t "connect() error identity": all 10 tests fail. Nine fail onexpect(await get).toBe(err)(the objects differ), one hangs on the older binary.bun bd test test/js/valkey/valkey-gc.test.tsand the redis regression tests (22243, 23621, 24385, 29925): 40 pass.bun bd test test/js/node/net/node-net.test.ts(it covers the sync dial errnos that the bsd.c change touches): theENOENT,ENOTSOCKandEADDRINUSEtests pass. The 10 failures in that file are the same with the unmodified binary in this container (localhost resolution).Visible changes
connect()rejections and theoncloseargument carry the specific reason. Six existing tests in connection-failures.test.ts pinned the generic one and are updated:onclosegetsERR_REDIS_IDLE_TIMEOUT.connect ECONNREFUSED 127.0.0.1:<port>.connect()fromoncloseafter WRONGPASS:ERR_REDIS_AUTHENTICATION_FAILEDwith the server text.onclosegets the server'sERR DB index is out of range.connect()rejects with the TLS error (ECONNRESET).Not in this PR: a new error code for a refused connection, and a docs change for the error code list.
Note for the merge order: #39546 adds a call to
on_close()and #39547 readsflags.failed. Whichever of those lands after this PR needs a one line update (on_close(CloseReason::SocketClosed),failure.is_some()).Background
ValkeyClient(valkey.rs) owns the protocol state and the command queues.JSValkeyClient(js_valkey.rs) owns the JS wrapper, theconnect()promise and theonclosecallback.ValkeyClient::on_closeruns the retry policy and then callsJSValkeyClient::on_valkey_close, which settles the promise and callsonclose.fail()rejects every queued command with one JS error object and closes the socket. It is the place that knows the real reason.Strongis a GC root for one JS value. A JS value that is only stored in a native struct field is not seen by the garbage collector. The error must survive fromfail()untilon_valkey_closeruns, and one path crosses an event loop turn in between, so the field is aStrong. The queues in the same struct already holdJSPromiseStrongvalues, so the struct already has to be dropped on the JS thread.connect()(for example a unix path that does not exist, errno is read at once). From the event loop throughon_connect_error(a refused port or a failed lookup, uSockets passes the errno or the resolver code). Or after a socket exists (auth, TLS verify, timeouts), which goes throughfail(). The first two did not go throughfail()with a specific message before, which is whyon_closenow takes aCloseReason.connect(2)by returning no socket. The only channel for the errno is the thread's errno, which is why bsd.c has to set it after it closes the socket.Fixes #23467
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