redis: accept unsubscribe acks that arrive outside subscriber mode

[robobun]

Repro

Debug (or ASAN) build with a redis-server on 6379:

const client = new Bun.RedisClient("redis://127.0.0.1:6379");
await client.connect();
await client.subscribe("a", () => {});
await client.subscribe("b", () => {});
// both UNSUBSCRIBEs are issued before either ack arrives
await Promise.all([client.unsubscribe("a"), client.unsubscribe("b")]);

panic: assertion failed: self.is_subscriber()
<bun_runtime::valkey_jsc::js_valkey::JSValkeyClient>::on_valkey_unsubscribe   src/runtime/valkey_jsc/js_valkey.rs:1320
<bun_runtime::valkey_jsc::valkey::ValkeyClient>::handle_subscribe_response    src/runtime/valkey_jsc/valkey.rs:945

client.punsubscribe("x*") (or client.send("UNSUBSCRIBE", [])) on a client that never subscribed aborts the same way. Sequential unsubscribes and a bare unsubscribe() are fine. Found by the fuzz suite (test/js/bun/util/fuzzy-wuzzy.test.ts), which takes the whole file down with this abort on debug builds.

Cause

on_valkey_unsubscribe asserted that the client is still in subscriber mode when an UNSUBSCRIBE ack arrives. That invariant does not hold by design:

• unsubscribe() removes the channel's handlers at call time. With two UNSUBSCRIBEs issued back to back the handler map is already empty when the first ack arrives, so remove_subscription() leaves subscriber mode right there. The second UNSUBSCRIBE (queued behind the first, since subscription commands are not pipelined) is acked afterwards, still has its own in-flight promise pair, and is routed to the same handler via request_is_subscribe while is_subscriber() is already false.
• punsubscribe() is allowed outside subscriber mode, and Meta::check marks raw UNSUBSCRIBE/PUNSUBSCRIBE sends as subscription requests too, so their acks reach the handler on a client that was never in subscriber mode at all.

The bookkeeping itself is fine in both cases: the handler's remaining work (flush the write queue, recompute the poll ref) is valid in either mode, remove_subscription() is idempotent, and each ack still resolves its own promise. Release builds, where debug_assert! compiles out, already behave correctly here (both promises resolve, the client is back in normal command mode). The assertion is the only thing that is wrong, so this removes it rather than changing when subscriber mode is exited.

Fix

Drop the debug_assert!(self.is_subscriber()) in on_valkey_unsubscribe; the source change is that one deletion, and the tests below pin down each case that reaches the handler outside subscriber mode. The matching assertion in on_valkey_subscribe stays: add_subscription() runs right before it, so it does hold there.

#34829 removes the same line in passing as part of a much larger refactor; this is the focused fix with tests.

Tests

test/js/valkey/reliability/resp-nesting-depth.test.ts ("RESP push frame routing", docker-free, mock server replaying the exact RESP3 frames redis 8 sends): overlapping unsubscribe() calls, punsubscribe() from a non-subscriber, raw UNSUBSCRIBE through send() from a non-subscriber. Each asserts the acks resolve and that a regular command is accepted afterwards.

test/js/valkey/valkey.test.ts (PUB/SUB, real server): overlapping unsubscribe() calls (also checks via publish that the server dropped both channels), and punsubscribe() from a non-subscriber.

All five abort with the panic above on an unfixed debug build and pass with the fix (bun bd test). The existing PUB/SUB describe still passes against a local redis 8.

[robobun]

Reproduced on a debug build of main (827475e) with redis 8 on 127.0.0.1:6379: two back to back unsubscribe() calls abort with panic: assertion failed: self.is_subscriber() in JSValkeyClient::on_valkey_unsubscribe, as does punsubscribe() on a client that never subscribed. Release builds are unaffected (the check compiles out and both cases already behave correctly).

Fix and tests are in this PR (current head ec7edb9: the source change is the one-line assertion removal, plus five tests). Each of the five tests aborts on the unfixed debug build and passes with the fix.

