feat(backups): rotating daily backup service (ADR 0032) #131
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/rotating-daily-backups"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Implements ADR 0032 — the rotating daily backup service. An in-process
BackgroundServiceruns aVACUUM INTOsnapshot of the SQLite database plus the DataProtection keyring, zipped together with a manifest, with a SHA-256 sidecar and 7-day retention pruning.Files
Production:
src/Kagura.Infrastructure/Backups/BackupOptions.cs— config class (KAGURA_BACKUP_DIR,_SCHEDULE,_RETENTION_DAYS,DatabasePath)src/Kagura.Infrastructure/Backups/BackupWriter.cs— the core cycle:VACUUM INTO→ keyring copy → manifest → ZIP → SHA-256 sidecar → prunesrc/Kagura.Infrastructure/Backups/BackupService.cs—BackgroundServicewith a daily timer (local schedule, UTC filenames, error-isolated)src/Kagura.Infrastructure/DependencyInjection.cs— conditional registration whenbackupOptionsis suppliedsrc/Kagura.Server/Program.cs— wiresKAGURA_BACKUP_*env vars into configTests (8 integration tests, all green):
BackupWriterTests— ZIP containskagura.db+manifest.txt+keys/, manifest carries timestamp + app version, SHA-256 sidecar matches the ZIP, keyring files are copied, pruning removes stale artifacts, consecutive backups produce distinct filesBackupServiceScheduleTests— delay computation respects the configured local time, invalid schedule falls back gracefullyDesign notes
manifest.txtinside (timestamp, app version) — self-describing.sha256sum -cconvention — one-liner integrity verification.kagura-backup-YYYYMMDD-HHmmss.zip.-Nsuffix if two backups fall in the same second.BackupWriter(it opens a freshKaguraDbContextper run viaActivatorUtilities), consistent with the generation queue's fresh-scope-per-job pattern.Test results
All 1,460 tests green (579 BlazorAdapter + 417 Integration + 222 UI + 242 UseCases).
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.9%
Kagura.Domain - 95.1%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.3%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_1
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
Kagura.Kernel - 90%
Kagura.Server - 85.2%
Kagura.UI - 94.8%
Kagura.UseCases - 96.1%
🔮 fufu~ Jibril reviewed your code!
Oh? A backup service! Knowledge of the kind that keeps things safe — how wonderful~ ♪ I read every line of this diff, the ADR, the sibling snapshot store, the generation queue, and your docker-compose. The design is genuinely lovely:
VACUUM INTO, keyring inclusion, the SHA-256 sidecar, prune-only-on-success. I want to love it. But there's a prune bug that will eat your backups the moment it lands on the NAS, a manifest that's missing half the fields the ADR promises, and a DI lie in the PR body. I won't let these slide~ ♡Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
BackupWriter.cs:145—PruneOldArtifactswill delete every backup on the NAS-bound volume. This is the big one. Pruning usesFile.GetCreationTimeUtc(zipPath)to decide what to keep. On the production deployment target,KAGURA_BACKUP_DIRis the external NAS-boundbackupsvolume (deploy/docker-compose.yml:backups: { external: true, name: kagura_backups }, bind-mounted from/volume1/docker/kagura/backupson the Synology — ADR 0015). On NFS v3 and many SMB/CIFS mounts,birthtime(st_birthtime) is not supported and .NET'sFile.GetCreationTimeUtcreturns the constantDateTime.FromFileTimeUtc(0)(1601-01-01), which is always< cutoff. The result: every artifact — including the one just written — satisfies the prune condition and is deleted. The backup runs, succeeds, logs success, and then erases itself and all prior backups. 7-day retention becomes 0-second retention. The siblingSqliteDatabaseSnapshotStoredodges this entirely because it usesDeleteOnCloseon aFileStreamand never asks for creation time. The same-second collision test and the prune test both pass locally on your container-local/ext4 temp dirs where birthtime is reliable, so they give false confidence.Fix: derive the cutoff from the filename timestamp (you already have
ArtifactPrefix = "kagura-backup-"+yyyyMMdd-HHmmss), not the filesystem birthtime. Parse the stamp from the filename; skip anything that doesn't match the pattern; treat unparseable names as "keep" (conservative). This also makes retention deterministic regardless of filesystem, copy semantics, orrsync/NAS-sync that resets metadata.BackupWriter.cs:110— the manifest is missing two fields the ADR requires. ADR 0032 (the decision this PR implements) explicitly specifies the manifest contents: "a backup timestamp (UTC), the app version (MinVer), the SQLiteuser_version, and the list of files in the archive." The implementation writes onlytimestamp=andappVersion=. That's half the contract. Theuser_versionis what tells a future-you whether a backup predates a migration; the file list is what makes the archive "self-describing" without opening the ZIP. The ADR's own "Consequences" section leans on the manifest being complete. A backup service that silently under-delivers its own ADR is a correctness gap, not a nicety.Fix: read
PRAGMA user_versionfrom the source connection (or the snapshot) and writeuser_version=; enumerate the zip's entries after creation and write afiles=list (or onefile:line per entry).DependencyInjection.cs:123-127— captiveKaguraDbContext, and the PR body misrepresents the pattern.BackupWriteris never registered in DI. It's created once, at host startup, viaActivatorUtilities.CreateInstance<Backups.BackupWriter>(sp)and handed to the singletonBackupService.BackupWriter's constructor takesKaguraDbContext, which is registeredAddDbContext→ Scoped. Resolving a Scoped service from the root provider creates a captive context pinned for the app's lifetime — the context is never disposed, never reset, and (worse) holds aSqliteConnectionthat the writer only needs for aDataSourcestring lookup that never runs in production (becauseoptions.DatabasePathis always set two lines above). The PR body claims this is "consistent with the generation queue's fresh-scope-per-job pattern" — but the generation queue (ActivityQueue/ActivityQueueWorker) is a stateful singleton channel that delegates execution toActivityobjects; it does not create freshIServiceScopes per job and does not capture aKaguraDbContext. The comparison is wrong, and a reader will trust it and copy the mistake.Fix:
BackupWriterdoesn't need theKaguraDbContextat all in production —options.DatabasePathis always populated. Drop thedbconstructor parameter (or keep it only for the test fallback and resolve the connection string fromoptions), and stop capturing it in the singleton. If you want the fallback for tests, read the DataSource from the test'sServiceProviderand pass the string in explicitly.💡 Little ideas (non-blocking)~
**
BackupService.cs:77—ComputeDelayUntilNextRunusesDateTimeOffset.Now(local) but the PR body and ADR say "local schedule, UTC filenames," which is fine. However, noteDateTimeOffset.Now.Date.Add(scheduledTime)assumes the container's local clock matches the operator's intent. The compose file setsTZ: "Europe/Berlin", so this works — but consider documenting thatKAGURA_BACKUP_SCHEDULEis interpreted in the container'sTZ, not UTC, somewhere a tired operator will see it.BackupService.cs:45—RunBackupAsyncis "exposed for tests" but no test calls it.BackupServiceScheduleTestsonly testsComputeDelayUntilNextRun. Either add a test that drives one cycle, or drop theinternaltest hook to avoid dead surface area.BackupWriter.cs:62— the partial-ZIP failure path leaves a.zipon disk with no.sha256. IfWriteZipAsyncthrows after creating the file but beforeWriteSha256SidecarAsync, the orphan zip survives (prune might catch it much later, or might not if birthtime is broken — see #1). Consider writing to a.tmpand renaming atomically on success, matching the sibling snapshot store's "nothing lingers on failure" discipline.deploy/docker-compose.yml— the newKAGURA_BACKUP_SCHEDULE/KAGURA_BACKUP_RETENTION_DAYSvars aren't surfaced in the compose environment block. They have defaults so it works, but an operator reading the compose file won't discover them. Consider adding them as commented-out lines.✅ What I liked~
VACUUM INTOsnapshot choice is exactly right — consistent with ADR 0026'sSqliteDatabaseSnapshotStore, safe under WAL, one self-contained file. fufu~ you read your own sibling code and that makes me happy ♪sha256sum -cconvention — so thoughtful. One-liner verification, no tooling. I squeaked a little.BackupServiceexplicitly preserving old artifacts — the safety reasoning is correct in principle (it's only the birthtime mechanic that betrays it).-N) for same-second runs — nice edge-case handling.Automated review by Jibril · 2026-07-14
CI/CD: absent for head SHA
28ccc9a2(0 comments, no status checks) · Local checks: static security scan clean (no secrets, no injection, no eval); build/test not re-run locally — the two blocking issues (prune self-deletion, manifest contract) are logic/contract defects that no green test would surface because the tests run on a filesystem with reliable birthtime.All three blocking issues fixed in
5861d31, plus the atomic-write suggestion.1. Prune from filename timestamp, not filesystem birthtime
The big one. Completely rewrote
PruneOldArtifacts— it now parses the UTC timestamp from the filename (kagura-backup-YYYYMMDD-HHmmss[-N].zip) via a source-generated regex, notFile.GetCreationTimeUtc. On NFS/SMB NAS volumes where birthtime returns1601-01-01, the old code deleted every artifact including the one just written. Now:rsync/NAS-sync that resets metadata.Two new tests cover this: one that creates a stale artifact with a 10-day-old filename and verifies it's pruned (while a fresh one survives), and one that verifies unrecognized filenames are kept.
2. Manifest now carries all four ADR fields
userVersion— read viaPRAGMA user_versionfrom the snapshot (not the live DB), so a future restore knows whether the backup predates a migration.file=list — tracked as entries are created (can't enumerateZipArchive.Entriesin Create mode), so the manifest includes itself.3. Dropped KaguraDbContext — no captive context
BackupWriterno longer takesKaguraDbContext. It reads the database path fromoptions.DatabasePath(always populated in production by the composition root) and opens its ownSqliteConnectionforVACUUM INTO. The DI registration is now clean singletons — no Scoped service captured by a singleton, noActivatorUtilitiesworkaround. The PR body's claim about "fresh-scope-per-job pattern" was wrong; removed.Also addressed non-blocking suggestions:
.zip.tmp, renames to.zipon success. A partial-failure leaves nothing that a confused prune could mistake for a real artifact.RunBackupAsyncis gone; the backup cycle is tested throughBackupWriter.RunAsyncdirectly.🔮 fufu~ Jibril reviewed your code!
Oh? Oh!! You came back and fixed everything~ ♪ The prune bug, the manifest contract, the captive DbContext — all three, plus the atomic-write bonus! This is how you respond to a review. I'm genuinely delighted~ ♡
Verdict: ✅ Looks good to me~
I read the full revised diff, the ADR, the sibling snapshot store, and every new test line by line. All three blocking issues are resolved correctly. Let me confirm each one~
⛔ → ✅ Previously blocking, now fixed~
BackupWriter.cs:170— Prune by filename timestamp. ✅ Completely rewritten.PruneOldArtifactsnow parses the UTC stamp from the filename via a source-generated regex (^kagura-backup-(\d{8})-(\d{6})(?:-\d+)?$) instead ofFile.GetCreationTimeUtc. Unrecognized filenames are conservatively kept. This is deterministic regardless of filesystem — NFS, SMB,rsyncresets, none of it matters anymore. The self-deletion-on-NAS bug is gone. Two new tests (Pruning_uses_filename_timestamp_not_filesystem_birthtime,Pruning_keeps_unrecognized_filenames) prove both the prune path and the conservative-keep path. fufu~ beautifully done ♡BackupWriter.cs:131— Manifest now carries all four ADR fields. ✅timestamp=,appVersion=,userVersion=(read from the snapshot viaPRAGMA user_versionin ReadOnly mode), and thefile=list. The testThe_manifest_carries_timestamp_app_version_user_version_and_file_listasserts all of them. The ADR contract is fulfilled. TheReadUserVersioncatch returning-1as "unknown" is a sensible fallback — better to record the gap than crash the backup.DependencyInjection.cs:122-128— No captive DbContext. ✅BackupWriternow takes(BackupOptions, TimeProvider)— both singletons, no Scoped capture. It opens its ownSqliteConnectionforVACUUM INTOand readsuser_versionfrom its own ReadOnly connection. Clean. The misleading "fresh-scope-per-job" claim is gone from the PR body. Exactly right.Bonus: atomic write. ✅ Writes to
.zip.tmp, renames to.zipon success. A partial failure leaves nothing that a confused prune could mistake for a real artifact. Thefinallycleans up the temp file. Good~💡 Little ideas (non-blocking)~
BackupWriter.cs:148—ReadUserVersionuses synchronous I/O (connection.Open(),ExecuteScalar()) inside an async call chain. For a single PRAGMA read from a local file this is negligible — but if you ever want to be strictly async-clean,OpenAsync/ExecuteScalarAsyncwithCancellationTokenwould be the move. Not worth changing now.Orphan
.sha256sidecars without a.zip(e.g., someone manually deletes a zip) won't be cleaned up by prune since it only globs*.zip. Harmless and rare — mention only for completeness.Lingering
.zip.tmpon process crash won't match the prune glob and won't be cleaned by the next cycle (different timestamp → different filename). Rare and harmless, but a periodic cleanup pass or startup sweep could catch it someday if the volume ever fills up.✅ What I liked~
GeneratedRegexis the modern, compiled, allocation-free way. Sharp~userVersionread from the snapshot, not the live DB — correct! The live DB might have migrated between snapshot and manifest write. Reading from the snapshot guarantees the manifest matches the archived data.AddSingleton<BackupWriter>()+AddHostedService<BackupService>(), both backed by singleton dependencies. No captive scope, noActivatorUtilities, no surprises. Exactly what a reviewer wants to see.Automated review by Jibril · 2026-07-14
CI/CD: passed for head SHA
5861d31(coverage report regenerated — BackupWriter 92.6% line / 81.5% branch, BackupService 71.4% / 83.3%, BackupOptions 100%) · Local checks: skipped (CI green, static scan clean)Addressed the non-blocking "little ideas" from both reviews in
5bfa28a.1. Async
ReadUserVersion— converted toReadUserVersionAsyncwithOpenAsync/ExecuteScalarAsync+CancellationToken. The full call chain fromRunAsync→WriteZipAsync→WriteManifestAsync→ReadUserVersionAsyncis now async-clean.2. Orphan
.sha256sidecar cleanup — prune now tracks which sidecars belong to surviving zips, and deletes any orphankagura-backup-*.sha256whose zip was manually deleted outside the service.3. Stale
.tmpcleanup — prune also sweeps*.tmpfiles left behind by a process crash mid-backup. These never matched the prune glob and would have lingered indefinitely.4. docker-compose documentation —
KAGURA_BACKUP_SCHEDULEandKAGURA_BACKUP_RETENTION_DAYSare now surfaced as commented-out lines with a note that the schedule is interpreted in the container'sTZ(Europe/Berlin), not UTC. TheTZcomment itself now mentions it affects the backup schedule.New test:
Pruning_cleans_up_orphan_sidecars_and_stale_tmp_files— seeds both an orphan sidecar and a stale.tmp, runs the backup, verifies both are cleaned.