fix(tier-2): CodeRabbit follow-ups from PR #100#101
Merged
Conversation
Second pass on the CodeRabbit review surfaced four findings that the
previous fix wave did not address. This commit closes them all.
## Real bugs
- `src/cluster/command.rs::format_node_line`: `NodeFlags::Replica { master_id }`
was rendered as `"slave <master_id>"` inside the flags column AND the
master-id was emitted again in its own dedicated column. The result was a
malformed `CLUSTER NODES` / `CLUSTER REPLICAS` line where the master-id
appeared twice and every column right of "flags" shifted by one — breaking
whitespace-tokenized parsers (including redis-cli's own splitter). Flags
is now `"slave"` / `"myself,slave"` only; master-id stays in its own column.
- `src/command/key_extra.rs` `COPY ... DB <idx>`: the DB index token was
consumed by `i += 1` without parsing. The handler-level intercept normally
validates the DB index before reaching dispatch, but any path that
bypassed the intercept (or future caller wiring) would silently accept
garbage like `COPY src dst DB abc`. Now defensively `parse::<usize>()`s
the token and returns the canonical `ERR value is not an integer or out
of range` on malformed input.
## Test hardening
- `src/cluster/command.rs` test `cluster_replicas_lists_replicas`: was
loose (only checked >=9 fields, gathered ids). It would not have caught
the duplicate-master-id bug above. Now pins the exact replica-line
layout: 9 whitespace-separated columns, `fields[2] == "slave"` (not
"slave <master_id>"), `fields[3]` is exactly the 40-char master id, and
the master-id appears exactly once on the line. Any future regression
that re-embeds master-id in the flags column will flip the column count
from 9 → 10 and trip the assertion.
## Script hygiene
- `scripts/test-consistency.sh` SWAPDB block:
* Seed step previously used `both SELECT 0; both SET swapkey hello;
both SELECT 1; both DEL swapkey`. `redis-cli SELECT` does NOT persist
across separate invocations, so the trailing `DEL swapkey` ran against
db0 and removed the key we had just seeded. Now uses explicit
`redis-cli -n <db>` for each step.
* Out-of-range parity check captured `$redis_oor` but only asserted
`$rust_oor` contained `ERR`. A divergence (moon errors, Redis silently
OKs) would have passed. Now requires both sides to return `ERR` and
prints both messages on failure.
## Scoped out (intentionally not in this PR)
The remaining CodeRabbit comments need design work, not patches:
- `src/shard/coordinator.rs` is over the 1500-LOC cap. Splitting it out
(e.g. `coordinator::swapdb`) is mechanical refactor work that belongs
with the parallel `handler_{sharded,monoio}/mod.rs` split — they're all
over the cap and should land together.
- Snapshot-COW interaction with `SwapDb` / `MOVE` in `src/shard/spsc_handler.rs`
needs a design pass on snapshot reachability for two-DB commands, not a
quick guard.
## Verification
- `cargo build --release` PASS
- `cargo build --no-default-features --features runtime-tokio,jemalloc` PASS
- `cargo clippy --release -- -D warnings` PASS
- `cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings` PASS
- `cargo fmt --check` PASS
- `cargo test --release --lib` — 3256 PASS / 0 FAIL
- `cargo test --no-default-features --features runtime-tokio,jemalloc --lib` — 2649 PASS / 0 FAIL
- Functional: `COPY src dst DB abc` returns ERR; `COPY src dst DB 99999`
returns ERR (out-of-range intercept still fires); CLUSTER REPLICAS
layout verified by the strengthened unit test.
author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThree independent fixes improve command parsing, test determinism, and output formatting: COPY now rejects malformed DB arguments, SWAPDB tests use per-command database targeting for reproducibility, and CLUSTER NODES removes duplicate master-id embedding from replica flag fields. ChangesCommand handling and test improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ 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 |
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.
Summary
Closes the four remaining CodeRabbit findings from PR #100's review that the first fix wave didn't address.
Real bugs
src/cluster/command.rs::format_node_line—NodeFlags::Replica { master_id }was rendered as"slave <master_id>"inside the flags column AND the master-id was emitted again in its own dedicated column. The result was a malformedCLUSTER NODES/CLUSTER REPLICASline where the master-id appeared twice and every column right of "flags" shifted by one. Flags is now"slave"/"myself,slave"only.src/command/key_extra.rsCOPY ... DB <idx>— the DB index token was consumed by `i += 1` without parsing. Any path that bypasses the handler-level intercept would silently accept garbage like `COPY src dst DB abc`. Now defensively `parse::()`s and returns the canonical `ERR value is not an integer or out of range` on malformed input.Test hardening
Script hygiene
Scoped out (separate PR — design work, not a patch)
Verification
Test plan
Summary by CodeRabbit
Bug Fixes
Tests