fix: backfill below the first head on an empty store - #609
Open
blacks1ne wants to merge 2 commits into
Open
Conversation
…n empty store (all node roles)
The global-frame poller's forward-fill guard
if config.forward_fill && last_frame > 0 && new_number > last_frame + 1 {
treats an empty store (last_frame == 0 — every fresh boot) as "nothing to
fill": the first successful head poll stores ONLY the head frame, fires
on_frame for it, and latches last_frame = head. Frames 1..head-1 are never
fetched, and no runtime path revisits frames below the cursor (the
record-gap scan runs at bootstrap only), so the hole is permanent and the
serial materializer wedges below it.
This is NOT archive-specific. master_node/archive_sync.rs sets
forward_fill: true for EVERY node role — deliberately, because regular
nodes need contiguity too: their app-shard storage attestations anchor
ρ_N to an exact global frame that must be present in the local clock
store ("anchored global frame unavailable for ρ_N" when it is not).
Regulars only shrink the window (gossip stores frames independently; a
far-behind boot state-jumps): a client whose store is empty at its first
head poll while gossip missed the early frames — boot-time partition,
late-forming mesh, gossip drops — hits the identical skip with the
identical consequences. Archives, with no gossip arm and no state-jump,
hit it deterministically.
Observed live in the devnet partition harness: an archive isolated while
frames 1-2 finalize returns NotFound for them forever, failing the
terminal committed-range safety check ("safety fetch incomplete — ready
nodes could not serve their committed range").
This commit adds an in-process repro and NO fix:
- frame_sync: extract the poller's fetch surface into PollerFrameSource
(impl'd by ArchiveClient) and split run_archive_poller into a thin mTLS
wrapper over run_archive_poller_with_connector, so tests can inject an
in-process source. The loop body is unchanged; the public signature and
the sole production caller (master_node/archive_sync.rs) are untouched.
- forward_fill_repro::empty_store_first_head_poll_must_backfill_full_history
(archive mode, gossip_freshness: None) drives the REAL poller loop over
a fresh Rocks store against a fake archive serving frames 1..=5 and
asserts contiguity — FAILS on this branch (only frame 5 is stored;
on_frame saw [5]).
- forward_fill_repro::client_mode_empty_store_hits_the_same_skip
(regular-node mode, gossip_freshness: Some with a quiet mesh) — FAILS
identically, proving the guard is role-independent.
- forward_fill_repro::seeded_store_gap_is_forward_filled is the passing
contrast: the same gap above a NON-empty cursor (store holds frame 1)
is filled correctly — the empty-store case is an inconsistency in the
guard, not a designed limitation of the loop.
Deleting `last_frame > 0 &&` from the guard turns both repros green
(verified locally), though a production fix likely wants a bounded
initial fill rather than an unconditional genesis-to-head crawl.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the bug @dazthecorgi reported and reproduced in QuilibriumNetwork#587; his repro commit is included unchanged. The forward-fill guard read if config.forward_fill && last_frame > 0 && new_number > last_frame + 1 so an empty store — every fresh boot — was treated as "nothing to fill". The first successful head poll stored ONLY the head and latched the cursor past everything below it, and nothing revisits frames under the cursor at runtime (the record-gap scan is bootstrap-only), so the hole was permanent and a serial materializer wedged beneath it. Not archive-specific: `forward_fill` is set for every node role, because regulars need contiguity too — their app-shard storage attestations anchor ρ_N to an exact global frame that must be present locally. Regulars just have a smaller window to hit it in. An empty store is a gap like any other, so the `last_frame > 0` arm is gone. Removing it alone would let a fresh archive at mainnet height attempt a genesis-to-head crawl inside one tick — an unbounded loop with no cancellation check — so the fill is now bounded to MAX_FORWARD_FILL_PER_TICK (512) frames per tick. A truncated chunk does NOT fall through to head processing: storing the head there would latch the cursor past the unfetched remainder and reintroduce the very hole this fixes. It resumes below the head on the next tick instead. The bound costs no throughput — each frame is a serial round-trip, so the achievable rate is far below the ceiling — and it makes the loop responsive to `cancel` during a long catch-up, which it was not before. Behaviour change worth noting: a regular node whose store is empty and whose state-jump does not complete now crawls up from genesis instead of skipping to head. That is the contiguity ρ_N requires, and the runtime far-behind rescue still attempts the jump first. Tests: @dazthecorgi's two repros fail before this change (on_frame sees only [5]) and pass after; his seeded-cursor contrast still passes. Adds `gap_wider_than_one_chunk_fills_contiguously_across_ticks` for the new chunk boundary. Full -p quil-rpc suite: 129/129. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
blacks1ne
force-pushed
the
blacks1ne/fix-poller-empty-store-backfill
branch
from
August 18, 2026 10:35
c2f9e51 to
6d24cdf
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.
An attempt to fix the bug @dazthecorgi found and reproduced in #587. His repro commit is included unchanged, so his two red tests are the proof here; this adds the fix he handed off.
Base:
932045a3The bug. The forward-fill guard read
config.forward_fill && last_frame > 0 && new_number > last_frame + 1, so an empty store — every fresh boot — was "nothing to fill". The first head poll stored only the head and latched the cursor past everything below it, and nothing revisits frames under the cursor at runtime (the record-gap scan is bootstrap-only), so the hole was permanent and a serial materializer wedged beneath it. Not archive-specific:forward_fillis on for every role, because regulars need contiguity too for their ρ_N storage anchors.The fix. An empty store is a gap like any other, so the
last_frame > 0arm is gone. Dropping it alone would let a fresh archive at mainnet height attempt a genesis-to-head crawl inside one tick — an unbounded loop with no cancellation check — so the fill is bounded to 512 frames per tick. A truncated chunk deliberately does not fall through to head processing: storing the head there would latch the cursor past the unfetched remainder and reintroduce the same hole. It resumes below the head next tick.The bound costs no throughput (each frame is a serial round-trip, so the real rate is far below the ceiling) and it makes the loop responsive to
cancelduring a long catch-up, which it was not before.Behaviour change worth flagging: a regular node with an empty store whose state-jump does not complete now crawls up from genesis instead of skipping to head. That is the contiguity ρ_N requires, and the runtime far-behind rescue still tries the jump first — but it is a real change for that path.
Tests. Daz's two repros fail before this change (
on_framesees only[5]) and pass after; his seeded-cursor contrast still passes. Two more added:gap_wider_than_one_chunk_fills_contiguously_across_ticks— pins the new chunk boundary.regular_node_far_behind_state_jumps_instead_of_crawling_from_genesis— pins the claim above rather than leaving it in prose: the rescue fires, nothing below the jump target is fetched, and the post-jump gap is still filled. It neededspawn_pollerto delegate to aspawn_poller_with_jump; Daz's call sites are untouched.Full
-p quil-rpc: 130/130.