valkey: close at once when a TLS fast shutdown is deferred

Merged
AL
3mo ago
+297-20

Problem

  • close() on a rediss:// client does not close when the peer has stopped reading. It returns with connected still true, no onclose, and every in-flight command pending. A peer that never reads keeps it that way for good.
  • Cause: close() asks for a TLS fast shutdown. If the socket still holds ciphertext the kernel will not take, usockets parks the shutdown behind that spill with no timer (packages/bun-usockets/src/crypto/openssl.c, us_internal_ssl_close).

Fix

  • close() still asks for the fast shutdown. If the socket is still open when that returns, usockets deferred it. close() then closes again with the client-detected-failure code, which closes at once and sends an RST.
  • Correct because it detects the deferral instead of predicting it. usockets first tries to drain the spill, so a close() after a stall the peer has recovered from still ends in a FIN. Plain TCP never defers, so redis:// is unchanged.
  • Visible change: close() on a stuck rediss:// peer now returns with connected false, onclose fired, and each pending command rejected with ERR_REDIS_CONNECTION_CLOSED.
  • Verified: test/js/valkey/reliability/connection-failures.test.ts, three new tests. The stuck rediss:// peer test fails on main.

Background

  • A TLS fast shutdown closes without waiting for the peer's close_notify. usockets defers it only when the close carries no reason pointer. This client passes none. A comment at the call site says so.
  • The postgres and mysql clients have the same exposure. Not fixed here.
  • The durable fix is in usockets: bound the deferral with the socket timeout, or add a close code that skips it. Then this check can go.
Notes

History: one fix from a post-merge review of #39511, #39513 and #38281. It was stacked on #39546 (the disconnect() rewrite), which has merged. This branch is rebased onto main. The duplicate() fix has its own PR now, and the TLS context change moved to #39542.

An earlier revision asked whether a spill existed and closed with an RST whenever it did. That would have cut short the recovered-peer case, where usockets can still drain. The current check does not.

A comment in node:net's _handle.close() path described the deferral as waiting only on our own fd. It is corrected. Behaviour there is unchanged.

Test mechanics:

  • Stuck peer over redis://, against an in-process stub. The stub stops reading after HELLO. The client writes 256 KB values until two flushes in a row hand nothing to the socket. Then close() must settle everything at once, the stub must see end (not ECONNRESET) once it reads again, and connect() must open a second connection.
  • The two rediss:// tests use a TLS stub run under Node in its own process, and skip when Node is not installed. It must be a separate process so it can read while the client's loop is blocked. It runs under Node so that its report of the peer's close does not come from the socket code under test. (When this was written, Bun's own sockets reported data followed by a reset as an orderly end. #39600 fixed that and is merged into this branch.) When told to read again, it writes a file once its byte count reaches what the client says it handed over, less one spilled batch, and has stopped growing. When the connection ends it writes one byte, to tell a FIN (the kernel takes it) from an RST (the kernel refuses it).
  • Stuck peer over rediss://: same stall. close() must settle everything at once. The stub, reading again afterwards, must find no end and a reset. Fails on the base branch: close() returns with connected still true.
  • Recovered peer over rediss://: same stall, then the stub drains while the client's loop is blocked, so the spill is still held when close() runs with no loop turn in between. The stub must see the rest of the data, an end, and its write taken. Passes on the base branch, and pins what the earlier revision would have changed. On macOS it cannot tell the two apart: the reset arrives while the stub's receive window is still shut and the kernel discards it. Linux accepts the reset and discards the unread data with it, so there the test should fail against the earlier revision.

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