Support CLIENT NO-TOUCH as a connection-setup command

[fcostaoliveira]

Summary

Fixes #525. Adds --client-no-touch, which sends CLIENT NO-TOUCH ON once per connection as a new step in the existing connection-setup ladder (alongside AUTH/SELECT/HELLO/READONLY/CLUSTER SLOTS in shard_connection.cpp) — not via --command, since that would re-send it every N ops and pollute the ops budget and latency histogram. Redis protocol only (7.2+; deps/commands_json/client-no-touch.json confirms since: 7.2.0); the CLI rejects the flag on memcache protocols, and an unsupported/erroring server fails the connection loudly via the existing report_connection_stage_failure path rather than silently benchmarking the wrong thing.

• shard_connection.h/shard_connection.cpp: new rt_no_touch request type and m_no_touch_state ladder member, threaded through connect()/disconnect()/is_conn_setup_done()/send_conn_setup_commands()/process_response()/the replay-drop list/push_req()/get_last_request_type(). Appended to the ladder after READONLY (not between HELLO and READONLY as in an earlier version of this PR) so AUTH/SELECT/HELLO/READONLY keep the relative wire order they've always had — nothing in the ladder depends on NO-TOUCH's position specifically, since it's one pipelined pass with no reply waited on in between. Sent via bufferevent_write with the RESP bytes inline (not a protocol-level evbuffer_add call), following the same precedent READONLY already sets for a shard connection discovered mid-run through a live CLUSTER SLOTS refresh (attached via bufferevent_socket_new + a later bufferevent_socket_connect). Whether that precedent's original diagnosis (the first user-level send on such a connection doesn't get its EPOLLOUT armed by the plain evbuffer path) actually holds is an open question -- empirically, HELLO already flushes fine via the plain path on the same connection type, which contradicts it as a universal claim -- tracked in Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532 rather than settled here, since bufferevent_write is safe either way (identical wire bytes). Unlike READONLY, NO-TOUCH arms on every connection including primaries.
• memtier_benchmark.h/memtier_benchmark.cpp: --client-no-touch CLI flag, redis-protocol-only validation (checked both at parse time and again at connect-arm time), --help text, --show-config/JSON provenance fields (placed right after print-all-runs, before the #ifdef HAVE_EVHTTP block, in both config_print() and config_print_to_json() — matching how print-all-runs itself was appended in PR#298).
• cluster_client.cpp: classify_read() classifies rt_no_touch as non-read, like the other setup commands.
• memtier_benchmark.1, bash-completion/memtier_benchmark: documented (hand-edited rather than a full help2man regen, to avoid pulling in unrelated pre-existing drift in the man page).
• tests/test_cli_validation_client_no_touch.py: parse-time protocol-gating tests (no live server needed).
• tests/test_client_no_touch.py: functional tests against a real redis-server — OBJECT IDLETIME must not reset under the flag (positive case, also asserting ALL STATS.Gets.Count > 0 so it can't pass vacuously on zero traffic) and must reset without it (negative control). A third test uses an ACL user with client|no-touch denied to prove the server-rejection path fails loudly, without needing an actual pre-7.2 server.
• tests/test_client_no_touch_cluster.py + .github/workflows/ci.yml: two tests, both actually running (not skipped) in a new dedicated OSS-CLUSTER + replicas: client-no-touch CI cell — one confirms NO-TOUCH lands on a replica connection via MONITOR, the other on a non-seed primary shard (any shard beyond memtier's bootstrap seed connection is reached via the identical live-discovery path, so this needs no replicas at all).
• tests/run_tests.sh: the OSS_CLUSTER_REPLICAS=1 block now sets MEMTIER_CLUSTER_REPLICAS_EXPECTED=1 inside its own subshell (not globally), so the new cluster test's hard-fail gate below is driven by a signal scoped to the specific RLTest invocation that actually requested replicas, not one that stays set across a script run that makes more than one invocation.
• The RLTest fork's branch→SHA pin (tests/test_requirements.txt) was split out into ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531, opened directly off master, so that CI-dependency change isn't gated on this PR's review. This branch does not carry that pin itself -- tests/test_requirements.txt here matches master (still the branch name) until ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 merges and this branch picks it up on the next merge from master. The new OSS-CLUSTER + replicas: client-no-touch cell's hard-fail-on-missing-replicas behavior is unaffected by which of the two tests/test_requirements.txt states is in effect; only the SHA-vs-branch-name mutation risk depends on ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 landing first.

Known limitation (pre-existing, not introduced here)

Under --retry-on-error, several places gate a send on the connection just being TCP-connected rather than on the full setup ladder being done (is_conn_setup_done()): drain_replay_queue_after_reconnect() runs before fill_pipeline() (the only caller of send_conn_setup_commands()) on reconnect, handle_retry_drain_event() gates only on m_connection_state, and the cross-shard MOVED-redirect path in cluster_client.cpp deliberately uses a partial readiness check. So a replayed, retried, or redirected request can beat AUTH, SELECT, HELLO, READONLY, and CLIENT NO-TOUCH on a connection that hasn't finished its ladder yet — landing on the wrong DB under --select-db is a materially worse instance of this than one key's recency being wrong under --client-no-touch.

This predates this PR and affects every setup-ladder-dependent flag, not just this one. I found and initially fixed the reconnect-replay instance, then added a --client-no-touch-specific warning about it, but both were the wrong scope for this PR: the fix needs its own PR + regression test (reliably testing a reconnect-timing race turned out to be genuinely nontrivial — see the issue for what I tried), and a flag-specific runtime warning understated the actual affected set. Filed as #527 with the full finding, including the retry-drain and MOVED-redirect instances found later. Where to disclose #527 in-product oscillated across review rounds (a --client-no-touch-specific --help/man-page line, added, then reverted as still too narrow; a general one, added, then moved to README); landed on README's Known limitations section, positioned near the other cluster/testing caveats rather than as the first thing under "Using memtier_benchmark" — not on --client-no-touch's own --help/man-page entry, since #527 predates this flag and hits --select-db harder.

Test plan

• Clean autoreconf -ivf && ./configure && make build, no warnings; make format / clang-format --dry-run --Werror clean on every touched file.
• Manual redis-cli MONITOR verification against real clusters (2-shard, TLS and non-TLS, with and without a real cluster-replicated replica): CLIENT NO-TOUCH ON is the first command observed on the bootstrap connection, a non-seed primary, and a replica alike.
• Functional verification against a real redis-server: OBJECT IDLETIME keeps growing under --client-no-touch GET traffic, resets without it, and the ACL-rejection path aborts the run with a clear CLIENT NO-TOUCH failed message and non-zero exit.
• --client-no-touch rejected with a clear error on -P memcache_text/memcache_binary.
• All new tests (test_cli_validation_client_no_touch.py, test_client_no_touch.py, test_client_no_touch_cluster.py) pass under RLTest, standalone and cluster (including the new dedicated CI matrix cell).
• Backward-compat: identical --help/--show-config/CLI-validation behavior vs origin/master when the flag is unset, aside from the additive client-no-touch config-echo key; disassembly check found no added instructions on the request-processing dispatch path when the flag is off (is_conn_setup_done() does carry one more && term now, a real but trivial addition at its one call site).
• Reconnect stress run (--reconnect-on-error + --client-no-touch under repeated CLIENT KILL): clean completion, no hang.

---

Note

Medium Risk
Changes the per-connection setup ladder and readiness gating on a hot path when the flag is enabled; default-off behavior is preserved, but mis-timed sends under --retry-on-error remain a documented pre-existing risk (#527).

Overview
Adds --client-no-touch, a Redis-protocol-only flag that sends CLIENT NO-TOUCH ON once per connection as a new step in the existing setup ladder (after READONLY, before CLUSTER SLOTS), so benchmark traffic does not refresh key LRU/LFU recency. Rejected servers or ACL denials fail the connection via the same stage-failure path as other ladder steps; memcache protocols are rejected at CLI parse time.

Implementation extends shard_connection with rt_no_touch / m_no_touch_state (included in is_conn_setup_done(), reconnect re-arm, replay drops), uses bufferevent_write for the wire bytes (including cluster-discovered shards), and treats the step as non-read in cluster_client. Config surfaces through --show-config, JSON output, man page, and bash completion.

Tests and CI: new RLTest modules for CLI validation, standalone behavior (OBJECT IDLETIME, loud failure, reconnect MONITOR), and cluster coverage (replica + non-seed primary); a dedicated OSS-CLUSTER + replicas: client-no-touch matrix job; shared helpers in tests/include.py and MEMTIER_CLUSTER_REPLICAS_EXPECTED scoping in run_tests.sh. README documents the pre-existing --retry-on-error setup-ladder race (#527) and revises read-preference testing limitations.

^{Reviewed by Cursor Bugbot for commit 9c31edc. Bugbot is set up for automated code reviews on this repo. Configure here.}

[github-actions]

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

Thanks for this — went through the ladder threading and the CLI/doc surface, a few comments below.

The rt_no_touch plumbing looks complete to me: every site that currently handles rt_readonly/rt_hello (disconnect()'s replay-drop list, get_last_request_type(), push_req()'s switch, process_response(), cluster_client::classify_read()) has NO-TOUCH wired in, and the AGENTS.md new-flag checklist is fully covered — enum, long_options[], switch case, config field, usage(), man page, bash completion, tests. Setup requests are never m_serialized, so try_claim_for_retry() can't pick one up, which seems right. Default-off really is a no-op apart from the extra && in is_conn_setup_done(), whose only call site is fill_pipeline().

Two things I'd want settled first:

RLTest dependency ordering. The README now says "This repo's pinned RLTest fork (tests/test_requirements.txt)", but on this branch that file is still master's git+https://github.com/fcostaoliveira/RLTest.git@fix/cluster-aware-replicas — a mutable branch on a personal fork, not a pin. Meanwhile the new OSS-CLUSTER + replicas: client-no-touch cell is, per its own docstring, the first test in the repo to hard-fail rather than skip when no replica is discovered. So we'd be making CI red-able by a force-push to a branch outside this repo. Wouldn't it be safer to land #531 and rebase first, so the hard-fail and the README's "pinned" wording become true at the same time?

The second literal RESP string. AUTH/SELECT/HELLO/CLUSTER SLOTS all go through m_protocol; READONLY_CMD was the one exception, and NO_TOUCH_CMD now makes two. You've been honest that #532 leaves the original EPOLLOUT rationale unproven — but that cuts the other way for me: I'd rather not grow a second copy of a workaround we can't currently justify. Is there a reason the bytes can't live next to the other command writers (a write_command_no_touch()), even if the send still goes out via bufferevent_write? I may be wrong about how cleanly that composes with the evbuffer path, so this may be worth a second look from whoever owns the cluster/bufferevent side.

Smaller things, none blocking:

• README's new top-level Known limitations section is about Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527, which predates this PR and hits --select-db harder — I agree it's worth documenting, I'm just not sure a --client-no-touch PR is where it should land. Its own doc PR?
• Nitpicking: in test_client_no_touch_resent_after_reconnect, new_ids = client_list(...) - pre_run_ids kills every id that appeared since the snapshot. In a shared env that window isn't strictly memtier's, is it? Filtering on laddr/name would be tighter. Given this file already burned two rounds on CI flakes, probably cheap insurance.
• 40 commits titled "Nth review round" — assuming these get squashed on merge, but worth confirming.

The core change looks solid and unusually well covered by tests; I'd mainly want #531 sequenced ahead of this and the bufferevent_write layering question answered before it goes in.

(would file this as a comment, not a formal block)

[fcostaoliveira]

Thanks for the thorough re-review — went through each point:

• Unused write_command_client_no_touch(): dropped, along with the two assert(0) memcache stubs. Agreed, no reason to keep the wire format in two places once nothing calls the virtual.
• Replay-ordering reorder: reverted out of this PR. You're right that it's broader than --client-no-touch and deserved dedicated test coverage rather than riding along here. While building that regression test I found the race is genuinely hard to pin down reliably — a CLIENT-KILL-churn stress test against a password-protected server only catches it probabilistically, and pushing the kill interval aggressive enough to reproduce it consistently causes stalls under both the buggy and the fixed code, so "does it hang" turned out not to be a discriminating signal either. Filed as Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 with notes on what I tried, so it doesn't get lost.
• Cluster coverage for the live-discovery bufferevent_write path: fair callout that the MONITOR verification was manual, not automated. Left as-is for this PR (didn't want to add a second not-fully-baked test alongside the one above), but agree it's worth a cheap automated check — possibly piggybacking on however Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 ends up testing reconnect/discovery timing, since it's a related problem (proving something landed on a specific connection at a specific point in its lifecycle).
• Generic --connection-setup-command: I'd lean toward keeping dedicated ladder steps for now — CLIENT NO-TOUCH needs its own response handling (loud failure on rejection, not just fire-and-forget), which a fully generic passthrough wouldn't get without extra machinery anyway. Open to revisiting if a third command shows up and the churn argument gets stronger.
• is_ready_for_reads() / m_no_touch_state: intentional — the NO-TOUCH bytes are already ahead of any user traffic by construction (sent before READONLY/CLUSTER SLOTS), so gating on it would be redundant rather than wrong. Appreciate the flag regardless, it's subtle.
• maxmemory-policy not restored: fixed, both tests now save/restore the prior value.

PR description updated to match (test-plan checkbox corrections, known-limitation note restored, #527 linked).

[fcostaoliveira]

Appreciate the ones you went and checked yourself too (bufferevent_write/set_buffers() ordering, config_init_defaults()) — saves us both a round-trip.

1. Vacuous-pass gap: fair, and an easy miss on my part since the negative-control test happens to be self-protecting there but the positive one isn't. Fixed — it now asserts ALL STATS.Gets.Count > 0 from mb.json before checking idle time, same invariant-pinning approach as test_json_output_integrity.py.
2. Document the --retry-on-error gap: agreed, done in both --help and the man page entry, with the Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 reference in the man page (kept --help itself terse/self-contained, matching how this project doesn't otherwise cite issue numbers in user-facing --help text — only in code comments).
3. is_ready_for_reads() comment: added, and tried to be precise about the routable-but-not-sendable distinction the way you framed it — is_conn_setup_done() is the send gate and includes NO-TOUCH, is_ready_for_reads() is the routing-eligibility check and doesn't need to, since by construction anything routable already has NO-TOUCH's bytes ahead of it on the wire.
4. Comment nitpick: fixed — you're right, it's wire ordering in one pipelined write that matters, not HELLO's negotiation having completed (NO-TOUCH's reply is a plain +OK regardless of RESP2/RESP3).

All pushed. PR description's test-plan and follow-ups sections updated to match.

[fcostaoliveira]

Went through all four:

--retry-on-error warning: agreed — added a startup warning, not just documentation. Someone combining the two flags now sees it immediately regardless of whether they read --help.

Cluster-path test: this ran into a real, pre-existing wall — get_cluster_replica_connections() (used by this repo's own --read-preference test suite) returns an empty list under RLTest's --use-slaves, because the resulting slaves are started via --slaveof without --cluster-enabled yes and never join cluster gossip. It's documented in README's "Testing limitations" section and tracked as #462, filed for the identical gap affecting test_read_preference_failover.py and friends. I didn't want to build a one-off cluster-bootstrap fixture in this PR that duplicates whatever #462's real fix ends up being, so I added tests/test_client_no_touch_cluster.py using the same get_cluster_replica_connections() gate and skip pattern this repo's read-preference tests already use — it'll start actually verifying automatically once #462 lands. In the meantime I manually bootstrapped a real CLUSTER MEET/CLUSTER REPLICATE cluster and ran the test's exact logic against it: CLIENT NO-TOUCH ON is the first command MONITOR observes on the replica connection, confirming the test itself is correct, not just theoretically plausible.

skipOnVersionSmaller / the loud-failure path: confirmed real — RLTest.Env.skipOnVersionSmaller exists in the pinned RLTest fork (0.7.27) and every functional test in this PR's history has used it successfully against the CI-installed Redis (8.x, well above 7.2). For exercising the actual server-rejection path without a pre-7.2 binary: good idea on the ACL angle — added test_client_no_touch_rejected_by_server_fails_loudly, which denies the client|no-touch subcommand to an ACL user and confirms memtier aborts with a clear CLIENT NO-TOUCH failed message and non-zero exit. -NOPERM and an old server's -ERR unknown command hit the exact same code path (rt_no_touch's error branch in process_response()), so this is a faithful proxy.

--help length / issue-number citation: fair, trimmed it back to just what the flag does. The #527 detail now lives only in the man page.

Comment nitpicks: both shortened as suggested — the NO-TOUCH send comment now points at READONLY's comment instead of restating it, and the is_ready_for_reads() note moved to sit above the function.

PR description and test-plan updated.

[fcostaoliveira]

All three real ones fixed, plus the nitpick:

• --show-config gap: confirmed and fixed — config_print() was missing client-no-touch while config_print_to_json() already had it. Added right next to select-db/no-expiry, same as the JSON echo's placement.
• Stale predicate comment in cluster_client.cpp: fixed, added m_no_touch_state to the explanatory list so it matches what is_conn_setup_done() actually checks.
• Disassembly claim: fair, softened. is_conn_setup_done() does carry one more && now and fill_pipeline() calls it per pipeline slot — a real (if trivial) addition on that specific call site. Didn't want the test plan to read as more measured than what was actually checked.
• Docstring nitpick: fixed — three tests, three bullets now.

Also confirms the ASAN OSS-CLUSTER TLS reconnections failure on this run was an unrelated pre-existing flake (test_reconnections.py, untouched by this PR, hit its hardcoded 60s+10s subprocess timeout under the ASAN+TLS+cluster combination) — passed clean on re-run, and nothing in this PR's diff at that point touched anything relevant to reconnection timing.

[fcostaoliveira]

Good catch on the comment, and good push on the test:

is_ready_for_reads(): you're right, and I should have traced the primary case through before writing that. Corrected — the exclusion is still safe, but for the reason you named (fill_pipeline()'s separate is_conn_setup_done() gate), not because NO-TOUCH is already ahead on the wire, which only actually holds for the replica/READONLY branch. Also toned down the peer_client_has_any_setup_in_progress() claim to "believed harmless" rather than implying it's been verified for that caller's specific needs.

READONLY comment: fixed, scoped to the branch it's actually about now.

"Is there anything cheaper": yes, and thanks for pushing on it — cluster_client's CLUSTER-SLOTS-reply loop connects every shard beyond memtier's single bootstrap seed via connect_shard_connection(), same as a replica, so a plain multi-shard cluster (no --use-slaves needed) already exercises the exact path in question for shard 2+. Added test_client_no_touch_lands_on_non_seed_primary_discovered_via_cluster_slots, verified against a real 2-shard cluster, and it runs (not skips) in the standard OSS-CLUSTER matrix. If someone simplifies NO-TOUCH back onto the plain evbuffer_add path now, this actually goes red.

All pushed, PR description updated.

[fcostaoliveira]

Good catch on all three, and fair pushback on my own hedging:

Comment collapse: agreed, and slightly embarrassed I didn't notice the inconsistency with the three siblings myself. Collapsed to one line.

Swallowed exception: yes — lifted the connection/monitor helpers into tests/include.py (get_redis_conn_for_node(), capture_monitor()), and capture_monitor() now takes an optional errors list instead of a bare except: pass, surfaced in the assertion message on failure. Both new tests now share one ~40-line helper instead of duplicating ~85 lines each.

"CLIENT_NO-TOUCH" typo: fixed.

Hard error vs. warning for --retry-on-error: thought about this properly rather than just picking one. Landed on keeping the warning — the gap is conditional (only replayed requests racing an actual reconnect are affected) and its blast radius is per-key recency, not run correctness, which reads closer to this project's existing "has no effect" warnings than to the hard errors reserved for combinations that can't produce a meaningful result at all. A hard error would block the combination entirely until #527 lands, which felt disproportionate. Wrote up the reasoning in the PR description; happy to hear if you'd weigh it differently.

All pushed.

[fcostaoliveira]

Thanks for actually reading the CI yaml rather than trusting my sixth-round claim — you were right and I wasn't:

CI matrix gap: confirmed, every OSS-CLUSTER + replicas cell pins TEST: to a specific test_read_preference_*.py file, so my file was dead weight regardless of #462. Added "OSS-CLUSTER + replicas: client-no-touch" as its own cell.

Man-page regen: good catch, and I should have thought of this myself given round 4 was literally me trimming that same note out of --help. Added a short line back (no issue number this time, so it stays terse and doesn't reopen the length objection) — the fuller version with the #527 link stays in the man page.

get_redis_conn_for_node() docstring: went with the actual fold-over rather than just fixing the wording — test_mget_protocol.py now uses the shared helpers for real, all 7 of its tests still pass.

--cluster-mode duplicate: fixed, dropped from the test's explicit args.

All pushed.

[fcostaoliveira]

Both real ones fixed:

get_cluster_replica_connections() TLS gap: no good reason, just missed it — round 7's consolidation covered the call site I was actively touching (the non-seed-primary test) but not the pre-existing one this replica test also happens to depend on. Routed it through get_redis_conn_for_node() too (now takes **extra_kwargs to keep decode_responses=True/socket_connect_timeout=5 for this call site), verified against a real cluster+replica.

Man page / usage() drift: yeah, I fixed the symptom in round 8 (kept a note surviving regen) without checking the two actually matched. Same string now.

config_print() ordering: noted, appreciate it not being a blocking ask.

Warning-vs-hard-error human sign-off: agreed, that's the one call in this PR that deserves an explicit human decision rather than my own reasoning (however sound) plus automated-review approval. Flagging in the PR description for whoever does final review.

All pushed.

[fcostaoliveira]

All four, in order.

• --help/man-page pointer: you're right, that's not the best home for it — no context for the issue number, and it goes stale silently. Moved it into a new README "Known limitations" section instead (written generally, same "not NO-TOUCH-specific" framing as before), and reverted --help/the man page back to the plain description. --client-no-touch's entry is back to the same length as its siblings.
• capture_monitor/get_redis_conn_for_node vs test_mget_protocol.py's originals: kept them separate — the round-12 scope-discipline reasoning (this is a new-flag PR, not a test-infra-DRY PR) still applies, and I'd rather not fold in a working, independently-owned test file on the same pass a reviewer is trying to evaluate the feature itself. But the trap you flagged is real, so I documented it: get_redis_conn_for_node() now says explicitly that it has no unix-socket branch unlike _get_redis_conn(), so whoever reaches for it next for a standalone test knows to check first.
• Missing reconnect-resend test: no good reason, just hadn't gotten to it — added test_client_no_touch_resent_after_reconnect. It kills the live memtier connection via CLIENT KILL mid-run and asserts CLIENT NO-TOUCH ON shows up on MONITOR a second time once memtier reconnects, same "was it re-sent" framing you suggested. Ran it locally a few times to make sure the timing (2s before kill, 7s total test-time, 1s flat reconnect backoff) isn't flaky — clean pass each time, connection error → reconnect → resumed traffic all visible in the debug log.
• RLTest-fork-pin scope: I'd rather keep the hard-fail in this PR. It's scoped to this PR's own new test file, not any pre-existing one, so it isn't "flipping" anything that existed before — and making it skip-tolerant would just reproduce the exact silent-coverage-gap failure mode the dedicated CI cell exists to catch, which was the whole reason it's a hard-fail in the first place (round 15/16 of this thread). Agreed the SHA pin from round 20 is the right mitigation for the "individual's fork gating CI" risk itself. Open to a maintainer overruling this, per your note.

Rebuilt clean, format-check clean, test_client_no_touch.py re-verified locally at 4/4 including the new test.

[fcostaoliveira]

Appreciate the pattern-recognition on item 1 and 4 — you're right that those have been going in circles across rounds without new information each time, which is exactly the signal to stop self-adjudicating. Addressing in order.

1. Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 disclosure: fixed the stale description line (still said --help/man page after round 21 moved it to README). Beyond that, I've added an explicit "Open questions for a maintainer" section to the PR description laying out the options tried and the tradeoff, rather than making a fifth call on my own. Genuinely don't have a confident answer between "quiet" (README) and "in your face" (issue number in --help) — that's a real product decision, not something I should keep resolving unilaterally.

2. peer_client_has_any_setup_in_progress() comment: agreed, "deliberately" oversold it. Reworded to say it's the safer of two options tried, not settled — and filed Investigate whether peer_client_has_any_setup_in_progress() gating on is_ready_for_reads() (not is_conn_setup_done()) can livelock read routing #529 to track the actual investigation (connection_stage_should_abort() + reconnect/read-preference interaction) so it doesn't get lost the same way Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 almost did before being written down. is_conn_setup_done()'s vestigial const: still leaving it, still harmless, still not worth a revert.

3. Server-rejection UX: added "(this command requires Redis 7.2+)" as a trailing reminder on the error message — trailing rather than leading, since the ACL-denial test in this same PR fails with this exact message on a fully current server, so it can't claim to diagnose the cause, just remind of the version floor. On "did you check the default path end to end": I hadn't, so I did — ran the ACL-denial scenario manually with no --connection-stage-timeout override (real default: 30s). Confirmed: aborts at 30.046s wall-clock, exit code 2, "aborting after 30 seconds of connection-stage failures (last error: CLIENT NO-TOUCH failed: ...)". No hang — same mechanism the committed test proves at the accelerated 3s, just slower. Didn't add a second, 30s-slower committed test for that since it wouldn't prove anything the fast one doesn't already.

4. RLTest fork dependency: added to the same "Open questions" section rather than resolving it myself again. My position (hard-fail is correct, stated in round 21) hasn't changed, but you're right that "CI depends on a fork I control" is the project's call to make, not mine to settle by re-explaining it a third time.

Rebuilt clean, format-check clean, test_client_no_touch.py re-verified locally (4/4).

[fcostaoliveira]

Good round, one real correctness bug in there (item 4). Taking them in order.

1. peer_client_has_any_setup_in_progress() / is_ready_for_reads() comments: agreed and done — dropped the Investigate whether peer_client_has_any_setup_in_progress() gating on is_ready_for_reads() (not is_conn_setup_done()) can livelock read routing #529 pointer and "safer of two options tried" framing from the code comment; Investigate whether peer_client_has_any_setup_in_progress() gating on is_ready_for_reads() (not is_conn_setup_done()) can livelock read routing #529 is the right home for that, not this function. Left the is_ready_for_reads() comment alone, since you flagged that one as the genuinely useful part.
2. 29 rounds of commits — will this squash? Checked, and this repo's own history already answers it: PR feat: --read-preference (cluster + standalone read-endpoint routing) #456 (the read-preference PR, also went through ~100 review-round commits) is ee757e7 on master today — a single squashed commit. Same practice applies here; the round-by-round history is scaffolding for the review, not what lands.
3. get_redis_conn_for_node() / test_mget_protocol.py's _get_redis_conn(): you're right the smaller version is worth doing. Added the missing env.isUnixSocket() branch to the shared helper and made test_mget_protocol.py's version delegate to it instead of carrying its own copy (dropped the now-dead redis/TLS_* imports it no longer needs directly). Verified: test_mget_protocol.py still 7/7.
4. The kill loop in test_client_no_touch_resent_after_reconnect: real bug, thanks for catching it — in a shared env it absolutely could have killed an idle connection from an earlier test in the same file. Replaced the cmd/flags heuristic with a pre-run client-id snapshot; only ids that show up after the snapshot (i.e. memtier's) get killed now. Re-verified locally, 4/4.
5. README rewrite mixed into a connection-setup PR: fair, noted, not re-splitting at this point as you said.
6. Truncation risk in the version-hint message: you're right that it matters more than I weighted it — moved the hint to lead instead of trail, so it survives regardless of how long the server's status text is. Still phrased as a reminder ((this command requires Redis 7.2+): %s), not a diagnosis, for the same ACL-denial reason as before.

On your answer to the #527 disclosure question: took it — moved "Known limitations" down next to the other cluster/testing caveats instead of being the first thing under "Using memtier_benchmark", and dropped that item from the PR description's open-questions section since there's now a clear recommendation applied rather than an unresolved oscillation. Left the RLTest-fork-dependency question in place, since you were explicit that one isn't yours to decide either.

Rebuilt clean, format-check clean.

[fcostaoliveira]

Thanks for closing out the RLTest question — taking your verdict (keep the hard-fail, the SHA pin covers the real risk, it's already scoped to the one cell that opts in) and dropped the now-resolved "Open questions" section from the description.

• Placement: no reason it had to precede READONLY, you're right — moved CLIENT NO-TOUCH to after READONLY, before CLUSTER SLOTS, so AUTH/SELECT/HELLO/READONLY keep the exact relative order they've always had and this is a pure append. Since this changes wire order on real replica connections specifically (primaries were unaffected either way — READONLY's a no-op branch for them), re-verified against the dedicated OSS_CLUSTER_REPLICAS=1 cell locally (2/2) rather than just trusting the "no dependency" reasoning.
• Error message wording: fair, it was asserting a cause it can't know. Changed to "(may require Redis 7.2+ or be denied by ACL)" — still leading rather than trailing (per round 23's truncation point), just not claiming a specific diagnosis anymore.
• Generic setup-command option: already tracked in Consider a generic --connection-setup-command flag instead of per-command booleans #528 from round 18 (CLIENT SETINFO/SETNAME/CLIENT REPLY) — added CLIENT NO-EVICT there as another example from this round. Agree it's a real question, and agree with your own counter-argument for why a fixed boolean per command might still be right (reply-validation/failure semantics get messier for a generic version) — that's exactly the tension Consider a generic --connection-setup-command flag instead of per-command booleans #528 is there to hold.

Rebuilt clean, format-check clean, re-verified test_client_no_touch.py (4/4) and the dedicated replica cell (2/2) given the wire-order change.

[fcostaoliveira]

• RLTest fork: mirroring it under an org this project controls is a real option, but it's infrastructure this PR doesn't own the keys to — leaving that to whoever does, consistent with round 24's verdict that the hard-fail itself is right and scoped correctly. Did take the actionable half of this: the failure message now leads with "test-harness problem, not a memtier bug" and says explicitly that the test never got as far as sending CLIENT NO-TOUCH, so whoever hits this in CI doesn't have to reconstruct that from "expected at least one replica connection" on their own.
• test harness: read-preference CI cell silently skips all tests #462 reading: confirmed correct — yes, OSS-CLUSTER + replicas: read-preference has been going green while skipping the replica path since the fork started producing visible replicas. That's exactly what the comment I posted on test harness: read-preference CI cell silently skips all tests #462 in round 18 documents; nothing new to add there.
• _capture_monitor() DRY: same call as round 23's _get_redis_conn() — did it. _start_monitor() now targets include.py's capture_monitor() directly (one call site, compatible signature — the extra errors param just defaults to None, same silent-swallow behavior as before) instead of carrying its own copy. Verified test_mget_protocol.py still 7/7.
• "Two unrelated stories" nitpick: understood, appreciate you not pushing on it at round 25 of an already-long thread.

Rebuilt clean (Python-only this round), re-verified test_mget_protocol.py (7/7) and the dedicated replica cell (2/2).

[fcostaoliveira]

1. README ladder order stale: confirmed, fixed. It was exactly the leftover you guessed — round 24 reordered the code, I updated the paragraph's prose earlier in that round but missed this specific parenthetical list. Now says (AUTH, SELECT, HELLO, READONLY, CLIENT NO-TOUCH), matching send_conn_setup_commands().

2. Splitting the test-infra changes into their own PR: I hear the ask, and I don't disagree with the principle — but 26 rounds in, actually splitting now means extracting specific hunks across many commits into a fresh branch and re-running review from scratch on a piece that's already been checked this thoroughly here. I'd rather leave it bundled at this point than pay that cost for a cleanliness improvement, especially since the new test_client_no_touch_cluster.py genuinely needs the shared helpers it introduces. Not pushing back on the principle for next time, just on redoing it now.

3. RLTest fork availability: this one changed my answer. I went and checked rather than repeating the previous rounds' framing — pip install -r tests/test_requirements.txt runs inside every single matrix-cell job across ci.yml/asan.yml/tsan.yml/ubsan.yml, not a shared setup step. So you're right that a deleted/private fork breaks every Python-test cell at dependency-install time, not just the cluster one. That also means the skip-vs-hard-fail question several rounds have circled is actually moot for this specific risk: pip install failing happens before any test even collects, so it doesn't matter whether the downstream test would have skipped or hard-failed on missing replicas — the job dies either way, earlier. Filed All CI test cells depend on a personal fork's availability (tests/test_requirements.txt -> fcostaoliveira/RLTest) #530 with that finding. I don't have the access to actually mirror the fork under the org, so it's tracked rather than fixed here, same as the "mirror it" idea from round 25.

4. Kill-window question: also worth a correction on my part — I went and checked instead of assuming, and each test function gets a completely fresh redis-server instance (confirmed via CI/local logs: distinct per-test logfile/dbfilename hash, one "Redis master command" per test). So the specific scenario my round-23 commit message described — killing a connection left idle by an earlier test in the same file — was never actually possible; each test's env is isolated. The snapshot-based fix is still correct and cheap, just not defending against the thing I said it was. In this test's actual 2-second window, nothing besides memtier is plausibly connecting.

Rebuilt (README-only this round).

[fcostaoliveira]

Item 1 was a real bug, thanks for tracing it that precisely.

1. env_started_with_slaves() false positive: confirmed exactly as you described, and reproduced it locally before fixing — OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 (this file's own second docstring example) did trip the hard-fail branch during the plain-cluster pass. Switched to checking sys.argv for --use-slaves instead of the environment variable: run_tests.sh's run_tests() spawns a fresh python3 -m RLTest subprocess per invocation, so sys.argv is naturally scoped per-run in a way the shell-level env var isn't, and it still avoids introspecting RLTest's internal envRunner attribute shape (the original reason round 17 moved off that). Re-ran the exact dual-invocation scenario after the fix: plain pass now skips cleanly, replicas pass passes (MONITOR showing READONLY then CLIENT NO-TOUCH ON, matching round 24's reorder).
2. RLTest fork red-vs-skip: this has a clear paper trail at this point — round 24 said keep the hard-fail (correctly scoped, SHA pin covers the real risk), round 26 found and filed the more precise availability risk as All CI test cells depend on a personal fork's availability (tests/test_requirements.txt -> fcostaoliveira/RLTest) #530 (which also settles that skip-vs-hardfail is moot for that specific risk, since pip install fails before test collection either way). Not re-litigating further absent new information.
3. Generic setup-command hook: still Consider a generic --connection-setup-command flag instead of per-command booleans #528 — added CLIENT TRACKING to the list of examples there this round.
4. --show-config/mb.json coverage gap: real gap, added test_client_no_touch_show_config in the same shape as test_cli_validation_prometheus.py's V17 test. One correction while writing it: config_print() uses yes/no for booleans (matching every sibling field — transaction, retry_on_error, print_all_runs, etc.), not true/false — that's config_print_to_json()'s convention, which I asserted separately against mb.json.

Verified locally: test_cli_validation_client_no_touch.py 4/4, plus the reproduced-then-fixed dual-invocation scenario for item 1.

[fcostaoliveira]

Biggest round in a while — taking each point.

• Split the RLTest pin out: agreed this specific piece is small and independent enough to actually do, unlike the broader test-infra bundle (which I'm still not re-splitting, per round 23/26). Opened ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 off master with just the branch→SHA change, and reverted this branch's tests/test_requirements.txt to match master exactly — it'll pick the pin back up on the next merge from master once ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 lands.
• Replica-test gate "fails open": you're right that inferring from sys.argv/env var is the wrong shape for a gate whose whole point is to not silently skip. Went with your suggestion directly: run_tests.sh's OSS_CLUSTER_REPLICAS=1 block now sets MEMTIER_CLUSTER_REPLICAS_EXPECTED=1 inside its own subshell only (not globally, unlike OSS_CLUSTER_REPLICAS itself), and the test hard-requires that dedicated signal instead. This also happens to be the actual root-cause fix for round 27's false-positive, not just a workaround — the underlying problem there was OSS_CLUSTER_REPLICAS being scoped to the whole shell process instead of the specific invocation that passes --use-slaves. Re-verified the OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 scenario end-to-end again: still correct.
• Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527 window widening: good catch, worth having on the issue where the next person debugging it will look rather than buried in this thread — added a comment on Replayed requests can race ahead of the connection-setup ladder (AUTH/SELECT/HELLO/READONLY) after a reconnect #527.
• Generic setup-command hook: still Consider a generic --connection-setup-command flag instead of per-command booleans #528, nothing new to add this round.
• --show-config/mb.json coverage: that was last round, already closed.
• MONITOR-registration race: real, and a good one to catch — fixed with an active poll (checking CLIENT LIST for the MONITOR flag) instead of a fixed 0.15s sleep, so it can't fire before conn.monitor()'s lazily-connecting socket actually registers on a loaded runner.
• Sanitizer cell cost: checked a completed ASAN OSS-CLUSTER API: TCP Plaintext run — both tests' cluster boot+run+teardown together cost about 15-20 seconds inside a ~13-minute job that also runs every other OSS-CLUSTER test file. Not something driving that cell's duration.
• Comment/changelog-style nitpick: trimmed env_started_with_slaves()'s docstring and the rt_no_touch snprintf comment back to the technical why, dropped the "here's what an earlier version got wrong" framing.
• README section placement: moved "Known limitations" out from between "Testing limitations" and "Using rate-limiting" — it's now its own top-level section, not nested inside Read Preference / cluster-mode material a --select-db-under---retry-on-error reader has no reason to open.

Rebuilt clean, format-check clean. Verified locally: test_client_no_touch.py 4/4 with the polling fix, and the OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 dual-invocation scenario end-to-end with the new dedicated env var (plain pass skips, replicas pass 2/2).

[fcostaoliveira]

The bufferevent_write question deserved real investigation rather than a reflex defense, so I went and did it.

Empirically: attached redis-cli MONITOR to a non-seed shard in a local 3-node cluster and ran memtier --cluster-mode against the bootstrap seed only, no special flags. Real SET/GET traffic flowed on the non-seed connection — which can only happen if m_hello reached setup_done there, meaning HELLO's plain-evbuffer_add write (the exact path your comment says shouldn't flush on this connection type) did get sent and acknowledged. (MONITOR itself doesn't show HELLO — Redis filters it like AUTH — so this is inferred from user traffic flowing, not observed directly, but the inference is solid: nothing gets past the ladder without it.)

Also checked: bufferevent_enable(m_bev, EV_READ | EV_WRITE) at BEV_EVENT_CONNECTED predates the original READONLY fix entirely — it's present in the commit before PR #456 merged. And that same original fix commit (0f53dc2) also fixed an unrelated fill_pipeline spin that could starve the event loop from ever firing BEV_EVENT_CONNECTED for the replica connection in the first place. That's a real alternate explanation for what was observed — if the replica's connect callback never fired, READONLY's bytes never got a chance to go out regardless of which write path was used, and fixing the spin alone would have resolved the symptom.

I don't think I should resolve this unilaterally in a feature PR — filed #532 for someone to actually isolate the two changes and confirm which one was load-bearing. bufferevent_write is safe either way (identical wire bytes), so nothing here needs to block on that answer; I reworded the CLIENT NO-TOUCH comment to say it follows READONLY's precedent rather than re-asserting the same causal claim as settled fact, and points at #532.

• PR description: fixed — it claimed this branch carries the SHA pin, but that's ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 now, landing separately.
• benchmark_error_log NULL-arg: leaving as-is, matches the sibling AUTH/READONLY style exactly — fixing it only for NO-TOUCH would be a new inconsistency, not a fix.
• env_started_with_slaves() redundancy: it's down to one real consumer now (the defensive stderr warning in get_cluster_replica_connections()), but that's still a genuine, if lower-stakes, use — not currently expected to fire, but useful if the fork ever does regress in a way that produces some-but-wrong replica state. Keeping it.

Rebuilt clean (comment-only C++ change), re-verified test_client_no_touch.py (4/4).

[fcostaoliveira]

• ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 ordering: agreed on the principle, but I don't think I should be the one to close this loop by merging it — branch protection requires a review, and self-approving my own PR isn't a call to make unilaterally even though I have admin on the repo. ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 is open, green, and ready; it needs an actual maintainer to look at and merge it. That's a real action for a human at this point, not something another review round can settle.
• env_started_with_slaves() vs MEMTIER_CLUSTER_REPLICAS_EXPECTED: you're right, now that the env var is scoped correctly per-invocation there's no reason to keep two answers to the same question. Removed env_started_with_slaves() entirely; get_cluster_replica_connections()'s warning path reads MEMTIER_CLUSTER_REPLICAS_EXPECTED directly now. Re-verified the dual-invocation scenario once more after the removal — still correct.
• Contradicting comments in peer_client_has_any_setup_in_progress(): real bug in a pre-existing comment, thanks for catching it — is_ready_for_reads() never checks m_hello, so the comment three lines up listing "HELLO" among what gates it was simply wrong. Fixed it instead of leaving the two disagreeing.
• Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532 told three ways: fair, and this was on me — the PR description and the test's docstring both still asserted the original diagnosis as settled fact after I'd already hedged the code comment in round 29. Reworded both to match: follows READONLY's precedent, whether that precedent holds is open and tracked in Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532 (and empirically doubtful, per round 29's finding), bufferevent_write is safe regardless of the answer.
• Generic setup-command hook: still Consider a generic --connection-setup-command flag instead of per-command booleans #528.
• Test-infra scope nitpick: same answer as round 23/26/28 — not re-splitting further at this point, though I hear it every round.

Rebuilt clean, format-check clean, re-verified the OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 scenario end-to-end once more (plain pass skips, replicas pass 2/2) after removing env_started_with_slaves().

[fcostaoliveira]

Three real fixes, one more restated.

• ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 ordering: nothing new to add — still needs a maintainer to actually merge it, which I've flagged directly outside this thread too.
• Fixed pre-kill sleep: exactly the same class of issue as the MONITOR-registration one last round, and you're right it's the tighter budget of the two given where this file gets collected. Replaced the fixed 2.0s sleep with a poll on the results list for the first CLIENT NO-TOUCH ON to actually show up (bounded, 15s) — ties the wait to the condition the test actually needs instead of guessing at connect+ladder timing.
• Redundant import os: removed — you're right, os is already module-scope, that one wasn't in the same boat as sys/redis.
• Stale caller list in get_redis_conn_for_node()'s docstring: dropped the enumeration, kept the TLS-aware/extra_kwargs-precedence part that's actually load-bearing.
• NO-TOUCH comment volume: trimmed to what you asked for last round — the ordering-consistency sentence plus the one Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532 sentence, nothing else.

Verified locally: test_client_no_touch.py 4/4 with the poll-based kill wait, dedicated replica cell 2/2 (confirms the import os cleanup didn't touch the warning path's behavior).

[fcostaoliveira]

All four, and a bug I caught while doing item 1.

1. Lift the MONITOR-poll into include.py: agreed, done — client_list()/wait_for_monitor_registered() now live in include.py, and test_client_no_touch_cluster.py's _run_and_check_no_touch() uses the poll instead of time.sleep(0.15). Needed a control connection to the specific node being monitored (CLIENT LIST is per-server, and conn is busy in the monitor thread) — my first pass built that via redis.Redis(**conn.connection_pool.connection_kwargs), which throws TypeError: unexpected keyword argument 'himport_registry' (redis-py's ConnectionPool.connection_kwargs carries internal bookkeeping, not just constructor args). Caught it locally re-running the OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 scenario before pushing — fixed by pulling just host/port and rebuilding through get_redis_conn_for_node(), same as everywhere else in this file.
2. ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 ordering: nothing new — still needs a maintainer to merge it.
3. OR the two "replicas expected" signals: you're right, round 30 threw out the argv fallback entirely rather than OR-ing it, which is exactly the regression you're describing for a direct RLTest invocation. Added replicas_expected() in include.py (ORs MEMTIER_CLUSTER_REPLICAS_EXPECTED with the sys.argv check) and pointed both get_cluster_replica_connections()'s warning and the cluster test's hard-fail gate at the same shared function instead of each reimplementing the OR.
4. Older-server clause: added to both --help and the man page.
5. README/test harness: read-preference CI cell silently skips all tests #462 scope: same answer as before, not re-splitting.

Verified locally: test_client_no_touch.py 4/4, and — after finding and fixing the connection_kwargs bug — the OSS_CLUSTER=1 OSS_CLUSTER_REPLICAS=1 scenario end-to-end (plain pass 2/2, replicas pass 2/2). Rebuilt clean, format-check clean.

[fcostaoliveira]

• ci: pin RLTest fork dependency to a commit SHA instead of a branch name #531 ordering: same answer — needs a maintainer to merge it, not another round.
• Second literal RESP string / write_command_no_touch(): good catch, and it connects to something I should have surfaced earlier — redis_protocol::write_command_readonly() (protocol.cpp) already exists as dead code today. It does a plain evbuffer_add(), and send_conn_setup_commands() bypasses it entirely with the inline READONLY_CMD + bufferevent_write(), for the exact reason NO_TOUCH_CMD now does too. So this isn't just "is bufferevent_write necessary" — it's "why do AUTH/SELECT/HELLO/CLUSTER SLOTS live in protocol.cpp while READONLY and CLIENT NO-TOUCH live inline in shard_connection.cpp, with an unreachable same-named method sitting unused." I don't think I should resolve this myself right now: if bufferevent_write turns out to be unnecessary (per Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532), the fix is straightforward — wire both through proper protocol.cpp methods and delete the inline literals. If it's necessary, giving abstract_protocol bufferevent access isn't a small change (protocol.h currently has zero libevent/bufferevent dependency — it only sees evbuffers via set_buffers(), a real architectural boundary). Posted this on Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong #532 so whoever resolves the EPOLLOUT question settles both at once, since the right structure depends on the answer.
• README "Known limitations" scope: same answer as prior rounds, not splitting further right now.
• new_ids kill-scope in the reconnect test: I don't think this one actually applies here, though I understand the caution given the file's CI history — test_client_no_touch.py is standalone-only, and round 26 confirmed empirically that each test function gets a completely fresh redis-server instance (distinct logfile/dbfilename per test, one "Redis master command" per test in the CI logs). There's no shared env within a single test's execution for anything else to be connecting during that window — the pre-run snapshot already includes master_connection and monitor_conn (both established before it's taken), so anything that shows up after is memtier's, by construction, not by luck. Filtering on laddr/name would be defense against a scenario this test's actual server-lifecycle model doesn't have.
• 40 commits, squash on merge: answered in round 30 — this repo already has direct precedent (PR feat: --read-preference (cluster + standalone read-endpoint routing) #456, also ~100 review-round commits, landed as one squashed commit ee757e7 on master).

No code changes this round — filed the write_command_no_touch() question on #532 instead of resolving it unilaterally, and the rest are answers rather than fixes.