persist: skip GC rollup invariant assert under active GC#37237
Open
DAlperin wants to merge 1 commit into
Open
Conversation
The soft-assert at the tail of `gc_and_truncate` ("earliest state fetched
during GC did not have corresponding rollup") checks that
`states.state()` references a rollup for the earliest fetched seqno. That
premise holds only when `states` was built from `fetch_all_live_states`,
where iterating to the end leaves `states.state()` at the genuine current
state.
Under active GC (`persist_gc_use_active_gc`, on by default in CI for
versions after v0.143.0-dev) GC instead fetches diffs only through
`seqno_since`, so `states.state()` is the reconstructed state at
`seqno_since`, not the current state. A rollup's entry is inserted into
the rollups map by the `add_rollup` transition at a seqno strictly
greater than the seqno it rolls up. When `seqno_since` precedes that
insertion, the reconstructed state legitimately lacks its own rollup
entry and the assert false-fires, aborting the persist worker in
instrumented builds (Antithesis, nightly). The check is a
`soft_assert_or_log!`, so production only logs to Sentry; the impact is a
recurring test blocker, not a customer-facing crash.
Gate the check to the classic path, where it is sound. The active-GC path
already validates the same invariant against fresh data when it resolves
the rollup for the earliest seqno, so the tripwire is preserved on the
classic path for any genuine regression.
Adds the datadriven regression test
`tests/machine/regression_gc_active_rollup_assert`, which reproduces the
panic under active GC on the unfixed code.
Fixes database-issues#10092
def-
approved these changes
Jun 23, 2026
Contributor
There was a problem hiding this comment.
Nightly triggered: https://buildkite.com/materialize/nightly/builds/16856
No complaints if green Edit: LGTM
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.
Summary
The soft-assert at the tail of
gc_and_truncate("earliest state fetched during GC did not have corresponding rollup") checks thatstates.state()references a rollup for the earliest fetched seqno. That premise holds only whenstateswas built fromfetch_all_live_states, where iterating to the end leavesstates.state()at the genuine current state.Under active GC (
persist_gc_use_active_gc, on by default in CI for versions after v0.143.0-dev) GC instead fetches diffs only throughseqno_since, sostates.state()is the reconstructed state atseqno_since, not the current state. A rollup's entry is inserted into the rollups map by theadd_rolluptransition at a seqno strictly greater than the seqno it rolls up. Whenseqno_sinceprecedes that insertion, the reconstructed state legitimately lacks its own rollup entry and the assert false-fires.The check is a
soft_assert_or_log!, so production only logs to Sentry — the impact is a recurring test blocker (Antithesis, nightly, Long Workload Replay), not a customer-facing crash. The PER-16add_rollupfix (#36740) addressed an adjacent but distinct invariant and did not stop this.Fix
Gate the check to the classic path, where it is sound. The active-GC path already validates the same invariant against fresh data when it resolves the rollup for the earliest seqno (the early-returns at the top of that branch), so the tripwire is preserved on the classic path for any genuine regression.
Tests
Adds the datadriven regression test
tests/machine/regression_gc_active_rollup_assert, which reproduces the panic under active GC on the unfixed code and passes with the fix.Fixes database-issues#10092, PER-10
🤖 Generated with Claude Code