Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect

[fcostaoliveira]

Summary

shard_connection::handle_event()'s BEV_EVENT_CONNECTED handler calls drain_replay_queue_after_reconnect() (replays in-flight requests that survived a disconnect) before fill_pipeline() gets a chance to re-send the connection-setup ladder (AUTH/SELECT/HELLO/READONLY/CLUSTER SLOTS) on the fresh connection:

// After (re)connect: replay any in-flight requests that survived the
// disconnect. This must happen *before* fill_pipeline() so the old
// requests get back on the wire first; otherwise pipeline ordering
// would shuffle replayed work behind fresh work.
if (m_config->retry_on_error && m_replay_queue && !m_replay_queue->empty()) {
    drain_replay_queue_after_reconnect();
}

Since a fresh TCP connection has none of that connection-scoped server-side state yet, and redis processes a pipelined connection strictly in receipt order, a replayed request can reach the server before AUTH (or SELECT/HELLO/READONLY) is even sent on that connection — under --authenticate + --retry-on-error + --reconnect-on-error, this means a replayed command can get rejected with -NOAUTH on a connection that will authenticate a moment later, silently corrupting that one request's result (or its retry accounting) instead of cleanly failing.

Discovered while working on #525 / PR #526

While implementing --client-no-touch, I found and initially fixed this by moving the setup-ladder send ahead of the replay drain (shard_connection.cpp), and it was called out as worth its own PR + dedicated test coverage during review of #526 (see that PR's history/comments — the fix was reverted out of #526 to keep that PR focused on the --client-no-touch feature itself).

What I found trying to write a regression test

This is genuinely tricky to test reliably:

• A CLIENT-KILL-churn stress test (matching tests/test_reconnections.py's pattern) against a password-protected server, with --retry-on-error --reconnect-on-error, can reproduce the bug via a MONITOR capture (grouping commands by source address:port and asserting the first command from any connection is always AUTH) — but only probabilistically; it depends on catching a kill at the exact moment a request is genuinely in-flight.
• More aggressive kill intervals (~0.1–0.4s) reliably trigger some kind of stall/non-completion within the benchmark's --test-time window, but I found this happens under both the buggy and the fixed code at that aggressivenes, so a "does it hang" signal doesn't actually discriminate between them — it's a separate, orthogonal reconnect-storm-robustness question, not proof of this specific ordering bug.
• A fully deterministic reproduction (guaranteeing something is in-flight at the exact moment of a forced disconnect, without relying on network-timing luck against a live CLIENT KILL) would likely need either a fault-injection hook in the client itself, or a way to pause the C++ event loop at a known point — more invasive than a black-box Python test can do cleanly today.

Suggested approach

1. Re-apply the ordering fix (send the setup ladder before draining the replay queue on reconnect) — verified safe and effective via manual redis-cli MONITOR testing in Support CLIENT NO-TOUCH as a connection-setup command #526's history.
2. Get a reliable regression test in place — possibly by:
   • Increasing pipeline depth / in-flight window further and running many more kill iterations for better odds, accepting some probabilistic slack (matching the rigor bar from PR#364, adapted for a timing-dependent bug), and/or
   • A cheaper, more targeted unit-level test if there's a lower-level hook into shard_connection's reconnect path that doesn't require racing a live process.

Happy to pick this up; flagging now so it's tracked rather than lost in #526's review thread.

[fcostaoliveira]

Update from PR #526's review: this is broader than the --client-no-touch framing I originally scoped it to.

drain_replay_queue_after_reconnect() (shard_connection.cpp:1501) runs before fill_pipeline() (shard_connection.cpp:1508), and fill_pipeline() is the only thing that calls send_conn_setup_commands(). So a replayed request beats the entire setup ladder on a fresh connection, not just CLIENT NO-TOUCH — AUTH, SELECT, HELLO, and READONLY are all exposed the same way. A replayed request landing on the wrong DB under --select-db, or racing ahead of AUTH, is a materially worse outcome than one key's recency being wrong.

I'd added a --client-no-touch-specific startup warning in #526 to at least flag the NO-TOUCH slice of this, but naming only that flag understates the actual affected set and reads as if it's the only thing at risk. Removed that warning from #526 rather than leave a misleadingly narrow one in place — this issue should carry the full disclosure once someone picks it up, covering all the setup-ladder flags this ordering bug actually touches.

[fcostaoliveira]

Another update from PR #526's review — this is a broader class of bug than "replay after reconnect," with at least two more triggers besides drain_replay_queue_after_reconnect():

• handle_retry_drain_event() (shard_connection.cpp:885) gates draining the retry queue only on m_connection_state == conn_connected and m_bev being set — no is_conn_setup_done() check at all. A request queued for retry can be replayed onto a connection that's TCP-connected but still mid-ladder.
• cluster_client.cpp:2731 (the cross-shard MOVED-redirect path) deliberately picks get_connection_state() == conn_connected for writes rather than the full is_ready_for_reads() predicate — the comment there justifies this against m_cluster_slots transiently being non-setup_done during a live refresh, but the same check also skips AUTH/SELECT/HELLO/READONLY/NO-TOUCH. A MOVED'd write can land on a target that's connected but still mid-ladder, with no reconnect involved at all.

So the actual shape of the bug is: several places pick conn_connected (or an incomplete subset of the ladder) as "good enough to send on," when the only thing that's actually correct is is_conn_setup_done(). The reconnect-replay path I originally found is one instance, not the whole bug. Whoever picks this up should probably audit every site that guards a send on connection state rather than trace this fix path-by-path as more get found.