Conversation
…scarded
moon#965's title says "AOF tail loss". It is not: both writes are on disk and
both replay. The newer one is thrown away at the END of recovery, and the
server logs it doing so.
On `runtime-tokio` with `--shards 1`, `main.rs` deliberately creates no
`AofManifest` (v2 single-file recovery owns single-shard durability), so
nothing ever seeds a `MOON.COLDCUT` head. `MOON.SPILLED` markers, however, are
emitted unconditionally by `ColdMarkerSink::emit`. That configuration has been
running the moon#902 mechanism half-armed — markers without the cut:
1. The replayed marker drops the key's hot copy (restart-as-cold).
2. The key's next write replays through `Database::set`'s `Inserted` arm,
which by design leaves the cold shadow standing (only `Updated`
invalidates it). The key ends replay hot (new) AND cold (old).
3. With no cut, `finish_replay_cold_reconcile` takes the legacy task #56
branch and resolves COLD-wins:
Phase 4b cold-plane reconcile (gated=false): 1 hot shadow(s) demoted
`replay_cold_spilled` claimed it was "safe to call without a gate … the demote
pass would have made the same decision less precisely". Without the gate the
drop MANUFACTURES the hot∩cold state the demote pass then resolves backwards.
A replayed marker is proof the log is #902-era; whether its head also carries
the cut is an artifact of which runtime wrote it, not of the data. So a marker
now records `replay_saw_cold_marker`, and reconcile resolves hot-wins for that
generation with or without a cut. Hot-wins is value-correct for every way a key
can be hot∩cold there: written after its marker (hot is newest); a stale entry
in a file the manifest still lists (likewise); or its own marker lost under AOF
backpressure — both planes then hold the same value and the cost is
restart-as-cold for that one key, already documented at the emit site. A log
with neither record (pre-#902) keeps task #56 verbatim, and no gate is
installed, so replay-time read visibility is unchanged.
Rejected: seeding a `MOON.COLDCUT` head into the legacy single-file AOF (better
long-term, tracked as moon#914, but it changes replay-time read visibility — a
much larger blast radius needing a correct watermark at generation-open); and
per-key demote tracking (leaves the stale-manifest case exposed and adds a third
resolution rule, keeping the runtimes divergent).
Cost: one bool store per replayed marker, one `mem::take` at end of replay. No
serving-path change, no fsync.
The guard that should have caught this, `tests/cold_shadow_single_shard_tokio.rs`
— written for exactly this configuration — was RED on main and ran nowhere: it
was `#[ignore]`d because it shelled out to `redis-cli`, which hosted runners do
not install. It now speaks RESP through `common::Conn` and is un-ignored.
`find_moon_binary()` already resolves `CARGO_BIN_EXE_moon`, built with the same
features as the test, so every `runtime-tokio` leg drives a tokio binary with
no `MOON_BIN` to remember.
Verified, with `redis-cli` removed from PATH and `MOON_BIN` unset:
unit a_write_after_its_marker_beats_the_cold_copy_in_a_legacy_generation
RED with the fix inert (hot_demoted: 1), GREEN with it
e2e cold_shadow_single_shard_tokio
RED 3/3 through nextest retries — 36, 38, 47 stale probes — GREEN with it
clippy --all-targets -D warnings clean on both runtimes; fmt clean
Closes #965
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesCold reconciliation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant AOFReplay
participant Database
participant ColdReconcile
participant ColdShadow
AOFReplay->>Database: replay MOON.SPILLED
Database->>ColdReconcile: provide marker and gate state
ColdReconcile->>ColdShadow: preserve newer hot value
ColdReconcile->>ColdShadow: drop superseded cold entry
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The recovery change preserves newer writes for single-shard Tokio crash recovery while retaining legacy handling for logs without replay markers. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The recovery implementation addresses the stale-value defect from
✨ 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 |
CHANGELOG.md was the only conflict. Main's CI-sharding entry (#1009) and this branch's moon#965 entry were both added at the top of [Unreleased]. Both are kept: the CI entry stays under Changed, and the moon#965 entry moves to Fixed, where a bug fix belongs. 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.
Closes #965.
The issue's title names the wrong mechanism
"AOF tail loss" — but nothing is lost from the AOF. Both writes are on disk and both replay. The newer one is deliberately discarded at the end of recovery, and the server logs it:
Mechanism
runtime-tokio+--shards 1creates noAofManifest(main.rs:1840), so noMOON.COLDCUThead is ever seeded — yetMOON.SPILLEDmarkers emit unconditionally. The #902 mechanism runs half-armed:Database::set'sInsertedarm, which by design leaves the cold shadow standing → the key ends replay hot (new) and cold (old).This is the same quadrant as #1007 and #914: the multi-shard path is correct, the tokio single-shard path was never fully covered.
Fix
A replayed marker proves the log is #902-era, so it now sets
replay_saw_cold_marker, and reconcile resolves hot-wins for that generation with or without a cut. Hot-wins is value-correct for every way a key can end hot∩cold there (written after its marker; a stale entry in a still-listed file; or a marker lost under backpressure, where both planes hold the same value). Pre-#902 logs keep task #56 unchanged; no gate is installed, so replay-time read visibility is untouched. Cost: one bool per replayed marker.Rejected: seeding a
MOON.COLDCUThead for the legacy AOF (#914 — right long-term, but it changes replay-time read visibility, a much larger blast radius); per-key demote tracking (leaves the stale-manifest case exposed and adds a third resolution rule).The guard that should have caught this — now actually runs
tests/cold_shadow_single_shard_tokio.rswas written for exactly this configuration and was red on main for weeks, but ran nowhere: it was#[ignore]d because it shelled out toredis-cli, which hosted runners don't install. It now speaks RESP throughcommon::Connand is un-ignored.find_moon_binary()already resolvesCARGO_BIN_EXE_moon(built with the test's own features), so everyruntime-tokioleg drives a tokio binary with noMOON_BINto remember.Verification — with
redis-cliremoved from PATH andMOON_BINunset, as CI runs ita_write_after_its_marker_beats_the_cold_copy_in_a_legacy_generationhot_demoted: 1)cold_shadow_single_shard_tokioClippy
--all-targets -D warningsclean on both runtimes; fmt clean.Unblocks
cold_reconciliation_property_660, which is this bug.retries = 0override treated the symptom; with the cause fixed it can be reconsidered.Summary by CodeRabbit
Bug Fixes
Tests