From 8bc92fd0b462241ccf8056d04082128844179b3c Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Tue, 11 Aug 2026 22:26:55 +0700 Subject: [PATCH 1/3] docs(add): close watch-cas-transactions at PASS, record what the scope gate did not prove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code shipped in #470; this closes the task record. The first `gate PASS` was refused with `scope_violation` naming 7020 files, including `.github/workflows/*.yml` and `Cargo.lock` — paths this task never touched. Cause: the scope baseline is a whole-tree byte snapshot taken at the tests->build crossing, with no git awareness. Between that snapshot and the gate, five dependabot PRs merged into main (#420, #417, #346, #358, #359) and target/ was rebuilt, so every unrelated change read as "this task touched it". Rather than re-snapshot quietly, §6 now records: - the task's ACTUAL change set from `git show --name-only 151a1857 1c12af0b`, every path of which is inside the declared §5 Scope; - the two entries that needed naming instead of glossing — `conn/mod.rs` (one line, `pub mod watch;`, undeclared) and `.add/tooling/add.py` (25 lines of engine fix carried in from an earlier session, genuinely outside §5); - that the re-taken baseline is the post-merge tree, so the scope walk at this gate compares the tree against itself and proves nothing. The evidence that the build stayed in scope is the git file list, not the green gate. Also ticks the §6 verify checklist against evidence already gathered: the stubbed-counter A/B (reverting only next_birth_version turns 4 new tests red, so the green is earned), the 41->1 tokio failure delta traced to a diskfull $TMPDIR rather than to the change, and the 9/9 dispatched matrix including the Windows/macOS/console jobs no PR run executes. Adds .add/.gitignore, which `add.py init` scaffolds but this project never got. Without it the two 40MB scope-snapshot.json sidecars are commit candidates; they are regenerable working state, and the durable scope declaration is the state.json anchor. One 436K sidecar is already tracked from an older task and is left alone. Method delta for the next loop: as built, any task gated after unrelated merges land inherits an unfalsifiable scope violation. The anchor wants to be a git tree-ish, or the walk should skip gitignored paths and diff the merge-base. author: Tin Dang --- .add/.gitignore | 9 +++ .add/state.json | 22 +++++-- .add/tasks/watch-cas-transactions/TASK.md | 76 +++++++++++++++++++---- 3 files changed, 90 insertions(+), 17 deletions(-) create mode 100644 .add/.gitignore diff --git a/.add/.gitignore b/.add/.gitignore new file mode 100644 index 00000000..d1b1aff2 --- /dev/null +++ b/.add/.gitignore @@ -0,0 +1,9 @@ +# ADD engine transient artifacts — local working state, never committed. +# (Body matches what `add.py init` scaffolds; this project predates that step.) +# scope-snapshot.json is the tests->build touch baseline the verify scope-gate +# reads from disk — the DURABLE scope declaration is the state.json anchor, so +# the sidecar is regenerable working state. They run 40MB here because the walk +# covers the whole tree including target/. +scope-snapshot.json +pre-archive-state.bak.json +.update-cache.json diff --git a/.add/state.json b/.add/state.json index 7ded1c3c..7545bad2 100644 --- a/.add/state.json +++ b/.add/state.json @@ -275,14 +275,14 @@ }, "watch-cas-transactions": { "title": "WATCH/UNWATCH optimistic locking on both production dispatch paths", - "phase": "build", - "gate": "none", + "phase": "done", + "gate": "PASS", "milestone": "v0-9-client-compat", "depends_on": [ "client-compat-harness" ], "created": "2026-08-09T16:42:29+00:00", - "updated": "2026-08-11T12:31:35+00:00", + "updated": "2026-08-11T15:25:45+00:00", "tripwire": { "contract_md5": "6f2cd3da5562c2cc4bd2fcf12c373405", "tests": {} @@ -293,9 +293,19 @@ "src/server/conn/watch.rs", "src/server/conn/core.rs" ], - "snapshot_md5": "fe6a109ebabb70ce61c2310ddd28f5e2" + "snapshot_md5": "adc23f358324f9512fde7a243a7c06e8" }, - "flag_verified": true + "flag_verified": true, + "heal": { + "attempts": 1, + "history": [ + { + "at": "2026-08-11T15:07:13+00:00", + "reason": "scope_violation: task 'watch-cas-transactions' touched outside its declared \u00a75 Scope \u2014 .github/workflows/ci.yml \u00b7 .github/workflows/console-integration.yml \u00b7 .github/workflows/docs.yml \u00b7 .github/workflows/release.yml \u00b7 Cargo.lock (7020 total)", + "source": "scope" + } + ] + } }, "protocol-error-lifetime": { "title": "Protocol errors reply and close cleanly, never stall or eat the valid prefix", @@ -423,7 +433,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-08-11T12:31:35+00:00", + "updated": "2026-08-11T15:25:45+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/watch-cas-transactions/TASK.md b/.add/tasks/watch-cas-transactions/TASK.md index 28788f5a..e3260780 100644 --- a/.add/tasks/watch-cas-transactions/TASK.md +++ b/.add/tasks/watch-cas-transactions/TASK.md @@ -2,7 +2,7 @@ slug: watch-cas-transactions · created: 2026-08-09 · stage: production autonomy: auto -phase: build +phase: done > One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. > When a phase is unclear, read its book chapter in `.add/docs/`. @@ -387,14 +387,30 @@ non-transaction hot path (the watch check must be skipped entirely when `watched ## 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 — adversarial refute-read; 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 +- [x] all tests pass — full suite both runtimes; 41 tokio-leg failures traced to `MOONERR diskfull` + (`$TMPDIR` filesystem at 3.5% free, under Moon's 5% guard), not to the change: repointing + `TMPDIR` took it 41 -> 1, and the last one (`unlink_colocated_fastpath_persists_across_restart`, + a startup "connection refused") passed 10/10 in isolation. +- [x] coverage did not decrease — 10 new wire-level tests (`tests/watch_cas_transactions.rs` wc1-wc10) + plus 8 locality-lattice unit tests and 5 birth-ticket unit tests; no test deleted or weakened. +- [x] no test or contract was altered during build — §3 moved ONCE, before build, as the recorded + v1 -> v2 amendment (ABA hole + scope widening 6 -> 13 paths), re-approved via `freeze`. The + `build_tampered` tripwire was NOT laundered by re-snapshotting. +- [x] the green was EARNED — the stubbed-counter A/B is the proof: reverting only + `next_birth_version()` to a constant turns 4 of the new tests red, so they bind to the fix and + not to incidental behavior. The two-connection probe reports the exact inverse of the measured + pre-fix result. +- [x] concurrency / timing of the risky operation is safe — version snapshots are read WHERE THE KEY + LIVES (`snapshot_versions` groups by owning shard, one hop each); the EXEC-time re-check runs + inside the owner's execution, so no cross-shard read races the write. Cross-thread reply uses + `flume` oneshots, never a `monoio::spawn` waker. +- [x] no exposed secrets, injection openings, or unexpected dependencies — no new crate; WATCH is + refused inside MULTI rather than queued, closing the "queue then re-enter" path. +- [x] layering & dependencies follow CONVENTIONS.md — the duplicated WATCH/UNWATCH arm was extracted + to `src/server/conn/watch.rs` so the two production handlers cannot drift again, which is the + precise failure this task existed to fix. +- [x] a person reviewed and approved the change — freeze approved at v2; PR #470 merged after the + 9/9 dispatched matrix. ### Build expectations — what "correct" looks like - [x] The §0 two-connection probe, replayed against the built binary, reports `EXEC -> Null` and @@ -417,8 +433,10 @@ non-transaction hot path (the watch check must be skipped entirely when `watched leg under test (0.2pp apart), so both deltas are drift, not signal. A first pass at `-P 1` was DISCARDED as uninformative: 13.9% noise floor with the control moving MORE (-3.9%) than SET (-0.8%) — recorded because "we benched it" is worthless without the noise floor beside it. -- [ ] The full matrix is green via `gh workflow run CI --ref ` BEFORE merge, per the - standing merge bar — Windows/macOS/console are skipped on PRs. +- [x] The full matrix is green via `gh workflow run CI --ref ` BEFORE merge, per the + standing merge bar — Windows/macOS/console are skipped on PRs. Dispatched on + `fix/watch-cas-transactions`: **9/9 green**, including the three jobs no PR run executes + (`Check (Windows)`, `Check (macOS)`, `Check (console feature)`). Merged as `1f5218f2`. ### Durability (kill-9) leg Run against the fat-LTO `target/release/moon`: **22 passed, 1 failed**. The failure, @@ -436,6 +454,42 @@ Two defects found in that suite, both OUT OF SCOPE here, both filed rather than 2. `backup_restore_parity` asserts a filename the server no longer produces. Both are `#[ignore]`d, so CI never runs them — which is exactly how a durability gate rots into proving nothing. +### Scope gate: what it did and did NOT verify at close (recorded, not laundered) + +The first `gate PASS` was REFUSED — `scope_violation`, 7020 files, naming `.github/workflows/*.yml` +and `Cargo.lock`. None of those were touched by this task. The snapshot was taken at the +tests→build crossing; between then and the gate, five dependabot PRs merged into `main` +(#420, #417, #346, #358, #359 — `Cargo.lock`, `Cargo.toml`, workflow files) and `target/` was +rebuilt. The walk is a whole-tree byte diff with no git awareness, so all unrelated movement +reads as "this task touched it". + +The task's ACTUAL change set, from git rather than from the walk — `git show --name-only +151a1857 1c12af0b`: + +`CHANGELOG.md` · `scripts/test-commands.sh` · `scripts/test-consistency.sh` · +`src/server/conn/{core,shared,watch,mod,handler_single}.rs` · +`src/server/conn/handler_{monoio,sharded}/{mod,write}.rs` · +`src/shard/{coordinator,dispatch,spsc_handler}.rs` · +`src/storage/db/{mod,kv_ops,accessors}.rs` · `tests/watch_cas_transactions.rs` + +Every path is inside the §5 declared Scope. Two entries need naming rather than glossing: +- `src/server/conn/mod.rs` — one line, `pub mod watch;`, mechanically required by `watch.rs` + being in scope. Not separately declared. +- `.add/tooling/add.py` — 25 lines, an engine fix for `Status:` parsing carried in from an earlier + session. ADD tooling, not product code, and genuinely outside §5. Recorded here rather than + quietly excluded. + +The snapshot was then re-taken (phase returned to `tests`, re-advanced) so the gate could complete. +**Being explicit about what that costs: the re-taken baseline is the post-merge tree, so the scope +walk at this gate compares the current tree against itself and proves nothing.** The evidence that +this build stayed in scope is the git file list above, not the green scope gate. The gate is +recorded as PASS on the strength of §6's test/bench/matrix evidence and that list — not because +the walk agreed. + +Method delta worth carrying: the scope anchor should be a git tree-ish, or the walk should ignore +gitignored paths and diff against the merge-base, or a task should be gated before unrelated merges +land. As built, any task whose gate trails its merge inherits an unfalsifiable scope violation. + ## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md Watch (reuse scenarios as monitors): EXEC abort rate (a CAS-heavy client whose abort rate falls to From c5882968b64ee177c58c13a90da869859eb54c36 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 12 Aug 2026 01:21:15 +0700 Subject: [PATCH 2/3] feat(command): derive COMMAND/ROLE/RESET/HELLO from real server state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moon's client-identity surface answered from compile-time constants instead of from the server. The failures were invisible by eye because redis-cli renders `:0` and `*0` identically as "0", so every assertion here is on raw RESP bytes. COMMAND and COMMAND COUNT each returned the OTHER'S TYPE: bare COMMAND replied `:0` (an Integer where an Array belongs) and COMMAND COUNT replied `*0` (an Array where an Integer belongs). COMMAND INFO/DOCS/LIST/GETKEYS all replied an empty array. A driver that builds its command map at connect time does not read that as "unsupported" — it reads it as a protocol violation. All six now derive from the COMMAND_META phf registry (263 commands), so registering a command is what makes it introspectable and there is no second table to drift. ROLE and RESET were unknown commands. RESET was registered in the metadata table with full flags while dispatch rejected it — the same advertise-then- reject class as WATCH/UNWATCH before v0.8.6 — and a partial RESET existed only inside handler_sharded's subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere else. RESET's "default state" is taken from restore_migrated_state(None, ..), the same function ConnectionState::new uses, so it cannot drift from what a fresh connection means by default. HELLO contradicted INFO replication on the same connection: hello_acl built `mode`/`standalone` and `role`/`master` as literals, so a replica announced itself a master. Both fields now read ReplicationState/ClusterState. Redis deliberately uses three vocabularies for this one fact (HELLO -> replica, INFO -> slave, ROLE -> slave); that was measured against a live replica pair on redis-server 8.6.1, and it corrected this task's own frozen contract, which had said "slave" everywhere. The test was made stronger rather than matched to the code: ci12 asserts all three surfaces agree, each in its own vocabulary, plus a negative assertion that HELLO no longer claims master. CLIENT INFO / CLIENT LIST reported the literal `laddr=127.0.0.1:0` from inside the format string, so every client on every listener showed port 0. It now carries the real local address. Wired on all three connection handlers plus the inline fast path. ROLE and RESET cannot ride the shared dispatch table — their answers live on ConnectionContext/ReplicationState, not in the Database that dispatch() receives — so each handler needs its own intercept, and handler_single had neither. That handler is reachable only via listener::run_with_shutdown (an in-process tokio API used by a few test suites; main.rs and embedded.rs both route through run_sharded), which is precisely why it had drifted, and why an A/B caught the gap: reverting the fix there left ci12 green. ci14 drives that handler in-process and goes red with `-ERR unknown command 'ROLE'` without it. Tests: 14 raw-RESP scenarios in tests/client_identity_introspection.rs, with parity legs on the monoio and sharded handlers and the inline path. Startup is serialised across the suite's threads — 13 servers initialising data dirs at once left one unable to answer PING inside 30s at ~1 run in 8; a longer timeout would have been slower and still flaky, and a shared OnceLock server would have leaked a live moon past exit because statics are never dropped. 32/32 green at --test-threads=13 after the fix. Found and filed, not folded in: response batches are serialised at flush time using the FINAL protocol version, so a protocol-changing command retro-encodes earlier replies in the same batch (`HELLO 3` alone -> `%7`; `HELLO 3` + `HELLO 2` in one write -> `*14`). Pre-existing — that reproducer touches none of this code — but RESET, which reverts the protocol by contract, adds a second trigger. Filed as ADD task batch-protocol-version-fidelity with the measurement table. MONITOR was split out to monitor-command-feed at freeze: it is a stream rather than a reply, the only item touching the per-command hot path, and the only one exposing other clients' credentials. Gates: cargo fmt --check, clippy (default + tokio/jemalloc, --all-targets), cargo test --release, and the full tokio CI-parity suite all green. author: Tin Dang --- .add/state.json | 53 +- .../batch-protocol-version-fidelity/TASK.md | 205 ++++ .../client-identity-introspection/TASK.md | 585 ++++++++++-- .add/tasks/monitor-command-feed/TASK.md | 201 ++++ CHANGELOG.md | 26 + scripts/test-commands.sh | 13 + scripts/test-consistency.sh | 64 ++ src/client_registry.rs | 134 ++- src/command/connection.rs | 29 +- src/command/identity.rs | 124 +++ src/command/introspect.rs | 385 ++++++++ src/command/metadata.rs | 4 + src/command/mod.rs | 6 +- src/server/conn/core.rs | 20 + src/server/conn/handler_monoio/dispatch.rs | 8 + src/server/conn/handler_monoio/mod.rs | 36 + src/server/conn/handler_sharded/mod.rs | 47 + src/server/conn/handler_single.rs | 65 ++ src/server/conn/shared.rs | 108 +++ tests/client_identity_introspection.rs | 887 ++++++++++++++++++ 20 files changed, 2902 insertions(+), 98 deletions(-) create mode 100644 .add/tasks/batch-protocol-version-fidelity/TASK.md create mode 100644 .add/tasks/monitor-command-feed/TASK.md create mode 100644 src/command/identity.rs create mode 100644 src/command/introspect.rs create mode 100644 tests/client_identity_introspection.rs diff --git a/.add/state.json b/.add/state.json index 7545bad2..6821d819 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,7 +1,7 @@ { "project": "moon", "stage": "production", - "active_task": "watch-cas-transactions", + "active_task": "client-identity-introspection", "active_milestone": "v0-9-client-compat", "tasks": { "hotpath-lock-quickwins": { @@ -209,14 +209,37 @@ }, "client-identity-introspection": { "title": "HELLO/COMMAND/ROLE/RESET identity + registry-dispatch reconciliation", - "phase": "ground", - "gate": "none", + "phase": "done", + "gate": "PASS", "milestone": "v0-9-client-compat", "depends_on": [ "client-compat-harness" ], "created": "2026-08-09T07:32:03+00:00", - "updated": "2026-08-09T07:32:03+00:00" + "updated": "2026-08-11T18:20:42+00:00", + "flag_verified": true, + "tripwire": { + "contract_md5": "783b47c7b6e4897eaf7f049c3da5e3f2", + "tests": {} + }, + "scope": { + "declared": [ + "src/command/connection.rs", + "src/command/metadata.rs", + "src/command/mod.rs" + ], + "snapshot_md5": "d52700ba46d139e4de65276f8c64477d" + }, + "heal": { + "attempts": 1, + "history": [ + { + "at": "2026-08-11T18:02:08+00:00", + "reason": "tamper_detected:contract_tampered", + "source": "tamper" + } + ] + } }, "resp3-type-fidelity": { "title": "RESP3 reply-type parity, applied at one conversion choke point", @@ -328,6 +351,26 @@ ], "created": "2026-08-09T16:46:23+00:00", "updated": "2026-08-09T16:46:23+00:00" + }, + "monitor-command-feed": { + "title": "MONITOR command feed \u2014 stream executed commands, redacted, zero-cost when unattached", + "phase": "ground", + "gate": "none", + "milestone": "v0-9-client-compat", + "depends_on": [ + "client-compat-harness" + ], + "created": "2026-08-11T16:13:33+00:00", + "updated": "2026-08-11T16:13:33+00:00" + }, + "batch-protocol-version-fidelity": { + "title": "Response batch must be encoded in the protocol in effect when each reply was produced", + "phase": "ground", + "gate": "none", + "milestone": "v0-9-client-compat", + "depends_on": [], + "created": "2026-08-11T17:33:52+00:00", + "updated": "2026-08-11T17:33:52+00:00" } }, "milestones": { @@ -433,7 +476,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-08-11T15:25:45+00:00", + "updated": "2026-08-11T18:20:42+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 new file mode 100644 index 00000000..668b3d73 --- /dev/null +++ b/.add/tasks/batch-protocol-version-fidelity/TASK.md @@ -0,0 +1,205 @@ +# TASK: Response batch must be encoded in the protocol in effect when each reply was produced + +slug: batch-protocol-version-fidelity · created: 2026-08-12 · stage: production +autonomy: auto +phase: ground + + +> One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. +> When a phase is unclear, read its book chapter in `.add/docs/` (linked per section). +> The phase marker above is the single source of truth — keep it in sync via `add.py phase`. + +--- + +## 0 · GROUND — the real codebase ▸ docs/02-the-flow.md + +Touches (files · symbols · signatures): +Context (working folder): +Honors (patterns / conventions): +Anchors the contract cites: + +--- + +## 0 · GROUND — measured, not recalled + +**Found while verifying `client-identity-introspection` (2026-08-12).** Filed rather than folded +into that PR because the reproducer below touches none of that task's code. + +Moon accumulates the replies for every command in one read batch and serializes them at FLUSH time, +using whatever protocol version is in effect at the end of the batch. Any command later in the same +batch that changes the protocol therefore RETRO-ENCODES the earlier replies. Redis writes each +reply as it is produced, so it does not have this failure mode. + +Measured against this branch's binaries, single-shard, both handlers (monoio default and the tokio +`handler_single` path) — identical results on both: + +| bytes sent in ONE write | first bytes of the HELLO 3 reply | correct? | +|--------------------------------|----------------------------------|----------| +| `HELLO 3\r\n` (alone) | `%7` | yes | +| `HELLO 3\r\nPING\r\n` | `%7` | yes | +| `HELLO 3\r\nHELLO 2\r\n` | `*14` | **no** | +| `HELLO 3\r\nRESET\r\n` | `*14` | **no** | + +`*14` is the RESP2 flattening of the 7-entry map. A client that pipelines `HELLO 3` with any later +protocol-changing command reads the handshake reply in the wrong protocol and misparses it. + +`HELLO 3` + `HELLO 2` is the ownership proof: it predates and is independent of +`client-identity-introspection` (no `RESET`, no `ROLE`, no `COMMAND`, no `identity.rs`). RESET, +which reverts the protocol to RESP2 by contract, simply adds a second trigger for the same +pre-existing defect. + +Reproducer script: `/tmp/prepipe.sh` (raw `/dev/tcp` writes — `redis-cli` cannot express "two +commands in one write", which is why this survived). + +Shape of the fix (NOT yet decided — this is §0, not a contract): either flush the accumulated +responses BEFORE applying a protocol change, or tag each queued response with the protocol version +current when it was produced. The first is smaller; the second is harder to regress. + + +## 1 · SPECIFY — the rules ▸ docs/03-step-1-specify.md + +Feature: +Framings weighed: (chosen) · · +Must: + + - + +Reject: + + - -> "" + +After: + + - + +Assumptions — lowest-confidence first: + + ⚠ — lowest confidence because ; if wrong: + - [ ] — confirm or deny; never carry an open one forward + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +Scenario: + Given + When + Then + And # required for every rejection +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` + body: { } + 200 -> { } + 4xx -> { error: "" | "" } +Schema: +``` + +Status: DRAFT + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: +Plan (one test per scenario, asserting behavior not internals): + + - test_: arrange / act / assert + assert + + +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. + + + +--- + +## 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: + +### GATE RECORD +Outcome: +If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) +Reviewed by: · date: + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): + +### 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`. + +### 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`. + diff --git a/.add/tasks/client-identity-introspection/TASK.md b/.add/tasks/client-identity-introspection/TASK.md index 852624d1..76323b85 100644 --- a/.add/tasks/client-identity-introspection/TASK.md +++ b/.add/tasks/client-identity-introspection/TASK.md @@ -2,7 +2,7 @@ slug: client-identity-introspection · created: 2026-08-09 · stage: production autonomy: auto -phase: ground +phase: done @@ -15,37 +15,199 @@ phase: ground - --- ## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md @@ -53,52 +215,234 @@ Assumptions — lowest-confidence first: ```gherkin -Scenario: - Given - When - Then - And # required for every rejection +Scenario: COMMAND COUNT reports the registry size as an integer + Given a running server + When the client sends COMMAND COUNT + Then the reply is a RESP Integer equal to the registry size + And it is not an Array + +Scenario: bare COMMAND returns one spec per registered command + Given a running server + When the client sends COMMAND + Then the reply is an Array whose length equals the COMMAND COUNT integer + And each element is a 10-element Array whose first field is the lower-cased name + +Scenario: COMMAND INFO answers per requested name, in order + Given a running server + When the client sends COMMAND INFO GET nosuchcmd SET + Then the reply is a 3-element Array + And element 0 names "get", element 1 is Null, element 2 names "set" + And the registry is unchanged + +Scenario: COMMAND GETKEYS extracts keys from the registry key spec + Given a running server + When the client sends COMMAND GETKEYS MSET k1 v1 k2 v2 + Then the reply is the Array [k1, k2] + +Scenario: COMMAND GETKEYS refuses a keyless command + Given a running server + When the client sends COMMAND GETKEYS PING + Then the reply is the error "ERR The command has no key arguments" + And no other reply is emitted + +Scenario: COMMAND COUNT rejects an extra argument + Given a running server + When the client sends COMMAND COUNT extra + Then the reply is "ERR wrong number of arguments for 'command|count' command" + And the connection stays usable + +Scenario: ROLE reports master with its replication offset + Given a server with no master configured + When the client sends ROLE + Then the reply is a 3-element Array starting with "master" + And element 2 is an Array of connected replicas + +Scenario: ROLE reports slave on a replica + Given a server that has been made a replica of another + When the client sends ROLE + Then the reply starts with "slave" and carries the master host and port + And it agrees with the role reported by INFO replication + +Scenario: RESET returns the connection to default state + Given a connection that has sent HELLO 3, SELECT 5, CLIENT SETNAME bob, WATCH k, and MULTI + When the client sends RESET + Then the reply is the Simple String "RESET" + And a subsequent EXEC replies "ERR EXEC without MULTI" + And CLIENT GETNAME replies Null, the selected db is 0, and the protocol is RESP2 + +Scenario: RESET rejects arguments + Given a running server + When the client sends RESET now + Then the reply is "ERR wrong number of arguments for 'reset' command" + And the connection state is unchanged + +Scenario: HELLO agrees with INFO about role and mode + Given a server that has been made a replica of another + When the client sends HELLO 3 + Then the reply map's "role" field is "slave" + And it matches the role: line of INFO replication on the same connection + +Scenario: CLIENT INFO reports the real local port + Given a client connected to the server's listening port + When the client sends CLIENT INFO + Then the laddr field carries that listening port + And it is not port 0 + ``` - - --- ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md ``` - body: { } - 200 -> { } - 4xx -> { error: "" | "" } -Schema: +COMMAND -> Array[N] of spec, N == command_count() +COMMAND COUNT -> Integer(command_count()) +COMMAND LIST -> Array[N] of BulkString(name) +COMMAND INFO ... -> Array[len(names)] of (spec | Null) +COMMAND DOCS [...] -> Array of (BulkString(name), Map(doc)) +COMMAND GETKEYS -> Array of BulkString(key) + errors -> "ERR wrong number of arguments for 'command|count' command" + | "ERR The command has no key arguments" + | "ERR Invalid command specified" + | "ERR Invalid number of arguments specified for command" + +spec := Array[10] [ BulkString(name.to_lowercase()), Integer(arity), + Array(flag SimpleStrings), Integer(first_key), + Integer(last_key), Integer(step), + Array(acl-category SimpleStrings), Array(tips)=[], + Array(key_specs)=[], Array(subcommands)=[] ] + source: COMMAND_META — no second table + +ROLE -> master: Array[3] [ "master", Integer(offset), Array(replica triples) ] + -> replica: Array[5] [ "slave", host, Integer(port), state, Integer(offset) ] + source: ReplicationState.role — the same value build_info_replication reads + +RESET -> SimpleString("RESET") + after: in_multi=false, command_queue cleared, watched_keys cleared, + tracking disabled + untracked, subscriptions exited, selected_db=0, + client_name=None, protocol_version=2 + errors -> "ERR wrong number of arguments for 'reset' command" + +HELLO -> unchanged shape; "role" from ReplicationState.role, + "mode" = "cluster" when cluster_state is enabled else "standalone" + +CLIENT INFO/LIST -> unchanged shape; laddr carries the real local SocketAddr + +Schema: no persistent state, no server-global state. COMMAND_META (static phf) + is read-only. Per-connection: the RESET surface listed above. + +Out of contract (split to `monitor-command-feed`): MONITOR. ``` -Status: DRAFT - +Status: FROZEN @ v2 + +### Amendments v1 -> v2 (recorded, not silently edited) + +**v1 said a replica's HELLO answers `role: slave`. It does not — it answers `role: replica`.** + +Found at build time when ci12 failed against a CORRECT implementation. Rather than edit the test +to agree with my code, I measured a real replica pair on redis-server 8.6.1: + +| surface | word Redis uses for the SAME fact | +|---|---| +| `HELLO` | `role: replica` | +| `INFO replication` | `role:slave` | +| `ROLE` | `slave` | + +Three vocabularies, one fact, deliberately. v1's §1 Must and the §3 contract both stated `slave` +for HELLO, so the frozen contract was factually wrong about the parity target this whole task +exists to hit. Corrected here; `identity::hello_role_and_mode` returns `replica` and +`identity::role` returns `slave`, which is what the measurement shows. + +ci12 was corrected in the same direction and made STRONGER rather than weaker: it now asserts all +three surfaces agree the node is a replica, each in its own vocabulary, plus a negative assertion +that HELLO no longer claims `master`. A test that had been changed to match the implementation +would be the cardinal sin here — the distinction is that independent measurement, not the code, +decided which side was wrong. + +Least-sure flag surfaced at freeze: [contract] the 10-field `COMMAND` spec shape. Redis 7+ emits +10 fields; pre-7 clients expect 6. Emitting 10 with tips/key_specs/subcommands empty is what +redis-server 8.6.1 was measured doing for commands that have none, so it should satisfy both — but +a client that hard-indexes field 7 misreads it, and a subtly-wrong command map is worse than the +current empty array, because a client can detect empty and cannot detect wrong. Confirmed as the +choice at freeze (Tin Dang); the compat harness diffs Moon's reply against redis-server's field by +field so a shape divergence fails a test rather than reaching a driver. + +Second flag, RESOLVED at this freeze rather than carried: [spec] MONITOR was scoped into this task +before §0 showed it has no existing infrastructure, is a stream rather than a reply, is the only +item touching the per-command hot path, and is the only one exposing other clients' credentials. +Split to `monitor-command-feed` (Tin Dang, at freeze). Consequence accepted and recorded in §1. --- ## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md -Coverage target: +Coverage target: one wire-level test per Must and per Reject in §1 (12 scenarios), each asserting +RAW RESP BYTES rather than a client library's rendering — `redis-cli` prints `:0` and `*0` +identically as "0", which is precisely how this defect survived to now. Parity legs on the monoio +and sharded handlers plus the inline fast path. + Plan (one test per scenario, asserting behavior not internals): - - test_: arrange / act / assert + assert + - ci1_command_count_is_an_integer: send COMMAND COUNT / assert the reply byte 0 is b':' and the + value parses > 0 / assert byte 0 is not b'*' + - ci2_bare_command_array_len_matches_count: send COMMAND then COMMAND COUNT / assert the array + header count equals the integer / assert each element is a 10-field array + - ci3_command_info_order_and_null_element: send COMMAND INFO GET nosuchcmd SET / assert *3, + element 1 is exactly b"$-1" or b"_" per protocol / assert elements 0 and 2 name get and set + - ci4_command_getkeys_extracts: send COMMAND GETKEYS MSET k1 v1 k2 v2 / assert [k1, k2] + - ci5_command_getkeys_keyless_rejects: send COMMAND GETKEYS PING / assert the exact error text / + assert a following PING still replies +PONG + - ci6_command_count_arity_rejects: send COMMAND COUNT extra / assert the 'command|count' error + - ci7_command_list_names: send COMMAND LIST / assert the array contains "get" and "reset" + - ci8_role_master_shape: send ROLE / assert *3, "master", integer offset, array of replicas + - ci9_role_replica_agrees_with_info: REPLICAOF a second server / assert ROLE starts "slave" and + its host/port match INFO replication's master_host/master_port + - ci10_reset_returns_default_state: HELLO 3, SELECT 5, CLIENT SETNAME bob, WATCH k, MULTI, RESET + / assert +RESET / assert EXEC errors "without MULTI", CLIENT GETNAME is null, db is 0, and a + bare HELLO now returns a RESP2 array header not a RESP3 map header + - ci11_reset_arity_rejects: send RESET now / assert the arity error / assert MULTI still open + - ci12_hello_role_matches_info: on a replica, HELLO 3 / assert role field is "slave" / assert it + equals INFO replication's role on the same connection + - ci13_client_info_laddr_real_port: connect on a known port / assert laddr ends with that port / + assert it is not ":0" -Tests live in: `./tests/` · MUST run red (missing implementation) before Build. - +Tests live in: `tests/client_identity_introspection.rs` · MUST run red (missing implementation) +before Build. + +### Red run recorded — `cargo test --release --test client_identity_introspection` @ec0c4650 +**0 passed; 11 failed; 2 ignored.** Every failure is the shape defect, not a missing server: + +| test | red reason | +|---|---| +| ci1 | `*0\r\n` where an Integer belongs | +| ci2 | bare `COMMAND` is `:0` — no specs to count | +| ci3 | `*0` instead of one element per requested name | +| ci4 | `*0` — extracts no keys | +| ci5 | `*0` instead of "The command has no key arguments" | +| ci6 | arity unenforced | +| ci7 | `*0` — names nothing | +| ci8 | `-ERR unknown command 'ROLE'` | +| ci10 | `+QUEUED` — see below | +| ci11 | arity never reached (unknown command) | +| ci13 | `laddr=127.0.0.1:0` | + +Two required scrutiny rather than acceptance: +- **ci3 first failed with "server never answered PING"** — a spawn-contention artifact from 11 + servers starting at once, NOT the defect. Re-run alone it fails correctly with `*0` vs `*3`. + A red suite is only evidence if each test is red for ITS OWN reason, so this was checked rather + than counted. +- **ci10 failed with `+QUEUED`, not "unknown command"** — because the test opens MULTI first. Red + either way, but the reason mattered: it exposed that `RESET` must be intercepted BEFORE queueing + (added to §1 Must) and, separately, that Moon queues unknown commands where Redis rejects them + (recorded in §0 as out of scope, owned by `multi-exec-queue-semantics`). + +The RESP reader is deliberately length-aware rather than a single bounded read: the bare `COMMAND` +reply is one element per registered command and spans many reads, so a chunk-reader would assert +against a truncated reply and could go green or red for reasons unrelated to the server. @@ -106,49 +450,121 @@ 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/command/connection.rs` `src/command/metadata.rs` `src/command/mod.rs` +`src/command/introspect.rs` `src/server/conn/core.rs` `src/server/conn/shared.rs` +`src/server/conn/handler_monoio/` `src/server/conn/handler_sharded/` +`src/server/conn/handler_single.rs` `src/client_registry.rs` `src/shard/dispatch.rs` +`src/shard/spsc_handler.rs` `tests/client_identity_introspection.rs` +`scripts/test-consistency.sh` `scripts/test-commands.sh` `CHANGELOG.md` + +Strategy (ordered batches): +1. Red suite first — `tests/client_identity_introspection.rs`, every scenario, raw-RESP + assertions, failing for the right reason (shape, not absence-of-server). +2. `src/command/introspect.rs` (new): build a spec Frame from a `CommandMeta`, and the + `COMMAND` subcommand router. Keeps `connection.rs` from growing a second job and gives the + registry-derived logic one home. +3. Wire `COMMAND` in `dispatch` AND `dispatch_read` AND the inline path — all three, per the + three-dispatch-paths rule. +4. `RESET`: register the arm on all three paths; the state reset itself is one method on + `ConnectionState` so no path can implement a partial reset. +5. `ROLE` + HELLO role/mode from `ConnectionContext.repl_state` / `cluster_state`; delete the + hardcoded literals so there is no second source left to drift. +6. `CLIENT INFO` laddr from the real local address. +7. Consistency/command script entries; CHANGELOG. + +Safety rule (feature-specific): `RESET` must reset EVERY field named in §3, from ONE method on +`ConnectionState` that all three paths call. A per-path reset arm is how this surface drifted in the +first place — a partial reset that leaves tracking registered or the watch set populated is a +correctness bug that presents as a phantom invalidation on the NEXT connection to reuse the id. + +Code lives in: `src/` +Constraints: do NOT change any test or the contract; no new crate; `COMMAND` replies are built on +the cold introspection path — `Vec::with_capacity` for result building is fine there, but nothing +in this task may add work to the per-command hot path; `Frame::Error` in dispatch, never `Result`. --- ## 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 +- [x] all tests pass — 14 scenarios (ci1–ci14). Default suite 32 consecutive green runs at + `--test-threads=13`; the replica pair green under `--ignored`; ci14 green under the tokio + feature set. Full `cargo test --release` and full tokio CI-parity suite re-run after the + `shared.rs` refactor. +- [x] coverage did not decrease — one new test file, no test deleted or weakened. ci12 was + STRENGTHENED mid-build (see the v1→v2 amendment): it now asserts all three role surfaces + agree, plus a negative assertion. +- [x] no test or contract was altered during build — with one recorded exception that went the + OTHER way: §1/§3 were amended at v2 because independent measurement against redis-server + 8.6.1 proved the FROZEN CONTRACT wrong about HELLO's vocabulary, not the code. The test was + made stronger, never matched to the implementation. +- [x] the green was EARNED — proven by one-variable A/Bs, not by reading the diff: + · stub `COMMAND COUNT` → ci2 red. + · remove the `handler_single` ROLE/RESET intercepts → ci14 red with + `-ERR unknown command 'ROLE'`; restore → green. + · revert the `handler_single` HELLO role to the hard-coded `("master","standalone")` → + ci12 STAYED GREEN. That negative result is what exposed the coverage hole below, and is + recorded because it disproved a claim I had already made. +- [x] concurrency / timing of the risky operation is safe — no lock is held across an `.await` + in the new code; `try_handle_reset` takes the tracking and pubsub locks in separate, + non-overlapping scopes. The suite's own flake was root-caused rather than muted (below). +- [x] no exposed secrets, injection openings, or unexpected dependencies — no new dependency. + `COMMAND` replies are derived from a compile-time `phf` map, so no user input reaches them. + RESET's ACL identity is restored through `restore_migrated_state`, the same function + connection setup uses, so it cannot restore MORE privilege than a fresh connection has. +- [x] layering & dependencies follow CONVENTIONS.md — reply construction lives in + `src/command/`, handler wiring in `src/server/conn/`; the shared RESET body is in + `shared.rs` so no handler owns a private copy. +- [ ] a person reviewed and approved the change — PR pending. ### 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 +- [x] `COMMAND COUNT` is an Integer whose value equals the registry size — confirmed on the wire: + `:263`, matching `COMMAND_META`. (§0's claimed 271 was a bad `grep`; corrected on the record.) +- [x] The same four verbs answer identically on the INLINE fast path and the RESP-array path — + confirmed by raw `/dev/tcp` probes sending each form to the same server: `ROLE`, `COMMAND + COUNT`, `RESET` and the arity error are byte-identical either way. This mattered: the inline + path is where the #457 ACL bypass hid. +- [x] A real replica reports itself as a replica in all three of Redis's vocabularies — confirmed + against a live master/replica pair: HELLO `role: replica`, `ROLE` → `slave 127.0.0.1 7401 + connect 0`, `INFO replication` → `role:slave`. +- [x] `RESET` executes immediately inside MULTI instead of queueing — confirmed on the wire: + `MULTI` `+OK`, `RESET` `+RESET`, `EXEC` `-ERR EXEC without MULTI`. +- [x] `RESET` moves the WIRE codec, not just the connection struct — confirmed by `HELLO 3` + then `RESET` then `PING` on one connection: the `PING` reply comes back in RESP2. ### 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] WIRING (code) — this was the deep check that actually earned its keep, and it found a real + gap AFTER I had already claimed the task was wired. Moon has THREE connection handlers plus + an inline fast path. `COMMAND` routes through `dispatch`/`dispatch_read` (shared by all), + but `ROLE` and `RESET` cannot: their answers live on `ConnectionContext`/`ReplicationState`, + not in the `Database` that `dispatch()` receives, so each handler needs its own intercept. + `handler_monoio` and `handler_sharded` were wired; `handler_single` was NOT — it had + neither, and my first patch there hard-coded HELLO's role to `("master","standalone")` with + a comment asserting the path "has no replication state by construction". That comment was + false: `listener.rs:315` passes `Some(rs)`. Confirmed by grepping every call site rather + than trusting the claim. +- [x] REACHABILITY (code) — and the correction to the above: `handler_single` is not reachable + from the shipped binary at all. `main.rs` and `embedded.rs` BOTH call `run_sharded` → + `handler_sharded`; the only caller of `listener::run_with_shutdown` (the sole path to + `handler_single`) is a handful of in-process test suites. So my `--shards 1` tokio probes, + which I had described as exercising `handler_single`, were exercising `handler_sharded`. + The A/B above is what caught this: ci12 stayed green with the bug reinstated, which is only + possible if no spawned-process test reaches that handler. Kept the fix rather than reverting + it — a third copy of this surface silently drifting is the exact disease this task treats — + and closed the blind spot with ci14, which drives `run_with_shutdown` in-process. +- [x] DEAD-CODE (code) — no orphaned symbol. `introspect.rs` and `identity.rs` are both reached + from all wired paths; `PubSubTeardown` has exactly two impls and both are used + (`RwLock` for the two context-based handlers, `Mutex` for `handler_single`). +- [x] SEMANTIC (prose) — `scripts/test-consistency.sh` and `scripts/test-commands.sh` read in + full where edited; the new sections use `$PORT_RUST`, this repo's actual variable, after a + first draft referenced a `$MOON_PORT` that does not exist and would have silently asserted + against an empty string. ### GATE RECORD -Outcome: -If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) -Reviewed by: · date: +Outcome: PASS (pending human review on the PR) +Reviewed by: · date: 2026-08-12 @@ -156,7 +572,9 @@ Reviewed by: · date: ## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md -Watch (reuse scenarios as monitors): +Watch (reuse scenarios as monitors): `COMMAND COUNT` vs registry size (they can only diverge if +someone reintroduces a hand-maintained table); the `role` field across HELLO / `INFO replication` +/ `ROLE` on any node that changes role. ### Spec delta Forward changes for the next loop — each re-enters at Specify as the next task. One line @@ -167,3 +585,30 @@ the retry path (evidence: prod herd spikes)`). See the `add` skill's `deltas.md` 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`. + +- [SPEC · seeded] response batches are encoded in the protocol in effect at FLUSH time, so a + protocol-changing command retro-encodes earlier replies in the same batch. `HELLO 3` alone + returns `%7`; `HELLO 3` + `HELLO 2` in ONE write returns `*14`, the RESP2 flattening. Filed as + task `batch-protocol-version-fidelity` with the full measurement table. Pre-existing — the + `HELLO`+`HELLO` reproducer touches none of this task's code — but RESET, which reverts the + protocol by contract, adds a second trigger, which is how it surfaced here. +- [SPEC · open] `handler_single` is dead weight in the shipped binary (no non-test caller) yet + carries a full command loop that must be kept in parity with two live handlers. Either wire it + to something real or delete it; a third copy nobody executes is where drift hides. This task + paid that tax directly. +- [SPEC · open] `ServerConfig::default()` yields `databases: 0`, because the sane values live in + clap `default_value_t` attributes that apply only to CLI parsing. The handler then indexes + `db[0]` of an empty slice and PANICS on the first command. In-process harnesses hit this; + whether `--databases 0` is rejected at the CLI was not checked. +- [TDD · open] A test can pass for a path it never reaches. ci12 stayed green with the + `handler_single` HELLO bug reinstated, because every spawned-process test in this file reaches + only `handler_sharded`/`handler_monoio`. The A/B, not the green badge, is what proved coverage. + Lesson: for a fix on handler N, revert it and demand a specific RED before believing the green. +- [TDD · open] The suite's own flake was a thundering herd, not a slow server: 13 servers + initialising data dirs at once on an external volume left one unable to answer PING inside 30s + (~1 run in 8). Rejected a timeout bump (slower AND still flaky under heavier load) and a shared + `OnceLock` server (statics are never dropped, so the guard would leak a live moon past exit). + Serialising STARTUP only — test bodies still parallel — took the suite to 32/32 green in ~0.9s. +- [ADD · open] The scope gate is a whole-tree byte snapshot with no git awareness, so unrelated + merges and `target/` rebuilds read as "this task touched it" (7020 files). The task's git file + list is the real evidence; a re-taken baseline proves nothing and was recorded as such. diff --git a/.add/tasks/monitor-command-feed/TASK.md b/.add/tasks/monitor-command-feed/TASK.md new file mode 100644 index 00000000..44e374b4 --- /dev/null +++ b/.add/tasks/monitor-command-feed/TASK.md @@ -0,0 +1,201 @@ +# TASK: MONITOR command feed — stream executed commands, redacted, zero-cost when unattached + +slug: monitor-command-feed · created: 2026-08-11 · stage: production +autonomy: auto +phase: ground + + +> One file = one task. Fill sections top-to-bottom; the `add` skill drives each phase. +> When a phase is unclear, read its book chapter in `.add/docs/` (linked per section). +> The phase marker above is the single source of truth — keep it in sync via `add.py phase`. + +--- + +## 0 · GROUND — the real codebase ▸ docs/02-the-flow.md + +Split out of `client-identity-introspection` at its freeze (2026-08-11). That task measured the +whole identity surface against `redis-server` 8.6.1; MONITOR was carved off because it is a +different animal from the rest. Carrying the measured facts forward so this task does not re-probe: + +Measured on `main` @ec0c4650: `MONITOR` -> `-ERR unknown command 'MONITOR', with args beginning +with:` — absent from dispatch AND from the `COMMAND_META` registry (`src/command/metadata.rs`, +271 entries). Redis 8.6.1 replies `+OK` and then streams the command feed. + +There is NO existing infrastructure to build on: `grep -rn "monitor_feed\|MonitorFeed\|monitors" +src/` returns only `src/cluster/gossip.rs`, which is unrelated (gossip peer monitoring). + +Why it was split rather than kept (the sizing argument, recorded so it is not re-litigated): +- It is a STREAM, not a reply — every other item in that task was a cold introspection response. +- It is the only one touching the per-command hot path, on every shard. +- It is the only one with a security surface: a monitor observes every other client's traffic, + including credential-bearing arguments. Redis redacts AUTH; so must this. + +Touches (files · symbols · signatures): + +Anchors the contract will cite: + +### Design constraints inherited from the split decision +- The feed MUST be gated by a Relaxed atomic subscriber count checked BEFORE any formatting or + allocation, so a server with no monitor attached pays strictly one atomic load per command. + Anything more taxes every command for a feature almost nobody runs. +- Credential-bearing arguments (`AUTH`, `HELLO … AUTH`) must be redacted AT THE POINT OF + FORMATTING — never emitted into a buffer and filtered afterwards. +- `MONITOR` must require the `admin` ACL category; without it, any user could read every other + user's traffic. +- Redis's line format: ` [ ] "CMD" "arg" …`. +- The three-dispatch-paths rule applies (CLAUDE.md): `handler_monoio`, `handler_sharded`, and the + inline fast path, or the command is CI-invisible. + +Consequence accepted at the split: `redis-cli monitor` fails against Moon, and the +v0-9-client-compat goal "a monitoring agent works unmodified" is NOT met, until this task lands. + +--- + +## 1 · SPECIFY — the rules ▸ docs/03-step-1-specify.md + +Feature: +Framings weighed: (chosen) · · +Must: + + - + +Reject: + + - -> "" + +After: + + - + +Assumptions — lowest-confidence first: + + ⚠ — lowest confidence because ; if wrong: + - [ ] — confirm or deny; never carry an open one forward + + + + +--- + +## 2 · SCENARIOS — pass/fail cases ▸ docs/04-step-2-scenarios.md + + + +```gherkin +Scenario: + Given + When + Then + And # required for every rejection +``` + + + + + +--- + +## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md + +``` + body: { } + 200 -> { } + 4xx -> { error: "" | "" } +Schema: +``` + +Status: DRAFT + + +--- + +## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md + +Coverage target: +Plan (one test per scenario, asserting behavior not internals): + + - test_: arrange / act / assert + assert + + +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. + + + +--- + +## 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: + +### GATE RECORD +Outcome: +If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) +Reviewed by: · date: + + + +--- + +## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md + +Watch (reuse scenarios as monitors): + +### 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`. + +### 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`. + diff --git a/CHANGELOG.md b/CHANGELOG.md index 47654306..20aa63be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **`ROLE`, `RESET`, and a real `COMMAND` introspection surface.** `COMMAND` and `COMMAND COUNT` + each returned the OTHER'S RESP TYPE — bare `COMMAND` replied `:0` (an Integer where an Array + belongs) and `COMMAND COUNT` replied `*0` (an Array where an Integer belongs); `COMMAND + INFO`/`DOCS`/`LIST`/`GETKEYS` all replied an empty array. A driver that builds its command map at + connect time does not read that as "unsupported", it reads it as a protocol violation, and + `redis-cli` renders `:0` and `*0` identically as "0" so it never showed up by eye. All six now + derive from `COMMAND_META`, so registering a command is what makes it introspectable and there is + no second table to drift. `ROLE` and `RESET` were unknown commands: `RESET` was registered in the + metadata table with full flags while dispatch rejected it (the same advertise-then-reject class as + WATCH/UNWATCH before v0.8.6), and a partial `RESET` existed only inside `handler_sharded`'s + subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere + else. Both are now wired on every production path, and on `handler_single` (the in-process + single-shard handler that only tests reach) so the third copy of this surface cannot drift + unnoticed the way it just did. + +### Fixed +- **`HELLO` no longer contradicts `INFO replication` about what the server is.** `hello_acl` built + its reply with `mode` and `standalone` and `role` and `master` as compile-time literals, so a + replica told `HELLO` it was a master while telling `INFO` it was a slave — on the same connection. + Both fields now read `ReplicationState`/`ClusterState`, the same source `INFO` uses. Note that + Redis deliberately uses three vocabularies for this one fact (`HELLO` -> `replica`, `INFO` -> + `slave`, `ROLE` -> `slave`), verified against a live replica pair on redis-server 8.6.1, and Moon + now matches each. +- **`CLIENT INFO` / `CLIENT LIST` report the real `laddr`.** The field was the literal + `laddr=127.0.0.1:0` inside the format string, so every client on every listener reported port 0 — + a monitoring agent using `laddr` to tell listeners apart saw nothing. - **CI now tests the runtime Moon actually ships (`check-monoio`).** Every CI job that *executed* tests did so under `--no-default-features --features runtime-tokio,…`, while Moon's default feature set — and what ships on Linux — is `runtime-monoio`. 26 monoio integration test files and diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index a105ae0d..e33fdad9 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -675,6 +675,19 @@ if should_run "connection"; then assert_moon_ok "DBSIZE" DBSIZE assert_moon_ok "COMMAND" COMMAND assert_moon_ok "COMMAND COUNT" COMMAND COUNT + # `assert_moon_ok` passed on the old stub too — it replied `*0`, which is a + # perfectly well-formed reply of the WRONG TYPE. These assert content, so a + # regression to a stub cannot slip past as "ok". + assert_moon_contains "COMMAND INFO GET names the command" "get" COMMAND INFO GET + assert_moon_contains "COMMAND LIST includes reset" "reset" COMMAND LIST + assert_moon_contains "COMMAND GETKEYS extracts the key" "k1" COMMAND GETKEYS MSET k1 v1 k2 v2 + assert_moon_contains "COMMAND GETKEYS rejects keyless" "no key arguments" COMMAND GETKEYS PING + assert_moon_contains "COMMAND COUNT arity" "wrong number of arguments" COMMAND COUNT extra + assert_match "ROLE" ROLE + assert_moon_contains "ROLE reports master" "master" ROLE + assert_match "RESET" RESET + assert_moon_contains "RESET arity" "wrong number of arguments" RESET now + assert_moon_contains "CLIENT INFO laddr is not port 0" "laddr=127.0.0.1:$PORT_RUST" CLIENT INFO assert_moon_contains "MEMORY DOCTOR" "Per-subsystem (resident):" MEMORY DOCTOR fi diff --git a/scripts/test-consistency.sh b/scripts/test-consistency.sh index 40a715e2..bc2d8286 100755 --- a/scripts/test-consistency.sh +++ b/scripts/test-consistency.sh @@ -660,6 +660,70 @@ assert_eq "UNWATCH releases the dependency" \ assert_both "WATCH arity" WATCH assert_both "UNWATCH outside MULTI" UNWATCH +# =========================================================================== +# Identity / introspection (COMMAND, ROLE, RESET) +# =========================================================================== +log "=== IDENTITY/INTROSPECTION ===" + +# These compare REPLY SHAPE against Redis, not reply content: Moon registers a +# different command set than Redis, so `COMMAND COUNT` legitimately differs in +# value while its TYPE must not. The old bug was a type inversion — bare +# COMMAND replied an Integer and COMMAND COUNT replied an Array, each the +# other's type — which `redis-cli` renders identically as "0". Comparing +# rendered text here would have shown a false match, so shape is derived from +# the reply itself. + +# Integer-typed and positive on both servers. +count_is_positive_int() { + local port="$1" v + v="$(redis-cli -p "$port" COMMAND COUNT 2>&1)" + [[ "$v" =~ ^[0-9]+$ ]] && [[ "$v" -gt 0 ]] && echo "int>0" || echo "NOT-AN-INT:$v" +} +assert_eq "COMMAND COUNT is a positive integer" \ + "$(count_is_positive_int "$PORT_REDIS")" "$(count_is_positive_int "$PORT_RUST")" + +# The bare COMMAND array must hold exactly COMMAND COUNT entries on each server. +count_matches_specs() { + local port="$1" n specs + n="$(redis-cli -p "$port" COMMAND COUNT 2>&1)" + # One line per top-level spec: count the lines that open a spec's name. + specs="$(redis-cli -p "$port" COMMAND LIST 2>&1 | grep -c .)" + [[ "$n" == "$specs" ]] && echo "match" || echo "mismatch($n vs $specs)" +} +assert_eq "COMMAND COUNT equals COMMAND LIST length" \ + "$(count_matches_specs "$PORT_REDIS")" "$(count_matches_specs "$PORT_RUST")" + +assert_both "COMMAND GETKEYS extracts keys" COMMAND GETKEYS MSET ik1 v1 ik2 v2 +assert_both "COMMAND GETKEYS rejects a keyless command" COMMAND GETKEYS PING +assert_both "COMMAND COUNT arity" COMMAND COUNT extra +assert_both "COMMAND INFO unknown name" COMMAND INFO definitely-not-a-command + +# ROLE on a standalone master is byte-identical between the two. +assert_both "ROLE on a master" ROLE +assert_both "RESET replies +RESET" RESET +assert_both "RESET arity" RESET now + +# RESET must return the connection to default state, and it must do so INSIDE +# MULTI (measured on redis 8.6.1: executed immediately, transaction discarded) +# rather than being queued. Needs one held-open connection, same /dev/tcp +# technique as the WATCH tests above. +reset_state_outcome() { + local port="$1" line="" out="" + exec 3<>"/dev/tcp/127.0.0.1/${port}" || { echo "__CONNECT_FAILED__"; return 0; } + printf 'SELECT 5\r\nCLIENT SETNAME probe\r\nMULTI\r\nRESET\r\n' >&3 + # EXEC after RESET must fail: the transaction is gone. + printf 'EXEC\r\nCLIENT GETNAME\r\nECHO reset-done\r\n' >&3 + while IFS= read -r -t 5 line <&3; do + line="${line%$'\r'}" + [[ "$line" == *"without MULTI"* ]] && out="${out}exec-refused;" + [[ "$line" == "reset-done" ]] && break + done + exec 3>&- + echo "${out:-no-refusal}" +} +assert_eq "RESET inside MULTI discards the transaction" \ + "$(reset_state_outcome "$PORT_REDIS")" "$(reset_state_outcome "$PORT_RUST")" + # =========================================================================== # SWAPDB consistency # =========================================================================== diff --git a/src/client_registry.rs b/src/client_registry.rs index 2d9987fe..86f4c8ac 100644 --- a/src/client_registry.rs +++ b/src/client_registry.rs @@ -225,6 +225,10 @@ impl Drop for BlockedGuard { pub struct ClientEntry { pub id: u64, pub addr: String, + /// Local (server-side) address this client connected to. Was hardcoded + /// `127.0.0.1:0` in the CLIENT LIST format string; a monitoring agent + /// reading `laddr` to tell listeners apart saw port 0 for every client. + pub laddr: String, pub name: Option, pub user: String, pub shard: usize, @@ -289,6 +293,7 @@ impl ClientFlags { pub fn register( id: u64, addr: String, + laddr: String, user: String, shard: usize, kill_fd: i32, @@ -307,6 +312,7 @@ pub fn register( let entry = ClientEntry { id, addr, + laddr, name: None, user, shard, @@ -678,7 +684,7 @@ pub fn parse_kill_args(args: &[&[u8]]) -> Option { /// Field set matches real Redis (`clientCommand`/`catClientInfoString`) so /// tooling that parses CLIENT LIST by key (redis-cli, client libraries, /// `redis_exporter`) doesn't choke on missing keys. Fields Moon doesn't -/// track yet (`laddr`, `rbs`/`rbp`/`obl`/`oll`/`omem`, `tot-net-in/out`, +/// track yet (`rbs`/`rbp`/`obl`/`oll`/`omem`, `tot-net-in/out`, /// `cmd`, `events`) are emitted with honest placeholder values (0 / "NULL" / /// unknown) rather than omitted — see docs/redis-compat.md for the list of /// fields that are structurally present but not yet semantically populated. @@ -706,11 +712,23 @@ fn format_client_line(buf: &mut String, entry: &ClientEntry, now: Instant) { let tot_net_out = live.tot_net_out.load(Ordering::Relaxed); let _ = writeln!( buf, - "id={} addr={} laddr=127.0.0.1:0 fd=0 name={} age={} idle={} flags={} db={} \ + "id={} addr={} laddr={} fd=0 name={} age={} idle={} flags={} db={} \ sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 \ tot-net-in=0 tot-net-out={} rbs=1024 rbp=0 obl={} oll=0 omem={} tot-mem={} events=r \ cmd=NULL user={} redir=-1 resp=2 lib-name= lib-ver=", - entry.id, entry.addr, name, age, idle, flags, db, tot_net_out, omem, omem, omem, entry.user, + entry.id, + entry.addr, + entry.laddr, + name, + age, + idle, + flags, + db, + tot_net_out, + omem, + omem, + omem, + entry.user, ); } @@ -721,7 +739,14 @@ mod tests { #[test] fn test_register_and_list() { let id = 999_000; - register(id, "127.0.0.1:12345".into(), "default".into(), 0, -1); + register( + id, + "127.0.0.1:12345".into(), + "127.0.0.1:6379".into(), + "default".into(), + 0, + -1, + ); let list = client_list(); assert!(list.contains("id=999000")); assert!(list.contains("addr=127.0.0.1:12345")); @@ -734,7 +759,14 @@ mod tests { #[test] fn test_client_info() { let id = 999_001; - register(id, "10.0.0.1:5000".into(), "alice".into(), 1, -1); + register( + id, + "10.0.0.1:5000".into(), + "127.0.0.1:6379".into(), + "alice".into(), + 1, + -1, + ); let info = client_info(id); assert!(info.is_some()); assert!(info.as_ref().is_some_and(|s| s.contains("user=alice"))); @@ -748,7 +780,14 @@ mod tests { #[test] fn test_client_info_field_set_matches_redis() { let id = 999_030; - register(id, "10.0.0.3:7000".into(), "carol".into(), 0, -1); + register( + id, + "10.0.0.3:7000".into(), + "127.0.0.1:6379".into(), + "carol".into(), + 0, + -1, + ); let info = client_info(id).expect("registered client has info"); for field in [ "id=", @@ -796,7 +835,14 @@ mod tests { #[test] fn test_kill_by_id() { let id = 999_002; - let live = register(id, "10.0.0.2:6000".into(), "bob".into(), 0, -1); + let live = register( + id, + "10.0.0.2:6000".into(), + "127.0.0.1:6379".into(), + "bob".into(), + 0, + -1, + ); assert!(!is_killed(id)); assert!(!live.is_killed()); let count = kill_clients(&KillFilter::Id(id), None); @@ -822,6 +868,7 @@ mod tests { let live = register( id, "unix:socketpair".into(), + "127.0.0.1:6379".into(), "default".into(), 0, killed_end.as_raw_fd(), @@ -855,6 +902,7 @@ mod tests { let live = register( id, "unix:socketpair-self".into(), + "127.0.0.1:6379".into(), "default".into(), 0, self_end.as_raw_fd(), @@ -882,8 +930,22 @@ mod tests { fn test_kill_by_user() { let id1 = 999_010; let id2 = 999_011; - register(id1, "10.0.0.3:7000".into(), "eve".into(), 0, -1); - register(id2, "10.0.0.4:7001".into(), "eve".into(), 1, -1); + register( + id1, + "10.0.0.3:7000".into(), + "127.0.0.1:6379".into(), + "eve".into(), + 0, + -1, + ); + register( + id2, + "10.0.0.4:7001".into(), + "127.0.0.1:6379".into(), + "eve".into(), + 1, + -1, + ); let count = kill_clients(&KillFilter::User("eve".into()), None); assert_eq!(count, 2); assert!(is_killed(id1)); @@ -895,7 +957,14 @@ mod tests { #[test] fn test_update_and_touch() { let id = 999_003; - let live = register(id, "10.0.0.5:8000".into(), "default".into(), 0, -1); + let live = register( + id, + "10.0.0.5:8000".into(), + "127.0.0.1:6379".into(), + "default".into(), + 0, + -1, + ); update(id, |e| { e.name = Some("myconn".into()); }); @@ -909,7 +978,14 @@ mod tests { #[test] fn test_touch_uses_caller_clock_not_instant_now() { let id = 999_004; - let live = register(id, "10.0.0.6:9000".into(), "default".into(), 0, -1); + let live = register( + id, + "10.0.0.6:9000".into(), + "127.0.0.1:6379".into(), + "default".into(), + 0, + -1, + ); // touch takes the caller's (shard-cached) epoch clock — no per-op // Instant::now(). last_cmd_ms stays "ms since connect". live.touch(2, ClientFlags::default(), live.connected_at_epoch_ms + 5000); @@ -960,7 +1036,14 @@ mod striping_tests { (0..n) .map(|i| { let id = BASE + i; - let _ = register(id, format!("10.0.0.{i}:1234"), user.to_string(), 0, -1); + let _ = register( + id, + format!("10.0.0.{i}:1234"), + "127.0.0.1:6379".into(), + user.to_string(), + 0, + -1, + ); id }) .collect() @@ -1050,7 +1133,14 @@ mod idle_timeout_tests { /// (`SHARD_CONNS` is unset in unit tests, so the per-shard counters are /// inert for these ids.) fn client(id: u64, shard: usize) -> Arc { - register(id, format!("127.0.0.1:{id}"), "default".into(), shard, -1) + register( + id, + format!("127.0.0.1:{id}"), + "127.0.0.1:6379".into(), + "default".into(), + shard, + -1, + ) } /// `kill_fd` is -1 here, so `force_close_fd` is a no-op and the observable @@ -1319,9 +1409,23 @@ mod idle_timeout_tests { let mut clean_attempt_seen = false; for attempt in 0..5u64 { let id = 9_060 + attempt; - let _a = register(id, "t:1".into(), "default".into(), 908, -1); + let _a = register( + id, + "t:1".into(), + "127.0.0.1:6379".into(), + "default".into(), + 908, + -1, + ); let t1 = TOTAL_CLIENTS.load(Ordering::Relaxed); - let _b = register(id, "t:1".into(), "default".into(), 909, -1); + let _b = register( + id, + "t:1".into(), + "127.0.0.1:6379".into(), + "default".into(), + 909, + -1, + ); let t2 = TOTAL_CLIENTS.load(Ordering::Relaxed); deregister(id); assert!(live_handle(id).is_none(), "single deregister must clear"); diff --git a/src/command/connection.rs b/src/command/connection.rs index d9522438..80e1027f 100644 --- a/src/command/connection.rs +++ b/src/command/connection.rs @@ -638,6 +638,11 @@ pub fn hello_acl( client_id: u64, acl_table: &std::sync::Arc>, authenticated: &mut bool, + // (role, mode) read from ReplicationState/ClusterState by the caller. + // Previously these were `Bytes::from_static` literals — "master" and + // "standalone" — so a replica told HELLO it was a master while telling + // INFO replication it was a slave, on the same connection. + role_mode: (&'static str, &'static str), ) -> (Frame, u8, Option, Option) { let mut proto = current_proto; let mut client_name: Option = None; @@ -778,11 +783,11 @@ pub fn hello_acl( ), ( Frame::BulkString(Bytes::from_static(b"mode")), - Frame::BulkString(Bytes::from_static(b"standalone")), + Frame::BulkString(Bytes::from_static(role_mode.1.as_bytes())), ), ( Frame::BulkString(Bytes::from_static(b"role")), - Frame::BulkString(Bytes::from_static(b"master")), + Frame::BulkString(Bytes::from_static(role_mode.0.as_bytes())), ), ( Frame::BulkString(Bytes::from_static(b"modules")), @@ -925,6 +930,9 @@ pub fn hello( client_id: u64, requirepass: &Option, authenticated: &mut bool, + // Same (role, mode) contract as `hello_acl`: kept in lockstep so the two + // HELLO variants cannot report different identities. + role_mode: (&'static str, &'static str), ) -> (Frame, u8, Option) { let mut proto = current_proto; let mut client_name: Option = None; @@ -1020,11 +1028,11 @@ pub fn hello( ), ( Frame::BulkString(Bytes::from_static(b"mode")), - Frame::BulkString(Bytes::from_static(b"standalone")), + Frame::BulkString(Bytes::from_static(role_mode.1.as_bytes())), ), ( Frame::BulkString(Bytes::from_static(b"role")), - Frame::BulkString(Bytes::from_static(b"master")), + Frame::BulkString(Bytes::from_static(role_mode.0.as_bytes())), ), ( Frame::BulkString(Bytes::from_static(b"modules")), @@ -1229,7 +1237,7 @@ mod tests { #[test] fn test_hello_no_args_returns_current_proto() { let mut auth = true; - let (resp, proto, name) = hello(&[], 2, 1, &None, &mut auth); + let (resp, proto, name) = hello(&[], 2, 1, &None, &mut auth, ("master", "standalone")); assert!(matches!(resp, Frame::Map(_))); assert_eq!(get_proto_from_hello_response(&resp), Some(2)); assert_eq!(proto, 2); @@ -1245,6 +1253,7 @@ mod tests { 1, &None, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 3); assert_eq!(get_proto_from_hello_response(&resp), Some(3)); @@ -1259,6 +1268,7 @@ mod tests { 1, &None, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 2); assert_eq!(get_proto_from_hello_response(&resp), Some(2)); @@ -1279,6 +1289,7 @@ mod tests { 1, &pass, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 3); assert!(matches!(resp, Frame::Map(_))); @@ -1300,6 +1311,7 @@ mod tests { 1, &pass, &mut auth, + ("master", "standalone"), ); // Auth failed: proto stays at current, response is error assert_eq!(proto, 2); @@ -1320,6 +1332,7 @@ mod tests { 1, &None, &mut auth, + ("master", "standalone"), ); assert_eq!(name, Some(Bytes::from_static(b"myclient"))); } @@ -1333,6 +1346,7 @@ mod tests { 1, &None, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 2); // unchanged assert!(matches!(resp, Frame::Error(ref s) if s.starts_with(b"NOPROTO"))); @@ -1581,7 +1595,8 @@ mod tests { fn test_hello_acl_no_args() { let table = make_acl_table(); let mut auth = true; - let (resp, proto, name, user) = hello_acl(&[], 2, 1, &table, &mut auth); + let (resp, proto, name, user) = + hello_acl(&[], 2, 1, &table, &mut auth, ("master", "standalone")); assert!(matches!(resp, Frame::Map(_))); assert_eq!(proto, 2); assert!(name.is_none()); @@ -1603,6 +1618,7 @@ mod tests { 1, &table, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 3); assert!(matches!(resp, Frame::Map(_))); @@ -1625,6 +1641,7 @@ mod tests { 1, &table, &mut auth, + ("master", "standalone"), ); assert_eq!(proto, 2); // unchanged assert!(matches!(resp, Frame::Error(ref s) if s.starts_with(b"WRONGPASS"))); diff --git a/src/command/identity.rs b/src/command/identity.rs new file mode 100644 index 00000000..e7d21805 --- /dev/null +++ b/src/command/identity.rs @@ -0,0 +1,124 @@ +//! `ROLE` and the connection-state half of `RESET`. +//! +//! Both exist to stop the server contradicting itself. Before this module, +//! `HELLO` reported `role: master` from a `Bytes::from_static` literal while +//! `INFO replication` reported `role:slave` from `ReplicationState` on the same +//! connection — two sources of truth for one fact, which is the defect class +//! this task closes. Everything here reads `ReplicationState`, the same value +//! `build_info_replication` reads. + +use std::sync::atomic::Ordering; + +use bytes::Bytes; +use parking_lot::RwLock; + +use crate::framevec; +use crate::protocol::Frame; +use crate::replication::state::{ReplicaHandshakeState, ReplicationRole, ReplicationState}; + +/// `ROLE` — `[master, , [[ip, port, offset], …]]` on a master, +/// `[slave, , , , ]` on a replica. +/// +/// With replication disabled entirely there is no `ReplicationState`; the +/// honest answer is still "master with no replicas", which is what a +/// standalone redis-server reports. +pub fn role(repl_state: Option<&std::sync::Arc>>) -> Frame { + let Some(state) = repl_state else { + return Frame::Array(framevec![ + Frame::BulkString(Bytes::from_static(b"master")), + Frame::Integer(0), + Frame::Array(framevec![]), + ]); + }; + let s = state.read(); + match &s.role { + ReplicationRole::Master => { + let offset = s.master_repl_offset.load(Ordering::Relaxed) as i64; + let mut replicas = crate::protocol::FrameVec::with_capacity(s.replicas.len()); + for r in &s.replicas { + let ack: u64 = r + .ack_offsets + .iter() + .map(|a| a.load(Ordering::Relaxed)) + .sum(); + // Redis emits ip, port and offset as bulk strings here — not as + // an integer for the port — so a client parsing positionally + // gets the types it expects. + replicas.push(Frame::Array(framevec![ + Frame::BulkString(Bytes::from(r.addr.ip().to_string())), + Frame::BulkString(Bytes::from(r.addr.port().to_string())), + Frame::BulkString(Bytes::from(ack.to_string())), + ])); + } + Frame::Array(framevec![ + Frame::BulkString(Bytes::from_static(b"master")), + Frame::Integer(offset), + Frame::Array(replicas), + ]) + } + ReplicationRole::Replica { host, port, state } => { + // The link state string is the same vocabulary INFO uses, so the + // two cannot disagree about whether the link is up. + let link = match state { + ReplicaHandshakeState::Streaming => "connected", + _ => "connect", + }; + let offset = s.master_repl_offset.load(Ordering::Relaxed) as i64; + Frame::Array(framevec![ + Frame::BulkString(Bytes::from_static(b"slave")), + Frame::BulkString(Bytes::from(host.clone())), + Frame::Integer(*port as i64), + Frame::BulkString(Bytes::from_static(link.as_bytes())), + Frame::Integer(offset), + ]) + } + } +} + +/// The `role` and `mode` fields HELLO reports, read from real state rather than +/// hardcoded. Returned as `&'static str` so the HELLO builder stays allocation +/// free. +pub fn hello_role_and_mode( + repl_state: Option<&std::sync::Arc>>, + cluster_enabled: bool, +) -> (&'static str, &'static str) { + let role = match repl_state { + Some(s) => match s.read().role { + ReplicationRole::Master => "master", + ReplicationRole::Replica { .. } => "replica", + }, + None => "master", + }; + let mode = if cluster_enabled { + "cluster" + } else { + "standalone" + }; + (role, mode) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn role_without_replication_is_a_standalone_master() { + let Frame::Array(f) = role(None) else { + panic!("ROLE must be an array"); + }; + assert_eq!(f.len(), 3); + assert!(matches!(&f[0], Frame::BulkString(b) if b.as_ref() == b"master")); + assert!(matches!(f[1], Frame::Integer(0))); + assert!(matches!(&f[2], Frame::Array(r) if r.is_empty())); + } + + #[test] + fn hello_reports_standalone_master_by_default() { + assert_eq!(hello_role_and_mode(None, false), ("master", "standalone")); + } + + #[test] + fn hello_reports_cluster_mode_when_clustered() { + assert_eq!(hello_role_and_mode(None, true).1, "cluster"); + } +} diff --git a/src/command/introspect.rs b/src/command/introspect.rs new file mode 100644 index 00000000..fe0aa80c --- /dev/null +++ b/src/command/introspect.rs @@ -0,0 +1,385 @@ +//! `COMMAND` — the introspection surface, rendered from `COMMAND_META`. +//! +//! This module exists because the previous implementation +//! (`connection::command`, sixteen lines) answered from constants: bare +//! `COMMAND` replied `Integer(0)` and `COMMAND COUNT` replied an empty array — +//! each returning the OTHER'S RESP type. A driver that builds its command map +//! at startup does not read that as "unsupported"; it reads it as a protocol +//! violation. +//! +//! Everything here derives from `COMMAND_META`, deliberately. The registry +//! already carries name, arity, flags, key positions and ACL categories, so +//! registering a command is what makes it introspectable — there is no second +//! table to keep in sync, which is the drift this task exists to close. +//! +//! These are cold paths: a client calls them at connect time, not per +//! operation. Building result vectors with `Vec::with_capacity` here is fine +//! and is what the allocation rule permits for result building. + +use bytes::Bytes; + +use crate::command::metadata::{AclCategories, COMMAND_META, CommandFlags, CommandMeta}; +use crate::framevec; +use crate::protocol::Frame; + +/// Flag name as Redis spells it on the wire, paired with its bit. +const FLAG_NAMES: &[(CommandFlags, &str)] = &[ + (CommandFlags::WRITE, "write"), + (CommandFlags::READONLY, "readonly"), + (CommandFlags::FAST, "fast"), + (CommandFlags::ADMIN, "admin"), + (CommandFlags::PUBSUB, "pubsub"), + (CommandFlags::NOSCRIPT, "noscript"), + (CommandFlags::LOADING, "loading"), + (CommandFlags::STALE, "stale"), + (CommandFlags::SKIP_MONITOR, "skip_monitor"), + (CommandFlags::ASKING, "asking"), + (CommandFlags::NO_AUTH, "no-auth"), + (CommandFlags::MAY_REPLICATE, "may_replicate"), + (CommandFlags::SORT_FOR_SCRIPT, "sort_for_script"), +]; + +/// ACL category name as Redis spells it (`@`-prefixed on the wire). +const CATEGORY_NAMES: &[(AclCategories, &str)] = &[ + (AclCategories::KEYSPACE, "@keyspace"), + (AclCategories::READ_CAT, "@read"), + (AclCategories::WRITE_CAT, "@write"), + (AclCategories::SET, "@set"), + (AclCategories::SORTEDSET, "@sortedset"), + (AclCategories::LIST, "@list"), + (AclCategories::HASH, "@hash"), + (AclCategories::STRING, "@string"), + (AclCategories::STREAM, "@stream"), + (AclCategories::PUBSUB, "@pubsub"), + (AclCategories::GENERIC, "@generic"), + (AclCategories::TRANSACTIONS, "@transaction"), + (AclCategories::SCRIPTING, "@scripting"), + (AclCategories::CONNECTION, "@connection"), + (AclCategories::SERVER, "@server"), + (AclCategories::DANGEROUS, "@dangerous"), + (AclCategories::SLOW, "@slow"), + (AclCategories::FAST_CAT, "@fast"), + (AclCategories::SEARCH, "@search"), + (AclCategories::GRAPH, "@graph"), +]; + +/// The 10-field spec Redis 7+ emits. +/// +/// The last three (tips, key_specs, subcommands) are emitted EMPTY rather than +/// omitted: that is what redis-server itself does for a command that has none, +/// and a client indexing by position needs the field to exist. Emitting six +/// fields would make every modern driver's key-spec lookup fall off the end. +fn spec_frame(meta: &CommandMeta) -> Frame { + let mut flags = crate::protocol::FrameVec::new(); + for (bit, name) in FLAG_NAMES { + if meta.flags.contains(*bit) { + flags.push(Frame::SimpleString(Bytes::from_static(name.as_bytes()))); + } + } + let mut cats = crate::protocol::FrameVec::new(); + for (bit, name) in CATEGORY_NAMES { + if meta.acl_categories.contains(*bit) { + cats.push(Frame::SimpleString(Bytes::from_static(name.as_bytes()))); + } + } + Frame::Array(framevec![ + Frame::BulkString(Bytes::from(meta.name.to_ascii_lowercase())), + Frame::Integer(meta.arity as i64), + Frame::Array(flags), + Frame::Integer(meta.first_key as i64), + Frame::Integer(meta.last_key as i64), + Frame::Integer(meta.step as i64), + Frame::Array(cats), + Frame::Array(framevec![]), // tips + Frame::Array(framevec![]), // key specs + Frame::Array(framevec![]), // subcommands + ]) +} + +/// Every registered command's spec. Order is the registry's iteration order, +/// which Redis does not promise either. +fn all_specs() -> crate::protocol::FrameVec { + let mut out = crate::protocol::FrameVec::with_capacity(COMMAND_META.len()); + for meta in COMMAND_META.values() { + out.push(spec_frame(meta)); + } + out +} + +fn err(msg: &'static str) -> Frame { + Frame::Error(Bytes::from_static(msg.as_bytes())) +} + +/// Extract the key arguments of `argv` using the registry's key spec. +/// +/// `argv[0]` is the command name. Returns an error Frame rather than an empty +/// array when the command has no keys — "no keys" and "I did not understand +/// you" must not look identical to a cluster-aware client deciding where to +/// route a command. +fn getkeys(argv: &[Bytes]) -> Frame { + let Some(name) = argv.first() else { + return err("ERR Unknown subcommand or wrong number of arguments for 'GETKEYS'"); + }; + // `lookup` uppercases internally — no need to allocate an upper copy. + let Some(meta) = crate::command::metadata::lookup(name) else { + return err("ERR Invalid command specified"); + }; + + // Arity: positive = exact, negative = minimum. argv includes the name. + let n = argv.len() as i16; + let arity_ok = if meta.arity >= 0 { + n == meta.arity + } else { + n >= -meta.arity + }; + if !arity_ok { + return err("ERR Invalid number of arguments specified for command"); + } + + if meta.first_key <= 0 { + return err("ERR The command has no key arguments"); + } + + let last = if meta.last_key < 0 { + // -1 means "through the last argument"; -2 means "through the + // second-to-last", and so on. + (n + meta.last_key) as usize + } else { + meta.last_key as usize + }; + let step = if meta.step <= 0 { + 1 + } else { + meta.step as usize + }; + + let mut keys = crate::protocol::FrameVec::new(); + let mut i = meta.first_key as usize; + while i <= last && i < argv.len() { + keys.push(Frame::BulkString(argv[i].clone())); + i += step; + } + if keys.is_empty() { + return err("ERR The command has no key arguments"); + } + Frame::Array(keys) +} + +/// Minimal but SHAPE-correct docs: name followed by a map. Redis clients parse +/// the shape to build help/command maps; thin summary text is acceptable, a +/// wrong shape is not. +fn docs_for(meta: &CommandMeta) -> (Frame, Frame) { + let arity_note = if meta.arity < 0 { + format!("{} (variadic, minimum {})", meta.name, -meta.arity) + } else { + format!("{} (arity {})", meta.name, meta.arity) + }; + ( + Frame::BulkString(Bytes::from(meta.name.to_ascii_lowercase())), + Frame::Map(vec![ + ( + Frame::BulkString(Bytes::from_static(b"summary")), + Frame::BulkString(Bytes::from(arity_note)), + ), + ( + Frame::BulkString(Bytes::from_static(b"since")), + Frame::BulkString(Bytes::from_static(b"1.0.0")), + ), + ( + Frame::BulkString(Bytes::from_static(b"arity")), + Frame::Integer(meta.arity as i64), + ), + ]), + ) +} + +fn extract(f: &Frame) -> Option { + match f { + Frame::BulkString(b) | Frame::SimpleString(b) => Some(b.clone()), + _ => None, + } +} + +/// `COMMAND` and its subcommands, answered from the registry. +pub fn command(args: &[Frame]) -> Frame { + // Bare COMMAND: one spec per registered command. + let Some(sub) = args.first().and_then(extract) else { + return Frame::Array(all_specs()); + }; + + if sub.eq_ignore_ascii_case(b"COUNT") { + if args.len() != 1 { + return err("ERR wrong number of arguments for 'command|count' command"); + } + return Frame::Integer(crate::command::metadata::command_count() as i64); + } + + if sub.eq_ignore_ascii_case(b"LIST") { + // FILTERBY is not supported; reject rather than silently ignore the + // filter and hand back the unfiltered list as if it had been applied. + if args.len() != 1 { + return err("ERR Unknown subcommand or wrong number of arguments for 'LIST'"); + } + let mut out = crate::protocol::FrameVec::with_capacity(COMMAND_META.len()); + for meta in COMMAND_META.values() { + out.push(Frame::BulkString(Bytes::from( + meta.name.to_ascii_lowercase(), + ))); + } + return Frame::Array(out); + } + + if sub.eq_ignore_ascii_case(b"INFO") { + // No names => every command, same as bare COMMAND. + if args.len() == 1 { + return Frame::Array(all_specs()); + } + let mut out = crate::protocol::FrameVec::with_capacity(args.len() - 1); + for a in &args[1..] { + let spec = extract(a) + .and_then(|n| crate::command::metadata::lookup(&n)) + .map(spec_frame) + // An unknown name is a NULL ELEMENT inside the array, not a + // skipped entry: the reply is positional, so dropping it would + // silently misalign every name after it. + .unwrap_or(Frame::Null); + out.push(spec); + } + return Frame::Array(out); + } + + if sub.eq_ignore_ascii_case(b"DOCS") { + let mut out = crate::protocol::FrameVec::new(); + if args.len() == 1 { + out.reserve(COMMAND_META.len() * 2); + for meta in COMMAND_META.values() { + let (n, d) = docs_for(meta); + out.push(n); + out.push(d); + } + } else { + for a in &args[1..] { + if let Some(meta) = extract(a).and_then(|n| crate::command::metadata::lookup(&n)) { + let (n, d) = docs_for(meta); + out.push(n); + out.push(d); + } + // Redis omits unknown names from DOCS entirely (unlike INFO, + // which is positional and uses a null element). + } + } + return Frame::Array(out); + } + + if sub.eq_ignore_ascii_case(b"GETKEYS") { + if args.len() < 2 { + return err("ERR Unknown subcommand or wrong number of arguments for 'GETKEYS'"); + } + let argv: Vec = args[1..].iter().filter_map(extract).collect(); + return getkeys(&argv); + } + + Frame::Error(Bytes::from(format!( + "ERR Unknown subcommand '{}'. Try COMMAND HELP.", + String::from_utf8_lossy(&sub) + ))) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn bulk(s: &str) -> Frame { + Frame::BulkString(Bytes::from(s.to_string())) + } + + #[test] + fn count_is_an_integer_equal_to_the_registry() { + let n = crate::command::metadata::command_count() as i64; + assert!(matches!(command(&[bulk("COUNT")]), Frame::Integer(v) if v == n)); + } + + #[test] + fn bare_command_yields_one_spec_per_registered_command() { + let Frame::Array(specs) = command(&[]) else { + panic!("bare COMMAND must be an array"); + }; + assert_eq!(specs.len(), crate::command::metadata::command_count()); + } + + #[test] + fn every_spec_has_ten_fields() { + let Frame::Array(specs) = command(&[]) else { + panic!("array"); + }; + for s in &specs { + let Frame::Array(fields) = s else { + panic!("spec must be an array"); + }; + assert_eq!(fields.len(), 10, "spec must carry all 10 fields"); + } + } + + #[test] + fn info_is_positional_and_nulls_unknown_names() { + let Frame::Array(out) = command(&[bulk("INFO"), bulk("GET"), bulk("nope"), bulk("SET")]) + else { + panic!("array"); + }; + assert_eq!(out.len(), 3, "one element per requested name"); + assert!( + matches!(out[1], Frame::Null), + "unknown name is a null ELEMENT" + ); + } + + #[test] + fn count_rejects_extra_arguments() { + let r = command(&[bulk("COUNT"), bulk("extra")]); + let Frame::Error(e) = r else { + panic!("must be an error") + }; + assert!(String::from_utf8_lossy(&e).contains("command|count")); + } + + #[test] + fn getkeys_uses_the_registry_key_spec() { + let r = command(&[ + bulk("GETKEYS"), + bulk("MSET"), + bulk("k1"), + bulk("v1"), + bulk("k2"), + bulk("v2"), + ]); + let Frame::Array(keys) = r else { + panic!("array") + }; + assert_eq!(keys.len(), 2, "MSET keys are every other argument"); + } + + #[test] + fn getkeys_rejects_a_keyless_command() { + let Frame::Error(e) = command(&[bulk("GETKEYS"), bulk("PING")]) else { + panic!("keyless GETKEYS must be an error, never an empty array"); + }; + assert!(String::from_utf8_lossy(&e).contains("no key arguments")); + } + + #[test] + fn getkeys_rejects_an_unregistered_command() { + let Frame::Error(e) = command(&[bulk("GETKEYS"), bulk("NOSUCHCMD"), bulk("k")]) else { + panic!("must be an error"); + }; + assert!(String::from_utf8_lossy(&e).contains("Invalid command")); + } + + #[test] + fn getkeys_rejects_a_short_argv() { + // SET has arity -3; two argv entries is one short. + let Frame::Error(e) = command(&[bulk("GETKEYS"), bulk("SET"), bulk("k")]) else { + panic!("must be an error"); + }; + assert!(String::from_utf8_lossy(&e).contains("Invalid number of arguments")); + } +} diff --git a/src/command/metadata.rs b/src/command/metadata.rs index 17a7663e..ef2b5f20 100644 --- a/src/command/metadata.rs +++ b/src/command/metadata.rs @@ -352,6 +352,10 @@ pub static COMMAND_META: phf::Map<&'static str, CommandMeta> = phf_map! { "HELLO" => CommandMeta { name: "HELLO", arity: -1, flags: CommandFlags(CommandFlags::FAST.0 | CommandFlags::NO_AUTH.0), first_key: 0, last_key: 0, step: 0, acl_categories: CON }, "RESET" => CommandMeta { name: "RESET", arity: 1, flags: CommandFlags(CommandFlags::FAST.0 | CommandFlags::NO_AUTH.0 | CommandFlags::LOADING.0 | CommandFlags::STALE.0), first_key: 0, last_key: 0, step: 0, acl_categories: CON }, "CLIENT" => CommandMeta { name: "CLIENT", arity: -2, flags: A, first_key: 0, last_key: 0, step: 0, acl_categories: CON }, + // ROLE is LOADING|STALE like Redis: a client asking "what are you" must get + // an answer while the server is loading or link-down, precisely when the + // answer matters most for failover decisions. + "ROLE" => CommandMeta { name: "ROLE", arity: 1, flags: CommandFlags(CommandFlags::FAST.0 | CommandFlags::LOADING.0 | CommandFlags::STALE.0), first_key: 0, last_key: 0, step: 0, acl_categories: GEN }, "WAIT" => CommandMeta { name: "WAIT", arity: 3, flags: R, first_key: 0, last_key: 0, step: 0, acl_categories: GEN }, // ---- Server / admin commands ---- diff --git a/src/command/mod.rs b/src/command/mod.rs index 05fd536b..a0e885ba 100644 --- a/src/command/mod.rs +++ b/src/command/mod.rs @@ -10,7 +10,9 @@ pub mod graph; pub mod hash; pub mod helpers; pub mod hll; +pub mod identity; pub mod info_reclamation; +pub mod introspect; pub mod key; pub mod key_extra; pub mod keyspace; @@ -638,7 +640,7 @@ fn dispatch_inner( (7, b'c') => { // COMMAND if cmd.eq_ignore_ascii_case(b"COMMAND") { - return resp(connection::command(args)); + return resp(introspect::command(args)); } } (7, b'f') => { @@ -1367,7 +1369,7 @@ fn dispatch_read_inner(db: &Database, cmd: &[u8], args: &[Frame], now_ms: u64) - (7, b'c') => { // COMMAND if cmd.eq_ignore_ascii_case(b"COMMAND") { - return resp(connection::command(args)); + return resp(introspect::command(args)); } } (7, b'h') => { diff --git a/src/server/conn/core.rs b/src/server/conn/core.rs index 44b6b981..18402a38 100644 --- a/src/server/conn/core.rs +++ b/src/server/conn/core.rs @@ -301,6 +301,26 @@ pub(crate) struct ConnectionState { pub cached_metrics: crate::admin::metrics_setup::CachedMetricsHandles, } +impl ConnectionContext { + /// The server-side address clients on this listener connected to, for + /// `CLIENT INFO`/`CLIENT LIST`'s `laddr` field. + /// + /// Derived from the configured bind + port rather than the socket's real + /// `local_addr()`: the monoio handler's stream is a generic `S` with no + /// such method, and reporting a DIFFERENT laddr per runtime would recreate + /// the per-path divergence this task exists to remove. A wildcard bind is + /// rendered as loopback, matching the existing convention at + /// `handler_monoio/dispatch.rs`. Known limit: a TLS connection is reported + /// against the plain port. + pub fn local_addr_string(&self) -> String { + let host = match self.config.bind.as_str() { + "0.0.0.0" | "::" | "*" | "" => "127.0.0.1", + other => other, + }; + format!("{host}:{}", self.config_port) + } +} + impl ConnectionState { /// Create fresh connection state for a new client. pub fn new( diff --git a/src/server/conn/handler_monoio/dispatch.rs b/src/server/conn/handler_monoio/dispatch.rs index 6f56e2dd..88296061 100644 --- a/src/server/conn/handler_monoio/dispatch.rs +++ b/src/server/conn/handler_monoio/dispatch.rs @@ -84,6 +84,10 @@ pub(super) fn check_auth_gate( client_id, &ctx.acl_table, &mut conn.authenticated, + crate::command::identity::hello_role_and_mode( + ctx.repl_state.as_ref(), + ctx.cluster_state.is_some(), + ), ); if !matches!(&response, Frame::Error(_)) { conn.protocol_version = new_proto; @@ -352,6 +356,10 @@ pub(super) fn try_handle_hello( client_id, &ctx.acl_table, &mut conn.authenticated, + crate::command::identity::hello_role_and_mode( + ctx.repl_state.as_ref(), + ctx.cluster_state.is_some(), + ), ); if !matches!(&response, Frame::Error(_)) { conn.protocol_version = new_proto; diff --git a/src/server/conn/handler_monoio/mod.rs b/src/server/conn/handler_monoio/mod.rs index 92987738..12a44173 100644 --- a/src/server/conn/handler_monoio/mod.rs +++ b/src/server/conn/handler_monoio/mod.rs @@ -394,6 +394,7 @@ pub(crate) async fn handle_connection_sharded_monoio< crate::client_registry::register( client_id, peer_addr.clone(), + ctx.local_addr_string(), conn.current_user.clone(), ctx.shard_id, kill_fd, @@ -405,6 +406,7 @@ pub(crate) async fn handle_connection_sharded_monoio< crate::client_registry::register( client_id, peer_addr.clone(), + ctx.local_addr_string(), conn.current_user.clone(), ctx.shard_id, kill_fd, @@ -1401,6 +1403,27 @@ pub(crate) async fn handle_connection_sharded_monoio< { continue; } + // RESET sits ABOVE the ACL gate deliberately: the registry marks it + // NO_AUTH|LOADING|STALE, and returning a connection to its default + // (unauthenticated) state is exactly what a client does when it has + // lost track of that state. It is also above the MULTI queueing + // step below — measured on redis-server 8.6.1, RESET inside MULTI + // executes immediately and discards the transaction. + if cmd_len == 5 + && crate::server::conn::shared::try_handle_reset( + cmd, + cmd_args, + client_id, + &mut conn, + &ctx.requirepass, + &ctx.tracking_table, + &*ctx.pubsub_registry, + &mut responses, + Some(&mut codec), + ) + { + continue; + } // === ACL GATE - every privileged intercept MUST sit below this === // @@ -1633,6 +1656,19 @@ pub(crate) async fn handle_connection_sharded_monoio< if dispatch::try_handle_client_admin(cmd, cmd_args, client_id, &conn, &mut responses) { continue; } + // ROLE — connection-layer because the answer lives on + // ConnectionContext.repl_state, not in the keyspace. Post-ACL like + // the other server-introspection intercepts around it. + if cmd_len == 4 && cmd.eq_ignore_ascii_case(b"ROLE") { + responses.push(if cmd_args.is_empty() { + crate::command::identity::role(ctx.repl_state.as_ref()) + } else { + Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'role' command", + )) + }); + continue; + } // CLIENT TRACKING mutates server-side invalidation state — post-ACL // (H-3), matching handler_sharded's placement of all CLIENT subcmds. if cmd_len == 6 diff --git a/src/server/conn/handler_sharded/mod.rs b/src/server/conn/handler_sharded/mod.rs index 943d5acc..a25b1316 100644 --- a/src/server/conn/handler_sharded/mod.rs +++ b/src/server/conn/handler_sharded/mod.rs @@ -390,6 +390,7 @@ pub(crate) async fn handle_connection_sharded_inner< let client_live = crate::client_registry::register( client_id, peer_addr.clone(), + ctx.local_addr_string(), conn.current_user.clone(), ctx.shard_id, kill_fd, @@ -616,6 +617,10 @@ pub(crate) async fn handle_connection_sharded_inner< client_id, &ctx.acl_table, &mut conn.authenticated, + crate::command::identity::hello_role_and_mode( + ctx.repl_state.as_ref(), + ctx.cluster_state.is_some(), + ), ); if !matches!(&response, Frame::Error(_)) { conn.protocol_version = new_proto; @@ -722,6 +727,10 @@ pub(crate) async fn handle_connection_sharded_inner< if cmd.eq_ignore_ascii_case(b"HELLO") { let (response, new_proto, new_name, opt_user) = conn_cmd::hello_acl( cmd_args, conn.protocol_version, client_id, &ctx.acl_table, &mut conn.authenticated, + crate::command::identity::hello_role_and_mode( + ctx.repl_state.as_ref(), + ctx.cluster_state.is_some(), + ), ); if !matches!(&response, Frame::Error(_)) { conn.protocol_version = new_proto; } if let Some(name) = new_name { conn.client_name = Some(name); } @@ -741,6 +750,31 @@ pub(crate) async fn handle_connection_sharded_inner< continue; } + // --- RESET --- + // Above the ACL gate for the same reason HELLO is: the + // registry marks RESET NO_AUTH|LOADING|STALE, and returning + // to default (unauthenticated) state must not itself + // require authentication. Also above the MULTI queueing + // step — measured on redis-server 8.6.1, RESET inside MULTI + // executes immediately and discards the transaction. + // + // Shared with handler_monoio: a per-handler copy of this is + // how RESET came to exist ONLY inside this handler's + // subscribe-mode loop and nowhere else. + if crate::server::conn::shared::try_handle_reset( + cmd, + cmd_args, + client_id, + &mut conn, + &ctx.requirepass, + &ctx.tracking_table, + &*ctx.pubsub_registry, + &mut responses, + None, + ) { + continue; + } + // === ACL GATE - every privileged intercept MUST sit below === // // c10k hardening B1. This gate used to sit ~180 lines @@ -1014,6 +1048,19 @@ pub(crate) async fn handle_connection_sharded_inner< } // --- REPLICAOF / SLAVEOF --- + // ROLE — connection-layer, because the answer lives on + // ConnectionContext.repl_state rather than in the keyspace. + // Post-ACL, beside the other replication intercepts. + if cmd.eq_ignore_ascii_case(b"ROLE") { + responses.push(if cmd_args.is_empty() { + crate::command::identity::role(ctx.repl_state.as_ref()) + } else { + Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'role' command", + )) + }); + continue; + } if dispatch::try_handle_replicaof(cmd, cmd_args, ctx, &mut responses) { continue; } diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index 83f9cc9c..6bff6a54 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -389,6 +389,25 @@ pub async fn handle_connection( client_id, &acl_table, &mut conn.authenticated, + // Derived, never assumed. This + // handler is reached only via + // `listener::run_with_shutdown` + // (tokio-only; `main.rs` and + // `embedded.rs` both route through + // `run_sharded`), and THAT listener + // passes a real ReplicationState — + // `Some(rs)` at listener.rs:315. + // So a replica here would announce + // itself a master if this were the + // constant it used to be, which is + // the exact defect class this task + // exists to close. No cluster state + // reaches this handler, so the mode + // is standalone. + crate::command::identity::hello_role_and_mode( + repl_state.as_ref(), + false, + ), ); if !matches!(&response, Frame::Error(_)) { framed.codec_mut().set_protocol_version(new_proto); @@ -541,6 +560,12 @@ pub async fn handle_connection( client_id, &acl_table, &mut conn.authenticated, + // Derived from repl_state, never assumed + // — see the HELLO site above. + crate::command::identity::hello_role_and_mode( + repl_state.as_ref(), + false, + ), ); // CRITICAL: Set protocol version BEFORE sending response (Pitfall 6) if !matches!(&response, Frame::Error(_)) { @@ -588,6 +613,12 @@ pub async fn handle_connection( client_id, &acl_table, &mut conn.authenticated, + // Derived from repl_state, never assumed — + // see the HELLO site above. + crate::command::identity::hello_role_and_mode( + repl_state.as_ref(), + false, + ), ); // CRITICAL: Set protocol version BEFORE sending response (Pitfall 6) if !matches!(&response, Frame::Error(_)) { @@ -1638,6 +1669,40 @@ pub async fn handle_connection( } } + // === ROLE === + // Connection-layer, like the monoio/sharded handlers: + // the answer lives on the replication state, not in the + // keyspace, so `dispatch()` (which only receives a + // Database) cannot produce it. + if cmd.eq_ignore_ascii_case(b"ROLE") { + responses.push(if cmd_args.is_empty() { + crate::command::identity::role(repl_state.as_ref()) + } else { + Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'role' command", + )) + }); + continue; + } + + // === RESET === + // Shares `try_handle_reset` with the other two handlers + // rather than re-deriving "default state" here, so the + // three paths cannot drift on what RESET restores. + if crate::server::conn::shared::try_handle_reset( + cmd, + cmd_args, + client_id, + &mut conn, + &requirepass, + &tracking_table, + &*pubsub_registry, + &mut responses, + Some(framed.codec_mut()), + ) { + continue; + } + // === CLIENT PAUSE check === let pause_wait_ms = { let rt = runtime_config.read(); diff --git a/src/server/conn/shared.rs b/src/server/conn/shared.rs index 00809eef..ad6e2289 100644 --- a/src/server/conn/shared.rs +++ b/src/server/conn/shared.rs @@ -964,6 +964,114 @@ pub(crate) enum TxnLocality { CrossShard, } +/// Tear down every subscription a connection holds, abstracting ONLY the lock +/// the registry happens to sit behind. +/// +/// `ConnectionContext` holds it in an `RwLock`; `handler_single` holds it in a +/// `Mutex`. That difference is the sole reason `try_handle_reset` cannot simply +/// take a `&ConnectionContext`, and it is not worth a second copy of RESET. +pub(crate) trait PubSubTeardown { + /// Drop every channel AND pattern subscription held by `subscriber_id`. + fn unsubscribe_all_for(&self, subscriber_id: u64); +} + +impl PubSubTeardown for parking_lot::RwLock { + fn unsubscribe_all_for(&self, subscriber_id: u64) { + let mut reg = self.write(); + reg.unsubscribe_all(subscriber_id); + reg.punsubscribe_all(subscriber_id); + } +} + +impl PubSubTeardown for parking_lot::Mutex { + fn unsubscribe_all_for(&self, subscriber_id: u64) { + let mut reg = self.lock(); + reg.unsubscribe_all(subscriber_id); + reg.punsubscribe_all(subscriber_id); + } +} + +/// Handle `RESET`, returning `true` when the command was consumed. +/// +/// MUST be called BEFORE the MULTI queueing step. Measured against +/// redis-server 8.6.1: with a transaction open, `RESET` replies `+RESET` and +/// the following `EXEC` errors `without MULTI` — it is executed immediately, +/// never queued. Moon's red run caught this by replying `+QUEUED`. +/// +/// Shared by all THREE handlers for the same reason `WATCH` is: this surface +/// already drifted once. A partial RESET existed only inside +/// `handler_sharded`'s subscribe-mode loop, so RESET worked if you happened to +/// be subscribed on one runtime and was an unknown command everywhere else. +/// +/// "Default state" is deliberately taken from `restore_migrated_state(None, …)` +/// — the SAME function `ConnectionState::new` uses — so RESET's idea of default +/// cannot drift from connection setup's idea of default. +pub(crate) fn try_handle_reset( + cmd: &[u8], + args: &[Frame], + client_id: u64, + conn: &mut super::core::ConnectionState, + // Taken as three pieces rather than a `&ConnectionContext` so the embedded + // handler — which has no such struct and holds the registry behind a + // `Mutex` where the context uses an `RwLock` — can share this exact body + // instead of growing a second, drifting copy of "what RESET restores". + requirepass: &Option, + tracking_table: &parking_lot::Mutex, + pubsub: &dyn PubSubTeardown, + responses: &mut Vec, + // `None` on the sharded handler, which does direct buffer I/O with no + // codec object — there `conn.protocol_version` is itself authoritative. + codec: Option<&mut crate::server::codec::RespCodec>, +) -> bool { + if !cmd.eq_ignore_ascii_case(b"RESET") { + return false; + } + if !args.is_empty() { + // Registry arity is 1. A rejected RESET must not half-apply: return + // before touching any state. + responses.push(Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'reset' command", + ))); + return true; + } + + // Transaction + conn.in_multi = false; + conn.command_queue.clear(); + conn.watched_keys.clear(); + + // Client-side caching + conn.tracking_state = Default::default(); + conn.tracking_rx = None; + tracking_table.lock().untrack_all(client_id); + + // Pub/Sub — exit subscribe mode entirely. + if conn.subscription_count > 0 { + pubsub.unsubscribe_all_for(conn.subscriber_id); + } + conn.subscription_count = 0; + + // Identity + protocol, from the one definition of "default". + let (proto, db, authed, user, name) = + crate::server::conn::util::restore_migrated_state(None, requirepass); + conn.protocol_version = proto; + conn.selected_db = db; + conn.authenticated = authed; + conn.current_user = user; + conn.client_name = name; + // The wire codec must move with the connection, or the very next reply is + // serialized in a protocol the client is no longer speaking. + if let Some(codec) = codec { + codec.set_protocol_version(proto); + } + crate::client_registry::update(client_id, |e| { + e.name = None; + }); + + responses.push(Frame::SimpleString(Bytes::from_static(b"RESET"))); + true +} + /// Classify the WATCHed keys by the shard(s) they hash to. /// /// Same lattice as the body's: no keys is `Keyless`, all on one shard is diff --git a/tests/client_identity_introspection.rs b/tests/client_identity_introspection.rs new file mode 100644 index 00000000..9ada1034 --- /dev/null +++ b/tests/client_identity_introspection.rs @@ -0,0 +1,887 @@ +//! ADD task `client-identity-introspection` — failing-first suite. +//! +//! The identity/introspection surface answers from constants instead of from +//! server state. Measured on `main` @ec0c4650 against `redis-server` 8.6.1, +//! raw RESP on both sides: +//! +//! | input | moon | redis 8.6.1 | +//! |---|---|---| +//! | `COMMAND COUNT` | `*0` | `:274` | +//! | `COMMAND` (bare) | `:0` | `*274` + 10-field specs | +//! | `COMMAND INFO GET` | `*0` | `*1` + spec | +//! | `COMMAND GETKEYS SET k v` | `*0` | `*1 $1 k` | +//! | `ROLE` | `-ERR unknown command` | `*3 master :0 *0` | +//! | `RESET` | `-ERR unknown command` | `+RESET` | +//! | `CLIENT INFO` | `laddr=127.0.0.1:0` | `laddr=127.0.0.1:` | +//! +//! The headline is that bare `COMMAND` and `COMMAND COUNT` return EACH OTHER'S +//! TYPE — an Integer where an Array belongs and an Array where an Integer +//! belongs. A RESP3-typed driver does not read that as "unsupported"; it reads +//! it as a protocol violation. `src/command/connection.rs:113` says so in its +//! own doc comment. +//! +//! Assertions are on RAW BYTES throughout, because `redis-cli` renders `:0` and +//! `*0` identically as "0" — the rendering is exactly how this survived. +//! +//! Expected RED on main: +//! ci1 COMMAND COUNT is an Array, not an Integer +//! ci2 bare COMMAND is an Integer, so there are no specs to count +//! ci3 COMMAND INFO returns an empty array, not one element per name +//! ci4 COMMAND GETKEYS extracts nothing +//! ci5 COMMAND GETKEYS on a keyless command does not reject +//! ci6 COMMAND COUNT does not enforce arity +//! ci7 COMMAND LIST names nothing +//! ci8 ROLE is an unknown command +//! ci10 RESET is an unknown command +//! ci11 RESET does not enforce arity +//! ci13 CLIENT INFO reports laddr port 0 +//! +//! ci9 and ci12 (the replica legs of ROLE and HELLO) are `#[ignore]`d: acting +//! as a PSYNC master is monoio-only, so they cannot run on the tokio CI leg. +//! Run them with `--ignored` on a default-features build. +//! +//! Run alone with: cargo test --test client_identity_introspection + +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 { + connect_ready_in(port, None) +} + +/// `dir` lets a startup failure print the server's own logs instead of only +/// "never answered PING". +fn connect_ready_in(port: u16, dir: Option<&std::path::Path>) -> TcpStream { + let deadline = Instant::now() + Duration::from_secs(30); + let mut last_err = String::new(); + loop { + match TcpStream::connect(format!("127.0.0.1:{port}")) { + Ok(mut s) => { + s.set_read_timeout(Some(Duration::from_secs(10))).ok(); + s.set_write_timeout(Some(Duration::from_secs(10))).ok(); + 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; + } + } + } + Err(e) => last_err = e.to_string(), + } + assert!( + Instant::now() < deadline, + "server on {port} never answered PING in 30s (last connect error: {}){}", + if last_err.is_empty() { + "none — connected but no PONG" + } else { + &last_err + }, + dir.map(server_logs).unwrap_or_default() + ); + std::thread::sleep(Duration::from_millis(50)); + } +} + +// --------------------------------------------------------------------------- +// RESP reading +// +// A single bounded read is NOT enough here: the bare `COMMAND` reply is one +// element per registered command and arrives across many reads. A test that +// read one chunk would assert against a truncated reply and could pass or fail +// for reasons having nothing to do with the server. So: read until exactly one +// complete top-level value is present. +// --------------------------------------------------------------------------- + +/// Byte length of the ONE complete RESP value at the head of `b`, or None if +/// more bytes are needed. Handles the types this suite can encounter. +fn resp_len(b: &[u8]) -> Option { + fn line_end(b: &[u8], from: usize) -> Option { + // index just past the CRLF + let mut i = from; + while i + 1 < b.len() { + if b[i] == b'\r' && b[i + 1] == b'\n' { + return Some(i + 2); + } + i += 1; + } + None + } + fn parse_int(b: &[u8], start: usize, end: usize) -> Option { + std::str::from_utf8(&b[start..end - 2]).ok()?.parse().ok() + } + fn one(b: &[u8], at: usize) -> Option { + let kind = *b.get(at)?; + let hdr = line_end(b, at)?; + match kind { + // simple string, error, integer, boolean, double, big number, null + b'+' | b'-' | b':' | b'#' | b',' | b'(' | b'_' => Some(hdr), + // bulk string / verbatim: header then N bytes then CRLF + b'$' | b'=' => { + let n = parse_int(b, at + 1, hdr)?; + if n < 0 { + return Some(hdr); // $-1 null bulk + } + let end = hdr + n as usize + 2; + if b.len() >= end { Some(end) } else { None } + } + // aggregates: N elements (maps have 2N) + b'*' | b'~' | b'>' | b'%' => { + let n = parse_int(b, at + 1, hdr)?; + if n < 0 { + return Some(hdr); // *-1 null array + } + let count = if kind == b'%' { + n as usize * 2 + } else { + n as usize + }; + let mut cur = hdr; + for _ in 0..count { + cur = one(b, cur)?; + } + Some(cur) + } + _ => None, + } + } + one(b, 0) +} + +/// Send one command, return exactly one complete raw reply. +fn cmd(s: &mut TcpStream, args: &[&str]) -> Vec { + let mut out = format!("*{}\r\n", args.len()).into_bytes(); + for a in args { + out.extend_from_slice(format!("${}\r\n{a}\r\n", a.len()).as_bytes()); + } + s.write_all(&out).expect("write command"); + read_one_reply(s) +} + +fn read_one_reply(s: &mut TcpStream) -> Vec { + let mut buf: Vec = Vec::with_capacity(8192); + let deadline = Instant::now() + Duration::from_secs(15); + loop { + if let Some(n) = resp_len(&buf) { + buf.truncate(n); + return buf; + } + assert!( + Instant::now() < deadline, + "incomplete reply after 15s: {:?}", + String::from_utf8_lossy(&buf[..buf.len().min(200)]) + ); + let mut chunk = vec![0u8; 65536]; + match s.read(&mut chunk) { + Ok(0) => panic!( + "server closed mid-reply; got {:?}", + String::from_utf8_lossy(&buf[..buf.len().min(200)]) + ), + Ok(n) => buf.extend_from_slice(&chunk[..n]), + Err(e) => panic!("read reply: {e}"), + } + } +} + +fn text(reply: &[u8]) -> String { + String::from_utf8_lossy(reply).into_owned() +} + +/// Element count of an aggregate header, e.g. `*271\r\n` -> 271. +fn agg_count(reply: &[u8]) -> Option { + if !matches!(reply.first(), Some(b'*' | b'%' | b'~' | b'>')) { + return None; + } + let end = reply.windows(2).position(|w| w == b"\r\n")? + 1; + std::str::from_utf8(&reply[1..end - 1]).ok()?.parse().ok() +} + +/// Value of an integer reply, e.g. `:271\r\n` -> 271. +fn int_val(reply: &[u8]) -> Option { + if reply.first() != Some(&b':') { + return None; + } + let end = reply.windows(2).position(|w| w == b"\r\n")? + 1; + std::str::from_utf8(&reply[1..end - 1]).ok()?.parse().ok() +} + +/// Split the top-level elements of an aggregate into their raw byte slices. +fn agg_elements(reply: &[u8]) -> Vec> { + let Some(n) = agg_count(reply) else { + return vec![]; + }; + if n <= 0 { + return vec![]; + } + let hdr = reply.windows(2).position(|w| w == b"\r\n").unwrap() + 2; + let mut out = Vec::with_capacity(n as usize); + let mut cur = hdr; + for _ in 0..n { + match resp_len(&reply[cur..]) { + Some(len) => { + out.push(reply[cur..cur + len].to_vec()); + cur += len; + } + None => break, + } + } + out +} + +/// Read back what the server said, for a startup failure that would otherwise +/// report only "never answered PING" — a message that names a symptom and no +/// cause, and sends the next person hunting the wrong layer. +fn server_logs(dir: &std::path::Path) -> String { + let mut out = String::new(); + for f in ["moon.stderr.log", "moon.stdout.log"] { + match std::fs::read_to_string(dir.join(f)) { + Ok(s) if !s.trim().is_empty() => { + let tail: Vec<&str> = s.lines().rev().take(15).collect(); + out.push_str(&format!( + "\n--- {f} (last {} lines) ---\n{}\n", + tail.len(), + tail.into_iter().rev().collect::>().join("\n") + )); + } + Ok(_) => out.push_str(&format!("\n--- {f}: empty ---\n")), + Err(e) => out.push_str(&format!("\n--- {f}: unreadable ({e}) ---\n")), + } + } + out +} + +struct Server { + _guard: ServerGuard, + _dir: tempfile::TempDir, + port: u16, +} + +/// Serialises server STARTUP across the suite's threads. +/// +/// Thirteen servers starting at once — each initialising a data directory — +/// intermittently left one unable to answer PING inside the 30s readiness +/// window: reproduced at ~1 run in 8 with `--test-threads=13`, every failure +/// burning the full 30s rather than erroring fast. `spawn_listening` already +/// waits for the listener to ACCEPT, so the socket was up and the server behind +/// it simply had not finished coming up under the load. +/// +/// Two rejected alternatives: a longer timeout (slower AND still flaky under +/// heavier load — it hides the contention rather than removing it), and one +/// `OnceLock` server shared by all tests (statics are never dropped, so the +/// `ServerGuard` would not run and the suite would leak a live moon process +/// past exit — a failure mode this repo has paid for before). +/// +/// Holding the lock only across spawn+readiness keeps the test BODIES parallel. +fn startup_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} + +fn server(shards: u32) -> Server { + let _startup = startup_lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path(), shards); + // Prove the server is serving BEFORE the test body runs, and surface its + // logs here if it is not, so a startup failure is diagnosed at the point it + // happens rather than as an opaque timeout inside an assertion. + drop(connect_ready_in(port, Some(dir.path()))); + Server { + _guard: ServerGuard(child), + _dir: dir, + port, + } +} + +// --------------------------------------------------------------------------- +// COMMAND — the registry-derived replies +// --------------------------------------------------------------------------- + +/// ci1: COMMAND COUNT must be an Integer whose value is the registry size. +/// RED on main: replies `*0` — an Array where an Integer belongs. +#[test] +fn ci1_command_count_is_an_integer() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["COMMAND", "COUNT"]); + + assert_eq!( + r.first(), + Some(&b':'), + "COMMAND COUNT must reply a RESP Integer; got {:?}", + text(&r) + ); + let n = int_val(&r).expect("integer reply parses"); + assert!( + n > 0, + "COMMAND COUNT must report the real registry size, got {n}" + ); +} + +/// ci2: the bare COMMAND array length must equal COMMAND COUNT, and every +/// element must be the 10-field spec the contract froze. +/// RED on main: bare COMMAND replies `:0` — an Integer where an Array belongs. +#[test] +fn ci2_bare_command_array_len_matches_count() { + let srv = server(1); + let mut c = connect_ready(srv.port); + + let bare = cmd(&mut c, &["COMMAND"]); + assert_eq!( + bare.first(), + Some(&b'*'), + "bare COMMAND must reply an Array of specs; got {:?}", + text(&bare[..bare.len().min(40)]) + ); + + let count = int_val(&cmd(&mut c, &["COMMAND", "COUNT"])) + .expect("COMMAND COUNT must be an integer (see ci1)"); + assert_eq!( + agg_count(&bare), + Some(count), + "bare COMMAND element count must equal COMMAND COUNT" + ); + + let elements = agg_elements(&bare); + assert_eq!(elements.len() as i64, count, "all specs must be readable"); + for (i, e) in elements.iter().enumerate().take(20) { + assert_eq!( + agg_count(e), + Some(10), + "spec {i} must have 10 fields (name, arity, flags, first, last, step, \ + acl_cats, tips, key_specs, subcommands); got {:?}", + text(&e[..e.len().min(60)]) + ); + let fields = agg_elements(e); + let name = text(&fields[0]); + assert!( + name.contains(|ch: char| ch.is_ascii_lowercase()), + "spec {i} name must be lower-cased as Redis emits it; got {name:?}" + ); + } +} + +/// ci3: COMMAND INFO answers one element per requested name, in request order, +/// with a Null ELEMENT (not an empty array) for an unknown name. +/// RED on main: replies `*0` regardless of what was asked. +#[test] +fn ci3_command_info_order_and_null_element() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["COMMAND", "INFO", "GET", "nosuchcmd", "SET"]); + + assert_eq!( + agg_count(&r), + Some(3), + "COMMAND INFO must reply one element per requested name; got {:?}", + text(&r[..r.len().min(60)]) + ); + let e = agg_elements(&r); + assert!( + text(&e[0]).contains("get"), + "element 0 must describe GET; got {:?}", + text(&e[0]) + ); + assert!( + e[1].starts_with(b"$-1") || e[1].starts_with(b"_\r\n") || e[1].starts_with(b"*-1"), + "an unknown name must yield a Null ELEMENT inside the array; got {:?}", + text(&e[1]) + ); + assert!( + text(&e[2]).contains("set"), + "element 2 must describe SET; got {:?}", + text(&e[2]) + ); +} + +/// ci4: COMMAND GETKEYS extracts keys using the registry's first/last/step. +/// RED on main: replies `*0`, extracting nothing. +#[test] +fn ci4_command_getkeys_extracts() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd( + &mut c, + &["COMMAND", "GETKEYS", "MSET", "k1", "v1", "k2", "v2"], + ); + + assert_eq!( + agg_count(&r), + Some(2), + "MSET has two keys at step 2; got {:?}", + text(&r) + ); + let e = agg_elements(&r); + assert!( + text(&e[0]).contains("k1"), + "first key; got {:?}", + text(&e[0]) + ); + assert!( + text(&e[1]).contains("k2"), + "second key; got {:?}", + text(&e[1]) + ); +} + +/// ci5: COMMAND GETKEYS on a keyless command must reject, not return empty. +/// RED on main: replies `*0`, indistinguishable from "no keys found". +#[test] +fn ci5_command_getkeys_keyless_rejects() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["COMMAND", "GETKEYS", "PING"]); + + assert!( + text(&r).contains("The command has no key arguments"), + "keyless GETKEYS must be an error, not an empty array; got {:?}", + text(&r) + ); + // the connection stays usable after the rejection + assert!(text(&cmd(&mut c, &["PING"])).contains("PONG")); +} + +/// ci6: COMMAND COUNT takes no arguments. +/// RED on main: the stub ignores extra args and replies `*0`. +#[test] +fn ci6_command_count_arity_rejects() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["COMMAND", "COUNT", "extra"]); + + let t = text(&r); + assert!( + t.starts_with('-') && t.contains("wrong number of arguments"), + "COMMAND COUNT must enforce arity; got {t:?}" + ); + assert!( + t.contains("command|count"), + "the error must name the subcommand as 'command|count', matching Redis; got {t:?}" + ); +} + +/// ci7: COMMAND LIST names every registered command. +/// RED on main: replies `*0`. +#[test] +fn ci7_command_list_names() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["COMMAND", "LIST"]); + + let n = agg_count(&r).unwrap_or(0); + assert!( + n > 0, + "COMMAND LIST must name the registered commands; got {:?}", + text(&r[..r.len().min(60)]) + ); + let joined = text(&r); + for expect in ["get", "set", "reset"] { + assert!( + joined.contains(expect), + "COMMAND LIST must include {expect:?}" + ); + } +} + +// --------------------------------------------------------------------------- +// ROLE +// --------------------------------------------------------------------------- + +/// ci8: ROLE on a master replies [master, offset, replicas[]]. +/// RED on main: `-ERR unknown command 'ROLE'`. +#[test] +fn ci8_role_master_shape() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let r = cmd(&mut c, &["ROLE"]); + + assert_eq!( + agg_count(&r), + Some(3), + "ROLE on a master is a 3-element array; got {:?}", + text(&r) + ); + let e = agg_elements(&r); + assert!( + text(&e[0]).contains("master"), + "element 0 must be the role; got {:?}", + text(&e[0]) + ); + assert_eq!( + e[1].first(), + Some(&b':'), + "element 1 must be the replication offset as an Integer; got {:?}", + text(&e[1]) + ); + assert!( + e[2].first() == Some(&b'*'), + "element 2 must be the replica array; got {:?}", + text(&e[2]) + ); +} + +/// ci9: ROLE on a replica reports slave, and agrees with INFO replication. +/// Two sources of truth for one fact is the defect class this task closes, so +/// the assertion is that they AGREE, not merely that each is well-formed. +/// +/// `#[ignore]`: acting as a PSYNC master is monoio-only, so this cannot run on +/// the tokio CI leg. Run with `--ignored` on a default-features build. +#[test] +#[ignore = "PSYNC-as-master is monoio-only; run with --ignored on default features"] +fn ci9_role_replica_agrees_with_info() { + let master = server(1); + let replica = server(1); + let mut m = connect_ready(master.port); + assert!(text(&cmd(&mut m, &["PING"])).contains("PONG")); + + let mut r = connect_ready(replica.port); + cmd( + &mut r, + &["REPLICAOF", "127.0.0.1", &master.port.to_string()], + ); + std::thread::sleep(Duration::from_secs(2)); + + let role = cmd(&mut r, &["ROLE"]); + let e = agg_elements(&role); + assert!( + !e.is_empty() && text(&e[0]).contains("slave"), + "ROLE on a replica must report slave; got {:?}", + text(&role) + ); + + let info = text(&cmd(&mut r, &["INFO", "replication"])); + assert!( + info.contains("role:slave"), + "INFO must agree with ROLE; got {info:?}" + ); + assert!( + info.contains(&format!("master_port:{}", master.port)), + "ROLE and INFO must name the same master" + ); +} + +// --------------------------------------------------------------------------- +// RESET +// --------------------------------------------------------------------------- + +/// ci10: RESET returns the connection to default state. +/// Every effect asserted here was measured against redis-server 8.6.1 — in +/// particular the protocol reverting RESP3 -> RESP2, which is easy to assume +/// away. +/// RED on main: `-ERR unknown command 'RESET'`. +#[test] +fn ci10_reset_returns_default_state() { + let srv = server(1); + let mut c = connect_ready(srv.port); + + cmd(&mut c, &["HELLO", "3"]); + cmd(&mut c, &["SELECT", "5"]); + cmd(&mut c, &["CLIENT", "SETNAME", "bob"]); + cmd(&mut c, &["WATCH", "k"]); + cmd(&mut c, &["MULTI"]); + + let r = cmd(&mut c, &["RESET"]); + assert!( + r.starts_with(b"+RESET\r\n"), + "RESET must reply the simple string RESET; got {:?}", + text(&r) + ); + + assert!( + text(&cmd(&mut c, &["EXEC"])).contains("without MULTI"), + "RESET must discard MULTI" + ); + + let name = cmd(&mut c, &["CLIENT", "GETNAME"]); + assert!( + name.starts_with(b"$-1") || name.starts_with(b"_\r\n"), + "RESET must clear the client name; got {:?}", + text(&name) + ); + + let info = text(&cmd(&mut c, &["CLIENT", "INFO"])); + assert!( + info.contains("db=0"), + "RESET must return to db 0; got {info:?}" + ); + + let hello = cmd(&mut c, &["HELLO"]); + assert_eq!( + hello.first(), + Some(&b'*'), + "RESET must revert the protocol to RESP2, so bare HELLO replies a flat \ + Array rather than a RESP3 Map; got {:?}", + text(&hello[..hello.len().min(40)]) + ); +} + +/// ci11: RESET has arity 1 — the registry already says so. +/// RED on main: unknown command, so arity is never reached. +#[test] +fn ci11_reset_arity_rejects() { + let srv = server(1); + let mut c = connect_ready(srv.port); + + cmd(&mut c, &["MULTI"]); + let r = cmd(&mut c, &["RESET", "now"]); + let t = text(&r); + assert!( + t.starts_with('-') && t.contains("wrong number of arguments"), + "RESET takes no arguments; got {t:?}" + ); + // the rejected RESET must not have half-applied: MULTI is still open + assert!( + text(&cmd(&mut c, &["EXEC"])).starts_with('*'), + "a rejected RESET must not discard MULTI" + ); +} + +// --------------------------------------------------------------------------- +// HELLO / CLIENT INFO — stop contradicting the rest of the server +// --------------------------------------------------------------------------- + +/// ci12: HELLO's role field agrees with INFO replication on the same +/// connection. `hello_acl` hardcodes `role: master`, so on a replica the two +/// contradict each other today. +/// +/// `#[ignore]`: same monoio-only constraint as ci9. +#[test] +#[ignore = "PSYNC-as-master is monoio-only; run with --ignored on default features"] +fn ci12_hello_role_matches_info() { + let master = server(1); + let replica = server(1); + let mut r = connect_ready(replica.port); + cmd( + &mut r, + &["REPLICAOF", "127.0.0.1", &master.port.to_string()], + ); + std::thread::sleep(Duration::from_secs(2)); + + let hello = text(&cmd(&mut r, &["HELLO", "3"])); + let info = text(&cmd(&mut r, &["INFO", "replication"])); + let role = text(&cmd(&mut r, &["ROLE"])); + + assert!( + info.contains("role:slave"), + "precondition: the server must actually be a replica; got {info:?}" + ); + // Redis uses THREE vocabularies for this ONE fact, measured on 8.6.1 + // against a real replica pair: HELLO says "replica", INFO says "slave", + // ROLE says "slave". This test originally asserted "slave" in HELLO and + // failed against a CORRECT implementation — the assertion was wrong, not + // the code. Corrected against measurement (see §4 of the task record); the + // point of the test is unchanged and now stronger: all three must agree + // that this node is a replica, each in its own vocabulary. + assert!( + hello.contains("replica"), + "HELLO must report the real role — Redis spells it 'replica' here, not \ + a hardcoded 'master'; got {hello:?}" + ); + assert!( + !hello.contains("$6\r\nmaster"), + "HELLO must not still claim master on a replica; got {hello:?}" + ); + assert!( + role.contains("slave"), + "ROLE spells the same fact 'slave'; got {role:?}" + ); +} + +/// ci13: CLIENT INFO reports the real local address. +/// RED on main: `laddr=127.0.0.1:0` is a literal in the format string at +/// `src/client_registry.rs:709`. +#[test] +fn ci13_client_info_laddr_real_port() { + let srv = server(1); + let mut c = connect_ready(srv.port); + let info = text(&cmd(&mut c, &["CLIENT", "INFO"])); + + let laddr = info + .split_whitespace() + .find(|f| f.starts_with("laddr=")) + .unwrap_or_else(|| panic!("CLIENT INFO must carry an laddr field; got {info:?}")) + .to_string(); + + assert!( + !laddr.ends_with(":0"), + "laddr must be the real local address, not port 0; got {laddr:?}" + ); + assert!( + laddr.ends_with(&format!(":{}", srv.port)), + "laddr must carry the port the client connected to ({}); got {laddr:?}", + srv.port + ); +} + +// --------------------------------------------------------------------------- +// ci14 — the THIRD handler. +// +// `handler_single` is not reachable from the shipped binary: `main.rs` and +// `embedded.rs` both route through `run_sharded` -> `handler_sharded`. It is +// driven only by `listener::run_with_shutdown`, a tokio-only in-process API +// that several suites (kill_snapshot, graph_bench_*) use. Every other test in +// this file spawns a real `moon` process and therefore CANNOT reach it — an +// A/B proved exactly that: reverting the fix below left ci12 green. +// +// That blind spot is how this surface drifted in the first place, so the third +// copy gets its own test rather than an assurance. +#[cfg(feature = "runtime-tokio")] +// MUST be multi_thread: the body drives BLOCKING sockets and `std::thread::sleep` +// (the same raw-RESP helpers the rest of this file uses, so the assertions stay +// byte-level). On the default current-thread runtime those block the one worker +// and the spawned listener is never polled — the first cut of this test failed +// with "never answered PING" for exactly that reason, not for a server bug. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn ci14_handler_single_identity_surface() { + use moon::config::ServerConfig; + use moon::runtime::cancel::CancellationToken; + use moon::server::listener; + + let probe = std::net::TcpListener::bind("127.0.0.1:0").expect("bind probe"); + let port = probe.local_addr().expect("probe addr").port(); + drop(probe); + + let dir = tempfile::tempdir().expect("tempdir"); + let config = ServerConfig { + bind: "127.0.0.1".to_string(), + port, + shards: 1, + dir: dir.path().to_string_lossy().to_string(), + appendonly: "no".to_string(), + // MUST be set explicitly. `ServerConfig` derives `Default`, but the + // sane values live in clap `default_value_t` attributes, which apply + // ONLY to CLI parsing — `Default::default()` leaves this 0, and the + // handler then indexes `db[0]` of an empty slice and panics on the + // first command. That is a harness trap, not a server defect. + databases: 16, + // The data dir sits on a volume that hovers near the 5% diskfull guard; + // 0 disables it so a full disk cannot masquerade as an identity bug. + disk_free_min_pct: 0, + ..Default::default() + }; + + let token = CancellationToken::new(); + let server_token = token.clone(); + tokio::spawn(async move { + if let Err(e) = listener::run_with_shutdown(config, server_token).await { + eprintln!("ci14: run_with_shutdown failed: {e:#}"); + } + }); + + // Same readiness discipline as `connect_ready`: poll for a real PONG rather + // than sleeping a hopeful constant. + let deadline = std::time::Instant::now() + Duration::from_secs(30); + let mut c = loop { + assert!( + std::time::Instant::now() < deadline, + "handler_single server on port {port} never answered PING" + ); + if let Ok(mut 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(); + 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") + { + break s; + } + } + } + std::thread::sleep(Duration::from_millis(50)); + }; + + // ROLE — was an unknown command on this handler before this task. + let role = cmd(&mut c, &["ROLE"]); + assert!( + role.starts_with(b"*3\r\n"), + "handler_single ROLE must reply the 3-element master form; got {:?}", + String::from_utf8_lossy(&role) + ); + assert!( + role.windows(6).any(|w| w == b"master"), + "handler_single ROLE must name the role; got {:?}", + String::from_utf8_lossy(&role) + ); + + // HELLO's role field must be DERIVED. This server is a master, so the + // assertion a hard-coded "master" would also satisfy is worthless on its + // own — what it pins is that the field is present and well-formed, while + // the derivation itself is pinned by ci12 on the handlers that CI can spawn + // a replica for. + let hello = cmd(&mut c, &["HELLO"]); + assert!( + hello.windows(4).any(|w| w == b"role"), + "handler_single HELLO must carry a role field; got {:?}", + String::from_utf8_lossy(&hello) + ); + + // RESET — was likewise unknown here; it must reply +RESET and, per §1, be + // executed immediately inside MULTI rather than queued. + let reset = cmd(&mut c, &["RESET"]); + assert_eq!( + &reset[..], + b"+RESET\r\n", + "handler_single RESET must reply +RESET; got {:?}", + String::from_utf8_lossy(&reset) + ); + + assert_eq!(&cmd(&mut c, &["MULTI"])[..], b"+OK\r\n"); + let in_multi = cmd(&mut c, &["RESET"]); + assert_eq!( + &in_multi[..], + b"+RESET\r\n", + "RESET must execute immediately inside MULTI, never queue; got {:?}", + String::from_utf8_lossy(&in_multi) + ); + let exec = cmd(&mut c, &["EXEC"]); + assert!( + exec.starts_with(b"-ERR EXEC without MULTI"), + "RESET must have discarded the transaction; got {:?}", + String::from_utf8_lossy(&exec) + ); + + token.cancel(); +} From 31881d916e7843604a41ac0dc82180bb6c4e3c2a Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 12 Aug 2026 10:09:26 +0700 Subject: [PATCH 3/3] fix(command): queue ROLE inside MULTI, and stop the compat harness needing a live bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the client-compat harness, which is the check that failed on PR #471 — both real, neither visible to the 14 raw-RESP tests in this task's own suite. 1. ROLE executed at QUEUE time inside MULTI. The first cut intercepted ROLE at the connection layer, ahead of the MULTI queueing step, because its answer lives on ConnectionContext.repl_state rather than in the Database that dispatch() receives. So `MULTI; ROLE; EXEC` replied the role array immediately and EXEC then returned `*0`. The damage is worse than a wrong reply: the command silently vanishes from the EXEC array, so every LATER result shifts down one index and a client reads another command's answer as this one's. Redis queues ROLE like any other command. Fixed by answering ROLE from the shared dispatch table instead, reading the process-global replication handle that INFO already uses and that every entry point registers (main.rs, listener.rs, embedded.rs). That is the only placement where a queued ROLE can work at all, since EXEC replays the queue through dispatch() — and it deleted all three per-handler intercepts, so ROLE now lives in exactly two places instead of five. Verified on the wire: `MULTI; ROLE; PING; EXEC` returns `*2` with the role array first and +PONG still last, and a live replica still reports `slave` through the global handle. ci15 pins the alignment; it fails under both prior states (`*0`, and `*1` with an unknown- command error). Nothing in this task's suite exercised a connection-layer command INSIDE a transaction, which is the coverage lesson: a new intercept has to be tested in MULTI as well as standalone, because its POSITION relative to queueing is the thing that can be wrong. 2. The harness's own divergence test needed a live Moon defect to pass. test_a_diverging_entry_exits_one_and_names_the_divergence borrowed a real bug as its fixture, so it FAILED whenever someone FIXED that bug — a test that punishes the fix. Three fixtures had already been burned this way (GET-inside-MULTI #457, SISMEMBER RESP3 #463, and COMMAND COUNT, retired by this very task, which is what turned the PR red). The durable fix was already designed and filed as #461, so it is implemented here rather than rotating to a fourth defect: a test-only `inject_moon_reply` hook fabricates the divergence, with a guard test asserting the shipped manifest never uses one. Proven load-bearing by disabling the hook, which turns the test red. Manifest: the identity_command_count and identity_role waivers are retired as fixed, and COMMAND INFO / COMMAND GETKEYS / RESET are added as live entries. COMMAND INFO carries a new, accurate waiver — its 10-field SHAPE now matches, but acl_categories is thin (@string where Redis says @read @string @fast), key_specs is empty, and under RESP3 Redis types flags/acl_categories as Sets and key_specs entries as Maps where Moon emits Arrays. Owners recorded. Harness result against redis-server 8.6.1: PASS=181 FAIL=0 WAIVED=19 (was PASS=179 FAIL=8 before this commit). Gates: fmt, clippy (default + tokio/jemalloc, --all-targets), the identity suite (15 scenarios), and both compat suites (34 differ + 20 e2e) green. Note the unreachable-pattern warning caught during this work: (4, b'r') already existed in dispatch_inner for RPOP, so the first ROLE arm was dead code and EXEC still answered unknown command until it was merged into the existing arm. author: Tin Dang --- .../client-identity-introspection/TASK.md | 16 ++++++ CHANGELOG.md | 14 +++++- scripts/client-compat/differ.py | 31 ++++++++++++ scripts/client-compat/manifest.yaml | 40 +++++++++++---- scripts/client-compat/test_differ.py | 15 ++++++ scripts/client-compat/test_e2e.py | 44 +++++++--------- src/command/mod.rs | 40 +++++++++++++++ src/server/conn/handler_monoio/mod.rs | 13 ----- src/server/conn/handler_sharded/mod.rs | 13 ----- src/server/conn/handler_single.rs | 16 ------ tests/client_identity_introspection.rs | 50 +++++++++++++++++++ 11 files changed, 213 insertions(+), 79 deletions(-) diff --git a/.add/tasks/client-identity-introspection/TASK.md b/.add/tasks/client-identity-introspection/TASK.md index 76323b85..0b3ff13e 100644 --- a/.add/tasks/client-identity-introspection/TASK.md +++ b/.add/tasks/client-identity-introspection/TASK.md @@ -592,6 +592,22 @@ What did this loop teach the foundation? One line each, tagged by competency task `batch-protocol-version-fidelity` with the full measurement table. Pre-existing — the `HELLO`+`HELLO` reproducer touches none of this task's code — but RESET, which reverts the protocol by contract, adds a second trigger, which is how it surfaced here. +- [TDD · open] The client-compat harness caught a regression that FOURTEEN raw-RESP tests and + two full suites missed: `ROLE` intercepted at the connection layer ran ahead of the MULTI + queueing step, so `MULTI; ROLE; EXEC` executed ROLE at queue time and returned `*0` — the + command vanished from the EXEC array and shifted every later result index. Nothing in this + task's own suite exercised a connection-layer command INSIDE a transaction. Lesson: a new + intercept must be tested in MULTI as well as standalone, because the intercept's POSITION + relative to queueing is the thing that can be wrong. Fixed by moving ROLE into the shared + dispatch table (answered from the process-global replication handle `INFO` already uses), + which also deleted all three handler intercepts. +- [ADD · open] A harness test that borrows a live defect as its fixture punishes the fix. This + task retired the third such fixture (`COMMAND COUNT`); the durable hook was already designed + and filed as #461, so it was implemented here rather than rotating to a fourth defect. +- [SPEC · open] `COMMAND INFO`'s 10-field SHAPE now matches Redis, but three inner divergences + remain and are now WAIVED with owners rather than unmeasured: acl_categories is thin + (`@string` where Redis says `@read @string @fast`), key_specs is empty, and under RESP3 Redis + types flags/acl_categories as Sets and key_specs entries as Maps where Moon emits Arrays. - [SPEC · open] `handler_single` is dead weight in the shipped binary (no non-test caller) yet carries a full command loop that must be kept in parity with two live handlers. Either wire it to something real or delete it; a third copy nobody executes is where drift hides. This task diff --git a/CHANGELOG.md b/CHANGELOG.md index 20aa63be..56b7bb74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,9 +18,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 metadata table with full flags while dispatch rejected it (the same advertise-then-reject class as WATCH/UNWATCH before v0.8.6), and a partial `RESET` existed only inside `handler_sharded`'s subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere - else. Both are now wired on every production path, and on `handler_single` (the in-process + else. `RESET` is wired on every production path and on `handler_single` (the in-process single-shard handler that only tests reach) so the third copy of this surface cannot drift - unnoticed the way it just did. + unnoticed the way it just did. `ROLE` is answered from the shared dispatch table instead, + reading the process-global replication handle that `INFO` already uses — which is what lets a + QUEUED `ROLE` work: `EXEC` replays the queue through `dispatch()`, so a connection-layer + intercept would have executed `ROLE` at queue time and dropped it from the `EXEC` array, + shifting every later result index for the client. Caught by the client-compat harness. ### Fixed - **`HELLO` no longer contradicts `INFO replication` about what the server is.** `hello_acl` built @@ -30,6 +34,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Redis deliberately uses three vocabularies for this one fact (`HELLO` -> `replica`, `INFO` -> `slave`, `ROLE` -> `slave`), verified against a live replica pair on redis-server 8.6.1, and Moon now matches each. +- **The client-compat harness no longer needs a live Moon defect to test itself.** Its + divergence-classification test borrowed a real bug as its fixture, so it FAILED whenever + someone fixed that bug — three fixtures were burned this way (GET-inside-MULTI #457, + SISMEMBER RESP3 #463, and `COMMAND COUNT` here). It now fabricates the divergence through a + test-only `inject_moon_reply` hook, with a guard test asserting the shipped manifest never + uses it. The `identity_command_count` and `identity_role` waivers were retired as fixed. - **`CLIENT INFO` / `CLIENT LIST` report the real `laddr`.** The field was the literal `laddr=127.0.0.1:0` inside the format string, so every client on every listener reported port 0 — a monitoring agent using `laddr` to tell listeners apart saw nothing. diff --git a/scripts/client-compat/differ.py b/scripts/client-compat/differ.py index cb48e21e..c118fe74 100755 --- a/scripts/client-compat/differ.py +++ b/scripts/client-compat/differ.py @@ -256,6 +256,17 @@ class Entry: contexts: tuple[str, ...] = CONTEXTS protocols: tuple[str, ...] = PROTOCOLS expect_diff: Optional[str] = None + # TEST-ONLY. Replaces Moon's reply with these literal wire bytes before the + # comparison, so the harness's own tests can fabricate a divergence instead + # of borrowing a live Moon defect. + # + # Three fixtures were burned this way before the hook existed — GET-inside- + # MULTI (#457), SISMEMBER RESP3 (#463), COMMAND COUNT (this task) — each + # retired the moment someone FIXED the bug it depended on, so the suite + # punished the fix. Fabricating the divergence removes that incentive + # permanently. Never set this in the shipped manifest: an injected reply is + # not evidence about Moon. + inject_moon_reply: Optional[bytes] = None def load_manifest(path: str) -> list[Entry]: @@ -314,6 +325,14 @@ def load_manifest(path: str) -> list[Entry]: f"entry {item['name']}: expect_diff needs a reason") waiver = reason + inject = item.get("inject_moon_reply") + if inject is not None: + if not isinstance(inject, str): + raise HarnessError("ERR_BAD_MANIFEST", + f"entry {item['name']}: inject_moon_reply must be a string") + # Written with escapes in YAML (\r\n); decode to real wire bytes. + inject = inject.encode("latin1").decode("unicode_escape").encode("latin1") + entries.append(Entry( name=item["name"], command=item["command"], @@ -323,6 +342,7 @@ def load_manifest(path: str) -> list[Entry]: contexts=contexts, protocols=protocols, expect_diff=waiver, + inject_moon_reply=inject, )) return entries @@ -568,6 +588,17 @@ def _compare_entry(self, entry: Entry, protocol: str, context: str, assert r_sent == m_sent, "byte-identical send is a contract invariant" + if entry.inject_moon_reply is not None: + # Substitution happens AFTER the real exchange on purpose: the + # servers still run, the bytes are still sent identically, and only + # the reply under comparison is fabricated. A hook that skipped the + # exchange would also stop testing the exchange. + m_raw = entry.inject_moon_reply + try: + m_node = parse_resp(m_raw) + except Exception: + m_node = None + if r_node is None or m_node is None: return Result(entry.name, protocol, context, r_sent, r_raw, m_raw, "parse_error", None, "ERR_PROTOCOL_PARSE", diff --git a/scripts/client-compat/manifest.yaml b/scripts/client-compat/manifest.yaml index 08ace29d..eff49aa0 100644 --- a/scripts/client-compat/manifest.yaml +++ b/scripts/client-compat/manifest.yaml @@ -228,25 +228,45 @@ entries: # ── Identity & introspection (review findings, wire-confirmed) ──────── + # Waivers removed with client-identity-introspection: COMMAND COUNT answered + # an empty Array *0 (ignoring the subcommand and falling through to the bare + # COMMAND stub) and ROLE was -ERR unknown command. Both are live comparisons + # now. `type_only` is the right policy for the pair: the COUNT values are + # SUPPOSED to differ (Moon implements a different number of commands than + # Redis), and ROLE's offset moves — what must match is the reply TYPE, which + # is what clients dispatch on. - name: identity_command_count command: "COMMAND COUNT" policy: type_only - expect_diff: - reason: >- - Redis answers Integer :274; Moon answers an empty Array *0 — it ignores - the COUNT subcommand and falls through to the bare COMMAND stub. A - client using COMMAND COUNT to size its command table reads zero. - Owner: client-identity-introspection. - name: identity_role command: "ROLE" policy: type_only + + - name: identity_command_info_known_and_unknown + command: "COMMAND INFO GET nosuchcmd" + policy: type_only expect_diff: reason: >- - Redis answers *3[master, offset, replicas]; Moon answers - -ERR unknown command 'ROLE'. Sentinel-aware and failover-aware clients - call ROLE during topology discovery. - Owner: client-identity-introspection. + The 10-field spec SHAPE now matches (that was the fix), and the unknown + name correctly yields a Null element. Three inner divergences remain, + all pre-existing data/typing gaps rather than introspection bugs: + (1) acl_categories is thin — Redis reports @read @string @fast for GET, + Moon's registry carries only @string; + (2) key_specs is empty — Redis emits one spec for GET, Moon emits *0; + (3) under RESP3 Redis types flags and acl_categories as Sets (~) and + key_specs entries as Maps (%), while Moon emits Arrays (*). + A client dispatching on the reply shape is served; one reading + acl_categories to build a permission model is not. + Owner: (1)+(2) sdk-wire-form-fixes, (3) resp3-type-fidelity. + + - name: identity_command_getkeys + command: "COMMAND GETKEYS MSET k1 v1 k2 v2" + policy: exact + + - name: identity_reset + command: "RESET" + policy: exact # ── Error-code parity (compared on the code, never the message) ─────── diff --git a/scripts/client-compat/test_differ.py b/scripts/client-compat/test_differ.py index b12d9652..d4342c91 100644 --- a/scripts/client-compat/test_differ.py +++ b/scripts/client-compat/test_differ.py @@ -290,3 +290,18 @@ def test_unknown_context_rejected(self): if __name__ == "__main__": unittest.main() + + +class TestInjectionIsTestOnly(unittest.TestCase): + def test_shipped_manifest_injects_nothing(self): + """The shipped manifest must never fabricate a Moon reply. + + `inject_moon_reply` exists so the harness's OWN tests can build a + divergence without depending on a live Moon defect. An injected reply + in the shipped manifest would be a comparison against a fixture rather + than against Moon — a green that proves nothing. + """ + here = os.path.dirname(os.path.abspath(__file__)) + entries = load_manifest(os.path.join(here, "manifest.yaml")) + offenders = [e.name for e in entries if e.inject_moon_reply is not None] + self.assertEqual(offenders, [], f"shipped manifest fabricates replies: {offenders}") diff --git a/scripts/client-compat/test_e2e.py b/scripts/client-compat/test_e2e.py index 3a3fc946..83183bbc 100644 --- a/scripts/client-compat/test_e2e.py +++ b/scripts/client-compat/test_e2e.py @@ -125,44 +125,38 @@ def test_a_matching_entry_exits_zero(self): self.assertEqual(report.tally()["fail"], 0) def test_a_diverging_entry_exits_one_and_names_the_divergence(self): - # COMMAND COUNT: Redis answers Integer :274, Moon answers an empty - # Array *0 (it ignores the COUNT subcommand). The harness must surface - # this as a failure naming TYPE, not swallow it. - # - # THIRD fixture for this test. The rotation is the point, so read it - # before picking a fourth: + # The divergence is FABRICATED via inject_moon_reply, not borrowed from + # a live Moon defect. Three real fixtures were burned before this hook + # existed, each retired by someone FIXING the bug it depended on: # 1. GET-inside-MULTI — fixed by the v0.8.6 inline-GET hotfix (#457) - # 2. SISMEMBER RESP3 — fixed by resp3-type-fidelity (#463); Moon no - # longer over-converts Integer to Boolean, so this test began - # failing with `0 != 1` the moment that landed - # 3. COMMAND COUNT — open, owned by `client-identity-introspection` - # - # A test that needs a live defect to pass fails as a REWARD for fixing - # something, which is exactly backwards. #3 is chosen because it is - # owned by a different task than the reply-type work, so that line of - # work cannot silently retire it again — but that is damage control, - # not a fix. + # 2. SISMEMBER RESP3 — fixed by resp3-type-fidelity (#463) + # 3. COMMAND COUNT — fixed by client-identity-introspection, which + # is what forced this rewrite: Moon now answers `:263` where it + # answered `*0`, so the divergence became `value` instead of `type` + # and this test failed as a REWARD for the fix. # - # No permanent-by-construction TYPE divergence exists today: Moon's - # proprietary commands (TXN.ABORT, MQ PUSH) return an Error on BOTH - # servers, so they diverge in text but not in type; and redis 8.6.1 - # turns out to implement `hotkeys` too. The durable fix is a test-only - # injection hook that fabricates a divergence rather than borrowing a - # real one — tracked in #461 alongside the harness's other - # self-honesty gaps. + # PING is chosen as the carrier precisely because it is the least likely + # command in Redis to ever change: the entry asserts nothing about PING, + # it only needs a reply to overwrite. Redis answers `+PONG` (SimpleString) + # and the injected reply is an Integer, so the classifier must say TYPE. report = Runner(cfg(manifest_path=manifest(""" entries: - - name: command_count_type - command: "COMMAND COUNT" + - name: fabricated_type_divergence + command: "PING" policy: exact protocols: [resp3] contexts: [standalone] + inject_moon_reply: ":1\\r\\n" """))).run() self.assertEqual(report.exit_code(), 1) diffs = [r for r in report.results if r.verdict == "diff"] self.assertTrue(diffs) for r in diffs: self.assertEqual(r.divergence, "type") + # The fabricated reply must be what was compared and recorded — + # otherwise the hook is a no-op and this test proves nothing. + self.assertEqual(r.moon_raw, b":1\r\n") + self.assertEqual(r.redis_raw, b"+PONG\r\n") def test_info_manifest_reports_missing_fields_by_name(self): # run_id is emitted by real Redis and not by Moon, so it is a genuine diff --git a/src/command/mod.rs b/src/command/mod.rs index a0e885ba..d0564756 100644 --- a/src/command/mod.rs +++ b/src/command/mod.rs @@ -243,6 +243,25 @@ fn dispatch_inner( if cmd.eq_ignore_ascii_case(b"RPOP") { return resp(list::rpop(db, args)); } + // ROLE — answered HERE rather than at the connection layer so a + // queued ROLE inside MULTI works: EXEC replays the queue through + // dispatch(), and a connection-layer intercept would have executed + // ROLE immediately at queue time, dropping it from the EXEC array + // and shifting every later result index for the client. + // + // The replication state comes from the process-global handle every + // entry point registers (main.rs, listener.rs, embedded.rs), which + // is the same source INFO reads. Unset (before replication init) + // yields the master form, matching a `None` ReplicationState. + if cmd.eq_ignore_ascii_case(b"ROLE") { + return resp(if args.is_empty() { + identity::role(crate::admin::metrics_setup::get_global_repl_state_arc()) + } else { + Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'role' command", + )) + }); + } } (4, b's') => { // SCAN SADD SREM SPOP @@ -1372,6 +1391,27 @@ fn dispatch_read_inner(db: &Database, cmd: &[u8], args: &[Frame], now_ms: u64) - return resp(introspect::command(args)); } } + (4, b'r') => { + // ROLE — answered HERE rather than at the connection layer so a + // queued ROLE inside MULTI works: EXEC replays the queue through + // dispatch(), and a connection-layer intercept would have executed + // ROLE immediately at queue time, dropping it from the EXEC array + // and shifting every later result index for the client. + // + // The replication state comes from the process-global handle every + // entry point registers (main.rs, listener.rs, embedded.rs), which + // is the same source INFO reads. Unset (before replication init) + // yields the master form, matching a `None` ReplicationState. + if cmd.eq_ignore_ascii_case(b"ROLE") { + return resp(if args.is_empty() { + identity::role(crate::admin::metrics_setup::get_global_repl_state_arc()) + } else { + Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'role' command", + )) + }); + } + } (7, b'h') => { // HGETALL HEXISTS HOTKEYS if cmd.eq_ignore_ascii_case(b"HGETALL") { diff --git a/src/server/conn/handler_monoio/mod.rs b/src/server/conn/handler_monoio/mod.rs index 12a44173..83ac7b8c 100644 --- a/src/server/conn/handler_monoio/mod.rs +++ b/src/server/conn/handler_monoio/mod.rs @@ -1656,19 +1656,6 @@ pub(crate) async fn handle_connection_sharded_monoio< if dispatch::try_handle_client_admin(cmd, cmd_args, client_id, &conn, &mut responses) { continue; } - // ROLE — connection-layer because the answer lives on - // ConnectionContext.repl_state, not in the keyspace. Post-ACL like - // the other server-introspection intercepts around it. - if cmd_len == 4 && cmd.eq_ignore_ascii_case(b"ROLE") { - responses.push(if cmd_args.is_empty() { - crate::command::identity::role(ctx.repl_state.as_ref()) - } else { - Frame::Error(Bytes::from_static( - b"ERR wrong number of arguments for 'role' command", - )) - }); - continue; - } // CLIENT TRACKING mutates server-side invalidation state — post-ACL // (H-3), matching handler_sharded's placement of all CLIENT subcmds. if cmd_len == 6 diff --git a/src/server/conn/handler_sharded/mod.rs b/src/server/conn/handler_sharded/mod.rs index a25b1316..84e41d70 100644 --- a/src/server/conn/handler_sharded/mod.rs +++ b/src/server/conn/handler_sharded/mod.rs @@ -1048,19 +1048,6 @@ pub(crate) async fn handle_connection_sharded_inner< } // --- REPLICAOF / SLAVEOF --- - // ROLE — connection-layer, because the answer lives on - // ConnectionContext.repl_state rather than in the keyspace. - // Post-ACL, beside the other replication intercepts. - if cmd.eq_ignore_ascii_case(b"ROLE") { - responses.push(if cmd_args.is_empty() { - crate::command::identity::role(ctx.repl_state.as_ref()) - } else { - Frame::Error(Bytes::from_static( - b"ERR wrong number of arguments for 'role' command", - )) - }); - continue; - } if dispatch::try_handle_replicaof(cmd, cmd_args, ctx, &mut responses) { continue; } diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index 6bff6a54..046745a8 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -1669,22 +1669,6 @@ pub async fn handle_connection( } } - // === ROLE === - // Connection-layer, like the monoio/sharded handlers: - // the answer lives on the replication state, not in the - // keyspace, so `dispatch()` (which only receives a - // Database) cannot produce it. - if cmd.eq_ignore_ascii_case(b"ROLE") { - responses.push(if cmd_args.is_empty() { - crate::command::identity::role(repl_state.as_ref()) - } else { - Frame::Error(Bytes::from_static( - b"ERR wrong number of arguments for 'role' command", - )) - }); - continue; - } - // === RESET === // Shares `try_handle_reset` with the other two handlers // rather than re-deriving "default state" here, so the diff --git a/tests/client_identity_introspection.rs b/tests/client_identity_introspection.rs index 9ada1034..ca74c807 100644 --- a/tests/client_identity_introspection.rs +++ b/tests/client_identity_introspection.rs @@ -885,3 +885,53 @@ async fn ci14_handler_single_identity_surface() { token.cancel(); } + +// --------------------------------------------------------------------------- +// ci15 — ROLE must QUEUE inside MULTI, not execute at queue time. +// +// Caught by the client-compat harness, not by this suite, and it was a +// regression introduced by the first cut of this task: ROLE was intercepted at +// the connection layer, ahead of the MULTI queueing step, so `MULTI; ROLE; +// EXEC` replied the role array immediately and then EXEC returned `*0`. The +// damage is worse than a wrong reply — the command silently vanishes from the +// EXEC array, so every LATER result shifts down one index and a client reads +// another command's answer as this one's. +// +// The fix moved ROLE into the shared dispatch table (answered from the +// process-global replication handle), which is also the only way a queued ROLE +// can work: EXEC replays the queue through dispatch(). +#[test] +fn ci15_role_queues_inside_multi_and_keeps_result_alignment() { + let srv = server(1); + let mut c = connect_ready(srv.port); + + assert_eq!(&cmd(&mut c, &["MULTI"])[..], b"+OK\r\n"); + let queued = cmd(&mut c, &["ROLE"]); + assert_eq!( + &queued[..], + b"+QUEUED\r\n", + "ROLE must be QUEUED inside MULTI, not executed at queue time; got {:?}", + String::from_utf8_lossy(&queued) + ); + + // A second command AFTER ROLE is the alignment probe: if ROLE were dropped + // the array would be *1 and PING's reply would land at index 0. + assert_eq!(&cmd(&mut c, &["PING"])[..], b"+QUEUED\r\n"); + + let exec = cmd(&mut c, &["EXEC"]); + assert!( + exec.starts_with(b"*2\r\n"), + "EXEC must return one result per queued command (2); got {:?}", + String::from_utf8_lossy(&exec) + ); + assert!( + exec.windows(6).any(|w| w == b"master"), + "the queued ROLE must have produced the role array inside EXEC; got {:?}", + String::from_utf8_lossy(&exec) + ); + assert!( + exec.ends_with(b"+PONG\r\n"), + "PING must remain the LAST result — if it moved, alignment broke; got {:?}", + String::from_utf8_lossy(&exec) + ); +}