redis: release the event loop when close() is called while subscribed

[robobun]

Fixes #33103.

Repro

With any Redis server running, close a RedisClient that is still in subscriber mode. The script prints its last line but the process never exits.

import { RedisClient } from "bun";
const url = process.env.REDIS_URL;
const publisher = new RedisClient(url);
const subscriber = new RedisClient(url);
await publisher.connect();
await subscriber.connect();

const received = Promise.withResolvers<string>();
await subscriber.subscribe("chan", m => received.resolve(m));
await publisher.publish("chan", "hello");
console.log("got:", await received.promise);

subscriber.close();
publisher.close();
console.log("closed both clients, process should exit now");

$ timeout 15 bun repro.ts; echo "exit $?"
got: hello
closed both clients, process should exit now
exit 124        # killed by timeout; never exits on its own

Uncommenting await subscriber.unsubscribe("chan") before close() makes it exit 0 immediately.

Cause

JSValkeyClient::update_poll_ref() decides whether to hold the event-loop poll ref (and a strong ref to the JS wrapper) from has_activity, which is true whenever the subscription callback map is non-empty:

let subs_deletable = self.client.get().flags.finalized
    || !self._subscription_ctx.get().has_subscriptions(&self.global_object).unwrap_or(false);
let has_activity = has_pending_commands || !subs_deletable || self.client.get().flags.is_reconnecting;

A manual close() (disconnect() -> close() -> on_valkey_close()) sets is_manually_closed, closes the socket, and fails pending commands, but never clears the subscription callback map. So has_subscriptions() stays true, has_activity stays true, and the poll ref plus the strong this_value are held forever. The map is only emptied by an explicit unsubscribe(), which is why the workaround is to unsubscribe before closing.

Fix

Treat a manually-closed client the same as a finalized one when deciding whether subscriptions still count as activity. Once is_manually_closed is set the connection is terminally dead (the socket is gone and no reconnect happens), so its subscription handlers can no longer deliver a message and must stop pinning the event loop. is_manually_closed is reset in do_connect(), so a client that reconnects re-arms its keep-alive normally.

Verification

New test in test/js/valkey/valkey-gc.test.ts (hermetic: an in-process RESP3 mock server, no Docker). It spawns a subscriber subprocess that connects, subscribes, and close()s, with the mock server in the parent process so the subprocess's exit depends only on the client releasing its refs.

• Fails before the fix: the subprocess hangs and is killed by the spawn timeout (signalCode is SIGTERM, not null).
• Passes after the fix: the subprocess exits 0 on its own.

Manual checks against a real Redis with the debug build:

• subscribe -> publish -> receive still works, and close() while subscribed now exits 0.
• a subscribed client that does not close() still keeps the process alive (no over-eager ref drop).

[robobun]

^{Updated 9:23 PM PT - Jun 29th, 2026}

❌ @robobun, your commit baca8a8 has 1 failures in Build #67057 (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

---

🧪   To try this PR locally:

bunx bun-pr 33104

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

bun-33104 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Valkey client shutdown now releases poll references for manually closed or failed subscribers, clears buffered reply state on socket close, and adds concurrent child-process tests for subscriber exit and partial-reply disconnect paths.

Valkey client lifecycle and exit paths

Layer / File(s)	Summary
Poll-ref and activity gating src/runtime/valkey_jsc/js_valkey.rs	update_poll_ref now treats manually closed or terminally failed subscribers as deletable, and reconnect activity only counts while the client has not failed.
Socket close buffer reset src/runtime/valkey_jsc/valkey.rs, test/js/valkey/valkey-gc.test.ts	on_close clears the read buffer and resets the reply scanner, and a concurrent test verifies a dropped half-read reply still lets the process exit.
Subscriber exit and reconnect tests test/js/valkey/valkey-gc.test.ts	Three concurrent tests spawn child Bun processes against mock RESP3 servers and assert clean exit after close(), terminal disconnect with autoReconnect: false, and reconnect retry exhaustion.

Suggested reviewers

• cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately describes the main change to release the event loop on close while subscribed.
Description check	✅ Passed	It includes the repro, root cause, fix, and verification details, even though it doesn't use the template headings exactly.
Linked Issues check	✅ Passed	The code addresses #33103 by making manually closed subscribed clients stop pinning the poll ref, and tests the exit behavior.
Out of Scope Changes check	✅ Passed	The added tests and close-path cleanup stay focused on subscriber shutdown and event-loop release; no clear unrelated changes stand out.

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/valkey/valkey-gc.test.ts`:
- Line 403: The test currently adds an explicit per-test timeout via the
trailing 30_000 on the relevant valkey GC test, which conflicts with the
guideline to rely on Bun’s built-in timeouts. Remove the test-level timeout from
the affected test case in valkey-gc.test.ts and leave the child Bun.spawn({
timeout: 10_000 }) to bound the hang path instead; use the surrounding test
block in valkey-gc.test.ts to locate the change.

🪄 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: 6c92b19c-0d34-404a-b9a3-4abb5544e0a3

📥 Commits

Reviewing files that changed from the base of the PR and between 86d32c8 and b99fff0.

📒 Files selected for processing (2)

• src/runtime/valkey_jsc/js_valkey.rs
• test/js/valkey/valkey-gc.test.ts

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bun	🟢 Ready	View Preview	Jun 29, 2026, 10:47 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/js/valkey/valkey-gc.test.ts:403 — nit: test/CLAUDE.md says "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The Bun.spawn timeout: 10_000 already bounds the pre-fix hang for the signalCode assertion, so the outer 30_000 test-level timeout can be dropped to match the other tests in this file.

  Extended reasoning...

  What

  The new test at test/js/valkey/valkey-gc.test.ts:403 passes 30_000 as the second argument to test.concurrent(...), which sets an explicit per-test timeout in bun:test. test/CLAUDE.md (line 120, under the "### No timeouts" heading) states:

  CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

  None of the other four tests in valkey-gc.test.ts set a test-level timeout, so this is also locally inconsistent with the surrounding style.

  Why it's redundant

  The load-bearing bound for this test is the timeout: 10_000 passed to Bun.spawn at line 386. That spawn timeout is what kills the hung subscriber subprocess in the pre-fix case and produces a non-null signalCode, which is exactly what the assertion at line 398 checks. The outer 30s test-level timeout never participates in the pass/fail decision of the assertions themselves.

  Step-by-step

  1. Post-fix (happy path): the subprocess prints closed and exits 0 in well under a second. proc.exited resolves immediately, all assertions pass. Neither the 10s spawn timeout nor the 30s test timeout fires.
  2. Pre-fix (regression path): the subprocess hangs after printing closed. After 10s, Bun.spawn's timeout sends SIGTERM, proc.exited resolves, proc.signalCode is "SIGTERM", and expect(proc.signalCode).toBeNull() fails with a clear diff. The 30s outer timeout still never fires.

  So in both the passing and failing scenario the explicit per-test timeout is dead weight.

  Caveat

  There is one defensible reason for it: Bun's default per-test timeout is 5s, which is shorter than the 10s spawn timeout. On regression, without the explicit 30_000, the test framework would report a generic "timed out after 5000ms" before the spawn timeout fires and the signalCode assertion runs. That's a slightly worse diagnostic, but it still correctly fails the test — and the project rule is explicit that per-test timeouts shouldn't be used. If the diagnostic matters, lowering the spawn timeout to under 5s (e.g. 4_000) would preserve the clean signalCode diff while staying within the convention. Also worth noting: a few other files under test/js/valkey/ (e.g. test-utils.ts, valkey.test.ts) do set explicit timeouts, so the rule isn't uniformly enforced repo-wide — hence flagging this as a nit rather than a blocker.

  Fix

  Drop the trailing , 30_000 from line 403:

    } finally {
      server.close();
    }
  });

  Optionally, lower the Bun.spawn timeout to ~4s so the signalCode assertion still produces a precise failure message under the default test timeout in the regression case.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/valkey/valkey-gc.test.ts`:
- Around line 353-359: The mock server in the Valkey GC test is matching
protocol tokens directly against each raw TCP data chunk, so HELLO and SUBSCRIBE
can be missed when split across reads. Update the socket handler in the test’s
mock server logic to accumulate incoming bytes in a buffer and inspect the
assembled request framing before replying, using the existing
createServer/socket.on("data") flow so the responses are triggered once the full
command is observed.

🪄 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: 3e655fb1-3e52-4774-9da1-c07bbdb20158

📥 Commits

Reviewing files that changed from the base of the PR and between b99fff0 and 52c12d4.

📒 Files selected for processing (2)

