feat: multi-shard improvements Tier 1 — operator UX#99
Merged
Conversation
Previously SWAPDB returned "ERR SWAPDB is not supported in sharded mode", which was misleading — the command is not implemented at all, even on a single-shard server. COPY with the DB option had a similar vague message. MOVE (the db-move command) had no dispatch entry or metadata entry at all, causing it to fall through to an unhelpful "unknown command" path. Changes: - SWAPDB now returns: "ERR SWAPDB not implemented (tracked in T2.1)" - MOVE stub added to dispatch table and metadata registry, returns: "ERR MOVE not implemented (tracked in T2.2)" - COPY...DB n now returns: "ERR COPY DB option not implemented (tracked in T2.3)" The tracking references point operators to the T2.x tasks where each command will be fully implemented. Also fixes a pre-existing compile error on this branch: `maybe_force_checkpoint_on_wal_overflow` called `force_checkpoint` with 7 arguments but the function signature requires 9 (tombstone_retain_epochs and tombstone_retain_secs were added in a prior commit but this call site was not updated). The WAL-overflow path does not perform tombstone GC — that is the scheduled persistence tick's responsibility — so 0/0 is the correct value at this call site. Unit tests added: - `swapdb_error_is_not_implemented` (src/command/mod.rs) - `move_error_is_not_implemented` (src/command/mod.rs) - `test_copy_db_option_not_implemented` (src/command/key_extra.rs) author: Tin Dang
…rds)
Documented footgun: at c=50 with 8 shards the per-shard SPSC channels are
chronically under-subscribed (≈6 clients/shard), causing SET p=1 throughput
to collapse to 0.07× single-shard baseline. The empirical safe threshold
is ≥25 concurrent clients per shard (benchmark_scaling_concurrency_2026_04_26).
On startup, after resolving num_shards, Moon now checks:
if maxclients < 25 × num_shards → WARN with suggested fix
The warning is:
- Suppressed when num_shards == 1 (no cross-shard dispatch).
- Suppressed when maxclients == 0 (unlimited — no ceiling to warn about).
- Suppressed globally by setting MOON_NO_UNDERSUBSCRIPTION_WARN=1.
Implementation uses a pure function `should_warn_undersubscription(maxclients,
num_shards) -> Option<String>` so it is unit-testable without tracing_test.
The tracing::warn!() call site in main() reads the env var guard directly.
Also adds `compute_auto_shards(parallelism, conservative) -> usize` pure
function in preparation for T1.2 (opt-in conservative cap). Tests for both
functions are included in the same commit so the test suite stays green at
every commit boundary.
Unit tests (src/main.rs #[cfg(test)]):
- no_warn_single_shard
- no_warn_unlimited_clients
- no_warn_sufficient_clients
- warns_below_threshold
- warns_high_shard_count
- threshold_is_inclusive
- auto_shards_conservative_cap (pre-lands the T1.2 pure-function test)
author: Tin Dang
Default behaviour is UNCHANGED: --shards 0 still resolves to
available_parallelism(). This preserves backward compatibility for
operators on high-core hosts who are already using auto-detect.
Operators who observe sub-linear multi-shard scaling (documented at
benchmark_scaling_concurrency_2026_04_26: SET p=1 8-shard at c=50 was
0.07× baseline) can now opt into the empirical sweet-spot by setting:
MOON_AUTO_SHARDS_CONSERVATIVE=1
This caps the auto-detected shard count at min(2, available_parallelism()).
A startup INFO log always explains which path was taken.
The pure function `compute_auto_shards(parallelism, conservative) -> usize`
is unit-tested independently of the env var so correctness is verifiable
without process-level environment manipulation.
Deviation from PLAN.md original spec: the plan described opt-OUT semantics
(MOON_AUTO_SHARDS_AGGRESSIVE=1 to re-enable full vCPU). Changed to opt-IN
at user direction before kickoff — default must not silently degrade existing
operators. PLAN.md updated inline to reflect shipped reality.
Unit test: auto_shards_conservative_cap (src/main.rs)
author: Tin Dang
Counter storage type verified: The existing `record_dispatch_cross_*`
helpers use the `metrics` crate facade (`counter!()` macro → Prometheus
registry). This registry is NOT directly readable from the INFO path —
it only surfaces via the /metrics HTTP endpoint. The counters are therefore
NOT AtomicU64 values accessible via load(); they are opaque handles owned
by the Prometheus recorder. Added dedicated `static AtomicU64` counters
(Relaxed ordering, monotonic event counters) incremented in parallel inside
the existing `record_dispatch_*` helpers. These always work regardless of
whether Prometheus is enabled (admin_port=0).
New counters exposed in `INFO stats`:
total_dispatch_cross_read_fastpath — commands served via the RwLock
shared-read fast path (no SPSC message; low latency but holds a
read lock on the target shard's database).
total_dispatch_cross_spsc — commands dispatched through the SPSC
slow path (covers both reads when fast-path is off AND writes;
no separate write-only counter exists in the codebase).
Note on plan deviation: PLAN.md listed `total_dispatch_cross_write_spsc`
as a separate counter. There is no write-specific SPSC recording in the
existing helpers — both read and write cross-shard non-fastpath traffic
is recorded by `record_dispatch_cross_spsc`. Exposing a split counter
would require adding write-vs-read classification to the handler layer
(a T2.x task). INFO exposes the correct unified total instead.
`fastpath_avg_ns` is not emitted: the latency histogram at
`record_dispatch_cross_read_fastpath_timed` is Prometheus-only. Emitting
a meaningful average would require a second `FASTPATH_TOTAL_NS` atomic.
Deferred to a follow-up if operators request it; the histogram is already
available via /metrics when Prometheus is enabled.
Implementation:
- src/admin/metrics_setup.rs: two new static AtomicU64 counters
(DISPATCH_CROSS_READ_FASTPATH_TOTAL, DISPATCH_CROSS_READ_SPSC_TOTAL);
public getter fns total_dispatch_cross_read_fastpath() and
total_dispatch_cross_spsc(); increments wired into existing helpers.
- src/command/connection.rs: INFO # Stats section extended with the two
new keys.
Unit tests (src/admin/metrics_setup.rs):
- cross_read_fastpath_atomic_increments
- cross_read_fastpath_batch_atomic_increments
- cross_read_fastpath_batch_zero_is_noop
- cross_spsc_atomic_increments
- cross_spsc_batch_atomic_increments
- cross_spsc_batch_zero_is_noop
author: Tin Dang
…llapsible_if
`if let Some(msg) { if env.is_none() { ... } }` violates clippy::collapsible_if.
Use `if let ... && cond` (let-chains, stable since Rust 1.64) to collapse into a
single if-expression. Semantics and runtime behaviour are unchanged.
author: Tin Dang
Submodule HEAD advances from 3bbfd04 to 1d05828, which contains: 1d05828 docs(plan): update T1.2 spec to reflect opt-IN semantics This pointer bump was previously folded into a commit that was dropped during rebase. Lifting it into its own atomic commit so the parent repo and planning submodule agree on HEAD. 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? |
📝 WalkthroughWalkthroughThis PR adds cross-shard dispatch observability to INFO output, marks three commands (COPY DB, MOVE, SWAPDB) as unimplemented with tracking IDs, and introduces CPU-based auto-detection of shard count at startup with optional warnings for undersized client limits. ChangesCross-shard dispatch observability
Unimplemented command stubs
Startup auto-sharding and warnings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 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 `@src/admin/metrics_setup.rs`:
- Around line 1471-1480: The zero-count batch test
cross_read_fastpath_batch_zero_is_noop currently only checks monotonicity and
can miss regressions where record_dispatch_cross_read_fastpath_batch(0)
increments the counter; change the test to serialize access (avoid concurrent
interference) by running it single-threaded and assert equality instead of >=:
capture before = total_dispatch_cross_read_fastpath(), call
record_dispatch_cross_read_fastpath_batch(0), then assert after == before; apply
the same change to the other zero-count dispatch tests (the equivalent test
around lines 1506-1513) using their corresponding helper functions (e.g.,
total_dispatch_... and record_dispatch_..._batch) so zero-count calls are
verified to be no-ops.
🪄 Autofix (Beta)
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
Run ID: c4e96f79-2ef6-4fee-9f7c-8e778e84e7f1
📒 Files selected for processing (7)
.planningsrc/admin/metrics_setup.rssrc/command/connection.rssrc/command/key_extra.rssrc/command/metadata.rssrc/command/mod.rssrc/main.rs
Comment on lines
+1471
to
+1480
| fn cross_read_fastpath_batch_zero_is_noop() { | ||
| // batch(0) must not call fetch_add — the counter must not move | ||
| // backward; it may move forward due to concurrent tests. | ||
| let before = total_dispatch_cross_read_fastpath(); | ||
| record_dispatch_cross_read_fastpath_batch(0); | ||
| let after = total_dispatch_cross_read_fastpath(); | ||
| assert!( | ||
| after >= before, | ||
| "counter must be monotone; before={before} after={after}" | ||
| ); |
There was a problem hiding this comment.
Zero-count batch tests are currently non-detecting for regressions.
Both tests only assert monotonicity, so they still pass if *_batch(0) accidentally increments. Please serialize the dispatch-counter tests and assert equality for zero-count cases.
Suggested hardening
+ static DISPATCH_COUNTER_TEST_GUARD: once_cell::sync::Lazy<parking_lot::Mutex<()>> =
+ once_cell::sync::Lazy::new(|| parking_lot::Mutex::new(()));
#[test]
fn cross_read_fastpath_batch_zero_is_noop() {
+ let _guard = DISPATCH_COUNTER_TEST_GUARD.lock();
let before = total_dispatch_cross_read_fastpath();
record_dispatch_cross_read_fastpath_batch(0);
let after = total_dispatch_cross_read_fastpath();
- assert!(
- after >= before,
- "counter must be monotone; before={before} after={after}"
- );
+ assert_eq!(after, before, "batch(0) must not increment");
}
#[test]
fn cross_spsc_batch_zero_is_noop() {
+ let _guard = DISPATCH_COUNTER_TEST_GUARD.lock();
let before = total_dispatch_cross_spsc();
record_dispatch_cross_spsc_batch(0);
let after = total_dispatch_cross_spsc();
- assert!(
- after >= before,
- "counter must be monotone; before={before} after={after}"
- );
+ assert_eq!(after, before, "batch(0) must not increment");
}Also applies to: 1506-1513
🤖 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/admin/metrics_setup.rs` around lines 1471 - 1480, The zero-count batch
test cross_read_fastpath_batch_zero_is_noop currently only checks monotonicity
and can miss regressions where record_dispatch_cross_read_fastpath_batch(0)
increments the counter; change the test to serialize access (avoid concurrent
interference) by running it single-threaded and assert equality instead of >=:
capture before = total_dispatch_cross_read_fastpath(), call
record_dispatch_cross_read_fastpath_batch(0), then assert after == before; apply
the same change to the other zero-count dispatch tests (the equivalent test
around lines 1506-1513) using their corresponding helper functions (e.g.,
total_dispatch_... and record_dispatch_..._batch) so zero-count calls are
verified to be no-ops.
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
Tier 1 of the multi-shard improvements plan (see
.planning/multi-shard-improvements/CONTEXT.md+PLAN.md). Five tasks closing documented operator-UX footguns. No hot-path regressions.b9c0bee— Startup warn whenmaxclients < 25 × shards(footgun documented inbenchmark_scaling_concurrency_2026_04_26). Suppressible viaMOON_NO_UNDERSUBSCRIPTION_WARN=1. Pure-functionshould_warn_undersubscription(maxclients, num_shards) -> Option<String>with 7 unit tests.4575d34— Opt-IN auto-shard cap viaMOON_AUTO_SHARDS_CONSERVATIVE=1(clamps--shards 0auto-detect tomin(2, vCPU)). Default unchanged: baremoon --shards 0still resolves toavailable_parallelism(). User-requested deviation from the originally-planned opt-OUT semantic to avoid silently degrading existing operators on high-core hosts.60ab2fd— Reframed SWAPDB/MOVE/COPY-DB error text from misleading "not supported in sharded mode" to accurate "not implemented (tracked in T2.x)". Added MOVE to the dispatch table and COMMAND_META as a stub. 3 unit tests.959354a— Surface cross-shard dispatch metrics inINFO stats(total_dispatch_cross_read_fastpath,total_dispatch_cross_spsc) via dedicatedstatic AtomicU64withOrdering::Relaxed. Works without admin port enabled — the original gap.project_multi_shard_limitations.mdre:--shards 0default (still0, not1).6a1a044— clippy::collapsible_if cleanup on lint introduced by T1.1.46ed28a—.planningsubmodule pointer bump for T1.2 opt-IN doc.Benchmark validation (OrbStack moon-dev, aarch64,
--shards 4 -c 200)T1.4 is the only task touching a hot path. Median-of-3 at p=64:
4575d34)46ed28a)Mixed-sign deltas at same depth = noise, not regression. A real cost from the
Relaxedfetch_add would show consistent negative deltas across both ops. T1.4 is performance-neutral.INFO output verified live:
total_dispatch_cross_read_fastpath:1124781,total_dispatch_cross_spsc:1124709.Test plan
cargo fmt --check— cleancargo clippy --release -- -D warnings(default features) — cleancargo clippy --release --no-default-features --features runtime-tokio,jemalloc -- -D warnings— cleanredis-cli INFO stats | grep -i dispatchreturns the new counterscargo test --release(3200+ tests) — agent reported pass on the original commits; CI to re-confirm against rebased treeNotes for reviewer
d532b74after upstream advanced 9 commits during the impl session. The rebase dropped a problematic commit (117a9ab) that inverted tombstone GC semantics (0,0→u64::MAX,u64::MAX) on the WAL-overflow emergency checkpoint path. Main upstream independently arrived at the correct0,0. Seefeedback_agent_self_doubt_inversion.mdin~/.claude/.../memory/for the postmortem.c4a Axionper CLAUDE.md.Closes the T1 acceptance criteria in
.planning/multi-shard-improvements/CONTEXT.md.Summary by CodeRabbit
New Features
--shards 0is specified, with optional conservative capping via environment variable.maxclientsis insufficient for configured shards.Bug Fixes
SWAPDBandCOPY DBcommands with tracking references.Chores
MOVEcommand stub (returns "not implemented" error).