Skip to content

redis: re-issue SUBSCRIBE after an auto-reconnect - #33450

Open
robobun wants to merge 11 commits into
mainfrom
farm/dc8fa6cc/valkey-resubscribe-on-reconnect
Open

redis: re-issue SUBSCRIBE after an auto-reconnect#33450
robobun wants to merge 11 commits into
mainfrom
farm/dc8fa6cc/valkey-resubscribe-on-reconnect

Conversation

@robobun

@robobun robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

What

Bun.RedisClient with autoReconnect silently goes deaf after the first dropped connection. The client reconnects and sends HELLO 3, but it never re-issues SUBSCRIBE for the channels its listeners are registered on. The app sees connected === true, the callbacks stay registered, and no message ever arrives again. onclose is not fired on a reconnecting drop either, so the app is never told it needs to resubscribe.

node-redis, ioredis and go-redis all re-subscribe on reconnect.

Repro

Against a scripted RESP3 server that logs every command per connection:

const sub = new Bun.RedisClient(url, { autoReconnect: true, maxRetries: 10 });
await sub.connect();
await sub.subscribe("news", m => got.push(m));

const pub = new Bun.RedisClient(url, { autoReconnect: false });
await pub.connect();
console.log("receivers before the drop:", await pub.publish("news", "m1"));

serverSideSocketForSub.end();             // drop the subscriber's connection
await reconnected;

console.log("commands on the new connection:", commandsSeenOnReconnect);
console.log("receivers after:", await pub.publish("news", "m2"), "| connected =", sub.connected);
receivers before the drop: 1
commands on the new connection: [["HELLO 3"]]     <- no SUBSCRIBE
receivers after: 0 | connected = true             (the listener only ever saw "m1")

Cause

on_valkey_connect runs when the HELLO handshake completes. It caches the hello value, fires onconnect, resolves the connection promise, and then drains the offline queue. Nothing looks at the subscription callback map, even though the map is still populated and is_subscriber stays true across a reconnect. The server, meanwhile, has a brand-new connection with no subscriptions on it.

Fix

Replay SUBSCRIBE for every channel in the callback map as soon as the handshake completes, before anything queued goes out.

Two details make it safe:

  • The command goes straight into the write buffer (like HELLO/SELECT do), so it is the first thing on the wire on the new connection, ahead of whatever the offline queue still holds.
  • It carries no promise. A promise here has nobody awaiting it, so a rejection (error reply, or the connection dropping again mid-replay) would surface as an unhandled rejection. resubscribe_pending counts off the confirmations it produces instead, so they do not consume a queued command's promise pair.

That counter is load-bearing today: removing it makes SUBSCRIBE news sports consume the queued PING's reply slot, and await client.ping() resolves with 2 instead of "PONG". It also covers the error case, where the server refuses the replay (an ACL change during the outage) and replies with a single -ERR and no confirmations at all: that error must not eat a reply slot either, or the command behind it is dropped unsettled and its await hangs. (If #32858 lands, its per-pair remaining_replies accounting subsumes the counter and it can be dropped.)

Pattern subscriptions are untouched: psubscribe is a raw pass-through that registers no listener, so the client has no pattern set to replay.

Also adds JSC__JSMap__keys (a JSMapIterator over IterationKind::Keys into a JSArray) since the callback map had no key-iteration binding.

Verification

test/js/valkey/reliability/subscriber-reconnect.test.ts drives a scripted RESP3 server, so it runs without Docker. It asserts the exact command list the reconnected connection receives, that the original listener keeps getting messages, and the two negative contracts (unsubscribed channels are not replayed; a client that never subscribed sends no SUBSCRIBE).

$ bun bd test test/js/valkey/reliability/subscriber-reconnect.test.ts
(pass) replays SUBSCRIBE for every channel with a listener
(pass) does not replay channels the client unsubscribed from
(pass) a client that never subscribed sends no SUBSCRIBE on reconnect
(pass) does not replay a SUBSCRIBE the offline queue is already holding
(pass) an error reply to the replay settles the in-flight command instead of eating its reply slot
(pass) replays SUBSCRIBE after SELECT when the URL names a database

4 of the 6 fail on the unfixed build. The other two pin negative contracts: a client that never subscribed sends no SUBSCRIBE, and a lazy-connect subscribe() (no explicit connect()) is not replayed on top of the copy the offline queue already holds.

