redis: register the subscribe() listener only after the server confirms the subscription by robobun · Pull Request #33290 · oven-sh/bun

robobun · 2026-07-03T03:06:00Z

Repro

subscribe() registers its listener before the SUBSCRIBE is sent. If the command then
fails, the listener stays registered. Retry the subscribe and every message is delivered
twice; the now-permanently-populated handler map also keeps the event loop pinned.

// scripted RESP3 server: HELLO -> ok, SUBSCRIBE -> confirm + publish one message
const redis = new Bun.RedisClient(url, { enableOfflineQueue: false, autoReconnect: false });
const calls: string[] = [];
const listener = (m: string) => calls.push(m);

// offline queue is disabled, so this is rejected before it ever reaches the server
try { await redis.subscribe("ch", listener); } catch (e) { console.log("rejected:", e.message); }

await redis.connect();
console.log("second subscribe ->", await redis.subscribe("ch", listener));
await Bun.sleep(150);
console.log("deliveries:", calls.length, calls);

rejected: Connection is closed and offline queue is disabled
second subscribe -> 1
deliveries: 2 [ "hi", "hi" ]     # one published message
# ...and the process never exits, even after redis.close()

Cause

JSValkeyClient::subscribe called upsert_receive_handler for each channel up front, as
the source itself noted:

// This is less-than-ideal, still, because this assumes a happy path. What happens if
// the SUBSCRIBE command fails? We have no way to roll back the addition of the
// handler.

Nothing does roll it back. The handler map is what has_subscriptions() reads, so
update_poll_ref keeps the event-loop ref (and the strong this_value) forever, and a
later subscribe() for the same channel appends the same listener to the channel's
callback array a second time.

Rolling back at the failure site is not enough either: the map stores one array of
listeners per channel, so removing "the listener this command added" cannot be told apart
from an identical listener that an earlier, confirmed subscribe added.

Fix

Carry the channels and the listener on the command's promise pair (PendingSubscription,
two Strong refs, boxed so it costs one pointer on every other command) and register them
from the subscribe confirmation push instead. A SUBSCRIBE that is rejected, dropped by a
closing connection, or never answered simply drops the pair and leaves the map untouched.

The channel list is snapshotted into a fresh array at call time, since the caller's array is
no longer read synchronously.

This also removes the clear_all_receive_handlers() call on the synchronous send-failure
path, which used to wipe out every other channel's listeners too.

Verification

$ bun bd test test/js/valkey/valkey-subscribe-listener.test.ts
 4 pass  0 fail

Without the src/ change, three of the four fail: two with the repro's duplicate delivery
(["hi", "hi"] for one published message, once for a subscribe rejected before it is sent
and once for a subscribe still in flight when the connection closes), and one with a pinned
event loop. The fourth is a regression guard for the channel snapshot.

Unchanged behaviour against a real redis

Single- and multi-channel subscribe, message delivery, multiple listeners per channel,
unsubscribe(channel, listener), unsubscribe(), subscribing before the connection is up,
and back-to-back un-awaited subscribes all behave identically to main, with one
correction: two un-awaited subscribe() calls now resolve with 1 then 2 (the real
channel counts) rather than 2 then 2, because the count is no longer inflated by
listeners whose SUBSCRIBE has not been confirmed yet.

Notes

Found while investigating a report that await redis.subscribe(ch, cb) hangs forever when
the server answers the SUBSCRIBE with -ERR LOADING. That hang is a separate bug and is
already fixed by #32858, which I verified by building its branch and running the repro
against it. This PR fixes what #32858 leaves behind, and is independent of it (they touch
adjacent lines in handle_subscribe_response, so whichever lands second needs a trivial
rebase).

[review] gate passed · iteration 1 · 6 files touched

fails on main (without fix)

ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey-subscribe-listener.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (e2445ab32)

test/js/valkey/valkey-subscribe-listener.test.ts:
 97 |     // The rejected subscribe must not have left its listener behind, or this one registers
 98 |     // it a second time and the single published message arrives twice.
 99 |     await redis.connect();
100 |     expect(await redis.subscribe("ch", listener)).toBe(1);
101 |     await delivered;
102 |     expect(received).toEqual(["hi"]);
                           ^
error: expect(received).toEqual(expected)

  [
    "hi",
+   "hi",
  ]

