Skip to content

feat(backup): prove the dedicated bucket covers the stale shared Wedding catalogue - #4488

Merged
devantler merged 5 commits into
mainfrom
claude/remove-stale-wedding-shared-catalogue-4481
Oct 5, 2026
Merged

devantler merged 5 commits into
mainfrom
claude/remove-stale-wedding-shared-catalogue-4481

Conversation

@devantler

Copy link
Copy Markdown
Contributor

🤖 Generated by the Agentic Engineer

Why

An old copy of the Wedding database backups still sits in the shared backup bucket, where every other backup consumer can read it and nothing cleans it up any more. Removing it cannot be undone, so the decision needs fresh evidence that the dedicated Wedding bucket already holds everything in that copy.

What

Adds an on-demand, read-only check that compares the old shared copy with the dedicated bucket and reports whether every backup in the copy is safely held there. It changes nothing in either bucket. The removal itself is left as a separate, deliberate step for the maintainer.

Part of #4481

👉 After merge/promotion: once the check reports the copy as covered, the removal of the old shared copy is the maintainer's decision.

…ing catalogue

Removing the shared copy of the Wedding catalogue is irreversible, so the
decision needs current evidence that nothing would be lost. Add a read-only,
dispatch-only proof: one pod beside each credential lists its side, both hash
the objects an ETag cannot prove, and the reviewed evaluator requires every
shared object to be in the dedicated catalogue with matching content or to be
older than what the dedicated store's retention still keeps.

Part of #4481

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
devantler and others added 4 commits October 5, 2026 00:32
…have removed it

Review found that the tolerance for retention-pruned objects compared names
against whatever the dedicated bucket happened to hold, so WAL or a base backup
that was never copied passed as pruned. Judge by when the object was written
instead: before the oldest complete backup the dedicated store keeps and before
its 30-day retention window, and require the live store to declare that window.

Also refuse a non-empty object hashed as empty, refuse a store rooted above the
catalogue, redact bare hosts and addresses from pod errors, and report how many
objects were matched and how many were accepted as pruned.

Part of #4481

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…edates the window

Retention keeps the newest backup older than its window and everything written
since. While the oldest complete dedicated backup is still inside the window,
nothing has been pruned, so an older shared object missing there was never
copied. Require that backup to predate the window before accepting any missing
object, keep a clock margin against its start, refuse a backup ID that is not a
real time, and match a store rooted above the catalogue with a trailing slash.

Part of #4481

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Part of #4481

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 38 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository YAML (base), Organization UI (inherited)
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 61faf95b-8812-41b2-85c2-bd034ad807b2
📥 Commits

Reviewing files that changed from the base of the PR and between 460c0bf and 948fdc6.

📒 Files selected for processing (9)
  • .github/workflows/ci.yaml
  • .github/workflows/verify-wedding-shared-backup-coverage.yaml
  • docs/dr/velero-cnpg.md
  • scripts/mirror-wedding-backup-catalogue/coverage.go
  • scripts/mirror-wedding-backup-catalogue/coverage_test.go
  • scripts/mirror-wedding-backup-catalogue/main.go
  • scripts/tests/test-verify-wedding-shared-backup-coverage.sh
  • scripts/verify-wedding-shared-backup-coverage-pod.sh
  • scripts/verify-wedding-shared-backup-coverage.sh
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devantler devantler left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)

Reviewed commit: 948fdc63331e62ee9b211bea94ae97088413ecd6

  • CodeRabbit: rate limited — it refused the request for this head at 22:40Z ("Review limit reached").
  • Codex: usage limit since 2026-10-04T02:11Z.
  • Cursor Bugbot: usage limit since 2026-10-04T18:18Z; no review from it anywhere in the portfolio since.

Three independent reviewers read the change in turn. The first found a P0: a shared object missing from the dedicated bucket was accepted as pruned by comparing names against the dedicated bucket's own contents, so a genuinely missing object could pass. The second found a P0 in the repair: the pruning floor could sit inside the 30-day retention window. Both are fixed; a missing shared object is now accepted only when it was written before the oldest complete dedicated backup and that backup itself predates the retention window. The third reviewer read the whole branch at 50b0702c and found no P0/P1.

