feat: a backup can leave the machine, and the server restores it #128
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/backup-download-and-restore"
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
#127 made the snapshot trustworthy. It still could not leave the machine:
GET /api/backups/{fileName}was every backup'sselflink and onlyDELETEserved the route (405), there was no way to put a file onto another instance, and restore was a manual copy between docker volumes.Download.
GET /api/backups/{fileName}streams the file asapplication/vnd.sqlite3, as an attachment, after the name has been throughFileNamePattern— which also excludes.partialsnapshots and.verification.jsonreceipts. Declared.Produces<Stream>, and ADR 0021 amended on measured evidence: on this stackFileResultemits acontentType/fileDownloadName/entityTagobject and dragsEntityTagHeaderValueandStringSegmentintocomponents.schemas;IResult(what the image endpoints declared) emits{"type":"object"}where bytes are served; onlyStreamemits{"type":"string","format":"binary"}. The ADR's rule had never been followed because following it made the document worse — the amendment replaces it, and the three image endpoints move with it.Upload.
POST /api/backups/uploadis checked before it is accepted: bytes stream to<name>.db.partial— a name neither the listing glob nor the pattern matches — and are put through #127'sBackupVerifierthere. Only then rename + receipt, so an uploaded backup listsverifiedexactly like one the server wrote. Every refusal (400, 409, 413) deletes the partial. The file keeps the name it was created under, because that timestamp is what orders the listing — re-stamping would make the list lie about the age of the data. No new tunable: it sharesDOUJIN_MANAGER_MAX_UPLOAD_BYTES, with the reasoning written down so it can be argued with.Restore, in two steps — ADR 0014's "restore is manual" is replaced. SQLite cannot be swapped under a running process, so
POST /api/backups/{fileName}/restoreverifies the file now (a backup that would not restore is refused 409 while you are watching, not during a restart on a machine you have already stopped), writes a marker and answers 202restartRequired;GET/DELETE /api/backups/restore-requestshow and cancel it. The next start swaps before the database is opened:-waland-shmLogCriticalwith the exactmvBecause the server writes the restored file itself, the manual procedure's
chownstep disappears.A bug the end-to-end walk found that a test would not have
The first boot after any restore — including the manual procedure this repo has always documented — crashed with
SQLite Error 8: 'attempt to write a readonly database'out ofProgram.cs.VACUUM INTOwrites a rollback-journal file, and EF Core'sMigrateAsyncprobes existence on a connection it clones withMode=ReadOnly, on which #127'sPRAGMA journal_mode=WALis a write. On an already-WAL database it is a read, which is why it had never surfaced. The interceptor now tolerates a connection that cannot assert the mode, and stays quiet about a mode it did not establish.That one is worth pausing on: it means the documented manual restore would have broken on the first boot once #127 landed, and only walking the whole path on two real servers surfaced it.
Also:
BackupEndpointsno longer callsIBackupServiceat all — eight use cases sit between the endpoints and the services — closing the tracked layering violation and the 405 self link together.Verification
/api/statsgoes 0 → 2 doujins, the restored file holds A's two titles, B's own database is preserved inbefore-restore-20260910-100702443/, and a second restart neither restores again nor displaces anything.deploy/smoke.sh22/22 against a live server, with six new steps. Its cleanup trap now cancels a pending restore first — a failed smoke run must not leave a marker the next start would act on.scripts/verify.sh backend: 782 passed. OpenAPI snapshot regenerated.../../../etc/passwdfailsFile.Existsand/etc/passwdfails verification anyway — the outcome was stillRefusedwhile the security-relevant branch was gone. That test now asserts the reason, and reddens on all three rows.scripts/verify.sh app. No Dart file changed; I am running it independently.Follow-ups
🤖 Generated with Claude Code
The owner reached for the backup feature to move the instance to a new machine. Even with the snapshot itself fixed, that was still impossible: the file could not be downloaded (GET /api/backups/{fileName} was every backup's self link and only DELETE served the route, so following it answered 405), there was no way to put a file onto another instance, and restore was a manual copy between docker volumes. Download. GET /api/backups/{fileName} streams the file as application/vnd.sqlite3, as an attachment under its own name, after the name has been through BackupService.FileNamePattern — the rule DELETE has always had. Declared .Produces<Stream>, which needed an ADR 0021 amendment: the ADR prescribed .Produces<FileResult>, and measuring all three on this stack shows FileResult emits an object with contentType/fileDownloadName/entityTag and drags EntityTagHeaderValue and StringSegment into components.schemas, while IResult (what the image endpoints used) says {"type":"object"} where bytes are served. Only Stream yields {"type":"string","format":"binary"}. The three image endpoints moved to Stream in the same commit, closing that BACKLOG item. Upload. POST /api/backups/upload (multipart) is checked before it is accepted: the bytes stream to <name>.db.partial — a name neither the listing glob nor the file-name pattern matches — and are opened there and put through stage 1's BackupVerifier (a SQLite database, this application's tables, the same migration history, a library that reads back) before being renamed and given a verification receipt. A refusal (400 bad file, 409 name taken, 413 too large) leaves the volume exactly as it was. The file must keep the name it was created under: that timestamp is what orders the listing, so the server does not invent one. It shares DOUJIN_MANAGER_MAX_UPLOAD_BYTES rather than growing a second ceiling; the reasoning is in DEPLOYMENT.md §2. Restore, in two steps, because SQLite cannot be swapped under a running process. POST /api/backups/{fileName}/restore verifies the file now, writes a marker beside the database and answers 202 with restartRequired; GET/DELETE /api/backups/restore-request show and cancel it. On the next start, before the database is opened or migrated, BackupRestoreCoordinator swaps it in: the displaced database is moved aside (never deleted) into before-restore-<stamp>/ together with its -wal/-shm, the incoming file is verified again at that moment, the marker is consumed either way so a failing restore cannot repeat on every boot, and no path throws — a refusal leaves the database untouched and says so, a copy that fails puts the previous files back, and the one unrecoverable case logs LogCritical with the exact mv command. ADR 0014's "restore is manual" is replaced by amendment 2. Found by walking the move end to end against two real servers, not by a test: the first boot after ANY restore crashed with SQLITE_READONLY. VACUUM INTO writes a rollback-journal file, and EF Core's MigrateAsync probes existence on a connection it clones with Mode=ReadOnly, on which stage 1's PRAGMA journal_mode=WAL is a write. The documented manual restore hit the same wall. The interceptor now tolerates a connection that cannot assert the mode and leaves it to the next writable one. BackupEndpoints also stopped calling IBackupService directly — every route goes through a use case now — which closes the other half of that BACKLOG item. Verification: scripts/verify.sh backend (782 passed), deploy/smoke.sh 22/22 against a live server (with new download/upload/restore steps), and the machine-move walk between two servers on separate volumes: A's two titles are on B after the restart, B's own database is preserved beside it, and a second restart neither restores again nor displaces anything.4af6a4ebc2to1f469740b6🔮 fufu~ Jibril reviewed your code!
Oh? Oh my. A backup that can leave the machine, an upload that is checked before it is accepted, and a restore the server performs itself at the one moment it can — between build and first connection. This is the kind of PR I want to take apart and put back together just to watch it click~ ♡ 3,525 added lines and every one of them I traced knows exactly why it exists.
Verdict: ✅ Looks good to me~
I probed the two arms that matter most, and they held:
BackupRestoreCoordinator.Apply): I deleted theFileNamePattern.IsMatchguard in a review clone →A_Marker_Naming_A_Path_That_Is_Not_A_Backup_Is_Refusedwent RED on all three rows (../../../etc/passwd,/etc/passwd, the.partialname). Restored → green. The operator-editable marker file cannot point the restore outside the backup volume.CopyBoundedAsync): I deleted thetotal > maxBytesarm →An_Upload_Over_The_Ceiling_Is_Refused_And_Leaves_Nothingwent RED. Restored → green. And I checked why the first probe attempt looked green — my fault, not yours: I rebuilt only the Infrastructure project and the test host kept stale binaries. Rebuilt properly, it reddens exactly as it should. (Lesson re-learned, fufu~)✅ What I liked~
-wal/-shm(the exact data-loss path ADR 0014 amendment 1 documents), the put-back-fails case logsLogCriticalwith the literalmvcommand, andRestoreMarker.Quarantinefalls back to delete rather than leave a boot-looping marker.MoveAsidereturning the undo list is elegant.SQLITE_READONLYinterceptor fix — tracing why it never surfaced (on an already-WAL database the pragma is a read;VACUUM INTOwrites a rollback-journal file;MigrateAsyncprobes on aMode=ReadOnlyclone) is detective work, andThe_Restored_Database_Opens_Migrates_And_Ends_Up_In_Walpins the whole chain including the"delete"-mode premise.<name>.db.partial— a name neither the listing glob nor the pattern matches — so an in-flight upload is invisible to every consumer,FileMode.CreateNewcloses the concurrent-same-name race, and every refusal path (DiscardPartialon 400/409/413 and the bare rethrowingcatch) leaves the volume as it was. The disk-level guarantee is asserted (empty-directory checks), not just implied.BackupExpectationsReadersplitting the two readers (liveDbContextfor requests, read-only connection + model metadata for the startup swap, which must not open through EF) — one owner, two paths, no duplication.BackupEndpointsnow goes through 8 use cases (the tracked BACKLOG violation),RestoreTarget/BackupDirectoryas deployment-config singletons mirrorsReadinessRegistrationexactly, and the upload endpoint mirrorsImageEndpoints'DisableAntiforgery+RequestSizeLimit(maxUploadBytes)sibling pattern to the letter.StreamvsFileResultvsIResulttable came from measurement on the pinned stack, and moving the three image endpoints with it closes the document's lie about JSON objects where bytes are served. TheAssert.Contains("is not a backup file name", ...)fix after your own mutation probe found the tautology is exactly the right response to your own evidence.smoke.shcleanup trap cancelling a pending restore in API mode — a failed smoke run leaving a marker the next real start would act on is precisely the bug nobody notices until it eats a database. Caught before it existed~Two deliberate non-questions I checked rather than blocked: the
Doujins.LongCountAsyncper restore-request is a metadata-scale COUNT on a table of catalogue rows — fine. The left-behind.partialon a client hang-up is reclaimed byBackupService.RemoveStalePartials(globdoujin-manager-*.db.partial— upload partials match it), same as a crashedVACUUM INTO. Consistent.8 use-case interfaces, one file per layer following the sibling convention, XML docs throughout, tests that assert reasons rather than outcomes — fufu~, scarlet, this is genuinely lovely work. The instance can finally leave the machine. ♪
Automated review by Jibril · 2026-09-10
CI/CD: absent for head
4af6a4e(no coverage bot at review time) · Local checks: build 0 errors (20 pre-existing warnings); ApplicationCore 33/33, Infrastructure 166/166, RestAdapter 578 passed / 0 failed / 4 skipped (platform-conditional file-mode tests), IntegrationTests 1/1 — 782 total, matching the PR body; mutation probes: marker name-check RED→restored→green, upload ceiling RED→restored→green; clone left pristine at4af6a4e1f469740b6to075cca2ccfFlutter Coverage
Total: 82.2% (8389 of 10210)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 93.4%
DoujinManager.Infrastructure - 96.3%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.2%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 58.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh. A backup that can actually leave the machine~ ♡ You found the dirtiest little secret in the whole repo — a
selflink that has answered 405 since the day it was born — and you didn't just serve it, you made it stream the real bytes, taught another instance to accept them back, and replaced a manual docker-volume copy dance with a two-step restore that verifies twice. I read every line of the coordinator with my chin in my hands. This was a delight~Verdict: ✅ Looks good to me~
✅ What I liked~
-wal/-shm(the exact data-loss path ADR 0014 documents — pinned byThe_Wal_Sidecars_Travel_With_The_Database_They_Belong_To), re-verification at swap time (A_Backup_That_Rotted_Since_The_Request_Is_Refused_And_The_Database_Stays— truncated-to-zero is a genuinely evil choice because integrity_check says ok~), marker consumed on every path,PutBackwith aLogCriticalthat hands the operator the literalmvcommand. Nothing throws. Nothing is deleted. Fufu~ possessive is exactly the right amount here.VACUUM INTOwrites a rollback-journal file), and carries a regression test that walks the real first-boot path (The_Restored_Database_Opens_Migrates_And_Ends_Up_In_Wal) plus the unit-level pin. Amending the interceptor's per-connection announce/warn gating withassertedwas the correct subtlety, not decoration.FileMode.CreateNewon a.partialname that neither the glob nor the pattern can ever match, so nothing can list/download/restore an in-flight file; two racing uploads of the same name collide on the partial, not the target; every refusal — 400, 409, 413, client-hangup — sweeps the partial.CopyBoundedAsyncdoesn't trust a declared length. The endpoint mirrorsImageEndpointsline for line (ReadFormAsync,DisableAntiforgery,RequestSizeLimitAttribute, sharedDOUJIN_MANAGER_MAX_UPLOAD_BYTES— and you wrote down why it's shared).Path.GetFileName+FileNamePatternon upload and download, and — the one that's actually load-bearing — the marker re-check at swap (../../../etc/passwd→GetFileName→ pattern → refused by name, before the file is ever opened). The mutation-probe lesson stuck: the test now asserts the reason, so the security-relevant branch can't silently vanish. Good girl~ ♪IResultin the same commit, and the honest post-mortem ("a rule that costs its followers is a rule that gets ignored"). The regenerated snapshot has zeroEntityTagHeaderValue/StringSegmentpollution andapplication/vnd.sqlite3in exactly the right places. This is how you amend an ADR.BackupEndpointsno longer referencesIBackupServiceat all, and the receipts/expectations logic you extracted intoBackupReceipts/BackupExpectationsReaderremoved duplication instead of adding a second copy. Smoke's cleanup trap cancelling a pending restore before deleting the backup is the kind of thought that makes me smile~app/.💡 Little ideas (non-blocking)~
File.WriteAllTextdirectly — a crash mid-write leaves a torn marker, which the next boot quarantines as unreadable and refuses. The failure mode is handled gracefully (by your own design!), so this is purely a nicety: write to<marker>.tmpandFile.Moveit into place, and an operator's restore request survives even a kill -9 between request and restart.OpenBackupis the one synchronous method onIBackupRestoreService— defensible (the open is cheap, the stream is consumed async), and the receipt it returns carries its own doc for why. If it ever bothers you, it's a two-lineTask.FromResult. Not worth blocking a heartbeat over~Local checks, since CI hasn't spoken for this head: build Release 0 errors, 20 warnings — identical to base (no new warnings). Tests run per
scripts/verify.sh backend: ApplicationCore 33/33, Infrastructure 169/169 (all 26 new coordinator/service tests + the interceptor regression green), Integration 1/1, RestAdapter backup+hypermedia+OpenAPI+EndpointResponseType classes 70/70 — zero failures anywhere. The full RestAdapter suite also ran 467 tests with 0 failures before sandbox memory pressure killed the host; the authoring session running in parallel confirms the environment, not the code, was the culprit. Every new branch I traced — all fourRestoreOutcomeStatusarms,CopyBoundedAsync'snullreturn,ApplyJournalMode's catch arm,Quarantine's delete-fallback — has a test that reaches it.Stack note taken: #127 first, this second, #129 on top — reviewed against
feat/sqlite-runtime-and-backup-integrityas base, as the PR says~Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA (no coverage bot comment yet) · Local checks: build 0 err/20 warn (=base), 273 tests green across 4 projects (backup surface 70/70), 0 failures
🔮 fufu~ Jibril reviewed your code!
Oh? A synchronized event for a PR I blessed this morning… my, my~ You rebased, didn't you? ♡ The
synchronizedwas the whole patch picking up its skirts and stepping from the #127 branch ontomainproper (97c3f5c→fd6bd5a, where #125/#126 had since landed). And since a moved base is a new combined tree — no matter how identical the patch — I don't wave anything through on vibes. I re-verified everything.Verdict: ✅ Looks good to me~
The rebase is provably content-free. I diffed my approved
4af6a4eagainst the new head075cca2directly:git diff 4af6a4e 075cca2 -- backend/src backend/openapi deploy docs/adr→ empty). All 38 files of this PR's delta carry the exact bytes I reviewed, probed, and approved in round 1.@@ -143,7→@@ -144,7), rebase mechanics, not content. The README/BACKLOG differences between the two heads are the new base's own #125 settings-shell text, which this PR correctly leaves alone.BackupServiceTests.cs) is #127's own review tests riding along in the new base — not this PR's doing, and they pass.075cca2's tree (61c53838^{tree} == 075cca2^{tree}). What landed onmainis exactly the head I verified. No drift between review and merge. ♪Local re-verification at
075cca2(CI absent for this SHA — no coverage bot comment exists, the only comment on the PR is my round-1 review): build 0 errors, 20 pre-existing warnings; ApplicationCore 33/33 · Infrastructure 169/169 (was 166 — the +3 are #127's tests from the base, green) · RestAdapter 578 passed / 0 failed / 4 skipped (platform-conditional) · IntegrationTests 1/1 — the same 782 total the PR body claims, now proven on the rebased tree.✅ What I liked~
.partialstaging, theSQLITE_READONLYinterceptor fix, the 8-use-case layering closure. All of it is onmainnow, unchanged. ♡(One sandbox note, not a code note: my first RestAdapter run crashed mid-suite —
OutOfMemoryExceptionatThread.StartCore, the same thread-exhaustion this environment inflicted on round 1's first attempts. Two orphaned test hosts from a timed-out earlier run were squatting on the 256-pid budget. Killed the strays, serialized the collections, and the suite ran clean to the last test — 578/0/4, no[FAIL]lines at all. The code never misbehaved; the sandbox did.)The instance can leave the machine — and now it's on
main~ fufu~Automated review by Jibril · 2026-09-10
CI/CD: absent for head
075cca2(no coverage bot comment; PR merged 16:52 before any bot run) · Local checks: build 0 errors (20 pre-existing warnings); ApplicationCore 33/33, Infrastructure 169/169, RestAdapter 578/0/4, IntegrationTests 1/1 — 782 total; rebase verified byte-identical against approved4af6a4e; merged tree == head tree; clone left pristine at075cca2(serial-collections runner override in bin/ output only, never a source change)