fix(duckdb-service): replace fake weekly backup with a real one (for #232) - #268
Merged
Conversation
…one (for #232) The Sunday-03:00 job held the sole writer's global lock for the full duration of gzip-streaming the entire live DB into a temp dir, shipped it only as a Discord attachment (capped ~10 MB, far under prod's historical DB size), then deleted the temp dir either way — nothing was ever retained, and /data/images was never backed up at all. run_backup() now: - holds db.connection.lock only for a CHECKPOINT + raw file copy (not the gzip), with a disk-space pre-flight before the copy - gzips + sha256's + rotates outside the lock, retaining BACKUP_KEEP (default 4, floored at 1) generations under BACKUP_DIR - publishes the gz artifact atomically (temp name -> sha256 -> rename) so a hard kill mid-write can never corrupt or evict a good generation - sends Discord a text-only notification, never the DB file - warns (at boot and after each run) when neither DISCORD_WEBHOOK_URL nor a fresh off-host-sync heartbeat file is present See ADR-031 for the file-copy-vs-EXPORT-DATABASE decision and docs/11-risks-and-technical-debt for the incident writeup. A live restore drill was performed end-to-end on the dev stack (documented in production-deployment.md), stopping the service throughout to avoid the cross-process lock race a naive `docker compose exec` trigger would introduce. Deliberately out of scope, tracked in CLAUDE.md's priority queue: a real off-host sync destination (documented as a systemd/rsync template only) and an actual Step 0 run against production (which has no data yet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pin the thesis invariants (round 4, for #232) A fourth review pass found the "local-only" warning was silently dead in the recommended production configuration: _warn_if_local_only OR'd a configured DISCORD_WEBHOOK_URL with a fresh off-host heartbeat, but Discord only ever gets a text notification now, never the DB file, so having it configured says nothing about off-host redundancy. Removed that clause entirely. Also closed a real test-coverage gap: three rounds of review had pinned every peripheral constant (free-space factor, keep-clamps, heartbeat freshness) while the two invariants #232 actually exists to establish — the lock is held for checkpoint+copy, and gzip reads the copy rather than the live file — had zero test coverage. Removing the lock, or pointing gzip at DB_PATH directly, both left the suite green. Added tests for both, plus for _cleanup's exact argument list at each publish failure point, all mutation-verified before commit. Re-ran the restore drill with a mandatory `sha256sum -c` gate before the gunzip — the previous pass restored straight from the .gz without ever reading the sidecar, so the one integrity control ADR-031 credits the design with had never actually been exercised. Smaller fixes: BACKUP_DIR="" (present-but-blank, distinct from unset) now falls back to the default the same way BACKUP_KEEP already did; orphan .sha256 sidecars (crash between sidecar write and atomic rename) are now swept alongside the existing .tmp/.inprogress sweep; the off-host rsync template excludes in-flight markers so a sync mid-crash can't ship a partial file or abort the unit's later steps; a troubleshooting.md entry for the new free-space failure mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
The Sunday-03:00 backup job held the sole DB writer's global lock for the full duration of gzip-streaming the entire live DB into a temp dir, shipped the result only as a Discord attachment (capped ~10 MB, far under the DB's historical size), then deleted the temp dir either way — nothing was ever retained, off-host, or restore-tested, and
/data/imageswas never backed up at all.run_backup()now:db.connection.lockonly for aCHECKPOINT+ raw file copy (not the gzip), with a disk-space pre-flight (aborts safely rather than risk filling the volume mid-copy)BACKUP_KEEP(default 4, floored at 1) generations underBACKUP_DIRSee ADR-031 for the file-copy-vs-
EXPORT DATABASEdecision and chapter 11 for the incident writeup. The restore drill (stop → produce backup → stop → verify sha256 → restore → verify row counts) was performed live on the dev stack, not just described.Deliberately out of scope, tracked in
CLAUDE.md's priority queue: a real off-host sync destination (documented as a systemd/rsync template only — no real infra exists yet) and an actual Step 0 run against production (which has no data yet, confirmed directly rather than assumed).This went through four rounds of adversarial review; each round's fixes were mutation-tested (deliberately reintroducing the bug, confirming the new test catches it, then reverting) before moving on, rather than trusting the fix on inspection alone.
Test plan
cd duckdb-service && pytest tests/ -q— 292 passedruff check .— cleanmake check-citations/.husky/pre-pushchecks — all green (ran automatically on push)docker compose), including a mandatorysha256sum -cgate before the gunzip — row counts identical pre/post, service healthy after restart🤖 Generated with Claude Code