This head adds only a merge of main and one paragraph in docs/dr/velero-cnpg.md on top of 50b0702c. I read that delta myself: no file of the proof changed, and the paragraph matches what the workflow does (read-only, two pods, COVERED only on full content coverage or provable retention pruning).

The change only reads. The pod script is pinned by test to listing and reading commands, and nothing in this pull request removes anything from storage; the removal stays a maintainer step after a COVERED run.

Left open, none blocking: the EMPTY verdict has no positive control of its own, a short read is hashed as-is rather than refused, a half-present older dedicated backup is accepted as pruned, and a few runner-level failure cases (a re-check race, a pod exec failure) are not pinned by a test. Not exercised: the pod being admitted in umami; the first real run on main shows that.

Verdict: no P0/P1 findings

@devantler devantler left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)

Reviewed commit: 948fdc63331e62ee9b211bea94ae97088413ecd6

  • CodeRabbit: rate limited — it refused the request on this pull request at 22:40Z ("Review limit reached").
  • Codex: usage limit since 2026-10-04T02:11Z.
  • Cursor Bugbot: usage limit since 2026-10-04T18:18Z; no review from it on record since.

An independent reviewer read the change in four rounds, reproducing each finding against the evaluator rather than reasoning about it.

Round 1, at 397ead19, found one P0 and three P1, all in the allowance for backups the dedicated store has pruned by retention: it compared names against whatever the dedicated bucket happened to hold, so write-ahead log or a base backup that was never copied passed as pruned. Fixed in 4f876790: a missing object is judged by when it was written, a non-empty object hashed as empty is refused, and error output no longer leaks a bare host or address.

Round 2, at 4f876790, found one remaining P0: nothing required the oldest complete dedicated backup to predate the retention window, so an older backup missing from a bucket that had pruned nothing was still accepted. Fixed in 50b0702c: pruning is accepted only where that backup predates the 30-day window, with a one-hour clock margin, and a backup ID that is not a real time is refused.

Round 3, at 50b0702c, reproduced every fix and found no P0/P1. Round 4, at this head, confirmed the merge of main and the documentation commit changed none of the proof's own files and that the three suites pass.

Known and accepted, none above P2: the EMPTY result has no positive control (it prompts no deletion); a short non-empty read is not compared with the listed size; a half-present older dedicated backup is counted as pruned although it is evidence of a bad copy; the re-check after evaluation, a database replaced mid-run and a failed in-pod command fail closed on reading but have no runner-level test.

Not exercised: the pods against the real cluster and buckets. The first dispatch on main is that test, and it changes nothing in either bucket.

Verdict: no P0/P1 findings

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

Readiness at 948fdc63331e62ee9b211bea94ae97088413ecd6

  • Tested: all checks green at this head (25 passed, 8 skipped by path filter). The new suites run in CI: the evaluator tests, and the 33-case harness that drives the real runner, the real in-pod script and the real evaluator against stand-ins for the cluster and the bucket client.
  • Reviewed: CodeRabbit refused the request (rate limited), Codex and Bugbot are out of quota, so the review is the local round above — four independent passes, two real defects found and fixed, none left above P2.
  • Tried as a user: I ran the harness locally and read what an operator would see for each outcome — covered, covered with retention-pruned backups, not covered, empty, and every refusal. I also read the live cluster read-only: the Wedding namespace no longer holds the shared store or its credential projection, and the dedicated store declares the 30-day retention the proof requires. The proof itself cannot be run before it is on main; its first dispatch there is the remaining test and only reads.
  • Effect of merging: adds a dormant, confirmation-gated workflow. Nothing runs until it is dispatched, and it cannot write to or delete from either bucket.

@devantler
devantler marked this pull request as ready for review October 4, 2026 23:09
@devantler
devantler added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit b5fed1e Oct 5, 2026
33 checks passed
@devantler
devantler deleted the claude/remove-stale-wedding-shared-catalogue-4481 branch October 5, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

1 participant