feat: ship the detector model inside the image #193
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/model-in-image"
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
ADR 0028 decided in September that the weights ship inside the image. It has
been open in
docs/BACKLOG.mdsince #185 for one reason: the design needed afile at a URL, and nobody had put one there. The export is now published to the
Forgejo generic registry the images already push to, and the build fetches it.
scripts/fetch-detector-model.shdoes the fetch, the checksum and an atomicinstall. The Dockerfile calls it for
/app/models/regions.onnx; the localrecipe in
docs/DEVELOPMENT.mdcalls it with no argument. One implementation,because two would have drifted — and the local recipe no longer needs Python at
all, only a re-export does.
DOUJIN_MANAGER_DETECTOR_MODELalready defaulted to that path. It now pointsat something real.
Three things only building it could have found
mktempcreates 0600 andmvkeepsit. The script runs as root in the image; the server runs as
$APP_UID. Andbecause the file exists, startup registers the detector, tries to load it,
and the container dies on every boot — a failure that looks like a bad
image rather than a bad file mode.
chmod 0644, pinned byThe_Installed_File_Is_Readable_By_Whoever_Runs_The_Server..dockerignoreexcludesscripts/, soCOPYfailed on a file plainlypresent in the tree. Un-ignored the one script the build needs, with the
exception written next to the exclusion.
credentials, so a registry that demanded them would have sunk the design
rather than inconvenienced it. Verified before a line of the Dockerfile was
written:
GET 200, 41825853 bytes, no auth.Verification
scripts/verify.shgreen (== verify OK (all) ==) — backend 1,508 passed/ 5 skipped, app 1,530, analyze clean.
The part that matters is not the suite, though. The image was built and started:
Three classes, and the detector version derived from the model's own hash —
which is what ADR 0027's re-sweep gate reads, so an image carrying a new model
re-sweeps the library by itself.
FetchDetectorModelScriptTestscovers the script over afile://URL: thehappy path, the refusal on a checksum mismatch with both hashes named and
nothing installed, the skip when the destination is already right, the file
mode, and one override given without the other. Nothing in it touches the
network or a real model — 5 tests, 61 ms.
What did not change
CI still runs without a model. The weights are for the image, not the test
pipeline;
RealModelFactAttributekeeps the five real-model cases opt-in behindDOUJIN_MANAGER_DETECTOR_MODEL, exactly as before.Committing the weights, reconsidered
It came up and was rejected again, with a number: this repository's entire
packed history is 4.95 MiB across 564 commits; the model is 39.9 MiB of
already-compressed weights. Committing it makes the repo roughly nine times
larger, permanently, and
docker-publish.ymlchecks out withfetch-depth: 0on every push to main. Git LFS is worse here — not installed on the owner's
machine, the instance's LFS endpoint answers 404, and a missed install leaves a
130-byte pointer file that the build would copy into the image as "the model".
Recorded in ADR 0028 so the next person doesn't re-derive it.
Also
ADR 0027 and ADR 0028 move from Proposed to Accepted — 0027 because phase 1
is built end to end (#183, #185, #188, #191, #192), 0028 because this is its
implementation. 0028 gains a "what actually landed" section with the package
coordinates, the 0644 story, and the note that its promised
build-fails-on-a-wrong-checksum test is asserted against the script rather than
a full image build per test run.
The BACKLOG item open since #185 is closed.
Timing
The image builds on every push to main, not just tags — and the package is
already uploaded, so this can merge whenever. Nothing has to happen first.
🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.9%
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 PR whose entire reason to exist is that a checksum refuses to lie~ ♡ An atomic
mktemp→verify→mvinstall, achmod 0644somebody actually traced through uid 1654, one script with two callers so the implementations cannot drift, and tests that poke a real refusal instead of a green smoke puff. I pulled your 41,825,853 bytes down myself and hashed them:f3bf7701…aa1d, anonymous 200, checksum OK — every number in the PR body is a true number. This is the good stuff. But you knew I'd find the one seam~ ♪Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
FetchDetectorModelScriptTestsonly ever exercises the first. I mutated the script in a review clone (deleted the|| [[ -z "${DETECTOR_MODEL_URL:-}" && -n "${DETECTOR_MODEL_SHA256:-}" ]]disjunct, leaving everything else intact) and ran the suite: 5/5 green. The tests cannot detect the loss of half the guard. Your own house rule — written intoBashFactAttribute's doc comment andLinuxFactAttributebefore it — is that a branch without a pin isn't covered; ADR 0028's verification section says a check that has never been seen to fail might be spelled wrong. This half has literally never been seen to fail~ fufu~ you wouldn't leave an unpinned branch in production, would you? ♡Fix: mirror the existing case —
A_Checksum_Without_A_Url_Is_Refused_Before_Anything_Is_Downloaded: run the script withDETECTOR_MODEL_URLempty andDETECTOR_MODEL_SHA256set to the source's real hash, assert exit 2, nothing installed, and"together"in stderr. ~15 lines, and the probe that just went silently green goes red.💡 Little ideas (non-blocking)~
[[ "$actual" != "$SHA256" ]]is case-sensitive, so a custom pin written in uppercase hex (DETECTOR_MODEL_SHA256=AB…) is refused as a mismatch even when the bytes are right. It fails safe and loud — both hashes are printed side by side — so I'm only pouting, not blocking~ Atr 'A-F' 'a-f'on the override would make it forgiving.✅ What I liked~
mktempbeside the destination, verify,mv,chmod, clear the trap — an interrupted or corrupt download can never leave a corpse at the path the server loads from. ♪aspnet:10.0container, watched the container die on every boot, and pinned the mode with a test asserting readable-by-all, writable-by-none.The_Installed_File_Is_Readable_By_Whoever_Runs_The_Serveris load-bearing and now it cannot regress.docs/DEVELOPMENT.mdrecipe call the same script, and the local recipe needs no Python at all anymore. DRY exactly where drift would have bitten.file://— nothing touches the network or a real model, the refusal case asserts both hashes named and nothing installed, the skip case proves non-fetching by pointing the URL at nothing, and the empty-ARG-as-unset semantics the container build produces are documented in the test itself. 5/5 green here in 110 ms, and my probes agree they're directional: chmod deleted → exactly the mode test red; checksum comparisonif false→ exactly the refusal test red; skip block deleted → exactly the skip test red; whole guard deleted → exactly the guard test red.DEPLOYMENT.mdand the compose comments agree with the code; the ADR status transitions cite the PRs that earned them.Merge the mirrored test in and this is an enthusiastic approve — the design itself I have zero quarrel with~ ♡
Automated review by Jibril · 2026-09-21
CI/CD: ci.yml build+test green for
7cc173a(run 880); deploy-smoke red on head — but chronically red on main and every prior PR at the same ~180 s wall long before this PR, pre-existing, not a regression here · Local checks: 5/5 FetchDetectorModelScriptTests green (110 ms); full RestAdapter suite 1046 passed / 5 skipped (pre-existing real-model skips); model downloaded and sha256-verified against the pin; 5 mutation probes (4 red-on-exactly-target, 1 proving the blocker above); review clone restored pristine at7cc173aFixed at
a37ebab.The blocker: half a guard
You're right, and the way you proved it is the part worth keeping. I wrote a
guard that names two mistakes and a test that exercises one, then described it
in the PR body as though both were covered. Deleting the second disjunct left
5/5 green — that is not a covered branch, it is a comment that happens to
compile.
A_Checksum_Without_A_Url_Is_Refused_Before_Anything_Is_Downloadedmirrors theexisting case. I re-ran your exact mutation — the
|| [[ -z "${DETECTOR_MODEL_URL:-}" && -n "${DETECTOR_MODEL_SHA256:-}" ]]disjunctremoved, nothing else touched — and it now goes red.
Five probes on the script, each red on exactly its own test:
A_Checksum_Without_A_Url_…A_Url_Without_A_Checksum_…A_Pin_Written_In_Upper_Case_Is_The_Same_Pinif falseA_File_That_Does_Not_Match_Is_Refused_…chmod 0644removedThe_Installed_File_Is_Readable_By_Whoever_Runs_The_ServerThe non-blocking idea, taken
Case-sensitivity: taken, because the failure mode you describe is worse than a
pout. An upper-case pin refused as a mismatch prints both hashes side by side
spelling the same value twice, which is a maddening thing to meet at the
bottom of a build log. The override is folded with
tr 'A-F' 'a-f'beforecomparison — unconditional, so it adds no branch — and pinned by the test above.
On the red checks
Agreed that
deploy-smokeis not this PR's doing. While looking, I noticedsomething you didn't flag and I'd rather say than sit on:
flutter-cirun 879failed on a
pushtomain— that workflow only runs on main and on PRstouching
app/**, so that is main itself, after #192.I could not reproduce it. Running exactly what the workflow runs —
COVERAGE=1 bash scripts/verify.sh app, which is--coverage --concurrency=1and not what I had been running — passes here: exit 0, 1,530 tests, 2m21s,
well inside the job's 12-minute attempt budget. The workflow's own comment
documents a residual hang in coverage collection that the retry exists to
absorb, so that is my best guess, but a guess is all it is: the Actions jobs
API 404s for this token, so I cannot read the run's log to confirm. Worth a
look at the log when convenient — a chronically red job on main is one nobody
reads by the time it matters.
Verification
scripts/verify.sh backendgreen — 1,510 passed / 5 skipped.COVERAGE=1 scripts/verify.sh appgreen — 1,530 tests, analyze clean.FetchDetectorModelScriptTestsis now 7 cases, 138 ms, still touching neitherthe network nor a real model.
🔮 fufu~ Jibril reviewed your code!
Round 2~ And you closed the blocker the best way: by re-running my own mutation against your own guard before claiming it dead.
a37ebabis a surgical +40/−1 across exactly 2 files — the mirror test, the case-fold, and nothing else. No scope creep, no drive-by edits. That is how you answer a yandere, fufu~ ♡Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — CLOSED, and I re-proved it myself
a37ebab:|| [[ -z "$DETECTOR_MODEL_URL…" && -n "$DETECTOR_MODEL_SHA256…" ]]) deleted → red on exactlyA_Checksum_Without_A_Url_Is_Refused_Before_Anything_Is_Downloaded, 6/7 others green. My round-1 killer no longer survives~ ♪bash -nverified) → red on exactlyA_Url_Without_A_Checksum_…. The pair is symmetric now.tr 'A-F' 'a-f'fold removed → red on exactlyA_Pin_Written_In_Upper_Case_Is_The_Same_Pin. Unconditional, so the fold adds no branch — the guard's branch count is unchanged. Elegant.The new test's comment ("a checksum pinned against the built-in URL fetches the built-in model and verifies it against somebody else's hash") names why the arm matters, not just that it exists — that's the kind of comment that keeps the next reader from deleting it. Script restored pristine afterwards, md5 verified.
✅ What I liked~
url: string.Emptywith the env set-but-empty is exactly the shape the Dockerfile's emptyARGproduces — andRunScript's comment already documented why that's the honest form. The test inherits the container's real failure shape, not a convenient one.app/file and the workflow only fires on main and app-touching PRs. A chronically red main job nobody reads is worth an owner's eyeball on the log, but it neither blocks nor belongs to this PR.The guard that names two mistakes now has two witnesses, one per mistake. Fufu~ that's the whole job done properly.
Automated review by Jibril · 2026-09-21 (CEST)
CI/CD: coverage bot 8838 covers
7cc173aonly (stale fora37ebab) · Local checks: FetchDetectorModelScriptTests 7/7, full RestAdapter 1048 passed / 5 skipped, 3 mutation probes red-on-own-testReady to merge~ ♡