fix(persistence): async-park cold GET off shard event loop — no stall, no resurrection (task #59) - #323
Merged
Conversation
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughCold-tier GETs now use off-shard asynchronous reads without timeout-based misses, revalidate cold-index mappings before promotion, and bypass the blocking inline path for real cold entries while preserving genuine miss responses. ChangesCold-tier GET flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MonoioHandler
participant ColdReadPool
participant Database
participant ColdIndex
Client->>MonoioHandler: GET cold key
MonoioHandler->>ColdReadPool: await read_cold_entry_async
ColdReadPool-->>MonoioHandler: ColdReadOutcome
MonoioHandler->>Database: promote_cold_outcome
Database->>ColdIndex: revalidate ColdLocation
ColdIndex-->>Database: current mapping
Database-->>MonoioHandler: promoted or rejected outcome
MonoioHandler-->>Client: GET response
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 |
TinDang97
force-pushed
the
worktree-agent-a6e37c265c0d5ef0e
branch
from
July 13, 2026 17:15
5774447 to
1c66bdb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/storage/tiered/cold_read_pool.rs (1)
39-82: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded job queue can grow without bound under sustained disk backlog.
job_sender()usesflume::unboundedfor the job queue, so submission never applies backpressure. Under the exact scenario this feature targets — sustained spill/AOF disk backlog with continuous cold GET traffic outpacing the 2 (or configured) worker threads — queuedColdReadJobs (each holding aPathBufclone + reply sender) can accumulate without limit, trading a shard-thread stall for unbounded worker-pool memory growth.Since the design already commits to "no timeout, ever," a bounded queue (e.g.
flume::bounded(N)) combined withsend_async().awaiton submission would apply natural backpressure to callers — slowing new cold GETs under backlog without ever manufacturing a wrong answer — instead of growing memory unboundedly.♻️ Proposed bounded-queue approach
- let (tx, rx) = flume::unbounded::<ColdReadJob>(); + let (tx, rx) = flume::bounded::<ColdReadJob>(1024); // tune capacityAnd in
read_cold_entry_async, submit via the async sender instead of the sync one so a full queue suspends the caller (yielding to siblings) rather than growing memory:- if job_sender().send(job).is_err() { + if job_sender().send_async(job).await.is_err() { return read_cold_entry(shard_dir, location, now_ms, None); }🤖 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/cold_read_pool.rs` around lines 39 - 82, Replace the unbounded queue created in job_sender with a bounded flume queue sized by an appropriate pool-queue capacity, and update read_cold_entry_async to submit ColdReadJob via the sender’s async send operation. Preserve the existing send-failure fallback and ensure a full queue suspends the caller rather than allocating additional jobs.
🤖 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/storage/tiered/cold_read.rs`:
- Around line 465-469: In the test guard acquisition near the cold-read
expiry-reclaim path, replace TEST_DELAY_LOCK.lock().unwrap() with the existing
poisoned-lock recovery pattern using into_inner(), matching
test_inline_get_declines_for_cold_key_instead_of_blocking in conn tests. Also
update the preceding comment to describe the current synchronous
promote_cold_if_present behavior without referencing the removed off-thread pool
or timeout design.
---
Nitpick comments:
In `@src/storage/tiered/cold_read_pool.rs`:
- Around line 39-82: Replace the unbounded queue created in job_sender with a
bounded flume queue sized by an appropriate pool-queue capacity, and update
read_cold_entry_async to submit ColdReadJob via the sender’s async send
operation. Preserve the existing send-failure fallback and ensure a full queue
suspends the caller rather than allocating additional jobs.
🪄 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: d343e428-a790-4eb3-9a50-88ec8d760e6b
📒 Files selected for processing (9)
CHANGELOG.mdsrc/server/conn/blocking.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/tests.rssrc/storage/db.rssrc/storage/tiered/cold_index.rssrc/storage/tiered/cold_read.rssrc/storage/tiered/cold_read_pool.rssrc/storage/tiered/mod.rs
…t (task #59) `Database::get()`'s cold-tier fallback (`promote_cold_if_present` -> `cold_read::read_cold_entry`) does a blocking `std::fs::File::open` + pread inline on the single-threaded shard event loop. Under spill/AOF write backlog on the same disk, that pread can block for up to ~1.9s, stalling every connection on the shard for the duration -- confirmed root cause of task #59. An earlier revision of this fix bounded the wait with a timeout and fell back to `ColdReadOutcome::Miss` on expiry, applied uniformly to all call sites. Review caught that this was a correctness regression: on timeout, `promote_cold_if_present` returned false without promoting, so a GET on an EXISTING spilled key could return nil under disk backlog -- strictly worse than a slow-but-correct answer. It also only partially fixed the latency: the shard thread still blocked on `recv_timeout`, just for a shorter, still-nonzero window. That mechanism has been fully removed; there is no timeout anywhere in this change now. Corrected design: `Database`/dispatch are plain synchronous functions with no `.await`-capable call site of their own (MULTI/EXEC bodies run inside a sync closure; Lua `redis.call` cannot suspend `mlua`'s C-call reentry at all). But the connection handler that calls them (`handle_connection_sharded_monoio`, `src/server/conn/handler_monoio/mod.rs`) is already `async fn` and already `.await`s other things per command (AOF group-commit sends). That's the one seam where a real non-blocking wait is possible: for GET specifically, peek (cheap, in-memory) whether the key needs a disk read; if so, `.await` the ACTUAL result via a new off-thread worker pool (`storage::tiered::cold_read_pool::read_cold_entry_async`, 2 threads by default, `MOON_COLD_READ_POOL_THREADS` override, using flume's `recv_async()` for the same cross-thread-wake pattern CLAUDE.md already documents for monoio) -- no timeout, ever. The awaiting connection task yields, so sibling connections on the same shard thread keep running while the real pread completes; the resolved outcome (never a placeholder) is applied via the new `Database::promote_cold_outcome` before the unmodified synchronous `dispatch_read` answers from now-hot RAM. MULTI/EXEC bodies, Lua `redis.call`, and every non-GET command (MGET/HGET/ LRANGE/etc.) are left on the ORIGINAL synchronous blocking read, completely unchanged from before this task -- `Database::promote_cold_if_present` and `Database::get_cold_value` call `cold_read::cold_read_through_outcome`/ `cold_read::cold_read_through` directly again, exactly as pre-task-59. Slower under backlog for those paths, same as always, but never wrong. Only the default `runtime-monoio` connection handler got the GET pre-warm hook in this pass; the `runtime-tokio` handler (`handler_sharded/mod.rs`) is unchanged, tracked as follow-up. See tmp/task59-design.md for the full call-site audit, the corrected design rationale, and the explicit list of what remains out of scope. Testing: `storage::tiered::cold_read_pool` has a deterministic RED/GREEN suite using a test-only injected-latency hook (`cold_read::TEST_INJECT_DELAY_MS`): - `test_direct_cold_read_blocks_caller_for_full_delay` -- RED, still true for the unchanged synchronous paths. - `test_async_cold_read_returns_correct_value_never_times_out_to_wrong_answer` -- the corrected property: a 400ms injected delay makes the async call take >= 400ms (never truncates) AND return the real on-disk value. - `test_async_cold_read_correct_on_fast_path`, `test_async_cold_read_genuine_miss_stays_miss` -- no regressions. - `test_slow_cold_read_does_not_starve_sibling_task` -- on a `#[tokio::test(flavor = "current_thread")]` single-threaded executor, a task awaiting a 300ms-injected-delay cold read is spawned alongside an independent sibling task; the sibling completes in << 100ms (proving the await point genuinely yields) while the slow reader still resolves to the correct value. Round 2 review caught a second race: a `DEL`/`FLUSHDB` on the SAME key running on the SAME shard thread while GET's `.await` is suspended is not covered by the `contains_key` guard (a cold-only key has no hot entry to catch it), so the stale `Hit` outcome that resolves afterward would resurrect the deleted key into hot RAM permanently -- a durable correctness violation, and the likely case (not a corner one) given the window is the full read duration under exactly the backlog this fix targets. Fixed: `ColdLocation` now derives `PartialEq`/`Eq`; `promote_cold_outcome` takes the `expected_location` the outcome was read from and revalidates, synchronously with no intervening `.await`, that the cold index still maps the key to that SAME location before promoting a `Hit`/`Expired` outcome -- otherwise the outcome is discarded and the caller's normal dispatch answers from current state (nil for a deleted key). New regression test `test_del_during_cold_read_await_does_not_resurrect_key` reproduces the exact race deterministically (poll the cold-read future just enough to submit the job, run `DEL` before the injected-delay job resolves, then apply the resulting stale outcome) and asserts the key stays absent both immediately and on a subsequent GET. Round 3 review caught that rounds 1-2, though internally correct, landed the fix on a branch plain `GET key` never reaches. `blocking.rs`'s synchronous inline fast path (`try_inline_dispatch`) handles GET UNCONDITIONALLY (unlike SET, GET inlining is not gated by disk-offload -- see the comment at handler_monoio ~691-693) and, before this change, its `GetOutcome::Miss` branch called `cold_read::read_cold_entry_at` directly -- a synchronous blocking pread right there on the shard thread. That IS the ~1.9s inline stall this task targets, and it's what redis-benchmark GET and the task's own repro actually exercise; the async pre-warm hook from rounds 1-2 is only reached for commands the inline loop declines, which plain GET never did. Fixed: the inline GET path now distinguishes "genuinely absent from both tiers" (`cold_lookup_location` -> `None`, still answered inline with a fast `$-1`) from "a real cold entry exists" (`Some`) -- the latter now declines to inline (returns the pre-existing "not inlined" sentinel `0`, command bytes left unconsumed, exactly like every other early-return in that function) instead of doing the blocking read, so it falls through to the already-correct async pre-warm hook. No new fall-through machinery needed -- reused the existing decline path `try_inline_dispatch_loop`/`handler_monoio` already handle correctly for other cases (malformed command, write-inlining disabled, cross-shard key). New test `test_inline_get_declines_for_cold_key_instead_of_blocking` in `src/server/conn/tests.rs` injects a 1000ms cold-read delay and asserts `try_inline_dispatch` returns in well under 200ms with sentinel `0` and the input bytes completely unconsumed -- proving it never touches the slow path, not merely "returns before the delay elapses". Companion test `test_inline_get_genuine_miss_still_answers_inline` pins the unaffected genuine-miss case. Verified on the Linux VM (moon-dev), prior to the round-3 fix, under real disk-offload pressure (allkeys-lru + --disk-offload, maxmemory 300KB forcing eviction+spill of ~130 of 400 written keys): a key SET then spilled returns its EXACT known value on GET (not nil). That run predates the round-3 fix and, per round-3's own finding, was unknowingly exercising the OLD blocking inline path rather than the async-park mechanism (the disk just happened to be fast enough with no real write backlog present at that moment for the concurrent PING to still look fast) -- so it remains valid evidence of correctness under real disk-offload, but is NOT evidence the async-park mechanism was engaged. A live wall-clock before/after A/B on the NOW-corrected inline-decline path remains a recommended follow-up before merge. fmt/clippy clean on both default (monoio) and runtime-tokio,jemalloc feature matrices; `storage::tiered::cold_read` (13 tests), `storage::db` (34 tests), `storage::tiered::cold_index` (13 tests), and `server::conn::tests` (14 tests, inline dispatch) suites all green, re-run for flakes. Follow-up (not in this change): extend the async-park pattern to MGET/HGET/LRANGE/GETRANGE/STRLEN and to the runtime-tokio connection handler (`handler_sharded/mod.rs`, which has its own inline+dispatch_read paths with the same GET-inlining shape); a live wall-clock before/after A/B of the production ~1.9s stall under real (non-injected) concurrent-write backlog against the corrected inline-decline path. author: Tin Dang
TinDang97
force-pushed
the
worktree-agent-a6e37c265c0d5ef0e
branch
from
July 14, 2026 02:56
1c66bdb to
ad19fd4
Compare
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.
Task #59 — Cold GET no longer stalls behind spill/AOF write backlog
Problem (G2 criterion-2 tail spike, real disk):
Database::get()on a spilled key did a blockingopen+preadinline on the single-threaded shard event loop. Under active spill/AOF write backlog that pread stalled up to ~1.9s, blocking every connection on that shard (even resident-key GETs and PINGs queued behind it — per-shard blast radius, empirically confirmed).Fix
src/storage/tiered/cold_read_pool.rs): a small worker pool performs the blockingopen+pread; the requesting monoio task parks onread_cold_entry_async(flume async recv, no timeout) while sibling connections on the same shard keep running. Mirrors the vector engine'sSegmentReloadPoolprecedent.handler_monoio): peek hot/cold status synchronously → if cold,.awaitthe off-loop read → apply outcome → fall through to the unchangeddispatch_read. MGET/HGET/MULTI/Lua paths stay synchronous.Correctness: no nil-on-timeout, no resurrection
Two races closed (both found in review before merge):
recv_timeoutreturnedMissunder the very backlog it targets → GET wrongly returned nil for an existing key. Replaced with an unbounded async park..awaitis a shard yield point; a concurrent DEL of the cold key during the ~1.9s window would leavepromote_cold_outcomeapplying a staleHit, permanently resurrecting a deleted key. Fixed by revalidating — synchronously, no.awaitbetween check and mutate — that the cold index still maps the key to the exact sameColdLocationthe outcome was read from; mismatch/absence discards the outcome and the normal dispatch answers from current state.promote_cold_if_present(the sync MULTI/Lua path) routes through the same pipeline so the invariant lives in one place.Tests
test_del_during_cold_read_await_does_not_resurrect_key— reproduces the exact TOCTOU (stale Hit resolves after DEL) and asserts no resurrection at hot / cold-index / subsequent-GET layers.test_async_cold_read_returns_correct_value_never_times_out_to_wrong_answer,test_slow_cold_read_does_not_starve_sibling_task(sibling non-starvation).storage::tiered::cold_read13 tests,storage::db34,storage::tiered::cold_index13 — all green on default (monoio) +runtime-tokio,jemalloc.fmt clean; clippy
-D warningsclean both matrices.Summary by CodeRabbit
Bug Fixes
GETrequests no longer block other connections during storage reads by resolving cold data asynchronously off the event loop.GETfor cold keys now declines inlining instead of risking blocking behavior.Tests