valkey: count idle time from connect and restart it on incoming data

[alii]

Stacked on #37993, which makes an idle timeout actually close the socket. Will retarget to main once that lands.

The one timer is armed with connectionTimeout when the socket is opened and only re-armed by send(). So a connection that just sits there, or only receives (a subscriber), has its idleTimeout fire when connectionTimeout runs out, 10s by default, whatever idleTimeout was set to.

Re-arm it when the handshake completes, which switches it to the idle interval (or disarms it when idleTimeout is 0), and again on every incoming packet. This is what the postgres client already does in set_status and on_data.

With idleTimeout 100ms a silent server now gets closed at about 100ms, and a server pushing messages keeps the client alive until it stops. Release closes neither within 15s. Two tests in connection-failures.test.ts, no container needed.

---

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/reliability/connection-failures.test.ts

[robobun]

^{Updated 10:46 PM PT - Aug 13th, 2026}

✅ @robobun, your commit a603f2c8360454bf8eadc2a58b32e99eae9896c8 passed in Build #95180! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38281

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

bun-38281 --bun

[claude]

LGTM — the two re-arm sites are the right places and the Connected guard in on_data keeps handshake bytes from resetting the connect-timeout window. The inline nit on the new test's 25ms/100ms margin is worth a look but not blocking.

Extended reasoning...

Overview

Two production lines in src/runtime/valkey_jsc/js_valkey.rs: reset_connection_timeout() is now called (1) at the top of on_valkey_connect() once status is Connected, so the timer switches from connection_timeout_ms to idle_timeout_interval_ms (or disarms when idle timeout is 0, since RefCountedTimer::arm early-returns on ms == 0), and (2) in SocketHandler::on_data() after the read is processed, gated on status == Connected. The test file drops the connectionTimeout: 100 workaround from the existing idle-timeout test (no longer needed now that the timer re-arms on connect) and adds a new test that a server pushing RESP3 messages every 25ms keeps a 100ms idle timeout from firing.

Security risks

None. This only changes when an existing per-client EventLoopTimer is re-armed; no new input parsing, no auth/TLS surface, no cross-client state.

Level of scrutiny

Low-to-medium. The change is a small extension of an established pattern — reset_connection_timeout() is already called from send(), do_connect(), and reconnect(). get_timeout_interval() already branches on status, so calling it from on_valkey_connect (where status == Connected is asserted) picks up the idle interval as intended. The on_data call is placed after client_mut().on_data(data) and inside the existing ref_scope() guard, so this is kept alive and any status transition driven by the incoming bytes (HELLO → Connected, or a fail → Disconnected) is observed before deciding whether to re-arm. Re-arming twice on the HELLO packet (once in on_valkey_connect, once here) is harmless since arm() disarms first.

Other factors

The one finding is a test-timing nit: the new test's 25ms interval vs 100ms idle window is only a ~4× margin and asserts an exact push count, which could flake on the debug+ASAN lane. That's a fair hardening suggestion but doesn't affect the production change. The modified existing test getting simpler (dropping the connectionTimeout override) is a good signal the fix is at the right layer.

[alii]

@robobun fix

[robobun]

Rebased onto the current tip of ali/valkey-fail-recovery (890824d) and force-pushed; the PR is still stacked, so it was rebased onto the base branch rather than main. Head is now 1a13229, diff against the base is the same 5 src lines plus two tests (+64 / -0).

What the rebase ran into:

• on_data in js_valkey.rs now returns the callback's result on the base, so the re-arm sits between on_data() and update_poll_ref(); otherwise unchanged.
• The base grew its own idle timeout test.each (written to hold with or without this change, it pins what the timer does when it fires) and its helloServer hooks now get the socket themselves, so this PR's helper change is gone. Its two tests sit right after that test.each:
  • silent connection with connectionTimeout: 0, so the timer HELLO arms is the only thing that can close the connection; awaited for the first connection and for the one connect() dials after it. Hangs without the src change.
  • PING answered with 30 pushes 20ms apart and no PONG; PING has to be rejected with ERR_REDIS_IDLE_TIMEOUT only after all 30 went out. A timer that data does not restart fails it at about push 20; without the src change it times out.

With the rebase, the base's idle test.each now fires at 50ms instead of at its 500ms connectionTimeout; it and the two tests here pass the file on a debug build and 8 parallel runs of the four idle tests. The previous head (5805fd0) was fully green in CI, including both Windows lanes.

One thing noticed while writing the tests: the description says send() re-arms the timer, but it only arms it when the first command is what opens the connection. So before this change any client with an idleTimeout was closed with "Idle timeout reached" once connectionTimeout elapsed after dialing, busy or not. After it, sending still does not re-arm, so a command sent late in an idle window whose reply takes longer than what is left of the window is rejected as an idle timeout; the postgres client re-arms on sends as well. Left as is since it is a design call for this PR, and both tests hold either way.

Earlier notes (previous head 5805fd0)

The Windows timeouts in build 95116 were the process exits once auto-reconnect gives up hanging, which the base branch's own builds showed before its later commits (93883, 93925) and not after (94344 onwards); the rebase at the time picked those up and 95202 / 95256 were green on Windows. That head's tests were verified 20 out of 20 runs of the file on a Windows x64 debug build, and both tests time out against a build without the src change.

[claude]

LGTM — the reworked tests address the earlier flakiness concern (20ms/400ms is a 20× margin, and awaiting the ping() rejection instead of racing onclose against a sleep is structurally sound).