CI for this head (build 91838) finished with 192 jobs passed and no red test. The build is marked failed only because the four macOS 14 test jobs expired without ever getting a runner (the previous build's macOS jobs expired the same way); the macOS 26 jobs ran and passed. The four recorded test failures all passed on retry and are unrelated (next-pages/dev-server-ssr-100 on Windows aarch64, inspect-error-leak, malformed-integrity-base64, puppeteer download in test/package.json). The diff itself is ready to merge.

[coderabbitai]

Walkthrough

Valkey now accepts unsubscribe acknowledgements after subscriber mode clears. Tests cover concurrent unsubscriptions, pattern unsubscriptions without active subscriptions, null-channel replies, and resumed ordinary commands.

Changes

Valkey unsubscribe handling

Layer / File(s)	Summary
Accept late unsubscribe acknowledgements src/runtime/valkey_jsc/js_valkey.rs	on_valkey_unsubscribe no longer asserts that the client remains in subscriber mode.
Validate Pub/Sub transitions test/js/valkey/reliability/resp-nesting-depth.test.ts, test/js/valkey/valkey.test.ts	Tests cover concurrent and pattern unsubscriptions, raw unsubscribe acknowledgements, cleared subscriptions, and subsequent ordinary commands.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 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 identifies the primary fix: accepting unsubscribe acknowledgements outside subscriber mode.
Description check	✅ Passed	The description explains the cause, fix, affected scenarios, and verification results, covering the template requirements.

---

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

[claude]

LGTM — removes a debug assertion whose invariant is demonstrably false, with a clear explanatory comment in its place.

What was reviewed:

• Traced the ack path (handle_subscribe_response → on_valkey_unsubscribe → remove_subscription) and confirmed unsubscribe() clears handlers eagerly at call time, so the first ack of several in-flight UNSUBSCRIBEs legitimately flips is_subscriber off before the rest arrive.
• Confirmed request_is_subscribe (via Meta::SUBSCRIPTION_REQUEST) routes acks to this handler independent of is_subscriber(), so punsubscribe() / raw UNSUBSCRIBE from a non-subscriber reach it too.
• Checked that the handler's remaining work (on_writable, update_poll_ref) and the following remove_subscription() are safe/idempotent when already out of subscriber mode; the surviving this_value.is_strong() assertion still holds (in-flight promise pair keeps it upgraded).
• Tests cover all three variants against both a mock RESP3 server and the real-server harness, and assert normal-command mode is restored afterwards.

Extended reasoning...

Overview

The PR removes one debug_assert!(self.is_subscriber()) from JSValkeyClient::on_valkey_unsubscribe in src/runtime/valkey_jsc/js_valkey.rs and replaces it with a four-line comment explaining why the invariant does not hold. Five regression tests are added: three mock-server tests in test/js/valkey/reliability/resp-nesting-depth.test.ts (using the file's existing createMockRedisServer helper) and two real-server tests in the PUB/SUB describe of test/js/valkey/valkey.test.ts.

Root-cause verification

I traced the call path against the surrounding code. unsubscribe() in js_valkey_functions.rs calls clear_receive_handlers synchronously at call time, so with two back-to-back unsubscribes the callback map is empty before either ack lands. The ack path is valkey.rs:945 on_valkey_unsubscribe() → valkey.rs:946 remove_subscription(); remove_subscription() sets is_subscriber = false when has_subscriptions() is false, which it is on the first ack. The second ack still reaches handle_subscribe_response because valkey.rs:1147 also gates on request_is_subscribe (the promise pair's SUBSCRIPTION_REQUEST meta), independent of is_subscriber(). So the assertion was simply wrong. The handler body — on_writable() + update_poll_ref() — is mode-agnostic, and remove_subscription() is idempotent, so release builds (where debug_assert! is a no-op) already behaved correctly. The other assertion in the function (this_value.is_strong()) is left in place and still holds: reaching this handler requires an in-flight promise pair, which keeps this_value upgraded via update_poll_ref.

Security risks

None. This is a debug-build-only assertion removal in the Redis pub/sub ack handler; no auth, crypto, parsing, or memory-ownership changes. Release-build behavior is byte-for-byte unchanged.

Level of scrutiny

Low-to-medium. The runtime change is a one-line deletion whose only effect is to stop aborting debug/ASAN builds on a legitimate state; release semantics are untouched. The PR description proves the mechanism rather than correlating ("the crash goes away"), and #34829 independently deletes the same line as part of a larger refactor, corroborating that the assertion is not load-bearing.

Other factors

The tests follow the repo conventions well: they reuse the existing mock-server harness in the same file (with a small withMockClient helper to avoid duplicating try/finally), assert exact resolved values (not just "doesn't throw"), verify the client returns to normal command mode via set() (which throws synchronously in subscriber mode), and — in the real-server variant — check via publish() returning 0 that the server actually dropped both channels. The variant matrix (overlapping unsubscribe / punsubscribe-never-subscribed / raw send("UNSUBSCRIBE", [])) covers every route named in the cause analysis. The PR states all five abort on an unfixed debug build and pass with the fix, satisfying the "fails for the right reason" bar.

[robobun]

^{Updated 3:05 AM PT - Aug 11th, 2026}

❌ @robobun, your commit ec7edb9 has some failures in Build #91838 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37340

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

bun-37340 --bun

[claude]

LGTM — one-line deletion of a debug assertion that encodes an invariant that does not hold, with five tests pinning down each path.

What was reviewed

• Traced the caller in valkey.rs:handle_subscribe_response — on_valkey_unsubscribe runs before remove_subscription(), and remove_subscription() is idempotent, so the handler's remaining work (on_writable + update_poll_ref) is valid regardless of subscriber state.
• Checked the surviving this_value.is_strong() assertion still holds for the punsubscribe-from-non-subscriber path (update_poll_ref upgrades to strong on Connected, which must be the case to receive an ack).
• Tests follow existing harness patterns (createMockRedisServer, ctx.newSubscriberClient), assert both ack resolution and restored command mode, and the comment-cop feedback was addressed.

Extended reasoning...

Overview

The PR removes a single debug_assert!(self.is_subscriber()) from JSValkeyClient::on_valkey_unsubscribe in src/runtime/valkey_jsc/js_valkey.rs. Five new tests are added: three mock-server tests in test/js/valkey/reliability/resp-nesting-depth.test.ts (docker-free, replaying exact RESP3 push frames) and two real-server tests in test/js/valkey/valkey.test.ts (PUB/SUB describe). Each covers one of the three paths that legitimately reach the unsubscribe ack handler while is_subscriber() is false: overlapping unsubscribe() calls, punsubscribe() from a non-subscriber, and raw send("UNSUBSCRIBE", []) from a non-subscriber.

Security risks

None. This is a debug-only assertion; release builds compile it out and already behave correctly (both promises resolve, client returns to command mode). No untrusted input handling, no auth/crypto/permissions surface, no protocol parsing changes.

Level of scrutiny

Low. The source change is a one-line deletion whose only effect is on debug/ASAN builds (stops aborting on a legitimate sequence). I traced the call site in valkey.rs: handle_subscribe_response calls on_valkey_unsubscribe() then remove_subscription(). The handler body is now on_writable() + update_poll_ref(), both mode-agnostic; remove_subscription() is guarded on has_subscriptions() and idempotent. The PR description's mechanism analysis (handlers dropped at call time → map empty when first ack arrives → subscriber mode exits before the second ack) matches the code in remove_subscription() and js_unsubscribe. The remaining this_value.is_strong() assertion is preserved and still holds because update_poll_ref() upgrades to strong whenever status == Connected, which is required to receive an ack.

Other factors

Tests are well-constructed per REVIEW.md: they assert exact resolution values (toEqual([undefined, undefined]), { type: "punsubscribe", data: [...] }), verify normal command mode is restored via a follow-up set() (which throws synchronously in subscriber mode), and the real-server variant additionally verifies via publish() returning 0 that the server dropped both channels. The mock-server tests reuse the existing createMockRedisServer helper and add a small withMockClient wrapper that follows the file's try/finally cleanup pattern. The comment-cop bot flagged an earlier explanatory comment in the source, which was removed in ec7edb9 — the tests now serve as the documentation. PR notes that #34829 removes the same line as part of a larger refactor; this is the focused fix.

[robobun]

Still reproduces on current main (8326d1b) with a debug build, including the zero-argument form that test/js/bun/util/fuzzy-wuzzy.test.ts reaches through Bun.redis.punsubscribe():

const client = new Bun.RedisClient("redis://127.0.0.1:6379");
await client.connect();
await client.punsubscribe(); // panic: assertion failed: self.is_subscriber() (js_valkey.rs:1292, on_valkey_unsubscribe)

Release prints { type: "punsubscribe", data: [null, 0] }. The bare PUNSUBSCRIBE is acked with a null subject and goes through the same handle_subscribe_response -> on_valkey_unsubscribe path as the cases tested here, so the assertion removal in this PR covers it too.