valkey: release the socket keep-alive ref at the close-event entry

Merged
AL
3mo ago
+285-41

Supersedes #39193 and keeps its three tests.

The problem

connect() takes one keep-alive ref on the RedisClient for its socket. The ref was released inside on_valkey_close and on_valkey_reconnect. Both are reached only through ValkeyClient::on_close(). Every branch of on_close() rejects promises before it gets there, with a ? after each call. Rejecting a promise fails while the VM's termination is pending. That is the state a terminated worker tears its sockets down in. So on_close() returned early, no releaser ran, and the client's box leaked. LSan reports it as a direct leak of 880 bytes from Box::new. Since #39513 the deferred close for a dial with no socket took a stand-in ref that relied on the same path.

The first failed rejection also stopped the drain of the command queues. The remaining entries were never rejected. Until #39570 their promise handles and serialized bytes leaked as well, because the queues did not drop their items.

What changed

The socket ref is now adopted by a guard at the entry of each close event, right after the existing scoped ref: SocketHandler::on_close, SocketHandler::on_connect_error, and the half-open socket branch of ValkeyClient::close(). That branch also runs on_close() now when an exception is already pending, and returns both results. The deferred close for a dial with no socket no longer takes a stand-in ref. connect() forgets its socket ref only once it has a socket, and that task exists because it never got one, so there was nothing to give back. The two adopts inside on_valkey_close and on_valkey_reconnect are gone.

reject_all_pending_commands keeps reading both queues after a rejection fails and returns the first error. on_close() itself is unchanged.

Why this shape

The ref belongs to the socket. It is released where the socket's close event ends, not where a callee happens to be reached. Any new ? in on_close(), fail() or reject_all_pending_commands can no longer bring the leak back. The MySQL client already does this in its on_close. #34874 and #36837 become unnecessary.

Tests

test/js/valkey/valkey-gc.test.ts, ASAN only. The three cases from #39193 (retry scheduled, autoReconnect off, retries exhausted) terminate a worker with commands in flight. Two more cover the offline queue: commands queued behind a non-pipelined command, and commands queued behind a dial that never completes, which is the on_connect_error entry. All five report the leaked box on main and pass with this change. Two more cover the half-open socket branch of ValkeyClient::close(), on the main thread: the client dials an IP literal whose accept queue is full, so uSockets hands connect() a real socket that never opens and delivers no close event for it. One case calls close() while the dial is pending. The other lets connectionTimeout fire during it. Each asserts the connect() rejection, the queued command's rejection, one onclose call, connected false, and a clean exit under LSan. Both report the leaked box with that branch's adopt removed and pass with it. A main-thread process.exit() variant was tried and dropped: that teardown has no termination pending, the rejections succeed, and nothing leaked before this change either.

Not in this PR

The queues themselves: #39570 (merged) made them VecDeque, so an early return no longer leaks the items left in them. This PR is rebased on it. The drain loop here still rejects every entry instead of stopping at the first failure.

Verification

Debug ASAN build on linux-x64, branch as rebased onto main at 5ebcafff1e (#39570): commits 99cb497, 9aaa1ec, 06ff535. The src diff is the same as before the rebase apart from the pop_front() and mem::take renames that #39570 made in reject_all_pending_commands.

  • bun bd test test/js/valkey/valkey-gc.test.ts with src/ at main: the five worker.terminate() cases fail, 12 pass. Each failure reports the 880 byte Box<JSValkeyClient> as leaked. The two close() cases pass on main as well: that branch released the ref on main too, so they pin the rewritten branch rather than show the leak.
  • The same file with this branch: 17 pass.
  • Before the rebase, on main at 4c689909e6: test/js/valkey/reliability/{connection-failures,recovery,error-handling}.test.ts 28 pass (the rest need the docker setup and skip here, CI runs them), and test/js/web/workers/worker-terminate-lifetime.test.ts 24 pass. Its one failure (the dns.lookup c-ares case) fails the same way with src/ at main and does not involve valkey. It is reported separately.

9aaa1ec and 06ff535 only touch the test file.

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/valkey/valkey-gc.test.ts