What was reviewed:

• reset_connection_timeout() in on_valkey_connect correctly switches the timer from connection_timeout_ms to idle_timeout_interval_ms once status is Connected (or disarms when idleTimeout: 0, since arm(0) early-returns).
• on_data re-arm is gated on status == Connected, so a partial handshake reply doesn't reset the connect timeout, and a packet that triggers fail() (status → Disconnected) doesn't re-arm afterward.
• First new test: with connectionTimeout: 0 the initial arm(0) disarms, so without the on_valkey_connect fix nothing ever arms the idle timer and the test hangs — fails for the right reason.

Extended reasoning...

Overview

Two Rust additions in js_valkey.rs (5 lines total): reset_connection_timeout() at the top of on_valkey_connect(), and the same call in SocketHandler::on_data gated on status == Connected. The test file gains two new tests and threads the server socket through helloServer's reply callbacks so a handler can schedule writes on it.

Correctness

get_timeout_interval() returns idle_timeout_interval_ms only when status == Connected, so the on_valkey_connect call (which asserts status == Connected) is the first point where the timer can be armed with the idle interval — before this, do_connect/reconnect armed it with connection_timeout_ms while Connecting. The on_data re-arm is placed after client_mut().on_data(data) and re-reads status, so if that call completed the handshake (status now Connected) it re-arms with idle, and if it failed the connection (status now Disconnected) it skips. Re-arming redundantly right after on_valkey_connect already did is harmless — arm() disarms first. With idleTimeout: 0, arm(0) disarms and returns, so every packet becomes a cheap no-op. This mirrors the postgres client's set_status/on_data pattern per the PR description.

Prior feedback

My earlier note flagged the second test as a 4× timer race (25ms interval / 100ms idle) with an exact toBe(8). Commit 5805fd0 restructured both tests: the second now uses a 20× margin (20ms/400ms), stops at a fixed count of 30 pushes, and awaits client.ping()'s rejection with ERR_REDIS_IDLE_TIMEOUT rather than racing onclose against a wall-clock deadline. A >380ms same-process event-loop stall would be needed to break it, which is well beyond the margin I'd asked for. The first test uses connectionTimeout: 0 so that without the fix the timer is never armed at all and closed.promise hangs — a deterministic failure, not a race.

Security / scrutiny

No security surface. The change is confined to timer bookkeeping on an existing RefCountedTimer; arm() already handles the ref-count and disarm-first internally, so the added call sites don't introduce refcount imbalance. Small, mechanical, follows the sibling client's precedent.

Other factors

This is stacked on #37993 and the previous CI run had Windows timeouts that robobun reported reworking; those are process/CI matters the pipeline will surface. The code change itself is correct and the tests now fail for the right reason without the fix.

[claude]

LGTM — the reworked tests address the timer-race nit (20ms pushes vs 400ms idle, ~20× margin; connectionTimeout: 0 makes the silent-connection test hang outright without the fix), and both threads are resolved.

What was reviewed:

• reset_connection_timeout() in on_valkey_connect correctly switches the armed timer from connection_timeout_ms to idle_timeout_interval_ms via get_timeout_interval(), and disarms when idleTimeout: 0 (arm(_, 0) early-returns after disarm).
• The status == Connected guard in on_data skips re-arming when the packet itself caused a failure/close; ref_scope already brackets the call.
• robobun's note that send() still doesn't re-arm is a separate design question, not a regression here.

Extended reasoning...

Overview

Two small additions to src/runtime/valkey_jsc/js_valkey.rs (5 lines total): call reset_connection_timeout() at the top of on_valkey_connect() so the shared timer switches from counting connectionTimeout to idleTimeout once the handshake completes, and again in SocketHandler::on_data() (gated on status == Connected) so incoming traffic keeps the connection alive. Two new tests in test/js/valkey/reliability/connection-failures.test.ts exercise both paths with the existing helloServer stub — no container required.

Security risks

None. This is idle-timer bookkeeping on an already-open socket; no new input parsing, no auth/TLS changes, no user-controlled data reaches the timer path.

Level of scrutiny

Low-to-medium. The change follows the exact pattern the postgres client already uses (set_status/on_data re-arm). get_timeout_interval() returns idle_timeout_interval_ms when Connected and 0 when failed, and RefCountedTimer::arm(_, 0) disarms-then-returns, so idleTimeout: 0 correctly leaves no timer armed and a mid-on_data failure won't re-arm. The on_data re-arm is placed after client_mut().on_data(data) and before the pre-existing update_poll_ref(), inside the existing ref_scope() guard, so lifetime handling is unchanged. reset_connection_timeout() does not enter JS, so calling it while result may hold a pending JsError is fine (same as the pre-existing update_poll_ref() call in that position).

Other factors

I flagged a timer-race concern on the first revision (25ms/100ms, exact-count assert). robobun reworked both tests in 5805fd0: the silent-connection test now uses connectionTimeout: 0 so it hangs without the src change rather than passing after 10s, and the incoming-data test uses 30 pushes at 20ms against a 400ms idle timeout (a spurious failure would need a ~380ms single stall) with the assertion re-shaped so PING must reject with ERR_REDIS_IDLE_TIMEOUT only after all pushes went out. Both threads are marked resolved, robobun reports 20/20 passes on Windows debug and under 8 parallel Linux debug runs, and Build #95180 is green. The one open design note — send() not re-arming on outbound traffic like postgres does — was explicitly called out and left for a follow-up; it's not a regression introduced here.