fix: never open a SQLite database on the backup volume #132
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/backup-staging-on-local-volume"
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's in
Backups still fail in production after #127–#130. This is the actual cause, and it is one I got wrong the first time.
The symptom.
SQLite Error 5: 'database is locked'after 5,050 ms — the wholebusy_timeout— on a server that had served exactly one request since starting and had no other activity. The live database was not contended by anything.The cause. The backup volume is a CIFS mount.
VACUUM INTO '<path>'does not write a file; it creates a SQLite database at that path and takes a lock on it, and a network filesystem is precisely where that lock cannot be relied on. So did every other step: verification opens the candidate read-only, which takes a shared lock; an upload was written to the backup volume and opened there to be checked; the startup swap opened the backup where it lay.What hid this is that everything else on that mount worked perfectly — the listing, the receipts, the images and thumbnails all live there and are plain byte I/O. Only the operations that need a lock failed, and SQLite reports a failed lock with the same error it uses for a busy database.
deploy/docker-compose.ymlhad already written the rule down — for the other volume:…and then sent
VACUUM INTO's destination to a volume that makes no such promise.A correction to #127
#127's PR body said the root cause of the original 30,193 ms crash was
busy_timeout=0plus Microsoft.Data.Sqlite re-executing the whole statement on a 30 s command timeout. That was a real bug and it is fixed — this failure is visibly the fixed version: it waits its timeout, fails cleanly, and discards the partial instead of leaving a 0-byte file that lists as a backup. But it was not the whole cause, and I presented it as if it were. The lock was always there; #127 only made it fail fast and honestly.The rule
Bytes cross the volume boundary; databases do not.
No code opens a SQLite database that lives in
DOUJIN_MANAGER_BACKUP_DIR. Every path that creates, reads or checks one does it in<database directory>/.staging— derived fromDOUJIN_MANAGER_DB_PATH, on the one volume the deployment already promises can lock a SQLite file.VACUUM INTOwrites to staging and verifies there; only the finished file is copied across, under the.partialname, then renamed within the backup directory so publication stays atomic.Staging is swept at the top of
ApplyPendingRestore, which is the one thing the host calls unconditionally at startup — before the restore marker is even looked at, because the wreckage it clears is left by a killed backup, on boots where no restore is pending.A failure names which volume refused, at every step including the prologue. The two produce the same "database is locked" and mean opposite things — on the database volume something holds the live database; on the backup volume the destination's own lock never succeeded — and an operator cannot tell them apart from SQLite's message.
ADR 0014 amendment 4 carries the reasoning.
DEPLOYMENT.md§2 now says the backups volume may be a CIFS/NFS share and the db volume may not, plus the cost: the db volume needs room for one more copy of the database while a backup or restore is in flight. Two troubleshooting rows and the compose comment follow.Verification
verified; the backup volume left holding exactly the.dband its.verification.json— no-journal, no-wal, no-shm, no partial — and staging empty.SQLite Error 26: 'file is not a database', with the backup directory not created and staging empty.RESTORE APPLIED), the marker consumed, the previous database and both WAL sidecars inbefore-restore-…/, staging empty, and the backups untouched.EnforcedFileModes) and the count reads 882 passed / 4 skipped — same run, different uid.VACUUM INTOwritten straight to the backup volume, with verification and the copy reverted to match — the faithful old shape — reddens the SQL pin. (A partial revert that moves only theVACUUMtarget reddens three, because verification is then pointed at a file that no longer exists. Both directional; the first is the honest "old code" mutation.)failingVolumereassignment before the cross-volume copy reddens the mid-snapshot test.VACUUM INTOtarget is asserted in the SQL, through a capturingDbCommandInterceptor, because the destination file has been moved on by the time the call returns.VACUUM INTOruns — the only moment a test can act from between the two volumes, since the partial's name is timestamped to the millisecond and cannot be pre-empted. Planting a file where the backup directory belongs does not reach that line: it fails in the prologue, where the message is already right for a different reason, and that first version of the test was green under the mutation it was written for.Follow-ups
In
docs/BACKLOG.md:/healthcannot tell a directory that stores bytes from one that can host a SQLite database. The volume that broke every backup passed readiness on every boot, because a CIFS mount is writable. A probe that creates a tiny database in each data directory, writes to it and drops it would have made this a first-boot message instead of a 03:30 mystery. Staging means only the db volume needs to pass it now, which makes the probe cheap and what it catches severe.🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94.1%
DoujinManager.Infrastructure - 96.4%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.3%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 62.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A production root-cause hunt that ends in a one-sentence architectural rule — "bytes cross the volume boundary; databases do not" — applied to all four paths at once, with an ADR amendment that admits the previous diagnosis was incomplete. This is my favorite kind of PR~ ♪ I read every line of the diff, every changed file in full, ran your whole suite, and re-ran your mutation probes myself with my own hands. The rule is beautiful. The discipline is beautiful. And two things are not true yet, and you know how I feel about things that are not true~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Services/BackupStagingArea.cs:86+BackupRestoreCoordinator.cs:137— the startup sweep does not exist on a normal boot.Sweep's own doc says "Called once at startup",Program.cs:161calls onlyApplyPendingRestore, andApplyPendingRestorereturnsNoneat line 89 when no restore marker exists —SweepatApply:137sits inside the marker-present branch.grepconfirms no other call site, and there is no age-based cleanup for staging (theStalePartialAgesweep covers only the backup volume's.partialfiles). So the exact scenario your docs describe — "anything present is wreckage from a killed backup … and it is occupying the database volume" — is the one scenario the sweep misses: a backup killed mid-VACUUM(OOM, container restart), then a reboot with no marker → the staged copy of the whole database survives on the db volume indefinitely. The nextCreateBackupAsyncReserves a fresh timestamped name and never touches the old one. Meanwhile three documents promise the opposite: DEPLOYMENT.md §3 ("It is swept at every start"), ADR 0014 amendment 4 ("Staging is swept at startup"), and the code's own XML doc. An operator who reads §3 will trust a cleanup that never runs — on the one volume you just told them needs headroom. Fufu~ a documented invariant with no code and no test behind it… you wouldn't leave THAT in production, would you? ♡Fix: move the sweep to run unconditionally at startup —
Program.cs, immediately beforeApplyPendingRestore(passapp.Logger; it already hasdbPath). Keeping the sweep insideApplytoo is fine (harmless double-sweep on a restore boot). Pin it: seed staging with a leftover file, run the startup-shaped call with no marker present, assert the leftover is gone — that no-marker arm is precisely the branch that is dark today.BackupService.cs:165–171/203/229–232andBackupRestoreCoordinator.cs:141–148— the new failure-reporting arms are pinned by nothing. The volume-naming is a headline feature of this PR: the body, ADR amendment 4, and the new DEPLOYMENT.md troubleshooting row ("The error message names which volume failed") all sell it. Yet no test asserts which volume is named:BackupUnderContentionTests.cs:199checks only"database is locked"— swap the twofailingVolumeassignments, or delete thefailingVolume = …reassignment atBackupService.cs:203so a backup-volume failure reports "the database volume", and the entire 882-test suite stays green. Same for the coordinator's new fetch-failure arm ("could not be read off the backup volume",BackupRestoreCoordinator.cs:146): no test drivesFile.Copy(sourcePath, stagedPath)to failure, so that refuse path — marker quarantined, database untouched — has never once executed in CI. You added code paths and forgot to test them? After you wrote such lovely probes for the happy paths? Fufu~ that hurts me more than it hurts you~ ♡Fix: (a) extend the contention test to assert
"the database volume"appears in the locked-writer failure; (b) one test where the copy to the backup volume cannot succeed (e.g. a file planted where the backup directory belongs, soCreateDirectory/File.Copyfails after staging succeeded) asserting"the backup volume"; (c) one coordinator test that makes the candidate unreadable between check and copy (hold it open withFileShare.None, or plant a file where staging belongs soReservefails) asserting the refuse outcome and consumed marker.💡 Little ideas (non-blocking)~
docs/adr/0014-sqlite-vacuum-into-backups.md:5— "Amended twice on 2026-09-10" now introduces a list of four amendments, because this PR added the Amendment 3 link (stale from #129) and Amendment 4 to the very sentence that still says "twice". One word → "four times".VACUUM INTOtarget (pointing it back atpartialPath) reddened three tests (the SQL pin,A_Finished_Backup_…, andA_Restore_Request_…— they shareCreateBackupAsync), not exactly one. The restore-candidate probe claim holds only for the clean mutation (deletingOverLocalCopy); keeping both calls and reading in place passes all four, which is worth knowing about your pin. Still directional, still good pins — just not "exactly one".✅ What I liked~
VACUUM INTO+ verify on the db volume, uploads staged before they earn the trip, restore-requests throughOverLocalCopy, and the startup swap as an intra-volumeFile.Moveso the live database appears whole or not at all. That last insight (a half-copied database is the one state the procedure must never produce) is genuinely elegant~SqlCaptureDbCommandInterceptor— asserting theVACUUM INTOtarget in the SQL because the destination has been moved on by the time the call returns. Correct observation, clever instrument. Oh! And it works — my probe confirmed the pin reddens on target-swap./healthprobe in BACKLOG. This is how root-cause writeups should read.EnforcedFileModesskips — this box runs as root too; 878+4 = your 882), and mutation probes 2 and 3 redden exactly one test each, exactly as claimed.The rule is right, the tests you wrote are real, and the two gaps above are small next to what this fixes. Close them and this is one of the better backup PRs I've held my scalpel to~ fufu~ ♪
Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA
99347fd(no coverage-bot comment yet) · Local checks: full backend suite green (878 passed/4 skipped, root-env), 3 mutation probes re-run (results above)Both fixed in
d3fc813. Blocker 1 is the worse of the two and you found it in the right way — by reading the sentence and then looking for the call site.1. The sweep never ran on a normal boot. Exactly as you traced it:
Sweepsat insideApply,Applyonly runs when a marker exists, and there is no age-based cleanup for staging. So the one scenario the doc describes — a backup killed mid-VACUUM— is the one the sweep missed, and a whole copy of the database would sit on the db volume indefinitely, on the volume the same PR had just told the operator needs headroom.It now sweeps at the top of
ApplyPendingRestore, before the marker is looked at, rather than inProgram.csas you suggested. Same unconditional call —Program.cscalls it on every boot — but it stays reachable from a test, which theProgram.csversion would not have been. The comment says why it is there.Pinned by
A_Boot_With_Nothing_Pending_Still_Clears_Staging, and moving the sweep back inside the marker branch reddens it.What I should say plainly: I wrote the code and all three sentences claiming "swept at every start" in the same PR and never checked they agreed with each other. That is the third time in this series I have fixed something and left the prose describing a different version.
2. The volume naming — and my first attempt at the test did not pin it either. I wrote
A_Backup_Volume_That_Refuses_The_Finished_File_Is_Named_As_The_Backup_Volume(a file planted where the backup directory belongs), then ran your exact suggested mutation — deleting thefailingVolumereassignment at the copy — and it stayed green. Planting a file there fails in the prologue, where the message is already "the backup volume" for a different reason; it never reaches the reassignment.Driving that line needs the snapshot and the verification to succeed and only the copy across to fail, which nothing outside the method can arrange — the partial's name is timestamped to the millisecond, so it cannot be pre-empted. So the test acts from the one moment that sits between the two volumes: a
DbCommandInterceptorthat removes the backup directory whileVACUUM INTOis running. Deterministic, and the realistic version of the same thing — a share that drops under a long backup. That mutation now reddens.Also
(a)and(c)as you asked: the contention test asserts"the database volume"andDoesNotContain("the backup volume"); and the coordinator's fetch-failure arm is pinned by holding the candidate open withFileShare.None— refused, marker consumed, database byte-identical, staging empty.One thing your review made me notice. The prologue's work on the backup volume (
Directory.CreateDirectory,RemoveStalePartials) sat outside the reporting entirely and escaped as a rawIOExceptionwith no volume named at all. It is inside now, so every failure inCreateBackupAsyncsays which volume.Reservemoved inside the coordinator's guard for the same reason: a staging directory that cannot be made is a reason to leave the database alone, not to kill the container before it has started.Nit 1 — fixed, "Amended four times", and the list punctuation with it.
Nit 2 — you are right that the sentence overclaims, and the difference is worth writing down. Your probe and mine were different mutations. Mine reverted all three edits together (VACUUM target, verify target, and the copy), which is the faithful "old code" shape, and reddened exactly one — the SQL pin — because the result is a working backup written the old way. Yours moved only the
VACUUM INTOtarget back, leaving verification pointed at a staging file that no longer exists, so three tests fail on the broken intermediate state. Both are directional; the PR body's "each redden exactly one" was true of the mutations I ran and read as a general claim, which it is not. Body corrected to name the mutation.Your 878 passed / 4 skipped against my 882 is the same run: the 4
EnforcedFileModestests skip as root and pass here. Worth me quoting both numbers in future.Full suite now 886.
🔮 fufu~ Jibril reviewed your code! (round 2, synchronized
d3fc813)Back so soon~ ♪ One commit,
fix: sweep staging on every boot, and pin which volume a failure names, aimed straight at both of my blockers — and it does the thing I asked and the things I didn't think to ask. The sweep didn't just move to the top ofApplyPendingRestore; it now also covers the boot-time fetch of the candidate, withReservepulled inside the guard so a staging directory that cannot be made is a refuse instead of a crash-loop. AndfailingVolumedidn't just get pinned — it now starts on the backup volume, so the prologue (CreateDirectory, stale-partial removal) failures get named too. Oh? Oh! You went deeper than the review did~ that's the good stuff ♡Verdict: ✅ Looks good to me~
Round-1 blockers, both closed and mutation-probed by my own hands~
BackupRestoreCoordinator.cs:94— hoisted above the marker check, so the only call site (Program.cs:161, unconditional) sweeps on a normal boot.A_Boot_With_Nothing_Pending_Still_Clears_Stagingpins the exact dark arm from round 1 (leftover staged file + no marker →None+ leftover gone), and its comment cites both promises it makes true. Probe: commented the sweep out → exactly that test RED (1 failed / 13 passed). Restored → green.Rollback_Journal_Backup_Is_Locked_Out_By_A_Writernow assertsthe database volumepresent andthe backup volumeabsent. Probe: deleted the db-volume reassignment atBackupService.cs:180→ exactly that test RED on the contains-check. (b) Two new staging tests: a file where the backup directory belongs (prologue failure) and — this is the lovely one — aSqlCaptureonVacuumhook that deletes the backup directory mid-VACUUM INTO, driving the reassignment where it actually matters, the copy-across failing after snapshot and verification both succeeded. Probe: deleted the second reassignment atBackupService.cs:211→ exactlyA_Backup_Volume_That_Goes_Away_Mid_Snapshot_Is_Named_As_The_Backup_VolumeRED. (c) The coordinator's fetch-refuse arm:A_Backup_That_Cannot_Be_Read_Off_The_Backup_Volume_Is_Refusedholds the source withFileShare.None(correctly noting a permission bit refuses nobody as root), assertsRefused+ the message + database bytes unchanged + marker consumed + staging empty. Probe: replaced the refuse withthrow→ exactly that test RED (RefusedvsFailed). Four probes, four single-test reddens, tree restored pristine after each. TheDoesNotContainassertions make these double-ended — a swap can't slip through either direction~✅ What I liked~
SqlCapturewithonVacuumto make a mount disappear at the one moment a test can stand between the two volumes — that is genuinely clever instrumentation, and the comment explaining why planting a file can't reach the reassignment shows you understood which line each failure actually exercises. I got giddy~failingVolumestarting on the backup volume — round 1 missed that the old prologue failures (backup dir unreachable) fell into the db-volume default. You fixed a naming bug I hadn't found. That deserves the praise, not the complaint ♡if (partialPath.Length > 0),if (stagedPath.Length > 0)) andstagedPathprecomputed beforeReserve— the moved-in prologue made those guards necessary, and they arrived in the same commit rather than as a follow-up crash. Coherent thinking, not accident-driven.💡 Little ideas (non-blocking)~
882 passed (878 before); this commit adds 4 more tests, so the honest numbers are 886/882 (my local run on this root box: 882 passed / 4 skipped, root-env parity with the EnforcedFileModes skips). A one-line addendum for the new commit's four probes would keep the writeup as precise as the code.Independently verified locally at
d3fc813: fullscripts/verify.sh backendgreen — 49 ApplicationCore + 205 Infrastructure + 1 Integration + 627 RestAdapter = 833 passed / 4 skipped on this root box (the 4 are theHealth_ReportsUnhealthy_*permission skips; add back the 49 root-parity skips difference and your 886 holds), build 0 errors (19 warnings, all pre-existing: EF1002, CS8602, NU1903, ASPDEPR005). Coverage-bot comment 7679 covers99347fdonly — stale for this head, local checks used.Both blockers closed with directional pins, no new gaps opened, docs truthful. This is the kind of follow-up that makes reviewing feel less like guarding and more like watching someone think~ fufu~ ready to merge ♪
Automated review by Jibril · 2026-09-10
CI/CD: stale for head SHA
d3fc813(coverage bot 7679 covers99347fd) · Local checks: verify.sh backend green (882 passed / 4 skipped, root-env), 4 mutation probes — each reddened exactly its own test, clone restored pristineCorrection to my arithmetic above~ 49 + 205 + 1 + 627 = 882 passed / 4 skipped (= 886 total; on non-root CI the 4 permission skips run and pass, which is your 886). The "833" and the "49 root-parity skips" clause were my own silliness — the components were right, the sum wasn't. Fufu~ even the scalpel slips sometimes; the verdict and every probe result stand unchanged ♡