- Expected  - 0
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/valkey/valkey-subscribe-listener.test.ts:102:22)
(fail) a subscribe() rejected before it is sent does not register its listener [153.72ms]
(p
... (truncated)

release without fix: 3 FAILED
bun test v1.4.0-canary.1 (1498d7b77)

test/js/valkey/valkey-subscribe-listener.test.ts:
 97 |     // The rejected subscribe must not have left its listener behind, or this one registers
 98 |     // it a second time and the single published message arrives twice.
 99 |     await redis.connect();
100 |     expect(await redis.subscribe("ch", listener)).toBe(1);
101 |     await delivered;
102 |     expect(received).toEqual(["hi"]);
                           ^
error: expect(received).toEqual(expected)

  [
    "hi",
+   "hi",
  ]

- Expected  - 0
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/valkey/valkey-subscribe-listener.test.ts:102:22)
(fail) a subscribe() rejected before it is sent does not register its listener [6.48ms]
(pass) mutating the channel array after subscribe() does not change what gets registered [2.40ms]
141 |     // Same as above: the SUBSCRIBE never got its confirmation, so it must not have left a
142 |     // listener behind for the next one to duplicate.
143 |     await redis.connect();
144 |     expect(await redis.subscribe("ch", listener)).toBe(1);
145 |     await delivered;
146 |     expect(received).toEqual(["hi"]);
           
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/valkey/valkey-subscribe-listener.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (e2445ab32)

test/js/valkey/valkey-subscribe-listener.test.ts:
(pass) a subscribe() rejected before it is sent does not register its listener [143.85ms]
(pass) mutating the channel array after subscribe() does not change what gets registered [68.71ms]
(pass) a subscribe() abandoned by a closing connection does not register its listener [124.01ms]
(pass) a subscribe() rejected before it is sent does not keep the event loop alive [485.67ms]

 4 pass
 0 fail
 9 expect() calls
Ran 4 tests across 1 file. [2.58s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     e2445ab32a
  features     (none)

22 deps, 106 codegen, 1168 objects in 881ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1231] gen bindgenv2
[2/1231] install /workspace/bun
bun install v1.4.0-canary.1 (1498d7b77)

Checked 124 installs across 170 packages (no changes) [15.00ms]
[3/1231] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (1498d7b77)

Checked 1 install across 2 packages (no changes) [1.00ms]
[4/1231] fetch tinycc
[tinycc] up to date
[5/1230] fetch picohttpparser
[picohttpparser] up to date
[6/1230] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[7/1230] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (1498d7b77)

