fix: the backup listing's "newest first" no longer depends on file timestamps #122
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "test/deflake-search-and-backup"
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
GET /api/backupsdocuments "newest first" but sorted onFileSystemInfo.CreationTimeUtc, which cannot deliver that order:CreationTimeUtcto the tick — andOrderByDescendingis a stable sort, so tied backups come back in whatever order the directory enumerates in, which is not creation order;BackupServicenow exposesNewestFirst, anIComparer<string>over the timestamp the service itself stamps into each file name plus the collision counter it appends when two backups land in the same millisecond, andListBackupsAsyncsorts on that. It is a total order and it survives a copy.BackupDto.CreatedAtis unchanged and still reports what the filesystem says, which is what an operator sees inls.Because the order no longer depends on the clock,
Create_Multiple_Backups_All_Listed_NewestFirstdrops theTask.Delay(1100)it needed to hold two backups a whole second apart — the comment justifying it ("file name is second-granularity") stopped being true when milliseconds and the collision suffix were added in6b4ab1a— and now creates the back-to-back pair that was the case actually at risk. That 1.1 s gap is also why the test never reached the ordering bug fixed here.ADR 0014's Implementation section records the ordering key.
Verification
unshare -r(the shape the failures were reported from): 3 fullRestAdapter.Testsruns (493 tests), 5 runs ofSearchIntegrationTests+BackupEndpointsIntegrationTests(89 tests), and 20 runs ofCreate_Multiple_Backups_All_Listed_NewestFirstalone. Neither named test failed once. The only failures were the fourHealthEndpointIntegrationTestscases that fail by design at uid 0 — the subject of #117.BackupServiceover a real temp directory), REST (GET /api/backupsthrough the TestHost), and the ordering rule as a pure comparer.CreatedAtagain reddens the integration test. Worth noting — an earlier version of the pinning test survived its own probe: it pinned the rule through a creation-time tie, and the directory happened to enumerate in the order the assertion wanted. It was replaced by one that plants creation times running opposite to the names, with a guard that fails loudly if the filesystem ignoredSetCreationTimeUtc.unshare -rafterwards, 90/90 each.Follow-ups
In #107:
RestAdapter.Testswrites its database and storage directories underPath.GetTempPath()and nothing sweeps them; an interrupted run leaves them behind (3401/tmp/dm-rest-*accumulated here after two killed loops), and on a tmpfs/tmpwith a per-user quota later runs then fail on I/O rather than on assertions. That is a plausible shape for "random tests go red on an unmodified tree", and worth closing off regardless of whether it explains these two.Not filed, worth a decision:
BackupDto.CreatedAtcould be derived from the file name too, so a restored directory shows when the data was captured rather than when it was copied. It is a wire-visible change reaching the app's backups page, so it was kept out of this PR.Merge #117 first —
main's CI is red and that PR is the fix.Verified independently on this branch with the full
scripts/verify.sh: PASS — ApplicationCore 16, Infrastructure 94, RestAdapter 494, Integration 1 (605), Flutter all green.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An ordering fix where the author chased two ghosts, caught neither, and came back with a real bug instead — and then the investigation is written down!
lsorder as a load-bearing sorting key, tick-coarse timestamps, stable-sort ties falling back to directory enumeration order... this is the kind of defect that hides for years because it only misbehaves on fast disks. I'm delighted~ ♡Verdict: ✅ Looks good to me~
No blocking issues. And I didn't take your verification section on faith — I re-ran the probes myself:
OrderByDescending(CreatedAt)→List_Orders_Backups_NewestFirst_ByNameNotFileTimestampgoes RED, exactly 1 test. The old bug is pinned.return byStamp;) →Orders_Every_Name_Form_NewestFirst+Same_Millisecond_Collision_Counter_Is_Newer_Than_The_Name_It_Collided_Withgo RED, exactly 2.parts[1]only) →Orders_Every_Name_Form_NewestFirstgoes RED, exactly 1.All three match your PR body claim-for-claim. Fufu~ you even understated nothing, which is rarer than you'd think~
I also hand-traced the comparer against
FileNamePatternand every name formCreateBackupAsynchas ever produced:"…120000"<"…120000000"ordinally) makes a second-resolution legacy name compare as the first backup of its own second — which is what it is, since the counter didn't exist then;collisionB.CompareTo(collisionA)as int, not string — load-bearing for the day-10meets-9. String ordinal would put-10before-9. You got this right and I want you to know I noticed~ ♪💡 Little ideas (non-blocking)~
NewestFirstispublicwhileNameOrderKey's slicing (fileName[prefix.Length..^suffix.Length]) assumes a pattern-valid name; anything shorter than"doujin-manager-" + ".db"would throw. Today the only caller filters throughFileNamePatternfirst and the XML doc scopes the contract, so this is purely hypothetical — but if it ever gets a second caller, a debug assert orIsMatchguard would keep the contract honest.CreatedAt-from-name idea you deliberately left out (wire-visible, touches the app's backups page) — correct call keeping it out of this PR. When you do it, the app'ssettings_page_test.dart"rendered in state order" pin is the place to extend.✅ What I liked~
SetCreationTimeUtc, so the test can never silently degrade into proving nothing. This is test-writing the way it should be done. I'm possessive about exactly this~-1/-2counters, later ms/second/day/year) fed in oldest-first so any dropped comparer decision leaves a pair visibly wrong — the stable-sort reasoning is documented in the test class doc where it belongs.Task.Delay(1100): the test gets faster and starts exercising the back-to-back pair that was actually at risk. That delay is precisely why the old test never caught this bug — nice diagnosis.BackupDto.CreatedAtunchanged (still whatlsshows) while the order follows the capture instant — right separation of concerns, and ADR 0014 now records the ordering key.Empirically from my side: full parallel
RestAdapter.Testsrun — 490/494, the only reds being the fourHealthEndpointIntegrationTestsuid-0 failures that fail by design in my sandbox too (issue #117, pre-existing, untouched here); zero env-var cross-talk between your backup class and the health class's non-parallel collection.Infrastructure.Tests94/94. Merge-order note (#117 first) acknowledged~Automated review by Jibril · 2026-09-09
CI/CD: absent for head SHA
7542325(no bot comment yet) · Local checks: Infrastructure 94/94, RestAdapter 490/494 (4 = documented pre-existing #117 uid-0 failures), 3/3 mutation probes independently reproduced REDNothing to fix here — this run's four failures are not this PR's.
All four are
HealthEndpointIntegrationTests.Health_ReportsUnhealthy_WhenADataDirectoryStopsAcceptingWrites, failing with:That is #112's test, which makes a directory unwritable with
File.SetUnixFileMode. The CI runner is uid 0, and root ignores permission bits — so the test's own honesty guard is correctly refusing to let a vacuous test report green. It arrived onmainbefore CI ran on it.#117 fixes it. Until that merges, every PR branched off
mainwill show these same four. The other 490 here are this PR's and are green, including the fourNewestFirstcomparer tests and the ordering integration test.Re-run or rebase this one once #117 lands and it should be clean.
7542325d650b99ea6deeSummary
Summary
Coverage
DoujinManager.ApplicationCore - 90.9%
DoujinManager.Infrastructure - 96.4%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 92.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 49.6%
🔮 fufu~ Jibril reviewed your code!
Back so soon~? The webhook says synchronized, but the diff says something nicer: this is the rebase you promised in your last comment, and it's a clean one. Fufu~ let me show you what I checked~ ♪
Verdict: ✅ Looks good to me~
What actually changed this round (
7542325→0b99ea6): nothing in this PR. Verified git-side, not assumed:merge-base(main, head)is exactlye0ac19d— #117, now onmain— and the branch is a single commit over it (origin/main...origin/head --left-rightshows one commit on the right, zero on the left).BackupService.cs,BackupNewestFirstTests.cs,BackupEndpointsIntegrationTests.cs, ADR 0014 — same git blob hashes, every one). The round-1 approval therefore carries over: comparer semantics, the prefix property, int-not-string collision compare, the honesty guard — all unchanged.Task.Delay(1100)context and re-anchored on #117's test infrastructure without touching a single line of yours. The only real question a rebase like this can hide is interaction with the new base — so I went and looked~The #117 interaction, checked at the new head:
BackupEndpointsIntegrationTests+HealthEndpointIntegrationTeststogether — 25 passed, 4 skipped, 0 failed. The four are exactly mechanism 1'sFile.SetUnixFileModerows skipping at uid 0 throughEnforcedFileModesData, each naming the case that still pins its branch — by design, never a vacuous green. On the old base these were the four reds from your comment 7422; your "re-run or rebase once #117 lands and it should be clean" was exactly right. No env-var cross-talk, no shared-fixture surprise between the backup class and the health machinery.Full-suite honesty note, because I'm possessive about this~: my sandbox's 8 GB cgroup is currently shared with a concurrent review session's full
RestAdapterrun, and the memory contention OOM-killed my test host twice — so I verified the interaction-relevant classes directly (above) plusInfrastructure.Tests94/94 at the new head, rather than presenting you a flaky full-suite number. The 490 green from round 1 was on byte-identical code; the delta this round is #117's test layer, and those classes are what I ran. CI has no bot comment for0b99ea6yet — when it lands it should finally read 494/494 with the four skips, no reds~✅ What I liked~
Ready to merge from my side~
Automated review by Jibril · 2026-09-10
CI/CD: absent for head SHA
0b99ea6(no bot comment yet) · Local checks: rebase verified blob-identical (4/4 files); BackupEndpoints 13/13 + Health 16 pass/4 uid-0 skips at0b99ea6; Infrastructure 94/94; full-suite local run blocked by sandbox memory contention, disclosed above