Skip to content

valkey: reject connect() and call onclose with the real failure reason #39569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 6 commits into from
Closed

valkey: reject connect() and call onclose with the real failure reason #39569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d1ea20
redis: report the real failure reason to connect() and onclose
alii Aug 18, 2026
e8dba28
test: accept the errno message in the container-gated valkey tests
alii Aug 18, 2026
5a80b78
share the connect errno normalisation with the redis client
alii Aug 18, 2026
e99cc42
valkey: take the connect error by reference
alii Aug 18, 2026
44dfadb
usockets: restore the connect errno after the failed dial's socket is…
robobun Aug 18, 2026
ef6741a
valkey: test that a failed lookup rejects connect() with the resolver…
robobun Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions packages/bun-usockets/src/bsd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2066,11 +2066,13 @@ LIBUS_SOCKET_DESCRIPTOR bsd_create_connect_socket(struct sockaddr_storage *addr,

if (rc != 0) {
bsd_close_socket(fd);
/* bsd_do_connect_raw returned the connect error; re-arm it so the
* caller observes the connect failure rather than whatever closing the
* socket left behind. */
#ifdef _WIN32
/* bsd_do_connect_raw returned the WSA error; re-arm it so the Rust
* caller's WSAGetLastError() observes the connect failure rather than
* whatever closesocket() left behind. */
WSASetLastError(rc);
#else
errno = rc;
#endif
return LIBUS_SOCKET_ERROR;
}
Expand All @@ -2091,6 +2093,8 @@ static LIBUS_SOCKET_DESCRIPTOR internal_bsd_create_connect_socket_unix(const cha
bsd_close_socket(fd);
#ifdef _WIN32
WSASetLastError(rc);
#else
errno = rc;
#endif
return LIBUS_SOCKET_ERROR;
}
Expand Down Expand Up @@ -2127,7 +2131,9 @@ LIBUS_SOCKET_DESCRIPTOR bsd_create_connect_socket_unix(const char *server_path,
}
#elif defined(__linux__)
if (dirfd_workaround_for_unix_path_len != -1) {
int saved_errno = errno;
close(dirfd_workaround_for_unix_path_len);
errno = saved_errno;
}
#endif

Expand Down
30 changes: 30 additions & 0 deletions src/errno/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,36 @@ pub fn from_errno(errno: i32) -> SystemErrno {
SystemErrno::init(errno as i64).unwrap_or(SystemErrno::EIO)
}

/// The errno a failed `connect(2)` is reported with, as `node:net` reports it.
/// `raw` is what uSockets hands the connect-error callback (`SO_ERROR`, or the
/// WSA code on Windows) or what a dial that failed outright left in errno.
/// Unix-path connect errors keep their real code (a non-socket file is
/// `ENOTSOCK`, a permission-denied path is `EACCES`, a missing one is
/// `ENOENT`, an inexpressible path is `EINVAL`); everything else, including
/// Windows' `ENOTCONN` from the recv probe, is `ECONNREFUSED`.
pub fn connect_errno(raw: i32) -> SystemErrno {
#[cfg(windows)]
let raw: i32 = if raw >= 10000 {
SystemErrno::init(raw as u32)
.map(|e| e as i32)
.unwrap_or(SystemErrno::ECONNREFUSED as i32)
} else {
raw
};
const KEPT: [SystemErrno; 7] = [
SystemErrno::ENOENT,
SystemErrno::ENOTSOCK,
SystemErrno::EACCES,
SystemErrno::EINVAL,
SystemErrno::ECONNRESET,
SystemErrno::EADDRINUSE,
SystemErrno::EADDRNOTAVAIL,
];
KEPT.into_iter()
.find(|e| *e as i32 == raw)
.unwrap_or(SystemErrno::ECONNREFUSED)
}

#[cfg(not(windows))]
impl SystemErrno {
// `i64` covers every concrete call site (errno-range values).
Expand Down
47 changes: 3 additions & 44 deletions src/runtime/socket/socket_body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,50 +1128,9 @@ impl<const SSL: bool> NewSocket<SSL> {
)
} else {
debug_assert!(errno >= 0);
// uSockets hands us raw WSA codes (SO_ERROR, the recv probe) on
// Windows; map those onto the SystemErrno numbering the whitelist
// below compares against.
#[cfg(windows)]
let errno: c_int = if errno >= 10000 {
sys::SystemErrno::init(errno as u32)
.map(|e| e as c_int)
.unwrap_or(sys::SystemErrno::ECONNREFUSED as c_int)
} else {
errno
};
// Unix-path connect errors keep their real code (a non-socket file
// is ENOTSOCK, a permission-denied path is EACCES, a missing one is
// ENOENT, an inexpressible path is EINVAL); everything else stays
// ECONNREFUSED.
let errno_: c_int = if errno == sys::SystemErrno::ENOENT as c_int
|| errno == sys::SystemErrno::ENOTSOCK as c_int
|| errno == sys::SystemErrno::EACCES as c_int
|| errno == sys::SystemErrno::EINVAL as c_int
|| errno == sys::SystemErrno::ECONNRESET as c_int
|| errno == sys::SystemErrno::EADDRINUSE as c_int
|| errno == sys::SystemErrno::EADDRNOTAVAIL as c_int
{
errno
} else {
sys::SystemErrno::ECONNREFUSED as c_int
};
let code_ = if errno == sys::SystemErrno::ENOENT as c_int {
BunString::static_("ENOENT")
} else if errno == sys::SystemErrno::ENOTSOCK as c_int {
BunString::static_("ENOTSOCK")
} else if errno == sys::SystemErrno::EACCES as c_int {
BunString::static_("EACCES")
} else if errno == sys::SystemErrno::EINVAL as c_int {
BunString::static_("EINVAL")
} else if errno == sys::SystemErrno::ECONNRESET as c_int {
BunString::static_("ECONNRESET")
} else if errno == sys::SystemErrno::EADDRINUSE as c_int {
BunString::static_("EADDRINUSE")
} else if errno == sys::SystemErrno::EADDRNOTAVAIL as c_int {
BunString::static_("EADDRNOTAVAIL")
} else {
BunString::static_("ECONNREFUSED")
};
let errno_enum = bun_errno::connect_errno(errno);
let errno_: c_int = errno_enum as c_int;
let code_ = BunString::static_(<&'static str>::from(errno_enum));
#[cfg(windows)]
let errno_ = -sys::windows::libuv::e_discriminant_to_uv(errno_ as u16)
.unwrap_or(sys::windows::libuv::UV_ECONNREFUSED);
Expand Down
128 changes: 85 additions & 43 deletions src/runtime/valkey_jsc/js_valkey.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@
read_buffer: Default::default(),
reply_scanner: Default::default(),
retry_attempts: 0,
failure: None,
auto_flusher: Default::default(),
}),
global_object,
Expand Down Expand Up @@ -903,6 +904,7 @@
read_buffer: Default::default(),
reply_scanner: Default::default(),
retry_attempts: 0,
failure: None,
auto_flusher: Default::default(),
}),
global_object,
Expand Down Expand Up @@ -1006,25 +1008,22 @@

