fix: prevent rollup cleanup from permanently freezing rotation (#211) - #217
Draft
NikolayS wants to merge 1 commit into
Draft
fix: prevent rollup cleanup from permanently freezing rotation (#211)#217NikolayS wants to merge 1 commit into
NikolayS wants to merge 1 commit into
Conversation
Exclude expired raw minute groups from the pre-truncation completeness gate while preserving issue #81 protection inside rollup retention. Enforce raw/rollup retention geometry through config and rebuild validation, repairing unsafe existing installs non-destructively. Track consecutive failed rotation returns in ash.status() and reset the metric after a successful rotation. Add exact RED/GREEN coverage for the cleanup deadlock, recovery, retained unrolled groups, geometry validation, and failure visibility.
This was referenced Aug 3, 2026
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
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.
Fixes #211 — two of pg_ash's own scheduled jobs deadlock each other, permanently and silently.
Found by the pre-tag audit — see #160. Draft on purpose: needs REV and your explicit approval.
The bug
rollup_cleanup()deletes theash.rollup_1mrows thatrotate()'s pre-truncation completenessgate (#81) requires, and
rollup_minute()'s watermark only walks forward — so those minutes cannever be re-rolled and the gate can never be satisfied again.
…forever. Rotation stops, so every new sample piles into the frozen current slot until the volume
fills — exactly the failure the ring exists to prevent. Reachable on stock 30-day retention via
the README's own "increase raw retention" runbook (
rebuild_partitions(32,'yes')).Worst part: it is silent.
rotate()returns text on the failure path, so pg_cron recordsSUCCESS and
ash.status()showed nothing. Onlytime_since_rotationcrept up.Three parts
rollup_1m_retention_days. Rolling those up would only create rowsrollup_cleanup()deleteson its next pass, so there is nothing left to protect. Groups still inside retention keep the
full Critical bug: rollup/rotation can silently lose raw samples #81 check.
config_raw_rollup_geometry_checkconstraint:(num_partitions - 1) * rotation_period <= rollup_1m_retention_days * interval '1 day',enforced both as a table CHECK and in
rebuild_partitions(). A table CHECK is used becausethere is no supported setter for these values — direct
UPDATEis the interface.consecutive_rotate_failurescounter onash.config, surfaced inash.status(), incremented on every silentfailed:return and reset by a real rotation.Neutral
skipped:returns leave it alone.Verification
RED first against unpatched SQL, with exact-value assertions:
Then GREEN, including that the #81 gate still refuses to truncate when a group inside retention
is genuinely unrolled (the control that proves the fix didn't just disable the guard), and that the
counter goes
{1}→{2}across repeated failures and resets on success.Fresh install, full 1.0→2.0 chain, double re-apply, fresh-vs-chain schema equivalence, and a
zero-argument smoke of every reader all clean.
Two things I want a reviewer's eye on
(num_partitions - 1)is deliberately stricter than the(num_partitions - 2)retentionformula used everywhere else (
:2268,:2374,:2315,:2475, README:364 — and documentedby in-flight fix: correct admin documentation contracts #152). The
-1is correct here: it bounds the age of the oldest row in the slotabout to be truncated, not the user-visible retention window. But the two formulas now differ in
the same file with no comment saying why, so a later "consistency fix" to
-2would silentlyreintroduce this deadlock for the boundary slot. Worth adding that comment before merge.
rollup_1m_retention_daysraised to the minimum safe whole-day value so the constraint can beadded. It emits
raise warningfirst, and raising minute retention is the only non-destructivedirection (reducing
num_partitionswould drop raw data, and rejecting the installer would denyaffected systems the very fix they need) — but it does increase disk usage without asking.
🤖 Generated with Claude Code
https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj