fix(storage): cold index rebuild resolves a duplicated key by file_id, not manifest order (#983) - #996
Merged
Conversation
…, not manifest order `ColdIndex::rebuild_from_manifest_per_db` resolved a key present in two Active KvLeaf files by "last one seen wins", where "last" was the order `ShardManifest::files()` happened to list the files in. That is `add_file` push order, and push order is NOT recency order: the async spill path pushes a file when its background completion is applied, the durable batch path pushes at eviction time, so a `CONFIG SET appendonly` flip with a completion still in flight registers a higher file_id ahead of a lower one — and `gc_tombstones`, compaction and reopen all preserve that order across every restart. The rebuild then pointed the key at the OLDER file and cold read-through served the superseded value after recovery, with no error and no log line (moon#983). The recency order is `(file_id, page_idx, slot_idx)`: file_id is the shard's spill allocation sequence (monotonic per process, re-seeded on restart above every file on disk), a key can only be re-spilled after it came back hot, and within one file slots are packed in request order. `ColdLocation::recency_key` now names that order, and the bulk build (`from_pairs_newest_wins`, was `from_pairs_last_wins`) sorts by key then recency-descending and keeps the first of each run — the newest copy by construction, independent of the input order and of which duplicate a collection type happens to keep. Tests: - kv_spill unit test registers the newer file FIRST and asserts the rebuilt index points at the higher file_id; also the same key twice in one file resolves to the later (page, slot). Red pre-fix. - tests/cold_index_duplicate_resolution_983.rs boots the REAL binary on a newest-first corpus built with the spill thread's own writers (red pre-fix: GET answers the stale copy; a private key in the older file guards against a vacuous pass), and drives the real spill -> overwrite -> re-spill -> SIGKILL -> restart lifecycle at --shards 1 and 4. Refs moon#983 author: Tin Dang
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
…cency_key - CHANGELOG: the re-spilled-key recovery fix, with the reproduction on the pre-fix binary (newest-first manifest -> stale copy served) and the lifecycle pin at --shards 1 and 4. - `ColdLocation::recency_key` doc: file_id monotonicity is conditional on `next_spill_file_id_seed` scanning `data/` successfully; on any error but NotFound it defaults to 1 with a warn, and a reset counter overwrites `heap-000001.mpf` in place, so the older copy is destroyed before any ordering question arises. Documented, not changed: the consequence is the pre-existing B-2 overwrite, not misordering, and a fail-closed startup refusal has no error path out of shard init today. - rustfmt on the two new assertion sites. Refs moon#983 author: Tin Dang
Collaborator
Author
Independent verification — my own build, my own A/BBuilt Pre-fix server ( Post-fix server (this PR): Red with the stale value visible at both shard counts, green after. Confirmed. One thing worth stating plainly so the 4/4 is not over-read: two of the four tests pass On the
|
TinDang97
marked this pull request as ready for review
September 17, 2026 02:47
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
TinDang97
added a commit
that referenced
this pull request
Sep 17, 2026
…d guard #1003 was stacked on #991, which has since merged to main along with #990, #987, #993, #996 and #985. Retargeting to `main` left four conflicted files. Resolutions, each argued from behaviour rather than from which side is longer: * `src/command/sorted_set/sorted_set_write.rs` (3 hunks) — OURS, all three. The conflict looks like "main added `zunionstore`/`zinterstore`/`zstore_impl` and this branch did not", but `git diff c3c5e33 0cfab03 -- src/command/ sorted_set` is EMPTY: main's sorted-set content is exactly #991's, which is already in this branch's history, and #1003 then MOVED that family into the new `sorted_set_store.rs` when the file crossed 1500 lines. Taking main's side would have duplicated three functions. The `use` list is likewise ours: main's extra imports (`AggregateOp`, `parse_numkeys`, `zadd_member`, `zrange_by_*`, `clamp_nan_to_zero`) serve only the code that moved out, and `clippy --all-targets -D warnings` confirms nothing is unused or missing. * `scripts/test-commands.sh`, `scripts/test-consistency.sh` — both sides append independent sections and main's half of each conflict hunk is EMPTY, so the markers were removed in place rather than resolved by file. (`--ours` would have silently dropped the 616 lines main added elsewhere in `test-consistency.sh`; the merged files are pure additions against main — zero deleted lines — and `bash -n` is clean on both.) One row is ADDED by this merge: `test-consistency.sh`'s moon#592 `XW_CASES` sweep enumerates the cross-shard write family, and `ZDIFFSTORE` is now a member, so it gains a `zdiffstore` case beside `zunionstore`/`zinterstore` — the same row moon#645 added for `georadiusstore` when that command joined. * `src/server/conn/shared.rs` — the dangerous one, and not just a doc comment. ## ZDIFFSTORE: the routing invariant the two sides disagree about Main's doc block says `ZDIFFSTORE` is deliberately EXCLUDED from `touches_a_key_it_did_not_route_on` because it is "not implemented in moon (unknown command), so there is no write to misplace". moon#959 implements it. That sentence is now false, and shipping it as written would leave a cross-shard `ZDIFFSTORE` acking a destination that lands nowhere — the moon#592 misdirected destructive write that main's moon#962 work just closed for twelve other commands. The mechanical trap is that `ZDIFFSTORE` and `ZINTERCARD` are both ten bytes beginning with `z`, so the two sides edited the SAME match arm: HEAD: (10, b'z') => cmd.eq_ignore_ascii_case(b"ZDIFFSTORE") main: (10, b'z') => cmd.eq_ignore_ascii_case(b"ZINTERCARD") Keeping either arm alone drops the other command out of the guard, and keeping both as separate arms is an unreachable pattern. Resolved as one arm naming both spellings, with the collision documented at the arm so a future narrowing is not mistaken for a simplification. Two sites in main's own test code merged CLEANLY and were left asserting the pre-#959 world. Both are corrected here, because a conflict-free merge is not a correct one: * `out_of_family_and_malformed_argvs_keep_their_own_answers` asserted that `ZDIFFSTORE` must NOT be refused ("ZDIFFSTORE is unimplemented; t2k4 owns the moment that changes"). That assertion is now the bug. Removed, with a note saying where it went. * `ZDIFFSTORE` added to `FAMILY`, so `every_family_member_is_refused_across_a_boundary_and_only_there` now covers it positively in all four positions the table checks: refused across a boundary, NOT refused co-located, NOT refused under a `{hash}` tag, and NOT refused at `--shards 1`. The family doc block keeps main's moon#962 text verbatim (it is the accurate, more detailed version) minus the now-false `ZDIFFSTORE` bullet, and gains a short section recording the migration and the arm collision. `tests/two_key_write_cross_shard.rs` needed no change: #1003 had already moved `ZDIFFSTORE` from the `t2k4` unimplemented-store tripwire into `t2k1`'s `PROBES`, which is precisely the migration `t2k4` exists to force. CHANGELOG: the moon#959 entry now states that `ZDIFFSTORE` joins the moon#592 cross-shard write guard and shares moon#962's `(10, b'z')` arm. Verified on macOS (aarch64): `cargo clippy --all-targets -- -D warnings` clean; `cargo check --no-default-features --features runtime-tokio,jemalloc --all-targets` clean. Tests, release-fast: lib 5608 passed, plus `two_key_write_cross_shard` 5/5 (t2k1's 180 live placements at `--shards 4` now include `ZDIFFSTORE`), `multikey_read_cross_shard` 3/3, `zset_read_cold_tier_928` 5/5, `workspace_key_walker_668` 6/6, `tracking_movablekeys` 2/2. No behaviour from either side was dropped. One lib test is red and it is PRE-EXISTING, not merge-induced: `scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_ configured` fails 3/3 in the full suite and passes alone — its own doc comment says `MAXMEMORY_GLOBAL`/`DB_MAXMEMORY_ANY_SET` are process-global and other tests in the binary publish them. A/B settles it rather than argument: `0cfab03c` (origin/main, untouched) fails the SAME test, 5586 passed / 1 failed; this merge is 5608 passed / 1 failed, the +22 being moon#959's own unit tests, and skipping all 22 still leaves it red. This merge does not touch `src/scripting/` or `src/storage/eviction` at all (`git diff origin/main -- src/scripting/` is empty). author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 19, 2026
…t-drops The branch was stacked on #996 (moon#983), which has since been squash-merged to main as 7b864cd. Merging main absorbs the parent: the two parent commits now contribute no delta, and every file this PR touches differs from main by exactly the moon#875 change it carried before (checked per file against 7c77c72..c03956f). Conflicts: - src/storage/tiered/cold_index.rs: the two `recency_key` doc hunks take main's wording, since moon#997 replaced the `next_spill_file_id_seed` fallback-to-1 caveat with a prove-or-refuse seed. The rebuild's pass-2 hunk keeps this branch's `ColdRebuild { per_db, report }` with the `pending_unlink` retirement of missing files. - src/storage/tiered/kv_spill.rs: main's moon#983 tests call the rebuild and now read `.per_db` from the report-carrying return type. - CHANGELOG.md: main already carries the moon#983 entry, so only the moon#875 entry is added, at the top of `### Fixed` under `## [Unreleased]`. Refs #875 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 19, 2026
…r; indexed-but-unreadable is no longer a miss (#875) (#1004) * fix(storage): cold index rebuild resolves a duplicated key by file_id, not manifest order `ColdIndex::rebuild_from_manifest_per_db` resolved a key present in two Active KvLeaf files by "last one seen wins", where "last" was the order `ShardManifest::files()` happened to list the files in. That is `add_file` push order, and push order is NOT recency order: the async spill path pushes a file when its background completion is applied, the durable batch path pushes at eviction time, so a `CONFIG SET appendonly` flip with a completion still in flight registers a higher file_id ahead of a lower one — and `gc_tombstones`, compaction and reopen all preserve that order across every restart. The rebuild then pointed the key at the OLDER file and cold read-through served the superseded value after recovery, with no error and no log line (moon#983). The recency order is `(file_id, page_idx, slot_idx)`: file_id is the shard's spill allocation sequence (monotonic per process, re-seeded on restart above every file on disk), a key can only be re-spilled after it came back hot, and within one file slots are packed in request order. `ColdLocation::recency_key` now names that order, and the bulk build (`from_pairs_newest_wins`, was `from_pairs_last_wins`) sorts by key then recency-descending and keeps the first of each run — the newest copy by construction, independent of the input order and of which duplicate a collection type happens to keep. Tests: - kv_spill unit test registers the newer file FIRST and asserts the rebuilt index points at the higher file_id; also the same key twice in one file resolves to the later (page, slot). Red pre-fix. - tests/cold_index_duplicate_resolution_983.rs boots the REAL binary on a newest-first corpus built with the spill thread's own writers (red pre-fix: GET answers the stale copy; a private key in the older file guards against a vacuous pass), and drives the real spill -> overwrite -> re-spill -> SIGKILL -> restart lifecycle at --shards 1 and 4. Refs moon#983 author: Tin Dang * docs(storage): CHANGELOG entry for moon#983 and the seed caveat on recency_key - CHANGELOG: the re-spilled-key recovery fix, with the reproduction on the pre-fix binary (newest-first manifest -> stale copy served) and the lifecycle pin at --shards 1 and 4. - `ColdLocation::recency_key` doc: file_id monotonicity is conditional on `next_spill_file_id_seed` scanning `data/` successfully; on any error but NotFound it defaults to 1 with a warn, and a reset counter overwrites `heap-000001.mpf` in place, so the older copy is destroyed before any ordering question arises. Documented, not changed: the consequence is the pre-existing B-2 overwrite, not misordering, and a fail-closed startup refusal has no error path out of shard init today. - rustfmt on the two new assertion sites. Refs moon#983 author: Tin Dang * fix(storage): cold index rebuild reports every entry it cannot recover `ColdIndex::rebuild_from_manifest_per_db` dropped entries on three silent paths — a heap file that failed to read (`Err(_) => continue`), a page that failed magic/type/CRC (`from_bytes` → `None`, next page), and a trailing partial page (`chunks_exact` remainder) — and every entry lost that way read afterwards as an ABSENT key: `GET` nil, `EXISTS` 0, indistinguishable to a client from a key that was never written. Two more silent paths turned up while confirming those: a slot inside a CRC-valid page that does not decode (an unknown `ValueType` — a downgrade), and a file truncated on a page boundary, which no per-page check can see at all. The rebuild now returns a `ColdRebuildReport` alongside the per-db indexes, counting each skip by cause, and logs the first 16 of each with `file_id` (and page/slot). Recovery logs one per-shard summary — `cold index rebuild clean` at info, or `cold index rebuild DEGRADED` at error with every count — and folds the totals into `INFO` as `reclamation_cold_recovery_{files_missing,files_unreadable,files_short, pages_rejected,partial_page_bytes,entries_rejected}_total` so a monitor can alarm on the instance that lost data. A valid `KvOverflow` page, which `KvLeafPage::from_bytes` also rejects, is classified from its header first and is not a loss. Per class, the decision: - `NotFound`: warn, count, and queue the `file_id` on the rebuilt index's `pending_unlink` so the orphan sweep retires the manifest entry — this is the sweep's own unlink-before-commit crash window (nothing lost) or an external removal (nothing recoverable), and without the heal it re-warns on every boot forever (the moon#546a pattern). - any other read error: log at error, count, skip the file but NEVER tombstone it; a restart after the operator fixes it recovers the keys. A recovery `Err` today falls back to v2 recovery, which discards the entire v3 replay — strictly worse — and shard init has no refuse-boot path, so fail-closed is a follow-up, not folded in here. - corrupt page, partial page, undecodable slot, short file: the bytes are gone; count and log, keep everything else in the file. - manifest fails to open (recovery.rs Phase 3): the consequence — no cold index at all — is now stated at error, not just Phase 2's open failure. Read side: `ColdReadOutcome::Unreadable(ColdReadFault)` separates "indexed but the bytes could not be produced" from `Miss`, which now means only "no index entry". `read_cold_entry` classifies each failure (file missing / unreadable, page rejected, slot undecodable, overflow broken, value undecodable), counts it (`reclamation_cold_read_unreadable_total`), and logs the location rate-limited. `promote_cold_outcome` promotes nothing, fabricates nothing, and keeps the index entry so a later read retries. The wire reply for a value read stays nil for now: `Database::get` has no error channel, and a dispatch-boundary flag would report an IOERR on a write that had already executed — the `-IOERR` reply with fail-closed writes is a follow-up rather than a half-wired one. Evidence: `tests/cold_index_rebuild_silent_drops_875.rs` drives a real spill → `BGREWRITEAOF` (so the cold file is the only copy, as on any server that has auto-rewritten) → `SIGKILL` → on-disk damage (one file removed, one chmod 000, one page CRC broken, one file cut mid-page) → restart. On the pre-fix binary (`a8eb2efc`) the four keys answer nil with nothing in `INFO` or the log; the test fails at the first counter lookup. Unit coverage for every loss class in `cold_index_rebuild_tests.rs`, and for the read-side split in `cold_read.rs`. Stacked on #996 (`fix/983-coldindex-duplicate-resolution`), which must merge first. Refs moon#875 author: Tin Dang * test(storage): strip ANSI escapes before matching cold-recovery log lines `tracing_subscriber::fmt()` writes colour codes even into a file, so `file_id=15212` arrives split by escape sequences; match on the stripped text. Test-only. Refs moon#875 author: Tin Dang * fix(storage): an indexed-but-unreadable cold key answers -IOERR, never nil A cold read-through that finds the key INDEXED but its bytes unreadable (file missing or unreadable, page corrupt, slot undecodable, overflow chain broken) used to be folded into the plain miss: `GET` answered nil, `INCR` minted a counter from zero, `APPEND`/`HSET`/`LPUSH`/… fabricated a fresh value that shadowed the cold copy until the orphan sweep reclaimed the file for good. "Key not found" is a legitimate answer a client acts on, so no layer above could tell the loss from a key never written — the read-side half of moon#875. Mechanism, kept off the accessor signatures: `Database::cold_fault` (`AtomicU8`; the `Database` lives in an `RwLock` slot and must stay `Sync`) is raised by the two read-through funnels — `promote_cold_outcome` on `ColdReadOutcome::Unreadable` and `get_cold_value` (the `&self` shared-read path) — and consumed by whoever answers the client: - the six fabricating accessors (`get_or_create*`, listpack/intset siblings) refuse with `Database::cold_fault_error()` BEFORE `insert_fresh`, so nothing shadows the cold bytes; - `INCR*`/`INCRBYFLOAT` (via `incr_absent` → the general path), `APPEND`, `SETRANGE`, `GETSET`, `SET … GET|KEEPTTL` refuse before their `set`; - `command::cold_fault_gate` on `dispatch` and `dispatch_read` turns any remaining reply into the error — the catch-all for every `Database::get`-shaped read, which has no error channel. One relaxed load per command when nothing is pending. `try_inline_dispatch` stands down on any cold location and cannot raise the flag, so the third path needs no gate. The reply is `-IOERR cold tier: key is indexed but its data could not be read (see server log)` (static bytes). `EXISTS`/`DBSIZE`/`TYPE` keep saying the key is present (index-driven), the index entry is retained so a later read heals, plain `SET` still overwrites (it never reads the old value) and `DEL` discards — the two escape hatches. Every raise happens inside the raising command's own execution and every dispatch takes the flag, so it cannot outlive its command on a shard thread. On the tokio shared-read path a second connection's `dispatch_read` can theoretically interleave between the async GET pre-warm's raise and this connection's read; the pre-warm's own re-read re-raises for the right connection, and the other one gets a spurious IOERR on a disk that is already failing — documented, not hidden. Evidence: `live_damage_answers_ioerr_not_nil_{1,4}_shards` tiers three keys, removes one file and chmods another under the RUNNING server, then: pre-fix `a8eb2efc` answers `$-1` for the indexed key (RED); fixed answers `-IOERR` for GET/APPEND/INCR, counts every read in `reclamation_cold_read_unreadable_total`, serves the untouched key, and after `chmod 644` serves the ORIGINAL value (the refused APPEND fabricated nothing). Dispatch-level unit tests cover both paths, eleven refusing writes, the PING-after-IOERR non-leak, and SET/DEL as escape hatches. Refs moon#875 author: Tin Dang * test(storage): XADD exercises the generic get_or_create cold-fault guard; CHANGELOG for the -IOERR reply An attack that inverted the guard at the generic `get_or_create::<K>` site stayed green: HSET/LPUSH/SADD/ZADD all take the listpack/intset siblings first, so that site was never reached by the test. Streams have no compact sibling, so XADD reaches it directly. Refs moon#875 author: Tin Dang * fix(ci): exempt every cfg(test)-gated module file from the unwrap audit, not only tests.rs scripts/audit-unwrap.sh skipped a test-only module file only when it was named exactly `tests.rs`. A test module split into its own file under any other name - `#[cfg(test)] mod cold_index_rebuild_tests;` - was audited as production code, so its 28 test unwraps failed the Lint job. The exemption now keys on the declaration, not the file name: `<stem>.rs` is skipped when its parent (`<dir>/mod.rs` or the 2018-style `<dir>.rs`) declares `mod <stem>;` directly under a cfg(test) attribute. `tests.rs` is the special case of that rule. A module that is not gated is still audited whatever it is called. Verified: audit 28 un-annotated -> 0 on this branch; controls - a non-gated `zz_tests.rs` and a gated-under-another-name sibling are both still flagged, a correctly gated `zz_tests.rs` is exempt. Refs #875 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.
Summary
Fixes moon#983:
ColdIndex::rebuild_from_manifest_per_dbresolved a key present in two Active KvLeaf files by manifest push order, notfile_idorder. Push order is not recency order — the async spill path pushes at completion-apply time, the durable batch path at eviction time, so aCONFIG SET appendonlyyes→no flip with a completion still in flight pushes a higherfile_idahead of a lower one, andgc_tombstones/compaction/reopen all preserve that order across restarts. The rebuild then pointed the key at the OLDER copy and cold read-through answered a stale value after recovery, with no error and no log line.The fix orders duplicates by
ColdLocation::recency_key()=(file_id, page_idx, slot_idx)explicitly at the point of use (from_pairs_newest_wins: sort key-asc/recency-desc, keep the first of each run). Manifest order now decides nothing.Ordering invariant — verified, not assumed
file_idis allocated at eviction time from a per-shard counter that only increases (next_file_id), re-seeded on restart strictly above everyheap-*.mpfon disk (next_spill_file_id_seed). A key can only be spilled again after it came back hot (write or read-through promotion), so its second copy always has a strictly higherfile_id.(page_idx, slot_idx)is the batch builder's packing order = request order (a key overwritten while its first request sat in the flush buffer lands twice in one file; later slot is newer).recency_key, not changed): the monotonicity is conditional on the seed scan succeeding; on anyread_direrror butNotFoundthe seed falls back to1with a warn. A reset counter re-mintsheap-000001.mpfand the batch writer renames over the existing file, so the older copy is destroyed in place (the pre-existing B-2 overwrite) before any ordering question arises —recency_keymakes no scenario worse than the status quo. A fail-closed startup refusal has no error path out of shard init today and is a separate behaviour change; flagged for follow-up rather than folded in here.Other readers of the same assumption (enumerated, nothing changed):
ColdIndex::merge(recovery merges per-db indexes into an existing one viainsert— different dbs, no overlap),ColdIndex::insertlive path (completion order == allocation order except the CONFIG-flip race, where the superseded completion is withdrawn by #459's in-flight check, so live is correct),replay_cold_spilled/cold_location_visible(equality/gating onfile_id, order-insensitive),cold_file_watermark_hint(usesmax_file_id, already treatsfile_idas the order),classify_orphan_heap_files/ sweeps (set membership).rebuild_from_manifest(merged wrapper) is test-only now.Evidence
Pre-fix control:
$HOME/ab856-target/release/moon(release build ofa8eb2efc). Fixed:release-fastbuild of this branch. macOS host; no benchmark numbers.Unit (
kv_spill::tests::test_983_…) — swappedorigin/main'scold_index.rsin:FAILED … left: Some(401) right: Some(402)(older file wins). Restored: passes.Integration (
tests/cold_index_duplicate_resolution_983.rs,MOON_BINpinned)newest_first_manifest…_1_shard…_4_shardsrespilled_key…sigkill_1_shard…_4_shardsa8eb2efcgot Some("stale-value-from-file-1")The lifecycle tests (
respilled_key…) pass on BOTH binaries by design: on the in-order path manifest order andfile_idorder agree, so they are the regression guard for the real spill → overwrite →SIGKILL→ restart cycle (they refuse a vacuous pass: two on-disk copies of the key, first file still present, before the crash), not the discriminator. The discriminator boots the real binary on the on-disk state the race leaves behind, with a private key in the older file proving that file is served at all.Attack
FAILED Some(401); rebuilt binary: both newest-first testsFAILED got Some("stale-value-from-file-1"). Restored.dedup_byremoved (rely onBTreeMapkeep-last): unitFAILED Some(401)— the explicit dedup is load-bearing. Restored;git diffvs HEAD empty.Gates (final tree
7c77c724):cargo fmt --checkexit 0 ·cargo clippy --all-targets -- -D warningsexit 0 ·cargo check --all-targets --no-default-features --features runtime-tokio,jemallocexit 0 ·kv_spill::tests+cold_index::tests36 passed · 983 suite 4/4 on the final rebuilt binary · adjacentcold_reconciliation_property_6601/1 on the same binary. Not run:scripts/ci-local.sh(VM legs), hosted matrix dispatch, Linux/io_uring.Out of scope, flagged
run_eviction_tickdoes not re-syncnext_file_idfrom thespill_file_idCell at entry, and ends withspill_file_id.set(*next_file_id). On tokio the 1 ms periodic arm (sync-in) and the eviction arm are separateselect!arms; on monoioevent_loop.rs:2329has an.await(snap.finalize_async) between the sync-in (2307) and the tick (2453). A handler-side eviction in that window can allocate the samefile_idthe tick then allocates again, and the trailingsetcan move the Cell backwards. Not reproduced; noted for a follow-up.--appendonly nothe per-connection write-path eviction plain-drops (documented fix(storage): policy-aware eviction fail-close for disk-offload without a durability backstop #273 policy) while an older copy of the key may still sit in an Active spill file; nothing tombstones it, so a restart can resurrect it via the rebuild. Same neighbourhood as Cold-reconciliation property test reports a stale value after SIGKILL — the signature is AOF tail loss, masked as FLAKY by nextest retries #965/Cold-index rebuild drops entries silently on three paths, and a missing cold entry reads as an absent key #875; not this fix.Refs moon#983