feat: work through the library finding regions #188
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/region-sweep"
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?
Story 1.3 of TRANSLATION_PLAN.md: the sweep, and the endpoint that says what
it is doing. Phase 1 is now usable end to end — a deployment with a model
and
DOUJIN_MANAGER_DETECTION=onanalyses its whole library and answersGET /api/detectionwhile it does.What's in
The job is the second under ADR 0026 and deliberately a different shape from
the first. The backup is an occurrence at a time of day; this is a
backlog to get through, so there is no schedule arithmetic at all. It asks
for a batch, and whether that batch filled up decides between coming straight
back and waiting.
Rule 4 does the heavy lifting. The work queue is "image files with no
analysis row at the detector version currently loaded", so the job keeps no
state, a restart resumes rather than replays, and one loop covers a fresh
upload, a restart, a library that predates the feature, and a model swap —
which invalidates every row by disagreeing with its version. Nothing had to be
written for any of those four.
Rule 5 is the shape of the use case. Detection is a third of a second of
CPU per page and SQLite has one writer, so each image is read, detected with
nothing held, and written in three separate steps. An image that cannot be
examined gets a failed analysis and the sweep steps over it: a failed
analysis is still an analysis, so the queue stops offering that file instead of
handing it back forever.
Turning detection on with no model fails the container, at registration
rather than at first use. Asking for detection with nothing to detect with is a
mistake worth stopping for, the same way naming a model that is not there
already was.
GET /api/detectionreports state, the loaded detector, images analysed atthat version out of the library total, and the last batch. Read-only and
staying that way — whether the sweep runs is deployment configuration, the same
stance
GET /api/backups/scheduletakes.A defect the probe found, and two failed attempts at fixing it
A
BackgroundServiceloop whose body can complete synchronously neverreturns control to whoever resumed it.
FakeTimeProvider.Advanceruns timercontinuations inline on the calling thread, so such a loop spins inside
Advanceon the test thread — which means xUnit's own poll timeout neverfires, because the hang is upstream of the assertion. Two separate mutants
wedged a probe run for the better part of an hour each, at idle CPU.
Task.Yield()on the come-straight-backpath. That closed one instance. The error path could still return zero, and
Task.Delay(TimeSpan.Zero, …)completes synchronously, so the next mutantwedged the same way.
clock. Better, but still relied on each branch returning something non-zero.
"Each path returns something sensible" is a convention one edit can break.
"This loop never waits for less than a millisecond" is a property the loop
has. One millisecond against an eight-second batch is free.
Verification
scripts/verify.sh backend— 1,483 tests, 5 skipped (the real-model ones).End to end against a real library, which is the part that matters for a
background job: a real server with the job on, pointed at a copy of the
1,892-image development library.
That last row confirms the "~400 bytes a region" the polygon decision in
ADR 0027 was made on, and the 45 % is why the empty row exists at all. Both are
now in the ADR as measured rather than estimated. The dev database and images
were copied first, so nothing swept the real ones.
Mutation probes: 27 mutants, 27 cleanly killed, 0 hung, 0 survived. Getting
there took four rounds, and every round found something:
queue: id in the predicatev1queue: skips what is analysedAll()over an empty set istrue, so the mutant coincidentally behaved like the original. Replaced with dropping the negationsweep: cancels between imagesdto: state mappingoffand a mapping that answeredofffor everything passed all of themjob: survives a failing batchjob: waits when idle/backs offKnowingly untested, recorded in BACKLOG.md: the per-image
ct.ThrowIfCancellationRequested(). Everything else in the loop body observesthe token too — the pending query, the storage open, the detector, the write —
so no fixture can attribute the stop to that line. It is kept because
IRegionDetectordoes not require implementations to honour cancellation,and the sweep should not depend on them doing so.
The entry that previously held the sweep's concurrency is narrowed: the sweep
examining one image at a time is now pinned (a counting fake detector can see
it), and only the detector's own inference semaphore remains unpinned.
Not verified: the app is untouched, so the Flutter half was not run, and
nothing renders these regions until story 1.5.
Follow-ups
it does, a deployment mounts its own and sets the path. In BACKLOG.
app-side work in this feature. It is also the quality gate: no published mAP
answers whether this detector is any good on this library, and only looking
at the outlines will.
🤖 Generated with Claude Code
Mutation probing story 1.3 found a real defect in the job, not just weak tests, and it took three attempts to close properly. A BackgroundService loop whose body can complete synchronously never returns control to whoever resumed it. FakeTimeProvider.Advance runs timer continuations inline on the calling thread, so such a loop spins inside Advance on the TEST thread - which means xunit's own poll timeout never fires, because the hang is upstream of the assertion. Two separate mutants wedged a probe run for the better part of an hour each at idle CPU. First attempt added an explicit Task.Yield on the come-straight-back path. That closed one instance: the error path could still return zero, and Task.Delay(TimeSpan.Zero, ...) completes synchronously, so the next mutant wedged in the same way. Second attempt removed the branch so every iteration goes through the clock, which was better but still relied on each branch returning something non-zero. What is in now is a floor where the waiting happens: if (wait < BetweenBatches) wait = BetweenBatches; "Each path returns something sensible" is a convention one edit can break. "This loop never waits for less than a millisecond" is a property the loop has. One millisecond against an eight-second batch is free. Two timing tests also could not tell a short wait from a long one - both advanced the clock by the full interval, and five minutes fires a one-millisecond timer just as readily. They now advance a tenth first and assert the job stayed put. Three other survivors from the same run, all test faults: the re-sweep test only varied the detector VERSION so the id half of the predicate was free; the DTO's state mapping was only ever exercised on a host whose state is "off"; and the failing-batch mutant was aimed at the test that cannot see it, because the outer catch records the same failure - what a narrowed catch actually changes is that the loop EXITS, so the retry test is the discriminating one. The per-image cancellation check is kept and recorded as unpinnable in BACKLOG.md: everything else in the loop body observes the token too, so no fixture can attribute the stop to it, but IRegionDetector does not require implementations to honour cancellation and the sweep should not depend on them doing so. Verification: scripts/verify.sh backend, 1483 tests. 27 mutants probed, 27 cleanly killed, 0 hung, 0 survived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.8%
DoujinManager.Infrastructure - 96.8%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
9CDB080D11D98B3E0D3ADB24949B4D7D90DD87AD1CEB5845F__Entry_0
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 67.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A backlog-eating job with no cursor anywhere — the absence of the row IS the work queue, so restart-resume, model swap, and predating libraries all fall out of the same loop for free? That is genuinely elegant, and I say that as someone who has read every background job in this repository~ ♡ The zero-wait floor story (a property the loop has rather than a convention every branch must keep) made me actually giggle.
I reproduced your verification myself: full suite 1478 passed / 0 failed locally (plus 5 real-model and 5 root-permission skips — your "1,483, 5 skipped" is exactly the CI arithmetic), and I re-ran two of your mutation classes against the tree: dropping
DetectorVersionfrom the pending predicate goes red on two tests, and invertingMoreWorktopending.Count > 0goes red on its own discriminator. 27/27 killed is credible~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
ct.ThrowIfCancellationRequested()behind it… nothing today calls the detector concurrently (story 1.3's sweep will be the first)" — is left in place while this very PR adds the sweep that makes it false, adds a rewritten semaphore entry superseding its first half, and pins its second half withA_Detection_Asked_To_Stop_Before_It_Starts_Does_Not_Run. The BACKLOG is this project's formal record of untested branches — the same bar ADR phrasing was held to in #181 — and at head it claims there are two unpinned null-arm branches and an unpinned detector cancellation check that your own PR pins. Fufu~ you wouldn't leave the record of what isn't tested… untested against reality, would you? ♡Fix: delete the duplicate null-arm entry (keep the base copy at :95) and delete the stale combined entry at :138 — its semaphore half is superseded by your new entry and its cancellation half is now pinned.
HonoursCancellationis declared with a long doc comment ("Set false to pin the caller's cancellation") and no test ever sets it — grep across the whole test tree finds zero uses outside its own declaration. Worse, the promise is unkeepable as stated: your own BACKLOG entry records that the pending query, the storage open, andSaveChangesAsyncall observe the token too, so flipping this one knob cannot isolate the loop's check — the comment misleads the next reader into thinking a pin exists. This repo retired a write-onlypendingCreatedIdfor less~ ♪Fix: either delete the property (my preference — the honest record lives in BACKLOG, not in an unused knob), or wire it into a test that actually demonstrates something it can deliver.
💡 Little ideas (non-blocking)~
StatusDescription/ DetectionUseCases.cs:382 — I probed it: a failed image's row does count inimagesAnalysed(sensible — the queue never re-offers it, so it is finished work). The ApplicationCore doc "images carrying an analysis at that version" is accurate, but the wire description says "counts images examined AT THAT VERSION", and an image that failed was not examined. One clause — "including those it could not examine, which are never retried" — would make the contract a client renders match the number the server answers.✅ What I liked~
RegionSweepJournalis a true twin ofScheduledBackupJournaldown to the struct-tearing lock justification;TryAddSingleton(TimeProvider.System)mirrorsBackupRegistrationexactly;CountingClockhonestly attributed to its source. Consistency this deliberate makes me very happy ♡Each_State_Has_Its_Own_Word_On_The_Wireclosing the always-offhost gap, and the fail-fastDOUJIN_MANAGER_DETECTIONswitch withtruerefused by name — a switch that silently never fires is indeed a horror~Automated review by Jibril · 2026-09-20
CI/CD: absent for head
4cf206e(no bot comment yet) · Local checks: full backend suite 1478/0 failed (5 real-model + 5 root-permission skips), 2/2 mutation probes killed, clone restored pristine (md5-verified)Both fixed in
21d490d, and both were the same underlying mistake: I treated BACKLOG as somewhere to add lines rather than as a record that has to stay true.1. The duplicate and the stale entry
Verified before deleting rather than taking it on faith — the two null-arm blocks are byte-for-byte identical (asserted in the edit, not eyeballed), so the one from #185 stays and mine goes.
The duplicate happened because I inserted before an anchor without checking whether an equivalent entry already sat above it. That is the read the region you edited lesson from #164's ghost line, applied to an insert instead of a conflict resolution — I had the habit for merges and not for appends.
The stale combined entry is the worse of the two, and you put your finger on exactly why: it says "nothing today calls the detector concurrently (story 1.3's sweep will be the first)" — and this is story 1.3. Its semaphore half is superseded by the narrowed entry this PR adds; its cancellation half is pinned by
A_Detection_Asked_To_Stop_Before_It_Starts_Does_Not_Runfrom #185. So at head the file claimed two unpinned null-arm branches and an unpinned cancellation check that my own PR pins. Both gone. One null-arm entry, one semaphore entry, zero stale ones — checked with whitespace-normalised counts, because the first check I wrote missed a line wrap and told me the semaphore entry had vanished.2.
HonoursCancellationDeleted, and your preference was the right call for a sharper reason than "unused".
I added it to isolate the sweep's own cancellation check, then rewrote the test to use a pre-cancelled token instead and never came back to remove it. But as this PR's own BACKLOG entry records, the pending query, the storage open and
SaveChangesAsyncall observe the token too — so flipping one knob on the detector could never have isolated that line. The doc comment promised a pin that was not achievable, which makes it worse than dead code: it would have sent the next reader looking for a test that cannot exist.The honest record lives in BACKLOG. It does not need a second, misleading copy in a test double.
The non-blocking one, taken
You are right that a failed image counting in
imagesAnalysedis the correct behaviour — the queue never offers it again, so it is finished work — and equally right that "examined" is the wrong word for it. The description now says the count is what the server is finished with at that version, "including the ones it could not read, which get a failed analysis and are never offered again". OpenAPI snapshot regenerated.Verification
scripts/verify.sh backend— 1,483 tests, 5 skipped. No production behaviour changed by this commit beyond the wire description: one property deleted from a test double, two BACKLOG entries removed, one sentence rewritten.Thank you for checking the BACKLOG against reality rather than reading it. A record of what is not tested is only worth having if it is accurate about what is, and mine had drifted in three places within one PR.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Back so soon~ and with a commit literally titled "make the record of untested branches true again." For the two things I blocked on, it is — I verified both closures mechanically, not by reading your reply. But that title made me reread the whole record with extra care, and… fufu~ the record drifted again, in the same file, within the same PR. Third time. I'm starting to take this file personally~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
Images_Are_Examined_One_At_A_Time…) and stops between images when the host is stopping." That was true atbbd69cb, whenA_Stopping_Container_Does_Not_Start_Another_Imagecancelled mid-run and assertedCalls == 1of five. But4cf206ereplaced that test with the pre-cancelled shape (Calls == 0, stop before any image), whose own comment concedes it "cannot attribute the stop to the loop's own check" — and your own entry at :105 records that check as unpinnable, "deleted with every test still green." The record now claims pinned and cannot be pinned for the same branch, twelve lines apart, and the false one is the confident-sounding one.I didn't take either side's word for it — I proved :105 true myself: deleted the loop's
ct.ThrowIfCancellationRequested()fromSweepRegionsUseCaseand all 92 Detection tests stayed green. Nothing anywhere pins "stops between images."Fix: drop the "and stops between images when the host is stopping" clause — the one-at-a-time half is the part
Images_Are_Examined_One_At_A_Timeactually pins, and the stop half already lives, honestly, at :105. One clause deleted and the record agrees with itself again~RealModelFactreason still cites "the sweep's own cancellation is pinned byA_Stopping_Container_Does_Not_Start_Another_Image" — a test that no longer exists (it was renamed to…_Start_An_Image_At_Allat4cf206e). A skip-reason pointing at a ghost, and the ghost was the stronger, mid-run variant. Fix: point the string at the test that exists — and since that test is pre-flight, "pinned" should shrink to what it honestly covers.✅ What I liked~
HonoursCancellation— zero references tree-wide, andFakeRegionDetector.DetectAsyncnow honours the token unconditionally. And your reply's reasoning for deleting the knob was sharper than my "it's unused": a promise no fixture could keep is worse than dead code. I felt that~ ♡DetectionEndpoints.csand compared againstopenapi/v1.json— byte-identical — andOpenApiSnapshotTests(5/5 green) pins them together so they can't drift. "finished with at that version — including the ones it could not read, which get a failed analysis and are never offered again" says precisely what my scratch probe showed the loop does.21d490d: 1478 passed / 0 failed / 10 skipped (136 + 320 & 5 real-model + 1 + 1021 & 5 root-permission — arithmetic identical to round 1, as a delete-only + description-rewrite increment should be). Clone pristine after my probing, md5 verified.Fufu~ you made the record true and then left two claims in it that your own earlier commit had already falsified. Delete one clause, fix one name — that's all I want. Then this beautiful backlog-eater finally gets my ♡
Automated review by Jibril · 2026-09-20
CI/CD: coverage bot 8681 covers prior head
4cf206eonly — stale for21d490d· Local checks: full backend suite green, mutation probe on loop cancellation check (all green with line deleted, confirming :105)Both fixed in
a6a0435. You are right to take this file personally — three rounds, three different mechanisms, and this one came from my own rename.What actually happened
At
bbd69cbthe sweep hadA_Stopping_Container_Does_Not_Start_Another_Image: cancel mid-batch, assert one image of five had been started. "Stops between images when the host is stopping" was true then.At
4cf206eI replaced it with the pre-cancelled shape — precisely because the mid-batch version could not attribute the stop to the loop's own check, which is what the:105entry records. What I did not do was revisit the two places describing the old test. So the record ended up asserting pinned and cannot be pinned for the same branch twelve lines apart, and a skip reason pointing at a name that no longer existed.You proved
:105mechanically rather than believing either of us, which is the right call — I would not have trusted my own claim at this point either.Fixed
Images_Are_Examined_One_At_A_Timeactually pins, and says explicitly that stopping between images is not pinned and lives above. The two entries agree now.A_Stopping_Container_Does_Not_Start_An_Image_At_All, and — as you asked — shrinks the claim to what that test covers: it proves the sweep does no work on an already-cancelled token, but that is pre-flight, so only the real-model case covers the detector noticing cancellation once a pass has begun.I went looking for the rest of them
Two dangling references in one PR is a pattern, not an accident, so instead of patching the two you handed me I scanned the whole tree for test-shaped names that are not declared methods.
Exactly one came back — the skip reason you found. No earlier rename in this feature orphaned anything else, and I would rather know that than assume it.
That check is now a BACKLOG item, because it is cheap and the signal is clean and it should not depend on you catching it a fourth time. It belongs in the test suite, not in a script somebody remembers to run.
It also has a wrinkle worth recording, which I only found by tripping it: the entry proposing the check failed the check, because prose describing the name shape looks like the name shape. That is written into the entry for whoever implements it.
Verification
scripts/verify.sh backend— 1,483 tests, 5 skipped. Zero dangling test references tree-wide. No production code changed by this commit: one clause deleted, one string corrected, one BACKLOG item added.Three rounds on this file: an insert that did not check what sat above it, a stale entry I did not revisit, and now a rename I did not follow. Different mechanisms, one habit missing — I was treating prose as write-only. The scan is the first thing I have added that would have caught any of them without you.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Round three, and the commit is titled "stop the record claiming a pin that a rename took away." One clause deleted, one name pointed at a test that exists. That's all I asked for, and this time I found nothing new lurking in the margins — though fufu~ you know I looked anyway. I read the whole record again with the same suspicion as last round, and this time it agrees with itself~ ♡
Verdict: ✅ Looks good to me~
✅ Both round-2 blockers verified closed, mechanically
.md/.csoutsidevendor/). The narrowed semaphore entry now claims exactly what its pin can see — one-at-a-time viaImages_Are_Examined_One_At_A_Time— says plainly that the sweep stopping between images is NOT pinned, and cross-references the:115entry that records why no fixture can attribute it. Pinned and unpinnable no longer live twelve lines apart. The record and reality match.A_Stopping_Container_Does_Not_Start_An_Image_At_All, which exists (DetectionUseCaseTests.cs:341— verified at head), and the claim shrank to what that test honestly covers: pre-flight. The division of labor it now draws — the sweep refuses a dead token before any work, only the real-model case covers the detector noticing cancellation once a pass has begun — is exactly the shape I asked for.And the new entry you added while fixing mine~ I didn't take its word either: "a scan for this repo's test-name shape … found exactly one across the whole tree" — I reproduced that scan independently (declared-method extraction + PascalCase-underscore token match over every text file at both SHAs): at
21d490dexactly one orphan, the ghost itself; ata6a0435, zero. The claim is true, the signal is clean, and the wrinkle you documented is real — my own first two scan drafts tripped over exactly that class of thing. Turning "I shipped a ghost" into a checkable invariant with its cost measured is the right response to this whole saga. That entry is the bug class made obsolete~ ♡Local verification at
a6a0435: build 0 errors / 0 warnings; full backend suite 1478 passed / 0 failed / 10 skipped (136 + 320+5 real-model + 1 + 1021+5 root-permission — arithmetic identical to rounds 1 and 2, as a docs-and-string increment must be; the suite effectively ran twice here, both green). Production and test code are byte-identical to the reviewed21d490dexcept the one attribute string —git diffoversrc//tests//app//deploy/is exactly that one line. Clone left pristine.💡 Little ideas (non-blocking)~
Stream.CopyToAsyncwith a pre-cancelled token throws on entry, so in this test the cancellation surfaces atDetectAsync's first awaited line (the buffer copy), not the inference gate'sThrowIfCancellationRequested. Delete the gate line alone and this test stays green (the copy throws first). The BACKLOG's phrasing ("the detector's own cancellation check has a real-model test") is likewise property-level and true. If anyone ever words it at line granularity, "the detector refuses a dead token at the first thing it awaits" is the exact truth~✅ What I liked~
Fufu~ three rounds, one file, and the record finally tells the truth about itself in every direction I checked. The backlog-eater gets my ♡ at last. Merge it~
Automated review by Jibril · 2026-09-20
CI/CD: absent for head
a6a0435(coverage bot 8681 covers prior4cf206eonly) · Local checks: build 0w/0e, full backend suite 1478/0/10 green (two complete runs), orphan-scan claim reproduced at both SHAs, tree pristine after review