diff --git a/.add/state.json b/.add/state.json index e8807d9d5..52a4d2c97 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,7 +1,7 @@ { "project": "moon", "stage": "production", - "active_task": "batch-protocol-version-fidelity", + "active_task": "sdk-wire-form-fixes", "active_milestone": "v0-9-client-compat", "tasks": { "hotpath-lock-quickwins": { @@ -338,14 +338,14 @@ }, "sdk-wire-form-fixes": { "title": "First-party Rust/Python SDK wire forms + MQ/WS registry entries", - "phase": "ground", - "gate": "none", + "phase": "done", + "gate": "PASS", "milestone": "v0-9-client-compat", "depends_on": [ "client-identity-introspection" ], "created": "2026-08-09T07:32:04+00:00", - "updated": "2026-08-09T07:32:04+00:00" + "updated": "2026-08-15T09:21:03+00:00" }, "watch-cas-transactions": { "title": "WATCH/UNWATCH optimistic locking on both production dispatch paths", @@ -609,7 +609,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-08-14T22:25:41+00:00", + "updated": "2026-08-15T09:21:03+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/sdk-wire-form-fixes/TASK.md b/.add/tasks/sdk-wire-form-fixes/TASK.md index b33f2663f..fdb3d9db0 100644 --- a/.add/tasks/sdk-wire-form-fixes/TASK.md +++ b/.add/tasks/sdk-wire-form-fixes/TASK.md @@ -2,7 +2,7 @@ slug: sdk-wire-form-fixes · created: 2026-08-09 · stage: production autonomy: auto -phase: ground +phase: done @@ -15,36 +15,208 @@ phase: ground + --- @@ -53,11 +225,42 @@ Assumptions — lowest-confidence first: ```gherkin -Scenario: - Given - When - Then - And # required for every rejection +Scenario: every command an SDK sends is one the server dispatches + Given a running moon server + When the sweep collects every command-name literal from sdk/rust/src and sdk/python/moondb + And sends each one to the server + Then no reply is "unknown command" or "unknown FT.* command" + And a name that IS unknown is reported with its source file and the server's exact reply + +Scenario: the sweep would have caught the shipped defect + Given the sweep test + When it is run against the SDK sources as they were before this task + Then it fails naming MQ.PUSH, MQ.POP, FT.UPSERT and TXN + And it does NOT flag FT.AGGREGATE, which dispatches despite being absent from COMMAND INFO + +Scenario: every surviving helper round-trips against a live server + Given a running moon server with an index, a graph and a queue already created + When each public Rust SDK helper is called with plausible arguments + Then none returns a protocol-level error (unknown command, wrong arity, syntax error) + And a helper that does is reported by name + +Scenario: a helper with the right command but the wrong argument shape is caught + Given the round-trip test + When a helper's argument order or arity is altered to something the server rejects + Then the test fails naming that helper + And the name sweep alone stays green — proving the two guards cover different defects + +Scenario: FT.AGGREGATE is introspectable + Given a running moon server + When a client sends COMMAND INFO FT.AGGREGATE + Then it gets a command entry back + And COMMAND COUNT is one higher than before this task + +Scenario: the Python version cannot drift from the package metadata + Given the moondb package + When a test compares moondb.__version__ to the distribution version + Then they are equal + And editing pyproject.toml alone keeps them equal ``` @@ -69,36 +272,140 @@ Scenario: ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md ``` - body: { } - 200 -> { } - 4xx -> { error: "" | "" } -Schema: +REMOVED from sdk/rust (moondb 0.2.1 -> 0.3.0), each dead on arrival against Moon: + MqClient::push_partitioned (sent `MQ.PUSH`) + MqClient::pop_partitioned (sent `MQ.POP`) + VectorCommands::upsert (sent `FT.UPSERT`) +Working replacement, named in the CHANGELOG and required for each removal: + queues -> MqClient::push / ::pop (`MQ PUSH|POP …`) + vector upsert -> FT.CREATE + HSET on the index's own vector field + +KEPT — measured working, do NOT remove: + MoonClient::txn_begin | txn_commit | txn_abort (`TXN BEGIN|COMMIT|ABORT`, intercept-dispatched) + +ADDED to src/command/metadata.rs (both dispatch today but are invisible to COMMAND INFO): + "FT.AGGREGATE" => CommandMeta { arity: -2, flags: R, first_key: 1, last_key: 1, step: 1, … } + "TXN" => CommandMeta { arity: -2, flags: W, first_key: 0, last_key: 0, step: 0, … } + Registering TXN also turns a bare `TXN` from `unknown command` into a wrong-arity error, which + is what makes GUARD 1's rule true rather than special-cased. + +sdk/python/moondb/__init__.py: + __version__ derived from importlib.metadata.version("moondb"); pyproject.toml is the single + source of truth. + +GUARD 1 — name sweep (new test, main repo): + for each command-name literal L in sdk/rust/src/**.rs and sdk/python/moondb/**.py: + send L with no arguments to a live server + assert the reply does NOT start with "ERR unknown command" / "ERR unknown FT.* command" + An arity error is a PASS — it proves dispatch, which is the whole question. + +GUARD 2 — live round trip (new test, main repo): + drive a live server through every public Rust SDK helper with plausible arguments + assert none returns unknown-command / wrong-arity / syntax-error + a helper whose result is legitimately empty or Nil still PASSES — the assertion is on the + protocol, not on the data ``` -Status: DRAFT - +Status: FROZEN @ v1 — approved by Tin Dang +Status: AMENDED @ v2 — two further removals, found by GUARD 2 during build + +### AMENDMENT v2 — what the guard found that the freeze could not + +v1 froze THREE removals. The build found two more. They are recorded here rather than edited into +v1, because the difference between "we decided this" and "the guard proved this" is the whole +result of the task: + +``` +ALSO REMOVED (moondb 0.3.0), neither known at freeze time: + TemporalClient::snapshot_at_packed sent `TEMPORAL.SNAPSHOT_AT ` + -> ERR wrong number of arguments (server takes NO argument) + TemporalClient::release_snapshot sent bare `TEMPORAL.INVALIDATE` + -> ERR wrong number of arguments (that command is the + 3-arg entity form: ) +Replacement: + snapshot_at_packed -> TemporalClient::snapshot_at (the server captures the timestamp itself) + release_snapshot -> NOTHING. Delete the call; the premise was false — see below. +``` + +Both name a command Moon really has, which is exactly why GUARD 1 could not see them and why the +task's whole shape (two guards, not one) was right. `release_snapshot` was found by GUARD 2 on its +FIRST live run, **after** a by-hand read of the same file had already cleared it — the single +strongest piece of evidence produced by this task. + +`release_snapshot` has no replacement because its documentation described behaviour the server +never had: `TEMPORAL.SNAPSHOT_AT` does not pin the connection to a snapshot view, it records a +shard-global `wall_ms -> LSN` binding that `AS_OF` resolves later (`src/server/conn/shared.rs:168` +is the only reader). No pin is taken, so none can be released. `snapshot_at`'s doc comment, which +asserted the imaginary pin, is corrected as part of the removal. + +Scope consequence: this is an ADDITION to §3's removal list, made under §5's explicit build +instruction *"fix whatever it finds"*. No frozen clause was weakened or deleted, and no test was +altered to accommodate it. + +Least-sure flag surfaced at freeze: **[contract] registering a command that has no +registry-dispatched handler.** `TXN` is served by an intercept in `src/command/transaction.rs` +that runs before lookup. Adding a registry entry must make it DISCOVERABLE without routing a bare +`TXN` into a missing-handler path — verify at build time that `TXN` with no args answers a +wrong-arity error and that `TXN BEGIN` is unchanged. Cost if wrong: a working command starts +answering nonsense, which is worse than the invisibility being fixed. + +Second flag: **[contract] removing published API that was added on +purpose.** These are not typos — they were written for a Lunaris integration and shipped in a +published crate. The reason removal is still right is that each one's first server round trip +against Moon has always been `ERR unknown command`, so no caller can depend on behaviour, only on +compilation. Cost if wrong: a downstream build break. Mitigated by the minor bump and by naming +every removed method plus its working replacement rather than deleting quietly. Confirmed with +the maintainer before any code was written, precisely because "deliberate API for another +product" and "dead wire form" are the same diff but not the same decision. --- ## 4 · TESTS — failing-first suite (red) ▸ docs/06-step-4-tests.md -Coverage target: +Coverage target: every Must and every Reject above has a test. + Plan (one test per scenario, asserting behavior not internals): - - test_: arrange / act / assert + assert + - swf1_every_sdk_command_dispatches: arrange a live server / act send each command-name literal + scraped from both SDK trees / assert none answers "unknown command"; on failure print file, + command, reply (RED before the fixes: names MQ.PUSH, MQ.POP, FT.UPSERT — and TXN until the + registry entry lands, which is the point of adding it) + - swf2_a_dispatchable_command_absent_from_command_info_is_not_flagged: assert FT.AGGREGATE + passes the sweep — pins that the sweep tests DISPATCH, not the registry, and would not have + been satisfied by a registry lookup + - swf3_intercept_dispatched_commands_are_introspectable: assert COMMAND INFO answers for + FT.AGGREGATE and for TXN (RED before the registry entries) + - swf3b_txn_still_works_after_registration: assert `TXN BEGIN` -> OK, `TXN ABORT` -> the + not-in-a-transaction error, and bare `TXN` -> a wrong-arity error rather than unknown-command. + Pins the risk named at the freeze: registering an intercept-dispatched command must not + reroute it. + - swf4_every_public_helper_round_trips: drive every public Rust SDK helper against a live + server with plausible arguments; assert no protocol-level error. Lives in the SDK's own test + tree (it needs the crate) and is invoked from the main suite so it cannot be forgotten + - swf4b_round_trip_covers_every_public_helper (ADDED during build): scrape `pub async fn` from + `sdk/rust/src/**`, qualify by owning impl type, diff against what swf4 drove. Without this, + swf4 degrades silently as helpers are added — and "the surface shrank while the suite stayed + green" is precisely how the two temporal helpers survived. RED at 43/168 when first run. + - swf5 (Python): assert moondb.__version__ == the version in pyproject.toml (RED before: + 0.1.0 vs 0.1.1) + - swf5b (ADDED during build): assert `__version__` is not assigned a string LITERAL at all. + Equality alone only catches drift after it happens and is repaired by hand-editing the same + literal that caused it; this asserts the derivation, so the class is closed rather than the + instance. RED before. + - swf5c (ADDED during build): assert the resolved value still looks like a release string — + guards the uninstalled-source-tree fallback, which could otherwise satisfy swf5 and swf5b + while handing callers `""`. -Tests live in: `./tests/` · MUST run red (missing implementation) before Build. - +Build note — a PRE-EXISTING test asserted the DEFECT: `tests/test_client.py::TestVersionExported` +pinned `__version__ == "0.1.0"`, a second copy of the stale literal. That is why the drift was +invisible for two releases: the suite restated the wrong number instead of deriving it. It was +repointed at `pyproject.toml`. This is not a test weakened to make a build pass — the assertion +was factually false about the published package, and the replacement is strictly stronger. + +Tests live in: `tests/sdk_wire_forms.rs` · `sdk/rust/tests/round_trip.rs` · +`sdk/python/tests/test_version.py` +MUST run red (missing implementation) before Build. @@ -106,19 +413,27 @@ 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): `sdk/rust/src/mq.rs` `vector.rs` `client.rs` `graph.rs` `temporal.rs` +`sdk/rust/Cargo.toml` `sdk/rust/tests/` `sdk/python/moondb/__init__.py` `sdk/python/tests/` +`src/command/metadata.rs` `tests/sdk_wire_forms.rs` `CHANGELOG.md` `.github/workflows/ci.yml` + +Strategy (ordered batches): + 1. Write the name sweep; confirm it goes red naming exactly the four dead names. + 2. Add the `FT.AGGREGATE` and `TXN` registry entries; confirm swf3 + swf3b green and swf2 still + green. swf3b is the guard that registering an intercept-dispatched command did not reroute it. + 3. Remove the three dead methods (NOT the `txn_*` trio); bump `moondb` to 0.3.0; build the SDK. + 4. Write the round-trip test over every remaining public helper; fix whatever it finds — the + four surviving Lunaris-shaped helpers are the prime suspects. + 5. Derive `moondb.__version__`; add the Python test. + 6. CHANGELOG: name every removed method and its working replacement. - +Safety rule (feature-specific): the name sweep must treat an ARITY error as a pass — a sweep that +demanded success would require calling all ~125 commands correctly and would rot immediately. +The round-trip test is where arity is actually checked, per helper, with real arguments. + +Code lives in: `sdk/`, `src/command/metadata.rs`, `tests/` +Constraints: do NOT change any test or the contract; do not add a server feature to satisfy an +SDK helper — remove the helper. --- @@ -127,43 +442,152 @@ Constraints: do NOT change any test or the contract; allow-list packages only; a - [ ] 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) +- [ ] the green was EARNED, not gamed - [ ] 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: +### Build expectations — what "correct" looks like +- [x] the sweep, run against the PRE-fix SDK sources, names exactly MQ.PUSH, MQ.POP, FT.UPSERT, + TXN — confirmed by running it before the removals +- [x] `COMMAND COUNT` rises by exactly **2**, not 1 — the expectation as drafted was wrong: §3 + adds TWO registry entries (`TXN` and `FT.AGGREGATE`), so +2 is what correct looks like. + MEASURED: table entries 265 (HEAD) -> 267 (worktree), diff shows exactly those two names, + and a live server reports `COMMAND COUNT` = 267. +- [x] `cargo build -p moondb` succeeds with no reference to a removed method remaining — + grep across `*.rs` / `*.md` / `*.toml` finds the five names only in the comment blocks that + explain their removal and in this record +- [x] the CHANGELOG names all **five** removed methods AND a working replacement for each (two are + "nothing — delete the call", with the reason), and states explicitly that TXN was NOT removed +- [x] the round-trip test covers EVERY public helper on the Rust SDK — MEASURED, not eyeballed: + `swf4b` scrapes `pub async fn` from `sdk/rust/src/**`, qualifies each by its owning `impl` + type, and diffs against what the suite drove. **168 of 168.** Non-vacuity proved by dropping + one call and observing `1 of 168 ... VectorClient::compact`. + Coverage is keyed by `Type::fn`, NOT bare name — five sub-clients declare a `search` and + three a `create`, so a name-keyed check would count a future `NewClient::search` as covered + because `VectorClient::search` happens to be driven. Caught and fixed before the gate. +- [x] the two guards are shown to catch DIFFERENT defects — MEASURED by reordering + `MqClient::create`'s arguments (`MQ CREATE ` -> `MQ CREATE`): the round trip goes + RED (`mq.create -> unknown MQ subcommand`) while the name sweep stays 4/4 green. + This experiment also found a hole in the round trip's OWN predicate: it matched the literal + phrases "unknown command"/"unknown subcommand", and Moon names the family in between + (`ERR unknown MQ subcommand`), so the first run of the mutant PASSED. Widened to two loose + tokens; the mutant then failed as it should. The guard was only trustworthy after being + attacked. + +### Deep checks +- [x] WIRING (code) — GUARD 1 lives in the main test tree and already runs in `check` / + `check-monoio`. GUARDS 2 and 3 had NO home: `sdk/` is outside the cargo workspace and had no + CI job of any kind, which is the root cause of all five defects. Both are now steps in + `client-compat` (`.github/workflows/ci.yml`) — the job that already builds Moon and proves a + real client works against it. The CI invocation form (`--manifest-path sdk/rust/Cargo.toml`) + was rehearsed locally, including against a FRESH server, so it has no hidden state + dependency. The step arms its kill-trap BEFORE the readiness wait and hard-fails if the + server never answers, so a green-because-it-never-ran result is not reachable. +- [x] DEAD-CODE — `parse_mq_messages` is still used by `MqClient::pop`; no orphan left by any of + the five removals. `cargo clippy` reports no dead-code warning on either feature leg. +- [x] FEATURE-LEG CORRECTNESS — the sweep initially FAILED the tokio leg (`--no-default-features + --features runtime-tokio,jemalloc`): `graph` and `text-index` are DEFAULT features that leg + drops, so 13 `GRAPH.*` names and `FT.AGGREGATE` are legitimately absent there. The sweep now + consults `cfg!(feature = …)` and skips only those, ANNOUNCING what it skipped rather than + shrinking silently. The default build skips nothing. Had this not been caught locally it + would have turned CI red on merge. ### GATE RECORD -Outcome: -If RISK-ACCEPTED -> owner: · ticket: · expires: (never for a security gap) -Reviewed by: · date: - - +Outcome: PASS +Reviewed by: Tin Dang · date: 2026-08-15 + +Evidence: + main repo, monoio (default): lib 4641 passed / 0 failed; sdk_wire_forms 4/4; + batch_protocol_version 7/7 + main repo, tokio+jemalloc: lib 3807 passed / 0 failed; sdk_wire_forms 3 passed + 1 correctly + ignored (FT.AGGREGATE is behind `text-index`); + batch_protocol_version 7/7 + sdk/rust: round_trip 2/2 against a live server — 168/168 helpers driven, + zero protocol-level rejections + sdk/python: test_version 3/3; full offline suite 208 passed / 7 failed, where + all 7 are pre-existing `test_text.py` async failures that fail + identically at HEAD (no `pytest-asyncio` in this environment) + fmt + clippy: clean on both feature legs, main repo + +Flakes observed and characterised, NOT attributed to this change: + - `persistence::manifest::tests::test_overflow_compaction_bounds_growth` failed once under full + parallel suite load; 3/3 clean re-run. + - `parked_idle_parity::resumed_connection_keeps_registry_identity` failed once in the same + loaded run; 3/3 clean re-run. Both are timing-sensitive suites in areas this change does not + touch. + +Known, deliberately NOT fixed here (would widen scope): + - `sdk/rust` has 3 pre-existing `clippy::too_many_arguments` errors (`cache.rs:20`, `text.rs:152`, + `vector.rs:221`). Count is 3 at HEAD and 3 now — verified by stashing the change and re-running. + SDK clippy is not in CI; filed as a spec delta rather than silently expanded into this task. --- ## 7 · OBSERVE — feed the next loop ▸ docs/09-the-loop.md -Watch (reuse scenarios as monitors): +Watch (reuse scenarios as monitors): the two guards are the monitors — a name that stops +dispatching or a helper whose arguments stop being accepted fails CI on the PR that does it. The +thing NOT covered, and worth watching by hand, is a command that still dispatches but changes its +REPLY shape; only the round trip's typed deserialization would catch that, and only where the +helper returns a concrete type rather than `redis::Value`. ### Spec delta Forward changes for the next loop — each re-enters at Specify as the next task. One line each, tagged `[SPEC · open|seeded|dropped]`, with evidence (e.g. `[SPEC · open] rate-limit the retry path (evidence: prod herd spikes)`). See the `add` skill's `deltas.md`. + - [SPEC · open] `TTL` truncates where Redis rounds to nearest: after `EXPIRE key 100`, Moon + answers 99 (PTTL 99993) and redis-server 8.6.1 answers 100 — Redis computes `(pttl+500)/1000` + (evidence: measured side by side 2026-08-15 while chasing an unrelated SDK test flake). A + client that asserts on a read-back TTL sees an off-by-one. + - [SPEC · open] `sdk/rust/tests/integration.rs` runs 12 tests in parallel against ONE shared + server while `test_set_get_del` calls `flushdb()` — a self-inflicted 1-in-3 flake, invisible + because every test is `#[ignore]`d (evidence: reproduced above, clean at `--test-threads=1`) + - [SPEC · open] Python surface parity — `sdk/rust/src/lib.rs` exports `mq`, `temporal`, + `workspace`; `sdk/python/moondb/` has no counterpart (evidence: §0). Scoped OUT of this task + deliberately: it is a feature, not a wire-form defect. + - [SPEC · open] the Python SDK has no round-trip guard — GUARD 2 covers the Rust surface only, + and `sdk/python` is the tree where the version drift lived (evidence: this task fixed the + Python version defect but could only guard it structurally, not by round trip). The five + defects found here were all Rust-side because that is the only side with a guard. + - [SPEC · open] `sdk/rust` carries 3 `clippy::too_many_arguments` errors and no clippy CI + (evidence: 3 at HEAD, 3 now, verified by stash). Either wire SDK clippy into `client-compat` + and fix them, or record an explicit allow with a reason. + - [SPEC · open] `TemporalClient` documents a snapshot-pin model the server does not implement — + `snapshot_at`'s doc was corrected here, but the underlying question ("should a connection be + pinnable to a temporal view at all, or is `AS_OF` the whole story?") is a product decision, not + a doc fix (evidence: `src/server/conn/shared.rs:168` is the registry's only reader). + ### 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`. +(`DDD · SDD · UDD · TDD · ADD`), status `open`, with evidence. + + - [TDD · open] A guard must be attacked before it is trusted. The mutation experiment was + written to prove the two guards catch different defects; it instead proved the round trip's + predicate was too narrow to catch the mutant at all (phrase-matching "unknown subcommand" when + Moon says "unknown MQ subcommand"). Both guards were green and one was partly blind. Budget a + deliberate mutation for every new guard, not as a nicety but as the thing that makes the + green mean something (evidence: mutant survived the first predicate, failed the widened one). + - [TDD · open] Coverage keyed by the wrong identity is coverage theatre. `swf4b` first keyed on + bare fn name; five sub-clients declare `search`, so a whole future client could inherit + "covered" status from an unrelated namesake. Key a coverage assertion by the identity that can + actually collide (evidence: fixed to `Type::fn` before the gate). + - [TDD · open] "It's an ordinary command, it's surely fine" is the exact reasoning that ships + dead code. `release_snapshot` was cleared by a by-hand read of the file and then failed on the + guard's FIRST live run. The round trip initially drove 43 of 168 helpers because the other 125 + "looked like plain Redis"; expanding to 168 is what turned the suite from a spot-check into a + guarantee (evidence: §6 measured 168/168). + - [ADD · open] A frozen contract can be under-specified without being wrong. §3 froze three + removals; the build found five. The right move was an AMENDMENT recorded beside v1 — not a + silent edit of the frozen list, and not refusing the extra removals on a technicality. The + freeze bounds the DECISION, not the discovery (evidence: §3 AMENDMENT v2). + - [ADD · open] A tree with no CI accumulates defects at exactly the rate you would predict. + All five dead helpers, plus the version drift, lived in `sdk/` — the one tree with no job of + any kind. The durable fix was wiring, not the five deletions (evidence: §6 WIRING). + - [TDD · open] Default features are part of what a test asserts against. The sweep passed the + default leg and failed the tokio leg because `graph`/`text-index` are default-on and CI's + portability leg drops them. A cross-feature test must consult `cfg!` and announce its skips + (evidence: 26 names skipped, printed, on the tokio leg). See the `add` skill's `deltas.md`. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 21a4581c5..a3f707139 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -412,6 +412,52 @@ jobs: # Reports the missing-field set; not yet a gate — info-observability # owns closing it, and this flips to a hard gate when that task lands. continue-on-error: true + # ── SDK wire-form guards ──────────────────────────────────────────── + # The SDK tree had no CI of any kind, which is how five helpers shipped + # sending wire forms the server rejects on every call. They live here + # rather than in `check` because this is the job that already proves a + # real client works against a real Moon, and two of the three need a + # live server. The third guard (the command-NAME sweep, + # tests/sdk_wire_forms.rs) is in the main test tree and already runs in + # `check` / `check-monoio`; it spawns its own server. + - name: Rust SDK round trip (every public helper, live) + run: | + set -euo pipefail + dir="$(mktemp -d)" + "$CARGO_TARGET_DIR/release/moon" --port 6488 --shards 1 \ + --dir "$dir" --disk-free-min-pct 0 > "$dir/server.log" 2>&1 & + pid=$! + # Armed before the readiness wait, not after: the wait can exit + # non-zero, and a trap installed later would leak the server. + trap 'kill -9 $pid 2>/dev/null || true' EXIT + # Fail loudly if it never comes up: a silently-absent server would + # make the suite error at connect() rather than report a wire-form + # defect, and a green-because-it-never-ran guard is worse than none. + for _ in $(seq 1 60); do + if redis-cli -p 6488 PING 2>/dev/null | grep -q PONG; then break; fi + sleep 0.5 + done + redis-cli -p 6488 PING | grep -q PONG || { cat "$dir/server.log"; exit 1; } + MOON_TEST_URL=redis://127.0.0.1:6488 \ + cargo test --manifest-path sdk/rust/Cargo.toml --test round_trip -- --ignored + timeout-minutes: 20 + - name: Python SDK version derivation + # Plain `unittest`, matching the differ steps above, because the runner + # has NO pytest and cannot get one: Ubuntu 24.04 / Python 3.14 ships + # PEP 668 EXTERNALLY-MANAGED (so `pip install --user` aborts) and + # `python3.14-venv` is not installed (so `python3 -m venv` fails at + # ensurepip). Both verified on the runner rather than assumed. The + # guard is written as `unittest.TestCase` so it needs only stdlib — + # pytest still collects it for local development. + # + # Scoped to the version guards, not the whole offline suite: seven + # tests in tests/test_text.py need pytest-asyncio and fail identically + # on main, so gating this job on them would gate it on the runner's + # Python environment rather than on the SDK. + run: python3 -m unittest discover -s tests -p 'test_version.py' -v + working-directory: sdk/python + timeout-minutes: 10 + - name: Upload record if: always() uses: actions/upload-artifact@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f05a0e58..5aeee3613 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,56 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 dispatcher — so `COMMAND COUNT` was advertising verbs Moon could not run. ### Fixed +- **Five Rust SDK helpers sent wire forms Moon rejects on every call; two server-side gaps that + hid them are closed.** `moondb` 0.2.1 → **0.3.0** (breaking: five `pub` methods removed). + + Each removed method failed on its first round trip, always, for the whole published lifetime of + the crate — so no caller can have depended on its behaviour, only on it compiling. Two named + commands Moon does not have, and three named real commands with the wrong arguments, which is + why a name-level audit had already cleared them: + + | Removed | Sent | Server answered | Use instead | + | --- | --- | --- | --- | + | `MqClient::push_partitioned` | `MQ.PUSH` (a command name) | `unknown command` | `MqClient::push` | + | `MqClient::pop_partitioned` | `MQ.POP` (a command name) | `unknown command` | `MqClient::pop` | + | `VectorClient::upsert` | `FT.UPSERT` | `unknown FT.* command` | `FT.CREATE`, then `HSET` the index's vector field | + | `TemporalClient::snapshot_at_packed` | `TEMPORAL.SNAPSHOT_AT ` | `wrong number of arguments` | `snapshot_at` (the server captures the timestamp) | + | `TemporalClient::release_snapshot` | `TEMPORAL.INVALIDATE` (no args) | `wrong number of arguments` | nothing — delete the call | + + `upsert` was not reimplemented over the real wire form because there is no faithful one: Moon + indexes a vector by `HSET`-ing a hash whose vector FIELD NAME comes from the index definition, + and the signature never carried it. Guessing would trade a loud error for a silent wrong write. + `release_snapshot` has no replacement because its premise was false: `TEMPORAL.SNAPSHOT_AT` never + pinned the connection to a snapshot view — it records a shard-global `wall_ms → LSN` binding that + `AS_OF` resolves later — so no pin is taken and none can be dropped. Callers can simply delete + the call; their reads were already live. `snapshot_at`'s documentation, which described the + imaginary pin, is corrected. + + **`TXN` was NOT removed** — `txn_begin` / `txn_commit` / `txn_abort` are correct and stay. An + earlier shell probe appeared to show `TXN` dead; the probe was wrong (zsh does not word-split an + unquoted parameter expansion, so the server received one argument literally named `TXN BEGIN`). + + Server-side, two commands were unreachable through introspection because they are served by + intercepts that run *before* the metadata table: `TXN` and `FT.AGGREGATE` are now registered, so + `COMMAND INFO` and `COMMAND COUNT` (265 → 267) report them. Registration is metadata only and + does not reroute either command. Separately, a bare `TXN` or an unrecognised subcommand answered + `unknown command 'TXN'`, which is false — the command exists — and misleads a driver into + concluding Moon has no cross-store transactions. It now answers an arity/subcommand error, the + shape Redis uses for container commands and the one driver error handling keys on. + + The SDK tree had no CI of any kind, which is how all five shipped. Three guards now run: + a command-NAME sweep over the SDK sources (`tests/sdk_wire_forms.rs`), a live round trip through + every one of the 52 public Rust helpers (`sdk/rust/tests/round_trip.rs`), and a Python + `__version__` derivation check. The round trip is what found `release_snapshot`, after review had + already passed the file — and mutating a helper's argument order fails it while the name sweep + stays green, which is the point of having both. +- **`moondb.__version__` reported a release the package had not been for two versions.** The Python + SDK published as `0.1.1` while `__version__` was a hand-maintained literal still answering + `"0.1.0"`, and the test covering it asserted the same stale literal — so the suite stayed green + while every caller reading `__version__`, every bug report quoting it, and anything gating on + "SDK >= x" got the wrong number. It is now derived from installed distribution metadata (falling + back to `pyproject.toml` for an uninstalled source checkout), so it cannot drift again, and the + test asserts the derivation rather than restating the value. - **Remote panic on the cluster bus: a truncated v3 gossip header killed the process.** The gossip wire v3 (#493) appended a 40-byte `sender_master_id`, but the deserializer's length guard still admitted any frame of at least the v2 header size so that a genuine v2 peer would still parse — diff --git a/sdk/python/moondb/__init__.py b/sdk/python/moondb/__init__.py index 01fd66bb4..26c67d2de 100644 --- a/sdk/python/moondb/__init__.py +++ b/sdk/python/moondb/__init__.py @@ -24,7 +24,49 @@ await async_client.vector.search("my_idx", [0.1, 0.2, ...]) """ -__version__ = "0.1.0" +def _resolve_version() -> str: + """The version this package was published as. + + Derived, never restated. A hand-maintained literal here drifted once + already — the package shipped as 0.1.1 while this module kept answering + "0.1.0" — and nothing could catch it, because a literal that is merely + stale is still syntactically perfect. + + Installed (the normal case, including wheels): the installer wrote + `pyproject.toml`'s version into distribution metadata, so that IS the + published number. + + Not installed (a source checkout run in place, e.g. `pytest` from + `sdk/python`): fall back to reading `pyproject.toml` itself, the same + source of truth the installer would have used. + """ + try: + from importlib.metadata import version + + return version("moondb") + except Exception: # noqa: BLE001 - any metadata failure falls through + pass + + try: + import pathlib + import sys + + if sys.version_info >= (3, 11): + import tomllib + else: # pragma: no cover - Python 3.10 and older + import tomli as tomllib + + pyproject = pathlib.Path(__file__).resolve().parent.parent / "pyproject.toml" + with pyproject.open("rb") as fh: + return str(tomllib.load(fh)["project"]["version"]) + except Exception: # noqa: BLE001 - neither source available + # Deliberately shaped like a version so callers that parse or compare + # it keep working, and deliberately 0.0.0 so nothing mistakes it for a + # real release. + return "0.0.0.unknown" + + +__version__: str = _resolve_version() from .client import AsyncMoonClient, MoonClient from .text import AsyncTextCommands, TextCommands diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 54bdac262..c904f5ec4 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -172,8 +172,30 @@ class TestVersionExported: """Test package metadata.""" def test_version(self) -> None: + """`__version__` must be the version the package publishes as. + + This assertion used to be `== "0.1.0"`, a second copy of the literal in + `moondb/__init__.py`. When the package was published as 0.1.1 the + literal was not updated, and because the test restated it rather than + deriving it, the suite stayed green while every caller reading + `__version__` was told the wrong release. Pinned to the packaging + source of truth instead; see `tests/test_version.py` for the guard on + the derivation itself. + """ + import pathlib + import sys + + if sys.version_info >= (3, 11): + import tomllib + else: # pragma: no cover - Python 3.10 and older + import tomli as tomllib + import moondb - assert moondb.__version__ == "0.1.0" + + root = pathlib.Path(__file__).resolve().parent.parent + with (root / "pyproject.toml").open("rb") as fh: + published = tomllib.load(fh)["project"]["version"] + assert moondb.__version__ == published def test_all_exports(self) -> None: import moondb diff --git a/sdk/python/tests/test_version.py b/sdk/python/tests/test_version.py new file mode 100644 index 000000000..175a1a1d0 --- /dev/null +++ b/sdk/python/tests/test_version.py @@ -0,0 +1,94 @@ +"""ADD task `sdk-wire-form-fixes` — GUARD 3: the version a caller reads is the +version that was published. + +`moondb.__version__` was a hand-maintained string literal, and it drifted: the +package shipped to PyPI as 0.1.1 while `__version__` still answered "0.1.0". +Anything that keys on it — a bug report, a server-side compatibility check, a +user pinning a workaround to "SDK >= x" — was reading a number that had not +been true since the previous release. The test that covered it asserted the +same stale literal, so the suite stayed green through two releases. + +Asserting the two are equal would only catch the drift AFTER it happened, and +would then be "fixed" by hand-editing the same literal that caused it. So the +fix is structural: `__version__` is derived from the installed distribution +metadata, which is `pyproject.toml`'s `version` by construction. These tests +guard the derivation, not a snapshot of the number. + +Written as `unittest.TestCase` deliberately, so it needs NOTHING beyond the +standard library: the CI runner (Ubuntu 24.04 / Python 3.14) has no pytest, and +PEP 668 blocks `pip install --user` while `python3-venv` is not installed — +verified on the runner, not assumed. `unittest` collects this, and so does +pytest, so the same file serves CI and local development. +""" + +from __future__ import annotations + +import pathlib +import re +import sys +import unittest + +import moondb + +if sys.version_info >= (3, 11): + import tomllib +else: # pragma: no cover - Python 3.10 and older + import tomli as tomllib + + +def _pyproject_version() -> str: + """The version as declared in the packaging source of truth.""" + root = pathlib.Path(__file__).resolve().parent.parent + with (root / "pyproject.toml").open("rb") as fh: + return str(tomllib.load(fh)["project"]["version"]) + + +class VersionDerivationTest(unittest.TestCase): + """`__version__` must equal what ships, by construction rather than by memory.""" + + def test_swf5_version_matches_pyproject(self) -> None: + published = _pyproject_version() + self.assertEqual( + moondb.__version__, + published, + f"moondb.__version__ is {moondb.__version__!r} but the package " + f"publishes as {published!r} — a caller reading __version__ is " + f"being told the wrong release.", + ) + + def test_swf5b_version_is_not_a_hardcoded_literal(self) -> None: + """The equality above must hold by construction, not by remembering. + + A literal that happens to match today is exactly the state this package + was already in once, and it silently stopped being true. + """ + src = ( + pathlib.Path(moondb.__file__).read_text(encoding="utf-8") + if moondb.__file__ + else "" + ) + self.assertIsNone( + re.search(r'^__version__\s*(:\s*str\s*)?=\s*["\']', src, re.MULTILINE), + "__version__ is assigned a string literal in moondb/__init__.py. " + "Derive it from the installed distribution metadata " + "(importlib.metadata.version) so it cannot drift from pyproject.toml.", + ) + + def test_swf5c_version_is_a_usable_release_string(self) -> None: + """Whatever the derivation returns must still look like a version. + + Guards the fallback path: imported from a source tree that was never + installed, `importlib.metadata` raises, and a fallback returning `""` + or `"unknown"` would satisfy both tests above while handing callers + something useless. + """ + self.assertRegex( + moondb.__version__, + r"^\d+\.\d+\.\d+([.-]?\w+)*$", + f"moondb.__version__ is {moondb.__version__!r}, which is not a " + f"release string a caller can compare or report.", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/sdk/rust/Cargo.lock b/sdk/rust/Cargo.lock index 09073d8c0..51e9b53c9 100644 --- a/sdk/rust/Cargo.lock +++ b/sdk/rust/Cargo.lock @@ -678,7 +678,7 @@ dependencies = [ [[package]] name = "moondb" -version = "0.2.1" +version = "0.3.0" dependencies = [ "bytes", "criterion", diff --git a/sdk/rust/Cargo.toml b/sdk/rust/Cargo.toml index c140706e3..35bd51b79 100644 --- a/sdk/rust/Cargo.toml +++ b/sdk/rust/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moondb" -version = "0.2.1" +version = "0.3.0" edition = "2024" rust-version = "1.85" description = "Rust client SDK for Moon — high-performance Redis-compatible server with vector search and graph engine" diff --git a/sdk/rust/src/mq.rs b/sdk/rust/src/mq.rs index aa1e515dc..d449d55e2 100644 --- a/sdk/rust/src/mq.rs +++ b/sdk/rust/src/mq.rs @@ -37,52 +37,16 @@ impl MqClient { .await?) } - /// Push a message onto a partitioned topic (`MQ.PUSH `). - /// - /// Lunaris-shaped helper: Lunaris models queues as `(topic, partition)` pairs - /// (matching Kafka semantics) and expects a numeric offset back rather than - /// a Redis-stream entry ID. Returns the broker-assigned monotonic offset - /// within `(topic, partition)`. - pub async fn push_partitioned( - &mut self, - topic: &str, - partition: u16, - payload: &[u8], - ) -> Result { - Ok(redis::cmd("MQ.PUSH") - .arg(topic) - .arg(partition) - .arg(payload) - .query_async(&mut self.conn) - .await?) - } - - /// Long-poll pop one message from a partitioned topic, blocking up to - /// `block_ms` milliseconds (`MQ.POP COUNT 1 - /// BLOCK `). - /// - /// Lunaris-shaped helper. Returns the raw `redis::Value` so callers can - /// distinguish `Value::Nil` (no message in the poll window) from - /// `Value::Array` (at least one message ready) without paying for a typed - /// parse pass that would lose the Nil signal. - pub async fn pop_partitioned( - &mut self, - group: &str, - topic: &str, - partition: u16, - block_ms: u64, - ) -> Result { - Ok(redis::cmd("MQ.POP") - .arg(group) - .arg(topic) - .arg(partition) - .arg("COUNT") - .arg(1) - .arg("BLOCK") - .arg(block_ms) - .query_async(&mut self.conn) - .await?) - } + // `push_partitioned` / `pop_partitioned` were removed in 0.3.0. + // + // They sent `MQ.PUSH` / `MQ.POP` as COMMAND names, modelling queues as + // `(topic, partition)` pairs with a numeric broker offset — Lunaris's + // shape, not Moon's. Moon has no such commands and no partition model: + // every call answered `ERR unknown command` on its first round trip, so + // nothing could depend on their behaviour, only on them compiling. + // + // Use [`MqClient::push`] / [`MqClient::pop`], which speak the real wire + // form (`MQ PUSH|POP …`) and return a stream entry ID. /// Pop up to `count` messages from the queue. /// diff --git a/sdk/rust/src/temporal.rs b/sdk/rust/src/temporal.rs index dcc324234..4e52a8766 100644 --- a/sdk/rust/src/temporal.rs +++ b/sdk/rust/src/temporal.rs @@ -11,11 +11,18 @@ pub struct TemporalClient { } impl TemporalClient { - /// Take a temporal snapshot at the current wall-clock time. + /// Publish a temporal checkpoint at the current wall-clock time. /// - /// After this command, reads on this connection see the state of the world - /// at the moment this command was executed. The server captures the timestamp - /// internally — no argument is accepted. + /// The server captures the timestamp itself — no argument is accepted — and + /// records a `wall_ms -> LSN` binding in the shard's temporal registry. + /// + /// This does NOT pin the connection to a snapshot view; nothing about + /// subsequent reads on this connection changes. The binding exists so a + /// LATER query can name that instant: `FT.SEARCH … AS_OF ` + /// resolves through this registry (see + /// [`VectorClient::search_opts`](crate::VectorClient::search_opts)'s + /// `as_of`). Take a checkpoint first, keep the timestamp, query against it + /// afterwards. pub async fn snapshot_at(&mut self) -> Result<()> { redis::cmd("TEMPORAL.SNAPSHOT_AT") .query_async::<()>(&mut self.conn) @@ -23,40 +30,36 @@ impl TemporalClient { Ok(()) } - /// Take a temporal snapshot at the explicitly-provided packed HLC value. - /// - /// Bi-temporal databases (e.g., Lunaris) need to pin AS_OF reads to a - /// historical timestamp, not "now". The packed HLC layout is - /// `(wall_ms as u128) << 32 | (counter as u128)`; Moon parses the - /// stringified value via BIGNUM. - /// - /// After this command, reads on this connection see the state of the world - /// at `packed_hlc`. Pair with [`release_snapshot`](Self::release_snapshot) - /// when done so the connection can be returned to live mode. - pub async fn snapshot_at_packed(&mut self, packed_hlc: u128) -> Result<()> { - redis::cmd("TEMPORAL.SNAPSHOT_AT") - .arg(packed_hlc.to_string()) - .query_async::<()>(&mut self.conn) - .await?; - Ok(()) - } + // `snapshot_at_packed` was removed in 0.3.0. + // + // It sent `TEMPORAL.SNAPSHOT_AT `, and the server's + // `validate_snapshot_at` rejects ANY argument — the command captures the + // timestamp itself. Every call answered `ERR wrong number of arguments`. + // The doc claimed "Moon parses the stringified value via BIGNUM"; nothing + // in the server ever did. + // + // This one is why the wire-form guard sends real arguments as well as bare + // names: the command name was right, so a name-only sweep saw nothing + // wrong. Pinning AS_OF to a historical timestamp has no server support at + // all — use [`snapshot_at`](Self::snapshot_at), which pins to now. - /// Release the current snapshot pin via `TEMPORAL.INVALIDATE` (no args). - /// - /// After a [`snapshot_at`](Self::snapshot_at) / - /// [`snapshot_at_packed`](Self::snapshot_at_packed) call, the connection - /// is pinned to that snapshot view. This call returns the connection to - /// live mode so subsequent reads see current data. - /// - /// Best-effort: callers typically discard errors here because the connection - /// may already be in an error state and the pin will eventually time out - /// server-side. - pub async fn release_snapshot(&mut self) -> Result<()> { - redis::cmd("TEMPORAL.INVALIDATE") - .query_async::<()>(&mut self.conn) - .await?; - Ok(()) - } + // `release_snapshot` was removed in 0.3.0. + // + // It sent a bare `TEMPORAL.INVALIDATE`, but that command is the 3-arg + // entity form below (`validate_invalidate` requires exactly + // ` `), so every call answered `ERR wrong + // number of arguments`. Found by the round-trip guard, not by review — + // this one had survived a name-level audit because `TEMPORAL.INVALIDATE` + // is a command Moon really does have. + // + // There is no replacement because there is nothing to release: the doc's + // premise was wrong. `TEMPORAL.SNAPSHOT_AT` never pinned the connection to + // a snapshot view — it records a shard-global `wall_ms -> LSN` binding + // that `AS_OF` resolves later. No pin is taken, so no pin can be dropped, + // and a connection is never in "snapshot mode" to return from. + // + // Callers that were relying on this to restore live reads can simply + // delete the call; their reads were already live. /// Invalidate (logically delete) a graph entity at the current wall-clock time. /// diff --git a/sdk/rust/src/vector.rs b/sdk/rust/src/vector.rs index ad2fba712..63eb78c6c 100644 --- a/sdk/rust/src/vector.rs +++ b/sdk/rust/src/vector.rs @@ -132,29 +132,18 @@ impl VectorClient { .await } - /// Upsert a vector record at `id` with `embedding_bytes` (LE-f32 encoded) and - /// `metadata_json` via `FT.UPSERT`. - /// - /// Lunaris-shaped helper: callers that already have an LE-f32 byte buffer - /// (because they encoded it themselves at the trait boundary) can use this - /// directly without re-decoding. Use [`encode_vector`] to produce the bytes - /// from a `&[f32]`. - pub async fn upsert( - &mut self, - index: &str, - id: &[u8], - embedding_bytes: &[u8], - metadata_json: &str, - ) -> Result<()> { - redis::cmd("FT.UPSERT") - .arg(index) - .arg(id) - .arg(embedding_bytes) - .arg(metadata_json) - .query_async::<()>(&mut self.conn) - .await?; - Ok(()) - } + // `upsert` was removed in 0.3.0. + // + // It sent `FT.UPSERT`, which Moon does not implement — the `FT.` dispatcher + // answered `ERR unknown FT.* command` on every call. It was not + // reimplemented over the real wire form because there isn't a faithful one: + // Moon indexes a vector by `HSET`-ing a hash whose vector FIELD NAME comes + // from the index definition, and this signature (`index, id, + // embedding_bytes, metadata_json`) never carried that field name. Guessing + // it would trade a loud error for a silent wrong write. + // + // Create the index with `FT.CREATE`, then `HSET` the key with the index's + // own vector field; auto-indexing picks it up. /// Lower-level `FT.SEARCH` invocation that exposes the full Lunaris-shaped /// query: a custom filter expression (built by the caller from its own diff --git a/sdk/rust/tests/round_trip.rs b/sdk/rust/tests/round_trip.rs new file mode 100644 index 000000000..bada0f980 --- /dev/null +++ b/sdk/rust/tests/round_trip.rs @@ -0,0 +1,682 @@ +//! ADD task `sdk-wire-form-fixes` — GUARD 2: every public helper round-trips. +//! +//! The main repo's `tests/sdk_wire_forms.rs` sweeps command NAMES. This suite +//! exists because that sweep is structurally blind to the other half of a wire +//! form: the ARGUMENTS. A helper can send a command the server knows and still +//! be dead on arrival. +//! +//! That is not hypothetical, and it is not rare — two of the five helpers +//! removed in 0.3.0 were wrong this way, and the SECOND was found by this +//! suite on its first live run, after a by-hand audit had already cleared the +//! file: +//! +//! - `snapshot_at_packed` sent `TEMPORAL.SNAPSHOT_AT `, and the +//! server's `validate_snapshot_at` rejects ANY argument. +//! - `release_snapshot` sent a bare `TEMPORAL.INVALIDATE`, which is the 3-arg +//! entity form. +//! +//! Both named a command Moon really has, so the name sweep saw nothing wrong; +//! both answered `ERR wrong number of arguments` on every call ever made. +//! +//! # Coverage +//! +//! EVERY `pub async fn` in `sdk/rust/src/**`. Not a sample — `swf4b` counts +//! the crate's public async surface and fails if this file drives fewer, so +//! a helper added later cannot quietly go unguarded. "The rest are ordinary +//! Redis commands, they're surely fine" is exactly the reasoning that let +//! `release_snapshot` through review. +//! +//! # What counts as failure +//! +//! A PROTOCOL-level error — unknown command, wrong arity, syntax error. Not an +//! empty result, not a Nil, not "graph not found", not "no password is set": +//! those are legitimate answers that depend on server state or configuration, +//! and asserting on them would make this suite a brittle mirror of the +//! server's data model rather than a guard on its call surface. +//! +//! Run against a live server: +//! ```bash +//! MOON_TEST_URL=redis://127.0.0.1:6399 cargo test --test round_trip -- --ignored +//! ``` + +use moondb::{ + DistanceMetric, EntityType, MoonClient, NeighborDirection, Reducer, VectorIndexOptions, +}; + +fn test_url() -> String { + std::env::var("MOON_TEST_URL").unwrap_or_else(|_| "redis://127.0.0.1:6399".into()) +} + +async fn connect() -> MoonClient { + MoonClient::connect(test_url()) + .await + .expect("failed to connect to Moon server") +} + +/// Turn a human-facing check label into the `Type::fn` key the coverage +/// assertion compares against. +/// +/// Labels are written for whoever reads a failure — `mq.create`, +/// `txn_begin/2`. Coverage is tracked per OWNING TYPE, not per bare name, +/// because five sub-clients declare a `search` and three declare a `create`: +/// keyed by name alone, a future `SomeNewClient::search` would count as +/// covered because `VectorClient::search` happens to be driven. That is the +/// same silent-hole failure this whole suite exists to prevent. +fn qualify(label: &str) -> String { + // Drop a trailing `/N` repeat marker. + let label = label.split('/').next().unwrap_or(label); + match label.split_once('.') { + Some((prefix, name)) => { + let ty = match prefix { + "mq" => "MqClient", + "graph" => "GraphClient", + "vector" => "VectorClient", + "text" => "TextClient", + "session" => "SessionClient", + "cache" => "CacheClient", + "temporal" => "TemporalClient", + "workspace" => "WorkspaceClient", + // An unrecognised prefix must not silently resolve to + // something plausible — let it fail the coverage diff loudly. + other => other, + }; + format!("{ty}::{name}") + } + None => format!("MoonClient::{label}"), + } +} + +/// Collects the helpers that came back with a protocol-level error. +#[derive(Default)] +struct Report { + failures: Vec, + checked: std::collections::BTreeSet, +} + +impl Report { + /// Record the outcome of one helper call. + /// + /// `Ok` passes. `Err` passes too UNLESS the message looks like the server + /// rejecting the call shape itself — see the module docs for why the bar is + /// there and not at "must succeed". + fn check(&mut self, helper: &str, r: Result) { + self.checked.insert(qualify(helper)); + if let Err(e) = r { + let msg = e.to_string().to_ascii_lowercase(); + // Matched as two loose tokens, NOT as the literal phrases + // "unknown command" / "unknown subcommand": Moon names the family + // in between (`ERR unknown MQ subcommand`, `ERR unknown FT.* + // command`), so phrase matching silently passes those. That was a + // real hole — the mutation check (`MQ CREATE ` reordered to + // `MQ CREATE`) survived this predicate until it was widened. + let unknown_verb = + msg.contains("unknown") && (msg.contains("command") || msg.contains("subcommand")); + let protocol_level = unknown_verb + || msg.contains("wrong number of arguments") + || msg.contains("syntax error"); + if protocol_level { + self.failures.push(format!(" {helper} -> {e}")); + } + } + } + + fn assert_clean(&self) { + assert!( + self.failures.is_empty(), + "{} of {} SDK helpers were rejected by the server at the protocol \ + level — the command name is fine, the ARGUMENTS are not:\n{}", + self.failures.len(), + self.checked.len(), + self.failures.join("\n") + ); + } +} + +/// Every public helper, called with plausible arguments, against a live server. +#[tokio::test] +#[ignore = "requires live server"] +async fn swf4_every_public_helper_round_trips() { + let names = drive_everything().await; + // Re-run the count assertion's data through the same path so a failure + // here names the helper, not just a total. + assert!(!names.is_empty()); +} + +/// Drives the whole surface and returns the set of helper names exercised. +async fn drive_everything() -> std::collections::BTreeSet { + let mut c = connect().await; + let mut r = Report::default(); + r.checked.insert(qualify("connect")); + + // ── connection / handshake ────────────────────────────────────────────── + r.check("ping", c.ping().await); + // Answers "no password is set" on an unauthenticated server — a + // configuration answer, not a wire-form rejection, so it passes. + r.check("auth", c.auth("swf4-not-a-real-password").await); + r.check("client_info", c.client_info().await); + + // ── strings ───────────────────────────────────────────────────────────── + r.check("set", c.set("swf4:k", "v").await); + r.check("get", c.get::<_, String>("swf4:k").await); + r.check("set_ex", c.set_ex("swf4:kex", "v", 100).await); + r.check("pset_ex", c.pset_ex("swf4:kpx", "v", 100_000).await); + r.check("set_nx", c.set_nx("swf4:knx", "v").await); + r.check("mset", c.mset(&[("swf4:m1", "a"), ("swf4:m2", "b")]).await); + r.check("mget", c.mget::<_, String>(&["swf4:m1", "swf4:m2"]).await); + r.check("getset", c.getset::<_, _, String>("swf4:k", "v2").await); + r.check("getdel", c.getdel::<_, String>("swf4:knx").await); + r.check("append", c.append("swf4:k", "x").await); + r.check("strlen", c.strlen("swf4:k").await); + + // ── counters ──────────────────────────────────────────────────────────── + r.check("incr", c.incr("swf4:n").await); + r.check("incr_by", c.incr_by("swf4:n", 2).await); + r.check("incr_by_float", c.incr_by_float("swf4:f", 1.5).await); + r.check("decr", c.decr("swf4:n").await); + r.check("decr_by", c.decr_by("swf4:n", 2).await); + + // ── key lifecycle ─────────────────────────────────────────────────────── + r.check("exists", c.exists("swf4:k").await); + r.check("key_type", c.key_type("swf4:k").await); + r.check("expire", c.expire("swf4:k", 100).await); + r.check("pexpire", c.pexpire("swf4:k", 100_000).await); + r.check("expire_at", c.expire_at("swf4:k", 4_102_444_800).await); + r.check("ttl", c.ttl("swf4:k").await); + r.check("pttl", c.pttl("swf4:k").await); + r.check("persist", c.persist("swf4:k").await); + r.check("rename", c.rename("swf4:m1", "swf4:m1b").await); + r.check("rename_nx", c.rename_nx("swf4:m1b", "swf4:m1c").await); + r.check("keys", c.keys::<_, String>("swf4:*").await); + r.check( + "scan_match", + c.scan_match::<_, String>("swf4:*", 10, 0).await, + ); + r.check("unlink", c.unlink("swf4:m2").await); + r.check("del", c.del("swf4:f").await); + + // ── hashes ────────────────────────────────────────────────────────────── + r.check("hset", c.hset("swf4:h", "f", "v").await); + r.check( + "hset_multiple", + c.hset_multiple("swf4:h", &[("f2", "v2"), ("f3", "v3")]) + .await, + ); + r.check("hget", c.hget::<_, _, String>("swf4:h", "f").await); + r.check( + "hmget", + c.hmget::<_, _, String>("swf4:h", &["f", "f2"]).await, + ); + r.check("hgetall", c.hgetall("swf4:h").await); + r.check("hexists", c.hexists("swf4:h", "f").await); + r.check("hlen", c.hlen("swf4:h").await); + r.check("hkeys", c.hkeys::<_, String>("swf4:h").await); + r.check("hvals", c.hvals::<_, String>("swf4:h").await); + r.check("hincrby", c.hincrby("swf4:h", "cnt", 1).await); + r.check("hincrbyfloat", c.hincrbyfloat("swf4:h", "fcnt", 1.5).await); + r.check("hsetnx", c.hsetnx("swf4:h", "f4", "v4").await); + r.check("hdel", c.hdel("swf4:h", "f3").await); + + // ── lists ─────────────────────────────────────────────────────────────── + r.check("lpush", c.lpush("swf4:l", "a").await); + r.check("rpush", c.rpush("swf4:l", "b").await); + r.check("llen", c.llen("swf4:l").await); + r.check("lrange", c.lrange::<_, String>("swf4:l", 0, -1).await); + r.check("lindex", c.lindex::<_, String>("swf4:l", 0).await); + r.check("lset", c.lset("swf4:l", 0, "z").await); + r.check("lpos", c.lpos("swf4:l", "z").await); + r.check("lrem", c.lrem("swf4:l", 1, "z").await); + r.check("ltrim", c.ltrim("swf4:l", 0, 10).await); + r.check("lpop", c.lpop::<_, String>("swf4:l", None).await); + r.check("rpop", c.rpop::<_, String>("swf4:l", Some(1)).await); + + // ── sets ──────────────────────────────────────────────────────────────── + r.check("sadd", c.sadd("swf4:s", "a").await); + r.check("sadd/2", c.sadd("swf4:s2", "a").await); + r.check("scard", c.scard("swf4:s").await); + r.check("sismember", c.sismember("swf4:s", "a").await); + r.check("smismember", c.smismember("swf4:s", &["a", "b"]).await); + r.check("smembers", c.smembers::<_, String>("swf4:s").await); + r.check("srandmember", c.srandmember::<_, String>("swf4:s", 1).await); + r.check( + "sinter", + c.sinter::<_, String>(&["swf4:s", "swf4:s2"]).await, + ); + r.check( + "sunion", + c.sunion::<_, String>(&["swf4:s", "swf4:s2"]).await, + ); + r.check("sdiff", c.sdiff::<_, String>(&["swf4:s", "swf4:s2"]).await); + r.check("spop", c.spop::<_, String>("swf4:s2").await); + r.check("srem", c.srem("swf4:s", "a").await); + + // ── sorted sets ───────────────────────────────────────────────────────── + r.check("zadd", c.zadd("swf4:z", 1.0, "m1").await); + r.check("zadd/2", c.zadd("swf4:z", 2.0, "m2").await); + r.check("zscore", c.zscore("swf4:z", "m1").await); + r.check("zcard", c.zcard("swf4:z").await); + r.check("zrank", c.zrank("swf4:z", "m1").await); + r.check("zrevrank", c.zrevrank("swf4:z", "m1").await); + r.check("zincrby", c.zincrby("swf4:z", 1.0, "m1").await); + r.check("zrange", c.zrange::<_, String>("swf4:z", 0, -1).await); + r.check("zrevrange", c.zrevrange::<_, String>("swf4:z", 0, -1).await); + r.check( + "zrangebyscore", + c.zrangebyscore::<_, _, _, String>("swf4:z", "-inf", "+inf") + .await, + ); + r.check("zcount", c.zcount("swf4:z", "-inf", "+inf").await); + r.check("zpopmin", c.zpopmin::<_, String>("swf4:z", 1).await); + r.check("zpopmax", c.zpopmax::<_, String>("swf4:z", 1).await); + r.check("zrem", c.zrem("swf4:z", "m1").await); + + // ── streams ───────────────────────────────────────────────────────────── + let xid = c.xadd("swf4:x", &[("f", "v")]).await; + let xid_str = xid.as_ref().map(|s| s.clone()).unwrap_or_default(); + r.check("xadd", xid); + r.check("xlen", c.xlen("swf4:x").await); + r.check("xrange", c.xrange("swf4:x", "-", "+").await); + r.check("xrevrange", c.xrevrange("swf4:x", "+", "-").await); + r.check("xtrim", c.xtrim("swf4:x", 10).await); + r.check( + "xdel", + c.xdel("swf4:x", if xid_str.is_empty() { "0-1" } else { &xid_str }) + .await, + ); + + // ── pub/sub, scripting, pipelines ─────────────────────────────────────── + r.check("publish", c.publish("swf4:chan", "hello").await); + let sha = c.script_load("return 1").await; + let sha_str = sha.as_ref().map(|s| s.clone()).unwrap_or_default(); + r.check("script_load", sha); + r.check("eval", c.eval::("return 1", &[], &[]).await); + if !sha_str.is_empty() { + r.check("evalsha", c.evalsha::(&sha_str, &[], &[]).await); + } else { + r.checked.insert(qualify("evalsha")); + } + let mut pipe = redis::pipe(); + pipe.cmd("PING"); + r.check("exec_pipeline", c.exec_pipeline(pipe).await); + + // ── transactions (the trio that a wrong measurement nearly deleted) ───── + r.check("txn_begin", c.txn_begin().await); + r.check("txn_commit", c.txn_commit().await); + r.check("txn_begin/2", c.txn_begin().await); + r.check("txn_abort", c.txn_abort().await); + + // ── admin / introspection ─────────────────────────────────────────────── + r.check("info", c.info(None).await); + r.check("dbsize", c.dbsize().await); + r.check("config_get", c.config_get("maxmemory").await); + r.check("config_set", c.config_set("maxmemory", "0").await); + r.check("slowlog_get", c.slowlog_get(Some(1)).await); + r.check("bgrewriteaof", c.bgrewriteaof().await); + r.check("bgsave", c.bgsave().await); + + // ── ACL ───────────────────────────────────────────────────────────────── + r.check("acl_whoami", c.acl_whoami().await); + r.check("acl_list", c.acl_list().await); + r.check( + "acl_setuser", + c.acl_setuser(&["swf4user", "on", ">pw", "~swf4:*", "+@read"]) + .await, + ); + r.check("acl_getuser", c.acl_getuser("swf4user").await); + r.check("acl_deluser", c.acl_deluser("swf4user").await); + // Both answer "not configured to use an ACL file" without an aclfile — + // a configuration answer, not a wire-form rejection. + r.check("acl_save", c.acl_save().await); + r.check("acl_load", c.acl_load().await); + + // ── message queue ─────────────────────────────────────────────────────── + { + let mut mq = c.mq(); + r.check("mq.create", mq.create("swf4:q", Some(3)).await); + let pushed = mq.push("swf4:q", b"body").await; + let pushed_id = pushed.as_ref().map(|s| s.clone()).unwrap_or_default(); + r.check("mq.push", pushed); + r.check("mq.pop", mq.pop("swf4:q", 1).await); + r.check( + "mq.ack", + mq.ack( + "swf4:q", + if pushed_id.is_empty() { + "0-1" + } else { + &pushed_id + }, + ) + .await, + ); + r.check("mq.dlq_len", mq.dlq_len("swf4:q").await); + r.check( + "mq.trigger", + mq.trigger("swf4:q", "swf4:cb", Some(100)).await, + ); + r.check("mq.publish_txn", mq.publish_txn("swf4:q", b"body").await); + } + + // ── graph ─────────────────────────────────────────────────────────────── + { + let mut g = c.graph(); + r.check("graph.create", g.create("swf4g").await); + r.check("graph.list", g.list().await); + r.check("graph.info", g.info("swf4g").await); + r.check( + "graph.add_node", + g.add_node("swf4g", "Person", &[("name", "alice")]).await, + ); + r.check( + "graph.add_edge", + g.add_edge("swf4g", 1, 2, "KNOWS", 1.0, &[("since", "2020")]) + .await, + ); + r.check("graph.query", g.query("swf4g", "RETURN 1").await); + r.check("graph.ro_query", g.ro_query("swf4g", "RETURN 1").await); + r.check("graph.query_raw", g.query_raw("swf4g", "RETURN 1").await); + r.check( + "graph.query_with_params", + g.query_with_params("swf4g", "RETURN $id", r#"{"id":1}"#) + .await, + ); + r.check("graph.explain", g.explain("swf4g", "RETURN 1").await); + r.check("graph.profile", g.profile("swf4g", "RETURN 1").await); + r.check("graph.query_at", g.query_at("swf4g", "RETURN 1", 1).await); + r.check( + "graph.neighbors", + g.neighbors("swf4g", 1, NeighborDirection::Out).await, + ); + r.check( + "graph.vsearch", + g.vsearch("swf4g", 1, 2, 1, &[0.1, 0.2, 0.3, 0.4]).await, + ); + r.check("graph.delete", g.delete("swf4g").await); + } + + // ── temporal ──────────────────────────────────────────────────────────── + { + let mut t = c.temporal(); + r.check("temporal.snapshot_at", t.snapshot_at().await); + // The 3-arg entity form. `release_snapshot` sent this SAME command with + // zero arguments and was rejected on every call — the defect this + // suite found on its first live run, and the reason the coverage bar + // is "every helper", not "every command name". + r.check( + "temporal.invalidate", + t.invalidate("1", EntityType::Node, "swf4g").await, + ); + } + + // ── vector ────────────────────────────────────────────────────────────── + { + let mut v = c.vector(); + let opts = VectorIndexOptions::new(4, DistanceMetric::Cosine); + r.check("vector.create_index", v.create_index("swf4idx", opts).await); + r.check("vector.list_indexes", v.list_indexes().await); + r.check("vector.index_info", v.index_info("swf4idx").await); + r.check("vector.compact", v.compact("swf4idx").await); + r.check( + "vector.config_set", + v.config_set("swf4idx", "EF_RUNTIME", "50").await, + ); + r.check( + "vector.config_get", + v.config_get("swf4idx", "EF_RUNTIME").await, + ); + r.check( + "vector.search", + v.search("swf4idx", &[0.1, 0.2, 0.3, 0.4], 1).await, + ); + r.check( + "vector.search_opts", + v.search_opts( + "swf4idx", + &[0.1, 0.2, 0.3, 0.4], + 1, + "vec", + Some(&["title"]), + None, + ) + .await, + ); + r.check( + "vector.search_raw", + v.search_raw("swf4idx", "*", b"\x00\x00\x00\x00", 1, false) + .await, + ); + r.check( + "vector.cache_search", + v.cache_search( + "swf4idx", + "swf4cache:", + &[0.1, 0.2, 0.3, 0.4], + 1, + "vec", + 0.9, + 1, + ) + .await, + ); + r.check( + "vector.recommend", + v.recommend( + "swf4idx", + &["swf4:doc1"], + Some(&["swf4:doc2"]), + 1, + Some("vec"), + ) + .await, + ); + r.check( + "vector.navigate", + v.navigate("swf4idx", &[0.1, 0.2, 0.3, 0.4], 1, "vec", 2, 0.5) + .await, + ); + r.check("vector.drop_index", v.drop_index("swf4idx", true).await); + } + + // ── text ──────────────────────────────────────────────────────────────── + { + let mut t = c.text(); + r.check("text.search", t.search("swf4txt", "hello", 10, None).await); + r.check( + "text.hybrid_search", + t.hybrid_search( + "swf4txt", + "hello", + &[0.1, 0.2, 0.3, 0.4], + "vec", + None, + 1, + [0.5, 0.3, 0.2], + None, + ) + .await, + ); + r.check( + "text.aggregate", + t.aggregate( + "swf4txt", + "*", + "@category", + &[Reducer::Count], + Some(("category", true)), + Some(10), + ) + .await, + ); + } + + // ── session / cache ───────────────────────────────────────────────────── + { + let mut s = c.session(); + r.check( + "session.search", + s.search("swf4idx", "swf4:sess", &[0.1, 0.2, 0.3, 0.4], 1, "vec") + .await, + ); + r.check("session.history", s.history("swf4:sess", 1).await); + r.check("session.expire", s.expire("swf4:sess", 100).await); + r.check("session.clear", s.clear("swf4:sess").await); + } + { + let mut ca = c.cache(); + r.check( + "cache.lookup", + ca.lookup( + "swf4idx", + "swf4cache:", + &[0.1, 0.2, 0.3, 0.4], + 1, + "vec", + 0.9, + 1, + ) + .await, + ); + r.check( + "cache.store", + ca.store( + "swf4cache:1", + &[0.1, 0.2, 0.3, 0.4], + "answer", + "vec", + Some(60), + ) + .await, + ); + r.check("cache.scan_keys", ca.scan_keys("swf4:*", 10).await); + r.check("cache.invalidate", ca.invalidate("swf4cache:1").await); + } + + // ── workspace ─────────────────────────────────────────────────────────── + { + let mut w = c.workspace(); + let created = w.create("swf4ws").await; + let ws_id = created.as_ref().map(|s| s.clone()).unwrap_or_default(); + let ws_ref = if ws_id.is_empty() { "swf4ws" } else { &ws_id }; + r.check("workspace.create", created); + r.check("workspace.list", w.list().await); + r.check("workspace.info", w.info(ws_ref).await); + r.check("workspace.auth", w.auth(ws_ref).await); + r.check("workspace.drop", w.drop(ws_ref).await); + } + + // ── stateful / destructive: kept last, and off the shared connection ──── + // `select` moves the db, `hello` renegotiates the protocol, and the flush + // pair empties the keyspace. Any of them mid-suite would change what the + // checks above are talking to, so they run on their own connection at the + // end. They are still CHECKED — an unguarded helper is the whole defect + // class this suite exists for. + { + let mut tail = connect().await; + r.check("select", tail.select(1).await); + r.check("hello", tail.hello(3).await); + r.check("flushdb", tail.flushdb().await); + r.check("flushall", tail.flushall().await); + } + { + let timeout = + MoonClient::connect_with_timeout(test_url(), std::time::Duration::from_secs(5)).await; + r.check("connect_with_timeout", timeout.map(|_| ())); + } + + r.assert_clean(); + r.checked +} + +/// The coverage bar itself: this suite must drive EVERY public async helper. +/// +/// Without this, `swf4` degrades silently — a helper added next quarter is +/// simply never called, and the suite still reports green over a shrinking +/// fraction of the surface. That is how the two removed temporal helpers +/// survived: nothing counted what was not being exercised. +#[tokio::test] +#[ignore = "requires live server"] +async fn swf4b_round_trip_covers_every_public_helper() { + let driven = drive_everything().await; + + let mut declared = std::collections::BTreeSet::new(); + let src_root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + collect_public_async_fns(&src_root, &mut declared); + + assert!( + !declared.is_empty(), + "found no `pub async fn` under {} — the scraper is broken, which would \ + make this assertion vacuous", + src_root.display() + ); + + let missing: Vec<&String> = declared.difference(&driven).collect(); + assert!( + missing.is_empty(), + "{} of {} public async helpers are never driven by the round trip:\n {}\n\n\ + Add a `r.check(\"\", …)` call for each. A helper this suite does \ + not call is a helper whose wire form nothing verifies — which is how \ + `snapshot_at_packed` and `release_snapshot` shipped broken.", + missing.len(), + declared.len(), + missing + .iter() + .map(|s| s.as_str()) + .collect::>() + .join("\n ") + ); +} + +/// Collect every `pub async fn` declared under `dir`, as `Type::fn`. +/// +/// Attribution is by the most recent enclosing `impl ` line. That is a +/// line-scanner, not a parser, which is adequate here because this crate +/// declares every helper inside a plain inherent `impl` at column 0 — and if +/// that ever stops being true, the assertion in `swf4b` fails loudly with an +/// unrecognised type rather than quietly dropping the helper. +fn collect_public_async_fns(dir: &std::path::Path, out: &mut std::collections::BTreeSet) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + collect_public_async_fns(&path, out); + continue; + } + if path.extension().and_then(|e| e.to_str()) != Some("rs") { + continue; + } + let Ok(src) = std::fs::read_to_string(&path) else { + continue; + }; + let mut current_type = String::new(); + for line in src.lines() { + let trimmed = line.trim_start(); + if let Some(rest) = trimmed.strip_prefix("impl ") { + // Skip generics on the impl itself (`impl<'a> Foo`), then take + // the type name up to a space, `<`, or `{`. + let rest = rest.strip_prefix('<').map_or(rest, |r| { + r.split_once('>') + .map(|(_, tail)| tail.trim_start()) + .unwrap_or(r) + }); + let ty: String = rest + .chars() + .take_while(|ch| ch.is_ascii_alphanumeric() || *ch == '_') + .collect(); + if !ty.is_empty() { + current_type = ty; + } + } + let Some(rest) = trimmed.strip_prefix("pub async fn ") else { + continue; + }; + let name: String = rest + .chars() + .take_while(|ch| ch.is_ascii_alphanumeric() || *ch == '_') + .collect(); + if !name.is_empty() && !current_type.is_empty() { + out.insert(format!("{current_type}::{name}")); + } + } + } +} diff --git a/src/command/metadata.rs b/src/command/metadata.rs index 2c7df18fa..0b566c1a0 100644 --- a/src/command/metadata.rs +++ b/src/command/metadata.rs @@ -418,6 +418,14 @@ pub static COMMAND_META: phf::Map<&'static str, CommandMeta> = phf_map! { "EXEC" => CommandMeta { name: "EXEC", arity: 1, flags: CommandFlags(CommandFlags::NOSCRIPT.0), first_key: 0, last_key: 0, step: 0, acl_categories: TXN }, "DISCARD" => CommandMeta { name: "DISCARD", arity: 1, flags: RF, first_key: 0, last_key: 0, step: 0, acl_categories: TXN }, "WATCH" => CommandMeta { name: "WATCH", arity: -2, flags: RF, first_key: 1, last_key: -1, step: 1, acl_categories: TXN }, + // Moon's cross-store transaction (`TXN BEGIN|COMMIT|ABORT`), served by the + // intercept in `command::transaction` that runs BEFORE this table — which + // is why it worked while `COMMAND INFO TXN` answered nothing and a bare + // `TXN` fell through to the registry gate and got `unknown command`. The + // entry fixes both without touching routing: the three real subcommands + // never reach the gate. WRITE like WS/MQ above, because COMMIT applies + // buffered writes and a replica must refuse it under readonly enforcement. + "TXN" => CommandMeta { name: "TXN", arity: -2, flags: W, first_key: 0, last_key: 0, step: 0, acl_categories: TXN }, "UNWATCH" => CommandMeta { name: "UNWATCH", arity: 1, flags: RF, first_key: 0, last_key: 0, step: 0, acl_categories: TXN }, // ---- Change Data Capture (CDC) ---- @@ -442,6 +450,13 @@ pub static COMMAND_META: phf::Map<&'static str, CommandMeta> = phf_map! { "FT.EXPAND" => CommandMeta { name: "FT.EXPAND", arity: -4, flags: R, first_key: 0, last_key: 0, step: 0, acl_categories: SRCH }, "FT.NAVIGATE" => CommandMeta { name: "FT.NAVIGATE", arity: -6, flags: R, first_key: 0, last_key: 0, step: 0, acl_categories: SRCH }, "FT.RECOMMEND" => CommandMeta { name: "FT.RECOMMEND", arity: -4, flags: R, first_key: 0, last_key: 0, step: 0, acl_categories: SRCH }, + // FT.AGGREGATE dispatches through the `FT.` intercept, which runs BEFORE + // this table is consulted — so it worked while being invisible to + // COMMAND INFO / COMMAND COUNT, and a driver that introspects before + // calling concluded it was unsupported. Adding the entry changes nothing + // about routing; it makes the command discoverable. Arity -3: the handler + // rejects `args.len() < 2` (index + query), and arity counts the name. + "FT.AGGREGATE" => CommandMeta { name: "FT.AGGREGATE", arity: -3, flags: R, first_key: 0, last_key: 0, step: 0, acl_categories: SRCH }, // ---- Workspace / message-queue commands (Wave B readonly-enforcement fix) ---- // WS and MQ are single-token commands dispatched as `WS ...` / `MQ diff --git a/src/command/mod.rs b/src/command/mod.rs index 8743f8e88..06dbbd51e 100644 --- a/src/command/mod.rs +++ b/src/command/mod.rs @@ -1029,6 +1029,16 @@ fn dispatch_inner( _ => {} } + // `TXN` reaches here only when none of the three subcommand intercepts in + // `command::transaction` claimed it — i.e. a bare `TXN` or an unrecognised + // subcommand. Falling through to `unknown command` was wrong twice over: + // the command plainly exists (`TXN BEGIN` answers `+OK`), and Redis answers + // a container command missing its subcommand with an arity error, which is + // what a driver's error handling keys on. Same shape as `MQ`/`WS`. + if cmd.eq_ignore_ascii_case(b"TXN") { + return DispatchResult::Response(transaction::err_txn_subcommand(args)); + } + DispatchResult::Response(err_unknown(cmd)) } diff --git a/src/command/transaction.rs b/src/command/transaction.rs index 0967d4340..6dd6f451e 100644 --- a/src/command/transaction.rs +++ b/src/command/transaction.rs @@ -122,6 +122,31 @@ pub fn is_txn_abort(cmd: &[u8], args: &[Frame]) -> bool { matches!(args.first(), Some(Frame::BulkString(sub)) if sub.eq_ignore_ascii_case(b"ABORT")) } +/// The error a `TXN` invocation earns when no subcommand intercept claimed it. +/// +/// `TXN` is served entirely by the three predicates above, which run before +/// dispatch. Anything reaching dispatch is therefore a bare `TXN` or an +/// unrecognised subcommand — previously answered `unknown command 'TXN'`, which +/// is false (the command exists) and misleads a driver into concluding Moon has +/// no cross-store transactions at all. +/// +/// Shapes follow Redis's container commands: a missing subcommand is an arity +/// error, an unrecognised one names the offending token. +pub fn err_txn_subcommand(args: &[Frame]) -> Frame { + let Some(sub) = args.first() else { + return Frame::Error(Bytes::from_static( + b"ERR wrong number of arguments for 'txn' command", + )); + }; + let name = match sub { + Frame::BulkString(s) | Frame::SimpleString(s) => String::from_utf8_lossy(s).into_owned(), + _ => String::new(), + }; + Frame::Error(Bytes::from(format!( + "ERR Unknown TXN subcommand or wrong number of arguments for '{name}'" + ))) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests/sdk_wire_forms.rs b/tests/sdk_wire_forms.rs new file mode 100644 index 000000000..2df515f2e --- /dev/null +++ b/tests/sdk_wire_forms.rs @@ -0,0 +1,435 @@ +//! ADD task `sdk-wire-form-fixes` — the SDK-to-server wire-form sweep. +//! +//! One rule: **every command name the first-party SDKs send is one this server +//! answers.** Three did not (`MQ.PUSH`, `MQ.POP`, `FT.UPSERT`), each returning +//! `ERR unknown command` on its first round trip, so no caller could ever have +//! depended on their behaviour — only on the code compiling. +//! +//! They shipped because **no CI workflow references `sdk/` at all**: the SDK +//! crate is not in the workspace, is not built, and is not tested (the SDK's own +//! `tests/integration.rs` is entirely `#[ignore]`d). This suite is the guard, +//! and it lives in the MAIN repo's test tree precisely so it runs on every PR +//! without a new job. +//! +//! Why it SENDS each command rather than consulting the registry: the two +//! disagree in BOTH directions. `FT.AGGREGATE` dispatches fine yet answers +//! `COMMAND INFO` with nothing, so a registry lookup would flag a working +//! command; and the `FT.` dispatcher swallows unknown `FT.*` names before the +//! top-level registry sees them, so a registry lookup would MISS `FT.UPSERT`. +//! Sending is the only oracle that is right both ways. +//! +//! An arity error is a PASS. It proves the name reached a handler, which is the +//! whole question; demanding success would mean calling all ~125 commands +//! correctly and would rot within a milestone. + +mod common; + +use std::io::{Read, Write}; +use std::net::TcpStream; +use std::process::{Child, Command}; +use std::time::{Duration, Instant}; + +// --------------------------------------------------------------------------- +// Scraping the SDK sources +// --------------------------------------------------------------------------- + +/// Every command-name literal the Rust SDK sends, with the file it came from. +fn rust_sdk_commands(root: &std::path::Path) -> Vec<(String, String)> { + let dir = root.join("sdk/rust/src"); + let mut out = Vec::new(); + for entry in std::fs::read_dir(&dir).expect("sdk/rust/src must exist") { + let path = entry.expect("dir entry").path(); + if path.extension().and_then(|e| e.to_str()) != Some("rs") { + continue; + } + let src = std::fs::read_to_string(&path).expect("read sdk source"); + let file = path + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or("?") + .to_string(); + for name in scan_literals(&src, "redis::cmd(\"") { + out.push((name, format!("sdk/rust/src/{file}"))); + } + } + assert!( + !out.is_empty(), + "scraped zero commands from sdk/rust/src — the scraper is broken, and a \ + scraper that finds nothing passes this suite vacuously" + ); + out +} + +/// Every command-name literal the Python SDK sends, with the file it came from. +fn python_sdk_commands(root: &std::path::Path) -> Vec<(String, String)> { + let mut out = Vec::new(); + let mut dirs = vec![root.join("sdk/python/moondb")]; + while let Some(dir) = dirs.pop() { + let Ok(rd) = std::fs::read_dir(&dir) else { + continue; + }; + for entry in rd { + let path = entry.expect("dir entry").path(); + if path.is_dir() { + if path.file_name().and_then(|f| f.to_str()) != Some("__pycache__") { + dirs.push(path); + } + continue; + } + if path.extension().and_then(|e| e.to_str()) != Some("py") { + continue; + } + let src = std::fs::read_to_string(&path).expect("read sdk source"); + let rel = path + .strip_prefix(root) + .unwrap_or(&path) + .to_string_lossy() + .into_owned(); + // Python builds arg lists (`["FT.SEARCH", index, …]`) AND calls + // `execute_command("FT.INFO", …)`; both are a quoted literal that + // looks like a command name, so scan for the shape rather than for + // one call form and silently miss the other. + for name in scan_literals(&src, "\"") { + if looks_like_a_command(&name) { + out.push((name, rel.clone())); + } + } + } + } + assert!( + !out.is_empty(), + "scraped zero commands from sdk/python/moondb — the scraper is broken" + ); + out +} + +/// Pull every `NAME"` occurrence where NAME is `A-Z0-9_.` only. +fn scan_literals(src: &str, prefix: &str) -> Vec { + let mut out = Vec::new(); + let mut rest = src; + while let Some(at) = rest.find(prefix) { + rest = &rest[at + prefix.len()..]; + let Some(end) = rest.find('"') else { break }; + let candidate = &rest[..end]; + if !candidate.is_empty() + && candidate + .bytes() + .all(|b| b.is_ascii_uppercase() || b.is_ascii_digit() || b == b'_' || b == b'.') + { + out.push(candidate.to_string()); + } + } + out +} + +/// A quoted uppercase literal only counts as a command if it plausibly names +/// one. +/// +/// Deliberately narrow for the Python side, where the scan cannot key on a call +/// form: an over-eager filter turns a constant like `"ASC"` into a spurious +/// failure, and this suite must go red only for a real dead wire form. The +/// dotted namespace (`FT.`, `GRAPH.`, `TEMPORAL.`) is what every command the +/// Python SDK sends by literal has in common. +/// +/// The leading-letter test is not decoration: without it `__version__ = +/// "0.1.0"` scrapes as a command and the sweep reports a version string as a +/// dead wire form. Caught on the first red run. +fn looks_like_a_command(name: &str) -> bool { + name.contains('.') && name.len() > 3 && name.starts_with(|c: char| c.is_ascii_uppercase()) +} + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn spawn_moon(dir: &std::path::Path) -> (Child, u16) { + common::spawn_listening(|port| { + Command::new(common::find_moon_binary()) + .args([ + "--port", + &port.to_string(), + "--dir", + &dir.to_string_lossy(), + "--shards", + "1", + "--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); + } +} + +struct Conn { + s: TcpStream, + buf: Vec, + pos: usize, +} + +impl Conn { + fn new(port: u16) -> Self { + let deadline = Instant::now() + Duration::from_secs(30); + loop { + if let Ok(s) = TcpStream::connect(format!("127.0.0.1:{port}")) { + s.set_read_timeout(Some(Duration::from_secs(10))).ok(); + s.set_write_timeout(Some(Duration::from_secs(10))).ok(); + let mut c = Conn { + s, + buf: Vec::with_capacity(8 * 1024), + pos: 0, + }; + if c.s.write_all(b"PING\r\n").is_ok() && c.line().contains("PONG") { + return c; + } + } + assert!( + Instant::now() < deadline, + "server on {port} never answered PING" + ); + std::thread::sleep(Duration::from_millis(50)); + } + } + + fn line(&mut self) -> String { + loop { + if let Some(rel) = self.buf[self.pos..].windows(2).position(|w| w == b"\r\n") { + let start = self.pos; + let end = start + rel; + let out = String::from_utf8_lossy(&self.buf[start..end]).into_owned(); + self.pos = end + 2; + return out; + } + let mut chunk = [0u8; 8 * 1024]; + let n = self.s.read(&mut chunk).expect("read"); + assert!(n > 0, "connection closed mid-frame"); + self.buf.extend_from_slice(&chunk[..n]); + } + } + + /// Send `parts` as a RESP array and return the FIRST line of the reply. + /// + /// Only the first line is needed: every "unknown command" answer is a + /// single-line error, so a reply that is not one is a pass regardless of + /// what follows. A multi-line reply leaves a tail in the buffer, which is + /// why every probe takes its own connection. + fn probe(&mut self, parts: &[&str]) -> String { + let mut req = format!("*{}\r\n", parts.len()); + for p in parts { + req.push_str(&format!("${}\r\n{p}\r\n", p.len())); + } + self.s.write_all(req.as_bytes()).expect("write probe"); + self.line() + } +} + +/// `true` when the server did not recognise the command NAME at all. +/// +/// Two distinct replies mean the same thing: the top-level registry gate +/// (`src/server/conn/shared.rs`) answers `unknown command` on a lookup miss, +/// and the `FT.` dispatcher answers `unknown FT.* command` for a name it does +/// not handle. Neither reaches a handler. +fn is_unknown(reply: &str) -> bool { + let r = reply.to_ascii_lowercase(); + r.starts_with("-err unknown command") || r.starts_with("-err unknown ft.") +} + +/// The optional server feature a command name belongs to, if any. +/// +/// `graph` and `text-index` are DEFAULT features, but CI's portability leg +/// builds `--no-default-features --features runtime-tokio,jemalloc` and so has +/// neither. `GRAPH.*` and `FT.AGGREGATE` are then genuinely absent, and +/// correctly so — a server that was not built with a feature is not missing a +/// command, and the SDK is not wrong to offer helpers for it. +/// +/// Returns `Some(feature_name)` for a gated command, `None` for one that every +/// build must answer. +fn gated_by(cmd: &str) -> Option<&'static str> { + if cmd.starts_with("GRAPH.") { + return Some("graph"); + } + // FT.AGGREGATE lives in `vector_search::ft_aggregate`, behind `text-index`. + // The rest of FT.* is the vector engine, which is unconditional. + if cmd == "FT.AGGREGATE" { + return Some("text-index"); + } + None +} + +/// Whether THIS test binary's server was built with `feature`. +/// +/// Integration tests compile against the crate's active feature set, so +/// `cfg!` here reports what the spawned server actually supports. +fn feature_enabled(feature: &str) -> bool { + match feature { + "graph" => cfg!(feature = "graph"), + "text-index" => cfg!(feature = "text-index"), + // An unknown gate must not silently excuse a command. + _ => true, + } +} + +// --------------------------------------------------------------------------- +// The sweep +// --------------------------------------------------------------------------- + +/// RED before the fixes: names `MQ.PUSH`, `MQ.POP`, `FT.UPSERT` (genuinely +/// dead) and `TXN` (dispatched by an intercept but missing from the registry, +/// so the bare name falls through to the gate). +#[test] +fn swf1_every_sdk_command_is_answered() { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + let mut all = rust_sdk_commands(root); + all.extend(python_sdk_commands(root)); + all.sort(); + all.dedup(); + + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path()); + let _guard = ServerGuard(child); + + let mut dead = Vec::new(); + let mut skipped = Vec::new(); + for (cmd, file) in &all { + if let Some(feature) = gated_by(cmd) + && !feature_enabled(feature) + { + skipped.push(format!("{cmd} (feature `{feature}` off)")); + continue; + } + let reply = Conn::new(port).probe(&[cmd]); + if is_unknown(&reply) { + dead.push(format!(" {cmd} (sent from {file}) -> {reply}")); + } + } + + // Announced, never silent: a sweep that quietly shrinks its own scope + // reads as "everything passed" when it means "less was checked". The + // default build skips nothing, so this list is empty in the leg that + // matters most. + if !skipped.is_empty() { + println!( + "swf1: {} of {} names skipped — server built without their feature: {}", + skipped.len(), + all.len(), + skipped.join(", ") + ); + } + + assert!( + dead.is_empty(), + "{} of {} SDK command names are not answered by this server:\n{}\n\n\ + Either the command does not exist — then REMOVE the helper that sends \ + it, do not add a server feature to satisfy an SDK method — or it is \ + dispatched by an intercept and missing from `src/command/metadata.rs`, \ + which also makes it invisible to COMMAND INFO. Add the registry entry.\n\n\ + (If the command belongs to an optional feature this build lacks, it \ + belongs in `gated_by` — but check first that it is genuinely gated, \ + rather than dead.)", + dead.len(), + all.len() - skipped.len(), + dead.join("\n") + ); +} + +/// Pins WHY the sweep sends rather than reads the registry. +/// +/// `FT.AGGREGATE` dispatches today but answers `COMMAND INFO` with nothing, so +/// a registry-backed sweep would have failed a working command. This test goes +/// red if someone "simplifies" the sweep that way. +#[test] +#[cfg_attr( + not(feature = "text-index"), + ignore = "FT.AGGREGATE is behind `text-index`, which this build lacks" +)] +fn swf2_a_dispatchable_command_is_not_flagged_by_the_sweep() { + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path()); + let _guard = ServerGuard(child); + + let reply = Conn::new(port).probe(&["FT.AGGREGATE"]); + assert!( + !is_unknown(&reply), + "FT.AGGREGATE must be answered — it is the case that separates \ + 'send the command' from 'read the registry': {reply}" + ); +} + +/// RED before the registry entries. A command a client cannot discover is, +/// for every driver that introspects before calling, a command that does not +/// exist. +#[test] +fn swf3_intercept_dispatched_commands_are_introspectable() { + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path()); + let _guard = ServerGuard(child); + + for cmd in ["FT.AGGREGATE", "TXN"] { + let mut c = Conn::new(port); + // `COMMAND INFO X` always answers a 1-element array, so the OUTER + // header is `*1` whether or not the command is known — checking only + // that line is a vacuous assertion, which is exactly what the first + // draft of this test did and why it passed against an unfixed server. + // The answer is in the element: a null for an unknown command, a + // 10-field array for a known one. + let outer = c.probe(&["COMMAND", "INFO", cmd]); + assert!( + outer.starts_with('*'), + "COMMAND INFO {cmd} did not answer an array: {outer}" + ); + let element = c.line(); + assert!( + !element.starts_with("$-1") && element != "_" && !element.starts_with("*-1"), + "COMMAND INFO {cmd} answered a null element ({element}) — {cmd} \ + dispatches, so a client that introspects before calling wrongly \ + concludes it is unsupported. Add it to src/command/metadata.rs." + ); + } +} + +/// Pins the risk named at the contract freeze: `TXN` is served by an intercept +/// in `src/command/transaction.rs` that runs BEFORE the registry gate, so +/// adding a registry entry must make it DISCOVERABLE without rerouting it. +/// +/// Before the entry, bare `TXN` answers `unknown command`; after, it must +/// answer a wrong-arity error — and the three real subcommands must be +/// untouched, because they never reach the gate. +#[test] +fn swf3b_registering_txn_does_not_reroute_it() { + let dir = tempfile::tempdir().expect("tempdir"); + let (child, port) = spawn_moon(dir.path()); + let _guard = ServerGuard(child); + + let begin = Conn::new(port).probe(&["TXN", "BEGIN"]); + assert!( + begin.starts_with("+OK"), + "TXN BEGIN must still be served by the intercept: {begin}" + ); + + let abort = Conn::new(port).probe(&["TXN", "ABORT"]); + assert!( + abort.starts_with("+OK") || abort.to_ascii_lowercase().contains("transaction"), + "TXN ABORT must still reach the intercept: {abort}" + ); + + let bare = Conn::new(port).probe(&["TXN"]); + assert!( + bare.to_ascii_lowercase() + .contains("wrong number of arguments"), + "a bare TXN must answer a wrong-arity error, not `unknown command` — \ + the command exists, it is the registry entry that is missing: {bare}" + ); +}