perf(shard): lock-free cross-shard read recovery — idle+batch-gated reply spin + dead-flag cleanup

[pilotspacex-byte]

Summary

Recovers the cross-shard single-key read latency that shardslice-migration (#175) gave up, without re-introducing a cross-thread lock or growing per-key memory — discharging the cross-shard-read half of that PR's RISK-ACCEPTED waiver (expires 2026-08-01).

A cross-shard GET pays two mandatory cross-thread wakes per round-trip (origin→owner request, owner→origin reply). This PR removes the reply-side wake with an idle-gated, batch-depth-gated reply-side spin: when the requesting shard has no other in-flight cross-shard waiters and the batch contains a single cross-shard read, the connection polls its reply slot instead of parking + being woken across threads. The second (origin→owner) wake is irreducible without RCU/snapshot reads, which were declined this milestone to protect moon's low-RSS differentiator — so this is the documented single-connection floor for the safe approach.

Also hard-removes the orphaned --cross-shard-fast-path flag and its dead metric/histogram/functions (zero production callers since the shardslice cutover).

What changed

• C2 — idle + batch-depth gated reply spin (src/shard/slice.rs, both connection handlers, src/server/response_slot.rs)
  • Thread-local Cell<u32> inflight gate (xshard_may_spin) — single-threaded shard ⇒ no atomic/lock. XshardWaitGuard RAII spans the reply-wait.
  • Batch-depth gate (xshard_should_spin(batch_remote), XSHARD_SPIN_MAX_BATCH_REMOTE=1): only a singleton cross-shard read spins; any pipelined fan-out parks (prevents serializing the pipeline).
  • ResponseSlot take consolidated into one take_if_filled() helper (net −2 unsafe blocks).
• C4 — dead fast-path surface removed (BREAKING): --cross-shard-fast-path flag, CrossShardFastPath enum, cross_shard_fast_path_enabled, moon_cross_shard_lock_contention_total, moon_dispatch_cross_read_fastpath_latency_us, the record_dispatch_cross_read_fastpath_* fns, and the hardcoded cross_read_fast_dispatches. clap now rejects --cross-shard-fast-path. See CHANGELOG.

Evidence

3-way same-run, best-of-5, quiesced + core-pinned OrbStack VM (absolute VM RPS is untrusted per the milestone's documented instrument exception — relative same-run ratio only):

cell	no-mech	with-mech (fix)	Δ
s4-c1-GET (the win)	20243	24892	+22.9%
s4-P16-GET (pipelined guard)	2479339	2608696	+5.2% (no serialization)
s4-c100-GET (starvation guard)	188679	208189	+10.3%
s1-P16-CTRL (noise floor)	4166667	4109589	−1.4% (flat)

vs the pre-regression tag 3e376a1, the with-mech c1-GET regression is −19.4% ≤ ~20% (the contract's relative target). The absolute "≥30k" line was retired (whole-VM baseline itself drifted 37580→31437→20243 across clean runs) — the durable signal is the same-run +18–23% recovery.

• Correctness/liveness: cross-shard surface 23/23 monoio + 23/23 tokio on the fix (shardslice_live = real cross-shard reads, cross_shard_consistency_red = data correctness). scripts/test-consistency.sh 197/197 @ shards=1/4/12 unchanged.
• Safety: audit-unsafe.sh 218/218 (net −2 unsafe), audit-unwrap.sh 0/0, zero new cross-thread lock (shardslice_shape.rs 5/5), RSS flat (49456→48992 kB HWM under 4-shard -r 1M).
• clippy -D warnings ×2 featuresets + fmt --check clean.

Verify caught + fixed (in-phase)

The first with-mech build regressed pipelined throughput −27.5% on s4-P16 — a synchronous spin serializes a pipelined cross-shard fan-out (exactly the contract's pre-named flag-#1 risk). Fixed by the batch-depth gate so only singleton reads spin; re-measured to +5.2% (table above).

Scope / deferral

Cross-connection read coalescing (C3, the multi-client P1 case) is deferred to follow-up task xshard-read-coalescing (human-approved v2 change request). The dead-code type CoalescedReadBatch is retained as the seam.

Breaking change

--cross-shard-fast-path is removed; passing it now errors. It has been a no-op since the shardslice cutover. Documented in CHANGELOG under [Unreleased] › Removed (BREAKING).

🤖 ADD task xshard-read-fastpath · milestone v2-performance · gate PASS 2026-06-14

Summary by CodeRabbit

Release Notes

• Breaking Changes

  • Removed --cross-shard-fast-path command-line flag.

• Performance Improvements

  • Optimized cross-shard single-key read latency using lock-free, adaptive reply-side polling.

• Documentation

  • Updated project conventions and development documentation for upcoming milestone versions.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

📝 Walkthrough

Walkthrough

Implements an idle-gated adaptive busy-spin for cross-shard single-key read replies on both the monoio and sharded handlers, using a new thread-local in-flight counter and RAII XshardWaitGuard. Adds ResponseSlot::try_take for non-blocking polling. Removes the now-superseded --cross-shard-fast-path CLI flag, CrossShardFastPath enum, and all associated metrics/config. Closes milestone v1-shared-nothing and opens v2-performance with the completed task recorded.

Changes

xshard-read-fastpath: idle-gated busy-spin + dead flag cleanup

Layer / File(s)	Summary
Idle-gate contract: constants, helpers, and XshardWaitGuard src/shard/slice.rs, src/shard/dispatch.rs	Adds thread-local XSHARD_INFLIGHT, spin constants (XSHARD_SPIN_GATE, XSHARD_SPIN_BUDGET, XSHARD_SPIN_MAX_BATCH_REMOTE), public xshard_may_spin/xshard_should_spin helpers, RAII XshardWaitGuard, and the deferred-coalescing CoalescedReadBatch type stub.
ResponseSlot::try_take and poll_take refactor src/server/response_slot.rs	Extracts a private take_if_filled helper for the FILLED→EMPTY state transition, refactors poll_take to use it, and adds try_take as a non-blocking busy-poll variant for spinning callers.
Handler wiring: spin path in monoio and sharded handlers src/server/conn/handler_monoio/mod.rs, src/server/conn/handler_sharded/mod.rs	Wires XshardWaitGuard and the xshard_should_spin decision into both runtimes' cross-shard reply-wait paths; the monoio handler busy-polls reply_rx.try_recv and the sharded handler busy-spins on the response slot via try_take before falling back to parked awaits. Updates per-batch dispatch counters.
Remove CrossShardFastPath enum, config field, and metrics src/config.rs, src/admin/metrics_setup.rs, src/command/connection.rs	Deletes CrossShardFastPath enum, cross_shard_fast_path config field, cross_shard_fast_path_enabled(), the DISPATCH_CROSS_READ_FASTPATH_TOTAL atomic and its record_*/total_* helpers, and the total_dispatch_cross_read_fastpath INFO stat field.
API pin tests, cleanup shape test, and test harness cleanup tests/xshard_fastpath_api.rs, tests/xshard_cleanup_shape.rs, tests/integration.rs, tests/*.rs	Adds idle-gate boundary/composition pin tests and a CoalescedReadBatch type assertion; adds a source-grep test enforcing all dead fast-path symbols are absent; removes the cross_shard_fast_path field from ServerConfig initializations across all integration test helpers.
Benchmarking and measurement scripts scripts/baseline-xshard-quiesced.sh, scripts/probe-xshard.sh, scripts/remeasure-xshard-fix.sh, scripts/spike-xshard-read.sh, scripts/bench-cross-shard-fastpath.sh (deleted)	Adds four new measurement/spike scripts (multi-commit quiesced RPS, single-client GET probe, batch-gate regression validation, focused cross-shard spike); removes the now-dead bench-cross-shard-fastpath.sh.
ADD/project docs: task spec, milestone closure, and process conventions .add/tasks/xshard-read-fastpath/TASK.md, .add/milestones/v1-shared-nothing/..., .add/milestones/v2-performance/MILESTONE.md, .add/state.json, .add/CONVENTIONS.md, .add/PROJECT.md, CHANGELOG.md	Adds the full xshard-read-fastpath task document (SPECIFY/SCENARIOS/CONTRACT/TESTS/VERIFY/OBSERVE), closes v1-shared-nothing with a RETRO, introduces the v2-performance milestone, advances state.json to v2, updates CONVENTIONS.md with red-suite and ADD-freeze rules, and records CHANGELOG entries for the breaking flag removal and the performance change.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Handler as monoio / sharded Handler
  participant Gate as xshard_should_spin
  participant Slot as ResponseSlot / reply_rx
  participant ShardWorker as Target Shard Worker

  Client->>Handler: cross-shard GET request
  Handler->>ShardWorker: dispatch read (SPSC channel)
  Handler->>Handler: XshardWaitGuard::new() → XSHARD_INFLIGHT++
  Handler->>Gate: xshard_should_spin(batch_remote_total)
  alt inflight ≤ SPIN_GATE AND batch_remote = 1
    loop ≤ XSHARD_SPIN_BUDGET iterations
      Handler->>Slot: try_take() / try_recv()
      ShardWorker-->>Slot: write response
      Slot-->>Handler: Some(frames)
      Handler-->>Client: reply (early return)
    end
  else gate closed or deep pipeline
    Handler->>Slot: await future_for / race2 chunked wait
    Slot-->>Handler: frames
    Handler-->>Client: reply
  end
  Handler->>Handler: XshardWaitGuard::drop() → XSHARD_INFLIGHT--

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• pilotspace/moon#136: Touches the same monoio cross-shard dispatch and reply-wait pipeline in handler_monoio/mod.rs that this PR extends with the adaptive spin path.
• pilotspace/moon#17: Introduced the ResponseSlot design that this PR refactors (adding take_if_filled and try_take) to support busy-polling callers.

Suggested labels

enhancement

🐇 Hop, hop — the cross-shard wait is slimmed!
No lock to grab, no mutex brimmed,
A spin-guard wakes the sleeping slot,
try_take checks if data's hot,
The dead flag's gone, the path is clear —
v1 is closed, v2 is here! 🌙✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely summarizes the main changes: lock-free cross-shard read recovery via idle+batch-gated reply spin, and removal of dead flag code.
Description check	✅ Passed	The PR description comprehensively covers all required sections: Summary (what and why), detailed What changed breakdown, Evidence with performance tables and test results, verification findings, and scope/deferral notes. All checklist items are addressed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/xshard-read-fastpath

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

scripts/probe-xshard.sh (1)

27-30: 💤 Low value

Consider making commit hashes configurable.

The hardcoded commit hashes 3e376a1 and a497602 reduce flexibility. For a reusable diagnostic, consider accepting them as environment variables with these as defaults:

+BASELINE_COMMIT="${BASELINE_COMMIT:-3e376a1}"
+CURRENT_COMMIT="${CURRENT_COMMIT:-a497602}"
 for r in 1 2 3; do
-  probe "$BINS/moon-3e376a1-monoio" 1 "3e376a1 (v0.3.0 fast) LOCAL"
-  probe "$BINS/moon-3e376a1-monoio" 4 "3e376a1 (v0.3.0 fast) XSHARD"
-  probe "$BINS/moon-a497602-monoio" 1 "a497602 (HEAD slow)  LOCAL"
-  probe "$BINS/moon-a497602-monoio" 4 "a497602 (HEAD slow)  XSHARD"
+  probe "$BINS/moon-${BASELINE_COMMIT}-monoio" 1 "${BASELINE_COMMIT} (v0.3.0 fast) LOCAL"
+  probe "$BINS/moon-${BASELINE_COMMIT}-monoio" 4 "${BASELINE_COMMIT} (v0.3.0 fast) XSHARD"
+  probe "$BINS/moon-${CURRENT_COMMIT}-monoio" 1 "${CURRENT_COMMIT} (HEAD slow)  LOCAL"
+  probe "$BINS/moon-${CURRENT_COMMIT}-monoio" 4 "${CURRENT_COMMIT} (HEAD slow)  XSHARD"

🤖 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/probe-xshard.sh` around lines 27 - 30, Replace the hardcoded commit
hashes 3e376a1 and a497602 in the probe function calls with environment
variables. Define environment variables at the top of the script (for example,
COMMIT_HASH_1 and COMMIT_HASH_2) with default values set to the current
hardcoded hashes, then substitute these variables into the probe calls. This
allows the script to be reused with different commit hashes without requiring
edits to the script itself.

scripts/remeasure-xshard-fix.sh (1)

24-29: 💤 Low value

Consider cleaning up temporary directories.

The measure function creates a temp directory on line 24 but doesn't remove it after killing the process on line 28. While the OS will eventually clean /tmp, explicitly removing it would be cleaner:

   vals+=("$(bench "$4" "$5" "$6")")
   kill "$pid" 2>/dev/null; wait "$pid" 2>/dev/null
+  rm -rf "$dir"
 done

🤖 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/remeasure-xshard-fix.sh` around lines 24 - 29, The measure function
creates a temporary directory stored in the dir variable but does not clean it
up after the process is killed. After the kill and wait commands on line 28, add
an explicit cleanup command to remove the temporary directory stored in the dir
variable to ensure proper resource management and avoid accumulating temporary
files in /tmp.

scripts/baseline-xshard-quiesced.sh (1)

146-147: 💤 Low value

Median calculation handles both odd and even sample counts correctly.

The ternary expression correctly computes the median: for odd N it takes the middle element a[(NR+1)/2], and for even N it averages the two middle elements (a[NR/2]+a[NR/2+1])/2. Note that remeasure-xshard-fix.sh uses a simpler rounding approach instead of averaging; consider aligning them for consistency.

🤖 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/baseline-xshard-quiesced.sh` around lines 146 - 147, The median
calculation in the med variable using the awk ternary expression is logically
correct, but for consistency with the simpler rounding approach used in
remeasure-xshard-fix.sh, update the med variable calculation to match that
file's median computation method instead of using the averaging approach for
even-sized sample counts.

🤖 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/spike-xshard-read.sh`:
- Line 30: Remove the `--disk-offload disable` flag from the Redis server
startup command in the script to prevent unrecognized option errors when
measuring against older commits that don't support this flag. The flag appears
in the command string alongside other flags like `--appendonly no` and should be
deleted entirely from the line to ensure compatibility across different binary
versions being benchmarked.

In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 1649-1660: The busy-spin logic controlled by
xshard_should_spin(batch_remote_total) is currently applied to all remote
operations, including writes, but it should only apply to remote reads. Add an
additional condition to the if statement that gates the spin loop to ensure it
only executes for read operations, not write operations. Since meta already
carries aof_bytes information, check that aof_bytes is zero or empty (indicating
a read-only operation with no AOF overhead) before entering the spin loop. This
ensures the fast path optimization remains limited to singleton remote reads as
originally intended, preventing unnecessary CPU usage on the slower write/AOF
path.

In `@src/shard/slice.rs`:
- Around line 283-315: The XshardWaitGuard struct must be made thread-affine to
prevent it from migrating across threads when held across an await point on a
multithreaded runtime. Replace the `_priv: ()` field with `_priv:
PhantomData<Rc<()>>` to add a !Send marker. This enforces that the guard cannot
move between threads, ensuring the increment and decrement operations on the
thread-local XSHARD_INFLIGHT counter happen on the same thread and preventing
counter corruption as documented in the struct comments.

---

Nitpick comments:
In `@scripts/baseline-xshard-quiesced.sh`:
- Around line 146-147: The median calculation in the med variable using the awk
ternary expression is logically correct, but for consistency with the simpler
rounding approach used in remeasure-xshard-fix.sh, update the med variable
calculation to match that file's median computation method instead of using the
averaging approach for even-sized sample counts.

In `@scripts/probe-xshard.sh`:
- Around line 27-30: Replace the hardcoded commit hashes 3e376a1 and a497602 in
the probe function calls with environment variables. Define environment
variables at the top of the script (for example, COMMIT_HASH_1 and
COMMIT_HASH_2) with default values set to the current hardcoded hashes, then
substitute these variables into the probe calls. This allows the script to be
reused with different commit hashes without requiring edits to the script
itself.

In `@scripts/remeasure-xshard-fix.sh`:
- Around line 24-29: The measure function creates a temporary directory stored
in the dir variable but does not clean it up after the process is killed. After
the kill and wait commands on line 28, add an explicit cleanup command to remove
the temporary directory stored in the dir variable to ensure proper resource
management and avoid accumulating temporary files in /tmp.

🪄 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: f2627f49-9063-465a-9828-5d01b2477c8c

📥 Commits

Reviewing files that changed from the base of the PR and between 05efb80 and 5985c30.

📒 Files selected for processing (39)

• .add/CONVENTIONS.md
• .add/PROJECT.md
• .add/milestones/v1-shared-nothing/MILESTONE.md
• .add/milestones/v1-shared-nothing/RETRO.md
• .add/milestones/v2-performance/MILESTONE.md
• .add/state.json
• .add/tasks/hotpath-lock-quickwins/TASK.md
• .add/tasks/xshard-read-fastpath/TASK.md
• CHANGELOG.md
• scripts/baseline-xshard-quiesced.sh
• scripts/bench-cross-shard-fastpath.sh
• scripts/probe-xshard.sh
• scripts/remeasure-xshard-fix.sh
• scripts/spike-xshard-read.sh
• src/admin/metrics_setup.rs
• src/command/connection.rs
• src/config.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn/handler_sharded/mod.rs
• src/server/response_slot.rs
• src/shard/dispatch.rs
• src/shard/slice.rs
• tests/ft_search_multi_shard_as_of.rs
• tests/ft_search_temporal_parity.rs
• tests/hybrid_filter_backward_compat.rs
• tests/hybrid_filter_multishard.rs
• tests/hybrid_filter_tag.rs
• tests/integration.rs
• tests/kill_snapshot.rs
• tests/lunaris_hybrid_ft_search.rs
• tests/mq_integration.rs
• tests/pipeline_auto_index.rs
• tests/replication_test.rs
• tests/txn_ft_search_snapshot.rs
• tests/txn_kv_wiring.rs
• tests/vacuum_commands.rs
• tests/workspace_integration.rs
• tests/xshard_cleanup_shape.rs
• tests/xshard_fastpath_api.rs

💤 Files with no reviewable changes (16)

• scripts/bench-cross-shard-fastpath.sh
• tests/replication_test.rs
• src/command/connection.rs
• tests/hybrid_filter_tag.rs
• tests/mq_integration.rs
• tests/txn_kv_wiring.rs
• tests/hybrid_filter_multishard.rs
• tests/ft_search_temporal_parity.rs
• tests/lunaris_hybrid_ft_search.rs
• tests/txn_ft_search_snapshot.rs
• tests/hybrid_filter_backward_compat.rs
• tests/workspace_integration.rs
• tests/ft_search_multi_shard_as_of.rs
• tests/pipeline_auto_index.rs
• src/admin/metrics_setup.rs
• src/config.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if --disk-offload is recognized by the binary
"$BINARY" --help 2>&1 | grep -q 'disk-offload' || echo "WARN: --disk-offload not supported by this binary"

Repository: pilotspace/moon

Length of output: 109

---

Remove --disk-offload disable flag or expect unrecognized option errors when measuring older commits.

The --disk-offload flag is not supported by the binary being measured. While acceptable for this focused spike, measuring commits from before this feature was introduced will require removing this flag to avoid errors.

🤖 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/spike-xshard-read.sh` at line 30, Remove the `--disk-offload disable`
flag from the Redis server startup command in the script to prevent unrecognized
option errors when measuring against older commits that don't support this flag.
The flag appears in the command string alongside other flags like `--appendonly
no` and should be deleted entirely from the line to ensure compatibility across
different binary versions being benchmarked.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate the busy-spin to remote reads only.

This condition currently spins for any singleton cross-shard batch, including writes, because it only checks batch_remote_total. meta already carries aof_bytes, so a lone remote SET/write will still busy-spin on the executor thread before falling back to future_for(...).await, which widens this fast path beyond the PR’s “singleton read” contract and risks burning CPU on the slower write/AOF path.

Suggested fix

-                            if crate::shard::slice::xshard_should_spin(batch_remote_total) {
+                            let is_single_remote_read =
+                                matches!(meta.as_slice(), [(_, None, _)]);
+                            if is_single_remote_read
+                                && crate::shard::slice::xshard_should_spin(batch_remote_total)
+                            {
                                 for _ in 0..crate::shard::slice::XSHARD_SPIN_BUDGET {
                                     if let Some(r) = response_pool.slot_for(target).try_take() {
                                         spun = Some(r);
                                         break;
                                     }

📝 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

	if crate::shard::slice::xshard_should_spin(batch_remote_total) {
	for _ in 0..crate::shard::slice::XSHARD_SPIN_BUDGET {
	if let Some(r) = response_pool.slot_for(target).try_take() {
	spun = Some(r);
	break;
	}
	core::hint::spin_loop();
	}
	}
	match spun {
	Some(r) => r,
	None => response_pool.future_for(target).await,
	let is_single_remote_read =
	matches!(meta.as_slice(), [(_, None, _)]);
	if is_single_remote_read
	&& crate::shard::slice::xshard_should_spin(batch_remote_total)
	{
	for _ in 0..crate::shard::slice::XSHARD_SPIN_BUDGET {
	if let Some(r) = response_pool.slot_for(target).try_take() {
	spun = Some(r);
	break;
	}
	core::hint::spin_loop();
	}
	}
	match spun {
	Some(r) => r,
	None => response_pool.future_for(target).await,

🤖 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/handler_sharded/mod.rs` around lines 1649 - 1660, The
busy-spin logic controlled by xshard_should_spin(batch_remote_total) is
currently applied to all remote operations, including writes, but it should only
apply to remote reads. Add an additional condition to the if statement that
gates the spin loop to ensure it only executes for read operations, not write
operations. Since meta already carries aof_bytes information, check that
aof_bytes is zero or empty (indicating a read-only operation with no AOF
overhead) before entering the spin loop. This ensures the fast path optimization
remains limited to singleton remote reads as originally intended, preventing
unnecessary CPU usage on the slower write/AOF path.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Make XshardWaitGuard thread-affine with a !Send marker.

XshardWaitGuard currently implements Send because it only holds (), allowing the guard to migrate across threads if held across an .await on a multithreaded runtime. When the guard drops on a different thread than where it was created, the decrement will operate on a different thread-local XSHARD_INFLIGHT counter than the increment, corrupting both counters and skewing xshard_may_spin() decisions. Adding PhantomData<Rc<()>> enforces thread-affinity by making the type !Send, matching the documented invariant.

Suggested fix

 pub struct XshardWaitGuard {
-    // Private field: construct only via `new()`, and a `PhantomData<Rc<()>>`-free
-    // marker is unnecessary because the type holds no data that could be moved
-    // across threads; callers keep it on the shard thread's stack.
-    _priv: (),
+    // Keep the guard thread-affine: the thread-local counter must be decremented
+    // on the same thread that incremented it.
+    _not_send: PhantomData<Rc<()>>,
 }
 
 impl XshardWaitGuard {
     /// Enter a cross-shard reply-wait: increment this thread's in-flight count.
     #[inline]
     pub fn new() -> Self {
         XSHARD_INFLIGHT.with(|c| c.set(c.get().saturating_add(1)));
-        Self { _priv: () }
+        Self {
+            _not_send: PhantomData,
+        }
     }
 }

📝 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

	/// RAII guard that marks the current connection as an in-flight cross-shard
	/// reply-waiter for its lifetime: increments `XSHARD_INFLIGHT` on construction,
	/// decrements on drop. Wrap every cross-shard reply-wait in one so the idle gate
	/// reflects reality even across early returns / cancellation. `!Send` in spirit —
	/// the thread-local it guards is per-thread and the guard must not cross threads.
	#[must_use = "the guard must be held for the duration of the reply-wait; dropping it early ends the in-flight window"]
	pub struct XshardWaitGuard {
	// Private field: construct only via `new()`, and a `PhantomData<Rc<()>>`-free
	// marker is unnecessary because the type holds no data that could be moved
	// across threads; callers keep it on the shard thread's stack.
	_priv: (),
	}
	impl XshardWaitGuard {
	/// Enter a cross-shard reply-wait: increment this thread's in-flight count.
	#[inline]
	pub fn new() -> Self {
	XSHARD_INFLIGHT.with(|c| c.set(c.get().saturating_add(1)));
	Self { _priv: () }
	}
	}
	impl Default for XshardWaitGuard {
	#[inline]
	fn default() -> Self {
	Self::new()
	}
	}
	impl Drop for XshardWaitGuard {
	#[inline]
	fn drop(&mut self) {
	XSHARD_INFLIGHT.with(|c| c.set(c.get().saturating_sub(1)));
	/// RAII guard that marks the current connection as an in-flight cross-shard
	/// reply-waiter for its lifetime: increments `XSHARD_INFLIGHT` on construction,
	/// decrements on drop. Wrap every cross-shard reply-wait in one so the idle gate
	/// reflects reality even across early returns / cancellation. `!Send` in spirit —
	/// the thread-local it guards is per-thread and the guard must not cross threads.
	#[must_use = "the guard must be held for the duration of the reply-wait; dropping it early ends the in-flight window"]
	pub struct XshardWaitGuard {
	// Keep the guard thread-affine: the thread-local counter must be decremented
	// on the same thread that incremented it.
	_not_send: PhantomData<Rc<()>>,
	}
	impl XshardWaitGuard {
	/// Enter a cross-shard reply-wait: increment this thread's in-flight count.
	#[inline]
	pub fn new() -> Self {
	XSHARD_INFLIGHT.with(|c| c.set(c.get().saturating_add(1)));
	Self {
	_not_send: PhantomData,
	}
	}
	}
	impl Default for XshardWaitGuard {
	#[inline]
	fn default() -> Self {
	Self::new()
	}
	}
	impl Drop for XshardWaitGuard {
	#[inline]
	fn drop(&mut self) {
	XSHARD_INFLIGHT.with(|c| c.set(c.get().saturating_sub(1)));

🤖 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/shard/slice.rs` around lines 283 - 315, The XshardWaitGuard struct must
be made thread-affine to prevent it from migrating across threads when held
across an await point on a multithreaded runtime. Replace the `_priv: ()` field
with `_priv: PhantomData<Rc<()>>` to add a !Send marker. This enforces that the
guard cannot move between threads, ensuring the increment and decrement
operations on the thread-local XSHARD_INFLIGHT counter happen on the same thread
and preventing counter corruption as documented in the struct comments.