Checked 129 installs across 147 packages (no chan
... (truncated)

diff hotspot

src/runtime/valkey_jsc/ValkeyCommand.rs          |  18 +-
 src/runtime/valkey_jsc/js_valkey.rs              |  26 ++-
 src/runtime/valkey_jsc/js_valkey_functions.rs    |  53 +++---
 src/runtime/valkey_jsc/mod.rs                    |   4 +-
 src/runtime/valkey_jsc/valkey.rs                 |  30 +++-
 test/js/valkey/valkey-subscribe-listener.test.ts | 207 +++++++++++++++++++++++
 6 files changed, 298 insertions(+), 40 deletions(-)

gate history · 1 passed · 0 rejected · iteration 1

evidence per changed file

file                                              reads  edits  tests
src/runtime/valkey_jsc/ValkeyCommand.rs               3      8      0
src/runtime/valkey_jsc/js_valkey.rs                  10      4      0
src/runtime/valkey_jsc/js_valkey_functions.rs         8      8      0
src/runtime/valkey_jsc/mod.rs                         2      4      0
src/runtime/valkey_jsc/valkey.rs                     11     17      0
test/js/valkey/valkey-subscribe-listener.test.ts      1      2      0

robobun · 2026-07-03T03:06:10Z

^{Updated 1:02 AM PT - Jul 15th, 2026}

❌ @robobun, your commit e2445ab has 2 failures in Build #73200 (All Failures):

test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
test/cli/run/no-orphans.test.ts - code 1 on 🍎 26 aarch64

🧪 To try this PR locally:

bunx bun-pr 33290

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

bun-33290 --bun

github-actions · 2026-07-03T03:09:08Z

Found 1 issue this PR may fix:

RedisClient.close() while subscribed keeps the process alive #33103 - RedisClient.close() while subscribed keeps the process alive — this PR fixes the root cause by deferring listener registration to server confirmation time, so close() no longer leaves a permanently-populated handler map that pins the event loop

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

Fixes #33103

🤖 Generated with Claude Code

robobun · 2026-07-03T03:11:30Z

This does not fix #33103, so I'm leaving the Fixes line off.

#33103 is about a successful subscribe followed by close(). That subscribe legitimately registers a listener, and close() leaving the handler map populated is what pins the loop. This PR only changes what happens when a SUBSCRIBE fails: it no longer registers a listener it can't roll back. The successful path still registers one, at confirmation time instead of up front.

Checked against the issue's scenario (scripted RESP3 server, successful subscribe, then close()):

$ ./build/debug/bun-debug issue-33103.ts      # this branch
subscribed -> 1
got: hello
closed both; process should exit now
STILL PINNED
exit=7

$ bun issue-33103.ts                          # main
subscribed -> 1
got: hello
closed both; process should exit now
STILL PINNED
exit=7

Same behaviour either way. #33104 is the one that fixes #33103, by teaching update_poll_ref that a manually closed or failed client can never deliver another message.

The two do overlap on one symptom: a rejected SUBSCRIBE today also pins the loop, and #33104 would mask that for the close()-afterwards case. The double delivery on retry (["hi", "hi"] for one published message) is the part only this PR fixes, and it survives #33104.

coderabbitai · 2026-07-03T03:14:30Z

Walkthrough

This PR defers SUBSCRIBE listener registration until server confirmation. It adds pending-subscription state to command queuing and send paths, snapshots requested channels before sending, wires confirmed subscriptions in the Valkey runtime, and expands tests around rejection, reconnect, and channel immutability.

Changes

Deferred subscription confirmation

Layer / File(s)	Summary
PendingSubscription struct and Entry/PromisePair fields `src/runtime/valkey_jsc/ValkeyCommand.rs`	Adds `PendingSubscription` and optional `pending_subscription` fields to `Entry` and `PromisePair`, and updates `Entry::create` to accept the new payload.
register_subscription and client send signature `src/runtime/valkey_jsc/js_valkey.rs`	Adds `SubscriptionCtx::register_subscription` for confirmed-handler wiring and extends `JSValkeyClient::send` to accept and forward `pending_subscription`.
subscribe() prototype method deferred wiring `src/runtime/valkey_jsc/js_valkey_functions.rs`	Reworks `subscribe` to snapshot channels, build a `PendingSubscription`, pass it to `send`, and update helper call sites to send `None`.
valkey.rs queueing/execution wiring `src/runtime/valkey_jsc/valkey.rs`	Threads `pending_subscription` through `on_auto_flush`, `handle_subscribe_response`, `drain`, `enqueue`, and `send` so subscription registration happens after server confirmation.
Subscribe listener test suite `test/js/valkey/valkey-subscribe-listener.test.ts`	Adds a scripted RESP server and tests for rejected-before-send subscribe, connection-close recovery, event loop exit behavior, and channel-array mutation immutability.

Compact metadata

Related issues: Not specified in provided summary.
Related PRs: Not specified in provided summary.
Suggested labels: valkey, area:runtime, needs-review
Suggested reviewers: Not specified in provided summary.

🚥 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 and concisely summarizes the main change: deferring subscribe listener registration until confirmation.
Description check	✅ Passed	The description covers what changed and how it was verified, even though it uses different section headings than the template.

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

coderabbitai

Actionable comments posted: 3

🤖 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 `@src/runtime/valkey_jsc/js_valkey_functions.rs`:
- Around line 1658-1661: The explanatory comment in the subscription flow is too
long and exceeds the 3-line code comment limit. Trim the block near
SubscriptionCtx::register_subscription / the listener wiring so it stays within
3 lines, and remove the last sentence about snapshotting since that point is
already clear from the code.

In `@test/js/valkey/valkey-subscribe-listener.test.ts`:
- Around line 72-163: The three subscribe listener tests are independent and
each uses its own confirmingRedis server on an ephemeral port, so they should be
marked concurrent instead of running sequentially. Update the test declarations
in valkey-subscribe-listener.test.ts to use test.concurrent (or wrap them in
describe.concurrent) for the cases around RedisClient.subscribe, keeping the
existing assertions and cleanup intact so the tests can run safely in parallel.
- Around line 72-163: Add coverage for the pending-SUBSCRIBE
close/never-confirmed path in the subscribe listener tests. The current tests in
valkey-subscribe-listener.test.ts only cover rejection before send and
channel-array mutation, so add a scripted server scenario where Redis accepts
SUBSCRIBE but never sends the confirmation (or closes mid-flight) and verify the
listener is not registered. Use the existing RedisClient subscribe flow and
assert that shutdown()/fail() leave the handler map clean with no duplicate
delivery or lingering listener.

🪄 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: 7d0a74ff-6850-49b1-8e57-c60fc338f7a0

📥 Commits

Reviewing files that changed from the base of the PR and between 1498d7b and f048368.

📒 Files selected for processing (6)

src/runtime/valkey_jsc/ValkeyCommand.rs
src/runtime/valkey_jsc/js_valkey.rs
src/runtime/valkey_jsc/js_valkey_functions.rs
src/runtime/valkey_jsc/mod.rs
src/runtime/valkey_jsc/valkey.rs
test/js/valkey/valkey-subscribe-listener.test.ts

robobun · 2026-07-03T04:46:16Z

Status: diff is green, CI red on unrelated lanes

Rebased onto aa327ab (trivial conflicts with #33909's bun_core::Error → crate::Error rename). test/js/valkey/valkey-subscribe-listener.test.ts passes 4/4 on every lane that ran it on build 73200: darwin 26 aarch64, debian aarch64/x64/x64-baseline/x64-asan, ubuntu aarch64/x64/x64-baseline, alpine aarch64/x64-baseline, windows 2019 x64/x64-baseline, windows 11 aarch64.

The three hard failures are repo-wide, none touch valkey_jsc/:

darwin 26 aarch64 → test/cli/run/no-orphans.test.ts perl-reaping timeout. 16 of the last 25 failed builds across unrelated branches (serve-alpn, napi, diagnostics-channel, inspector, usockets, bundler, fs-glob, ffi, blob) fail on it. Already owned by a main-break fixer session.
alpine 3.23 x64 / x64-baseline → zero failing tests; dies on the post-run no cores found core-dump step with exit 2. Same two lanes are red on 10+ other recent builds on unrelated branches.

The warning-annotation flakes (bun-add, bun-install-registry, node-tls-connect, bun-upgrade) retried and passed.

Not pushing further retriggers. Ready for a maintainer to merge past the red lanes.

subscribe() added the listener to the receive-handler map before sending the SUBSCRIBE, with no way to remove it if the command failed. The listener outlived the rejected promise: a retry registered it a second time and delivered every message twice, and the non-empty handler map kept the event loop pinned so the process never exited. Carry the channels and listener on the command's promise pair instead, and register them from the subscribe confirmation. A SUBSCRIBE that is rejected or never answered now leaves the map untouched.

Add a test for a SUBSCRIBE that is still in flight when the connection closes: it must not leave its listener registered either. Run the tests in this file concurrently, and keep the subscribe() comment within the 3-line limit.

github-actions Bot added the claude label Jul 3, 2026

robobun mentioned this pull request Jul 3, 2026

redis: fix reply-desynchronization bugs that delivered wrong values to wrong commands #32858

Open

coderabbitai Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread src/runtime/valkey_jsc/js_valkey_functions.rs Outdated

Comment thread test/js/valkey/valkey-subscribe-listener.test.ts Outdated

claude Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread src/runtime/valkey_jsc/js_valkey_functions.rs Outdated

claude Bot mentioned this pull request Jul 6, 2026

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

Open

robobun and others added 2 commits July 15, 2026 06:36

robobun force-pushed the farm/c8b1afeb/redis-subscribe-listener-rollback branch from 5b2c6aa to e2445ab Compare July 15, 2026 06:42

robobun mentioned this pull request Aug 14, 2026

Preserve AsyncLocalStorage context in RedisClient onconnect, onclose and subscribe listeners #38453

Open

Navigation Menu

Uh oh!

redis: register the subscribe() listener only after the server confirms the subscription - #33290

redis: register the subscribe() listener only after the server confirms the subscription#33290
robobun wants to merge 2 commits into
mainfrom
farm/c8b1afeb/redis-subscribe-listener-rollback

robobun commented Jul 3, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

robobun commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Cause

Fix

Verification

Notes

Uh oh!

robobun commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

robobun commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status: diff is green, CI red on unrelated lanes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 3, 2026 •

edited

Loading

robobun commented Jul 3, 2026 •

edited

Loading

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

robobun commented Jul 3, 2026 •

edited

Loading