fix: separate raw retention boundary from oldest held sample (#212) - #219
Draft
NikolayS wants to merge 1 commit into
Draft
fix: separate raw retention boundary from oldest held sample (#212)#219NikolayS wants to merge 1 commit into
NikolayS wants to merge 1 commit into
Conversation
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 #212 — the README's flagship drill hard-errors on any fresh install, and
ash.status().raw_retention_startis unusable as the planning boundary it is documented to be.Found by the pre-tag audit (#160). Draft on purpose: needs REV and your explicit approval.
The bug
ash._raw_retention_start()returnsmin(sample_ts)— the oldest sample currently held — but_raise_tie_retention()treats it as a retention boundary, andstatus()publishes it under aname documented for window planning. On a young install with the data 100% present:
Feed it
status().raw_retention_startand it still raises (the reader floorssinceto theminute before the guard runs). Follow the error's own printed advice and you get 0 rows — the
whole partial first minute discarded. With
ash.start()'s 1s default this fires 59/60 of the time.The fix
Splits the two conflated concepts rather than fudging one:
ash._raw_retention_boundary()(new) — the configured ring boundary,rotated_at - (num_partitions - 2) * rotation_period, floored to the minute, honouring an oldersample if one is still physically retained. Used by
status()and every tie guard.ash._raw_retention_start()(unchanged) — still the exact oldest held sample. Still used bysource selection and report attribution, where "what data exists" is the right question.
boundary is accepted.
Verification
RED against unpatched SQL — the real README:193 failure, reproduced:
GREEN, with exact-value assertions rather than "returns rows":
1 / 212212 / raw, metrics(0.03, 0.13, 0.13, 8.00, 100.00)status().raw_retention_startstraight back in → same eight backend-secondsrotate()truncates it, itscount is asserted to be exactly zero, and the old window raises SQLSTATE
P0001with the exactunrecoverable message. That is the control proving the guard was fixed, not disabled.
Fresh install, double re-apply, full 1.0→2.0 chain, and fresh-vs-chain schema/ACL equivalence all
pass on PG 18.3.
#129 must merge first, then this. Composition was tested against #129 head
2006159, notassumed.
The code cherry-picks with no textual conflict — but one of #129's own test assertions
becomes wrong once this lands. #129 asserts that
status().raw_retention_startequals the exactoldest retained sample; this PR redefines that field as the configured ring boundary. Whoever
merges must update that assertion to compute the boundary from
rotated_at,num_partitionsandrotation_period. Verified composed and green after exactly that change:This branch deliberately remains one clean commit on
37a0e2a; the composition was validated on ascratch branch and not included here.
Review note
raw_retention_startnow means two different things on two user-facing surfaces:status()reports the planning boundary, whilereport().coverage.raw_retention_startkeepsthe exact oldest sample (it describes attribution availability, not planning). Each choice is
defensible on its own; sharing the name is the wart. Worth renaming one before this ships —
report()'s payload contract is frozen per 2.0 minor line, so it is easier to change now than later.Related but distinct: #217 introduces a geometry constraint using
(num_partitions - 1). That isnot an inconsistency with the
- 2used here —-1bounds the physical age of rows in the slotabout to be truncated,
-2is the user-visible retention window. Both PRs would benefit from acomment saying so.
🤖 Generated with Claude Code
https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj