Repository navigation
cold-store-reaper v3: backup retention (prune-backups), get-backup, backup-d1, nightly + drill scripts - #965
Conversation
- prune-backups [--db] [--apply]: retention 30 daily / 12 weekly /
12 monthly as a union of newest-per-period over filename timestamps
(pure function of the folder listing); dry-run by default; --apply
trashes via files.update {trashed: true} (never deletes) and writes
one transfer_ledger row per file (direction 'trash', kind
'backup-prune'). Unparseable names are never pruned.
- get-backup <db> <name> <out>: exact-name lookup + full download,
reports size + sha256.
- backup-d1: deterministic SQL export of the D1 cold-store-manifest
database via the REST query API (shared _d1_endpoint/_d1_select
helpers extracted from sync-manifest), filed through put-backup.
- schema.sql: transfer_ledger CHECK gains the 'trash' direction;
existing manifests are migrated in db() by rebuilding the table
from the current definition. The D1 copy of the schema needs the
same one-line change before prune rows can sync.
Fake Drive gains files.update PATCH (trash), trashed-aware listings, name+parent file matching and full-media downloads; Fake D1 can answer SELECTs from a backing sqlite database. New coverage: - prune: hand-computed 37-file retention scenario (daily/weekly/ monthly union, reasons, unparseable untouched, already-trashed invisible), --apply trash calls + 'trash' ledger rows, 400-day history bounds, unknown-db rejection - get-backup: round-trip bytes + sha256, missing/unknown-db errors - backup-d1: export against the stubbed D1 loads back into sqlite byte-identical (incl. quote escaping and NULLs), token required - migration: a pre-v3 manifest (CHECK without 'trash') gains the trash direction with its rows preserved
nightly-backup.sh: backup-d1 -> prune-backups --apply -> sync-manifest -> status, aborting loudly on any failure. restore-drill.sh (DESIGN section 8): three pseudo-random cold objects rehydrated through the production Worker with sha256 verification, plus the newest d1 backup downloaded, loaded into in-memory sqlite and row-count-compared against live D1 (local manifest as fallback oracle). Per-item DRILL PASS/FAIL lines and a final verdict; non-zero exit on any failure. Both shellcheck-clean, chmod +x, secrets only via documented environment variables.
Documents prune-backups (30 daily / 12 weekly / 12 monthly union, trash-not-delete, unparseable names never pruned), get-backup, backup-d1, both operator scripts with their environment prerequisites, and the schema v1.1 'trash' caveat for the D1 copy.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if got != want: | ||
| ok = False |
There was a problem hiding this comment.
After a successful run of scripts/nightly-backup.sh, this comparison rejects a healthy backup: the script runs backup-d1 before sync-manifest, but uploading the export adds a new transfer_ledger row after the snapshot was taken. The subsequent sync therefore makes live D1 contain more ledger rows than the newest backup, and the local fallback includes that extra row too. Compare against counts or integrity metadata captured with the backup rather than against a subsequently modified database.
Useful? React with 👍 / 👎.
| if [[ ${#COLD_HASHES[@]} -eq 0 ]]; then | ||
| fail "no cold objects in the local manifest — nothing to rehydrate" | ||
| else | ||
| PICKED="$(printf '%s\n' "${COLD_HASHES[@]}" | shuf | head -n 3)" |
There was a problem hiding this comment.
With a sufficiently large cold-object manifest, head -n 3 closes the pipe while shuf is still writing, causing SIGPIPE. Because this script enables set -euo pipefail, the assignment terminates the entire drill before verification or a final verdict; this was reproduced with 10,000 hash lines and exit status 141. Local shuf --help documents -n, --head-count=COUNT as “output at most COUNT lines”; use that limit instead of piping unlimited output into head.
Useful? React with 👍 / 👎.
| "SELECT name FROM sqlite_master WHERE type='table'" | ||
| " AND name NOT LIKE 'sqlite_%' AND name NOT LIKE '_cf_%'" | ||
| " ORDER BY name" |
There was a problem hiding this comment.
For any manifest containing bundle members, the alphabetical table order emits members inserts before objects. Replaying the export into empty manifest tables with foreign-key enforcement enabled, as used by ColdStore.db(), fails with FOREIGN KEY constraint failed because members.object_hash references objects that have not yet been inserted. The restore drill misses this because its in-memory SQLite connection leaves foreign keys disabled. Emit tables in dependency order or explicitly defer constraints within a restore transaction.
Useful? React with 👍 / 👎.
| self.ledger_add( | ||
| conn, "trash", f["size"], "backup-prune", f["name"] | ||
| ) |
There was a problem hiding this comment.
If one backup is successfully trashed and a later PATCH fails, the enclosing connection transaction rolls back every earlier ledger insert even though the Drive trash operations cannot be rolled back. A retry excludes those already-trashed files, so their audit rows are permanently lost. This was reproduced with one successful PATCH followed by an HTTP 500, leaving zero ledger rows. Persist each successful action independently so a later failure does not erase the record of completed pruning.
Useful? React with 👍 / 👎.
| ACCOUNT = "234b866b99ce2b142d7963d8d139cbfa" | ||
| DATABASE = "15d5f460-835c-4f24-b229-222908a5b30c" |
There was a problem hiding this comment.
When the operator configures a different D1 account or database through the supported environment variables or state.json, backup-d1 and sync-manifest use that configuration, but this drill still queries the hardcoded production database. The request also ignores COLD_STORE_D1_BASE. Consequently, the drill either compares the restored backup against an unrelated database or silently falls back to the local manifest despite the configured D1 being reachable. Resolve the endpoint through the same configuration logic as ColdStore._d1_endpoint() so the drill verifies the database that actually produced the backup.
Useful? React with 👍 / 👎.
| BACKUP_NAME="$("${COLD_STORE}" list-backups --db d1 2>/dev/null | | ||
| sed -n 's/.*\(d1-[0-9]*T[0-9]*Z-[^ ]*\).*/\1/p' | head -n 1)" || true |
There was a problem hiding this comment.
For valid D1 dumps uploaded through put-backup, this expression can change the filename before passing it to exact-name get-backup: d1-20261026T000000Z-my dump.sql becomes d1-20261026T000000Z-my, while d1-20261026T000000Z-d1-20250101T000000Z-older.sql becomes only the inner dated name. Both transformations were reproduced with the script's exact expression; the installed GNU sed manual confirms that regex matching is greedy and “selects the longest.” The drill therefore fails to download an existing backup for these accepted basenames. Select the complete name from structured listing data rather than extracting a whitespace-delimited timestamp pattern.
Useful? React with 👍 / 👎.
Extends pkgs/cold-store-reaper (v1 #923, v2 #929, sync fix #933) with the DESIGN §5/§8 operator layer:
Commands
prune-backups [--db <db>] [--apply]— retention 30 daily / 12 weekly / 12 monthly per db-backups/ folder, computed as a pure function of the listing (filename timestamps, newest-first; no wall clock). Keep set = union of newest-per-day (30 distinct days) + newest-per-ISO-week (12) + newest-per-month (12). Unparseable names are never pruned (reported skipped). Dry-run by default;--applytrashes via files.update {trashed: true} — never a permanent delete — and writes one transfer_ledger row per file (directiontrash, kindbackup-prune).get-backup <db> <name> <out>— exact-name lookup, full download, prints size + sha256.backup-d1— deterministic SQL export of the D1 cold-store-manifest database via the REST query API (shared_d1_endpoint/_d1_selecthelpers refactored out of sync-manifest), filed through put-backup.Schema note: the
trashledger direction extends the transfer_ledger CHECK (schema.sql, marked v1.1). Existing local manifests are migrated in place on first open (table rebuild preserving rows, covered by a test). The D1 copy of the schema needs the same one-line change before prune rows can sync — until then sync-manifest surfaces D1's own error text. README documents this.Scripts (bash, shellcheck-clean, chmod +x, secrets via env only)
scripts/nightly-backup.sh— backup-d1 → prune-backups --apply → sync-manifest → status; fails loudly.scripts/restore-drill.sh— §8 quarterly drill: 3 pseudo-random cold objects fetched through the production rehydrator Worker with sha256 verification; newest d1 backup loaded into in-memory SQLite, per-table row counts compared against live D1 (local manifest fallback). Per-item DRILL PASS/FAIL + verdict, non-zero exit on failure. Failure paths smoke-tested locally.Tests: 26 passing (17 prior + 9 new), 1 skip (node worker-decoder, node_modules unavailable in sandbox — same as v2). Fake Drive gains PATCH/trash + trashed-aware listings + full-media downloads; Fake D1 can answer SELECTs from a backing sqlite DB. Highlights: hand-computed 37-file retention scenario with exact keep/prune sets and reasons; --apply trash calls + ledger rows; 400-day history bounds; backup-d1 export loads back byte-identical (quote escaping, NULLs); pre-v3 manifest migration.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit