fix(persistence): checkpoint publishes a redo point only over durable heap pages (#452)

[TinDang97]

Summary

This closes #452. Four of its five items were already fixed on main. This PR adds the one that was still open: item 3, the checkpoint data-file fsync. Two follow-up commits fix what adversarial reviews found in the first version. The first fixes four problems: the checkpoint could wedge, fsync helper threads could pile up, the WAL was fsynced once per page, and an FPI failure was not counted as a failed flush. The second stops the WAL-ceiling checkpoint from blocking a serving shard on the fsync. It also stops a failed control-file write from leaving an unpublished redo point in memory.

#452 per-item status on origin/main (checked in the code, not taken from the August notes)

#	Item	Status	Evidence on main
1	Appends dropped during a rewrite	Fixed (#454)	The RewriteOverflow spill buffer (src/persistence/aof/rewrite_overflow.rs: try_spill, finish_inner, arm_scoped) is armed in all six fold arms. Tests: pool_bounded_blocking_spills_instead_of_dropping_during_rewrite and the other rewrite_overflow::tests, plus the e2e tests/recovery_matrix_w1.rs::rewrite_under_pipelined_load_loses_no_acked_writes.
2	WAL v3 tear in the middle of the chain	Fixed (#454)	src/persistence/wal_v3/replay.rs: is_mid_chain_tear and replay_wal_v3_dir_until_with_salvage, override MOON_WAL_SALVAGE=1. Boot aborts through abort_boot_on_mid_chain_tear. Tests: test_v3_dir_mid_chain_tear_fails_loud_by_default, test_v3_dir_mid_chain_tear_salvage_continues.
3	Checkpoint data-file fsync	Open, fixed here	See below.
4	Degraded state after an append is dropped	Fixed (#454)	Sticky AOF_LAST_APPEND_OK latch (every drop goes through record_append_dropped), reported as INFO aof_last_append_status / aof_reason_del_dropped. Reason-DELs get the 500 ms bound, and the drop is counted and logged. "Never drop" was considered and rejected: a dead writer must not hang the shard, and a retry queue would replay the DEL before its SET. Tests: dropped_append_latches_degraded_status, record_reason_del_drop_is_counted_and_latches_degraded.
5	CLIENT TRACKING not invalidated on expiry or eviction	Fixed (#1013)	invalidate_server_removed is called from active and lazy expiry, eviction, kv_ops.rs and cold_index.rs. Tests: active_expiry_invalidates_tracking_clients, lazy_expiry_drain_invalidates_tracking_clients, plain_eviction_invalidates_tracking_clients, e2e tests/tracking_expiry_invalidation_1013.rs.

Item 3: what was wrong

handle_checkpoint_tick had three problems:

1. It never fsynced a heap file. FlushPages pwrote dirty pages. Finalize then wrote the control file, which makes redo_lsn the replay start, and recycled the WAL below that point. After a power loss the page could roll back, and the WAL that could have redone it was already gone.
2. The FullPageImage was written after the page, not before. The sweep collected every FPI and appended them only after the whole batch had been pwritten, into the WAL writer's memory buffer. A crash during the pwrite left a torn page with no image to repair it.
3. A page that failed to flush was counted as flushed. Finalize then published a redo point over a change that was in neither the heap nor the WAL.

Item 3: the fix

• Fsync before the redo point moves. CheckpointManager tracks the heap files each flush wrote, each with a write generation. Finalize fsyncs them before it appends the checkpoint record.
  • The fsync never blocks the shard thread. It runs on a helper thread (persistence::data_file_sync::DataFileSyncer, owned by the manager). Finalize starts a batch and returns "pending". Later ticks poll it with try_recv, the same off-loop pattern as the WAL's WalSyncAgent. The periodic tick has no recv_timeout, no sleep and no fsync.
  • At most one helper per shard. A second batch is refused while one is outstanding. On a hung disk, exactly one thread is stuck however many ticks ask. The helper is detached, so shutdown never waits on an fsync that does not return.
  • force_checkpoint has two contracts, now explicit (ForcedCheckpoint::{Shutdown, WalCeiling}). BGSAVE is not a caller; it only calls force_begin, and the periodic tick drives that checkpoint.
    • Shutdown (the tokio event loop at event_loop.rs ~1901, monoio ~2104): the shard serves no clients any more. It waits for the one outstanding helper, for at most WAIT_DURABLE_TIMEOUT counted from the helper's start, so a hung disk delays shutdown by 5 s once. Without this wait, the tick loop gives up before the fsync reports; a mutation test confirms it.
    • WalCeiling (the P6 trigger, maybe_force_checkpoint_on_wal_overflow): it runs on the shard thread while it serves clients, so it never waits on the fsync. When Finalize reports the fsync pending, it logs at info and returns at once, and the periodic tick finishes the checkpoint. A later ceiling trigger finds the checkpoint active and returns without waiting and without a second helper.
    • What the ceiling's emergency recycle cuts below. It uses control.last_checkpoint_lsn.min(control.graph_floor_lsn) from the in-memory control copy. With the checkpoint pending, that copy is unchanged, so the recycle stays below the published redo point. Reading this code turned up a bug that was already on main. Finalize set the in-memory copy before writing the control file, and left it set when the write failed. The recycle could then delete WAL above the redo point recovery would start from. Finalize now restores the published values when the write fails.
  • A result clears a file only for the generation it synced, so a write that lands after the batch was taken is never marked durable.
  • Failure classes: a file that exists but cannot be opened is retried with backoff. If an fsync returns an error, the helper poisons the shard permanently (fsyncgate: a retried fsync can report success for pages the kernel already dropped). The redo point stays, the WAL above it is kept, and recovery replays from there.
• A heap file that no longer exists does not hold the redo point back. Both sites that open the file handle this: the page write during the flush, and the fsync helper's open. On NotFound the checkpoint stops waiting on the file. It clears the dirty and FPI-pending bits of the file's cached pages; they stay cached, so reads the cache can serve keep working. It counts the file in VanishedDataFiles and logs it. It never re-creates the file. See the next section for why this is safe.
• Log before data, one WAL wait per batch. PageCache::flush_dirty_pages_with_fpi now appends the FPI of every page in the batch. It then calls the WAL barrier once, through the batch's highest page LSN and highest FPI LSN, and only then pwrites the pages. The order per page is unchanged, and each batch pays one WAL fsync instead of one per page. It returns a FlushOutcome { flushed, failed }.
• Any failed flush step makes the checkpoint flush again, keeping its redo_lsn. That covers a failed FPI append, a failed WAL barrier, and a failed page write. An FPI failure used to be dropped silently, and the checkpoint then published over the unwritten page. FlushOutcome::failed now counts it.

Why dropping a vanished heap file is safe

Before trusting "a file that no longer exists holds no data the redo point depends on", I checked every way a page of such a file could still be referenced. The full argument is in the doc comment of stop_waiting_on_vanished_heap_file (src/shard/checkpoint_heap_files.rs).

• Unlink sites. Heap files are unlinked in exactly two places:

  • ColdIndex::drain_pending_unlink deletes a file only once its live-reference count is zero, and re-checks that at drain time. It tombstones the manifest entry in the same ShardManifest that Finalize commits (step 3) before the control file publishes the redo point (step 4).
  • The boot orphan sweep (kv_spill::classify_orphan_heap_files) deletes only files the manifest never registered, which were never indexed.

  Neither can remove a file the cold index points into.

• Readers. Heap pages are read only through the cold index, by file_id. A zero-ref or unregistered file has no index entry, so no reader can reach it.

• Same name, new file. File ids come from one monotonic per-shard counter. It is seeded past every id on disk and in the manifest, and pinned by test_cold_file_id_counter_has_one_home_and_never_moves_back. A spill always writes a freshly minted id, so an in-flight spill cannot be the vanished file. An fsync by path cannot land on a different inode that has the same name.

• WAL. A page's FPI is appended after the checkpoint began, so it lies above the redo point and this checkpoint does not recycle it. Recovery replays it into a re-created file that no live manifest entry lists. Nothing reads that file; at worst a small file is left over (see Residuals). The page's own change record below the redo point is recycled. Nothing needs it, because the only file it applies to is gone and unreferenced.

• The case this does not cover. Something other than those two paths removed a file the manifest still lists as live: an operator, or disk tooling. Here a page was still referenced, and its keys are lost whatever the checkpoint does, because no retry can fsync the unlinked inode. Holding the redo point back would only grow the WAL until the disk is full, turning one lost file into a whole-shard outage. So the checkpoint stops waiting in this case too, but it logs an error and counts the file separately (VanishedDataFiles::still_registered). Cold reads of those keys report the file missing. They do not read as absent.

Reachability. PageCache::mark_dirty still has no production callers, so no heap page reaches this path today. This closes the hazards before the first in-place cold-page write can.

RED → GREEN

The RED run used head ee152dfa plus these tests and three test-only seams, with no fixes: a settable fsync fn on the manager, a thread-local FPI-failure hook, and a stubbed vanished_data_files() that returns zero. The RED result was the same on both runtimes: 7 passed, 6 failed. Failing assertions (monoio; the tokio lines are identical apart from thread ids):

test persistence::page_cache::tests::fpi_flush_batch_waits_for_wal_durability_once ... FAILED
  assertion `left == right` failed: one WAL durability wait per FPI page instead of one per batch:
  ["fpi:0", "wal:100", "write:0", "fpi:1", "wal:300", "write:1", "fpi:2", "wal:200", "write:2"]
    left: 3
   right: 1
test ...checkpoint_tick_tests::checkpoint_does_not_finalize_over_a_page_whose_full_page_image_failed ... FAILED
  Finalize published redo_lsn 2 over a page that was never written: its FullPageImage failed and the checkpoint did not re-flush it
test ...checkpoint_tick_tests::checkpoint_finalizes_when_a_written_heap_file_is_deleted_before_its_fsync ... FAILED
  checkpoint did not finalize within 4s: redo point still 0, wanted 2
test ...checkpoint_tick_tests::checkpoint_finalizes_when_a_dirty_pages_heap_file_is_deleted_before_the_flush ... FAILED
  checkpoint did not finalize within 4s: redo point still 0, wanted 2
test ...checkpoint_tick_tests::a_vanished_heap_file_the_manifest_still_lists_is_counted_as_unexpected ... FAILED
  checkpoint did not finalize within 4s: redo point still 0, wanted 2
test ...checkpoint_tick_tests::finalize_never_blocks_on_the_data_sync_and_never_runs_two_helpers ... FAILED
  a checkpoint tick blocked the shard thread for 5.016673s waiting on the data-file fsync, and 3 fsync helper
  thread(s) were started over 20 ticks (want: every tick returns at once, exactly 1 helper)
test result: FAILED. 7 passed; 6 failed

WAL-ceiling follow-up. The RED run used head 4a06517d plus the two tests below, with no fix. The only seam was a counter of blocking waits in the syncer. Both tests failed on both runtimes (tokio shown; monoio identical):

test ...checkpoint_tick_tests::the_wal_ceiling_trigger_never_waits_on_the_data_file_fsync ... FAILED
  the WAL-ceiling trigger blocked the serving shard thread on the heap data-file fsync (1 blocking wait(s)
  after the first trigger, 1 after the second); it must leave the checkpoint to the periodic tick
test ...checkpoint_tick_tests::a_failed_control_file_write_leaves_the_in_memory_redo_point_unpublished ... FAILED
  assertion `left == right` failed: the control file write failed, yet the in-memory copy claims redo_lsn 2 is published
    left: (2, 4, 5)
   right: (0, 0, 0)

The ceiling test is logical rather than timed, so CPU load cannot make it flaky. It counts blocking waits, and it releases the hung fsync only after both triggers have returned. It also asserts that the helper was still outstanding at that point, that helpers_started == 1, and that the in-memory and published redo points did not move. Finally, the periodic tick completes the checkpoint.

GREEN, after the fix and after merging main:

monoio: cargo test --lib -- checkpoint_tick_tests data_file_sync fpi_flush abandon_dirty unsynced_data_files -> 21 passed
tokio : same filter, --no-default-features --features runtime-tokio,jemalloc                             -> 21 passed
monoio: cargo test --lib -- persistence:: shard::persistence_tick                                       -> 699 passed
tokio : same filter                                                                                      -> 702 passed
monoio: cargo test --test moonstore_integration --test wal_v3_aggressive_recycle                        -> 7 + 4 passed

These tests are new, and the mechanisms they pin did not exist before: data_file_sync::tests::{outcomes_distinguish_synced_vanished_and_unopenable, an_fsync_error_poisons_and_stops_the_batch, a_second_batch_is_refused_while_one_is_outstanding, poll_is_non_blocking_and_wait_is_bounded_from_the_batch_start}, page_cache::tests::{fpi_flush_reports_every_page_it_did_not_write, abandon_dirty_pages_of_file_touches_only_that_file}, checkpoint_tick_tests::{a_heap_file_the_gc_tombstoned_and_unlinked_is_counted_as_retired, forced_checkpoint_completes_through_the_off_loop_data_sync, a_data_file_fsync_error_poisons_the_checkpoint}.

Mutation check. With the forced checkpoint's wait removed, forced_checkpoint_completes_through_the_off_loop_data_sync fails with assertion failed: !shard.checkpoint_mgr.is_active(). An earlier version of that test passed under the same mutation because the fsync finished too fast. It now uses a 400 ms fsync.

Changed tests. Two existing tests used to simulate "cannot open" by renaming the file away, which is NotFound: checkpoint_does_not_publish_redo_over_a_heap_file_it_cannot_fsync and checkpoint_does_not_finalize_over_a_page_that_failed_to_flush. Under the new semantics that means the file vanished. They now put a directory where the file was, so the open fails with EISDIR, or access denied on Windows, which is the retryable class. The two older FPI tick tests now tick at about 1 ms instead of spinning 100 times, because Finalize completes on a later tick.

Windows fix. Dispatch run 35438324468 failed Check (Windows 2/3) on this PR's own new test, the_wal_ceiling_trigger_never_waits_on_the_data_file_fsync, which panicked at std/src/time.rs:445. It computed Instant::now() - 1h, and a Windows Instant counts from boot, so the subtraction underflows on a CI host that has been up for less than an hour. The trigger's lag is 0 ms, so the test now passes Instant::now(). The other two failures in that job, channel_acl_holds_4_shards and test_ssm4a_aof_fold_cooperative, passed when nextest retried them and were reported as flaky. The single hard failure was this test.

Gates (head after merging main)

• cargo fmt --check
• scripts/audit-unwrap.sh, audit-unsafe.sh, audit-test-tempdirs.sh, audit-encoding-limits.sh
• cargo clippy --all-targets -- -D warnings (monoio)
• cargo clippy --all-targets --no-default-features --features runtime-tokio,jemalloc -- -D warnings
• cargo clippy --no-default-features --features runtime-tokio,graph,text-index -- -D warnings
• cargo doc --no-deps --lib: no warnings in the touched files
• The lib and integration tests above, on both runtimes
• The CHANGELOG entry under [Unreleased] / Fixed is updated to match

Merge of main through #1109 (0092c73)

Main moved by #1100 and #1109; neither touches the checkpoint path, and the only shared file (src/shard/event_loop.rs, where #1109 installs the per-shard pop log) merged cleanly. Gates on 0092c73: cargo fmt --check; the four audits; clippy -D warnings on monoio --all-targets, runtime-tokio,jemalloc --all-targets and runtime-tokio,graph,text-index; lib tests persistence::, shard::persistence_tick, shard::event_loop, blocking::: monoio 772 passed, tokio 775 passed; recovery_matrix_w1 and blocking_pop_owner_log_1056 pass against a release-fast monoio binary of the merged tree.

Merge of main through #1107 (f331b97)

Clean merge; only CHANGELOG.md is shared. Gates on f331b97: cargo fmt --check; the four audits; clippy -D warnings on the three configurations; lib tests persistence::, shard::persistence_tick, shard::event_loop, blocking::, tracking::: monoio 846 passed, tokio 849 passed.

Residuals, not fixed here

• Bounded WAL waits still run on the shard thread. WalWriterV3::wait_durable is a bounded condvar wait (5 s) on the shard thread. It was already on main, both for the log-before-data barrier and for Finalize step 2. It now runs once per flush batch instead of once per page. Making it non-blocking would mean carrying page snapshots across ticks, which is out of scope. It is the only blocking wait left in the non-shutdown paths this PR touches. The data-file fsync is waited on only at shutdown.
• A deleted file can come back as a small leftover. FPI replay in recovery opens heap files with create(true). An FPI above the redo point for a file the GC later unlinked re-creates a small file. No live manifest entry lists it, so nothing reads it. It is deleted at boot once its tombstone has been pruned from the manifest, and kept until then. This behaviour was already on main and this PR does not change it.
• The vanish counters are not exported yet. VanishedDataFiles lives on the manager and is logged. It is not yet in INFO or metrics.
• No new cost measurement. The per-batch WAL wait and the data-file fsync cost nothing today, because nothing dirties a heap page. I have not measured them.
• persistence_tick.rs is still over the size cap. It was 2576 lines on main and is 2730 here. This PR moves the checkpoint tests into src/shard/persistence_tick/checkpoint_tick_tests.rs, declared as a plain #[cfg(test)] mod so scripts/audit-unwrap.sh treats it as test code, and the Finalize durability step into src/shard/checkpoint_heap_files.rs. A full directory split would be a large move that other open PRs conflict with, so it is left for a separate change.
• No kill-9 end-to-end test. No production path dirties a heap page, so a spawned server never reaches this code. The contract is pinned at the level of the tick function instead.

Follow-up carried out of #452

The AOF_LAST_APPEND_OK doc comment names "reset the latch after a clean rewrite" as follow-up work "tracked in #452". That item is not one of #452's five, and this PR does not do it. It is now tracked in #1094, so closing #452 does not orphan it.

Fixes #452

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[coderabbitai]

Warning

Review limit reached

Next included review available in 54 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 4ec55a57-f986-4b18-96bb-badfe495f363

📥 Commits

Reviewing files that changed from the base of the PR and between b7204a0 and be1b4a7.

📒 Files selected for processing (10)

• CHANGELOG.md
• src/persistence/checkpoint.rs
• src/persistence/data_file_sync.rs
• src/persistence/mod.rs
• src/persistence/page_cache/mod.rs
• src/shard/checkpoint_heap_files.rs
• src/shard/event_loop.rs
• src/shard/mod.rs
• src/shard/persistence_tick.rs
• src/shard/persistence_tick/checkpoint_tick_tests.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}