feat(test): raw-RESP client-compatibility harness (Moon vs real redis-server) - #458
Merged
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TinDang97
added a commit
that referenced
this pull request
Aug 9, 2026
…ed MULTI GET on the inline read path (#457) Three defects, one root cause. The monoio inline fast path (`try_inline_dispatch`) answers the plain `*2 $3 GET <key>` shape straight from the shard map, never entering generic dispatch. Writes were gated on `can_inline_writes`; reads were gated on nothing. 1. ACL bypass (security). An authenticated but restricted user read any key with plain GET — the inline path runs neither the ACL command check nor the key-pattern check, and logs nothing. A `-@all` user read 160/160 keys at `--shards 1` and 24/160 at `--shards 4` (the shard-local fraction, so not a single-shard quirk). SET/DEL/MGET/HGET/TTL/EXISTS/KEYS/DBSIZE/INFO/FLUSHDB were all correctly denied — only GET leaked. 2. CLIENT TRACKING answered +OK and then never invalidated. The same path skips `tracking::invalidation::track_read_keys`, so a caching client's own GETs were never registered and nothing invalidated them — stale reads forever, no error surfaced. Reading the same key via MGET (not inline-eligible) received the invalidate; via GET it received nothing. Separately, `CLIENT TRACKING ON BCAST` with no PREFIX registered nothing at all, because the handlers only call `register_prefix` inside `for prefix in &prefixes`. 3. GET inside MULTI was executed instead of queued. `MULTI; GET k; EXEC` answered `+OK`, `$1 v`, `*0` where Redis answers `+OK`, `+QUEUED`, `*1[$1 v]` — the client receives a value where the protocol says `+QUEUED`, then an EXEC that silently omits the read. A redis-py/go-redis transaction returns an empty result set for an exchange it believes succeeded. Found by the new client-compat harness (PR #458) on its first run against a real redis-server. Fixes: `try_inline_dispatch{,_loop}` take `can_inline_reads`, and `can_inline_reads = acl_skip_allowed() && !conn.in_multi && !conn.tracking_state.enabled`. `acl_skip_allowed()` is hoisted and shared with the write gate, so the hot path pays nothing it did not already pay. Deliberately NOT gated on the process-global `tracking_active()`: only a connection's own reads populate its own invalidation set, so one caching client must not push every other connection off the fast path (writes keep the global gate — a non-tracking writer must invalidate everyone else). `parse_tracking_args` normalises prefix-less BCAST to the empty prefix, which `TrackingTable`'s `key.starts_with(prefix)` treats as all keys — one change covers all three handlers. The tokio handlers were never affected; they gate at `handler_single.rs` and `handler_sharded/mod.rs`. Every CI test job builds tokio, which is why none of this was visible in CI. Test-harness hardening: `spawn_moon` returned `Option<Moon>` and every caller did `let Some(m) = .. else { return }`, so a server that never became ready turned these suites into no-ops that still reported PASS. It now returns `Moon` and panics with diagnostics — child status plus moon's captured stderr, which was previously discarded to `Stdio::null()`. Applied to all three suites, because a guard that can quietly stop guarding is how the original defects survived two releases. Verification: - `tests/acl_inline_read_enforcement.rs` (new): deny-all and key-pattern users at `--shards 1` and `--shards 4`; red first, green after. - `tests/multi_queues_inline_get.rs` (new): queue-and-exec at both shard counts, plus controls pinning that MGET and SET still queue, that a plain GET outside a transaction still takes the fast path, and that a pipelined MULTI still queues the GET after it. That last one is documented as guarding the structural property (the inline loop bails at the first non-GET/SET command) rather than the gate itself — measured to pass with the gate removed. - `tests/client_tracking_invalidation.rs`: inline-GET tracking at `--shards 1` and BCAST with/without PREFIX; the pre-existing flaky `mset_invalidates_every_second_arg_key` is now deterministic. - `src/server/conn/tests.rs`: unit guard that a GET is refused when `can_inline_reads` is false, plus the new arg threaded through 16 call sites. - Wire diff vs real Redis 8.6.1: identical on all ACL and tracking probes at both shard counts. - Fast path preserved, measured via `moon_dispatch_path_total{path= "local_inline"}` rather than inferred from latency: 2000/2000 GETs still inlined for the unrestricted default user with AND without `--requirepass` and after a completed transaction; 0/2000 for restricted and tracking connections, which still return correct values via generic dispatch. - Linux A/B, one server alive at a time, alternating order, medians of 5: c=1 P=1 +4.95% (within noise), c=8 P=16 0.00%.
…0-9-client-compat The ADD tracker had drifted a full release train behind the repo: it still reported `v0-6-0-release` as the active milestone while the repo had shipped v0.6.0, v0.7.0, v0.8.0, and v0.8.5. `add.py check` was red on two records. Re-sync: - `shardslice-migration`: retire the RISK-ACCEPTED waiver that expired 2026-08-01. Its condition — the follow-up "cross-shard-read-acceleration (observe)" — was met in v2-performance / v2-2 xshard-read-validation (PRs #177/#178/#179): the C2 reply-side path recovers 38-49% of the cross-shard read penalty and the remainder is the ~10us irreducible hop. Gate raised RISK-ACCEPTED -> PASS, with the retired waiver kept verbatim in `waiver_retired` so the record is auditable rather than erased. Re-gated by direct state edit because `add.py gate` refuses with `tripwire_missing`. That guard is firing on a schema gap, not a finding: NO task in this project carries a tamper snapshot (all 17 predate the tripwire), so every task would trip it identically — `shardslice-migration` is simply the only one that needed its gate rewritten. Recorded as such in the note. - `fts-posting-rank-tf`: `depends_on: ["none"]` was a literal-string typo that `check` correctly read as an unresolvable dependency. Now `[]`. - `v3-3-vector-kv-polish`, `v3-4-kv-correctness`, `v3-5-write-path-durability`, `v0-6-0-release`: four empty shells, scaffolded but never populated, because delivery moved to PR-driven waves and never routed back through ADD. Marked `superseded` (not `done`) with a pointer to the PRs that actually shipped each scope. `add.py milestone-done` refuses a zero-task milestone — "nothing attached -> nothing proven" — and that refusal is right; claiming `done` would launder a gate that was never run. `superseded` is the honest record and only `done` is load-bearing anywhere in the engine. `add.py check`: 2 failed -> 89 passed, 0 failed, 0 warnings. New milestone `v0-9-client-compat` (production stage, now active), from the client/SDK deep review of v0.8.5. The two P0s found by that review ship separately as the v0.8.6 hotfix (PR #457); this milestone covers the remaining ~20 findings — the surface that stops an unmodified redis-py / go-redis / ioredis / monitoring agent from treating Moon as a drop-in. Eight tasks, breadth-first: `client-compat-harness` and `monoio-ci-coverage` have no dependencies and land first, because every other task cites the harness as its verifier and a verifier merged alongside the fix it verifies proves nothing. Then `client-identity-introspection`, `resp3-type-fidelity`, `pubsub-resp3-push`, `cluster-client-bootstrap`, `info-observability`, and `sdk-wire-form-fixes`. The shared decisions are what the review taught: real Redis is the oracle (never Moon's own expectation — the defects were found exactly where Moon tested Moon); RESP2 and RESP3 are both first-class; a command must not change shape by context (standalone vs MULTI vs pipeline); registered implies reachable; and behavior lands on all three dispatch paths, since a check present on two of three is the precise shape of the v0.8.6 P0. 11/11 exit criteria cite a verifier — the milestone is goal auto-ready. author: Tin Dang
… redis-server
Moon already had a Redis comparison suite. It could not see reply types.
`scripts/test-commands.sh` drives both servers through `redis-cli`, which renders
replies to human-readable text before any assertion runs — `assert_match_ttl`
even does `tr -d '(integer) '` — and `grep -c -- '-3 '` over its 2426 lines
returns 0, so the entire RESP3 surface was never compared at all. Every
assertion in `tests/redis_compat.rs` (968 lines) is Moon against a hand-written
expectation, with no `redis-server` in the loop. Between them, a wrong reply
TYPE was invisible. That is why ~22 type-level defects reached v0.8.5.
This adds a differ that speaks RESP on a raw socket, so the type byte survives
to the assertion:
- compares in a fixed order — TYPE, then SHAPE, then VALUE — and names which
one diverged, because "wrong reply type" and "right type, wrong value" are
different bugs that must not be reported as one
- runs the full {RESP2, RESP3} x {standalone, MULTI/EXEC, pipeline} matrix, so
a reply that changes shape by context is observable. This is not theoretical:
`apply_resp3_conversion` is called from 11 sites across 3 handlers rather
than one choke point, and the matrix immediately proved the EXEC path is one
of the misses.
- normalizes only what is declared, per entry: exact | sorted | type_only |
numeric_tolerance | ignore_value. No global fuzzy match — that would rebuild
the blindness being removed.
- compares errors on the code (first token) and never on message text
- refuses rather than skips: no redis-server is ERR_NO_ORACLE with exit 2. A
differential harness with nothing to differ against would report a green that
means nothing.
Findings on the first full run (oracle redis 8.6.1, moon 0.8.5+, 152
comparisons): 94 pass, 58 waived, 0 unexplained. Every waiver is a real,
reproduced divergence carrying a required reason and the task that owns its
fix, so CI is a ratchet — a NEW divergence fails the job, and `--strict` fails
the moment a waived one is fixed and its waiver goes stale.
Two of those findings are new, neither visible to any existing test:
1. GET inside MULTI is not queued. On the monoio inline fast path it executes
immediately: the client receives `$1 v` where Redis sends `+QUEUED`, and
EXEC then answers `*0` instead of `*1[$1 v]`. `MGET`, which is not
inline-eligible, queues correctly. Root cause is the same gate as the v0.8.6
P0 (PR #457): `can_inline_writes` carries `!conn.in_multi`,
`can_inline_reads` does not. Pre-existing on main and NOT fixed by #457.
`CONFIG GET` inside MULTI is a second, separate instance of the same class.
2. RESP3 conversion is not applied to EXEC inner replies. SMEMBERS is a Set
outside MULTI and a flat Array inside it; ZSCORE is a Double outside and a
Bulk inside. SISMEMBER passes inside MULTI only because two bugs cancel —
it is over-converted to Boolean outside, and unconverted inside.
Both are monoio-only. Every CI test job builds tokio, so neither could have
been caught by the existing matrix — the same structural blind spot that hid
the v0.8.6 ACL bypass.
`--info-manifest` additionally enumerates 34 INFO fields the standard
monitoring stack reads that Moon does not emit (run_id, tcp_port,
uptime_in_seconds, keyspace_hits/misses, evicted_keys, maxmemory*,
instantaneous_ops_per_sec, ...) — a monitoring agent loses those series
silently, which is worse than an error because the dashboard simply goes blank.
Tests are stdlib `unittest`, not pytest: pytest is absent from the moon-dev VM
that runs the self-hosted job, and a PR-gating job must not depend on a package
hand-installed into a runner that gets rebuilt. 33 unit tests (codec,
comparator, all five policies, every reject code) + 19 end-to-end tests against
real servers, all red before the implementation existed.
New CI job `client-compat` builds moon with the DEFAULT (monoio) runtime — the
one clients actually hit — then runs the unit suite, the e2e suite, the strict
diff, and the INFO coverage report, uploading the machine-readable record.
Also scaffolds ADD milestone v0-9-client-compat: this task plus the seven that
consume its output.
author: Tin Dang
`--strict` named them itself — `ERR_STALE_WAIVER: waivers no longer reproduce: multi_get_must_queue, error_wrongtype` — which is the stale-waiver mechanism doing exactly what it was built for: a waived divergence cannot silently become a permanent blind spot once it is fixed. Both entries stay in the manifest, now unwaived, as the permanent regression guard for the GET-inside-MULTI queueing bug this harness found. Strict run after the change: 152 comparisons, 98 pass, 0 fail, 54 waived. author: Tin Dang
…n build Adversarial read of the harness before recording its verify gate. Both findings were places where the harness could report a green, or a finding, that it had not actually earned. 1. The byte-identical-send invariant was vacuous in the MULTI context. `sent` was RECONSTRUCTED from the same argv for both servers, so `assert r_sent == m_sent` compared two identically-built values and could never fail. `RespConn` now logs what it actually writes, and `_exchange` returns that observation — the invariant is checked against the socket, not against a rebuild of its inputs. Standalone and pipeline were already accurate; they now go through the same path. 2. `--info-manifest` blamed Moon for a wrong pin. It checked only whether MOON emitted a pinned field, so a field that real Redis does not emit either was reported as a Moon defect. The differential has to cut both ways: a field absent from the oracle is a bad pin, and manufacturing a finding is the mirror image of the blindness this harness exists to remove. Absence from the oracle is now reported as "fix the pin, not moon" and never as a divergence. With this in place the pinned list validates itself: all 33 findings are fields Redis 8.6.1 emits and Moon does not — 0 bad pins. Two e2e tests failed after the change, both because they encoded assumptions the fixes invalidated rather than because anything regressed: - `test_a_diverging_entry_exits_one_and_names_the_divergence` asserted on GET-inside-MULTI, which this branch fixes, so it no longer diverges. Moved to SISMEMBER under RESP3 (Integer vs over-converted Boolean), with a note that a test needing a live defect to pass is maintenance debt. - `test_info_manifest_reports_missing_fields_by_name` pinned a field no Redis has, which is now correctly a bad pin. Moved to run_id (real Redis emits it, Moon does not), and a new test pins the bad-pin path itself. Validation: 33 unit + 20 e2e green; strict run 152 comparisons, 98 pass, 0 fail, 54 waived; no leaked servers. author: Tin Dang
…eltas Gate: PASS. The refute-read was done in-session rather than delegated and it found two real weaknesses, both fixed before this gate was recorded — a vacuous byte-identity assert and an INFO check that blamed Moon for a wrong pin. Build expectations are confirmed against evidence that was seen, not against test names: a type difference that renders identically as text is caught (SISMEMBER `:1` vs `#t`); the same command is compared in all three contexts, so a context-dependent shape is observable (SMEMBERS is a Set standalone and an Array inside EXEC); normalization is proven declared-not-global by a PAIR of tests, since a fuzzy comparator would satisfy the permissive one alone. Two honesty notes are recorded rather than smoothed over: - Build expectations were written AT the gate, not before the build as the phase requires. Nothing was falsified by it, but writing them afterwards cannot distinguish "the build was right" from "I described what the build did", and the ordering is what makes the section worth having. Filed as an ADD delta. - Three e2e tests were edited after first passing red-green. None loosened an assertion — each moved off an assumption the evidence falsified, two of them because this branch's own fixes changed the behaviour being asserted. Filed as a TDD delta, because "the test needs a live defect to keep passing" is a debt pattern worth naming. Nine spec deltas seeded for the milestone, each with evidence: CONFIG GET also bypasses MULTI queueing, RESP3 conversion is skipped for EXEC inner replies, MULTI has no EXECABORT, COMMAND COUNT returns an empty Array, ROLE is unknown, 33 INFO fields are missing, docs/redis-compat.md should be generated from the record, and the matrix needs a shard axis (the v0.8.6 defects all behaved differently at --shards 4). author: Tin Dang
TinDang97
force-pushed
the
feat/client-compat-harness
branch
from
August 9, 2026 09:32
7dfa311 to
6c7ac6e
Compare
TinDang97
changed the base branch from
fix/client-compat-p0-acl-tracking
to
main
August 9, 2026 09:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Moon already had a Redis comparison suite. It could not see reply types.
scripts/test-commands.shdrives both servers throughredis-cli, which renders replies to human-readable text before any assertion runs —assert_match_ttlliterally doestr -d '(integer) '— andgrep -c -- '-3 'over its 2426 lines returns 0, so the entire RESP3 surface was never compared. Every assertion intests/redis_compat.rs(968 lines) is Moon against a hand-written expectation with noredis-serverin the loop. Between them, a wrong reply type was structurally invisible. That is why ~22 type-level defects reached v0.8.5.What
A differ that speaks RESP on a raw socket, so the type byte survives to the assertion.
apply_resp3_conversionis called from 11 sites across 3 handlers rather than one choke point, and the matrix immediately proved the EXEC path is one of the misses.exact/sorted/type_only/numeric_tolerance/ignore_value. No global fuzzy match; that would rebuild the blindness being removed.redis-serverisERR_NO_ORACLE, exit 2. A differential harness with nothing to differ against would report a green that means nothing.Findings
First full run — oracle Redis 8.6.1, 152 comparisons: 98 pass, 54 waived, 0 unexplained. Every waiver carries a required reason and names the task that owns its fix, so CI is a ratchet: a new divergence fails the job, and
--strictfails the moment a waived one is fixed and its waiver goes stale.Two findings were new, neither visible to any existing test:
GETinsideMULTIwas executed, not queued.MULTI; GET k; EXEC→+OK,$1 v,*0where Redis sends+OK,+QUEUED,*1[$1 v]. Fixed in fix(security): ACL bypass + silent CLIENT TRACKING failure on the inline GET path #457 as a direct result.CONFIG GETinMULTIis a separate, still-open instance of the same class.EXECinner replies.SMEMBERSis a Set outsideMULTIand a flat Array inside;ZSCOREis Double outside, Bulk inside.SISMEMBERpasses insideMULTIonly because two bugs cancel.--info-manifestadditionally enumerates 33INFOfields the standard monitoring stack reads that Moon does not emit (run_id,tcp_port,uptime_in_seconds,keyspace_hits/misses,evicted_keys,maxmemory*, …). A monitoring agent loses those series silently — worse than an error, because the dashboard just goes blank.The stale-waiver mechanism then proved itself unprompted: once #457's fix landed,
--strictfailed on its own withERR_STALE_WAIVER: waivers no longer reproduce: multi_get_must_queue, error_wrongtype. Both are now unwaived permanent guards.Verification
sentwas reconstructed from the same argv for both servers, so it could never fail — now recorded from the socket), and--info-manifestblamed Moon for a wrong pin (it checked only Moon; a field the oracle also lacks is now "fix the pin, not moon"). With that in place the pin list validates itself: 33 findings, 0 bad pins.finallycovering every refusal path.Notes for review
unittest, not pytest — pytest is absent from the moon-dev VM that runs the self-hosted job, and a PR-gating job must not depend on a package hand-installed into a runner that gets rebuilt.client-compatjob builds moon with the default (monoio) runtime — the one clients actually hit. Every existing CI test job builds tokio, which is why all three defects found this week were invisible.v0-9-client-compat(this task plus the seven that consume its output) and this task's verify record, which files nine evidence-backed spec deltas plus two honesty deltas (build expectations were written at the gate rather than before the build; three e2e tests were edited after red-green because the branch's own fixes changed what they asserted).