fix(hardening): production-hardening audit batches A–D (28-finding sweep)#250
Merged
Conversation
Six wire-reachable command paths fed a client-supplied count straight
into Vec::with_capacity / Vec::resize with no upper bound. On a real host
the oversized request fails allocation and Rust's handle_alloc_error calls
abort() -- uncatchable by catch_unwind, crashing every shard and every
connection, not just the offending client. A single unauthenticated
request was therefore a full-process DoS.
Sites fixed (production-hardening audit Batch A + allocation sweep):
- FT.SEARCH HIGHLIGHT/SUMMARIZE FIELDS <count> (ft_text_search.rs)
clamp count to remaining-arg tokens before Vec::with_capacity
- HSCAN COUNT <n> (hash_read.rs)
bound pre-size by actual field count: count.min(total)
- SRANDMEMBER negative COUNT (+ readonly twin) (set_read.rs)
cap to members.len()*10 (matches HRANDFIELD/ZRANDMEMBER)
- BITFIELD SET/INCRBY bit-offset (string_bit.rs)
reject offset past the 512MB string limit (matches SETBIT); GET exempt
- ZMPOP COUNT <n> (sorted_set_write.rs)
bound pre-size by cardinality: pop_count.min(card)
MISSED by the audit finders; caught by the Batch A allocation sweep
Verified HRANDFIELD, LMPOP, ZUNIONSTORE numkeys, and RESTORE-family
numkeys sites already clamp. Each fix has a red/green test asserting the
huge-count request returns a bounded result / error instead of aborting;
271 touched-module tests pass.
Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 1-6, 9 (Batch A).
author: Tin Dang
Two critical failure-design defects from the production-hardening audit (Batch B), both dual-runtime (tokio + monoio): 1. The gossip PING ticker (cluster/gossip.rs) spawned an unbounded connect+write+read task every 100ms with no timeout. Against a dead or partitioned peer these tasks piled up forever (task/FD/memory leak). The "random peer" selection also always picked the first non-self node. Fixed: a single in-flight guard (AtomicBool) bounds outstanding probes to one, a probe timeout (node_timeout/2, >=100ms) cancels a hung connect/read (tokio::time::timeout; monoio select! against sleep), and ping targets now rotate across peers. 2. The monoio per-shard accept task (shard/event_loop.rs) used the BLOCKING flume::Sender::send() on a bounded(256) channel. The accept task shares the shard's single monoio thread with the event-loop consumer, so a full channel blocked the very loop that drains it -> shard deadlock under connection-storm churn. Fixed by send_async().await (yields cooperatively, keeps backpressure, no dropped connections). Both compile under runtime-monoio and runtime-tokio; clippy -D warnings clean on both; 189 cluster+shard module tests pass. The leak/deadlock behaviors are integration-level (dead peer / connection storm) so verification here is compile + reasoning + existing tests. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 7, 8 (Batch B). author: Tin Dang
…ctor WAL Batch C of the production-hardening audit -- the storage-path decisions. The DiskANN cold tier was live-by-default (--disk-offload defaults "enable", --segment-cold-after defaults 86400) yet incomplete: no cold-segment deletion (audit finding 19), blocking per-hop uring I/O holding the segment mutex (24), FT.INFO under-counts cold segments (28), unfinished PQ-codebook restart reload, and a library-code panic in DiskAnnSegment::new on file-open. - Fence the WARM->COLD transition behind MOON_VEC_COLD_TIER=1 (default off) at both event-loop cold-check gates. Warm-tier mmap + LRU eviction (--vec-warm-mmap-budget) handles out-of-RAM indexes by default; the experimental cold path only runs on explicit opt-in. - DiskAnnSegment::new -> io::Result: propagate the open error instead of panicking, so a cold transition can never abort the shard. - Delete src/vector/persistence/wal_record.rs: dead code, never on the live recovery path (manifest + segment + keymap + dedup rescan is the authority), and its own doc warned of double-apply if re-enabled. - Reconcile config docs: segment-cold-after IS consumed (behind the gate); segment-cold-min-qps / vec-diskann-* remain reserved for the cold path. Both runtimes compile + clippy -D warnings clean; 79 diskann+tiered module tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 19, 24, 28 + user decision (fence DiskANN, delete vector WAL). author: Tin Dang
Batch D (part 1) of the production-hardening audit. - APPEND now enforces the 512MB max string size like SETRANGE/SETBIT; before, repeated APPENDs could grow a value past the documented limit unbounded. (finding 22) - FT.* commands are now explicitly rejected inside MULTI/EXEC on the production sharded + monoio handlers, matching handler_single's guard, so the failure is a clear "not supported inside MULTI/EXEC" error instead of an incidental unknown/queued-command error. (finding 25) Both runtimes compile + clippy -D warnings clean; 119 string tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 22, 25. author: Tin Dang
The background/manual GraphUnion merge recall gate read `recall < recall_tolerance && recall > 0.0`, so a merge whose verified recall was exactly 0.0 (a total collapse) evaluated the condition as false and committed anyway. verify_merge_recall returns 1.0 (not 0.0) for the "too few vectors to measure" cases, so 0.0 is only ever a real measured zero-recall merge -- the `&& recall > 0.0` clause was a bug. Removed it so a 0.0-recall merge now aborts like any other sub-tolerance merge. 13 compaction tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 21 (Batch D). author: Tin Dang
…uracy Two related vector-index correctness gaps from the production-hardening audit (Batch D). #20 Multi-vector-field deletion resurrection: tombstone_key_in_index only walked the default field's SegmentHolder (idx.segments); secondary VECTOR fields live in a separate idx.field_segments map that it never iterated. DEL/UNLINK/HDEL therefore left the vector alive in every non-default field, and a subsequent `FT.SEARCH idx '@field2:[...]'` resurrected the deleted document -- under a synthetic `vec:<id>` key, since the shared key_hash_to_key map was cleared unconditionally. Extracted a `tombstone_key_in_holder` helper and now apply it to the default field AND each field_segments entry. #28 FT.INFO num_docs miscount: - MutableSegment::len() returns entries.len() including tombstoned slots (mark_deleted_* only flips delete_lsn in place until compaction), so num_docs inflated by up to compact_threshold under DEL/HSET churn. Added MutableSegment::live_len() (excludes tombstones) and use it for num_docs / mutable_vectors in ft_info.rs (top-level + both per-field blocks). - DiskANN cold-tier segments (experimental, gated by MOON_VEC_COLD_TIER) were never summed even though cold docs ARE returned by FT.SEARCH. Added a `snap.cold` loop backed by a new DiskAnnSegment::num_docs(). Red/green tests: - vector::segment::mutable::tests::test_live_len_excludes_tombstoned - vector::store::bg_compact_tests::test_del_tombstones_secondary_vector_field Both runtimes compile + clippy -D warnings clean; 65 vector tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 20, 28 (Batch D). author: Tin Dang
analyze_txn_locality derived per-command keys only from the command metadata key specs, which for SORT/GEORADIUS/GEORADIUSBYMEMBER declare just the SOURCE key (first_key==last_key==1). The optional STORE/STOREDIST destination is positional (the arg immediately after the token), so it was never considered when classifying a queued MULTI body. Consequence in a multi-shard deployment: `MULTI; SORT src STORE dst; EXEC` with src and dst on different shards was classified SingleShard(owner-of-src) and the whole body routed to that one shard, so `dst` was written to the WRONG shard's dataset -- a later normally-routed `GET dst` (which hashes by dst) missed, silently losing the write. EXEC still reported success. Fix: add a `store_clause_dest` helper that scans SORT/GEORADIUS args for a STORE/STOREDIST token and returns the following key; feed that key through the same owner-comparison in analyze_txn_locality so a cross-shard STORE destination forces CrossShard -> CROSSSLOT rejection instead of a silent misroute. Co-located (hash-tagged) src+dst still classify SingleShard. Residual (documented): the equivalent non-MULTI single-key routing path for SORT ... STORE is general command routing, out of scope for this transaction-locality fix. Red/green tests (server::conn::shared::txn_locality_tests): - sort_store_dest_on_other_shard_is_cross_shard - sort_store_dest_same_shard_stays_single_shard - georadius_storedist_dest_participates_in_locality Both runtimes compile + clippy -D warnings clean; 8/8 locality tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 15 (Batch D). author: Tin Dang
…oris Two connection-plane resource-exhaustion gaps from the production-hardening audit (Batch D), both the same class: an unbounded read after a resource (fd / maxclients slot / task) is already committed. #17 TLS handshake timeout: try_accept_connection() consumes a global maxclients slot BEFORE acceptor.accept(tcp_stream).await, which then ran with no deadline on both the monoio (spawn_monoio_connection) and tokio (spawn_tokio_connection) TLS branches. A peer that completes the TCP 3-way handshake on the TLS port and then withholds the ClientHello parks the task, fd, and maxclients slot indefinitely at zero CPU -- a slow-loris that can exhaust the connection limit for legitimate TLS clients. Both accepts are now wrapped in a 10s TLS_HANDSHAKE_TIMEOUT (monoio::time::timeout / tokio::time::timeout); on expiry the slot is released via record_connection_closed() and the stream dropped. #10 Cluster-bus gossip body read timeout: handle_cluster_peer guarded only the 4-byte length-prefix read with a shutdown-cancel select; the subsequent body read_exact had neither a timeout nor a shutdown arm. Any client reachable on the bus port could send a valid length header and never send the body, blocking the spawned task forever -- immune to graceful shutdown, socket never reclaimed; a handful exhaust the bus accept capacity. Both runtimes now read the body under a GOSSIP_BODY_READ_TIMEOUT (10s) + shutdown-cancel select and bail on expiry. Both runtimes compile + clippy -D warnings clean. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md findings 10, 17 (Batch D). author: Tin Dang
run_remote / execute_txn_on_owner and every scatter-gather leg in the
coordinator (coordinate_mget/mset/del/exists/bitop/copy, flush broadcast,
MULTI-on-owner) pushed a ShardMessage via the backoff-retried spsc_send and
then awaited `reply_rx.recv().await` with NO timeout.
spsc_send's retry budget only bounds getting the message INTO the target
shard's ring buffer. Once the push succeeds, if the target shard thread then
stalls while executing the command -- a synchronous fsync on a wedged disk
during a snapshot/AOF op, an uninterruptible D-state I/O stall, or a dead
shard -- the awaiting connection task parks on recv() forever: no response,
no self-wake, no cancel. An ordinary MGET/DEL/BITOP whose keys span shards
hangs the client indefinitely and holds its task/connection resources. This
is the exact failure the file's own comments flag for the push side but left
unaddressed on the reply-wait side.
Fix: add recv_reply_bounded(), which races the receiver against a
runtime-agnostic sleep (race2 + cfg-gated tokio/monoio timer) with a 30s
XSHARD_REPLY_TIMEOUT ceiling -- far above any legitimate cross-shard latency
incl. group-commit fsync, so it only fires for a genuinely wedged shard. On
timeout it returns Err(RecvError), identical to a closed channel, so all 11
call sites' existing error handling ("cross-shard reply channel closed" /
.ok() / ignore) applies unchanged -- the only behavior change is that none of
them can hang forever. race2 polls the recv arm first, so a ready reply
always wins the tie and the timer is dropped un-fired (cheap on both
runtimes).
Both runtimes compile + clippy -D warnings clean; dispatch/coordinator unit
tests pass.
Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 11 (Batch D).
author: Tin Dang
…ncing
replay_aof (the default single-file appendonly.aof startup path) handled
ANY mid-stream parse error by discarding one byte, scanning forward to the
next RESP '*' marker, and resuming dispatch of whatever followed as live
commands. appendonly.aof is raw unframed RESP with no per-record length or
CRC, and a 0x2A ('*') byte appears freely inside binary value blobs -- so
the resync can land mid-value and execute misaligned garbage as SET/DEL/etc.
against the recovering keyspace: silent data corruption with only a WARN.
The codebase's own per-shard sibling (aof_manifest/shard_replay) already
rejects this exact pattern.
Fix: clean tail-truncation is still handled gracefully (the Ok(None) arm).
On a genuine mid-stream hard parse error the default now STOPS replay,
keeping only the valid prefix already applied in-place, and never dispatches
anything past the corruption point. It logs a loud error! with the byte
offset + command count and points at redis-check-aof. (Returning Err was
rejected: the caller starts with an EMPTY database on Err, which would throw
away the valid prefix too -- stopping preserves it.) Operators who want the
old best-effort skip-and-resync can opt in via MOON_AOF_BEST_EFFORT_RESYNC=1
(loud warnings; may misapply corrupted data).
Red/green test: test_aof_replay_stops_at_midstream_corruption_by_default --
a valid SET, an invalid `*Z\r\n` frame, then a valid SET; asserts only the
pre-corruption command is applied (before the fix the post-corruption SET
was resynced and executed).
Both runtimes compile + clippy -D warnings clean; 7 AOF replay tests pass
(incl. the pre-existing tail-truncation test, unchanged).
Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 12 (Batch D).
author: Tin Dang
…looding handle_checkpoint_tick's Finalize arm returns false (without complete()) if wal.wait_durable / manifest.commit / graph_save / control.write fails, so CheckpointState stays Finalizing and last_checkpoint_lsn never advances. Because the state machine is driven from the 1ms persistence tick, the next tick re-enters Finalize from step 1 and re-appends a WAL Checkpoint record EVERY millisecond with no backoff. Under a sustained failure (slow/degraded disk causing repeated wait_durable timeouts, or a persistent graph_save failure) this floods the WAL, and since recycle_segments_before(redo_lsn) never fires while stuck, WAL disk usage grows fastest during the very disk-pressure incident that is the likely root cause -- compounding instead of backing off. Fix: CheckpointManager gains a finalize backoff (finalize_retry_at + finalize_attempts). note_finalize_failed(now) arms an exponential delay (50ms << (attempts-1), capped at 5s) on each failed attempt; the tick calls finalize_ready(now) BEFORE any finalize I/O (including the step-1 WAL append) and returns early if still gated, so retries happen on a bounded schedule rather than every tick. complete() clears the backoff so the next checkpoint starts clean. advance_tick()'s signature is unchanged (all existing state tests pass). Red/green tests (persistence::checkpoint::tests): - test_finalize_backoff_gates_retries (50ms→100ms doubling; complete() clears) - test_finalize_backoff_caps_at_5s (40 failures saturate, no shift overflow) Both runtimes compile + clippy -D warnings clean; 13 checkpoint tests pass. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 13 (Batch D). author: Tin Dang
Under the monoio runtime every shard binds bind:port with a SO_REUSEPORT socket (create_reuseport_socket, see event_loop.rs), and main.rs documents the central listener as coexisting with them via REUSEPORT. But run_sharded's monoio path bound the SAME port with a plain monoio::net::TcpListener::bind -- unlike the tokio sibling a few lines up, which deliberately uses create_reuseport_socket. Linux requires every socket sharing a REUSEPORT port, including the first one bound, to set the option. Depending on OS thread-scheduling at startup: (a) a shard's REUSEPORT bind wins first -> the central plain bind fails EADDRINUSE, `?` propagates out of run_sharded, and main.rs only logs the error -- the TLS listener setup and the conn_rx MPSC fallback silently never exist; or (b) the central plain bind wins first -> every per-shard REUSEPORT bind fails and all traffic funnels through the single central accept loop. Both outcomes are silent (log-only) and nondeterministic run-to-run. Fix: bind the monoio central listener via create_reuseport_socket + monoio TcpListener::from_std (the exact pattern the per-shard monoio listeners and the tokio central listener already use), with a plain-bind fallback on non-unix or an unparseable addr. The TLS central listener is left as a plain bind: shards only REUSEPORT-bind the plain port, so the TLS port is central-only with no socket to race (matches the tokio TLS path). Both runtimes compile + clippy -D warnings clean. Refs: tmp/PRODUCTION-HARDENING-AUDIT.md finding 16 (Batch D). 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 bounded networking waits, checkpoint finalize backoff, bounded cross-shard reply handling, transaction locality fixes, vector correctness updates, and allocation-size guards across several commands. ChangesNetworking and Cluster Hardening
Persistence Durability Fixes
Cross-Shard Coordination and Routing
Vector Search and Storage Corrections
Command-Level Allocation DoS Guards
Release Notes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Replace the PR #TBD placeholders on this branch's [Unreleased] entries (Batches A-D) with the assigned PR number. Prior-work entries below the batch block are left untouched. author: Tin Dang
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/vector/segment/compaction.rs (1)
1505-1509: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePre-existing: file exceeds 1500-line guideline.
This file has at least 1726 lines, exceeding the 1500-line limit in the coding guidelines. The 4-line comment addition in this PR doesn't cause the violation, but since the file is being modified, consider splitting
merge_graph_unionandverify_merge_recallinto a submodule (e.g.,compaction/merge.rs) in a follow-up.As per coding guidelines: "No single
.rsfile should exceed 1500 lines; split larger modules into submodules."🤖 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/vector/segment/compaction.rs` around lines 1505 - 1509, The issue is that compaction.rs already exceeds the 1500-line guideline, so this change should be used to start moving the large merge-related logic out of the file. In a follow-up, split the merge path helpers such as merge_graph_union and verify_merge_recall into a submodule like compaction/merge.rs, then update the compaction module to call into that submodule so the main file stays under the size limit.Source: Coding guidelines
src/command/vector_search/ft_info.rs (1)
159-174: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant duplicate
live_len()scan per field.For both the default field (Line 160 and Line 174) and each secondary field (Line 177 and Line 191),
s.mutable.live_len()is invoked twice to compute the same value — once to seeddocs, once again for the returned tuple.live_len()takes a read lock and does a fullO(n)linear scan overentries, so this doubles the locked scan cost per field and opens a small window where a concurrent delete between the two calls could make themutable_vectorsfield disagree with thenum_docstotal. Cache the value once instead.♻️ Proposed fix: cache live_len() once per field
- let mut docs = s.mutable.live_len(); + let mutable_live = s.mutable.live_len(); + let mut docs = mutable_live; let imm_count = s.immutable.len(); for imm in s.immutable.iter() { docs += imm.live_count() as usize; } for warm in s.warm.iter() { docs += warm.live_count() as usize; } for stub in s.unloaded.iter() { docs += stub.live_count() as usize; } for cold in s.cold.iter() { docs += cold.num_docs(); } - (docs, s.mutable.live_len(), imm_count) + (docs, mutable_live, imm_count)Apply the same pattern to the
else if let Some(fs) = ...branch.Also applies to: 175-191
🤖 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/vector_search/ft_info.rs` around lines 159 - 174, The `s.mutable.live_len()` value is being computed twice in `ft_info.rs` for the default field and again in the secondary-field branch, which duplicates the scan and can produce inconsistent counts. Update the logic around the `docs` calculation to call `live_len()` once per field, store it in a local variable, use that cached value to seed `docs`, and return the same cached value in the tuple. Apply the same fix in both the main field path and the `else if let Some(fs)` path, keeping the existing `live_count()` and `num_docs()` accumulation unchanged.src/command/string/string_write.rs (1)
518-526: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
saturating_addfor consistency with the BITFIELD guard.The
combined_lencomputation uses plain+, while the BITFIELD guard instring_bit.rsusessaturating_add. On 64-bit targets overflow is practically impossible here (existing strings are already bounded to 512MB), but usingsaturating_addwould be consistent and defensive.♻️ Optional consistency refactor
- let combined_len = existing_data.as_ref().map_or(0, |v| v.len()) + append_val.len(); + let combined_len = existing_data + .as_ref() + .map_or(0, |v| v.len()) + .saturating_add(append_val.len());🤖 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/string/string_write.rs` around lines 518 - 526, The APPEND size check in string_write.rs uses plain addition for `combined_len`, unlike the defensive pattern used by the BITFIELD guard in `string_bit.rs`. Update the `combined_len` calculation in the APPEND path to use `saturating_add` so the limit check remains consistent and overflow-safe, and keep the existing 512MB error return in place.
🤖 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/cluster/gossip.rs`:
- Around line 582-598: The PONG handling in the gossip receive path is using
single read calls that can return partial data, so fragmented frames may be
dropped before `deserialize_gossip` runs. Update the read logic in the PONG
frame branch to use an exact-read loop or `read_exact` for both the 4-byte
length header and the payload, matching the pattern used in `bus.rs`, so the
full frame is always assembled before decoding and merging into state.
In `@src/command/set/mod.rs`:
- Around line 154-172: The test is asserting the capped SRANDMEMBER behavior
instead of the intended exact count behavior for large negative COUNT values.
Update test_srandmember_huge_negative_count_capped_no_abort to match the final
contract for srandmember and srandmember_readonly, using the relevant helper
methods in src/command/set/mod.rs; if exact |count| semantics are restored,
change the assertions to expect the requested count rather than the
members.len()*10 cap.
In `@src/command/set/set_read.rs`:
- Around line 298-306: `set_read.rs` is capping negative `SRANDMEMBER` requests
in the duplicate-allowed path, which can silently return fewer items than
requested. Update the `srandmember`/`srandmember_readonly` logic in the
`Frame::Array` branch to preserve the full requested `count.unsigned_abs()` for
`SRANDMEMBER key -N`, while keeping a separate allocation guard if needed so the
result length still matches the request exactly.
In `@src/persistence/aof/mod.rs`:
- Around line 629-635: The AOF corruption log in the replay path reports a
relative offset from resp_data, so when an RDB preamble exists it points
operators to the wrong location. Update the offset calculation in the affected
AOF replay logging paths around the corruption handling in
src/persistence/aof/mod.rs to include resp_start when present, and use that
absolute byte offset in the messages emitted for both the primary corruption log
and the related follow-up guidance.
- Around line 739-760: This test currently depends on the ambient
MOON_AOF_BEST_EFFORT_RESYNC environment, so it can take the resync path and
incorrectly replay the trailing command. Update
test_aof_replay_stops_at_midstream_corruption_by_default to avoid reading global
env state by exercising the replay/parsing path through an internal helper that
takes the resync mode explicitly, or by serializing access with a process-wide
test lock around the env. Use replay_aof and the underlying AOF replay helper in
src/persistence/aof/mod.rs to keep the test deterministic regardless of external
shell or CI settings.
In `@src/shard/coordinator.rs`:
- Around line 199-214: The bounded receive helper is currently private, so the
remaining cross-shard ack paths still use unbounded reply waits. Make
recv_reply_bounded reusable by exposing it from shard::coordinator or moving it
into a shared shard/runtime utility, then update the TXN.COMMIT materialization
ack code paths to call recv_reply_bounded(reply_rx).await instead of
reply_rx.recv().await so all cross-shard replies use the same timeout-bounded
behavior.
- Line 1256: The remote MSET completion path in coordinator logic is ignoring
`recv_reply_bounded(reply_rx)` failures, which can let a timed-out or closed leg
still return OK. Update the MSET handling in the shard coordinator flow to check
the result of `recv_reply_bounded` instead of discarding it, and propagate a
partial/timeout failure when any remote write leg does not confirm. Make sure
the final response is only OK when all distributed write legs complete
successfully.
In `@src/shard/persistence_tick.rs`:
- Around line 1058-1060: The finalize backoff is being armed from an outdated
timestamp captured before blocking work in the persistence tick flow. Update the
finalize failure path in persistence_tick.rs so the backoff start time is taken
at the moment the failure is detected, including the branches around
finalize_ready, wait_durable, manifest.commit, graph_save, and control.write,
rather than reusing the earlier now value. Use the finalize/checkpoint logic in
persistence_tick to find the affected retry handling and ensure the retry
deadline is based on the actual failure time.
---
Nitpick comments:
In `@src/command/string/string_write.rs`:
- Around line 518-526: The APPEND size check in string_write.rs uses plain
addition for `combined_len`, unlike the defensive pattern used by the BITFIELD
guard in `string_bit.rs`. Update the `combined_len` calculation in the APPEND
path to use `saturating_add` so the limit check remains consistent and
overflow-safe, and keep the existing 512MB error return in place.
In `@src/command/vector_search/ft_info.rs`:
- Around line 159-174: The `s.mutable.live_len()` value is being computed twice
in `ft_info.rs` for the default field and again in the secondary-field branch,
which duplicates the scan and can produce inconsistent counts. Update the logic
around the `docs` calculation to call `live_len()` once per field, store it in a
local variable, use that cached value to seed `docs`, and return the same cached
value in the tuple. Apply the same fix in both the main field path and the `else
if let Some(fs)` path, keeping the existing `live_count()` and `num_docs()`
accumulation unchanged.
In `@src/vector/segment/compaction.rs`:
- Around line 1505-1509: The issue is that compaction.rs already exceeds the
1500-line guideline, so this change should be used to start moving the large
merge-related logic out of the file. In a follow-up, split the merge path
helpers such as merge_graph_union and verify_merge_recall into a submodule like
compaction/merge.rs, then update the compaction module to call into that
submodule so the main file stays under the size limit.
🪄 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: 92857a16-5975-4511-bd8a-10432634f78b
📒 Files selected for processing (31)
CHANGELOG.mdsrc/cluster/bus.rssrc/cluster/gossip.rssrc/command/hash/hash_read.rssrc/command/hash/mod.rssrc/command/set/mod.rssrc/command/set/set_read.rssrc/command/sorted_set/mod.rssrc/command/sorted_set/sorted_set_write.rssrc/command/string/string_bit.rssrc/command/string/string_write.rssrc/command/vector_search/ft_info.rssrc/command/vector_search/ft_text_search.rssrc/config.rssrc/persistence/aof/mod.rssrc/persistence/checkpoint.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/shared.rssrc/server/listener.rssrc/shard/conn_accept.rssrc/shard/coordinator.rssrc/shard/event_loop.rssrc/shard/persistence_tick.rssrc/storage/tiered/cold_tier.rssrc/vector/diskann/segment.rssrc/vector/persistence/mod.rssrc/vector/persistence/wal_record.rssrc/vector/segment/compaction.rssrc/vector/segment/mutable.rssrc/vector/store.rs
💤 Files with no reviewable changes (2)
- src/vector/persistence/wal_record.rs
- src/vector/persistence/mod.rs
…g sweep Eight review findings on PR #250, all verified against the code and fixed: - RANDMEMBER family (SRANDMEMBER/HRANDFIELD/ZRANDMEMBER) negative COUNT: the relative len()*10 DoS cap silently truncated legitimate requests on small collections (e.g. -50 on a 3-member set returned 30), breaking Redis's exact-|COUNT|-with-duplicates contract. Replaced at all six sites (incl. the pre-existing HRANDFIELD/ZRANDMEMBER caps) with an absolute 2^20 cap that returns "ERR COUNT is out of range" beyond it — exact count within the cap, loud refusal past it, never a short reply. Red/green tests updated + i64::MIN guard test; verified end-to-end against a live server for all three commands. - Gossip PONG probe (monoio): single read() calls dropped fragmented but valid PONG frames, leaving pong_recv_ms stale and pushing healthy peers toward PFAIL. Now uses the bus listener's exact-read loop; both runtimes additionally cap the peer-controlled PONG length at the bus's 64KB frame bound before sizing the buffer (tokio probe allocated an unchecked vec![0u8; pong_len]). - Cross-shard MSET: a timed-out/closed/errored remote MultiExecute ack was discarded and the client still received OK. All legs are drained (each was already dispatched; the local leg still persists), but any failed leg now surfaces as an error instead of a false ack. - recv_reply_bounded extended to the handler plane: made pub(crate) and applied to the 11 remaining direct cross-shard reply awaits (WS.DROP cleanup, routed MQ, routed graph, TXN.COMMIT MQ materialization acks, txn-abort remote rollback; both handler_monoio and handler_sharded). Owner-side execution on these paths is synchronous on the shard thread (no blocking-wait commands route through them), so the 30s bound cannot cut off a legitimate long wait. - Checkpoint finalize backoff: armed from a timestamp captured before the finalize I/O; when wait_durable/manifest.commit/graph_save/control write exceeded the backoff window the retry deadline was already in the past (instant per-tick retry). Failure branches now re-stamp Instant::now(). - AOF replay corruption/truncation logs now report file-absolute offsets (include the RDB preamble length) so operator repair guidance points at the real location. - AOF midstream-corruption test no longer depends on ambient MOON_AOF_BEST_EFFORT_RESYNC: replay_aof delegates to a replay_aof_with_resync(mode) helper, the test pins mode=false, and a new companion test pins the opt-in resync branch (mode=true). Validation: cargo fmt --check; clippy -D warnings on default (monoio) and runtime-tokio,jemalloc features; full lib suite green on both runtimes (4076 monoio / 3282 tokio); live-server smoke of the RANDMEMBER contract (exact counts, over-cap error, i64::MIN, WITHVALUES/ WITHSCORES doubling). Refs: PR #250 CodeRabbit review 2026-07-09 author: Tin Dang
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/conn/handler_monoio/write.rs (1)
155-172: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWS.DROP cleanup ack discarded without logging — inconsistent with
handler_sharded/write.rs.
handler_sharded/write.rs(Lines 162-176) logs the deleted-key count on success and a warning on channel closure/timeout for this same cleanup hop. Here the bounded-recv result is dropped entirely, so a stuck/backpressured cleanup on a foreign shard leaves no trace.♻️ Suggested fix
- let _ = crate::shard::coordinator::recv_reply_bounded(reply_rx) - .await; + match crate::shard::coordinator::recv_reply_bounded(reply_rx) + .await + { + Ok(count) => tracing::debug!( + "WS.DROP cleanup: deleted {} keys on shard {}", + count, + owner + ), + Err(_) => tracing::warn!( + "WS.DROP cleanup: reply channel closed for shard {}", + owner + ), + }🤖 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_monoio/write.rs` around lines 155 - 172, The WsDropCleanup ack handling in write() drops the recv_reply_bounded result silently, unlike handler_sharded::write which logs success and warns on timeout/closure. Update the foreign-shard cleanup path to inspect the reply from recv_reply_bounded, log the deleted-key count on success, and emit a warning when the oneshot/channel is closed or times out so stalled cleanup is observable.src/cluster/bus.rs (1)
105-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLength-prefix read still has no timeout — same DoS class the body-read fix targets.
The body read now bounds against a stalling peer, but the 4-byte length-prefix read at Lines 106-113 only races against
shutdown.cancelled(). A client that opens the TCP connection and never sends the 4-byte header will pin this task/socket forever (until server shutdown), exactly the "exhaust the bus listener's accept capacity" scenario the comment above (Lines 33-37) describes for the body — just shifted one step earlier in the protocol.🛡️ Proposed fix
// Read 4-byte length prefix let mut len_buf = [0u8; 4]; tokio::select! { result = stream.read_exact(&mut len_buf) => { result?; } _ = shutdown.cancelled() => return Ok(()), + _ = tokio::time::sleep(GOSSIP_BODY_READ_TIMEOUT) => { + anyhow::bail!( + "gossip header read from {} timed out after {}s", + peer_addr, + GOSSIP_BODY_READ_TIMEOUT.as_secs() + ); + } }🤖 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/cluster/bus.rs` around lines 105 - 117, The length-prefix read in the bus receive loop still lacks a timeout and can be stalled indefinitely by a peer that never sends the 4-byte header. Add the same timeout protection used for the body read around the read_exact call in the bus message loop, so the task exits or closes the connection if the prefix is not received in time. Use the existing loop in bus::... and the MAX_GOSSIP_FRAME_LEN validation path as the anchor for the fix, keeping shutdown.cancelled() as an additional exit path.
🤖 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/cluster/bus.rs`:
- Line 209: The monoio header read path in monoio_read_exact currently only
waits on shutdown.cancelled(), so a peer that never sends the 4-byte length
header can block indefinitely. Update the bus read flow around monoio_read_exact
and its caller that reads the header to race the read against a timeout, and
propagate a timeout error if the deadline is exceeded; keep the shutdown
cancellation handling intact so both cancellation and timeout are covered.
In `@src/server/conn/handler_monoio/txn.rs`:
- Around line 180-184: The bounded reply wait in the monoio transaction handler
is discarding timeout/closed-channel results, so mirror the sharded path’s
observability. In `handler_monoio::txn`, around the
`recv_reply_bounded(reply_rx).await` call, capture the result instead of
assigning it to `_` and emit a `tracing::warn!` when the ack is closed or times
out, using the same context style as `handler_sharded/txn.rs` so operators can
see backpressure or dead-shard signals.
---
Outside diff comments:
In `@src/cluster/bus.rs`:
- Around line 105-117: The length-prefix read in the bus receive loop still
lacks a timeout and can be stalled indefinitely by a peer that never sends the
4-byte header. Add the same timeout protection used for the body read around the
read_exact call in the bus message loop, so the task exits or closes the
connection if the prefix is not received in time. Use the existing loop in
bus::... and the MAX_GOSSIP_FRAME_LEN validation path as the anchor for the fix,
keeping shutdown.cancelled() as an additional exit path.
In `@src/server/conn/handler_monoio/write.rs`:
- Around line 155-172: The WsDropCleanup ack handling in write() drops the
recv_reply_bounded result silently, unlike handler_sharded::write which logs
success and warns on timeout/closure. Update the foreign-shard cleanup path to
inspect the reply from recv_reply_bounded, log the deleted-key count on success,
and emit a warning when the oneshot/channel is closed or times out so stalled
cleanup is observable.
🪄 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: ef81d7ca-565f-46b4-9a9d-761cfb11dda5
📒 Files selected for processing (16)
CHANGELOG.mdsrc/cluster/bus.rssrc/cluster/gossip.rssrc/command/hash/hash_read.rssrc/command/mod.rssrc/command/set/mod.rssrc/command/set/set_read.rssrc/command/sorted_set/sorted_set_read.rssrc/persistence/aof/mod.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_monoio/write.rssrc/server/conn/handler_sharded/txn.rssrc/server/conn/handler_sharded/write.rssrc/shard/coordinator.rssrc/shard/persistence_tick.rssrc/transaction/abort.rs
✅ Files skipped from review due to trivial changes (2)
- src/command/mod.rs
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/shard/persistence_tick.rs
- src/cluster/gossip.rs
- src/persistence/aof/mod.rs
| /// Monoio has no `read_exact` — we loop on `stream.read()` accumulating bytes. | ||
| #[cfg(feature = "runtime-monoio")] | ||
| async fn monoio_read_exact( | ||
| pub(crate) async fn monoio_read_exact( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Same missing-timeout gap on the monoio header read.
Mirrors the tokio-path finding above: monoio_read_exact(&mut stream, 4) at Lines 289-294 only races shutdown.cancelled(), so a peer that never sends the length header stalls this task/socket indefinitely on this runtime too.
🛡️ Proposed fix
let len_data = monoio::select! {
result = monoio_read_exact(&mut stream, 4) => {
result.map_err(|e| anyhow::anyhow!(e))?
}
_ = shutdown.cancelled() => return Ok(()),
+ _ = monoio::time::sleep(GOSSIP_BODY_READ_TIMEOUT) => {
+ anyhow::bail!(
+ "gossip header read from {} timed out after {}s",
+ peer_addr,
+ GOSSIP_BODY_READ_TIMEOUT.as_secs()
+ );
+ }
};Also applies to: 287-294
🤖 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/cluster/bus.rs` at line 209, The monoio header read path in
monoio_read_exact currently only waits on shutdown.cancelled(), so a peer that
never sends the 4-byte length header can block indefinitely. Update the bus read
flow around monoio_read_exact and its caller that reads the header to race the
read against a timeout, and propagate a timeout error if the deadline is
exceeded; keep the shutdown cancellation handling intact so both cancellation
and timeout are covered.
Comment on lines
+180
to
184
| // Await the ack before replying OK to the client | ||
| // (bounded: a stuck owner shard must not hang the | ||
| // connection forever). | ||
| let _ = crate::shard::coordinator::recv_reply_bounded(reply_rx).await; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bounded-recv failure silently discarded — no parity with handler_sharded/txn.rs.
The equivalent MQ materialize ack wait in handler_sharded/txn.rs (Line 196-205) logs tracing::warn! on a closed/timed-out reply so operators can see backpressure/dead-shard signals. Here the result is fully discarded with let _ =, losing that visibility on the monoio runtime path.
♻️ Suggested fix
- let _ = crate::shard::coordinator::recv_reply_bounded(reply_rx).await;
+ if crate::shard::coordinator::recv_reply_bounded(reply_rx)
+ .await
+ .is_err()
+ {
+ tracing::warn!(
+ "TXN.COMMIT MQ materialize: reply channel closed for shard {}",
+ owner
+ );
+ }📝 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
| // Await the ack before replying OK to the client | |
| // (bounded: a stuck owner shard must not hang the | |
| // connection forever). | |
| let _ = crate::shard::coordinator::recv_reply_bounded(reply_rx).await; | |
| } | |
| // Await the ack before replying OK to the client | |
| // (bounded: a stuck owner shard must not hang the | |
| // connection forever). | |
| if crate::shard::coordinator::recv_reply_bounded(reply_rx) | |
| .await | |
| .is_err() | |
| { | |
| tracing::warn!( | |
| "TXN.COMMIT MQ materialize: reply channel closed for shard {}", | |
| owner | |
| ); | |
| } | |
| } |
🤖 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_monoio/txn.rs` around lines 180 - 184, The bounded
reply wait in the monoio transaction handler is discarding
timeout/closed-channel results, so mirror the sharded path’s observability. In
`handler_monoio::txn`, around the `recv_reply_bounded(reply_rx).await` call,
capture the result instead of assigning it to `_` and emit a `tracing::warn!`
when the ack is closed or times out, using the same context style as
`handler_sharded/txn.rs` so operators can see backpressure or dead-shard
signals.
pilotspacex-byte
pushed a commit
that referenced
this pull request
Jul 9, 2026
Replace the PR #TBD placeholders on this branch's [Unreleased] entries (Batches A-D) with the assigned PR number. Prior-work entries below the batch block are left untouched. author: Tin Dang
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
2 participants
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.
Production-Hardening Audit — Batches A–D
Implements the adversarially-verified production-hardening audit (37-agent workflow, 28 confirmed findings). Every fix follows red→green TDD; both runtimes compile,
clippy -D warningsclean,fmt --checkclean.Gate
runtime-tokio,jemalloc).Batch A — unbounded client-controlled allocation (DoS → allocator abort → process crash)
HSCAN COUNT, SRANDMEMBER negative COUNT, BITFIELD bit-offset, FT text HIGHLIGHT/SUMMARIZE FIELDS, plus a finder-missed ZMPOP site — all clamped to the collection size before
Vec::with_capacity.Batch B — remaining criticals
send_asyncin the monoio accept task (avoids shard deadlock).Batch C — vector storage decisions
MOON_VEC_COLD_TIER(default no-op).wal_record.rs, double-apply footgun, zero live refs).DiskAnnSegment::newpanic-on-IO →io::Result.Batch D — timeouts / durability / txn correctness
recall==0.0total collapse (removed the&& recall > 0.0exemption)field_segments), not just the default fieldnum_docsuseslive_len()(excludes tombstoned mutable entries) + counts cold-tier segmentsSORT/GEORADIUS ... STORE dstcross-shard destination → CROSSSLOT instead of a silent misrouterecv_reply_bounded, 11 sites) — a wedged shard can no longer hang a client foreverMOON_AOF_BEST_EFFORT_RESYNC=1opt-in)Deferred to their own PRs
--disk-offload+engine_offload_idle_secs; proper fix is an off-loop background reload pool (mirroring the graph-tiersearch_pool) + oneshot reply, needing disk-offload integration testing on the Linux VM.std::sync::RwLock→parking_lot(65 call sites) — its own focused compiler-verified commit.Acknowledged, no change (LOW, deliberate design)
monoio::select!); low-confidence finding.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
COUNT/offsets are now safely bounded (e.g., HSCAN, HRANDFIELD, SRANDMEMBER, ZMPOP, ZRANDMEMBER, BITFIELD) andAPPENDenforces the 512MB limit.SORT ... STORE, vector tombstoning across fields/tier,FT.INFO num_docsexcludes tombstones and includes cold tier docs, and GraphUnion merge recall gating.Changed
FT.*commands are rejected insideMULTI/EXEC.