Repository navigation
cmd/prune: billing_event + io_log retention pruner (ruled shape A) - #59
Merged
Conversation
Implements the 2026-09-30 retention rulings (rulings.md queue #5 + all follow-ons) on the append-only evidence ledger: - billing_event pruned on created_at at 30 days default, configurable, HARD FLOOR 7 days: below the floor a later rater re-rate can reconcile-DELETE previously billed rated_usage rows. ONE horizon for every row class - withheld/invalid rows prune the same as clean rows; the pruner deliberately does not reimplement the rater's withholding logic (a second copy of money logic that can silently diverge). - io_log pruned on created_at at its OWN 7-day period - the retention job the 0003 migration index always promised. - Shape A by ruling: batched ctid-targeted DELETEs looping to zero. No schema change, no partition; the table, the PK, and the drainer's ON CONFLICT (request_id) dedup stay untouched. rated_usage is never referenced. Archive out of scope by ruling. - Idempotent reruns (cutoff truncated to the second); exit 0 ok / 1 fatal; floors refused, not clamped. Integration tests on live Postgres (isolated schema, real migrations): cutoff selectivity, batch loop, floor refusal, rerun no-op, io_log's own period, and an EXPLAIN pin that the predicate uses billing_event_created_at_ix / io_log_created_at_ix at table scale. Docs per the docs ruling: migrations/README.md states the finite retention; billing-reconciliation.md amends the repair procedure (raw evidence past the horizon is gone by design; the manager rollup and WAL are the durable records).
All 14 round-1 findings confirmed (two reviewers independently reproduced the batchSize semantics; the EXPLAIN gap, timeout gap, missing-file divergence, and coverage holes verified by direct read/execution): - Store.Prune now refuses batchSize < 1 before any query (0 made LIMIT 0 a silent no-op reported as success; negative made LIMIT -1 unlimited, defeating batching). Prune's doc comment states the explicit-days contract: zero/negative is an error at the store seam; Config.WithDefaults is the single defaulting layer. - Missing settings file tolerated, mirroring the rater/drainer house idiom (ruled defaults need no config); other read/parse errors stay fatal. - Each table's prune runs under a 30-minute context; an interrupted table is named in the log. Continue-past-failed-table and exit-1-if-any-failed unchanged. - run() extracted from main (config path + env injection) so the exit-code contract is testable. - pruneQuery() extracted so the index test EXPLAINs the exact production DELETE (asserts Tid Scan + created_at index + no Seq Scan) instead of a copy of the inner SELECT. - New coverage: sqlmock loop/floor/batch-error/partial-total/guard-order tests; negative-horizon Validate cases; boundary selectivity at cutoff±2s (deterministic given second truncation); a concurrent-insert test pinning the ctid-snapshot safety (2000 old rows pruned while 50 fresh rows insert - all fresh survive); the harness applies ALL migrations from the FS (no stale hardcoded list); cmd-level integration tests for continue-past-failed-table (exit 1, io_log still pruned), missing-config/no-DSN, and below-floor refusal before any DB open. Full gate green: build, vet, unit, race, and the complete integration lane on the isolated container.
…d, harness isolation All 10 round-1 fixes were independently verified by the round-2 reviewers (incl. -race and determinism analysis). Round-2 findings, all confirmed: - real bug: cancel() ran before the ctx.Err() check, so EVERY failed table was logged as a per-table timeout (cancel always sets ctx.Err non-nil). The context error is captured before cancel; the timeout form logs only on context.DeadlineExceeded; the cmd test asserts the missing-table failure is NOT reported as a timeout. - no upper bound on retentionDays: time.Duration(retentionDays)*24h overflows past ~106752 days, wrapping the cutoff into the future so created_at < cutoff matches the WHOLE table — one run would wipe billing_event silently at exit 0. Config.Validate now enforces MaxBillingEventRetentionDays 36500 / MaxIoLogRetentionDays 3650 with unit cases at and past the bounds. - the DB ping is bounded at 10s (PingContext), mirroring the rater/drainer house idiom; an unbounded ping could hang the CronJob past the next tick against a blackholed host. - the two DB-free cmd tests moved to an untagged main_test.go so the always-on make test gate exercises run()'s config/validation/exit-code paths. - integration harnesses use a per-process schema name (os.Getpid suffix) so concurrent test runs against a shared database stop stomping each other (demonstrated live by two reviewers). - CREATE EXTENSION btree_gist is serialized with a phoebe-test advisory lock: IF NOT EXISTS is not atomic and the two parallel package test binaries raced on a fresh database (reproduced 2/2 as 23505); proven fixed by dropping the extension and re-running the concurrent pair. Full gate green: build, vet, unit, race, the complete integration lane, and two CONCURRENT integration runs (plus a fresh-database extension re-proof).
… ceiling at both seams All six round-2 fixes were independently verified by the round-3 reviewers (wrapping proven against real pgx, bounds arithmetic checked, moved tests run in the plain lane, advisory-lock paths reviewed). Round-3 findings: - the advisory lock only serialized prune-vs-prune: internal/e2e still created btree_gist unlocked, so the proven 23505 cross-package race survived on fresh databases. Since NO migration requires the extension (re-verified on the container), the prune harnesses now do not create it at all — e2e is the sole creator and there is nothing to race with. Proven: two concurrent fresh-DB lanes, zero 23505, exactly one extension afterwards. (The full-lane concurrent run still trips PRE-EXISTING fixed-schema stomps in internal/rating and internal/e2e — 39+7 hardcoded schema sites in packages this diff does not touch; CI runs one lane per job so it never bites CI. Tracked as a separate-batch item.) - Store.Prune now mirrors the retention ceiling (the floor was already duplicated at this seam); both guard layers agree, and a direct library call past 106751 days cannot overflow into a future cutoff. - positive timeout-attribution coverage: timeoutForm(err, ctxErr) extracted and both branches pinned in unit tests (raw/wrapped/pre-cancel deadline -> timeout form; missing-table/conn-reset/cancelled -> plain), plus an integration assertion that store.Prune with an expired context returns an error wrapping context.DeadlineExceeded through the %w wrap. Full gate green: build, vet, unit, race, the complete integration lane.
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
1 participant
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.
What
Implements the 2026-09-30 retention rulings (
rulings.mdqueue #5 + all four follow-ons — every design decision ruled, verbatim) for the append-only evidence ledger. This is the follow-up to the design proposal in saturn-architecture-docs (agent-wip/billing-event-retention-proposal.md, contractcontracts/billing-event-ledger.md); nothing destructive landed before the ruling.The ruled policy, as implemented
billing_eventis pruned oncreated_atat a 30-day default horizon, configurable per install, hard floor 7 days — below the floor a later rater re-rate can reconcile-DELETE previously billedrated_usagerows. ONE horizon for every row class: withheld/invalid rows prune on the same predicate as clean rows; the pruner deliberately does NOT reimplement the rater's withholding logic. Floors are refused, not clamped.io_logis pruned oncreated_atat its own 7-day period — the retention job the 0003 migration's index always promised.ON CONFLICT (request_id)dedup stay untouched.rated_usage(money) is never referenced. Archive out of scope by ruling — the manager's money rollup outlives the trailing window.Verification
billing_event_created_at_ix/io_log_created_at_ixat 20k-row scale.go build,go vet, unit, race (proxy/waker), and the complete integration lane (make integration-testagainst my own isolated Postgres container — e2e, iolog, prune, rating all ok).Docs (per the docs ruling, verbatim: "the docs are wrong, please fix")
migrations/README.md: states the finite retention on both tables.docs/billing-reconciliation.md: the repair procedure now says raw evidence past the horizon is gone by design (manager rollup + WAL are the durable records);rated_usageis never pruned.Deploy note
The pruner binary ships in the image (
phoebe-prune) but does nothing until the chart CronJob (saturn-k8s follow-up PR) is installed AND unsuspended.Battery
Tier 1, 4 rounds, 24 findings (0 critical / 0 high), all terminal (FIX_VERIFIED or evidenced REFUTED); gate green on the final snapshot (full integration lane on live Postgres); CI green on the final head. Verdict: DRY. Report:
saturn-architecture-docs/agent-wip/battery-reports/battery-59-prune-phoebe.md.Round content highlights: a real timeout-attribution bug in the new code (cancel-before-check marked every failure as a timeout); a missing retention ceiling that could have overflowed the cutoff into the future and wiped a table at exit 0 (bounded at both guard seams now); test-harness hardening (per-process schemas, the btree_gist catalog race removed by construction — no migration needs the extension). One non-material log-only nit (a plain failure microseconds before the deadline could misattribute one log line; exit code correct either way) is an evidenced defer with an optional follow-up noted in the report.