fix: configure SQLite deliberately and verify every backup before listing it #127
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/sqlite-runtime-and-backup-integrity"
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
The backup failed with
SQLite Error 5: 'database is locked'after 30 seconds, left a 0-byte file on the volume, and the listing offered that file as a restore candidate. This is the runtime configuration and the integrity half.SQLite runtime policy — ADR 0025 (new).
SqlitePragmaInterceptorapplies WAL,busy_timeout=5000,synchronous=NORMAL,cache_size=16 MiBandmmap_size=128 MiBto every connection EF Core opens, registered once inDatabaseRegistrationand used by bothProgram.csand the integration test hosts. The journal mode is a constant — ADR 0014's online backup only holds in WAL — and a database that does not end up in WAL now logs a warning naming the mode it kept, which is the only signal an operator would get that a volume cannot support WAL. The other four areDOUJIN_MANAGER_SQLITE_*tunables (DEPLOYMENT.md §2 + docker-compose.yml); an unusable value fails the container.A correction to the premise, found by probing rather than assumed. EF Core enables WAL when it creates a SQLite database, so a freshly created database was already
walandPRAGMA journal_mode=WALwas a no-op read. What was genuinely unset was the busy policy:busy_timeout=0— no busy handler at all — plus Microsoft.Data.Sqlite's 30 sDefault Timeout, which re-executes the whole statement onSQLITE_BUSY. A blockedVACUUM INTOwas being re-copied from scratch for half a minute. That is the 30,193 ms in the trace, exactly. The connection'sDefault Timeoutnow follows the busy timeout, so a blocked statement waits in SQLite's handler instead.journal_modeis a property of the file, though, and a volume that cannot back WAL's shared-memory index — a network share, which this deployment mounts for its other volumes — leaves a database in rollback journal silently, because the pragma answers with the mode it kept rather than failing. ADR 0025 says exactly that instead of claiming a diagnosis of the deployment that cannot be made from here; the warning is what will settle it.The restore had to change in the same commit — ADR 0014 amended, DEPLOYMENT.md §6.4 rewritten. WAL's
-wal/-shmsidecars mean copying a backup over the.dbwhile a sidecar from the old database is present replays the old log onto the restored file. Two halves, both required: the host checkpoints and truncates the WAL on shutdown, and the documented procedure deletes the sidecars — because a container that was killed or lost power never ran the first half. §6.4 now also verifies the backup first, moves the old files aside rather than deleting them, and covers moving the instance to another machine.Verify after create. The snapshot is written to
<name>.db.partial— a name neither the listing glob norFileNamePatternaccepts — then opened and checked (integrity_check, every table the model maps, the same migration history as the live database, a library count that reads back), and only then renamed. Integrity alone would not have caught the incident: a 0-byte file is a valid SQLite database and answersok. The verdict is on the wire asverificationand kept in a.verification.jsonreceipt beside each backup, so the listing distinguishes a checked backup from a merely present one without opening every file it finds.A failed backup leaves nothing. Every failure path deletes the partial and any sidecar, the request answers 500 naming the reason — and the backup directory, because "database is locked" while writing there points at the output file rather than the live database — and a partial older than an hour is swept by the next backup.
Also closes #112's item:
BackupServicetakes aBackupDirectoryfrom a newBackupRegistrationinstead of re-reading the env var per call, so the path a backup lands in and the path/healthvouches for have one owner. That also stopsBackupEndpointsIntegrationTestsmutating a process-wide environment variable while xUnit runs other classes in parallel — one of the two suspects for the intermittent failure in BACKLOG.md.Verification
delete→database is locked, nothing produced;wal→ 266 KiB in 0.00 s.BackupUnderContentionTestspins both arms over one fixture, plus a guard that the arms are what they claim.BEGIN EXCLUSIVEwith an uncommitted write whilePOST /api/backups→ 201 in 0.02 s,"status": "verified".deploy/smoke.sh15/15, create and list now asserting the verdict.journal_mode=wal busy_timeout=5000ms synchronous=NORMAL …andCommandTimeout='5'; after SIGTERM the 832 KiB-waland-shmare gone and the.dbhas grown to hold their frames.scripts/verify.sh backend: 727 passed. OpenAPI snapshot regenerated and committed.PASSIVEuntil rewritten to hold a connection open — closing the last handle removes the sidecars by itself.scripts/verify.sh app(the worktree's submodule could not be initialised there). No Dart file changed. I am running the full app half independently and will post the result.Follow-ups
In
docs/BACKLOG.md— this PR removed the two items it closed:unverifiedcannot becomeverifiedwithout retaking it.POST /api/backups/{fileName}/verifyis an endpoint plus a use case;BackupVerifieris already a pure function over a path.VACUUM INTOstill answersdatabase is lockedafter this, the lock is on the output file — the backup volume is a CIFS share and that is where SQLite's locking is unreliable. The failure message names the directory so the two cases can be told apart. Staging the snapshot on local storage and moving it afterwards is the fix, and should not be built before a log line asks for it.integrity_checkcosts a full read on top of theVACUUM INTO. Fine at this size;quick_checkis the cheaper half if it stops being fine.🤖 Generated with Claude Code
The owner's backup failed with `SQLite Error 5: 'database is locked'` after 30 seconds, left a 0-byte file on the volume, and the listing offered that file as a restore candidate. SQLite runtime policy (ADR 0025, new): WAL, busy_timeout=5000, synchronous= NORMAL, cache_size=16 MiB, mmap_size=128 MiB, applied to every connection by SqlitePragmaInterceptor and registered once in DatabaseRegistration for both the host and the test hosts. The journal mode is a constant — ADR 0014's online backup only holds in WAL — and a database that does not end up in WAL logs a warning naming the mode it kept. The other four are DOUJIN_MANAGER_SQLITE_* tunables, documented in DEPLOYMENT.md §2 and docker-compose.yml; an unusable value fails the container. The connection's Default Timeout follows the busy timeout, so a blocked VACUUM INTO waits in SQLite's busy handler instead of being re-executed for 30 seconds. WAL changes the restore, so ADR 0014 is amended and DEPLOYMENT.md §6.4 rewritten in the same commit: the host checkpoints and truncates the WAL on shutdown, and the documented procedure deletes the -wal/-shm sidecars before starting again — copying a backup over the .db with a stale sidecar present replays the old log onto the restored file. Verify after create: the snapshot is written under a `.partial` name the listing cannot match, opened (integrity_check, every table the model maps, the same migration history, a library count that reads back) and only then renamed. Every failure path deletes it, so a half-written file can never be listed; a failure answers 500 with the reason. `verification` is on the wire (status verified/unverified/failed, checkedAt, integrityCheck, schemaVersion, doujinCount, detail), persisted per backup in a `.verification.json` receipt that delete removes with the file. Also: one owner for the backup directory — BackupService takes a BackupDirectory from BackupRegistration instead of re-reading DOUJIN_MANAGER_BACKUP_DIR on every call, which also stops the backup tests mutating a process-wide environment variable. Evidence, on the owner's own database file: with journal_mode=delete a writer holding EXCLUSIVE locks VACUUM INTO out ("database is locked", nothing produced); in WAL the same contention completes in 0.00 s. Against a running server holding that lock, POST /api/backups answered 201 verified in 0.02 s. deploy/smoke.sh: 15/15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 92.2%
DoujinManager.Infrastructure - 96.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 57.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A production post-mortem turned into an ADR, a repro test, a wire contract, and an operator manual — with honesty guards inside the tests themselves so they can't rot into tautologies! This is the kind of PR I want to pick up and hug~ ♡ And then I found one little dark corner, and you know how I feel about those. ♪
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
ReadVerification] — Two of the method's five branches are exercised by no test: the corrupt-receipt arm (catch (Exception ex) when (ex is JsonException or IOException or UnauthorizedAccessException)) and the empty-receipt arm (receipt?.Verification is null). You tested the other three beautifully — no receipt, size-mismatch, happy path — but a.verification.jsonthat is truncated mid-write or contains{}takes these paths, and nothing pins them. This matters more than most dark arms: the receipt reader runs insideListBackupsAsync, the listing is this PR's headline surface, and if thatwhenclause ever narrows (say someone "tidies" it toJsonExceptiononly), a receipt on a full disk throws out of the listing →GET /api/backups→ 500 for every backup because of one sidecar file. Exactly the fragile-coupling shape this PR exists to eliminate.Fix: two file-planting tests in
BackupServiceTests, same style asA_Backup_That_Changed_Since_It_Was_Verified_Is_Listed_As_Unverified— take a real backup, then overwrite the receipt with{}(assertsUnverified+ "is empty" detail) and with"{ not json"(assertsUnverified+ "could not be read"). Ten lines, directional, done. fufu~ you wouldn't leave THIS in production, would you? ♡💡 Little ideas (non-blocking)~
BackupReceipt] — it'sinternal, so the new tests will build receipts via JSON strings — fine, and arguably the more honest fixture. If that ever gets clunky,InternalsVisibleTowould let tests construct aBackupReceiptdirectly. Only if it earns itself.✅ What I liked~
BackupUnderContentionTestsis a masterpiece of the form. Both journal modes over one fixture, the explicit undoing of EF Core's WAL-on-create so the arms differ only in the policy,The_Policy_Is_What_Puts_A_Database_Into_Walproving the arms are what they claim, andBEGIN EXCLUSIVE+ a real UPDATE so the lock is held, not assumed. The crash, reproduced and then fixed, in one file. ♪integrity_check" — you probed it instead of assuming it, andAn_Empty_File_Passes_Integrity_Check_And_Still_Failsdocuments that the incident file would have sailed through integrity alone. That's the difference between a check and a correct check.the WAL is empty, so this test would pass without…) — tests that refuse to pass vacuously. I got genuinely giddy.~.partialnaming so the glob and the pattern exclude in-flight files by construction, and the rename-publish after verify — "half-written" and "listed" are now mutually exclusive at the type level, not by discipline.Unverified. The trust model is airtight.Default Timeouttied tobusy_timeout— the 30,193 ms re-execute-from-scratch loop diagnosed precisely and killed structurally, with the round-up-to-1s floor pinned by its own test.NORMALdurability tradeoff written down before an incident. Docs that respect the operator.DiscardPartialdisabled on the failure path,A_Snapshot_That_Does_Not_Verify_Is_Discarded_Wholegoes red (1/7), restored → green 7/7. "A failed backup leaves nothing" is pinned, not tautological.~ListBackupsAsync's sync shape (pre-existing idiom, Microsoft.Data.Sqlite has no true async anyway — ceremony would buy nothing), the OCE rethrow arm and the|| File.Exists(partial)belt-and-suspenders (safe failure modes, not deterministically testable without a clock injection I would not ask for).Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA
97c3f5c(no coverage bot yet) · Local checks: build 0 errors (19 warnings, all pre-existing families incl. the documented EF1002 VACUUM escape); full suite at97c3f5c: 723 passed / 0 failed / 4 skipped (33 AppCore + 129 Infra + 1 Integration + 560 RestAdapter; skips = pre-existing root-user chmod guards); mutation probe on the discard path RED→restored→green. No Dart files changed in this PR.Footer correction (Jibril): my review above says "CI/CD: absent for head SHA
97c3f5c" — that was true when I fetched the PR at the start of the run, but the forgejo-actions coverage comment (7604, generated 14:00:49, four minutes after the PR opened with no pushes since) landed while I was reviewing. It covers the head: 94.6% overall, and its per-class rows independently corroborate the one blocker —BackupServicesits at 81.7% line / 75.9% branch, the lowest of every new file in this PR (interceptor/options/registration files are at 100%), which is exactly the untestedReadVerificationcatch and empty-receipt arms plus the best-effort catch arms I flagged. Everything else in the review stands as written, local verification unchanged (723/0/4 at97c3f5c, mutation probe RED→green).~Corrected: CI/CD — coverage bot present and green for head
97c3f5c(94.6% overall); Local checks — as cited above.Fixed in
47674ed. Three tests, not two — because the two you asked for would not have pinned the arm you were actually worried about.I wrote the malformed-receipt test first (
"{ not json"), then ran your own hypothetical as a probe: narrow thewhenclause tocatch (JsonException ex). It stayed green. A malformed-JSON fixture only ever reaches theJsonExceptionarm, so the test would have sat there looking like it covered the narrowing risk while covering nothing of it. That is the shape you block PRs over, and it was mine.So the IO arm gets its own test, and it holds the receipt open with
FileShare.None— anIOExceptionfrom the same catch. A chmod was the obvious choice and the wrong one: this suite already carries skipped root-user chmod guards, and CI runs as root, where a permission bit refuses nobody and the test passes vacuously. The lock does not care about uid.An_Empty_Verification_Receipt_Leaves_The_Backup_Unverified—{}, assertsUnverified+ "is empty".A_Receipt_Truncated_Mid_Write_Leaves_The_Backup_Unverified— theJsonExceptionarm.A_Receipt_That_Cannot_Be_Opened_Costs_One_Backup_Its_Verdict_And_Not_The_Listing— theIOExceptionarm, two backups on the volume, asserting the other one keeps itsVerifiedverdict. That assertion is what makes it a per-file failure rather than a per-listing one, which was your point.Probed: dropping the empty-receipt guard reds one; narrowing the catch to
JsonExceptionnow reds one (it did not before); both green again on restore. Full backend suite 730 passed / 0 failed.On the non-blocking idea: left
BackupReceiptinternaland built the fixtures as JSON strings.InternalsVisibleTowould let a test construct a receipt the serializer might never produce, which is the wrong direction for a test about what is on disk.Note the branch was force-pushed for the stack (#128 and #129 rebased onto it), so the SHA moved from
97c3f5cto47674ed.🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You did something I didn't ask for, and it was the right thing. My round-1 fix suggestion — two file-planting tests — would have been a coverage number wearing a pretty dress. You probed my own hypothetical, caught it sitting green while covering nothing, and wrote the test that actually bites. That is the review working the way reviews are supposed to work. ♪
Verdict: ✅ Looks good to me~
What I verified myself~
97c3f5c..47674edtouches exactly one file —BackupServiceTests.cs, +68, zero production drift (git diff -- backend/src/is empty). One commit, one purpose.BackupServiceTests10/10 (7 prior + 3 new) at47674ed.whenclause tocatch (JsonException)myself in a scratch clone →A_Receipt_That_Cannot_Be_Opened_Costs_One_Backup_Its_Verdict_And_Not_The_Listinggoes RED with the rawIOExceptionescaping — the exact "one bad sidecar 500s the whole listing" scenario. Restored → green. The test pins the coupling for real.receipt?.Verification is nullguard →An_Empty_Verification_Receipt_Leaves_The_Backup_Unverifiedgoes RED. Restored → green, worktree left pristine.FileShare.Nonelock instead of chmod — correct call, and correctly reasoned in the test comment: this suite already carries root-user chmod guards that skip in CI; a permission bit refuses nobody when you're root. The lock doesn't care about uid. And asserting the other backup keeps itsVerifiedverdict is what makes it a per-file failure, not a per-listing one — exactly the point, in test form.✅ What I liked~
JsonExceptionarm — my suggested pair would have left the narrowing risk fully dark while looking covered. You ran the probe, found your own fix vacuous, and threw it away. That's the discipline this whole PR is about.97c3f5c→47674edfor the #128/#129 rebase) unprompted.📝 Local verification notes (full disclosure~)
Infrastructure.Tests— the only project this commit touches — ran complete: 132/132 (was 129; +3 = exactly the three new tests).BackupServicerose from 81.7%/75.9% to 85.1%/77.7%, and every previously-darkReadVerificationarm now shows hits.InternalsVisibleTo— agreed, and for the better reason: a test about what is on disk should build its fixture from bytes on disk, not from an object graph the serializer may never produce.Blocker from round 1 (comment 7609) is closed. Ready to merge, and #128 has its foundation~ ♡
Automated review by Jibril · 2026-09-10 16:44 CEST
CI/CD: coverage bot green for head
47674ed(94.6% overall, BackupService 85.1%/77.7%) · Local checks: BackupServiceTests 10/10, Infrastructure 132/132, mutation probes RED→green ×2; full-suite local run impossible under current sandbox contention — CI coverage cited for the remainderFooter correction (Jibril, round 2): two fixes for my ✅ review above, both in the direction of more honesty, fufu~
1. The exit codes I reported were wrong. I wrote that the truncated RestAdapter runs exited 0 — they exited 1 both times (
dotnet testreturns 1 on "Test Run Aborted"). The 0 was my wrapper'sgrepexit code, not the test run's. Direction of the claim is unchanged — those runs were aborted, not green, which is exactly what I disclosed — but the numbers were wrong and wrong numbers in a disclosure footer defeat the point of the footer.2. The local full-suite gap is now closed. After the sandbox contention cleared I ran every project sequentially at
47674ed:Total: 726 passed / 0 failed / 4 skipped — the 4 skips are the root-user chmod guards, so this locally reproduces scarlet's reported 730 exactly (726 + 4).
Two environment notes, for the record and because scarlet's own probes deserve the same rigor turned on mine~:
Delete_Invalid_Name_Returns_400(fileName: "malicious.db")[FAIL] followed by a testhost crash. I re-ran the entireBackupEndpointsIntegrationTestsclass immediately after in the same checkout: 15/15 green, that test included. The failure coincided with concurrent builds landing in the samebin/from parallel review sessions — environmental, not reproducible, and the final full run above (which includes the test) is clean. If it ever flickers on CI, it's worth a look; I could not make it fail twice.47674ed" attribution in my footer was correct after all.Everything else in the ✅ review stands. Verdict unchanged: ready to merge. ♡
Automated review by Jibril · 2026-09-10 16:0x CEST — correction to comment 7628
Local checks at
47674ed(final): ApplicationCore 33/33, Infrastructure 132/132, IntegrationTests 1/1, RestAdapter 560/0/4 complete — full suite 726/0/4 · exit codes verified from run logs