Without the fix (USE_SYSTEM_BUN=1)
error: expect(received).toEqual(expected)

  [
    "HELLO 3",
-   "SUBSCRIBE news sports",
    "PING",
  ]

The existing docker-free valkey tests (resp-nesting-depth, valkey-gc, valkey-incremental-scan, valkey-tls-verify) still pass: 21 pass, 0 fail.

Deliberately out of scope

Two adjacent bugs in the same subsystem are left alone, since fixing them here would mean rewriting code that two other open PRs already rewrite:

If #32858 lands first, its per-pair remaining_replies accounting subsumes resubscribe_pending and the counter can be deleted.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6081058c-5904-4ce5-985e-2926c2c7408c

📥 Commits

Reviewing files that changed from the base of the PR and between 4961856 and 11855d4.

📒 Files selected for processing (3)
  • docs/runtime/redis.mdx
  • src/runtime/valkey_jsc/js_valkey.rs
  • test/js/valkey/reliability/subscriber-reconnect.test.ts

Walkthrough

Adds a JSMap keys() binding (Rust and C++), uses it to expose subscribed channel names from SubscriptionCtx, and implements automatic resubscription of channels after Valkey reconnects, tracking pending confirmations via a new resubscribe_pending field. Adds tests and documentation.

Changes

Resubscribe on reconnect

Layer / File(s) Summary
JSMap keys binding
src/jsc/bindings/bindings.cpp, src/jsc/JSMap.rs
Adds JSC__JSMap__keys C++ export and a corresponding Rust JSMap::keys method returning map keys as a JS array in insertion order.
Resubscribe logic on reconnect
src/runtime/valkey_jsc/js_valkey.rs
Adds SubscriptionCtx::subscribed_channels, initializes resubscribe_pending: 0 on client creation/cloning, and introduces resubscribe() invoked from on_valkey_connect to replay SUBSCRIBE for tracked channels.
Response handling for replayed SUBSCRIBE
src/runtime/valkey_jsc/valkey.rs
Adds resubscribe_pending field, updates handle_response to skip promise-pair consumption for replay confirmations/errors, and resets the counter on on_open.
Reconnect subscriber tests and documentation
test/js/valkey/reliability/subscriber-reconnect.test.ts, docs/runtime/redis.mdx
Adds a scripted RESP3 mock server and five reconnect tests validating SUBSCRIBE replay ordering, unsubscribe exclusion, error handling, and SELECT/SUBSCRIBE ordering; documents the reconnect behavior.

Sequence Diagram(s)

sequenceDiagram
  participant ValkeyClient
  participant JSValkeyClient
  participant SubscriptionCtx
  participant Server

  ValkeyClient->>JSValkeyClient: on_open (reset resubscribe_pending=0)
  JSValkeyClient->>JSValkeyClient: on_valkey_connect
  JSValkeyClient->>JSValkeyClient: resubscribe()
  JSValkeyClient->>SubscriptionCtx: subscribed_channels(global_object)
  SubscriptionCtx-->>JSValkeyClient: channel keys array
  JSValkeyClient->>Server: SUBSCRIBE channels
  JSValkeyClient->>ValkeyClient: set resubscribe_pending = channel count
  Server-->>ValkeyClient: PUSH Subscribe confirmation
  ValkeyClient->>ValkeyClient: decrement resubscribe_pending
Loading

Related PRs: None identified.

Suggested labels: valkey, redis, jsc, tests, documentation

Suggested reviewers: None identified.

Poem

A rabbit hops through wires anew,
When sockets drop, it knows just what to do—
It gathers channels, keys in a row,
Whispers SUBSCRIBE before you know,
And listeners hum again, as if they knew. 🐇📡

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: re-issuing SUBSCRIBE after auto-reconnect.
Description check ✅ Passed The description covers what changed and how it was verified, though it uses custom headings instead of the template's exact section titles.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 6, 2026
@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:05 AM PT - Jul 7th, 2026

@robobun, your commit 11855d4 has some failures in Build #69625 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33450

That installs a local version of the PR into your bun-33450 executable, so you can run:

bun-33450 --bun

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. node-redis Pub/Sub silently fails to reconnect in rare cases #21622 - node-redis Pub/Sub silently enters a non-functional state after network failure (no messages, no errors, unsubscribe hangs) — this PR's reconnect resubscription and response accounting directly address these symptoms

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #21622

🤖 Generated with Claude Code

