feat(command): derive COMMAND/ROLE/RESET/HELLO from real server state - #471
Merged
Conversation
…e gate did not prove The code shipped in #470; this closes the task record. The first `gate PASS` was refused with `scope_violation` naming 7020 files, including `.github/workflows/*.yml` and `Cargo.lock` — paths this task never touched. Cause: the scope baseline is a whole-tree byte snapshot taken at the tests->build crossing, with no git awareness. Between that snapshot and the gate, five dependabot PRs merged into main (#420, #417, #346, #358, #359) and target/ was rebuilt, so every unrelated change read as "this task touched it". Rather than re-snapshot quietly, §6 now records: - the task's ACTUAL change set from `git show --name-only 151a185 1c12af0`, every path of which is inside the declared §5 Scope; - the two entries that needed naming instead of glossing — `conn/mod.rs` (one line, `pub mod watch;`, undeclared) and `.add/tooling/add.py` (25 lines of engine fix carried in from an earlier session, genuinely outside §5); - that the re-taken baseline is the post-merge tree, so the scope walk at this gate compares the tree against itself and proves nothing. The evidence that the build stayed in scope is the git file list, not the green gate. Also ticks the §6 verify checklist against evidence already gathered: the stubbed-counter A/B (reverting only next_birth_version turns 4 new tests red, so the green is earned), the 41->1 tokio failure delta traced to a diskfull $TMPDIR rather than to the change, and the 9/9 dispatched matrix including the Windows/macOS/console jobs no PR run executes. Adds .add/.gitignore, which `add.py init` scaffolds but this project never got. Without it the two 40MB scope-snapshot.json sidecars are commit candidates; they are regenerable working state, and the durable scope declaration is the state.json anchor. One 436K sidecar is already tracked from an older task and is left alone. Method delta for the next loop: as built, any task gated after unrelated merges land inherits an unfalsifiable scope violation. The anchor wants to be a git tree-ish, or the walk should skip gitignored paths and diff the merge-base. author: Tin Dang
Moon's client-identity surface answered from compile-time constants instead of from the server. The failures were invisible by eye because redis-cli renders `:0` and `*0` identically as "0", so every assertion here is on raw RESP bytes. COMMAND and COMMAND COUNT each returned the OTHER'S TYPE: bare COMMAND replied `:0` (an Integer where an Array belongs) and COMMAND COUNT replied `*0` (an Array where an Integer belongs). COMMAND INFO/DOCS/LIST/GETKEYS all replied an empty array. A driver that builds its command map at connect time does not read that as "unsupported" — it reads it as a protocol violation. All six now derive from the COMMAND_META phf registry (263 commands), so registering a command is what makes it introspectable and there is no second table to drift. ROLE and RESET were unknown commands. RESET was registered in the metadata table with full flags while dispatch rejected it — the same advertise-then- reject class as WATCH/UNWATCH before v0.8.6 — and a partial RESET existed only inside handler_sharded's subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere else. RESET's "default state" is taken from restore_migrated_state(None, ..), the same function ConnectionState::new uses, so it cannot drift from what a fresh connection means by default. HELLO contradicted INFO replication on the same connection: hello_acl built `mode`/`standalone` and `role`/`master` as literals, so a replica announced itself a master. Both fields now read ReplicationState/ClusterState. Redis deliberately uses three vocabularies for this one fact (HELLO -> replica, INFO -> slave, ROLE -> slave); that was measured against a live replica pair on redis-server 8.6.1, and it corrected this task's own frozen contract, which had said "slave" everywhere. The test was made stronger rather than matched to the code: ci12 asserts all three surfaces agree, each in its own vocabulary, plus a negative assertion that HELLO no longer claims master. CLIENT INFO / CLIENT LIST reported the literal `laddr=127.0.0.1:0` from inside the format string, so every client on every listener showed port 0. It now carries the real local address. Wired on all three connection handlers plus the inline fast path. ROLE and RESET cannot ride the shared dispatch table — their answers live on ConnectionContext/ReplicationState, not in the Database that dispatch() receives — so each handler needs its own intercept, and handler_single had neither. That handler is reachable only via listener::run_with_shutdown (an in-process tokio API used by a few test suites; main.rs and embedded.rs both route through run_sharded), which is precisely why it had drifted, and why an A/B caught the gap: reverting the fix there left ci12 green. ci14 drives that handler in-process and goes red with `-ERR unknown command 'ROLE'` without it. Tests: 14 raw-RESP scenarios in tests/client_identity_introspection.rs, with parity legs on the monoio and sharded handlers and the inline path. Startup is serialised across the suite's threads — 13 servers initialising data dirs at once left one unable to answer PING inside 30s at ~1 run in 8; a longer timeout would have been slower and still flaky, and a shared OnceLock server would have leaked a live moon past exit because statics are never dropped. 32/32 green at --test-threads=13 after the fix. Found and filed, not folded in: response batches are serialised at flush time using the FINAL protocol version, so a protocol-changing command retro-encodes earlier replies in the same batch (`HELLO 3` alone -> `%7`; `HELLO 3` + `HELLO 2` in one write -> `*14`). Pre-existing — that reproducer touches none of this code — but RESET, which reverts the protocol by contract, adds a second trigger. Filed as ADD task batch-protocol-version-fidelity with the measurement table. MONITOR was split out to monitor-command-feed at freeze: it is a stream rather than a reply, the only item touching the per-command hot path, and the only one exposing other clients' credentials. Gates: cargo fmt --check, clippy (default + tokio/jemalloc, --all-targets), cargo test --release, and the full tokio CI-parity suite all green. author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe change implements Redis-compatible ChangesClient identity and introspection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionHandler
participant IdentityCommands
participant ReplicationState
participant ClientRegistry
Client->>ConnectionHandler: Send HELLO or ROLE
ConnectionHandler->>ReplicationState: Read current replication state
ConnectionHandler->>IdentityCommands: Build identity response
IdentityCommands-->>ConnectionHandler: Return role and mode data
ConnectionHandler->>ClientRegistry: Store local address
ConnectionHandler-->>Client: Return RESP response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (7)
.add/tasks/batch-protocol-version-fidelity/TASK.md (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain the measurement inputs with the task evidence. Both records depend on probe sources that exist only in temporary session storage.
.add/tasks/batch-protocol-version-fidelity/TASK.md#L53-L54: commit/tmp/prepipe.shor include its exact raw-RESP writes..add/tasks/client-identity-introspection/TASK.md#L44-L45: commitidentity_probe.shand the raw-RESP helper, or embed their inputs and expected outputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.add/tasks/batch-protocol-version-fidelity/TASK.md around lines 53 - 54, Retain the measurement inputs used by both task records: in .add/tasks/batch-protocol-version-fidelity/TASK.md lines 53-54, commit /tmp/prepipe.sh or embed its exact raw-RESP writes; in .add/tasks/client-identity-introspection/TASK.md lines 44-45, commit identity_probe.sh and the raw-RESP helper or embed their inputs and expected outputs.src/client_registry.rs (1)
228-231: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider sharing the
laddrstring instead of copying it per connection.Every
ClientEntrynow owns a separateStringthat holds the same listener address for all connections on that listener.ConnectionContext::local_addr_stringbuilds a freshStringon each registration. At the c1M connection target this is one extra small heap allocation per connection for a value with very few distinct instances.Store
Arc<str>inClientEntryand cache one instance perConnectionContext.format_client_lineneeds no change becauseArc<str>formats the same way.♻️ Sketch
- pub laddr: String, + pub laddr: std::sync::Arc<str>,pub fn register( id: u64, addr: String, - laddr: String, + laddr: std::sync::Arc<str>, user: String,Cache the value once on
ConnectionContextand clone theArcat each registration site.Also applies to: 293-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client_registry.rs` around lines 228 - 231, Change ClientEntry::laddr from String to Arc<str> and cache the converted local address once in ConnectionContext instead of rebuilding it per registration. Update all ClientEntry construction sites, including the code around the additional registration paths, to clone the cached Arc; preserve format_client_line behavior and the existing address value.src/command/mod.rs (1)
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
connection::commandimplementation.Delete it and its obsolete
test_command_*tests. Keep theconnectionmodule because other handlers still use its functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/mod.rs` around lines 13 - 15, Remove the unreachable connection::command implementation and its obsolete test_command_* tests, while retaining the connection module and its functions used by other handlers. Update related module declarations or references as needed so the remaining command handlers continue compiling.src/command/metadata.rs (1)
355-358: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAdd
CommandFlags::NOSCRIPTtoROLE.COMMAND INFO ROLEis rendered fromCOMMAND_META; without this flag, the response omits Redis'snoscriptflag and clients can detect a metadata mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/metadata.rs` around lines 355 - 358, Add CommandFlags::NOSCRIPT to the flags for the ROLE entry in COMMAND_META, preserving its existing FAST, LOADING, and STALE flags so COMMAND INFO ROLE reports the complete Redis metadata.src/command/connection.rs (1)
641-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider two named parameters instead of a positional tuple.
role_mode.0is the role androle_mode.1is the mode. A caller that swaps the tuple elements compiles cleanly and reports the mode as the role. Two parameters (role: &'static str, mode: &'static str) do not remove that risk entirely, but a small named struct does, and the call sites already read the values fromhello_role_and_mode. This is cosmetic; the current code is correct and allocation-free.Also applies to: 786-791, 933-935, 1031-1036
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/connection.rs` around lines 641 - 645, Replace the positional role_mode tuple with a small named struct containing role and mode fields, then update the affected function signatures and call sites—including hello_role_and_mode usage—to access those named fields. Preserve the existing values and allocation-free behavior.src/command/identity.rs (1)
100-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd direct unit coverage for replica responses.
ROLEandRESETare already registered with ACL categories. Add unit tests forReplicationRole::Replicaand the"replica"branch ofhello_role_and_mode;ci9andci12are ignored on the tokio CI leg.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/identity.rs` around lines 100 - 124, Add direct unit tests in the existing tests module for ReplicationRole::Replica: verify role(None) returns the expected replica response, and verify hello_role_and_mode(None, ...) reports "replica" for the replica branch. Follow the existing standalone-master assertions and account for ci9/ci12 being ignored on the tokio CI leg.Source: Learnings
tests/client_identity_introspection.rs (1)
570-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an enabled test for replica identity derivation.
cargo nextest run --profile ciexcludes#[ignore]tests. No monoio CI job runsclient_identity_introspectionwith--ignored, soci9_role_replica_agrees_with_infoandci12_hello_role_matches_infodo not run. Other replica tests do not coverhello_role_and_mode, andci14only tests a master.Add a unit test that sets
ReplicationStatetoReplicationRole::Replicaand assertsrolereturnsslaveandhello_role_and_modereturnsreplica. Alternatively, run the ignored identity tests in a monoio CI step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/client_identity_introspection.rs` around lines 570 - 572, Add an enabled unit test alongside ci9_role_replica_agrees_with_info that initializes ReplicationState with ReplicationRole::Replica and verifies role returns “slave” and hello_role_and_mode returns “replica”. Ensure the test runs under the default cargo nextest ci profile rather than relying on ignored monoio-only tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.add/state.json:
- Around line 220-231: Synchronize durable scope records before retaining any
verified PASS: in .add/state.json lines 220-231, set
client-identity-introspection.scope.declared to the approved task scope; in
.add/state.json lines 301-308 and 319-331, represent watch-cas-transactions’
tooling-file exception and approved scope instead of relying on the
false-positive whole-tree scan; in
.add/tasks/client-identity-introspection/TASK.md lines 453-458, match the frozen
build scope to durable state; and in .add/tasks/watch-cas-transactions/TASK.md
lines 457-492, separate .add/tooling/add.py or document its explicitly approved
exception, without treating post-merge self-comparison as scope evidence.
In @.add/tasks/batch-protocol-version-fidelity/TASK.md:
- Line 107: Update the new contract fences to include the text language tag: use
```text for the protocol-shape block in
.add/tasks/batch-protocol-version-fidelity/TASK.md lines 107-107, the contract
block in .add/tasks/client-identity-introspection/TASK.md lines 299-299, and the
contract block in .add/tasks/monitor-command-feed/TASK.md lines 103-103.
In @.add/tasks/client-identity-introspection/TASK.md:
- Around line 282-284: Update the HELLO response expectation in the relevant
task scenarios from role "slave" to "replica", while keeping the ROLE and INFO
replication expectations as "slave". Apply this change to both referenced HELLO
checks only.
- Around line 25-26: Use the corrected registry count of 263 consistently:
update .add/tasks/client-identity-introspection/TASK.md lines 25-26 and 206-208,
and .add/tasks/monitor-command-feed/TASK.md lines 22-24; alternatively label the
271 value at the first site explicitly as a discarded measurement.
- Around line 157-158: Update the acceptance criteria and test plan in TASK.md
to explicitly include handler_single alongside handler_monoio, handler_sharded,
and the inline fast path. Ensure the handler-specific verification covers all
four paths, including the ci14 check for handler_single.
- Around line 382-410: Reconcile the test-plan scenario count with the
verification record: update the plan to include the missing ci14 scenario and
state 14 scenarios, or change the verification record to match the existing
ci1–ci13 list. Keep the counts and identifiers consistent across both sections.
In @.add/tasks/monitor-command-feed/TASK.md:
- Around line 3-8: Update the task metadata for monitor-command-feed to declare
risk: high and lower autonomy from auto to either manual or conservative before
advancing beyond the ground phase.
In @.add/tasks/watch-cas-transactions/TASK.md:
- Around line 390-395: The checklist’s “all tests pass” statement should
explicitly qualify that all non-ignored tests pass. Update the affected
checklist entry while preserving the existing explanation about ignored tests
and the pre-existing failure.
In `@scripts/test-consistency.sh`:
- Around line 696-702: Replace the assert_both check for COMMAND COUNT extra
with an assertion that independently verifies each reply is an ERR response,
without comparing full error text. Preserve the existing arity-error scenario
and leave the surrounding COMMAND and ROLE consistency checks unchanged.
In `@src/command/introspect.rs`:
- Around line 128-137: In the argument validation logic of the command
introspection handler, reject argv lengths exceeding the i16 arity domain before
converting argv.len() to i16. Return the existing invalid-argument-count error
for oversized inputs, and preserve the current exact/minimum arity checks for
representable lengths so downstream last-key calculations receive a valid n.
In `@src/server/conn/core.rs`:
- Around line 315-321: Update local_addr_string to bracket non-wildcard IPv6
bind addresses before appending config_port, producing forms such as [::1]:6379
while preserving existing wildcard and IPv4 formatting.
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1406-1426: Move the existing try_handle_reset block in the
connection command-processing flow before check_auth_gate, so unauthenticated
RESET succeeds and returns +RESET when requirepass is enabled. Keep its current
argument handling, state reset behavior, and immediate execution semantics
unchanged.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 764-776: Allow RESET through the pre-authentication command match
in both `src/server/conn/handler_sharded/mod.rs` lines 585-658 and
`src/server/conn/handler_single.rs` lines 532-595, routing it to the existing
`try_handle_reset` handling so authenticated and unauthenticated clients receive
`+RESET` rather than `NOAUTH`; no direct change is needed at the cited call
sites `src/server/conn/handler_sharded/mod.rs` lines 764-776 and
`src/server/conn/handler_single.rs` lines 1692-1704.
- Around line 1054-1063: The ROLE intercept must participate in MULTI queuing
rather than execute immediately. In
src/server/conn/handler_sharded/mod.rs:1054-1063 and
src/server/conn/handler_single.rs:1677-1686, move or restructure the ROLE
handling relative to each handler’s MULTI queue gate at 1303-1318 and 1740-1755
so open transactions return +QUEUED and ROLE executes during EXEC; preserve
RESET’s immediate behavior and follow the existing CONFIG/SLOWLOG convention.
In `@src/server/conn/shared.rs`:
- Around line 1054-1069: Update the client_registry::update closure in the RESET
restore path to publish the restored identity: clear e.name and assign e.user
from the restored default user, while preserving the existing connection state
and protocol updates. Ensure the registry user no longer retains the pre-RESET
identity; leave db handling unchanged because it is refreshed separately.
- Around line 1038-1041: Update try_handle_reset to handle conn.active_cross_txn
before clearing transaction state: either abort it through
abort_cross_store_txn_routed using the transaction’s stored db_index, or reject
RESET while conn.in_cross_txn() is active. Ensure RESET cannot leave kv_intents
pinned or cause disconnect teardown to target conn.selected_db instead of the
transaction database.
- Around line 1048-1052: Update PubSubTeardown::unsubscribe_all_for and the
RESET teardown flow around conn.subscription_count to return the removed channel
and pattern names, then call unpropagate_subscription for each removed
subscription, including subscriber-mode RESET paths. Ensure remote subscriber
maps are cleaned while preserving the existing local unsubscribe behavior and
subscriber count handling.
In `@tests/client_identity_introspection.rs`:
- Around line 317-320: Update startup_lock to use parking_lot::Mutex instead of
std::sync::Mutex, keeping the static LOCK declaration and Mutex::new
initialization. Remove the poisoning-specific unwrap_or_else handling and return
the guard directly from LOCK.lock().
---
Nitpick comments:
In @.add/tasks/batch-protocol-version-fidelity/TASK.md:
- Around line 53-54: Retain the measurement inputs used by both task records: in
.add/tasks/batch-protocol-version-fidelity/TASK.md lines 53-54, commit
/tmp/prepipe.sh or embed its exact raw-RESP writes; in
.add/tasks/client-identity-introspection/TASK.md lines 44-45, commit
identity_probe.sh and the raw-RESP helper or embed their inputs and expected
outputs.
In `@src/client_registry.rs`:
- Around line 228-231: Change ClientEntry::laddr from String to Arc<str> and
cache the converted local address once in ConnectionContext instead of
rebuilding it per registration. Update all ClientEntry construction sites,
including the code around the additional registration paths, to clone the cached
Arc; preserve format_client_line behavior and the existing address value.
In `@src/command/connection.rs`:
- Around line 641-645: Replace the positional role_mode tuple with a small named
struct containing role and mode fields, then update the affected function
signatures and call sites—including hello_role_and_mode usage—to access those
named fields. Preserve the existing values and allocation-free behavior.
In `@src/command/identity.rs`:
- Around line 100-124: Add direct unit tests in the existing tests module for
ReplicationRole::Replica: verify role(None) returns the expected replica
response, and verify hello_role_and_mode(None, ...) reports "replica" for the
replica branch. Follow the existing standalone-master assertions and account for
ci9/ci12 being ignored on the tokio CI leg.
In `@src/command/metadata.rs`:
- Around line 355-358: Add CommandFlags::NOSCRIPT to the flags for the ROLE
entry in COMMAND_META, preserving its existing FAST, LOADING, and STALE flags so
COMMAND INFO ROLE reports the complete Redis metadata.
In `@src/command/mod.rs`:
- Around line 13-15: Remove the unreachable connection::command implementation
and its obsolete test_command_* tests, while retaining the connection module and
its functions used by other handlers. Update related module declarations or
references as needed so the remaining command handlers continue compiling.
In `@tests/client_identity_introspection.rs`:
- Around line 570-572: Add an enabled unit test alongside
ci9_role_replica_agrees_with_info that initializes ReplicationState with
ReplicationRole::Replica and verifies role returns “slave” and
hello_role_and_mode returns “replica”. Ensure the test runs under the default
cargo nextest ci profile rather than relying on ignored monoio-only tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7958f89a-70b2-4df8-9cc4-68675db7730d
📒 Files selected for processing (22)
.add/.gitignore.add/state.json.add/tasks/batch-protocol-version-fidelity/TASK.md.add/tasks/client-identity-introspection/TASK.md.add/tasks/monitor-command-feed/TASK.md.add/tasks/watch-cas-transactions/TASK.mdCHANGELOG.mdscripts/test-commands.shscripts/test-consistency.shsrc/client_registry.rssrc/command/connection.rssrc/command/identity.rssrc/command/introspect.rssrc/command/metadata.rssrc/command/mod.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_single.rssrc/server/conn/shared.rstests/client_identity_introspection.rs
Comment on lines
+220
to
+231
| "flag_verified": true, | ||
| "tripwire": { | ||
| "contract_md5": "783b47c7b6e4897eaf7f049c3da5e3f2", | ||
| "tests": {} | ||
| }, | ||
| "scope": { | ||
| "declared": [ | ||
| "src/command/connection.rs", | ||
| "src/command/metadata.rs", | ||
| "src/command/mod.rs" | ||
| ], | ||
| "snapshot_md5": "d52700ba46d139e4de65276f8c64477d" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Synchronize the durable scope before recording a verified PASS. .add/.gitignore identifies .add/state.json as the durable scope anchor, but the state entries and frozen task records do not describe the same allowed paths. The watch record also admits that its replacement snapshot proves nothing and that a tooling file was out of scope.
.add/state.json#L220-L231: updateclient-identity-introspection.scope.declaredto the approved task scope..add/state.json#L301-L308: do not leavewatch-cas-transactionsmarked verified until its scope exception is represented..add/state.json#L319-L331: record the actual exception and the approved scope, not only the false-positive whole-tree scan..add/tasks/client-identity-introspection/TASK.md#L453-L458: keep the frozen build scope synchronized with the durable state..add/tasks/watch-cas-transactions/TASK.md#L457-L492: separate.add/tooling/add.pyor record an explicit approved exception; do not use the post-merge self-comparison as scope evidence.
📍 Affects 3 files
.add/state.json#L220-L231(this comment).add/state.json#L301-L308.add/state.json#L319-L331.add/tasks/client-identity-introspection/TASK.md#L453-L458.add/tasks/watch-cas-transactions/TASK.md#L457-L492
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/state.json around lines 220 - 231, Synchronize durable scope records
before retaining any verified PASS: in .add/state.json lines 220-231, set
client-identity-introspection.scope.declared to the approved task scope; in
.add/state.json lines 301-308 and 319-331, represent watch-cas-transactions’
tooling-file exception and approved scope instead of relying on the
false-positive whole-tree scan; in
.add/tasks/client-identity-introspection/TASK.md lines 453-458, match the frozen
build scope to durable state; and in .add/tasks/watch-cas-transactions/TASK.md
lines 457-492, separate .add/tooling/add.py or document its explicitly approved
exception, without treating post-merge self-comparison as scope evidence.
|
|
||
| ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to all new contract fences. The same MD040 warning appears in three task records.
.add/tasks/batch-protocol-version-fidelity/TASK.md#L107-L107: use```textfor the protocol-shape block..add/tasks/client-identity-introspection/TASK.md#L299-L299: use```textfor the contract block..add/tasks/monitor-command-feed/TASK.md#L103-L103: use```textfor the contract block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 3 files
.add/tasks/batch-protocol-version-fidelity/TASK.md#L107-L107(this comment).add/tasks/client-identity-introspection/TASK.md#L299-L299.add/tasks/monitor-command-feed/TASK.md#L103-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/batch-protocol-version-fidelity/TASK.md at line 107, Update the
new contract fences to include the text language tag: use ```text for the
protocol-shape block in .add/tasks/batch-protocol-version-fidelity/TASK.md lines
107-107, the contract block in .add/tasks/client-identity-introspection/TASK.md
lines 299-299, and the contract block in .add/tasks/monitor-command-feed/TASK.md
lines 103-103.
Source: Linters/SAST tools
Comment on lines
+25
to
+26
| - `src/command/metadata.rs:87` — `CommandMeta { name, arity, flags, first_key, last_key, step, | ||
| acl_categories }`; `COMMAND_META` (phf, 271 entries); `command_count()` at :702. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one corrected registry count across the task records. The client task records 263 as the corrected count, but stale 271 values remain in both task files.
.add/tasks/client-identity-introspection/TASK.md#L25-L26: replace 271 with 263 or label it as the discarded measurement..add/tasks/client-identity-introspection/TASK.md#L206-L208: update the assumption to 263..add/tasks/monitor-command-feed/TASK.md#L22-L24: update the inherited measurement to 263.
📍 Affects 2 files
.add/tasks/client-identity-introspection/TASK.md#L25-L26(this comment).add/tasks/client-identity-introspection/TASK.md#L206-L208.add/tasks/monitor-command-feed/TASK.md#L22-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/client-identity-introspection/TASK.md around lines 25 - 26, Use
the corrected registry count of 263 consistently: update
.add/tasks/client-identity-introspection/TASK.md lines 25-26 and 206-208, and
.add/tasks/monitor-command-feed/TASK.md lines 22-24; alternatively label the 271
value at the first site explicitly as a discarded measurement.
Comment on lines
+157
to
+158
| - Every one of the above behaves identically on `handler_monoio`, `handler_sharded`, and the | ||
| inline fast path. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Name every handler in the acceptance criterion.
The requirement lists handler_monoio, handler_sharded, and the inline path, but omits handler_single. Later verification identifies three handlers plus inline and adds ci14 for handler_single. Include handler_single in the contract and test plan so a handler-specific regression cannot pass the stated criteria.
Proposed contract wording
- - Every one of the above behaves identically on `handler_monoio`, `handler_sharded`, and the
- inline fast path.
+ - Every one of the above behaves identically on `handler_monoio`, `handler_sharded`,
+ `handler_single`, and the inline fast path.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Every one of the above behaves identically on `handler_monoio`, `handler_sharded`, and the | |
| inline fast path. | |
| - Every one of the above behaves identically on `handler_monoio`, `handler_sharded`, | |
| `handler_single`, and the inline fast path. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/client-identity-introspection/TASK.md around lines 157 - 158,
Update the acceptance criteria and test plan in TASK.md to explicitly include
handler_single alongside handler_monoio, handler_sharded, and the inline fast
path. Ensure the handler-specific verification covers all four paths, including
the ci14 check for handler_single.
Comment on lines
+282
to
+284
| Then the reply map's "role" field is "slave" | ||
| And it matches the role: line of INFO replication on the same connection | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the v2 HELLO vocabulary.
The amendment at Lines 343-357 says HELLO reports role: replica, while ROLE and INFO replication use slave. Lines 282-284 and 407-408 still require slave. Change only the HELLO expectation to replica.
Proposed wording
- Then the reply map's "role" field is "slave"
+ Then the reply map's "role" field is "replica"
- assert role field is "slave"
+ assert role field is "replica"Also applies to: 407-408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/client-identity-introspection/TASK.md around lines 282 - 284,
Update the HELLO response expectation in the relevant task scenarios from role
"slave" to "replica", while keeping the ROLE and INFO replication expectations
as "slave". Apply this change to both referenced HELLO checks only.
Comment on lines
+1038
to
+1041
| // Transaction | ||
| conn.in_multi = false; | ||
| conn.command_queue.clear(); | ||
| conn.watched_keys.clear(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every site that releases active_cross_txn and check RESET is among them.
set -euo pipefail
rg -n -C6 'active_cross_txn' src/server/conn
rg -n -C4 'abort_cross_store_txn_routed' srcRepository: pilotspace/moon
Length of output: 49394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RESET implementation and surrounding helper ---'
sed -n '980,1075p' src/server/conn/shared.rs
printf '%s\n' '--- RESET call sites and result handling ---'
rg -n -C12 'handle_reset|RESET|Reset' src/server/conn --glob '*.rs'
printf '%s\n' '--- connection actions and async dispatch paths ---'
rg -n -C8 'enum .*Action|ConnectionAction|process.*command|handle_command' src/server/conn --glob '*.rs' | head -n 500Repository: pilotspace/moon
Length of output: 42686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CrossStoreTxn fields and rollback database usage ---'
rg -n -C12 'struct CrossStoreTxn|impl CrossStoreTxn|selected_db|txn\.selected_db|active_cross_txn\.take' src/transaction src/server/conn --glob '*.rs' | head -n 700
printf '%s\n' '--- RESET tests and transaction tests ---'
rg -n -C10 'RESET|reset|cross.*txn|TXN\.BEGIN|TXN\.ABORT' src --glob '*test*.rs' --glob '*.rs' | head -n 700
printf '%s\n' '--- RESET placement in all handlers ---'
sed -n '1360,1440p' src/server/conn/handler_monoio/mod.rs
sed -n '730,785p' src/server/conn/handler_sharded/mod.rs
sed -n '1660,1720p' src/server/conn/handler_single.rsRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rollback and commit database selection ---'
sed -n '95,145p' src/transaction/abort.rs
sed -n '419,490p' src/transaction/abort.rs
rg -n -C10 'db_index|abort_cross_store_txn_routed|try_handle_reset' src/server/conn/handler_sharded/txn.rs src/server/conn/handler_monoio/txn.rs src/server/conn/handler_sharded/mod.rs src/server/conn/handler_monoio/mod.rs src/server/conn/handler_single.rs
printf '%s\n' '--- focused RESET and transaction tests ---'
rg -n -i -C8 'reset|cross.?store|txn\.begin|txn\.abort' src/server/conn/tests.rs tests --glob '*.rs' 2>/dev/null | head -n 500 || true
printf '%s\n' '--- RESET call ordering and all active transaction teardown sites ---'
sed -n '1380,1435p' src/server/conn/handler_monoio/mod.rs
sed -n '745,780p' src/server/conn/handler_sharded/mod.rs
sed -n '1680,1710p' src/server/conn/handler_single.rs
rg -n 'active_cross_txn\.(take|is_none|is_some)|active_cross_txn\s*=' src/server/conn --glob '*.rs'Repository: pilotspace/moon
Length of output: 50371
Abort the active cross-store transaction during RESET.
try_handle_reset clears in_multi and changes conn.selected_db, but it does not take conn.active_cross_txn. This leaves kv_intents pinned until a later transaction command or disconnect. On disconnect, teardown passes the reset conn.selected_db to abort_cross_store_txn_routed, so rollback can target the wrong database instead of the transaction’s stored db_index. Route an abort before completing RESET, or reject RESET while conn.in_cross_txn().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1038 - 1041, Update try_handle_reset
to handle conn.active_cross_txn before clearing transaction state: either abort
it through abort_cross_store_txn_routed using the transaction’s stored db_index,
or reject RESET while conn.in_cross_txn() is active. Ensure RESET cannot leave
kv_intents pinned or cause disconnect teardown to target conn.selected_db
instead of the transaction database.
Comment on lines
+1048
to
+1052
| // Pub/Sub — exit subscribe mode entirely. | ||
| if conn.subscription_count > 0 { | ||
| pubsub.unsubscribe_all_for(conn.subscriber_id); | ||
| } | ||
| conn.subscription_count = 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm every local unsubscribe path in the sharded handler is paired with unpropagate_subscription.
set -euo pipefail
rg -n -C8 'unpropagate_subscription' src
rg -n -C4 'fn unsubscribe_all\b|fn punsubscribe_all\b' src/pubsubRepository: pilotspace/moon
Length of output: 29166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RESET implementation and surrounding state ---'
rg -n -C18 'subscription_count|unsubscribe_all_for|RESET' src/server/conn/shared.rs
printf '%s\n' '--- pub/sub trait and implementations ---'
rg -n -C12 'trait .*PubSub|unsubscribe_all_for|unsubscribe_all|punsubscribe_all' src/pubsub src/server
printf '%s\n' '--- propagation and publish-count consumers ---'
rg -n -C12 'propagate_subscription|unpropagate_subscription|remote_sub|publish' src/server src/shard src/pubsubRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shared.rs matches ---'
rg -n 'RESET|unsubscribe_all_for' src/server/conn/shared.rs
printf '%s\n' '--- relevant shared.rs sections ---'
while IFS=: read -r file line rest; do
start=$((line > 35 ? line - 35 : 1))
end=$((line + 45))
sed -n "${start},${end}p" "$file"
done < <(rg -n 'RESET|unsubscribe_all_for' src/server/conn/shared.rs)
printf '%s\n' '--- exact unsubscribe_all_for definitions and calls ---'
rg -n -C10 'unsubscribe_all_for' srcRepository: pilotspace/moon
Length of output: 1442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shared.rs RESET helper and cleanup trait ---'
sed -n '950,1085p' src/server/conn/shared.rs
printf '%s\n' '--- all unsubscribe_all_for references ---'
rg -n -C14 'unsubscribe_all_for' src || true
printf '%s\n' '--- connection handler RESET call sites ---'
rg -n -C18 'try_handle_reset|handle_reset|eq_ignore_ascii_case\(b"RESET"\)' src/server/connRepository: pilotspace/moon
Length of output: 31757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- propagation map mutation ---'
sed -n '55,145p' src/server/conn/util.rs
printf '%s\n' '--- publish routing and remote count handling ---'
rg -n -C18 'try_handle_publish|PubSubPublishBatch|publish.*count|remote.*count|subscriber_map|RemoteSubscriberMap' src/server/conn src/shard src/pubsub
printf '%s\n' '--- RESET-specific tests ---'
rg -n -C12 'RESET|reset' tests src --glob '*test*' --glob '*.rs' | head -n 500Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- RemoteSubscriberMap implementation ---'
rg -n 'struct RemoteSubscriberMap|impl RemoteSubscriberMap|fn add|fn remove|fn target_shards' src/shard
sed -n '1,260p' src/shard/remote_subscriber_map.rs
printf '%s\n' '--- publish message execution ---'
rg -n -C20 'PubSubPublish\b|PubSubPublishBatch|publish_shared\(' src/shard src/server/conn
printf '%s\n' '--- all RESET branches in subscriber loops ---'
sed -n '205,255p' src/server/conn/handler_sharded/pubsub.rs
rg -n -C12 'eq_ignore_ascii_case\(b"RESET"\)|unsubscribe_all\(conn.subscriber_id\)|punsubscribe_all\(conn.subscriber_id\)' src/server/conn/handler_monoio src/server/conn/handler_shardedRepository: pilotspace/moon
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- subscriber-mode control flow around RESET ---'
sed -n '120,265p' src/server/conn/handler_sharded/pubsub.rs
rg -n -C12 'subscription_count|try_handle_reset' src/server/conn/handler_single.rs src/server/conn/handler_monoio/mod.rs src/server/conn/handler_sharded/mod.rs
printf '%s\n' '--- direct RESET paths for both sharded runtimes ---'
rg -n -C16 'eq_ignore_ascii_case\(b"RESET"\)' src/server/conn/handler_sharded src/server/conn/handler_monoioRepository: pilotspace/moon
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- single-handler RESET in subscriber mode ---'
rg -n -C10 'cmd.eq_ignore_ascii_case\(b"RESET"\)|RESET' src/server/conn/handler_single.rs | head -n 120
printf '%s\n' '--- monoio RESET in subscriber mode ---'
rg -n -C10 'cmd.eq_ignore_ascii_case\(b"RESET"\)|RESET' src/server/conn/handler_monoio/mod.rs | head -n 160
printf '%s\n' '--- behavioral verifier for stale-map fan-out ---'
python3 - <<'PY'
from dataclasses import dataclass, field
`@dataclass`
class RemoteMap:
channels: dict[str, set[int]] = field(default_factory=dict)
patterns: dict[str, set[int]] = field(default_factory=dict)
def add(self, channel, shard, pattern=False):
table = self.patterns if pattern else self.channels
table.setdefault(channel, set()).add(shard)
def remove(self, channel, shard, pattern=False):
table = self.patterns if pattern else self.channels
if channel in table:
table[channel].discard(shard)
if not table[channel]:
del table[channel]
def targets(self, channel):
out = set(self.channels.get(channel, ()))
out.update(
shard for pattern, shards in self.patterns.items()
if pattern.endswith("*") and channel.startswith(pattern[:-1])
for shard in shards
)
return out
# RESET removes the local subscriber but the current teardown trait does not
# remove the corresponding remote-map entry.
remote = RemoteMap()
remote.add("news", shard=1)
local_subscribers = {"news": 1}
local_subscribers.pop("news")
targets = remote.targets("news")
remote_count = 0 # the target shard's registry is empty after RESET
returned_count = 0 + remote_count
assert targets == {1}
assert returned_count == 0
print({
"stale_remote_targets": sorted(targets),
"remote_registry_count": remote_count,
"returned_publish_count": returned_count,
"unnecessary_cross_shard_dispatch": bool(targets),
})
PYRepository: pilotspace/moon
Length of output: 4266
Remove RESET subscriptions from remote subscriber maps.
PubSubTeardown::unsubscribe_all_for removes subscriptions only from the local registry and discards their names. After RESET, matching PUBLISH commands still dispatch unnecessary cross-shard messages to this empty registry. Return the removed channel and pattern lists and call unpropagate_subscription for each, including subscriber-mode RESET paths. The stale map does not inflate the returned subscriber count by itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1048 - 1052, Update
PubSubTeardown::unsubscribe_all_for and the RESET teardown flow around
conn.subscription_count to return the removed channel and pattern names, then
call unpropagate_subscription for each removed subscription, including
subscriber-mode RESET paths. Ensure remote subscriber maps are cleaned while
preserving the existing local unsubscribe behavior and subscriber count
handling.
Comment on lines
+1054
to
+1069
| // Identity + protocol, from the one definition of "default". | ||
| let (proto, db, authed, user, name) = | ||
| crate::server::conn::util::restore_migrated_state(None, requirepass); | ||
| conn.protocol_version = proto; | ||
| conn.selected_db = db; | ||
| conn.authenticated = authed; | ||
| conn.current_user = user; | ||
| conn.client_name = name; | ||
| // The wire codec must move with the connection, or the very next reply is | ||
| // serialized in a protocol the client is no longer speaking. | ||
| if let Some(codec) = codec { | ||
| codec.set_protocol_version(proto); | ||
| } | ||
| crate::client_registry::update(client_id, |e| { | ||
| e.name = None; | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Refresh the registry user and db, not only name.
restore_migrated_state resets conn.current_user to the default user and conn.selected_db to 0, but the client_registry::update closure clears only e.name.
The registry copy of user is what CLIENT LIST reports and what CLIENT KILL USER <name> matches on. ConnectionState::adopt_user is the only writer, and RESET does not call it. After an authenticated client runs RESET, CLIENT LIST still reports the pre-RESET username and CLIENT KILL USER <old-name> still matches the connection, even though the connection no longer holds that identity.
db self-heals on the sharded handler through the batch-end client_live.touch, but user does not.
🔧 Proposed fix to publish the restored identity
+ let restored_user = conn.current_user.clone();
crate::client_registry::update(client_id, |e| {
e.name = None;
+ e.user = restored_user;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Identity + protocol, from the one definition of "default". | |
| let (proto, db, authed, user, name) = | |
| crate::server::conn::util::restore_migrated_state(None, requirepass); | |
| conn.protocol_version = proto; | |
| conn.selected_db = db; | |
| conn.authenticated = authed; | |
| conn.current_user = user; | |
| conn.client_name = name; | |
| // The wire codec must move with the connection, or the very next reply is | |
| // serialized in a protocol the client is no longer speaking. | |
| if let Some(codec) = codec { | |
| codec.set_protocol_version(proto); | |
| } | |
| crate::client_registry::update(client_id, |e| { | |
| e.name = None; | |
| }); | |
| // Identity + protocol, from the one definition of "default". | |
| let (proto, db, authed, user, name) = | |
| crate::server::conn::util::restore_migrated_state(None, requirepass); | |
| conn.protocol_version = proto; | |
| conn.selected_db = db; | |
| conn.authenticated = authed; | |
| conn.current_user = user; | |
| conn.client_name = name; | |
| // The wire codec must move with the connection, or the very next reply is | |
| // serialized in a protocol the client is no longer speaking. | |
| if let Some(codec) = codec { | |
| codec.set_protocol_version(proto); | |
| } | |
| let restored_user = conn.current_user.clone(); | |
| crate::client_registry::update(client_id, |e| { | |
| e.name = None; | |
| e.user = restored_user; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1054 - 1069, Update the
client_registry::update closure in the RESET restore path to publish the
restored identity: clear e.name and assign e.user from the restored default
user, while preserving the existing connection state and protocol updates.
Ensure the registry user no longer retains the pre-RESET identity; leave db
handling unchanged because it is refreshed separately.
Comment on lines
+317
to
+320
| fn startup_lock() -> std::sync::MutexGuard<'static, ()> { | ||
| static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
| LOCK.lock().unwrap_or_else(|e| e.into_inner()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use parking_lot::Mutex for the startup lock.
Line 318 declares static LOCK: std::sync::Mutex<()>. The repository guideline requires parking_lot locks. parking_lot::Mutex::new is const, so the static works unchanged, and the poison handling at Line 319 disappears because parking_lot does not poison.
As per coding guidelines, "Use parking_lot::RwLock and parking_lot::Mutex rather than std::sync locks; never hold locks across await points; do not use poisoning-oriented unwrap calls."
♻️ Proposed change
-fn startup_lock() -> std::sync::MutexGuard<'static, ()> {
- static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
- LOCK.lock().unwrap_or_else(|e| e.into_inner())
+fn startup_lock() -> parking_lot::MutexGuard<'static, ()> {
+ static LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(());
+ LOCK.lock()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn startup_lock() -> std::sync::MutexGuard<'static, ()> { | |
| static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | |
| LOCK.lock().unwrap_or_else(|e| e.into_inner()) | |
| } | |
| fn startup_lock() -> parking_lot::MutexGuard<'static, ()> { | |
| static LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); | |
| LOCK.lock() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/client_identity_introspection.rs` around lines 317 - 320, Update
startup_lock to use parking_lot::Mutex instead of std::sync::Mutex, keeping the
static LOCK declaration and Mutex::new initialization. Remove the
poisoning-specific unwrap_or_else handling and return the guard directly from
LOCK.lock().
Source: Coding guidelines
…eding a live bug Two findings from the client-compat harness, which is the check that failed on PR #471 — both real, neither visible to the 14 raw-RESP tests in this task's own suite. 1. ROLE executed at QUEUE time inside MULTI. The first cut intercepted ROLE at the connection layer, ahead of the MULTI queueing step, because its answer lives on ConnectionContext.repl_state rather than in the Database that dispatch() receives. So `MULTI; ROLE; EXEC` replied the role array immediately and EXEC then returned `*0`. The damage is worse than a wrong reply: the command silently vanishes from the EXEC array, so every LATER result shifts down one index and a client reads another command's answer as this one's. Redis queues ROLE like any other command. Fixed by answering ROLE from the shared dispatch table instead, reading the process-global replication handle that INFO already uses and that every entry point registers (main.rs, listener.rs, embedded.rs). That is the only placement where a queued ROLE can work at all, since EXEC replays the queue through dispatch() — and it deleted all three per-handler intercepts, so ROLE now lives in exactly two places instead of five. Verified on the wire: `MULTI; ROLE; PING; EXEC` returns `*2` with the role array first and +PONG still last, and a live replica still reports `slave` through the global handle. ci15 pins the alignment; it fails under both prior states (`*0`, and `*1` with an unknown- command error). Nothing in this task's suite exercised a connection-layer command INSIDE a transaction, which is the coverage lesson: a new intercept has to be tested in MULTI as well as standalone, because its POSITION relative to queueing is the thing that can be wrong. 2. The harness's own divergence test needed a live Moon defect to pass. test_a_diverging_entry_exits_one_and_names_the_divergence borrowed a real bug as its fixture, so it FAILED whenever someone FIXED that bug — a test that punishes the fix. Three fixtures had already been burned this way (GET-inside-MULTI #457, SISMEMBER RESP3 #463, and COMMAND COUNT, retired by this very task, which is what turned the PR red). The durable fix was already designed and filed as #461, so it is implemented here rather than rotating to a fourth defect: a test-only `inject_moon_reply` hook fabricates the divergence, with a guard test asserting the shipped manifest never uses one. Proven load-bearing by disabling the hook, which turns the test red. Manifest: the identity_command_count and identity_role waivers are retired as fixed, and COMMAND INFO / COMMAND GETKEYS / RESET are added as live entries. COMMAND INFO carries a new, accurate waiver — its 10-field SHAPE now matches, but acl_categories is thin (@string where Redis says @READ @string @fast), key_specs is empty, and under RESP3 Redis types flags/acl_categories as Sets and key_specs entries as Maps where Moon emits Arrays. Owners recorded. Harness result against redis-server 8.6.1: PASS=181 FAIL=0 WAIVED=19 (was PASS=179 FAIL=8 before this commit). Gates: fmt, clippy (default + tokio/jemalloc, --all-targets), the identity suite (15 scenarios), and both compat suites (34 differ + 20 e2e) green. Note the unreachable-pattern warning caught during this work: (4, b'r') already existed in dispatch_inner for RPOP, so the first ROLE arm was dead code and EXEC still answered unknown command until it was merged into the existing arm. author: Tin Dang
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/client-compat/manifest.yaml`:
- Around line 267-269: Update the identity_reset manifest entry to exclude the
generic MULTI probe by restricting its policy to standalone and pipeline. Do not
alter the RESET command or add runner behavior; the existing identity_reset
definition should no longer run under MULTI.
In `@src/command/mod.rs`:
- Around line 1394-1414: Split the command dispatch implementations from
src/command/mod.rs into directory submodules, placing read and write dispatch
logic in separate modules while preserving their existing behavior and symbol
visibility. Keep mod.rs focused on module declarations and re-export the
existing public dispatch API so all current callers continue using the same
interfaces.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e48a1a0-9d1f-4e2f-9f7a-51480a9e3d73
📒 Files selected for processing (11)
.add/tasks/client-identity-introspection/TASK.mdCHANGELOG.mdscripts/client-compat/differ.pyscripts/client-compat/manifest.yamlscripts/client-compat/test_differ.pyscripts/client-compat/test_e2e.pysrc/command/mod.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_single.rstests/client_identity_introspection.rs
💤 Files with no reviewable changes (3)
- src/server/conn/handler_monoio/mod.rs
- src/server/conn/handler_single.rs
- src/server/conn/handler_sharded/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .add/tasks/client-identity-introspection/TASK.md
- CHANGELOG.md
Comment on lines
+267
to
+269
| - name: identity_reset | ||
| command: "RESET" | ||
| policy: exact |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude RESET from the generic MULTI probe.
RESET executes immediately and clears MULTI. The compatibility runner then discards the RESET reply and compares the following EXEC error. This entry does not test RESET in the MULTI context.
Restrict this entry to standalone and pipeline, or extend the runner with explicit immediate-command handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/client-compat/manifest.yaml` around lines 267 - 269, Update the
identity_reset manifest entry to exclude the generic MULTI probe by restricting
its policy to standalone and pipeline. Do not alter the RESET command or add
runner behavior; the existing identity_reset definition should no longer run
under MULTI.
Comment on lines
+1394
to
1414
| (4, b'r') => { | ||
| // ROLE — answered HERE rather than at the connection layer so a | ||
| // queued ROLE inside MULTI works: EXEC replays the queue through | ||
| // dispatch(), and a connection-layer intercept would have executed | ||
| // ROLE immediately at queue time, dropping it from the EXEC array | ||
| // and shifting every later result index for the client. | ||
| // | ||
| // The replication state comes from the process-global handle every | ||
| // entry point registers (main.rs, listener.rs, embedded.rs), which | ||
| // is the same source INFO reads. Unset (before replication init) | ||
| // yields the master form, matching a `None` ReplicationState. | ||
| if cmd.eq_ignore_ascii_case(b"ROLE") { | ||
| return resp(if args.is_empty() { | ||
| identity::role(crate::admin::metrics_setup::get_global_repl_state_arc()) | ||
| } else { | ||
| Frame::Error(Bytes::from_static( | ||
| b"ERR wrong number of arguments for 'role' command", | ||
| )) | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this command dispatch module.
src/command/mod.rs now reaches Line 1654. It exceeds the 1500-line limit. Move the read and write dispatch implementations into directory submodules, and re-export the existing public dispatch API from src/command/mod.rs.
As per coding guidelines, “No single Rust file should exceed 1500 lines” and “Command groups exceeding 1000 lines should split read and write implementations into directory modules, with mod.rs re-exporting the public API.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/mod.rs` around lines 1394 - 1414, Split the command dispatch
implementations from src/command/mod.rs into directory submodules, placing read
and write dispatch logic in separate modules while preserving their existing
behavior and symbol visibility. Keep mod.rs focused on module declarations and
re-export the existing public dispatch API so all current callers continue using
the same interfaces.
Source: Coding guidelines
This was referenced Aug 14, 2026
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.
What
Moon's client-identity surface answered from compile-time constants instead of from the server. Every assertion in the new suite is on raw RESP bytes, because
redis-clirenders:0and*0identically as "0" — which is how these survived.COMMAND— the two halves returned each other's typeCOMMAND(bare):0— an Integer where an Array belongsCOMMAND COUNT*0— an Array where an Integer belongs:263COMMAND INFO/DOCS/LIST/GETKEYSCOMMAND_METAA driver that builds its command map at connect time doesn't read
:0as "unsupported" — it reads it as a protocol violation. All six now derive from theCOMMAND_METAphf registry, so registering a command is what makes it introspectable; there is no second table to drift.ROLE/RESET— advertised, then rejectedRESETwas registered in the metadata table with full flags while dispatch rejected it — the same advertise-then-reject class asWATCH/UNWATCHbefore v0.8.6. A partialRESETexisted only insidehandler_sharded's subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere else.ROLEwas simply unknown.RESET's "default state" comes fromrestore_migrated_state(None, ..)— the same functionConnectionState::newuses — so it cannot drift from what a fresh connection means by default, and cannot restore more privilege than one has.HELLOcontradictedINFO replicationon the same connectionhello_aclbuiltmode/standaloneandrole/masteras literals, so a replica announced itself a master. Both now readReplicationState/ClusterState.Redis deliberately uses three vocabularies for this one fact, measured against a live replica pair on redis-server 8.6.1:
HELLOreplicaINFO replicationrole:slaveROLEslaveThis corrected the task's own frozen contract, which had said
slaveeverywhere. The test was made stronger, not matched to the code: ci12 now asserts all three surfaces agree, each in its own vocabulary, plus a negative assertion thatHELLOno longer claimsmaster.CLIENT INFO/CLIENT LISTladdr=127.0.0.1:0was a literal inside the format string, so every client on every listener reported port 0. Now the real local address.Wiring — all three handlers plus the inline path
ROLEandRESETcan't ride the shared dispatch table: their answers live onConnectionContext/ReplicationState, not in theDatabasethatdispatch()receives. So each handler needs its own intercept — andhandler_singlehad neither.That handler is reachable only via
listener::run_with_shutdown(in-process, tokio-only;main.rsandembedded.rsboth route throughrun_sharded), which is exactly why it had drifted. An A/B caught it: reverting the fix there left ci12 green.ci14drives that handler in-process and goes red with-ERR unknown command 'ROLE'without the fix.try_handle_resetis shared by all three rather than copied; the only thing abstracted is the lock the pub/sub registry sits behind (RwLockin the context,Mutexinhandler_single).Tests
14 raw-RESP scenarios in
tests/client_identity_introspection.rs, with parity legs on the monoio and sharded handlers and the inline fast path (where the #457 ACL bypass hid). Verified inline and array forms are byte-identical forROLE,COMMAND COUNT,RESET, and the arity error.Suite startup is serialised across threads: 13 servers initialising data dirs at once left one unable to answer PING inside 30s at ~1 run in 8. A longer timeout would have been slower and still flaky; a shared
OnceLockserver would have leaked a live moon past exit, since statics are never dropped. 32/32 green at--test-threads=13.Found and filed, not folded in
Response batches are serialised at flush time using the final protocol version, so a protocol-changing command retro-encodes earlier replies in the same batch:
HELLO 3replyHELLO 3alone%7✅HELLO 3+PING%7✅HELLO 3+HELLO 2*14❌HELLO 3+RESET*14❌Pre-existing — the
HELLO+HELLOreproducer touches none of this code — butRESET, which reverts the protocol by contract, adds a second trigger. Filed as ADD taskbatch-protocol-version-fidelitywith the full measurement table.MONITORwas split tomonitor-command-feedat freeze: it's a stream rather than a reply, the only item touching the per-command hot path, and the only one exposing other clients' credentials.Gates
cargo fmt --check· clippy default +runtime-tokio,jemalloc(--all-targets,-D warnings) ·cargo test --release· full tokio CI-parity suite — all green.Full
workflow_dispatchmatrix triggered on this branch, since Windows/macOS/console are skipped on normal PRs.Update — second commit (
31881d91)The Client compat check caught two things the 14 raw-RESP tests here did not.
ROLEexecuted at queue time insideMULTI(regression in the first commit)Intercepting
ROLEat the connection layer put it ahead of the MULTI queueing step, soMULTI; ROLE; EXECanswered immediately andEXECreturned*0. Worse than a wrong reply: the command vanishes from the EXEC array, so every later result shifts down one index and a client reads another command's answer as this one's.Fixed by answering
ROLEfrom the shared dispatch table, reading the process-global replication handleINFOalready uses. That is the only placement where a queuedROLEcan work —EXECreplays the queue throughdispatch()— and it deleted all three per-handler intercepts, so ROLE lives in two places instead of five.ci15pins the alignment and fails under both prior states (*0, and*1with an unknown-command error). The coverage lesson: nothing here exercised a connection-layer command inside a transaction, and an intercept's position relative to queueing is exactly what can be wrong.The harness's divergence test needed a live Moon bug to pass
test_a_diverging_entry_exits_one_and_names_the_divergenceborrowed a real defect as its fixture, so it failed whenever someone fixed that defect. Three fixtures had already been burned this way — GET-inside-MULTI (#457), SISMEMBER RESP3 (#463), andCOMMAND COUNT, retired by this very PR, which is what turned it red.Rather than rotate to a fourth defect, this implements the hook already designed in #461: a test-only
inject_moon_replythat fabricates the divergence, plus a guard test asserting the shipped manifest never uses one. Proven load-bearing — disabling the hook turns the test red.Manifest:
identity_command_countandidentity_rolewaivers retired as fixed;COMMAND INFO,COMMAND GETKEYS, andRESETadded as live entries.COMMAND INFOcarries a new accurate waiver (shape matches;acl_categoriesthin,key_specsempty, RESP3 Set/Map vs Array typing) with owners recorded.Full matrix
workflow_dispatchrun on this branch — Windows, macOS, and console all pass, and those three never run on a PR. Console Integration and Crash Matrix (Cross-Plane) green too.