Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 21 additions & 6 deletions src/runtime/valkey_jsc/js_valkey.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,7 @@ impl JSValkeyClient {
reply_scanner: Default::default(),
retry_attempts: 0,
auto_flusher: Default::default(),
close_reason: None,
}),
global_object,
this_value: JsCell::new(JsRef::empty()),
Expand Down Expand Up @@ -930,6 +931,7 @@ impl JSValkeyClient {
reply_scanner: Default::default(),
retry_attempts: 0,
auto_flusher: Default::default(),
close_reason: None,
}),
global_object,
this_value: JsCell::new(JsRef::empty()),
Expand Down Expand Up @@ -1045,6 +1047,7 @@ impl JSValkeyClient {
// 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().close_reason = None;
let self_br = BackRef::new(self);
let _update = scopeguard::guard(self_br, |p| p.update_poll_ref());

Expand Down Expand Up @@ -1374,12 +1377,16 @@ impl JSValkeyClient {
};
this_jsvalue.ensure_still_alive();

// Create an error value
let error_value = protocol_jsc::valkey_error_to_js(
&global_object,
b"Connection closed",
protocol::RedisError::ConnectionClosed,
);
let error_value = match self.client_mut().close_reason.take() {
Some((message, err)) => {
protocol_jsc::valkey_error_to_js(&global_object, &*message, err)
}
None => protocol_jsc::valkey_error_to_js(
&global_object,
b"Connection closed",
protocol::RedisError::ConnectionClosed,
),
};
Comment thread
robobun marked this conversation as resolved.

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

Expand Down Expand Up @@ -1504,6 +1511,7 @@ impl JSValkeyClient {

fn connect(&self) -> Result<(), crate::Error> {
self.client_mut().flags.needs_to_open_socket = false;
self.client_mut().close_reason = None;

let _guard = self.ref_scope();

Expand Down Expand Up @@ -1978,6 +1986,13 @@ impl<const SSL: bool> SocketHandler<SSL> {
p.update_poll_ref();
});

if this.client.get().close_reason.is_none() {
this.client_mut().close_reason = Some((
Box::<[u8]>::from(&b"Failed to connect"[..]),
protocol::RedisError::ConnectionClosed,
));
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

narrow_terminated(this.client_mut().on_close())
}

Expand Down
10 changes: 10 additions & 0 deletions src/runtime/valkey_jsc/valkey.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,11 @@ pub struct ValkeyClient {

pub flags: ConnectionFlags,

/// First failure recorded via `fail()`. `on_valkey_close()` uses this so
/// the `connect()` promise rejects with the real cause (HELLO auth error
/// text, connection timeout, …) instead of a generic "Connection closed".
pub close_reason: Option<(Box<[u8]>, RedisError)>,

// Auto-pipelining
pub auto_flusher: AutoFlusher,

Expand Down Expand Up @@ -573,6 +578,10 @@ impl ValkeyClient {
return Ok(());
}

if self.close_reason.is_none() {
self.close_reason = Some((Box::<[u8]>::from(message), err));
}

if self.flags.finalized {
// We can't run promises inside finalizers.
if self.queue.readable_length() + self.in_flight.readable_length() > 0 {
Expand Down Expand Up @@ -1270,6 +1279,7 @@ impl ValkeyClient {
self.flags.failed = false;
self.flags.is_authenticated = false;
self.flags.is_selecting_db_internal = false;
self.close_reason = None;
if matches!(self.socket, AnySocket::SocketTcp(_)) {
// if is tcp, we need to start the connection process
// if is tls, we need to wait for the handshake to complete
Expand Down
95 changes: 95 additions & 0 deletions test/js/valkey/reliability/connection-failures.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,101 @@ describe.skipIf(!isEnabled)("Valkey: Connection Failures", () => {
});
});

describe("Valkey: connect() error identity", () => {
// Previously all connect-time failures surfaced as ERR_REDIS_CONNECTION_CLOSED / "Connection closed".
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const CRLF = "\r\n";

async function stubServer(helloReply: string | null) {
const server = net.createServer(sock => {
let received = "";
let replied = false;
sock.on("error", () => {});
sock.on("data", d => {
if (replied || helloReply === null) return;
received += d.toString().toUpperCase();
if (received.includes("HELLO") && received.endsWith(CRLF)) {
replied = true;
sock.write(helloReply);
}
});
});
await new Promise<void>(resolve => server.listen(0, "127.0.0.1", resolve));
const { port } = server.address() as net.AddressInfo;
return { port, close: () => server.close() };
}

async function connectError(url: string, options: any) {
const client = new RedisClient(url, options);
try {
await client.connect();
return { code: undefined, message: "<connected>" };
} catch (e: any) {
return { code: e?.code, message: e?.message };
} finally {
try {
client.close();
} catch {}
}
}

test("-WRONGPASS reply to HELLO rejects connect() with the server's error text", async () => {
const srv = await stubServer(`-WRONGPASS invalid username-password pair or user is disabled.${CRLF}`);
try {
expect(await connectError(`redis://:bad@127.0.0.1:${srv.port}`, { autoReconnect: false })).toEqual({
code: "ERR_REDIS_AUTHENTICATION_FAILED",
message: "WRONGPASS invalid username-password pair or user is disabled.",
});
} finally {
srv.close();
}
});

test("-NOAUTH reply to HELLO rejects connect() with the server's error text", async () => {
const srv = await stubServer(`-NOAUTH HELLO must be called with the client already authenticated${CRLF}`);
try {
expect(await connectError(`redis://127.0.0.1:${srv.port}`, { autoReconnect: false })).toEqual({
code: "ERR_REDIS_AUTHENTICATION_FAILED",
message: "NOAUTH HELLO must be called with the client already authenticated",
});
} finally {
srv.close();
}
});

test("connectionTimeout expiry rejects connect() with ERR_REDIS_CONNECTION_TIMEOUT", async () => {
const srv = await stubServer(null);
try {
expect(
await connectError(`redis://127.0.0.1:${srv.port}`, {
autoReconnect: false,
connectionTimeout: 200,
}),
).toEqual({
code: "ERR_REDIS_CONNECTION_TIMEOUT",
message: "Connection timeout reached after 200ms",
});
} finally {
srv.close();
}
});

test("TCP connect error rejects connect() with a distinguishable message", async () => {
const listener = net.createServer();
await new Promise<void>(resolve => listener.listen(0, "127.0.0.1", resolve));
const { port } = listener.address() as net.AddressInfo;
await new Promise<void>(resolve => listener.close(() => resolve()));

const err = await connectError(`redis://127.0.0.1:${port}`, {
autoReconnect: false,
connectionTimeout: 2000,
});
expect(err).toEqual({
code: "ERR_REDIS_CONNECTION_CLOSED",
message: "Failed to connect",
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
});

describe("Valkey: Auto-Reconnect In-Flight Commands", () => {
function readCommands(state: { buffer: Buffer }): string[][] {
const commands: string[][] = [];
Expand Down
Loading