feat: automated backups with a tested restore (#379) - #611
Merged
Conversation
Backup was documented but not implemented: backup-restore.rst listed pg_dump and
mc mirror commands for a human to run by hand, with no schedule, no verification,
and no rehearsed restore. The leadership deck ranked losing imported data as the
top risk, and this was the last item behind that framing.
scripts/backup.sh
One timestamped archive holding all three things that cannot be regenerated:
the PostgreSQL dump (custom format, so pg_restore --clean works), the MinIO
objects, and signatures.yml from the config volume. Plus a manifest recording
what was captured.
scripts/restore.sh
Restores an archive, and verifies rather than assuming: it counts the tables
actually present afterwards and fails if the database came back empty. It
rejects corrupt archives, tarballs that aren't TracePcap backups, and warns
when the manifest's database name doesn't match the target.
systemd service + timer
Nightly at 02:30, Persistent=true so a powered-off night is caught up rather
than skipped, RandomizedDelaySec so several hosts don't hit the same NAS at
once. Cron equivalent documented.
Two properties that matter more than the happy path:
- Old backups are pruned only AFTER the new archive passes its size check and
a tar -tzf read-back, so a failing run can never destroy the last good one.
- Both scripts exit non-zero on failure, so cron and systemd surface a broken
backup instead of passing over it silently.
Credentials come from .env and are passed to `mc` inside the container rather
than interpolated into a shell command, following the fix from #604.
Validated end to end, not just executed: uploaded and analysed a PCAP, took a
backup, dropped the public schema and wiped the bucket entirely, then restored.
Row counts returned exactly (files=1 packets=15 conversations=11), 31 tables
came back, the API served the file list, and the recovered PCAP was
byte-identical to the original by MD5. Corrupt/foreign/missing archives were all
rejected, and retention pruning kept the current backups while dropping a
30-day-old one.
PITR is deliberately out of scope — WAL archiving is a poor trade for a
single-server deployment whose primary data is re-importable. RPO (24h) and RTO,
and what is NOT covered (PITR, Keycloak users, redundancy), are documented.
Refs #379
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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. Comment |
Addresses code review on #611. Two findings were genuine blockers, and one of them reintroduced on the host exactly the bug #604 fixed for mc. .env was SOURCED, not parsed (backup.sh, restore.sh) `set -a; source ./.env` executes the file as shell. Verified: a password containing $(...) RUNS as a command, and one containing a space aborts the script with `spaces: command not found` before any backup happens. .env is Compose's format, not bash. Now parsed line by line — no execution path. Sourcing also inverted precedence: .env clobbered the real environment, so the systemd unit's BACKUP_DIR was silently discarded and every nightly archive would land on local disk instead of the off-host storage the docs call the whole point. The caller's environment now wins. Failed config restore printed NOTHING and still reported success (restore.sh) `docker cp … && log "restored"` with no || branch: under set -e a failed command in that position neither prints nor exits, so signatures.yml could go missing while the operator was told "Restore complete." Also fixed: - Restore now inspects pg_restore stderr, separating real errors from the "does not exist" notices --clean always emits. Previously the exit code was ignored entirely and a half-restored database still passed the table check. - Bucket restore uses --remove, making it the replacement the prompt and docs promise rather than a merge that orphans objects the database can't see. - Restore compares the bucket against the manifest's object count, so a partial transfer fails instead of silently restoring some PCAPs. - Backup cross-checks captured objects against the bucket, so a wrong bucket name can't produce a "successful" backup containing no PCAPs. - Staging moved from /tmp into BACKUP_DIR: it holds an uncompressed copy of the whole capture set, and /tmp is tmpfs on many server images. - Pruning reads the timestamp in the filename, not mtime, which resets when archives are copied to a NAS and would let BACKUP_DIR grow unbounded. - flock guards concurrent runs (timer + manual) from truncating one archive. - Service: Restart=on-failure for boot-time catch-up runs that fire before the containers are ready, and the [Install] section removed so enabling the service alongside the timer can't add a backup on every boot. - .gitignore: anchor /backups/ to the repo root. Verified: hostile .env ($(...) and spaces) completes with no execution; env overrides .env; full destroy/restore cycle still returns exact row counts and a byte-identical PCAP; tampered manifest and wrong bucket both rejected; pruning removes an old-named archive with a fresh mtime; both systemd units pass systemd-analyze verify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NotYuSheng
added a commit
that referenced
this pull request
Jul 28, 2026
production-readiness.rst and security-audit.rst were project tracking rendered as documentation: a Stage 1 checklist with Open/Done/Partial columns, effort estimates in weeks, and ~60 issue references between them. That belongs in the issue tracker, not on a site whose other pages tell an operator how to run the thing — and it goes stale on every merge. Both were already wrong, listing backups and credential rotation as open after #611 and #595 shipped. The analysis is not lost: it lives in #364, #365 and #366 where it originated. Kept the operator-facing half of security-audit.rst — its "Deployment Requirements" list is real guidance, not status, so it moves to production-hardening.rst as an up-front checklist (issue references stripped, since each item is already documented in detail on that page). scalability.rst and storage-redundancy.rst stay. Those are reference material an operator needs — rows per GB, indexing, partitioning, the MinIO SPOF and its SNMD/MNMD upgrade paths — with almost no status language, and backup-restore.rst links to both. Verified: no dangling :doc: references remain and Sphinx builds with no new warnings. Co-authored-by: Claude Opus 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.
Problem
Backup was documented but not implemented.
backup-restore.rstlistedpg_dumpandmc mirrorcommands for a human to run by hand — no schedule, no verification, no rehearsed restore.The leadership deck ranked this as the top risk ("No automatic backups — a real risk of losing imported data, which matters most here"), and it was the last Stage 1 item still behind that framing.
What's here
scripts/backup.sh— one timestamped archive holding all three things that cannot be regenerated: the PostgreSQL dump (custom format, sopg_restore --cleanworks), the MinIO objects, andsignatures.ymlfrom the config volume, plus a manifest of what was captured.scripts/restore.sh— restores an archive and verifies rather than assuming: counts the tables actually present afterwards and fails if the database came back empty. Rejects corrupt archives and non-TracePcap tarballs, and warns when the manifest's database name doesn't match the target.systemd service + timer — nightly at 02:30.
Persistent=trueso a powered-off night is caught up rather than skipped;RandomizedDelaySecso several hosts don't hit the same NAS simultaneously. Cron equivalent documented.Two properties that matter more than the happy path
tar -tzfread-back — so a failing run can never destroy your last good backup.Credentials come from
.envand are passed tomcinside the container rather than interpolated into a shell command — following the fix from #604.Validation: actually destroyed the data
Not just "the script ran". I uploaded and analysed a PCAP, took a backup, then dropped the
publicschema and wiped the bucket entirely:files=1 packets=15 conversations=11HTTP 200, file list rendersScope decision: no PITR
The issue title says "with PITR". I've deliberately left WAL archiving out — it roughly doubles the effort and is a poor trade for a single-server deployment whose primary data is re-importable. Losing up to 24h of PCAP analysis is very different from losing a transactional ledger.
The real exposure is called out in the docs: human labels and overrides entered since the last backup cannot be recovered, unlike the PCAPs themselves.
Happy to add PITR as a follow-up if you'd rather have it — hence
Refsrather thanCloses.Also documented
RPO (24h) and RTO, a rehearsal procedure operators can follow, and an explicit "What Is Not Covered" section: PITR, Keycloak users (the
keycloak_datavolume isn't captured), and redundancy — backups protect against deletion, not hardware failure.The timing figures are labelled as method, not capacity: sub-second on a trivial dataset tells you there's no fixed overhead, not how long real data takes.
Refs #379
🤖 Generated with Claude Code