// If was manually closed, reset that flag
self.client_mut().flags.is_manually_closed = false;
// Explicit connect() should also clear the sticky `failed` flag so the
// Explicit connect() should also clear the sticky failure so the
// client can recover after prior connection attempts exhausted retries.
// Without this, every subsequent command rejects with "Connection has
// failed" forever — see https://github.com/oven-sh/bun/issues/29925.
self.client_mut().flags.failed = false;
self.client_mut().failure = None;
let self_br = BackRef::new(self);
let _update = scopeguard::guard(self_br, |p| p.update_poll_ref());

if self.client.get().status == valkey::Status::NeverConnected {
self.poll_ref.with_mut(|r| r.ref_(vm_event_loop_ctx()));

if let Err(err) = self.connect() {
debug!(
"first dial failed before a socket was opened: {}",
err.name()
);
debug!("first dial failed before a socket was opened: {:?}", err);
// Settled by the deferred close like a refused dial: the
// promise, onclose and the retry policy all go through on_close().
self.close_without_socket_next_tick();
self.close_without_socket_next_tick(self.dial_error_message(&err));
return Ok(promise);
}

Expand Down Expand Up @@ -1095,7 +1094,7 @@

let _guard = self.ref_scope();
let _timer_ref = self.timer.take_fire_ref(self);
if self.client.get().flags.failed {
if self.client.get().failure.is_some() {
return Ok(());
}

Expand Down Expand Up @@ -1152,10 +1151,32 @@
/// commands queued for the retry or the rejection, and a disconnect()
/// marks the close as manual for the task to honour. `update_poll_ref`
/// keeps the wrapper and the event loop alive for it like a dial would.
fn close_without_socket_next_tick(&self) {
fn close_without_socket_next_tick(&self, reason: Box<[u8]>) {
self.client_mut().status = valkey::Status::Connecting;
self.update_poll_ref();
self.enqueue_deferred_close(DeferredClose::WithoutSocket);
self.enqueue_deferred_close(DeferredClose::WithoutSocket { reason });
}

/// The `CloseReason::DialFailed` text for a dial uSockets turned down
/// before it had a socket, in the shape of `node:net`'s: `connect ENOENT
/// /run/redis.sock`. `errno` is raw (a WSA code on Windows); `connect_errno`
/// normalises it the way `Bun.connect` does.
fn dial_error_message(&self, err: &uws::ConnectError) -> Box<[u8]> {
let &uws::ConnectError::FailedToOpenSocket { errno } = err;
Self::connect_error_message(&self.client.get().address, errno)
}

fn connect_error_message(address: &valkey::Address, errno: i32) -> Box<[u8]> {
use std::io::Write;
let mut message = Vec::new();
let _ = write!(message, "connect {} ", bun_errno::connect_errno(errno));
match address {
valkey::Address::Unix(path) => message.extend_from_slice(path),
valkey::Address::Host { host, port } => {
let _ = write!(message, "{}:{}", bstr::BStr::new(host), port);
}
}
message.into_boxed_slice()
}

fn enqueue_deferred_close(&self, what: DeferredClose) {
Expand Down Expand Up @@ -1204,13 +1225,10 @@
});

if let Err(err) = self.connect() {
debug!(
"reconnect failed before a socket was opened: {}",
err.name()
);
debug!("reconnect failed before a socket was opened: {:?}", err);
// Same outcome as a dial that fails asynchronously: another retry,
// or fail() and a settled connect() promise once retries are used up.
self.close_without_socket_next_tick();
self.close_without_socket_next_tick(self.dial_error_message(&err));
return Ok(());
}

Expand Down Expand Up @@ -1412,12 +1430,17 @@
return Err(bun_jsc::JsError::Thrown);
}

// Create an error value
let error_value = protocol_jsc::valkey_error_to_js(
&global_object,
b"Connection closed",
protocol::RedisError::ConnectionClosed,
);
// The error the commands were rejected with. `on_close()` records one
// before every call here; the fallback only stands in for a
// finalized client, whose `fail()` cannot make JS values.
let error_value = match &self.client.get().failure {
Some(failure) => failure.get(),
None => protocol_jsc::valkey_error_to_js(
&global_object,
b"Connection closed",
protocol::RedisError::ConnectionClosed,
),
};

let _exit = self.vm().enter_event_loop_scope();

Expand Down Expand Up @@ -1477,7 +1500,7 @@
self.reconnect_timer.disarm(self);
}

