fix(storage): in-flight spills are a storage plane, not a bookkeeping note (#459) - #465
Merged
Merged
Conversation
… note (#459) A key whose disk-offload spill was queued but not yet completed existed in no plane the database consults. `evict_one_async_spill` freed the hot entry the moment the `SpillRequest` was sent, and `cold_index` only learned about the key when the completion landed; `spill_inflight` held nothing but a request id and was read solely by the completion path. The eviction code documented the window and accepted it — "accept a brief read-miss ... AOF incr log is the durability backstop". The AOF does backstop durability. It does not backstop visibility, and nothing considered a write arriving inside the window. Measured on origin/main @4c9bd2c5, 400 x 4KiB keys against a 512KiB cap (the config of the failing test in #459): * DBSIZE answered 124 for 400 acked keys, then climbed to 400 on its own with no further writes — this is the `live 373 / recovered 400` that filed the issue; * GET and EXISTS denied live keys: `GET k0` -> nil, `EXISTS k0` -> 0, then 250ms later `GET k0` -> 4096 bytes, no intervening write; * 277 of 400 DELs answered :0 and were then reversed. The completion path inserted into `cold_index` unconditionally, so the resurrections reached the manifest and survived restart. A client that deleted data got it back. `spill_inflight` now carries the payload and is treated as the third plane it always was. The payload is the same refcounted `Bytes` the queued request already pins, so this is a refcount rather than a copy and adds no peak memory. * reads promote from it with no disk read, on all three dispatch paths — `promote_cold_if_present` (collections, Lua, MULTI), the monoio async GET pre-warm, and the inline GET fast path, which was framing `$-1` inline for keys EXISTS reported as present; * EXISTS / DEL count it; DBSIZE and `logical_len` count it; KEYS and RANDOMKEY enumerate it; * DEL, an overwriting SET, and a promoting read each retire the record, which withdraws the completion's authorization to publish. That is what makes a delete inside the window final, and it also stops a stale payload being published as a cold shadow behind a newer hot value; * unpublished completions are counted as `spill_completion_superseded`. Servers that are not spilling pay one `is_empty()` load on the affected paths. Known remaining gap, documented at the call site: SCAN's ordered cursor does not merge the unordered in-flight plane, so it may skip a key for the milliseconds its spill is queued. That is within SCAN's contract (only keys present for the whole iteration are guaranteed); KEYS, whose contract is point-in-time, does include them. Tests: `tests/spill_inflight_visibility.rs` — one wire-level test per fault, each red on the pre-fix binary for the right reason (277 resurrections, retry-liars, DBSIZE 124->400) and green after. Retry-and-compare is the assertion shape throughout: a key that answers nil and then returns a value with no write in between cannot be explained by legitimate allkeys-lru eviction, which never brings a key back. Plus five unit tests on the plane's own semantics in `src/storage/db/mod.rs`. Verified: full suite 5110/5110 (monoio, the shipped runtime), clippy clean, fmt clean; the three independent Python repro harnesses that found the faults all report zero on the fixed binary; `tests/dbsize_offload_logical.rs` — the suite that filed #459 — passes. Fixes #459 author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe disk-offload path now retains pending spill payloads in memory. Reads and keyspace operations include these entries, while deletion, overwrite, and promotion invalidate stale spill completions before cold-index publication. ChangesIn-flight spill visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GETHandler
participant Database
participant SpillThread
participant ColdIndex
Client->>GETHandler: GET key
GETHandler->>Database: promote_inflight_if_present
Database-->>GETHandler: restore value to hot tier
GETHandler-->>Client: return value
SpillThread->>Database: validate pending spill record
Database-->>SpillThread: authorize or reject completion
SpillThread->>ColdIndex: publish authorized spill
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 4
🧹 Nitpick comments (2)
src/storage/eviction.rs (1)
951-959: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: build the key
Bytesonce and clone the handle.Line 916 already allocates
Bytes::copy_from_slice(key.as_bytes())forSpillRequest.key. Line 952 allocates a second copy of the same bytes. ABytesclone is a refcount bump, so one allocation serves both.♻️ Proposed refactor
let file_id = *next_file_id; *next_file_id += 1; + // One allocation of the key; the mark below clones the handle. + let key_bytes = Bytes::copy_from_slice(key.as_bytes()); + // Second handle on the SAME payload the request carries (refcount, // not a copy) so the in-flight plane below can answer reads from RAM // while the request is queued. let pending_bytes = value_bytes.clone(); let req = SpillRequest { - key: Bytes::copy_from_slice(key.as_bytes()), + key: key_bytes.clone(), db_index,db.spill_inflight_mark( - Bytes::copy_from_slice(key.as_bytes()), + key_bytes, crate::storage::db::PendingSpill {🤖 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/storage/eviction.rs` around lines 951 - 959, Reuse the existing Bytes allocation from SpillRequest.key in the spill_inflight_mark call instead of creating a second Bytes::copy_from_slice from key.as_bytes(). Clone the stored Bytes handle when passing it to spill_inflight_mark, while preserving the existing PendingSpill fields.src/storage/db/kv_ops.rs (1)
513-523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
remove_cold_only, or the in-flight retirement stays hidden behind the name.
remove_cold_onlynow retires the in-flight record in addition to the cold-index entry. The doc comment states this, but the name says the opposite. The in-flight retirement is load-bearing: it is what withdraws the completion's authorization to publish, so a reader auditing "what makes DEL final" must not skip this function because its name reads as cold-index-only.A name such as
retire_non_hot_planesorremove_cold_and_inflightmakes the second effect visible at every call site (removeandremove_counting_cold).♻️ Proposed rename
- /// Drops the cold copy AND any in-flight spill record. + /// Drops the cold copy AND any in-flight spill record. /// /// Retiring the in-flight record is the load-bearing half (`#459`): it is /// the completion's authorization to insert into `cold_index`, so /// without this a DEL issued during the spill window was undone when the /// spill landed — and committed to the manifest, so it survived restart. #[inline] - fn remove_cold_only(&mut self, key: &[u8]) -> bool { + fn remove_cold_and_inflight(&mut self, key: &[u8]) -> bool { self.spill_inflight_forget(key); self.cold_index.as_mut().is_some_and(|ci| ci.remove(key)) }Update the two call sites in
remove(line 484) andremove_counting_cold(line 505).🤖 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/storage/db/kv_ops.rs` around lines 513 - 523, Rename remove_cold_only to a name that explicitly reflects both cold-index removal and in-flight spill retirement, such as remove_cold_and_inflight, and update both call sites in remove and remove_counting_cold. Keep the existing behavior unchanged.
🤖 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 `@CHANGELOG.md`:
- Around line 9-41: Move the new spill-inflight changelog entry from the first
### Fixed section to the top of the existing ### Fixed section under
[Unreleased]. Leave only one ### Fixed heading, preserving the entry as the
first fixed item and the conventional section order with ### Added and ###
Security.
In `@src/storage/db/mod.rs`:
- Around line 213-230: Update spill_inflight_mark, spill_inflight_clear, and
spill_inflight_forget to charge the retained pending payload when it is marked
and credit it on every retirement path, including completion, DEL, overwriting
SET, and promoting reads. Ensure key and value memory use the existing
accounting helpers so used_memory remains truthful while the spill is in flight;
verify all call sites and avoid leaving any retirement path unbalanced.
In `@src/storage/tiered/spill_thread.rs`:
- Around line 183-191: Wire spill_completion_superseded_total() into the
existing INFO/metrics section that reports other spill fields, using
spill_completion_superseded as the output key. If the chosen INFO key differs,
update the corresponding CHANGELOG entry to match the final field name.
In `@tests/spill_inflight_visibility.rs`:
- Around line 286-290: Replace the fixed-sleep drain helper with a poll that
repeatedly reads DBSIZE through the provided mutable client and waits for the
value to stabilize across consecutive reads, bounded by the existing
readiness_deadline CI-aware timeout. Update drain and both call sites to accept
and pass &mut c, while preserving the strict exact-count assertions and
load_keys behavior.
---
Nitpick comments:
In `@src/storage/db/kv_ops.rs`:
- Around line 513-523: Rename remove_cold_only to a name that explicitly
reflects both cold-index removal and in-flight spill retirement, such as
remove_cold_and_inflight, and update both call sites in remove and
remove_counting_cold. Keep the existing behavior unchanged.
In `@src/storage/eviction.rs`:
- Around line 951-959: Reuse the existing Bytes allocation from SpillRequest.key
in the spill_inflight_mark call instead of creating a second
Bytes::copy_from_slice from key.as_bytes(). Clone the stored Bytes handle when
passing it to spill_inflight_mark, while preserving the existing PendingSpill
fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d4819e0-833e-4140-9e70-19af361b2b3c
📒 Files selected for processing (10)
CHANGELOG.mdsrc/server/conn/blocking.rssrc/server/conn/handler_monoio/mod.rssrc/shard/persistence_tick.rssrc/storage/db/accessors.rssrc/storage/db/kv_ops.rssrc/storage/db/mod.rssrc/storage/eviction.rssrc/storage/tiered/spill_thread.rstests/spill_inflight_visibility.rs
Comment on lines
+213
to
+230
| /// Costs nothing to hold: `value_bytes` is the very same refcounted | ||
| /// `Bytes` the queued `SpillRequest` already pins for the whole window, | ||
| /// so this is one refcount, not a copy. | ||
| /// | ||
| /// Empty for every server not actively spilling, which is what makes the | ||
| /// `is_empty()` fast bail on the hot paths below honest rather than a | ||
| /// hopeful guess. Size is bounded by the spill channel's capacity: | ||
| /// `evict_one_async_spill` marks only after a successful `try_send`, and | ||
| /// a full channel makes it bail. | ||
| /// | ||
| /// Retention note: a record is retired by its completion, by DEL, by an | ||
| /// overwriting SET, or by a promoting read. A LOST completion (see | ||
| /// `spill_completion_dropped_total` — the rare shutdown-with-full-channel | ||
| /// edge) therefore now strands a payload rather than the bare u64 it used | ||
| /// to, so the value stays charged to RAM until the key is next written or | ||
| /// deleted. Reads stay correct throughout; the cost is that the eviction | ||
| /// did not actually reclaim that key's memory. | ||
| spill_inflight: std::collections::HashMap<bytes::Bytes, PendingSpill>, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
The pending payload is resident RAM that used_memory no longer counts.
evict_one_async_spill calls db.remove(key) before spill_inflight_mark. remove_hot credits back entry_overhead(key, &entry), so the ledger drops by the full entry cost. spill_inflight_mark then retains value_bytes (and the key Bytes) without charging anything back.
For the length of the window the eviction loop believes it reclaimed memory it did not reclaim. evict_to_budget re-reads estimated_memory() after each victim, so within one tick it can select additional victims to close a gap that the in-flight plane is still holding. The retained bytes are bounded by the spill channel capacity, so this is bounded over-eviction rather than unbounded growth — but the doc block here states the cost only for the LOST-completion case, which understates it.
Two options:
- Charge the pending payload in
spill_inflight_markand credit it in every retirement path (spill_inflight_clear,spill_inflight_forget). This keepsused_memorytruthful and stops the over-eviction, at the cost of the eviction loop making less apparent progress per victim. - Keep the current accounting and state the trade-off here, so a future reader does not treat
used_memoryas exact during a spill burst.
Run the following script to confirm no other call site charges the pending payload:
#!/bin/bash
# Description: Check whether any path charges/credits memory for the in-flight spill payload.
set -euo pipefail
# All mark/clear/forget call sites, with context, to see adjacent memory accounting.
rg -n -C 6 'spill_inflight_(mark|clear|forget)\s*\(' --type=rust
# Memory accounting helpers, to confirm none of them is keyed to spill_inflight.
rg -n -C 3 '\b(charge_memory|credit_memory|adjust_memory|used_memory)\b' --type=rust -g '!**/tests/**'🤖 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/storage/db/mod.rs` around lines 213 - 230, Update spill_inflight_mark,
spill_inflight_clear, and spill_inflight_forget to charge the retained pending
payload when it is marked and credit it on every retirement path, including
completion, DEL, overwriting SET, and promoting reads. Ensure key and value
memory use the existing accounting helpers so used_memory remains truthful while
the spill is in flight; verify all call sites and avoid leaving any retirement
path unbalanced.
Comment on lines
+183
to
+191
| /// `maxmemory` is too small for the working set. Exposed as | ||
| /// `spill_completion_superseded` in INFO. | ||
| static SPILL_COMPLETION_SUPERSEDED: AtomicU64 = AtomicU64::new(0); | ||
|
|
||
| /// Cumulative superseded (unpublished) spill completions. For INFO / metrics. | ||
| #[inline] | ||
| pub fn spill_completion_superseded_total() -> u64 { | ||
| SPILL_COMPLETION_SUPERSEDED.load(Ordering::Relaxed) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the superseded-spill counter is read by an INFO/metrics emitter.
set -euo pipefail
# Callers of the new getter.
rg -n -C 5 'spill_completion_superseded_total' --type=rust
# The INFO field name itself.
rg -n -C 3 'spill_completion_superseded' --type=rust
# Compare with the established sibling counter to see the expected wiring shape.
rg -n -C 5 'spill_failed_reinserted_total' --type=rustRepository: pilotspace/moon
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files around storage/tiered and info/metrics =="
git ls-files | rg '(^src/.+\.rs$|CHANGELOG|README|.*info|.*metric|.*stat|.*stats|.*tiered|spill_thread)' | sed -n '1,200p'
echo
echo "== occurrences =="
rg -n 'spill_completion_superseded_total|spill_completion_superseded|spill_failed_reinserted_total|spill_failed_reinserted' --type=rust || trueRepository: pilotspace/moon
Length of output: 5597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --short
echo
echo "== changed files =="
git diff --name-only HEAD~1..HEAD 2>/dev/null || git diff --name-only --cached || true
echo
echo "== search all tracked text for superseded counter =="
rg -n 'spill_completion_superseded' . || trueRepository: pilotspace/moon
Length of output: 1015
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== spill_thread relevant lines =="
sed -n '160,215p' src/storage/tiered/spill_thread.rs
echo
echo "== persistence_tick call site =="
sed -n '700,755p' src/shard/persistence_tick.rs
echo
echo "== sibling counter definitions/usages =="
rg -n 'spill_failed_reinserted_total|spill_failed_reinserted|record_spill_failed_reinserted' --type=rust || true
echo
echo "== INFO field candidates around storage/tiered/spill/stat/reclamation =="
rg -n 'spill_|reinsert|tiered|superseded|failed_reinserted|Storage\s*:\s*{|pub struct.*Info|Section\.Info|info_reclamation|server_admin|info' src/command src/admin src/server -g '*.rs' | sed -n '1,240p'Repository: pilotspace/moon
Length of output: 5044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== info command file outline =="
ast-grep outline src/command/info_reclamation.rs --view compact || true
echo
echo "== command info_reclamation relevant sizes and contents =="
wc -l src/command/info_reclamation.rs
sed -n '1,260p' src/command/info_reclamation.rs
echo
echo "== all tracked references to spill_thread getters =="
rg -n -C 3 -e 'spill_completion_superseded|spill_failed_reinserted|record_spill_completion_superseded|record_spill_failed_reinserted' . || trueRepository: pilotspace/moon
Length of output: 19649
Wire spill_completion_superseded into INFO fields.
record_spill_completion_superseded() is called from src/shard/persistence_tick.rs:731, but src/storage/tiered/spill_thread.rs:189's getter is not read by any INFO/metrics caller. Add spill_completion_superseded_total() to an INFO section alongside existing spill fields, and update the field name in CHANGELOG.md:35 if the final output key differs.
🤖 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/storage/tiered/spill_thread.rs` around lines 183 - 191, Wire
spill_completion_superseded_total() into the existing INFO/metrics section that
reports other spill fields, using spill_completion_superseded as the output key.
If the chosen INFO key differs, update the corresponding CHANGELOG entry to
match the final field name.
#459) Addresses the PR #465 review. - INFO: `spill_completion_superseded` is now actually emitted next to its sibling `spill_failed_reinserted`. The counter and getter existed but were never wired into the INFO persistence section, so the CHANGELOG claimed a field that did not exist. Verified on a live server. - CHANGELOG: `[Unreleased]` had gained a second `### Fixed` heading. Keep a Changelog allows one section per change type per release, and a parser that indexes by heading silently drops one block. Folded into the existing section. - tests: replace the fixed 5s drain with a poll for a STABLE DBSIZE, and fail a refused SET with an explicit "this run never reached the in-flight window" message. Both failure modes were environmental (slow disk backs the spill queue up, `try_send` fails, eviction surfaces OOM) and would have been read as the defect returning. Re-confirmed the amended tests still fail on the pre-fix binary with identical diagnostics — 277 resurrections, retry-liars, DBSIZE 124->400 — so they still discriminate. Also ~3x faster (12.9s -> 3.9s) since the poll returns as soon as the queue is quiet. - docs: record the memory-accounting gap the review surfaced. The pending payload is resident RAM `used_memory` does not count: `db.remove()` credits back the whole entry cost while the queued request still holds a full `Bytes::copy_from_slice` of the value. Verified PRE-EXISTING on main, which makes the same copy and the same credit-back; this PR adds only the key `Bytes` plus a small struct per in-flight key, and moves the end of the retention window from "channel drained" to "completion applied". Charging the bytes needs `evict_to_budget` taught to stop while pending bytes are large, or it evicts in a runaway loop chasing memory that cannot drop yet — a design change, tracked in #466 rather than smuggled into a visibility fix. Verified: fmt clean; clippy clean on default features AND on runtime-tokio,jemalloc; the three wire-level tests green on monoio and tokio, and still red on the pre-fix binary. author: Tin Dang
Collaborator
Author
|
@coderabbitai review Second commit (3c19dd9) addresses all four findings from the first pass:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and commits =='
gh pr view 465 --repo pilotspace/moon --json number,title,headRefOid,baseRefName,commits,files \
--jq '{number,title,headRefOid,baseRefName,commits:[.commits[]|{oid,messageHeadline}],files:[.files[]|{path,additions,deletions}]}'
echo '== Follow-up commit diff =='
git show --format=fuller --find-renames --find-copies 3c19dd9b -- \
CHANGELOG.md \
src/server/conn \
src/storage/tiered/spill_thread.rs \
tests/spill_inflight_visibility.rs
echo '== Issue `#466` =='
gh issue view 466 --repo pilotspace/moon --json number,title,state,body,url,assignees \
--jq '{number,title,state,body,url,assignees:[.assignees[].login]}'
echo '== Full PR changed-file patch summary =='
git diff --stat origin/main...HEAD
git diff --check origin/main...HEADLength of output: 16633 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Module outlines =='
for file in \
src/command/connection.rs \
src/shard/persistence_tick.rs \
src/storage/db/mod.rs \
src/storage/db/kv_ops.rs \
src/storage/db/accessors.rs \
src/storage/eviction.rs \
src/storage/tiered/spill_thread.rs \
src/server/conn/blocking.rs \
src/server/conn/handler_monoio/mod.rs \
tests/spill_inflight_visibility.rs
do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo '== Spill lifecycle symbols and INFO emission =='
rg -n -C 5 \
-e 'spill_inflight_(mark|forget|alive|entry|value|keys|is_empty)' \
-e 'PendingSpill' \
-e 'spill_completion_superseded|spill_failed_reinserted' \
-e 'SpillCompletion|SpillRequest' \
-e 'promote_inflight_if_present' \
src tests
echo '== Changed source patch, excluding already-inspected changelog/test follow-up =='
git diff --find-renames origin/main...HEAD -- \
src/command/connection.rs \
src/server/conn/blocking.rs \
src/server/conn/handler_monoio/mod.rs \
src/shard/persistence_tick.rs \
src/storage/db/accessors.rs \
src/storage/db/kv_ops.rs \
src/storage/db/mod.rs \
src/storage/eviction.rs \
src/storage/tiered/spill_thread.rsLength of output: 174732 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Database counting and key enumeration =='
sed -n '598,750p' src/storage/db/kv_ops.rs
echo '== In-flight methods =='
sed -n '300,420p' src/storage/db/mod.rs
echo '== Completion handling =='
sed -n '613,750p' src/shard/persistence_tick.rs
echo '== DBSIZE dispatch call sites =='
rg -n -C 4 -P '\blogical_len\s*\(|\bDBSIZE\b' src --glob '*.rs'
echo '== In-flight payload reads and expiry call sites =='
rg -n -C 4 \
-e 'spill_inflight_(entry|value|alive)\s*\(' \
-e '\bcheck_expired\s*\(' \
-e '\bdrop_if_expired\s*\(' \
src --glob '*.rs'
echo '== Existing logical-length expiry tests =='
rg -n -C 8 -P 'fn\s+(test_len_and_expires_count|.*logical_len.*|.*expiry.*)' \
src/storage/db/mod.rs src/storage/db/kv_ops.rsLength of output: 77023
The four follow-up changes are correct:
I also traced request retirement and completion publication. I found no new actionable findings. ✅ Action performedReview finished.
|
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 `@tests/spill_inflight_visibility.rs`:
- Around line 305-326: Update the test helper drain around drain to use an
explicit spill-completion idle signal, such as a completion watermark or
dedicated queue-idle observation, instead of treating stable DBSIZE samples as
completion. Ensure the deletion-finality checks run only after all pending
SpillCompletions have been applied.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b665634-e44c-445c-b49e-7104fb437d83
📒 Files selected for processing (4)
CHANGELOG.mdsrc/command/connection.rssrc/storage/db/mod.rstests/spill_inflight_visibility.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- src/storage/db/mod.rs
CodeRabbit re-review of #465 caught a soundness bug in the test's own drain condition, and it fails in the direction that matters. `DBSIZE` alone cannot tell "the spill pipeline is quiet" from "the pipeline has not started applying completions yet". In the deletion-finality test that distinction is the whole point: after the DEL sweep every key is either deleted or in-flight-invisible, so DBSIZE reads 0 IMMEDIATELY while completions are still queued. A stability gate can therefore return before any completion is applied, and a completion that republishes a deleted key lands after the assertions have already run — a FALSE PASS hiding the exact P0 this file exists to catch. It caught the bug anyway only because completions land inside the poll's ~1s floor. That is luck, not a guarantee, and it would have decayed silently on a slower or busier machine. - `drain` now gates on server-reported spill PROGRESS as well as DBSIZE: `spill_batches_flushed` + `spill_completion_superseded` from INFO must ALSO hold still, for six consecutive samples. Either counter moves while anything is left in the pipeline. - the deletion test gains a vacuity guard. "Nothing came back" only means something if the completions actually ARRIVED and were refused, so the test now asserts `spill_completion_superseded > 0` — the count of completions the event loop applied and declined to publish because DEL had retired their in-flight record, which is the mechanism under test. Without it, a run where nothing spilled, or where the queue drained before the DELs, passes while proving nothing. `info_counter` returns None for an absent field rather than defaulting to 0, so a missing counter cannot satisfy the guard. Verified: still RED on the pre-fix binary with identical diagnostics — 277 resurrections, retry-liars, DBSIZE 124->400 — and the deletion test still fails on the resurrection assertion, not on the new guard, so the ordering reports the defect rather than the guard. Green on monoio and tokio. fmt and clippy clean. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 10, 2026
Merging main (which now carries #465) back into this branch re-created the duplicate `### Fixed` under `[Unreleased]`: this branch predates the restructure that #465 applied, so it re-added its own heading above the existing one. Two `### Fixed` sections in one release is the exact defect flagged on #465 — Keep a Changelog allows one section per change type, and a parser that indexes by heading reads one block and silently drops the other. Merging as-is would have regressed main straight back into it, and no CI check looks at changelog structure. Folded into the single section, preserving main's `Added -> Security -> Fixed` order and both entries. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 10, 2026
…alling context (#463) * fix(protocol): RESP3 reply types match Redis, and stop depending on calling context A live differential sweep against redis-server 8.6.1 found Moon's RESP3 conversion table wrong in both directions and, structurally, unable to be right: it keyed only on the command NAME, while WITHSCORES, WITHVALUES and a <count> argument are what actually decide the reply shape. Under-conversion, client-breaking: - ZRANGE/ZREVRANGE/ZRANGEBYSCORE/ZDIFF/ZUNION/ZINTER/ZRANDMEMBER ... WITHSCORES arrived as a flat array of bulk strings instead of pair-wrapped [member, Double]. An unmodified redis-py raises "ValueError: not enough values to unpack" on this, so every RESP3 application reading sorted-set scores was broken outright. - HRANDFIELD WITHVALUES / ZRANDMEMBER WITHSCORES answered a Map where Redis answers an array of pairs. - ZMSCORE, GEOPOS, SPOP <count>, ZPOPMIN/ZPOPMAX and XINFO STREAM were not converted at all. Over-conversion, equally wrong: - SISMEMBER, HEXISTS, EXPIRE, PEXPIRE, PERSIST, SETNX and MSETNX returned Boolean where Redis returns Integer. - INCRBYFLOAT/HINCRBYFLOAT returned a lossy Double (,10.6) where Redis returns the exact Bulk "10.59999999999999964". Emptiness must not change the reply TYPE either: HGETALL and CONFIG GET on a miss answered *0 where Redis answers %0, so a client dispatching on the type byte broke on exactly the path it hits most. The conversion is now decided by (command, args) at one policy choke point (Resp3Shape in src/protocol/resp3.rs) instead of by 11 call sites across three handlers. The cross-shard reply loop no longer has the command's args by the time its batch returns, so the shape is classified at ENQUEUE time and a 1-byte Copy tag travels in RemoteMeta -- no per-command allocation on the shard hot path. execute_transaction_sharded now takes the connection's protocol version and converts each inner reply with its own command, so a command answers the same shape standalone, inside MULTI/EXEC and inside a pipeline; previously SMEMBERS was a Set outside a transaction and a flat Array inside one. CONFIG GET (Map) and CLIENT INFO (Verbatim) are fixed at their intercepts, which short-circuit the dispatch exit entirely -- the reason CONFIG could never be fixed before. RESP2 is byte-identical, pinned by a test written before the fix. Verification (all against a LIVE redis-server 8.6.1, never against Moon's own expectations): - scripts/test-client-compat.sh --strict: PASS 98 -> 157, WAIVED 54 -> 25, FAIL 0, exit 0, with all 13 now-stale waivers deleted. - tests/resp3_type_fidelity.rs: 13 new tests asserting the wire type byte directly, including shape equality across standalone/MULTI/pipeline and across a 4-shard server. - Raw-socket byte-diff of the miss path: BYTE-IDENTICAL to redis 8.6.1. - redis-py acceptance: zrange withscores went from raising to [['a', 1.0]]. - clippy -D warnings clean on default and runtime-tokio,jemalloc; fmt clean. The empty-reply defect was found by an adversarial re-read AFTER every bar was green: all 152 harness cases populated their key first, so the entire miss path was undiffed. Five miss-path cases were added and verified discriminating by running them against the pre-fix binary, where all five fail. Refs: .add/tasks/resp3-type-fidelity, milestone v0-9-client-compat author: Tin Dang * chore(add): record resp3-type-fidelity verify evidence + pre-existing-failure A/B Advances the task to phase=verify and fills the GATE RECORD with the measured evidence. Documents the same-load A/B that cleared this change of the two full-suite residual failures: a pre-change server built from main fails the dbsize_offload_logical guard MORE often (7/8) than this branch (5/8), with an identical assertion, line and key counts on both legs. The gate OUTCOME is deliberately left open — §6's 'a person reviewed and approved the change' is not the AI's box to tick, and this change carries a disclosure (a unit test was deleted during build) that warrants a human read. author: Tin Dang * chore(add): gate resp3-type-fidelity PASS — tokio CI parity green, follow-ups filed Records the human-approved gate (Tin Dang, 2026-08-10) and the two verification runs added after the first evidence pass: - tokio CI-parity suite (the feature set every CI test job builds): 195 binaries, 4352 passed, 0 failed, exit 0. It also passed both tests that failed the monoio run, independently corroborating the contention diagnosis. - Client-compat harness against the DEPLOYED :6381 flag set on a throwaway server with an empty data dir: PASS 157 / FAIL 0. Live :6381 was deliberately not used as the target -- differ.py FLUSHALLs before every one of its 182 entries and that instance holds ~764k actively-growing keys. Residual findings filed rather than folded in: #459 #460 #461 #462. author: Tin Dang * test(client-compat): re-point the divergence fixture — SISMEMBER is fixed now The harness self-test test_a_diverging_entry_exits_one_and_names_the_divergence asserts that a diverging entry exits 1 and names TYPE. It borrowed a REAL divergence as its fixture: SISMEMBER under RESP3, where Moon over-converted Integer to Boolean. This PR fixes that over-conversion, so the fixture stopped diverging and the test began failing with '0 != 1' — it failed as a reward for fixing something, which is backwards. Re-pointed at COMMAND COUNT (Redis :274 vs Moon *0), an open TYPE divergence owned by client-identity-introspection, so the reply-type line of work cannot silently retire it again. This is the THIRD fixture for this test (GET-inside-MULTI -> SISMEMBER -> COMMAND COUNT); the comment now records the rotation so the next person sees the pattern instead of rediscovering it. No permanent-by-construction TYPE divergence exists to use instead: Moon's proprietary commands return an Error on both servers, and redis 8.6.1 implements 'hotkeys' as well. The durable fix is a test-only injection hook that fabricates a divergence rather than borrowing one — tracked in #461. Caught by CI, not locally: the client-compat job runs this Python suite in addition to the harness itself, and I had only run the harness. author: Tin Dang * docs(bench): BENCHMARK.md 2.11 — RESP3 type-fidelity no-regression gate on GCE Records the performance check for PR #463 on both arches. The change adds a shape classification at enqueue and a conversion at the reply exit, both gated on proto >= 3; redis-benchmark speaks RESP2, so this measures the cost of the GATE, which is what could regress existing workloads. Result: no regression detectable on either arch. x86 median delta +0.00% with 0/18 rows outside their noise floor; ARM +0.72% with 1/18 (a marginal GAIN, not claimed -- Redis moved in the same direction on that row). Method note worth keeping: the first attempt was a single full bench-compare matrix, and it was DISCARDED. It showed apparent Moon regressions of -16.3%, -15.5% and -11.4% on x86 -- but Redis, unchanged code benchmarked in both legs, drifted up to -27.0% between the same legs. When the control moves further than the subject, no per-row number means anything. Replaced with an interleaved 5-repetition A/B (main -> branch alternating) so drift hits both legs equally, with Redis re-measured every repetition. The record states the supportable claim (any effect is below a 3.6-6.0% noise floor) rather than the unsupportable one (there is no regression). author: Tin Dang * docs(changelog): fold the duplicate Fixed heading after the main merge Merging main (which now carries #465) back into this branch re-created the duplicate `### Fixed` under `[Unreleased]`: this branch predates the restructure that #465 applied, so it re-added its own heading above the existing one. Two `### Fixed` sections in one release is the exact defect flagged on #465 — Keep a Changelog allows one section per change type, and a parser that indexes by heading reads one block and silently drops the other. Merging as-is would have regressed main straight back into it, and no CI check looks at changelog structure. Folded into the single section, preserving main's `Added -> Security -> Fixed` order and both entries. 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
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.
Fixes #459.
Filed as a possible durability bug from a flaky
dbsize_offload_logicalassertion (
live 373, recovered 400). That symptom turned out to be themildest of three faults from one root cause.
Root cause
evict_one_async_spillfreed the hot entry the instant theSpillRequestwasqueued, and
cold_indexonly learned the key when the completion landed. Inbetween, the key was in no plane the database consults —
spill_inflightheld only a request id and was read solely by the completion path.
The eviction code named the window and accepted it:
The reasoning was half right. The tentative
cold_indexentry was correctlyrejected (the slot is unknown at evict time, so it would serve a different
key's value), and the AOF genuinely does backstop durability. What nothing
covered is visibility — a read-miss is not a latency artifact, it is a
wrong answer — and nothing considered a write landing inside the window.
Measured
origin/main@4c9bd2c5, 400 × 4 KiB keys against a 512 KiB cap, 1 shard,--disk-offload enable(the failing test's exact config):DBSIZEafter all SETs ackedGET/EXISTSon live keysDELacked, then reversed:0, all 277 read backDBSIZE0The third is the severe one and it is not transient: the completion path
inserted into
cold_indexunconditionally, so the resurrections reachedthe manifest and survived restart. A client that deleted data got it back.
Blast radius is
--disk-offload enable, which is opt-in — but for those usersit is a P0.
Fix
spill_inflightbecomes the third storage plane it always was, carrying thepayload. That payload is the same refcounted
Bytesthe queued requestalready pins, so this is a refcount rather than a copy and adds no peak
memory.
reach a cold key:
promote_cold_if_present(collections, Lua, MULTI), themonoio async GET pre-warm, the inline GET fast path (which was framing
$-1inline), andget_cold_valuefor the&selfshared-read*_readonlyhandlers;EXISTS/DELcount it;DBSIZEandlogical_lencount it;KEYSandRANDOMKEYenumerate it;DEL, an overwritingSET, and a promoting read each retire the record,which withdraws the completion's authorization to publish. That is what
makes a delete inside the window final, and it also stops a stale payload
landing as a cold shadow behind a newer hot value;
spill_completion_superseded.Non-spilling servers pay one
is_empty()load on the affected paths.Two traps worth knowing about for review
db.remove()clears the in-flight record(that is what makes
DELfinal), so the mark must follow it — markingfirst erases itself. Both run synchronously on the shard thread with no
.awaitbetween, so no reader sees the gap.get_readonlycallsget_cold_valuedirectly, not viacold_read_only. Patchingcold_read_onlylooked correct and changednothing; monoio was fully green while tokio still failed with
EXISTS=1 andGET=nil. All CI test jobs build tokio, so both legs were run.Verification
Red-first.
tests/spill_inflight_visibility.rs— one wire-level test perfault, each failing on the pre-fix binary for the right reason (277
resurrections / retry-liars / DBSIZE 124→400) and green after. The assertion
shape throughout is retry-and-compare: a key that answers nil and then
returns a value with no write in between cannot be explained by legitimate
allkeys-lrueviction, which never brings a key back. Plus five unit tests onthe plane's own semantics.
tokio (CI parity), both exit 0
cargo clippy --all-targetsclean on default features and onruntime-tokio,jemalloc;cargo fmt --checkclean#[ignore]d offload crash suites pass, includingoverwritten_cold_key_returns_new_value_after_crash, which exercises theset()change across a killtests/dbsize_offload_logical.rs— the suite that filed the issue — passesTwo things deliberately not done
The bench is inconclusive, not clean. The change adds one
is_empty()load to
set()and to the inline GET path. A 9-rep interleaved A/B on themoon-devVM (alternating leg order, Redis re-measured every rep as acontrol) gave mixed-sign deltas —
get p1−0.29%,get p16+2.35%,set p1+5.31%,set p16−1.52% — every one inside its own per-cell noisefloor of 7–29%. That resolution cannot see an effect this small, so it is not
evidence of "no regression". It is not recorded in
BENCHMARK.md: §2 isthe GCloud section and OrbStack numbers are development-only per CLAUDE.md.
A GCE-grade number can be taken on
moon-bench-arm/moon-bench-x86ifreviewers want one.
SCAN's ordered cursor still skips in-flight keys. Merging an unordered
plane into an ascending
hash48walk that mutates under the cursor is realwork and was not attempted. This stays within SCAN's contract (only keys
present for the whole iteration are guaranteed);
KEYS/RANDOMKEY, whosecontracts are point-in-time, do include them. Documented at the call site
rather than left silent.
Summary by CodeRabbit