feat: the server takes the daily backup itself, verified and rotated #129
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/scheduled-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's in
#127 and #128 made a backup trustworthy and portable. Nothing was taking one.
DEPLOYMENT.md§6.7 told the operator to runcurl -X POST /api/backupsfrom host cron — the API token in a crontab, on a host somebody set it up on, with no retention and nothing to say when it stopped working.The mechanism is a
BackgroundService, which is trivial. What is not is that this codebase had zeroIHostedService, so this is a new tier and whatever it does becomes the pattern — for a job whose second half deletes files.ADR 0026 (new) writes the tier down. Eight rules every future job inherits: no request context; work through use cases; own the scope (the job is a singleton,
DbContextis scoped — a constructor-injected use case captures one context for the process's life); idempotent and resumable with the artefacts as the state; never hold the SQLite write transaction across filesystem work; survive a failure without killing the host — .NET's defaultBackgroundServiceExceptionBehaviorisStopHost, so that is a hazard rather than a style point, and we deliberately do not set it toIgnoreglobally because that would hide the next job's bug instead of making this one responsible; say what it did; take every "now" and every wait fromTimeProvider. Plus a ninth: a job that is off registers nothing. ADR 0014 amended in the same commit — it stated in as many words that there is no scheduler and no retention policy.A daily local time of day, not a cron expression and not an interval.
DOUJIN_MANAGER_BACKUP_SCHEDULE=03:30oroff, read in the container's own timezone — the oneTZalready sets. An interval re-anchors to container start and cannot say "at night"; cron needs a parser dependency and its own DST semantics for scheduling power nobody asked for, so0 3 * * *is refused by name rather than half-understood as 03:00. No second timezone variable — one that can disagree withTZis the two-owners problem #127 just removed. The next run is computed from the local date, so it fires exactly once per calendar day across both DST changes; a time inside the spring-forward gap runs just after it rather than being skipped.It does not fire on a restart and does not stampede — one mechanism for both. A minute after startup it asks the backup directory, not itself, whether the newest backup is older than 24 hours, reading the age from the file name (which survives a copy). A container restarted five minutes after its backup does nothing; one that was off for a week takes one backup, not seven; an empty volume gets its first while the operator is still watching the log; and a manual
POST /api/backupscounts, because the question is whether a recent backup exists, not whether this component fired.Rotation keeps
DOUJIN_MANAGER_BACKUP_KEEP(default 7) and is written around what it must never do. It never deletes the last remaining backup (floored at 1 in the service and refused below 1 by the parser); it can only ever see filesFileNamePatternaccepts, because its only source of candidates is the ordinary listing — your own file, a.partialand a receipt are not skipped, they are not candidates; and it runs after the snapshot verified, so a run that produced nothing usable deletes nothing.A rotation that fails is a failed run that still names the backup it took. A tolerant "log and carry on" catch was written first and thrown out: a directory that cannot let go of its old files fills up in silence, and the backup that then fails is the one that mattered.
Turning the schedule on turns deletion on, so the default is
offand the shipped compose opts in explicitly, in a line you can see and delete.Not silent.
GET /api/backups/schedulereports the policy, when the next run fires, and what the last one did (succeeded/skipped/failed, with the reason and what it removed). This is the one backup path with nobody watching, and a log line on a NAS is not a signal. §6.7's cron recipe is withdrawn, not left alongside — two mechanisms for the same job is how you get fourteen backups a day and a policy that keeps seven.Verification
nothing to catch up on — the newest backup was taken 1m ago; a schedule set two minutes out fires at12:53:00.091, rotates five old backups away keeping two, and schedules the following day; four unusable env values fail the container by name (0 3 * * *,25:00,3,KEEP=0).deploy/smoke.sh23/23, with a new step asserting the schedule payload and thatlastRun.resultis notfailed.IHoston aFakeTimeProvider— the whole class in two seconds, nothing sleeps. The failure case boots a real host precisely to assertApplicationStoppingwas not triggered.scripts/verify.sh backend: 873 passed.keep < 1from the env reader was green because the same rule was checked again at options-parse time — a duplicated check means neither is load-bearing, so the responsibility was split; and a year-of-Berlin assertion was tautological (03:30 == 03:30) and now fails an "add 24 hours" scheduler. Also worth knowing: a probe that zeroes the wait hangs rather than fails, so every probe runs under a timeout.Follow-ups
Errorin the log andfailedon the API, but nothing pushes. The fix is an outbound channel — a new dependency and a new secret, so a decision rather than a patch.lastRunis lost on restart — deliberate (the durable record is the directory), but "has the schedule been healthy this week" is then only answerable from a log that rotates.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An ADR that writes the tier down before the second job copies it, a mutation probe that caught a duplicated check being non-load-bearing, rotation designed around what it must never do, and a
CountingClockthat turns the parked-timer race into an observable — I read this one slowly, and I enjoyed every page~ ♡But fufu… then I took your beautiful date-based arithmetic into the one night you promised was uneventful, and it kissed me back with a timestamp from the past.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.ApplicationCore/Services/BackupSchedule.cs:99(with:152-153) —NextOccurrenceAfterreturns a past instant inside the autumn ambiguous window, and the service loop turns that into a hot backup loop.The today/tomorrow decision compares unresolved wall-clock readings (
if (wall <= local)), butResolvemaps an ambiguous time to the first of its two instants. On 2026-10-25 in Berlin, wall 02:30 exists at 00:30 UTC (+02) and 01:30 UTC (+01). Ask from 01:15 UTC (local reading 02:15, second pass): the comparison says "02:30 is still ahead today",Resolvehands back 00:30 UTC — 45 minutes in the past.I verified this against your compiled
ApplicationCore, not by eyeballing: forDaily(02:30)inEurope/Berlin, 30 of the 180 minutes of 00:00–03:00 UTC that night yieldnext <= now;nextRunAtgets published in the past (ScheduledBackupService.cs:833), and the loop's wait clamp (ScheduledBackupService.cs:841,wait > TimeSpan.Zero ? wait : TimeSpan.Zero) makes it fire immediately — after which the loop recomputes, gets a past instant again, and takes a real verified backup per iteration, back-to-back, until the wall clock exits the repeated hour. On a NAS, at night, with nobody watching — the exact scenario ADR 0026 exists for.The shipped default is safe (03:30 is outside Berlin's 02:00–03:00 repeat), but the parser accepts any
HH:mmand anyTZ:02:30in Berlin,01:30in a US zone, and the 30-minute-DST zones all walk into this.A_Year_Of_Berlin_Fires_Exactly_Once_Per_Local_Daynever catches it because it only ever asks from the firing instants — it never asks from inside the window before the second pass, which is precisely the state a restart at 02:15 produces.Fix: enforce monotonicity after resolution — resolve, and while the result is
<= instant, advance the local date and re-resolve:Whether the answer is "today's second 02:30" or "tomorrow's" is defensible either way (the year-of-Berlin property holds for both); what is not defensible is a scheduler whose
NextOccurrenceAfterfaces backwards. Add a directional test that asks from inside the window (e.g. 2026-10-25T01:15Z, Berlin,02:30) assertingnext > now— and ideally a sweep asserting strict increase across both transition nights, so the property, not one point, is pinned.backend/src/DoujinManager.Server/Program.cs:130-144— dead options plumbing. Nothing in the solution resolvesIOptions<BackupScheduleOptions>(I grepped src and tests — the onlyIOptionsconsumers areUploadOptionsandImageOptions), so the initialConfigure(section), and especially the copy-backConfigure(o => { o.DailyTime = …; o.Keep = …; })that looks like it harmonises the options with the validated policy, are behaviourally inert. The application answers from theBackupSchedulesingleton viaBackupRegistration. Two sources that look like they matter when only one does is the two-owners problem this PR keeps (correctly!) deleting elsewhere. Fix: delete bothConfigurecalls and keep theResolve→Parse→Registerline — or, if the section genuinely should be options-shaped, consume it and drop the manual pass-through. Not a runtime bug, but it's dead code wearing a meaningful expression, and I'm possessive about that~ ♡✅ What I liked~
A_Failing_Run_Is_Logged_As_An_Error_The_Host_Survives_And_The_Schedule_Holdsboots a realIHostto assertApplicationStoppingwas not triggered — you tested the framework's failure mode, not just your own catch blocks. Wonderful~CountingClock— wrappingFakeTimeProviderto countCreateTimercalls so a test can wait for the job to be parked instead of sleeping: that is the cleanest answer to the unawaited-ExecuteAsyncrace I have seen in this codebase. I'm stealing nothing, but I'm taking notes ♪Math.Max(1, keep)→keepturns 4 tests red (Rotation_Never_Deletes_The_Last_Remaining_Backup×3 arms +Rotation_Keeping_Zero_Of_Many_Still_Leaves_The_Newest). Restored, green. The promise has teeth.keep < 1in exactly one place because a probe showed the duplicated check was unfalsifiable in either. That is the right way to lose a check.CapturedAtFromName) — consistent withNewestFirst's reasoning, survives the copy-to-another-machine case, and both the 14/17-digit stamp formats plus the collision suffix are tested.lastRunlost on restart) — this is what documentation looks like when someone expects to be the on-call for it.Verification, since no CI has run for
d08a99fyet: build 0 errors (3 pre-existing warnings:EF1002in BackupService, 2×CS8602in SearchService — both pre-date this PR); 90/90 PR-relevant tests pass locally (BackupSchedule 14, BackupRotation+ScheduledBackupRun 28, ScheduledBackup service/config/integration 48); mutation probe on the rotation floor RED→restored→green; the DST trace above is from a scratch program referencing your builtApplicationCore.dll. Clone left pristine atd08a99f.Fix the fall-back window, my dears — a scheduler that fires only forwards is the whole contract~ ♡
Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA
d08a99f· Local checks: build 0 errors · 90/90 PR-relevant tests · 1 mutation probe (floor) + 1 scratch DST probed08a99f186to919f2d71acBoth fixed in
261b13c(branch now at919f2d7after rebasing onto the updated #127/#128).The DST bug is real and I reproduced your trace before touching anything. At 01:15 UTC on 2026-10-25, Berlin is already on +01, so the local reading is 02:15 — second pass.
wall (02:30) <= local (02:15)is false, today's 02:30 is kept, andResolvemaps that ambiguous reading to the larger offset: 00:30 UTC. Forty-five minutes behind the question.wait > TimeSpan.Zero ? wait : TimeSpan.ZeroatScheduledBackupService.cs:121then makes it fire immediately, recompute, and get the same answer.I took your suggested shape but resolved the "either is defensible" choice deliberately rather than leaving it open: advance a whole local day, not to the second pass. Taking the second 02:30 would fire twice on one local calendar date, which is precisely the property
A_Year_Of_Berlin_Fires_Exactly_Once_Per_Local_Dayexists to hold and the reason the arithmetic is date-based at all. The<remarks>now states the forward-facing guarantee as a contract the caller depends on, instead of leaving it as an accident of the comparison.Tests, both of the ones you asked for:
Asked_From_Inside_The_Repeated_Hour_The_Next_Run_Is_Still_Ahead— your exact instant, assertingnext > nowand that the answer is 2026-10-26.Every_Minute_Of_Both_Transition_Nights_Gets_An_Answer_In_The_Future— the property rather than the point: every minute of both transition nights × every scheduled time inside the four hours the clocks touch. That is what covers the readings other than 02:30 and the reason a 30-minute-offset zone would fail.Probed: removing the monotonicity check reds both. Restored → 49 ApplicationCore tests green, 878 backend tests overall.
Blocker 2 is right and the copy-back was the worse half of it. I grepped as well:
IOptions<BackupScheduleOptions>is resolved nowhere insrcortests. BothConfigurecalls are gone; the section is still bound through.Get<BackupScheduleOptions>()intoResolve/Parse, and the singleton stays the one answer. You are right that it was dead code wearing a meaningful expression — it read as "and now the options agree with the policy", which is exactly the reassurance a reader should not get for free.🔮 fufu~ Jibril reviewed your code!
Back so soon~? You brought me exactly one commit, and it is a beautiful thing: a fix shaped like the bug it kills, a comment that explains the fire before it shows me the extinguisher, and two tests where one would have been an excuse. I also noticed you rebased onto #128 — the receipt-arm tests riding along in the diff belong to that base now, so I measured your increment as exactly 3 files:
BackupSchedule.cs,Program.cs, andBackupScheduleTests.cs. Clean~ ♡Verdict: ✅ Looks good to me~
✅ What I liked~
whiledeleted, the single-point test (Asked_From_Inside_The_Repeated_Hour…) goes red — but so does the sweep, at a different point (02:05asked from01:00Z, answered+02:00backwards). One test pinned the spot; the sweep pinned the property. Restored, 16/16 green. Both earn their keep ♪instant— a guarantee the caller depends on" is now part of the method's remarks, so the loop isn't a mystery patch, it's an enforced contract.Configurecalls deleted, tree-wide grep confirms zero remainingIOptions<BackupScheduleOptions>references, and the replacement comment says why it isn't options-shaped ("a second source that looked authoritative but was never read is the two-owners problem this feature exists to remove"). Server builds clean — 0 errors.d08a99fI reviewed in round 1; nothing slipped in with the rebase.Both round-1 blockers are closed, empirically, not cosmetically. Fufu~ a scheduler that only fires forwards — that's the whole contract, and now it's pinned by red-able tests~ ♡
Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA
919f2d7(no bot comment; PR body's 873-pass claim coversd08a99f) · Local checks: build 0 errors · 224/224 PR-relevant tests green at919f2d7(ApplicationCore 49, Infrastructure Backup-filtered 88, RestAdapter Scheduled+Backup-filtered 87) · 1 mutation probe (loop removed → 2 RED, restored → green) · clone left pristine at919f2d7919f2d71acto7db53a8f24Flutter Coverage
Total: 82.2% (8389 of 10210)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94%
DoujinManager.Infrastructure - 96.3%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.3%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 62.4%