fn connect(&self) -> Result<(), crate::Error> {
fn connect(&self) -> Result<(), uws::ConnectError> {
if self.client.get().status == valkey::Status::NeverConnected {
self.client_mut().status = valkey::Status::Disconnected;
}
Expand Down Expand Up @@ -1516,11 +1539,7 @@
};
if tls_ctx_failed {
self.client_mut().flags.enable_auto_reconnect = false;
self.client_fail(
b"Failed to create TLS context",
protocol::RedisError::ConnectionClosed,
)?;
self.close_without_socket_next_tick();
self.close_without_socket_next_tick(Box::from(&b"Failed to create TLS context"[..]));
return Ok(());
}
let ssl_ctx: Option<*mut uws::SslCtx> = match &self.client.get().tls {
Expand Down Expand Up @@ -1584,11 +1603,8 @@
// deferred close then rejects it or a retry sends it, like a
// refused dial.
Err(err) => {
debug!(
"first dial failed before a socket was opened: {}",
err.name()
);
self.close_without_socket_next_tick();
debug!("first dial failed before a socket was opened: {:?}", err);
self.close_without_socket_next_tick(self.dial_error_message(&err));
}
Ok(()) => self.reset_connection_timeout(),
}
Expand Down Expand Up @@ -1886,7 +1902,8 @@
this.client_mut().status = valkey::Status::Disconnected;
let _defer = scopeguard::guard(BackRef::new(this), |p| p.update_poll_ref());

this.client_mut().on_close()
this.client_mut()
.on_close(valkey::CloseReason::SocketClosed)
}

pub(crate) fn on_end(this: &JSValkeyClient, socket: SocketType<SSL>) {
Expand All @@ -1898,18 +1915,42 @@
// anything here
}