• src/runtime/valkey_jsc/js_valkey.rs
• test/js/valkey/valkey-gc.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/valkey/valkey-gc.test.ts`:
- Around line 450-453: The test in the RedisClient subscribe flow is swallowing
the subscribe() rejection and then unconditionally logging “subscribed,” which
can mask the real regression. Update the code around the Bun.RedisClient
connect/subscribe sequence to let subscribe() failures surface instead of
catching them, and only emit the “subscribed” log after the subscription is
actually established. This should be fixed in the test logic that uses
sub.connect() and sub.subscribe("chan", ...), so the assertion fails for the
correct reason if subscriber mode is never reached.

🪄 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: 7990475b-c32e-4dad-8463-5044ea507ad8

📥 Commits

Reviewing files that changed from the base of the PR and between 6db13fc and b1f7bac.

📒 Files selected for processing (2)

• src/runtime/valkey_jsc/js_valkey.rs
• test/js/valkey/valkey-gc.test.ts

[claude]

🔴 There's one more sibling path where the new is_manually_closed check can't fire: close() called during reconnect backoff. js_disconnect() (line 1040) early-returns when status == Disconnected without setting is_manually_closed, cancelling the reconnect timer, or re-running update_poll_ref() — and status == Disconnected is exactly the state an auto-reconnecting subscribed client sits in for the whole backoff window. The user's close() is silently discarded, is_reconnecting=true keeps has_activity true, and when the timer fires the client reconnects despite having been closed. The early-return is pre-existing, but it's the JS entry point for the very close() flow this PR fixes; setting is_manually_closed=true, clearing is_reconnecting, cancelling the reconnect timer, and calling update_poll_ref() before (or instead of) the early-return covers it.

Extended reasoning...

What the bug is

The new || is_manually_closed term at line 1779 relies on js_disconnect() → ValkeyClient::disconnect() (valkey.rs:1457-1463) having set flags.is_manually_closed = true. But js_disconnect() (js_valkey.rs:1039-1044) early-returns when status == Disconnected:

pub fn js_disconnect(&self, ...) -> JsResult<JSValue> {
    if self.client.get().status == valkey::Status::Disconnected {
        return Ok(JSValue::UNDEFINED);   // ← never reaches disconnect()
    }
    self.client_mut().disconnect();
    ...
}

status == Disconnected is exactly the state a subscribed auto-reconnecting client sits in for the entire reconnect-backoff window: SocketHandler::on_close's defer guard sets status = Disconnected, and ValkeyClient::on_close()'s reconnect branch (valkey.rs:677-682) sets is_reconnecting = true and arms the reconnect timer with a 50ms–2s+ delay. Calling close() in that window is therefore a complete no-op — is_manually_closed stays false, is_reconnecting stays true, the reconnect timer stays armed, update_poll_ref() is never re-run.

Why the PR's check doesn't help here

At line 1779, is_manually_closed = false and failed = false (the reconnect branch doesn't call fail()), so neither new term fires. Even if subs_deletable were true, is_reconnecting = true independently keeps has_activity = true at line 1788. And nothing re-runs update_poll_ref() because js_disconnect() returned before doing anything.

The PR's new source comment at lines 1774-1777 says "a manual close() … must stop pinning the loop" — but on this path manual close() does nothing at all.

Step-by-step proof

Default options (autoReconnect: true, maxRetries: 20):

1. await sub.subscribe("chan", cb) — subscription map populated, status = Connected.
2. Server drops the socket. SocketHandler::on_close → ValkeyClient::on_close() → reconnect branch: is_reconnecting = true, reconnect timer armed via on_valkey_reconnect(). Defer guard then sets status = Disconnected.
3. User calls sub.close() (e.g. shutting down after a network blip). js_disconnect() sees status == Disconnected → return Ok(UNDEFINED). Nothing changes: is_manually_closed = false, failed = false, is_reconnecting = true, reconnect timer still armed, subscription map still populated.
4. Reconnect timer fires → reconnect() → connect().
   • If the server is back: client silently reconnects, status → Connected, update_poll_ref() upgrades this_value and refs the loop. The process is pinned by a client the user believes is closed.
   • If the server stays down: retries continue until exhaustion, then hits the still-open is_reconnecting hang from the other review comment.

This is the same user-visible failure the PR title describes ("close() while subscribed" not releasing the loop), on a sibling state the PR's tests don't exercise — both new tests call close() while status == Connected, or don't call close() at all.

Relationship to other comments

This is distinct from the resolved autoReconnect:false comment (no close() there), the open is_reconnecting-after-retries-exhausted comment (no close() there either, and that's post-exhaustion not mid-backoff), and bug_001 (read_buffer/has_pending_commands). The resolved comment's extended reasoning did flag this exact gap in passing ("the user can't even work around it by calling .close(): js_disconnect early-returns when status == Disconnected"), but it wasn't addressed.

Scope

The early-return in js_disconnect() is pre-existing — the PR doesn't touch that function. But it's the JS entry point for the very close() flow this PR fixes, in the same file, and it directly defeats the line-1779 addition on this path. The same "fix the whole class" rationale was already applied twice in this PR for the autoReconnect:false and failed siblings.

Suggested fix

Before (or instead of) the status == Disconnected early-return in js_disconnect(), set flags.is_manually_closed = true, clear flags.is_reconnecting, cancel the reconnect timer (self.remove_timer(&self.reconnect_timer)), and call self.update_poll_ref(). Alternatively drop the early-return and let disconnect() set the flag (it already guards close() on status), then cancel the timer and update_poll_ref() afterward. Either way reconnect() should probably also check is_manually_closed so an already-armed timer fizzles.

[robobun]

Confirmed real, but scoping this one out of this PR (leaving the thread open for a maintainer to weigh in). Reasoning: it is distinct from the event-loop leak this PR targets. The pure leak aspect is already bounded by the retries-exhausted fix here: if the server comes back the client reconnects and is pinned as a live subscriber (the existing intentional behavior), and if it stays down, retries exhaust and the new is_reconnecting gate releases it. What remains is a close()-semantics bug: close() during the reconnect-backoff window is silently ignored and the client reconnects anyway. Fixing that correctly means cancelling the pending reconnect (timer + is_reconnecting) in js_disconnects status == Disconnected` early-return, plus deciding pending-command/onclose behavior for that window, which is a reconnect-lifecycle change rather than the poll-ref decision this PR fixes. It also has no deterministic hermetic trigger (no event fires at backoff start and the 50ms delay is internal), so it is not cleanly testable here. Happy to do it as a focused follow-up, or fold it in if you would prefer it here.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@test/js/valkey/valkey-gc.test.ts`:
- Around line 570-573: The truncated-reply test is too weak because it only
checks that the child exits cleanly and prints "done", so it can pass even if
the mock never actually sent the partial GET reply. In the valkey GC test around
the `sentPartial` flag and the child-process cleanup, add an explicit assertion
that `sentPartial` became true after the child finishes, so the test proves
`c.get("k")` exercised the truncated-reply path. Keep the assertion tied to the
existing `sentPartial`, `socket.write`, and child completion flow so the failure
clearly indicates the mock path was not triggered.

🪄 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: 33495fe6-f129-4929-8a6f-c6bd4bbe2171

📥 Commits

Reviewing files that changed from the base of the PR and between b1f7bac and 8406aa9.

📒 Files selected for processing (3)

• src/runtime/valkey_jsc/js_valkey.rs
• src/runtime/valkey_jsc/valkey.rs
• test/js/valkey/valkey-gc.test.ts

[robobun]

CI red on this run is unrelated flake, not this diff:

• test/js/node/test/parallel/test-net-connect-memleak.js on alpine x64-baseline: a GC-timing assertion (collected is false at the setImmediate check) in a node:net memleak test. This PR does not touch node:net.
• test/bake/dev-and-prod.test.ts on Windows: flaky HMR dev-server test (auto-retried).

This PR's own tests (test/js/valkey/valkey-gc.test.ts, four regression tests) passed on every lane this run, including the Windows lanes that previously flagged the reconnect-retry test before it was reworked to avoid the closed-port SYN timeout. The diff is green; a re-run should clear the unrelated flakes.

[rppstd]

I'm encountering this in Bun 1.4.0 too. Looking forward to this being merged!

[robobun]

Thanks for the nudge. The fix is up in this PR and its tests are green; it's waiting on a maintainer to review and merge.

In the meantime, on 1.4.0 you can work around it by unsubscribing before closing:

await subscriber.unsubscribe(); // all channels (or pass a specific channel)
subscriber.close();

That releases the same event-loop reference the fix does, so the process can exit.

[robobun]

Closing this PR. #42590 fixed the same defect on main.

• JSValkeyClient::update_poll_ref in src/runtime/valkey_jsc/js_valkey.rs now counts a subscription as activity only while messages can still arrive: the client is connecting, connected, or reconnecting. After close(), or after a failure with no reconnect, a subscribed client releases the event loop.
• The test "a Redis subscription does not keep the process running once its client cannot hear anything" in test/js/bun/module-graph/module-graph-isolation.test.ts covers subscribe() followed by close().
• The on_close change of this PR (clear the read buffer, reset the reply scanner) landed earlier in valkey: run a dial that fails before there is a socket through the deferred close #39513.

Verified on a debug build of main at 0d3492e:

• The repro from RedisClient.close() while subscribed keeps the process alive #33103 prints both lines and exits 0. On a release build from before Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590 (c6b7fcb) it runs until timeout kills it (exit 124).
• The four tests of this PR pass on main without the src/ changes of this PR. On the build from before Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590, the three subscription tests time out.

The fix is in the next canary build and the next release. Until then, call await subscriber.unsubscribe() before close().