@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Not adding Fixes #21622 here: the symptoms line up, but the code paths don't.

#21622 is the npm redis package (node-redis), which Bun does not shim. It runs as-is on node:net/node:tls and does its own resubscription in JS, so none of this diff is reachable from it. That bug lives somewhere in the socket layer or node-redis's own reconnect bookkeeping on Bun.

This PR only changes Bun.RedisClient, the native client in src/runtime/valkey_jsc/.

@claude claude Bot left a comment

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.

I didn't find any bugs, but this touches the Redis reconnect state machine, adds promise-pair accounting via a new resubscribe_pending counter, and introduces a new C++ JSC binding — worth a human look at the lifetime handling in resubscribe() and the interaction with the in-flight queue.

Extended reasoning...

Overview

This PR fixes Bun.RedisClient silently going deaf after an auto-reconnect by replaying SUBSCRIBE for every channel in the callback map once the new connection's HELLO handshake completes. It touches:

  • src/runtime/valkey_jsc/js_valkey.rs — new resubscribe() that iterates the JS-side callback map, serializes a promise-less SUBSCRIBE straight into the write buffer, and sets resubscribe_pending; plus a subscribed_channels() helper on SubscriptionCtx.
  • src/runtime/valkey_jsc/valkey.rs — new resubscribe_pending: u32 field on ValkeyClient, reset in on_open, and a new match arm in handle_response that decrements it and skips promise-pair consumption for the corresponding subscribe-push replies.
  • src/jsc/bindings/bindings.cpp + src/jsc/JSMap.rs — new JSC__JSMap__keys binding that materializes a JSMap's keys into a JSArray via JSMapIterator.
  • docs/runtime/redis.mdx — one bullet documenting the new behavior.
  • test/js/valkey/reliability/subscriber-reconnect.test.ts — 257-line scripted RESP3 server harness with three tests asserting the exact wire-level command sequence on reconnect and the negative contracts.

Security risks

None apparent. The change reads from an internally-managed JSMap (populated only via .subscribe()) and writes RESP-encoded channel names to the socket. No new user-controlled input parsing, no auth/crypto/permission surface.

Level of scrutiny

Moderate-to-high. This is not a mechanical change:

  • The resubscribe_pending counter interacts with the existing promise-pair consumption logic in handle_response. The PR description itself calls it "load-bearing" — removing it makes a queued PING resolve with the subscribe-confirmation count instead of "PONG". It also notes this accounting may be subsumed by pending PR #32858, so a maintainer should confirm the two don't conflict.
  • resubscribe() iterates JS values (to_bun_stringto_utf8()channel_str.deref()) and holds the resulting ZigStringSlices in a Vec across the loop. Whether the slice borrows from the deref'd BunString or owns its allocation is a lifetime detail worth a maintainer's eye.
  • A new C++ JSC binding (JSC__JSMap__keys) with a ThrowScope, constructEmptyArray, iterator loop, and putDirectIndex — standard shape, but new JSC glue always deserves review for exception-check correctness.
  • The write goes into the buffer during on_valkey_connect (post-HELLO), which for database > 0 clients is before the SELECT reply is processed; the ordering looks correct but is subtle.

Other factors

The tests are well-constructed (scripted in-process RESP3 server, no Docker dependency, assert exact command lists, cover both positive and negative contracts, verified to fail on USE_SYSTEM_BUN=1). No prior human reviews on the PR. The bug-hunting system found nothing. Given the state-machine subtlety and new JSC binding, deferring to a human reviewer.

A reconnect opens a brand-new server-side connection, which carries none
of the previous connection's subscriptions. The client only sent HELLO,
so a subscriber reported connected === true with its listeners still
registered while no message ever arrived again.