/// `code` is the errno the connect ended with, or, when `dns_error` is
/// set, the `getaddrinfo(3)` code of the lookup that failed instead.
pub(crate) fn on_connect_error(
this: &JSValkeyClient,
_socket: SocketType<SSL>,
_code: i32,
socket: SocketType<SSL>,
code: i32,
) -> JsResult<()> {
// Read before the socket is detached: uSockets keeps the connecting
// socket alive for the whole dispatch.
let dns_error = socket.dns_error();
// Ensure the socket pointer is updated.
this.client_mut().socket = Socket::SocketTcp(uws::SocketTCP::detached());
let _guard = this.ref_scope();
this.client_mut().status = valkey::Status::Disconnected;
let _defer = scopeguard::guard(BackRef::new(this), |p| p.update_poll_ref());

this.client_mut().on_close()
let address = &this.client.get().address;
let message =
match bun_cares_sys::c_ares::Error::init_eai(dns_error).filter(|_| dns_error != 0) {
Some(dns_err) => {
use std::io::Write;
let mut message = Vec::new();
// `code()` is `DNS_ENOTFOUND`; `node:dns` and `Bun.connect`
// report it as `getaddrinfo ENOTFOUND host`.
let _ = write!(
message,
"getaddrinfo {} {}",
&dns_err.code()[4..],
bstr::BStr::new(address.hostname())
);
message.into_boxed_slice()
}
None => JSValkeyClient::connect_error_message(address, code),
};
this.client_mut()
.on_close(valkey::CloseReason::DialFailed(&message))

Check warning on line 1953 in src/runtime/valkey_jsc/js_valkey.rs

View check run for this annotation

Claude / Claude Code Review

Manual close() during async DNS reports misleading 'connect ECONNREFUSED' 

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 ca
Comment on lines +1949 to +1953

@claude claude Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. disconnect() → ValkeyClient::close(FastShutdown) → AnySocket::close → ConnectingSocket::close → us_connecting_socket_close(c).
  2. us_connecting_socket_close (socket.c:215-218): if (!c->error) c->error = ECONNABORTED;, then dispatches us_dispatch_connecting_error(c, c->error).
  3. That reaches SocketHandler::on_connect_error(this, from_connecting(c), ECONNABORTED).
  4. socket.dns_error() reads c->error_is_dns ? c->error : 0; error_is_dns was never set for a manual abort → returns 0.
  5. The DNS branch of on_connect_error is skipped (dns_error == 0), so it falls to JSValkeyClient::connect_error_message(address, ECONNABORTED).
  6. bun_errno::connect_errno(ECONNABORTED): ECONNABORTED is not in the KEPT list (ENOENT, ENOTSOCK, EACCES, EINVAL, ECONNRESET, EADDRINUSE, EADDRNOTAVAIL) → returns SystemErrno::ECONNREFUSED.
  7. Message becomes b"connect ECONNREFUSED <host>:<port>"; on_close(CloseReason::DialFailed(&message)) is called.
  8. In ValkeyClient::on_close, is_manually_closed == true and failure.is_none(), so it takes the first branch and calls self.fail(message, ConnectionClosed), which records that string as self.failure.
  9. on_valkey_close rejects the pending connect() promise and calls onclose with 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.

}

pub(crate) fn on_timeout(this: &JSValkeyClient, socket: SocketType<SSL>) {
Expand Down Expand Up @@ -2026,13 +2067,12 @@
}
}

#[derive(Clone, Copy)]
enum DeferredClose {
/// Close the socket the finalized wrapper left behind.
Socket,
/// Run the close path for a dial that never produced a socket
/// (`close_without_socket_next_tick`).
WithoutSocket,
/// (`close_without_socket_next_tick`); `reason` is why it did not.
WithoutSocket { reason: Box<[u8]> },
}

pub(crate) struct ValkeyDeferredClose {
Expand All @@ -2048,11 +2088,11 @@
let _enqueue_ref = unsafe { ScopedRef::adopt(self.ctx) };
// SAFETY: live per the ref above; tasks run on the JS thread.
let this = unsafe { &*self.ctx };
match self.what {
match &self.what {
DeferredClose::Socket => {
crate::dispatch::fold(this.client_mut().close(uws::CloseCode::FastShutdown))
}
DeferredClose::WithoutSocket => {
DeferredClose::WithoutSocket { reason } => {
// Holding Connecting (see `close_without_socket_next_tick`) is
// what keeps a dial from starting in between; if one did, its
// own callbacks own the close path now, so only drop our ref.
Expand All @@ -2063,7 +2103,9 @@
// which release the ref the socket would have held.
this.ref_();
this.client_mut().status = valkey::Status::Disconnected;
let closed = this.client_mut().on_close();
let closed = this
.client_mut()
.on_close(valkey::CloseReason::DialFailed(reason));
this.update_poll_ref();
crate::dispatch::fold(closed);
}
Expand All @@ -2081,7 +2123,7 @@
DeferredClose::Socket => task.run(),
// The VM is going away: `on_close()` would run `onclose`, so only
// give back what `close_without_socket_next_tick` took.
DeferredClose::WithoutSocket => {
DeferredClose::WithoutSocket { .. } => {
// SAFETY: as in `run`.
let _enqueue_ref = unsafe { ScopedRef::adopt(task.ctx) };
// SAFETY: live per the ref above.
Expand Down
Loading
Loading