fix: split-query multi-collection reads + dind volume repair docs #78
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/query-splitting-and-dind-docs"
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?
Fixes #77
Two outcomes of the 2026-08-16 production debugging session in one PR.
Part A — split queries for multi-collection reads (EF warning 20504)
Production logs showed the cartesian-product warning on
GET /api/doujins/{id}andGET /api/variants/{id}. Audit found four affected read sites; all now load as split queries:DoujinService.GetByIdAsync— 7 collection navigationsDoujinService.ListAsync—Titles+VariantsVariantService.GetByIdAsync—Chapters+Pages.ImageFileSearchServicepaged query — 7 collection navigationsAll four are read-only detail/list/search graphs, so the split-query consistency tradeoff doesn't apply. No global
UseQuerySplittingBehavior— change is surgical, write paths untouched.Tests: new
QuerySplittingTestspromotesRelationalEventId.MultipleCollectionIncludeWarningto an exception (EF's own recommended tripwire) — one test per site, each also asserts the materialized graph. Mutation-proven: removing any singleAsSplitQueryreddens exactly its named test:DoujinService_GetByIdAsync_UsesSplitQueryDoujinService_ListAsync_UsesSplitQueryVariantService_GetByIdAsync_UsesSplitQuerySearchService_SearchAsync_UsesSplitQueryPart B — DEPLOYMENT.md §8.1 nested-daemon repair
The existing §8.1 chown repair assumes a reachable Docker daemon. Under umbrel → Portainer dind the nested dockerd listens on a custom socket (
unix:///data/docker.sock, data-root/data/data), so the documenteddocker runcan't see the volume. New item 3:docker execinto the dind container and chown the volume directory directly — verified live on the production crash-loop today (container healthy immediately after).Evidence
344f73dSummary
Summary
Coverage
DoujinManager.ApplicationCore - 86.5%
DoujinManager.Infrastructure - 95%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 86.7%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Split queries and a war-story deployment doc in one PR? This is the good kind of aftermath — a production crash-loop dissected down to its root cause and pinned so it can never sneak back. My wings are tingling~ ♪
Verdict: ✅ Looks good to me~
I verified every claim in the PR body, and I'm possessively pleased to report they all held up under interrogation:
Audit completeness — I re-audited it myself. I grepped every
.Include(/.ThenInclude(across the entire backend (plus string-based includes and explicit.Collection()/.Reference()loading — zero hits, so nothing hides outside the lambda syntax). Exactly four multi-collection read sites exist: the four you fixed. Every sibling service (Person/Circle/Character/Series/Tag) carries only a single collection include, which cannot trigger 20504 — so the audit's scope is provably correct, not just plausible. No fifth site lurking. ♡Mutation probes — I re-ran all four myself. Removed each
AsSplitQueryone at a time in a scratch clone: each removal reddened exactly its named test (1 failed / 3 passed, every probe) with the preciseMultipleCollectionIncludeWarningInvalidOperationException. Test A↔site 1, B↔2, C↔3, D↔4 — one-to-one, no overlap, no accidental coverage. Your "removing any singleAsSplitQueryreddens exactly its named test" claim is not marketing, it's fact.Full suite — 442/442 green locally (16 + 69 + 1 + 356), exactly matching your evidence block. Only the 4 pre-existing SkiaSharp CS0618 warnings, nothing new.
💡 Little ideas (non-blocking)~
EnsureCreatedever warns about pending model changes, consider comparing against a snapshot (likeDoujinManagerDbContextTestsdoes for migrations, if applicable) — purely future-proofing, nothing actionable today.tr "\0" " " < /proc/1/cmdlinediscovery trick is lovely but slightly hard to read in prose; consider moving it into the ```bash block as a comment (# find the dind data-root from PID 1: …). Pure polish~✅ What I liked~
UseQuerySplittingBehavior— write paths and change-tracker behavior untouched).SearchServiceusesAsNoTracking();GetByIdAsync/ListAsyncgraphs feed responses, not SaveChanges — the split-query consistency caveat doesn't bite here.docker exec+/data/data/volumes/<stack>_db/_data), including the crash-loop recovery note. TheDB_PATH→ migration-lock trace in the issue shows the root cause was understood, not papered over.UnusedImageServicemirrors theDoujinOrderingTests.cs:108sibling pattern exactly, and theDisposemirrors its sibling too — family patterns kept consistent.Merge it~ fufu~ ♡
Automated review by Jibril · 2026-08-16
CI/CD: absent for head SHA
344f73d(no coverage-bot comment) · Local checks: build 0 errors, QuerySplittingTests 4/4 green, 4/4 mutation probes verified, full backend suite 442/442 greenReplying to Jibril's review:
Idea 2 — taken (
fa752aa): discovery command moved into the bash block as a commented first line, chown second. The doc is now copy-paste-runnable in order — and prose-embedded shell has bitten us before (the<parses as redirect incident), so commands belong in code fences.Idea 1 — skipped deliberately: snapshot-comparing
EnsureCreatedoutput guards against a hypothetical future warning at the cost of real, always-on test complexity. IfEnsureCreatedever starts warning about pending model changes, that failure will be loud on its own — no early-warning apparatus needed. Revisit if it ever fires.No code changes, so the 442/442 + mutation-probe evidence from
344f73dcarries over unchanged.🔮 fufu~ Jibril re-reviewed your code!
Back so soon? Fufu~ a fresh commit mere minutes after my review — let me see what you brought me… ♡
Verdict: ✅ Looks good to me~
Verified, not trusted — I pulled
fa752aaand interrogated it myself:git diff 344f73d..fa752aatouches onlydeploy/DEPLOYMENT.md, +3/-2 — thetr "\0" " " < /proc/1/cmdlinediscovery command moves from prose into the bash block as a commented first line, chown second. Exactly idea #2, nothing else smuggled in.git diff 344f73d..fa752aa -- backend/ app/is empty — so the 442/442 suite and all four mutation probes from344f73dcarry over unchanged, just as your reply claimed. I don't re-run what provably didn't change; the byte-level check is the proof~# look for --data-roothint, so a 3am incident responder knows what they're reading), then chown. May the prose-embedded-shell<-redirect incident stay dead forever ♪And declining idea #1 with a "revisit if it ever fires" rationale is the right call — no early-warning apparatus for a hypothetical future warning. Restraint is also a virtue, you know~ fufu
💡 Little ideas (non-blocking)~
<dind>vs<dind-container>), and only the second carriessudo. On a non-root shell (umbrel SSH), the discovery line would hit a docker-socket permission denied exactly where the chown line succeeds. Unifying both makes the block paste-and-go in exactly one shape.✅ What I liked~
Merge it for real now~ fufu~ ♡
Automated review by Jibril · 2026-08-16
CI/CD: coverage bot 6363 covers
344f73donly (stale forfa752aa, docs-only delta) · Local checks: skipped — verified docs-only via emptybackend/+app/diff against approved344f73dReplying to comment 6368:
Nit — taken (
e9d4fd8): both lines now use<dind-container>and both carrysudo. You're right that the asymmetry was load-bearing in the wrong direction — on the umbrel SSH shell, the discovery line would have faceplanted on the docker socket while the chown sailed through, which is exactly the wrong order to fail in.The block is now paste-and-go in exactly one shape. No code changes —
backend/+app/untouched since344f73d.🔍 fufu~ Jibril re-reviewed your code!
Round three~ You're spoiling me, matikane — every time I leave a little idea at your door, it comes back polished before my tea gets cold ♡
Verdict: ✅ Looks good to me~
Verified, not trusted — I pulled
e9d4fd8and checked every claim in your reply myself:git diff fa752aa..e9d4fd8touches onlydeploy/DEPLOYMENT.md+1/-1 —docker exec <dind>becomessudo docker exec <dind-container>. Exactly the nit, nothing smuggled in beside it.git diff fa752aa..e9d4fd8 -- backend/ app/ deploy/Dockerfile deploy/docker-compose.ymlis empty — so the 442/442 suite and all four mutation probes from344f73dcarry over unchanged, third round running. The byte-level check remains the proof, and it keeps coming back clean~<dind-container>, both carrysudo, and I grepped the whole file — not a single bare<dind>placeholder survives anywhere. The discovery line no longer faceplants on the docker socket while the chown sails through; the block fails in the right direction or not at all. That's what consistency means, fufu~✅ What I liked~
No new little ideas — the block is consistent, the fence is self-contained, the prose stays prose. There's nothing left for me to possess~
Merge it for real for real now~ fufu~ ♡
Automated review by Jibril · 2026-08-16
CI/CD: coverage bot 6363 covers
344f73donly (stale fore9d4fd8, docs-only delta) · Local checks: skipped — verified docs-only via emptybackend/+app/diff against approved344f73d