Replay SUBSCRIBE for every channel in the subscription callback map as
soon as the HELLO handshake completes. The command is written directly to
the write buffer, ahead of whatever the offline queue still holds, and
carries no promise; resubscribe_pending counts off the confirmations it
produces so they do not consume a queued command's promise pair.
@robobun
robobun force-pushed the farm/dc8fa6cc/valkey-resubscribe-on-reconnect branch from 3f4a78a to a547a48 Compare July 6, 2026 11:31
With a database in the URL, the replayed SUBSCRIBE is written while SELECT's
reply is still outstanding. Assert it lands behind SELECT on the wire, or it
would subscribe on database 0.
@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Answering the two things flagged as worth a closer look, and rebased onto main (the earlier red CI was a stale base: #33425 fixed the cookie-map Expires assertions after I branched, so every test-bun lane failed on a test file I don't touch).

Slice lifetimes in resubscribe()

let channel_str = channel.to_bun_string(&global_object)?;
args.push(channel_str.to_utf8());
channel_str.deref();

to_utf8() returns a ZigStringSlice that either takes its own ref on the WTFStringImpl or allocates a copy; impl Drop for ZigStringSlice releases that ref. So dropping our bun_core::String right after is correct, and the Vec<ZigStringSlice> keeps the bytes alive for the whole Command::write. Same shape as hset_impl in js_valkey_functions.rs, which has the comment spelling it out.

Ordering against SELECT when database > 0

authenticate() writes HELLO then SELECT and sets is_selecting_db_internal. The replay is written from on_valkey_connect, which runs on the HELLO reply, so SELECT's +OK is still outstanding. It lands third on the wire, after SELECT, and the server applies it to the right database. send_next_command() only flushes at that point (connection_ready() is false while is_selecting_db_internal is set), so nothing from the offline queue jumps the gap.

That was previously untested, so 953d661 pins it:

expect(commandLines(server.connections[2]))
  .toEqual(["HELLO 3", "SELECT 3", "SUBSCRIBE news", "PING"]);

Fails on the unfixed build as ["HELLO 3", "SELECT 3", "PING"]. 3 of the 4 tests in the file now fail without the fix; all 4 pass with it.

Comment thread src/runtime/valkey_jsc/valkey.rs
… slot

The replayed SUBSCRIBE carries no promise pair. If the server refuses it
(an ACL change during the outage, say) the reply is an Error rather than a
confirmation, so the push-kind accounting never ran and the error consumed
the pair of whatever user command was parked behind it in the in-flight
queue. The subscriber error path then dropped that pair without settling
it, hanging the caller's await.

Skip pair consumption for an error reply while confirmations are still
owed, and stop expecting them: the server rejects SUBSCRIBE as a whole.
Comment thread src/runtime/valkey_jsc/js_valkey.rs

@coderabbitai coderabbitai Bot left a comment

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.

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 `@docs/runtime/redis.mdx`:
- Around line 420-421: Clarify the reconnect behavior in the Redis docs by
noting that the replay logic only applies to channels registered via
`.subscribe()` and does not restore pattern subscriptions from `psubscribe()`.
Update the surrounding reconnect/resubscribe guidance in the Redis runtime docs
to explicitly call out this exception so users can distinguish how
`.subscribe()` and `psubscribe()` behave after a reconnect.

In `@src/runtime/valkey_jsc/js_valkey.rs`:
- Around line 1276-1277: The connect flow in `JsValkey::connect` / the
default-database path is leaving `connection_promise` unresolved when
`self.resubscribe()?` fails before the promise-handling block runs. Move the
resubscribe step to after the cached promise is resolved/rejected, or explicitly
reject and clear `connection_promise` on this failure path so `client.fail(...)`
cannot strand `.connect()`.

In `@test/js/valkey/reliability/subscriber-reconnect.test.ts`:
- Around line 190-318: These reconnect tests are independent and should be
parallelized to reduce runtime. Update the `describe("Valkey: subscriber
reconnect", ...)` block or each `test(...)` case to use concurrent execution via
`test.concurrent` or `describe.concurrent`, since each case creates its own
`startRespServer()` and isolated `RedisClient` instances. Make sure any shared
setup/teardown remains per-test so the `subscriber reconnect` scenarios stay
isolated under concurrency.
- Around line 222-225: The cleanup in these subscriber reconnect tests closes
`subscriber` and `publisher` while the client may still be subscribed, which can
leave lingering subscription state. In each affected `finally` block in
`subscriber-reconnect.test.ts`, explicitly unsubscribe the `subscriber` from its
channel(s) before calling `close()`, then close both clients as usual. Apply
this to the test cases around the reconnect scenarios that use
`subscriber`/`publisher` and the `"news"` subscription.
🪄 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: c26dd519-e201-47b0-8cde-f1de104b8518

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 4961856.

📒 Files selected for processing (6)
  • docs/runtime/redis.mdx
  • src/jsc/JSMap.rs
  • src/jsc/bindings/bindings.cpp
  • src/runtime/valkey_jsc/js_valkey.rs
  • src/runtime/valkey_jsc/valkey.rs
  • test/js/valkey/reliability/subscriber-reconnect.test.ts

Comment thread docs/runtime/redis.mdx Outdated
Comment thread src/runtime/valkey_jsc/js_valkey.rs Outdated
Comment thread test/js/valkey/reliability/subscriber-reconnect.test.ts
Comment thread test/js/valkey/reliability/subscriber-reconnect.test.ts
robobun added 2 commits July 6, 2026 12:59
Running resubscribe() ahead of the promise meant an exception from it
(building the channel list allocates) returned early and left .connect()
pending forever. It still lands ahead of the offline queue, which only
drains once the microtask scope and on_writable run.

Also note in the docs that psubscribe patterns are not replayed, run the
reconnect tests concurrently, and unsubscribe before close() so teardown
does not depend on GC to release the event loop.
subscribe() without an explicit connect() populates the handler map and
queues the SUBSCRIBE, so the map is non-empty before the handshake finishes.
Replaying it there would put SUBSCRIBE on the wire twice. Nothing caught
that, so assert the lazy-connect path sends it exactly once.

@claude claude Bot left a comment

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.

Both of my earlier comments are addressed (the error-reply path is now handled, and the is_subscriber guard is pinned by a test), and this pass found nothing new — but the reply-slot accounting and new JSC binding are worth a human look before merge.

Extended reasoning...

Overview

This PR makes Bun.RedisClient re-issue SUBSCRIBE for every channel in the callback map after an auto-reconnect, so listeners keep receiving messages instead of going silently deaf. It touches:

  • src/jsc/bindings/bindings.cpp + src/jsc/JSMap.rs — new JSC__JSMap__keys binding (JSMapIterator → JSArray) so Rust can enumerate the callback map's channels.
  • src/runtime/valkey_jsc/js_valkey.rs — new resubscribe() called from on_valkey_connect(), plus SubscriptionCtx::subscribed_channels() and resubscribe_pending init sites.
  • src/runtime/valkey_jsc/valkey.rs — new resubscribe_pending: u32 field; handle_response() now skips promise-pair consumption for the replay's Push(Subscribe) confirmations and for a RESPValue::Error reply while the counter is nonzero; on_open() resets it.
  • test/js/valkey/reliability/subscriber-reconnect.test.ts — 6 tests against a scripted RESP3 server (no Docker), asserting the exact command sequence on the reconnected socket, the SELECT/SUBSCRIBE ordering, the -NOPERM error path, and two negative contracts (never-subscribed / lazy-connect dedup).
  • docs/runtime/redis.mdx — one bullet noting the resubscribe behavior and that psubscribe patterns are not replayed.

Prior review status

I left two inline comments on earlier revisions; both threads are now resolved:

  • valkey.rs:1129 — the replay's error reply eating an unrelated command's promise pair. Fixed in 4961856 with the resubscribe_pending > 0 && matches!(value, Error(_)) guard and a dedicated test that hangs without it.
  • js_valkey.rs:1331 — the !is_subscriber() early-return skipping the drop-before-first-confirmation window. Author kept the guard with a well-reasoned rebuttal (that window's await subscribe() is rejected, not silent; dropping the clause double-sends on lazy connect) and added a test in 45ff3b0 that fails if the clause is removed.

CodeRabbit's four comments (docs note on psubscribe, resubscribe ordering vs. connect promise, describe.concurrent, unsubscribe-before-close in test cleanup) are all marked addressed in d044c4e. This bug-hunt pass found nothing.

Security risks

None identified. No auth, crypto, permission, or path-handling changes. The new C++ binding iterates a JSMap the client itself created; user code cannot reach it.

Level of scrutiny

Medium-high, human required. This is not a mechanical change: it modifies the RESP reply-slot accounting in handle_response(), which is the exact spot where an off-by-one silently misroutes a promise to the wrong command. The resubscribe_pending counter's correctness depends on wire ordering (replay is written before the offline queue drains, after HELLO/SELECT) and on Redis's whole-command error semantics for SUBSCRIBE. It also adds a new C++ → Rust JSC export. All of that is well-argued and well-tested here, but it is squarely in the "a maintainer who knows this state machine should sign off" category, not auto-approvable.

Other factors

  • Test coverage is strong: 6 concurrent tests, 4 of which fail on the unfixed build per the PR body, exact-command-list assertions, and a test that specifically pins the load-bearing is_subscriber guard.
  • The PR body explicitly calls out that #32858 will subsume the resubscribe_pending counter — a maintainer may want to weigh sequencing.
  • The pre-existing subscriber Error-arm hang (drops pair_maybe unsettled) is acknowledged and deliberately left for #32858 / #33290; that scoping decision seems reasonable but is a maintainer call.

Six of them at once drove six reconnects through six clients, and the
Windows agents blew the client's 10s connection timeout on three of them:

  ✗ an error reply to the replay ... [10081ms]
    Received message: "Connection timeout reached after 10000ms"
  ✗ replays SUBSCRIBE after SELECT ... [10080ms]
    RedisError: Connection has failed

The same file passed on windows x64, x64-baseline and win11-aarch64 before
it was made concurrent. Serial, it runs in 3s.

@claude claude Bot left a comment

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.

Both of my earlier inline concerns were addressed (the error-reply guard in 4961856 and the is_subscriber rationale + pinning test in 45ff3b0), and I found nothing new this pass. Still deferring: this touches the reply-slot accounting in handle_response, adds a new JSC binding, and explicitly interacts with #32858/#33290 — worth a maintainer's eyes on the merge ordering.

Extended reasoning...

Overview

This PR makes Bun.RedisClient re-issue SUBSCRIBE for every channel in the callback map after an auto-reconnect, so subscribers don't go silently deaf. It touches:

  • src/jsc/bindings/bindings.cpp + src/jsc/JSMap.rs: new JSC__JSMap__keys binding (JSMapIterator over Keys → JSArray)
  • src/runtime/valkey_jsc/js_valkey.rs: SubscriptionCtx::subscribed_channels(), new resubscribe() called from on_valkey_connect, field init in two constructors
  • src/runtime/valkey_jsc/valkey.rs: new resubscribe_pending: u32 field, reply-slot accounting in handle_response (skip promise-pair consumption for the replay's confirmations and its error reply), reset in on_open
  • test/js/valkey/reliability/subscriber-reconnect.test.ts: 6 tests against a scripted RESP3 server (no Docker)
  • docs/runtime/redis.mdx: 3-line doc note

Security risks

None identified. No auth/crypto/permission changes; the new C++ binding iterates an internal JSMap the client itself created (keys are channel-name strings), with proper RETURN_IF_EXCEPTION after each fallible JSC call.

Level of scrutiny

Medium-high. The mechanism is straightforward but the correctness rests on subtle invariants: the replayed SUBSCRIBE is written directly into the write buffer with no promise pair, and resubscribe_pending must exactly account for its replies so they don't consume another command's slot. My earlier review found one gap there (error reply eating a slot → hung await), which was fixed with a guard and a regression test. The ordering against SELECT and the offline queue is now pinned by tests. This is protocol-state-machine code where an off-by-one in reply accounting hangs a user's await forever.

Other factors

  • All prior review threads (mine and CodeRabbit's) are resolved with code changes and pinning tests.
  • The PR description explicitly calls out two adjacent bugs left out of scope because #32858 and #33290 rewrite the same code — a maintainer should sanity-check merge ordering and whether resubscribe_pending should be dropped once #32858 lands.
  • New C++ JSC binding: small and follows the neighboring pattern, but bindings changes conventionally get a human glance.
  • Tests are thorough (6 cases, 4 fail on unfixed build, negative contracts covered), hermetic (in-process RESP3 server), and the file documents why it's serial.

Not approving because this is a non-mechanical change to reply-slot accounting in a stateful protocol client, with cross-PR coupling the author flagged themselves.

Redis always confirms UNSUBSCRIBE, sending a single nil-channel reply when
the connection is subscribed to nothing. The scripted server stayed silent
in that case, so closeSubscriber()'s `await client.unsubscribe()` hung in
the teardown of every test whose reconnect carried no subscriptions.

That only bit on a build without the fix, where the replay never happens:
the four failing tests timed out after 5s apiece instead of reporting the
assertion that actually failed. Now they fail in 0.5s with the real diff.

@claude claude Bot left a comment

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.

My earlier concerns are addressed and I found nothing new, but this changes the reply-slot accounting in the RESP handler and adds a new JSC binding — worth a human pass on the protocol state-machine bits.

Extended reasoning...

Overview

Adds subscription replay after auto-reconnect for Bun.RedisClient. Touches six files: a new JSC__JSMap__keys C++ binding and its Rust wrapper, a resubscribe() method on JSValkeyClient that writes a promise-less SUBSCRIBE directly into the write buffer after the HELLO handshake, a resubscribe_pending counter on ValkeyClient that gates whether an incoming subscribe push (or its error reply) consumes an in-flight promise pair, a docs note, and a 360-line scripted-RESP3-server test file with six cases.

Security risks

None identified. The new binding iterates a JS Map's keys via JSMapIterator with proper RETURN_IF_EXCEPTION after every fallible call. No untrusted-input parsing, no auth/crypto surface.

Level of scrutiny

Medium-high. The core change is small but sits in the RESP reply-dispatch state machine: resubscribe_pending is a new piece of implicit per-connection state that decides whether a reply consumes a queued promise pair. Getting this wrong misattributes replies (my earlier comment caught one such case on the error path, now fixed and pinned by a test). The ordering invariant — replay is written after HELLO/SELECT but before the offline queue drains, and its confirmations are therefore the first replies on the new socket — is load-bearing and only enforced by call-site placement in on_valkey_connect. The PR description explicitly notes overlap with two other open PRs (#32858, #33290) that rewrite adjacent code, so a maintainer should confirm the sequencing.

Other factors

Both of my prior inline comments were addressed: the error-reply-eats-a-slot case was fixed in 4961856 with a regression test, and the is_subscriber() guard was kept with a clear justification plus a new test pinning why removing it would double-send on lazy connect. All CodeRabbit findings are resolved (including reverting describe.concurrent after it flaked on Windows CI). Test coverage is thorough — six cases against a scripted server, four of which fail on the unfixed build. The new C++ binding follows house style. I'm deferring rather than approving because reply-slot accounting in a pipelined protocol handler is exactly the kind of subtle state a maintainer familiar with the valkey subsystem should sign off on, and because of the stated interaction with #32858/#33290.

robobun added 3 commits July 6, 2026 17:42
socket.write is unbuffered: it reports how many bytes it took, 0 when the
send buffer is full and -1 once the socket is closing. The server ignored
that and dropped the remainder, so a reply could vanish.

A freshly accepted socket is not always writable yet on Windows, which lost
the HELLO reply outright. The client then sat in Connecting until its 10s
timeout, and the NOPERM case failed on windows 11 aarch64 every attempt:

  ✗ an error reply to the replay ... [10073.15ms]
    Expected pattern: /NOPERM/
    Received message: "Connection timeout reached after 10000ms"

Queue what the socket will not take and flush it again on drain.
windows 11 aarch64 fails this case on every attempt with

  Expected pattern: /NOPERM/
  Received message: "Connection timeout reached after 10000ms"

which can only happen if the reconnect never finished its handshake, since
the -NOPERM path sets flags.failed and that suppresses the timeout. The
other five reconnect cases pass on the same agent, so the error text alone
does not say which half broke.

Assert the ping outcome, the connected flag and the server's per-connection
command log together, so a failure shows whether HELLO, SUBSCRIBE and PING
reached the server at all. Also catches the no-replay case immediately
instead of waiting for PING to hang.
…t drop

The socket-drop tests relied on the auto-reconnect backoff timer firing and
the handshake finishing inside the client's 10s connection timeout. On the
loaded Windows CI agents that raced, and the subscriber tests flaked (win11
aarch64 most often).

close() + await connect() exercises the identical replay path
(on_valkey_connect -> resubscribe) and the connect() promise resolves only
once HELLO completes, so there is no timer to race. The per-connection wire
assertions are unchanged, 4 of 6 still fail on a build without the fix, and
the no-longer-needed waitForConnections barrier is removed.
@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for review. The diff is green on every test lane, including all Windows and Linux.

The one remaining red check is :darwin: 26 aarch64 - test-bun, and it is build infrastructure, not a test failure:

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

No test runs on that job when it fails (it dies fetching the build artifact). It is flaky across this branch's builds, independent of the diff: it passed on builds 68842 and 69165 and timed out on 68998, 69227, 69335, and 69625 (after one empty retrigger), with no change to the code in between.

The subscriber-reconnect tests themselves pass everywhere now, including win11-aarch64 (8/8 shards green on the latest builds). They earlier flaked on loaded Windows agents; that was fixed by triggering the reconnect with close() + await connect() (a deterministic handshake barrier) instead of a server-side socket drop racing the client's 10s connection timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant