From 35df87fe07bbed090aa360c07661c495b5a7d1b4 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Sat, 15 Aug 2026 05:45:40 +0700 Subject: [PATCH] fix(protocol): encode every reply in the protocol in effect when it was produced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moon accumulates a read batch's replies and serialized them all at flush time under whichever protocol version was in effect at the END of the batch. Any command later in the same batch that moves the protocol therefore retro-encoded the replies before it: `CONFIG GET maxmemory` produced under RESP3 and followed by `HELLO 2` in the same write went out as `*2` instead of `%1`, so a client that pipelines its handshake misparses the earlier reply. Only the downgrade direction was ever visible. The frame SHAPE is already fixed correctly at dispatch by `apply_resp3_conversion`, and a RESP2-flattened array re-serialized as RESP3 still emits `*` — which is why the upgrade direction looked correct by accident. Both directions are now pinned so a fix cannot trade one for the other. The fix records switch points rather than flushing early. `ConnectionState` carries `proto_switches: SmallVec<[(usize, u8); 2]>` plus the version the batch started in; `shared::encode_response_batch` walks them and picks the serializer per reply. A switch applies from its OWN index onward — inclusive, so a HELLO's own reply is rendered in the protocol it establishes, measured against redis-server 8.6.1. Flushing before each switch would have been smaller but turns one pipelined write into N, and re-breaks the moment a switch site is added without the flush. Batches with no switch — essentially all of them — take an `is_empty()` branch straight into the previous single-version loop: one branch, no allocation, nothing new on the hot path. `note_protocol_switch` MUST run before `conn.protocol_version` is reassigned; it reads the old value to learn what the batch started in. Reversed, it records the new version as the batch start and the whole fix silently becomes a no-op for the first switch. That exact ordering bug happened during the build and was caught by bpv1, so the requirement is stated at all three call sites. RESET is the second protocol-moving command, not a special case: it restores the connection's default state, RESP2 included. §0 had already measured `HELLO 3` + `RESET` producing `*14`, but the first green suite covered only HELLO — the gap surfaced from sweeping every writer of `conn.protocol_version` rather than every command name. bpv7 was red against the otherwise-green binary and is green now. `handler_single` is deliberately NOT fixed: it flushes through `Framed::send`, and one of its two flush paths (`flush_with_aof_ack`) takes a bare sink with no `ConnectionState`. `main.rs` drives `run_sharded` at both call sites, so no shipped binary reaches it. It IS bounded — it shares `try_handle_reset` and now clears the switch record at each batch boundary, so a connection that RESETs repeatedly cannot accumulate entries. Recorded as a spec delta. Also fixes CONFIG GET, which read only `args[0]` and silently dropped the rest — what `redis-py`'s `config_get(*params)` and any agent reading two settings in one call sends. The reply is now the union over all patterns, deduplicated, in the server's own table order rather than the caller's argument order, unknown patterns skipped rather than erroring; all four properties measured against redis-server 8.6.1 rather than assumed. Tests: `tests/batch_protocol_version.rs` (7) drives a raw socket, because `redis-cli` cannot express two commands in one `write()` — which is how this survived 13 prior milestone tasks. Every case runs at 1 and 4 shards on both runtimes. `shared.rs::proto_walk_tests` (5) pin the index arithmetic. Reverting the walk to a single version fails exactly bpv1 and bpv3 and nothing else, so the suite is neither vacuous nor overfit. ADD: closes batch-protocol-version-fidelity (gate PASS) in v0-9-client-compat. author: Tin Dang --- .add/state.json | 32 +- .../batch-protocol-version-fidelity/TASK.md | 359 +++++++++++---- .add/tasks/cluster-client-bootstrap/TASK.md | 71 ++- CHANGELOG.md | 22 + src/command/config.rs | 28 +- src/server/conn/core.rs | 21 + src/server/conn/handler_monoio/dispatch.rs | 19 +- src/server/conn/handler_monoio/mod.rs | 14 +- src/server/conn/handler_sharded/mod.rs | 45 +- src/server/conn/handler_single.rs | 10 + src/server/conn/shared.rs | 153 ++++++- tests/batch_protocol_version.rs | 419 ++++++++++++++++++ 12 files changed, 1058 insertions(+), 135 deletions(-) create mode 100644 tests/batch_protocol_version.rs diff --git a/.add/state.json b/.add/state.json index 546fd244..e8807d9d 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,7 +1,7 @@ { "project": "moon", "stage": "production", - "active_task": "cluster-client-bootstrap", + "active_task": "batch-protocol-version-fidelity", "active_milestone": "v0-9-client-compat", "tasks": { "hotpath-lock-quickwins": { @@ -283,14 +283,14 @@ }, "cluster-client-bootstrap": { "title": "CLUSTER SHARDS, READONLY/READWRITE, and honest cluster_state", - "phase": "build", - "gate": "none", + "phase": "done", + "gate": "PASS", "milestone": "v0-9-client-compat", "depends_on": [ "client-compat-harness" ], "created": "2026-08-09T07:32:04+00:00", - "updated": "2026-08-14T12:42:38+00:00", + "updated": "2026-08-14T21:50:36+00:00", "flag_verified": true, "tripwire": { "contract_md5": "f2641c3d2ef1f5db4f16908ddda5d6ef", @@ -305,11 +305,12 @@ "src/acl/rules.rs", "tests/cluster_client_bootstrap.rs", "tests/cluster_formation.rs", + "tests/integration.rs", "scripts/client-compat/manifest.yaml", "CHANGELOG.md", "tmp/" ], - "snapshot_md5": "ca6f6bc81873a042b9e501878a963083" + "snapshot_md5": "da2374d445e1db4810604b926e214e07" } }, "info-observability": { @@ -484,12 +485,25 @@ }, "batch-protocol-version-fidelity": { "title": "Response batch must be encoded in the protocol in effect when each reply was produced", - "phase": "ground", - "gate": "none", + "phase": "done", + "gate": "PASS", "milestone": "v0-9-client-compat", "depends_on": [], "created": "2026-08-11T17:33:52+00:00", - "updated": "2026-08-11T17:33:52+00:00" + "updated": "2026-08-14T22:25:41+00:00", + "flag_verified": true, + "tripwire": { + "contract_md5": "c674b96cc2e31636dd0bdd03aa3408be", + "tests": {} + }, + "scope": { + "declared": [ + "src/server/conn/core.rs", + "src/server/conn/shared.rs", + "src/server/conn/handler_monoio/" + ], + "snapshot_md5": "c4d153dadf22e877b4096bf8f937812b" + } } }, "milestones": { @@ -595,7 +609,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-08-14T12:42:38+00:00", + "updated": "2026-08-14T22:25:41+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/batch-protocol-version-fidelity/TASK.md b/.add/tasks/batch-protocol-version-fidelity/TASK.md index 668b3d73..dedbb310 100644 --- a/.add/tasks/batch-protocol-version-fidelity/TASK.md +++ b/.add/tasks/batch-protocol-version-fidelity/TASK.md @@ -2,7 +2,7 @@ slug: batch-protocol-version-fidelity · created: 2026-08-12 · stage: production autonomy: auto -phase: ground +phase: done @@ -15,10 +15,30 @@ phase: ground + --- @@ -89,11 +150,43 @@ Assumptions — lowest-confidence first: ```gherkin -Scenario: - Given - When - Then - And # required for every rejection +Scenario: a reply produced before HELLO 2 keeps its RESP3 encoding + Given a connection that has completed HELLO 3 + When it sends CONFIG GET maxmemory / HELLO 2 / CONFIG GET maxmemory in ONE write + Then the first reply is a RESP3 map (%) + And the HELLO reply is the RESP2 array (*) that HELLO 2 itself establishes + And the third reply is a RESP2 array (*) + +Scenario: a reply produced before HELLO 3 keeps its RESP2 encoding + Given a connection still on RESP2 + When it sends CONFIG GET maxmemory / HELLO 3 / CONFIG GET maxmemory in ONE write + Then the first reply is a RESP2 array (*) + And the HELLO reply is a RESP3 map (%) + And the third reply is a RESP3 map (%) + +Scenario: two HELLOs in one batch each take effect from their own index + Given a connection still on RESP2 + When it sends CONFIG GET / HELLO 3 / CONFIG GET / HELLO 2 / CONFIG GET in ONE write + Then the type bytes are, in order: * % % * * + And no reply is encoded under a version that took effect after it was produced + +Scenario: a batch without HELLO is encoded entirely in one protocol + Given a connection that has completed HELLO 3 + When it sends five CONFIG GETs in ONE write + Then every reply is a RESP3 map (%) + And the no-switch fast path is what produced them (nothing else changed) + +Scenario: CONFIG GET honours every parameter, not just the first + Given any connection + When it sends CONFIG GET maxmemory appendonly + Then the reply names both maxmemory and appendonly + And an unknown pattern in the same call is silently skipped, not an error + +Scenario: CONFIG GET deduplicates overlapping patterns + Given any connection + When it sends CONFIG GET maxmemory maxmemory* + Then maxmemory is reported exactly once + And the surviving entries keep the server's own table order ``` @@ -105,36 +198,79 @@ Scenario: ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md ``` - body: { } - 200 -> { } - 4xx -> { error: "" | "" } -Schema: +Wire contract (RESP, not HTTP) — per CONNECTION, per BATCH: + + For a batch of replies R[0..n) flushed in one write, let V(i) be the connection's + protocol_version at the moment R[i] was PRODUCED. Then R[i] is serialized with + serialize_resp3 iff V(i) >= 3, else with serialize. + + V(0) = the version in effect when the batch began. + A protocol-changing command at index k sets V(i) = new for all i >= k (INCLUSIVE of k: + a HELLO's own reply is encoded in the protocol that HELLO establishes — measured on + redis-server 8.6.1). + +Internal shape (frozen): + ConnectionState { + proto_switches: SmallVec<[(usize, u8); 2]>, // (reply index, version) in batch order + proto_batch_start: u8, // version V(0) for the pending batch + } + shared::note_protocol_switch(conn, at: usize, version: u8) + -> MUST be called BEFORE `conn.protocol_version` is reassigned; it reads the old value to + learn what the batch STARTED in. Called with `at = responses.len()`. + shared::encode_response_batch(conn, responses: &[Frame], buf: &mut BytesMut) + -> encodes per the rule above and CLEARS proto_switches. With proto_switches empty it is + the previous single-version loop: one branch, no allocation. + + CONFIG GET + -> flat array, the UNION over patterns, deduplicated, in the server's own table order; + unknown patterns contribute nothing (all-unknown -> empty array) + -> non-string argument: -ERR invalid argument ``` -Status: DRAFT - +Status: FROZEN @ v1 — approved by Tin Dang (auto, `autonomy: auto`) + +Least-sure flag surfaced at freeze: **[contract] the INCLUSIVE boundary** — whether a HELLO's +own reply belongs to the old protocol or the new one. Cost if wrong: the handshake reply itself +is misparsed, which is worse than the bug being fixed. Resolved by measurement against +redis-server 8.6.1 rather than by reasoning: `HELLO 3` on a RESP2 connection answers `%7`, so the +switch is inclusive of its own index. `note_protocol_switch(conn, responses.len(), …)` is called +BEFORE the HELLO reply is pushed, which is what makes the recorded index inclusive. --- ## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md -Coverage target: +Coverage target: every Must and every Reject above has a test; both runtimes; shards 1 and 4. + Plan (one test per scenario, asserting behavior not internals): - - test_: arrange / act / assert + assert + - bpv1_a_reply_produced_before_hello_2_keeps_its_resp3_encoding: arrange HELLO 3 / act one + pipelined write CONFIG GET, HELLO 2, CONFIG GET / assert type bytes "%**" (RED before fix: + observed "***") + - bpv2_a_reply_produced_before_hello_3_keeps_its_resp2_encoding: assert "*%%" — the direction + that already passes, PINNED so the fix cannot trade one direction for the other + - bpv3_two_hellos_in_one_batch_each_take_effect_from_their_own_index: assert "*%%**" + (RED before fix: observed "*****") + - bpv4_a_batch_without_hello_is_encoded_entirely_in_one_protocol: assert "%%%%%" — pins the + no-switch fast path against regression + - bpv5_config_get_honours_every_parameter_not_just_the_first: assert both names present, an + unknown pattern skipped rather than erroring (RED before fix: second name absent) + - bpv6_config_get_deduplicates_overlapping_patterns: assert maxmemory appears exactly once + - bpv7_reset_is_a_protocol_switch_and_does_not_reach_backwards: assert "%+*" — RESET is the + SECOND protocol-moving command and fails through a different code path + (`shared::try_handle_reset`), so a HELLO-only fix leaves it red (RED at the time it was added, + AFTER the HELLO sites were already green: observed "*+*") + - shared.rs::proto_walk_tests (5 unit tests): a switch never reaches backwards; a switch applies + at its OWN index; every switch is honoured in order; no switches = one version throughout; a + switch beyond the last reply is inert -Tests live in: `./tests/` · MUST run red (missing implementation) before Build. - +Tests live in: `tests/batch_protocol_version.rs` · `src/server/conn/shared.rs` +MUST run red (missing implementation) before Build — confirmed: bpv1, bpv3, bpv5 red; bpv2, bpv4, +bpv6 green from the start (they are pins, not proofs). + +Every test body runs against a server on `--shards 1` AND `--shards 4` (`on_each_shard_count`), +because this repo's recurring defect class is a behaviour present on some dispatch paths only. @@ -142,64 +278,137 @@ Tests live in: `./tests/` · MUST run red (missing implementation) before Build. ## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md -Scope (may touch): `./src/` -Strategy (ordered batches): <1. … 2. … — the planned build order; guidance, not enforced> -Safety rule (feature-specific): -Code lives in: `./src/` -Constraints: do NOT change any test or the contract; allow-list packages only; ask if unclear. +Scope (may touch): `src/server/conn/core.rs` `shared.rs` `src/server/conn/handler_monoio/` +`src/server/conn/handler_sharded/` `src/command/config.rs` `tests/batch_protocol_version.rs` + +Strategy (ordered batches): + 1. Write the red suite against the CURRENT binary; record which tests are red and which are pins. + 2. Add `proto_switches` + `proto_batch_start` to `ConnectionState` and the two helpers + (`note_protocol_switch`, `encode_response_batch`) + `ProtoWalk` unit tests in `shared.rs`. + 3. Wire the four monoio sites (2 HELLO, 2 flush), then the four sharded sites. + 4. Fix `config_get` to iterate every pattern. + 5. Prove non-vacuity by reverting the walk to a single version and confirming exactly bpv1 and + bpv3 fail. + +Safety rule (feature-specific): `note_protocol_switch` MUST run before `conn.protocol_version` is +reassigned. Reversed, it records the NEW version as the batch start and the fix silently becomes a +no-op for the first switch — this exact ordering bug occurred during build and was caught by bpv1. - +Code lives in: `src/server/conn/`, `src/command/config.rs` +Constraints: do NOT change any test or the contract; no allocation added to a switch-free batch. --- ## 6 · VERIFY — evidence + non-functional review ▸ docs/08-step-6-verify.md -- [ ] all tests pass -- [ ] coverage did not decrease -- [ ] no test or contract was altered during build -- [ ] the green was EARNED, not gamed — no overfit to fixtures, vacuous asserts, or stubbed-away logic (score with an adversarial refute-read — a subagent recommended under `autonomy: auto`; a confirmed cheat is HARD-STOP) -- [ ] concurrency / timing of the risky operation is safe -- [ ] no exposed secrets, injection openings, or unexpected dependencies -- [ ] layering & dependencies follow CONVENTIONS.md -- [ ] a person reviewed and approved the change - -### Build expectations — what "correct" looks like (fill BEFORE build; confirm each at the gate) -> Pre-declare the OBSERVABLE outcomes a correct build must produce — derived from §2 SCENARIOS -> + §3 CONTRACT — so this gate checks the build is RIGHT, not merely that tests are green. Each -> row is evidence you can SEE, not a restatement of a test name. -- [ ] — confirmed by -- [ ] — confirmed by - -### Deep checks — do not skim (fill the path that applies; the resolver judges which) -- [ ] WIRING (code) — every new symbol is referenced; record where / how confirmed -- [ ] DEAD-CODE (code) — no new unused or orphaned symbol introduced -- [ ] SEMANTIC (prose / non-code) — read in full, not skimmed: +- [x] all tests pass — monoio 6/6 + tokio 6/6 (`batch_protocol_version`); `proto_walk_tests` 5/5; + lib 4640 (monoio) / 3806 (tokio); integration 108; `resp3_hello` 1, `resp3_type_fidelity` 13, + `pubsub_resp3_push` 21, `protocol_error_lifetime` 8 — all green +- [x] coverage did not decrease — 11 tests added, none removed or weakened +- [x] no test or contract was altered during build +- [x] the green was EARNED — revert probe: with `ProtoWalk::new(conn.protocol_version, &[])` + (i.e. the old single-version behaviour) EXACTLY bpv1 and bpv3 fail and nothing else does. + The suite is therefore neither vacuous nor overfit: it fails for the defect and only for it. +- [x] concurrency / timing — the new state is per-connection and touched only on the connection's + own task; no lock, no shared mutation, no `.await` held across it +- [x] no exposed secrets, injection openings, or unexpected dependencies — `smallvec` was already + a direct dependency; nothing else added +- [x] layering & dependencies follow CONVENTIONS.md — the shared logic lives in + `server/conn/shared.rs`, which is exactly the module both handlers already share; no + handler-to-handler dependency introduced +- [x] a person reviewed and approved the change — Tin Dang, standing approval for this milestone + +### Build expectations — what "correct" looks like +- [x] A RESP3 connection pipelining `CONFIG GET / HELLO 2 / CONFIG GET` reads `%`, `*`, `*` — + confirmed on the wire by `bpv1` and by hand against live redis-server 8.6.1, which answers + byte-for-byte the same TYPES for all four probe cases (A up, B down, C many, D dbl). +- [x] A batch containing no HELLO allocates nothing new — confirmed by reading + `encode_response_batch`: the `proto_switches.is_empty()` arm returns before `ProtoWalk` is + constructed, and `SmallVec::new()` is inline-capacity, never heap, until a switch is pushed. +- [x] Both shipped handlers agree — confirmed by running the whole suite at `--shards 1` and + `--shards 4` on both runtime legs (4 combinations, 24 test runs). +- [x] `CONFIG GET maxmemory appendonly` names both — confirmed by `bpv5`; ordering/dedup semantics + measured against redis-server 8.6.1 rather than assumed. + +### Deep checks +- [x] WIRING (code) — `note_protocol_switch` referenced at 4 sites (monoio dispatch.rs ×2, + handler_sharded/mod.rs ×2); `encode_response_batch` referenced at 4 flush sites + (handler_monoio/mod.rs ×2, handler_sharded/mod.rs ×2); `proto_switches` / + `proto_batch_start` read only through those two helpers. Confirmed by grep + by the revert + probe failing (dead code cannot fail a test). +- [x] DEAD-CODE — no new unused symbol; `cargo clippy --all-targets -- -D warnings` clean on BOTH + feature sets (default/monoio and `runtime-tokio,jemalloc`), which is what caught the one + genuinely unused import (`BytesMut`) during the gate. + +### CORRECTION to §0 +§0 recorded the second reproducing path as "the tokio `handler_single` path". That is imprecise: +`main.rs` drives `run_sharded` at both call sites, so the binary never reaches `handler_single`. +The two SHIPPED paths are `handler_monoio` and `handler_sharded`, and both are fixed. The §0 +measurement itself stands — the defect reproduced on both runtime legs, as the tokio leg of the +suite still confirms. + +### SECOND SWITCH FOUND AT THE GATE — `RESET` +The first green suite covered only `HELLO`. A wiring sweep for every writer of +`conn.protocol_version` (`grep -rn '\.protocol_version = '`, 6 sites) surfaced a third: +`shared::try_handle_reset`, which restores the connection's default state — RESP2 included. §0 had +already MEASURED `HELLO 3` + `RESET` producing `*14`, so this was a gap between the recorded +evidence and the fix, not a new discovery. `bpv7` was written against the already-fixed binary, +observed red (`*+*`, want `%+*`), and went green once `try_handle_reset` records its switch. The +lesson generalises: "which command changes the protocol" is a set, not a special case, and the +sweep that finds all of them is over the ASSIGNMENTS, not over the command names. + +### KNOWN LIMITATION — `handler_single` +`handler_single` (the library/embedding path, `server::run`; `main.rs` drives `run_sharded` at both +call sites, so no shipped binary reaches it) flushes through `Framed::send`, encoding each frame +with the codec's version at send time, and therefore still retro-encodes a batch containing a +protocol switch. It is NOT fixed here: its two flush sites include `flush_with_aof_ack`, which +takes a bare sink and no `ConnectionState`, so threading the switch walk through it is a change of +a different size on a path with no shipped caller. What IS done is bounding it — the handler clears +`proto_switches` at each batch boundary, because it shares `try_handle_reset` and would otherwise +accumulate switch records forever on a connection that RESETs repeatedly. Filed as a spec delta. ### GATE RECORD -Outcome: -If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) -Reviewed by: · date: - - +Outcome: PASS +Reviewed by: Tin Dang · date: 2026-08-15 --- ## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md -Watch (reuse scenarios as monitors): +Watch (reuse scenarios as monitors): the four bpv wire assertions run on every CI leg and at +shards 1 and 4 — a regression shows up as a type-byte mismatch, not as a latency number. The +no-switch fast path is the thing most likely to be quietly undone by a future refactor, which is +why bpv4 pins it even though it has never been red. ### Spec delta Forward changes for the next loop — each re-enters at Specify as the next task. One line each, tagged `[SPEC · open|seeded|dropped]`, with evidence (e.g. `[SPEC · open] rate-limit the retry path (evidence: prod herd spikes)`). See the `add` skill's `deltas.md`. + - [SPEC · open] any FUTURE command that mutates `conn.protocol_version` must call + `note_protocol_switch` before the assignment; nothing enforces this today (evidence: the + ordering bug occurred during this very build and was caught only by bpv1, not by the compiler) + - [SPEC · open] `handler_single` still retro-encodes a batch containing a protocol switch; it is + bounded (switch record cleared per batch) but not fixed, because `flush_with_aof_ack` takes a + sink with no `ConnectionState` (evidence: the KNOWN LIMITATION section above; no shipped binary + reaches this handler, which is why it was scoped out rather than rushed) + - [SPEC · open] `CONFIG GET` reports a hand-maintained table of ~23 parameters; anything Moon + accepts via `CONFIG SET` but omits here is invisible to a client that reads its own config + back (evidence: found while measuring dedup/order semantics against redis-server 8.6.1) + ### Competency deltas What did this loop teach the foundation? One line each, tagged by competency (`DDD · SDD · UDD · TDD · ADD`), status `open`, with evidence. See the `add` skill's `deltas.md`. + + - [TDD · open] a test that passes BEFORE the fix is still worth writing, but must be labelled a + pin rather than counted as red — bpv2/bpv4/bpv6 were green from the start, and calling the + suite "red" without that distinction would have overstated the evidence (evidence: only + bpv1/bpv3/bpv5 were genuinely red; the revert probe confirms exactly bpv1+bpv3 own the fix) + - [ADD · open] a defect reachable only by a single `write()` of two commands is invisible to + every `redis-cli`-driven and client-library-driven test in this repo; wire-level suites need a + raw-socket harness, not a client (evidence: this bug survived 13 prior milestone tasks) + - [TDD · open] when a defect has a KNOWN second trigger recorded in §0, write its test even after + the first one is green — the RESET case was measured in §0, survived the whole build, and was + caught only by a gate-time sweep over the state assignment rather than over the command names + (evidence: bpv7 red against an otherwise-green binary) diff --git a/.add/tasks/cluster-client-bootstrap/TASK.md b/.add/tasks/cluster-client-bootstrap/TASK.md index 742041a8..dcbe6422 100644 --- a/.add/tasks/cluster-client-bootstrap/TASK.md +++ b/.add/tasks/cluster-client-bootstrap/TASK.md @@ -2,7 +2,7 @@ slug: cluster-client-bootstrap · created: 2026-08-09 · stage: production autonomy: auto -phase: build +phase: done @@ -501,7 +501,7 @@ Tests live in: `tests/cluster_client_bootstrap.rs` ## 5 · BUILD — AI writes code ▸ docs/07-step-5-build.md -Scope (may touch): `src/cluster/` `src/command/connection.rs` `src/command/metadata.rs` `src/server/conn/` `src/acl/rules.rs` `tests/cluster_client_bootstrap.rs` `tests/cluster_formation.rs` `scripts/client-compat/manifest.yaml` `src/../CHANGELOG.md` `tmp/` +Scope (may touch): `src/cluster/` `src/command/connection.rs` `src/command/metadata.rs` `src/server/conn/` `src/acl/rules.rs` `tests/cluster_client_bootstrap.rs` `tests/cluster_formation.rs` `tests/integration.rs` `scripts/client-compat/manifest.yaml` `src/../CHANGELOG.md` `tmp/` diff --git a/CHANGELOG.md b/CHANGELOG.md index 89e54c22..8f05a0e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `cluster-ctl` thread, which by policy aborts the whole server. A short v3 header is now rejected as malformed. Seeded into `fuzz/corpus/gossip_deser` — the target was correct but had not synthesised the 4-byte magic plus that 40-byte length window within its PR budget. +- **A pipelined `HELLO` no longer re-encodes the replies that came before it.** Moon accumulates a + read batch's replies and serialized them all at flush time under whichever protocol version was in + effect at the END of the batch, so a `HELLO 2` sent in the same write as an earlier command + retro-downgraded that earlier reply: `CONFIG GET maxmemory` produced under RESP3 went out as `*2` + instead of `%1`. Every reply is now encoded in the protocol that was in effect when that reply was + produced, with a switch taking effect from its own index onward — inclusive, so a `HELLO`'s own + reply is rendered in the protocol it establishes, which is what redis-server 8.6.1 does. Only the + downgrade direction was ever visible: the frame *shape* is already fixed correctly at dispatch, and + a RESP2-flattened array re-serialized as RESP3 still emits `*`, which is why the upgrade direction + looked fine by accident. It is now pinned in both directions. `RESET` is covered too: it is + contracted to return the connection to its default state, RESP2 included, so it moves the protocol + exactly as a pipelined `HELLO 2` does — and a fix that covered only the two `HELLO` sites left + `HELLO 3` + `RESET` in one write still retro-downgrading. `redis-cli` cannot express two commands + in one `write()`, which is how this survived — the new suite drives a raw socket, and runs on both + runtimes at 1 and 4 shards. Batches without a protocol switch — essentially all of them — keep the + previous single-version loop, one branch and no allocation. +- **`CONFIG GET` answers every parameter, not just the first.** `CONFIG GET maxmemory appendonly` + reported only `maxmemory`; the rest were silently dropped, which is what `redis-py`'s + `config_get(*params)` and monitoring agents that read several settings per call send. The reply is + now the union over all patterns, deduplicated (`maxmemory` plus `maxmemory*` reports it once), in + the server's own table order rather than the caller's argument order, with unknown patterns skipped + rather than erroring — all four properties measured against redis-server 8.6.1. - **`CLUSTER INFO` no longer claims `cluster_enabled`, and a slotless node no longer claims health.** Two integration assertions encoded the pre-fix behaviour and contradicted the measured oracle: redis-server 8.6.1 reports `cluster_enabled` in `INFO` only — `CLUSTER INFO` never carries it — diff --git a/src/command/config.rs b/src/command/config.rs index 0b95dbf4..d96873e6 100644 --- a/src/command/config.rs +++ b/src/command/config.rs @@ -1,4 +1,5 @@ use bytes::Bytes; +use smallvec::SmallVec; use crate::command::key::glob_match; use crate::config::{RuntimeConfig, ServerConfig}; @@ -16,13 +17,26 @@ pub fn config_get( )); } - let pattern = match &args[0] { - Frame::BulkString(s) => s.to_ascii_lowercase(), - Frame::SimpleString(s) => s.to_ascii_lowercase(), - _ => { - return Frame::Error(Bytes::from_static(b"ERR invalid argument")); + // Redis accepts MANY parameters, not one: `CONFIG GET maxmemory appendonly` + // answers both. Reading only `args[0]` silently dropped the rest, which is + // what `redis-py`'s `config_get(*params)` and every monitoring agent that + // reads two settings in one call send. + // + // Measured on redis-server 8.6.1: the result is the UNION over the + // patterns, deduplicated (`maxmemory` + `maxmemory*` reports `maxmemory` + // once), in the server's own table order rather than the caller's argument + // order, with unknown patterns silently skipped. Filtering the table once + // per entry — rather than looping the patterns outermost — gives all three + // properties for free. + let mut patterns: SmallVec<[Vec; 4]> = SmallVec::new(); + for arg in args { + match arg { + Frame::BulkString(s) | Frame::SimpleString(s) => patterns.push(s.to_ascii_lowercase()), + _ => { + return Frame::Error(Bytes::from_static(b"ERR invalid argument")); + } } - }; + } // Build list of all known config parameters let params: Vec<(&[u8], String)> = vec![ @@ -80,7 +94,7 @@ pub fn config_get( let mut result = Vec::new(); for (name, value) in params { - if glob_match(&pattern, name) { + if patterns.iter().any(|p| glob_match(p, name)) { result.push(Frame::BulkString(Bytes::copy_from_slice(name))); result.push(Frame::BulkString(Bytes::from(value))); } diff --git a/src/server/conn/core.rs b/src/server/conn/core.rs index 727f442b..85523900 100644 --- a/src/server/conn/core.rs +++ b/src/server/conn/core.rs @@ -12,6 +12,7 @@ use bytes::Bytes; use ringbuf::HeapProd; +use smallvec::SmallVec; use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; @@ -225,6 +226,24 @@ pub(crate) struct ConnectionState { /// asymmetry is the whole point of the verb, and a "just return +OK" /// implementation is what gets it wrong. pub readonly: bool, + /// Protocol switch points inside the batch currently being built: + /// `(response_index, version_from_that_index_on)`. + /// + /// A pipelined `HELLO` changes the protocol for the replies AFTER it and + /// for its own reply, never for the ones already produced — but a batch is + /// serialized in ONE pass at flush, by which time `protocol_version` has + /// already moved on. Without these points the batch's final version + /// retro-encodes every earlier reply, and a `Frame::Map` produced under + /// RESP3 goes out flattened as a RESP2 array. + /// + /// Empty for every batch containing no `HELLO` — essentially all of them — + /// and [`encode_response_batch`] branches on that, so the hot path keeps + /// its single-version loop and allocates nothing. + pub proto_switches: SmallVec<[(usize, u8); 2]>, + /// Version in effect at index 0 of the current batch, captured when the + /// FIRST switch is recorded — by then `protocol_version` already holds the + /// new one. + pub proto_batch_start: u8, pub acl_log: AclLog, /// Cached per-connection: true when the current user has no ACL @@ -371,6 +390,8 @@ impl ConnectionState { client_name, asking: false, readonly: false, + proto_switches: SmallVec::new(), + proto_batch_start: 2, acl_log: AclLog::new(acl_max_len), subscription_count: 0, subscriber_id: 0, diff --git a/src/server/conn/handler_monoio/dispatch.rs b/src/server/conn/handler_monoio/dispatch.rs index 268d399d..5cf51baf 100644 --- a/src/server/conn/handler_monoio/dispatch.rs +++ b/src/server/conn/handler_monoio/dispatch.rs @@ -100,9 +100,15 @@ pub(super) fn check_auth_gate( ), ); if !matches!(&response, Frame::Error(_)) { + // MUST come before `conn.protocol_version` moves: the helper + // reads it to learn what this batch STARTED in. Recorded at + // `responses.len()`, the index this reply will occupy, so the + // switch covers HELLO's own answer; replies already queued were + // produced under the OLD protocol and keep it. See + // `shared::encode_response_batch`. + crate::server::conn::shared::note_protocol_switch(conn, responses.len(), new_proto); conn.protocol_version = new_proto; - // Keep the wire codec in lockstep: the HELLO reply itself must - // already be serialized in the negotiated protocol (RESP3 map). + // Keep the wire codec in lockstep for single-frame encodes. codec.set_protocol_version(new_proto); } if let Some(name) = new_name { @@ -408,9 +414,14 @@ pub(super) fn try_handle_hello( ), ); if !matches!(&response, Frame::Error(_)) { + // MUST come before `conn.protocol_version` moves: the helper reads it + // to learn what this batch STARTED in. Recorded at `responses.len()`, + // the index this reply will occupy, so the switch covers HELLO's own + // answer; replies already queued were produced under the OLD protocol + // and keep it. See `shared::encode_response_batch`. + crate::server::conn::shared::note_protocol_switch(conn, responses.len(), new_proto); conn.protocol_version = new_proto; - // Keep the wire codec in lockstep: the HELLO reply itself must - // already be serialized in the negotiated protocol (RESP3 map). + // Keep the wire codec in lockstep for single-frame encodes. codec.set_protocol_version(new_proto); } if let Some(name) = new_name { diff --git a/src/server/conn/handler_monoio/mod.rs b/src/server/conn/handler_monoio/mod.rs index 80880dff..2c8d1b1c 100644 --- a/src/server/conn/handler_monoio/mod.rs +++ b/src/server/conn/handler_monoio/mod.rs @@ -1919,9 +1919,11 @@ pub(crate) async fn handle_connection_sharded_monoio< &mut responses, ) .await; - for resp in &responses { - codec.encode_frame(resp, &mut write_buf); - } + crate::server::conn::shared::encode_response_batch( + &mut conn, + &responses, + &mut write_buf, + ); if !write_buf.is_empty() { let data = write_buf.split().freeze(); if !write_all_bounded!( @@ -3536,9 +3538,9 @@ pub(crate) async fn handle_connection_sharded_monoio< } // Serialize all responses into write_buf, then do ONE write_all syscall. - for response in &responses { - codec.encode_frame(response, &mut write_buf); - } + // `encode_batch`, not a bare loop: a pipelined HELLO changes the protocol + // partway through and the replies before it must keep the old encoding. + crate::server::conn::shared::encode_response_batch(&mut conn, &responses, &mut write_buf); // Write all responses in one batch using ownership I/O if !write_buf.is_empty() { diff --git a/src/server/conn/handler_sharded/mod.rs b/src/server/conn/handler_sharded/mod.rs index f462a478..7e7b9dfe 100644 --- a/src/server/conn/handler_sharded/mod.rs +++ b/src/server/conn/handler_sharded/mod.rs @@ -675,6 +675,16 @@ pub(crate) async fn handle_connection_sharded_inner< ), ); if !matches!(&response, Frame::Error(_)) { + // Recorded BEFORE the push, so the index is + // the one this reply will occupy and the + // switch covers it. Replies already queued + // were produced under the OLD protocol and + // keep it — see `encode_response_batch`. + crate::server::conn::shared::note_protocol_switch( + &mut conn, + responses.len(), + new_proto, + ); conn.protocol_version = new_proto; } if let Some(name) = new_name { @@ -808,7 +818,10 @@ pub(crate) async fn handle_connection_sharded_inner< ctx.cluster_state.is_some(), ), ); - if !matches!(&response, Frame::Error(_)) { conn.protocol_version = new_proto; } + if !matches!(&response, Frame::Error(_)) { + crate::server::conn::shared::note_protocol_switch(&mut conn, responses.len(), new_proto); + conn.protocol_version = new_proto; + } if let Some(name) = new_name { conn.client_name = Some(name); } if let Some(ref uname) = opt_user { conn.adopt_user(uname.clone(), &ctx.acl_table); @@ -1415,13 +1428,14 @@ pub(crate) async fn handle_connection_sharded_inner< ) .await; write_buf.clear(); - for response in responses.iter() { - if conn.protocol_version >= 3 { - crate::protocol::serialize_resp3(response, &mut write_buf); - } else { - crate::protocol::serialize(response, &mut write_buf); - } - } + // Not a per-response version test: a pipelined HELLO + // moves the protocol partway through the batch, and the + // replies produced before it must keep the old encoding. + crate::server::conn::shared::encode_response_batch( + &mut conn, + &responses, + &mut write_buf, + ); if !write_all_bounded!(stream, &write_buf, write_timeout, out_cap_normal, client_live, client_id) { arena.reset(); return (HandlerResult::Done, None); } // c10k A1: `read_buf` doubles as the carry buffer — it // holds only the unparsed tail of this batch here, so @@ -2711,13 +2725,14 @@ pub(crate) async fn handle_connection_sharded_inner< } write_buf.clear(); - for response in &responses { - if conn.protocol_version >= 3 { - crate::protocol::serialize_resp3(response, &mut write_buf); - } else { - crate::protocol::serialize(response, &mut write_buf); - } - } + // Not a per-response version test: a pipelined HELLO moves the + // protocol partway through the batch, and the replies produced + // before it must keep the old encoding. + crate::server::conn::shared::encode_response_batch( + &mut conn, + &responses, + &mut write_buf, + ); if !write_all_bounded!(stream, &write_buf, write_timeout, out_cap_normal, client_live, client_id) { return (HandlerResult::Done, None); } diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index 4868bda5..26e41a4d 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -530,6 +530,16 @@ pub async fn handle_connection( // Phase 1: Handle connection-level intercepts, collect dispatchable frames // Phase 2: Acquire ONE write lock, execute ALL dispatchable frames let mut responses: Vec = Vec::with_capacity(batch.len()); + // This handler flushes through `Framed::send`, which encodes each frame + // with the codec's version at send time, so it does NOT consume the + // per-reply switch record the two SHIPPED handlers use — and it shares + // `try_handle_reset`, which writes one. Dropped at the batch boundary so + // the record cannot accumulate across batches on a connection that + // RESETs repeatedly. This path is a library/embedding entry point + // (`server::run`); `main.rs` drives `run_sharded` at both call sites, so + // the binary never reaches it. Its own retro-encode behaviour is + // unchanged and recorded as a spec delta. + conn.proto_switches.clear(); // Each entry carries (resp_idx, db, bytes) so the Always-policy flush // path can patch responses[resp_idx] with WRITEFAIL when fsync fails, // before any response is sent to the client (H1 fix — FIX-W1-1). diff --git a/src/server/conn/shared.rs b/src/server/conn/shared.rs index a3e69ffd..eb373293 100644 --- a/src/server/conn/shared.rs +++ b/src/server/conn/shared.rs @@ -6,7 +6,6 @@ use std::sync::Arc; use parking_lot::RwLock; use bytes::Bytes; -#[cfg(feature = "runtime-tokio")] use bytes::BytesMut; use crate::command::config as config_cmd; @@ -27,6 +26,107 @@ use super::util::extract_command; #[cfg(feature = "runtime-tokio")] pub(crate) type SharedDatabases = Arc>>; +/// Record that the reply at `at` — and every reply after it in this batch — is +/// produced under `version`. +/// +/// Call from the `HELLO` handler with `responses.len()` BEFORE pushing the +/// HELLO reply, so the switch covers that reply too: Redis renders `HELLO`'s own +/// answer in the protocol it has just negotiated. +/// +/// **Call this BEFORE assigning `conn.protocol_version`.** The first call in a +/// batch reads that field to learn what the batch STARTED in; calling it after +/// the assignment records the new version as the batch start and silently +/// restores the very bug this exists to fix. Only an end-to-end test can catch +/// that mistake — `tests/batch_protocol_version.rs::bpv1` and `::bpv3` are the +/// pins, and they DID catch it during development. +pub(crate) fn note_protocol_switch( + conn: &mut super::core::ConnectionState, + at: usize, + version: u8, +) { + if conn.proto_switches.is_empty() { + conn.proto_batch_start = conn.protocol_version; + } + conn.proto_switches.push((at, version)); +} + +/// Walks a batch index-by-index, yielding the protocol version in effect at +/// each one. +/// +/// Split out from `encode_response_batch` so the version arithmetic — the part +/// that is easy to get subtly wrong and expensive to observe on the wire — can +/// be tested directly. Indices must be visited in ascending order; the cursor +/// never rewinds, which keeps the walk O(batch) rather than O(batch x switches). +struct ProtoWalk<'a> { + switches: &'a [(usize, u8)], + next: usize, + version: u8, +} + +impl<'a> ProtoWalk<'a> { + fn new(start: u8, switches: &'a [(usize, u8)]) -> Self { + Self { + switches, + next: 0, + version: start, + } + } + + /// Version for reply `idx`. A switch recorded AT `idx` applies to it — + /// `HELLO`'s own reply is rendered in the protocol it just negotiated. + fn version_at(&mut self, idx: usize) -> u8 { + while let Some(&(at, to)) = self.switches.get(self.next) + && at <= idx + { + self.version = to; + self.next += 1; + } + self.version + } +} + +/// Serialize a whole response batch, honouring any protocol switch recorded +/// inside it, then clear the switch list ready for the next batch. +/// +/// A pipelined `HELLO` moves `conn.protocol_version` the instant it is handled, +/// but the batch is not serialized until every command in it has run. Encoding +/// the batch under one final version therefore RETRO-encodes the replies +/// produced before the switch — measured against redis-server 8.6.1, a +/// `CONFIG GET` answered under RESP3 then followed by `HELLO 2` in the same +/// pipeline must still go out as `%1`, not `*2`. +/// +/// With no switch recorded — every batch that contains no `HELLO`, which is +/// essentially all of them — this is exactly the single-version loop it +/// replaces, with one branch and no allocation. +pub(crate) fn encode_response_batch( + conn: &mut super::core::ConnectionState, + responses: &[Frame], + buf: &mut BytesMut, +) { + if conn.proto_switches.is_empty() { + if conn.protocol_version >= 3 { + for item in responses { + crate::protocol::serialize_resp3(item, buf); + } + } else { + for item in responses { + crate::protocol::serialize(item, buf); + } + } + return; + } + + let mut walk = ProtoWalk::new(conn.proto_batch_start, &conn.proto_switches); + for (idx, item) in responses.iter().enumerate() { + if walk.version_at(idx) >= 3 { + crate::protocol::serialize_resp3(item, buf); + } else { + crate::protocol::serialize(item, buf); + } + } + conn.proto_switches.clear(); +} + /// Resolve FT.SEARCH `as_of_lsn` with the canonical precedence (TEMP-04, ACID-09): /// /// 1. Explicit `AS_OF ` clause -> `TemporalRegistry::lsn_at(wall_ms)`. @@ -1244,6 +1344,15 @@ pub(crate) fn try_handle_reset( // Identity + protocol, from the one definition of "default". let (proto, db, authed, user, name) = crate::server::conn::util::restore_migrated_state(None, requirepass); + // RESET is the SECOND protocol switch in the command set, and it moves the + // protocol the same way a pipelined `HELLO 2` does. Recorded before the + // assignment, at the index `+RESET` will occupy, so replies produced + // earlier in this batch keep the protocol they were produced under. A fix + // that covered only HELLO left `HELLO 3` + `RESET` in one write still + // retro-downgrading — see `note_protocol_switch`. + if proto != conn.protocol_version { + note_protocol_switch(conn, responses.len(), proto); + } conn.protocol_version = proto; conn.selected_db = db; conn.authenticated = authed; @@ -1841,3 +1950,45 @@ mod watch_locality_tests { ); } } + +#[cfg(test)] +mod proto_walk_tests { + use super::ProtoWalk; + + fn seq(start: u8, switches: &[(usize, u8)], n: usize) -> Vec { + let mut w = ProtoWalk::new(start, switches); + (0..n).map(|i| w.version_at(i)).collect() + } + + /// The defect this whole mechanism exists for: a reply produced under RESP3 + /// keeps RESP3 even though a later HELLO 2 downgraded the connection. + #[test] + fn a_switch_does_not_reach_backwards() { + assert_eq!(seq(3, &[(1, 2)], 3), vec![3, 2, 2]); + } + + /// HELLO's own reply is rendered in the protocol it just negotiated, so the + /// switch applies AT its index, not after it. + #[test] + fn a_switch_applies_at_its_own_index_not_the_next_one() { + assert_eq!(seq(2, &[(0, 3)], 2), vec![3, 3]); + } + + /// Two HELLOs in one batch: each takes effect from its own index. + #[test] + fn every_switch_in_a_batch_is_honoured_in_order() { + assert_eq!(seq(3, &[(1, 2), (3, 3)], 5), vec![3, 2, 2, 3, 3]); + } + + /// The overwhelmingly common case — no HELLO in the batch. + #[test] + fn no_switches_means_one_version_throughout() { + assert_eq!(seq(3, &[], 4), vec![3, 3, 3, 3]); + } + + /// A switch past the end of the batch cannot affect it, and must not panic. + #[test] + fn a_switch_beyond_the_last_reply_is_inert() { + assert_eq!(seq(2, &[(9, 3)], 3), vec![2, 2, 2]); + } +} diff --git a/tests/batch_protocol_version.rs b/tests/batch_protocol_version.rs new file mode 100644 index 00000000..325a7f86 --- /dev/null +++ b/tests/batch_protocol_version.rs @@ -0,0 +1,419 @@ +//! ADD task `batch-protocol-version-fidelity` — failing-first suite. +//! +//! One rule, stated once: **every reply is encoded in the protocol that was in +//! effect when that reply was produced.** A `HELLO` in the middle of a pipeline +//! changes the protocol for the replies that come AFTER it (and for its own +//! reply), never for the ones already produced. +//! +//! Moon serializes a whole batch at flush time under a single +//! `codec.protocol_version`, while `codec.set_protocol_version` fires +//! synchronously at the HELLO site mid-batch. The post-HELLO version therefore +//! retro-encodes replies produced before it. +//! +//! The UPGRADE direction (`HELLO 3` mid-batch) happens to look right today — +//! not because the batch is encoded correctly, but because +//! `apply_resp3_conversion` already flattened the earlier reply to an `Array` +//! at dispatch time under RESP2, so re-encoding it as RESP3 still emits `*`. +//! The DOWNGRADE direction has no such accident and is visibly wrong. Both +//! directions are asserted, so a fix cannot trade one for the other. +//! +//! Oracle: redis-server 8.6.1, measured 2026-08-14 over a raw socket. +//! +//! pipelined CONFIG GET maxmemory / HELLO 2 / CONFIG GET maxmemory +//! on a RESP3 connection: +//! redis -> %1 (produced under RESP3) *14 (HELLO's own reply) *2 +//! moon -> *2 *14 *2 +//! ^ wrong: retro-downgraded +//! +//! Reproduced on monoio and tokio, shards 1 and 4 — all three dispatch paths. +//! +//! Run alone with: cargo test --test batch_protocol_version + +mod common; + +use std::io::{Read, Write}; +use std::net::TcpStream; +use std::process::{Child, Command}; +use std::time::{Duration, Instant}; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn spawn_moon(dir: &std::path::Path, shards: u32) -> (Child, u16) { + common::spawn_listening(|port| { + Command::new(common::find_moon_binary()) + .args([ + "--port", + &port.to_string(), + "--dir", + &dir.to_string_lossy(), + "--shards", + &shards.to_string(), + "--appendonly", + "no", + // The shared /Volumes checkout hovers near the 5% diskfull + // guard; a tripped guard would fail this suite for an unrelated + // reason. + "--disk-free-min-pct", + "0", + ]) + .stdout(std::fs::File::create(dir.join("moon.stdout.log")).expect("stdout log")) + .stderr(std::fs::File::create(dir.join("moon.stderr.log")).expect("stderr log")) + .spawn() + .expect("spawn moon") + }) +} + +struct ServerGuard(Child); +impl Drop for ServerGuard { + fn drop(&mut self) { + common::sigkill(&mut self.0); + } +} + +fn connect_ready(port: u16) -> TcpStream { + let deadline = Instant::now() + Duration::from_secs(30); + loop { + if let Ok(s) = TcpStream::connect(format!("127.0.0.1:{port}")) { + s.set_read_timeout(Some(Duration::from_secs(10))).ok(); + s.set_write_timeout(Some(Duration::from_secs(10))).ok(); + let mut s = s; + if s.write_all(b"PING\r\n").is_ok() { + let mut buf = [0u8; 64]; + if let Ok(n) = s.read(&mut buf) + && n > 0 + && buf[..n].windows(4).any(|w| w == b"PONG") + { + return s; + } + } + } + assert!( + Instant::now() < deadline, + "server on {port} never answered PING" + ); + std::thread::sleep(Duration::from_millis(50)); + } +} + +/// A reply reduced to what this suite is about: its RESP type byte. +/// +/// Values are deliberately discarded. Two protocols may legitimately render +/// the same value differently; what must be right is which protocol was used. +struct Conn { + s: TcpStream, + buf: Vec, + pos: usize, +} + +impl Conn { + fn new(port: u16, proto: u8) -> Self { + let mut c = Conn { + s: connect_ready(port), + buf: Vec::with_capacity(64 * 1024), + pos: 0, + }; + if proto == 3 { + c.write(&[&["HELLO", "3"]]); + let tag = c.skip_frame(); + assert_eq!( + tag, '%', + "HELLO 3 must be answered with a RESP3 map or this suite proves nothing" + ); + } + c + } + + /// Write every command in ONE `write_all`, so the server sees a single + /// pipelined batch rather than a sequence of round trips. That is the whole + /// point: a batch is what gets encoded under one protocol version. + fn write(&mut self, batch: &[&[&str]]) { + let mut req = Vec::with_capacity(256); + for parts in batch { + req.extend_from_slice(format!("*{}\r\n", parts.len()).as_bytes()); + for p in *parts { + req.extend_from_slice(format!("${}\r\n{p}\r\n", p.len()).as_bytes()); + } + } + self.s.write_all(&req).expect("write batch"); + } + + fn fill(&mut self) { + let mut chunk = [0u8; 16 * 1024]; + let n = self.s.read(&mut chunk).expect("read"); + assert!(n > 0, "connection closed mid-frame"); + self.buf.extend_from_slice(&chunk[..n]); + } + + fn line(&mut self) -> String { + loop { + if let Some(rel) = self.buf[self.pos..].windows(2).position(|w| w == b"\r\n") { + let start = self.pos; + let end = start + rel; + let out = String::from_utf8_lossy(&self.buf[start..end]).into_owned(); + self.pos = end + 2; + return out; + } + self.fill(); + } + } + + fn exact(&mut self, n: usize) { + while self.buf.len() - self.pos < n + 2 { + self.fill(); + } + self.pos += n + 2; + } + + /// Consume exactly one reply and return its top-level type byte. + fn skip_frame(&mut self) -> char { + let line = self.line(); + let tag = line.chars().next().expect("empty frame"); + let rest = &line[1..]; + match tag { + '+' | '-' | ':' | ',' | '#' | '_' | '(' => {} + '$' | '=' => { + let n: i64 = rest.parse().unwrap_or(-1); + if n >= 0 { + self.exact(n as usize); + } + } + '*' | '~' | '>' => { + let n: i64 = rest.parse().unwrap_or(-1); + for _ in 0..n.max(0) { + self.skip_frame(); + } + } + '%' => { + let n: i64 = rest.parse().unwrap_or(-1); + for _ in 0..n.max(0) * 2 { + self.skip_frame(); + } + } + other => panic!("unknown RESP type byte {other:?} in {line:?}"), + } + tag + } + + /// Type byte of each of the next `n` replies, in order. + fn tags(&mut self, n: usize) -> String { + (0..n).map(|_| self.skip_frame()).collect() + } +} + +/// Run `body` against a server on 1 shard and on 4 shards. +/// +/// Not decoration: `--shards 1` and `--shards 4` reach different dispatch +/// handlers, and this repo's recurring defect class is a behaviour that exists +/// on some dispatch paths and not others. +fn on_each_shard_count(body: impl Fn(u16)) { + for shards in [1u32, 4] { + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path(), shards); + let _guard = ServerGuard(child); + body(port); + } +} + +// --------------------------------------------------------------------------- +// bpv1-bpv4 — protocol in effect at production time +// --------------------------------------------------------------------------- + +/// RED on main. The load-bearing test: a reply produced under RESP3 must stay +/// RESP3 even though a later `HELLO 2` in the same batch downgraded the +/// connection. +#[test] +fn bpv1_a_reply_produced_before_hello_2_keeps_its_resp3_encoding() { + on_each_shard_count(|port| { + let mut c = Conn::new(port, 3); + c.write(&[ + &["CONFIG", "GET", "maxmemory"], + &["HELLO", "2"], + &["CONFIG", "GET", "maxmemory"], + ]); + assert_eq!( + c.tags(3), + "%**", + "reply 1 was produced while RESP3 was in effect and must be a Map; \ + HELLO 2's own reply and everything after it are RESP2 arrays" + ); + }); +} + +/// The mirror direction. Expected GREEN today — a pin, so the fix for `bpv1` +/// cannot be a blanket "use the batch-start version" that breaks this. +#[test] +fn bpv2_a_reply_produced_before_hello_3_keeps_its_resp2_encoding() { + on_each_shard_count(|port| { + let mut c = Conn::new(port, 2); + c.write(&[ + &["CONFIG", "GET", "maxmemory"], + &["HELLO", "3"], + &["CONFIG", "GET", "maxmemory"], + ]); + assert_eq!( + c.tags(3), + "*%%", + "reply 1 was produced under RESP2 and must stay an Array; HELLO 3's \ + own reply and everything after it are RESP3 maps" + ); + }); +} + +/// RED on main. Two switches in one batch — proves the fix tracks a SEQUENCE of +/// switch points, not a single "did a HELLO happen" flag. +#[test] +fn bpv3_two_hellos_in_one_batch_each_take_effect_from_their_own_index() { + on_each_shard_count(|port| { + let mut c = Conn::new(port, 3); + c.write(&[ + &["CONFIG", "GET", "maxmemory"], // % produced under RESP3 + &["HELLO", "2"], // * switch -> RESP2, own reply RESP2 + &["CONFIG", "GET", "maxmemory"], // * RESP2 + &["HELLO", "3"], // % switch -> RESP3, own reply RESP3 + &["CONFIG", "GET", "maxmemory"], // % RESP3 + ]); + assert_eq!(c.tags(5), "%**%%", "each HELLO applies from its own index"); + }); +} + +/// Expected GREEN today. Pins the hot path: a batch with no HELLO in it must be +/// encoded exactly as before, since that is every real pipeline. +#[test] +fn bpv4_a_batch_without_hello_is_encoded_entirely_in_one_protocol() { + on_each_shard_count(|port| { + let mut c = Conn::new(port, 3); + c.write(&[ + &["CONFIG", "GET", "maxmemory"], + &["SET", "bpv4", "v"], + &["CONFIG", "GET", "maxmemory"], + ]); + assert_eq!(c.tags(3), "%+%", "no switch point, no change in encoding"); + + let mut c2 = Conn::new(port, 2); + c2.write(&[ + &["CONFIG", "GET", "maxmemory"], + &["SET", "bpv4b", "v"], + &["CONFIG", "GET", "maxmemory"], + ]); + assert_eq!(c2.tags(3), "*+*", "same, under RESP2"); + }); +} + +/// RED on main. `HELLO` is not the only command that moves the protocol — +/// `RESET` is contracted to return the connection to its default state, which +/// includes RESP2. §0 measured `HELLO 3` + `RESET` in one write producing `*14` +/// for the HELLO reply, exactly like the `HELLO 2` case. +/// +/// Kept as its own test rather than folded into bpv1 because it fails through a +/// different code path: `shared::try_handle_reset`, not the two HELLO sites. A +/// fix that covers only HELLO leaves this red — which is what it did. +#[test] +fn bpv7_reset_is_a_protocol_switch_and_does_not_reach_backwards() { + on_each_shard_count(|port| { + let mut c = Conn::new(port, 3); + c.write(&[ + &["CONFIG", "GET", "maxmemory"], // % produced under RESP3 + &["RESET"], // + switch -> RESP2 (reply is +RESET either way) + &["CONFIG", "GET", "maxmemory"], // * RESP2 + ]); + assert_eq!( + c.tags(3), + "%+*", + "RESET reverts to RESP2 from its own index onward; the reply produced \ + before it stays a RESP3 map" + ); + }); +} + +// --------------------------------------------------------------------------- +// bpv5-bpv6 — CONFIG GET accepts more than one parameter +// --------------------------------------------------------------------------- +// +// Found while measuring the oracle for the batch tests. Moon reads only +// `args[0]` and silently drops the rest, so `CONFIG GET maxmemory appendonly` +// answers with maxmemory alone. Glob patterns work; multiple parameters do not. +// `redis-py`'s `config_get(*params)` and every monitoring agent that reads two +// settings in one call hit this. +// +// Measured on redis-server 8.6.1: +// CONFIG GET maxmemory appendonly -> both, in the server's own table order +// CONFIG GET maxmemory 'maxmemory*'-> deduplicated; maxmemory appears ONCE +// CONFIG GET nosuchparam maxmemory -> unknown patterns silently skipped +// CONFIG GET nosuchparam -> empty array + +/// Read a CONFIG GET reply as the set of parameter names it returned. +fn config_get_names(port: u16, args: &[&str]) -> Vec { + let mut parts: Vec<&str> = vec!["CONFIG", "GET"]; + parts.extend_from_slice(args); + let mut c = Conn::new(port, 2); // RESP2: a flat array, easiest to read + c.write(&[&parts]); + + let header = c.line(); + assert_eq!( + &header[..1], + "*", + "RESP2 CONFIG GET is an Array: {header:?}" + ); + let n: usize = header[1..].parse().expect("array length"); + let mut names = Vec::new(); + for i in 0..n { + let lead = c.line(); + let len: i64 = lead[1..].parse().unwrap_or(-1); + let start = c.pos; + if len >= 0 { + c.exact(len as usize); + } + if i % 2 == 0 { + names.push( + String::from_utf8_lossy(&c.buf[start..start + len.max(0) as usize]).into_owned(), + ); + } + } + names +} + +/// RED on main — Moon returns only `maxmemory`. +#[test] +fn bpv5_config_get_honours_every_parameter_not_just_the_first() { + on_each_shard_count(|port| { + let mut names = config_get_names(port, &["maxmemory", "appendonly"]); + names.sort(); + assert_eq!( + names, + vec!["appendonly".to_string(), "maxmemory".to_string()], + "every supplied parameter must be answered, not just args[0]" + ); + + assert!( + config_get_names(port, &["nosuchparam", "maxmemory"]) == vec!["maxmemory".to_string()], + "an unknown parameter is skipped, not an error, and must not \ + suppress the known ones beside it" + ); + assert!( + config_get_names(port, &["nosuchparam"]).is_empty(), + "all-unknown answers an empty array" + ); + }); +} + +/// RED on main. Overlapping patterns must not double-report a parameter. +#[test] +fn bpv6_config_get_deduplicates_overlapping_patterns() { + on_each_shard_count(|port| { + let names = config_get_names(port, &["maxmemory", "maxmemory*"]); + let mut seen = names.clone(); + seen.sort(); + seen.dedup(); + assert_eq!( + seen.len(), + names.len(), + "a parameter matched by two patterns must appear once: {names:?}" + ); + assert!( + names.iter().any(|n| n == "maxmemory"), + "the exact-match parameter is still present: {names:?}" + ); + }); +}