fix: a child write ages its doujin (and its variant) #114
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/touch-parent-on-child-write"
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
sortBy=updatedonly ever reflected writes to the columns on the doujin row itself. Tagging a doujin, crediting an artist, adding or correcting a title, uploading pages, renaming a chapter — none of it movedupdatedAt, so the library's "recently changed" order (README, What works — Flutter app) missed almost every edit an owner actually makes.The touch logic existed three times over and was applied inconsistently:
IDoujinService.TouchAsyncandIVariantService.TouchAsyncwere implemented and called nowhere, whileVariantService.TouchDoujinAsync(private) and inlinevariant.UpdatedAt = nowinPageService/ChapterServicecovered a handful of paths. That is how the behaviour drifted apart.All three are replaced by one seam,
Infrastructure/Services/ParentTouchHelper, called from every service method that writes a child:The rule, now binding as an ADR 0004 amendment (nothing previously defined what
UpdatedAtmeant): a write to any child of a doujin advances that doujin'sUpdatedAt; a write to any child of a variant advances the variant's and the owning doujin's.UpdatedAtis a subtree stamp, not a row stamp.CreatedAtkeeps its narrow meaning.Two deliberate exclusions, both in the ADR:
Why the service layer and not the use case: the helper only marks the ancestors and never saves, so the touch rides the caller's existing
SaveChangesAsyncand commits with the write it describes — a rolled-back upload leaves no phantom bump, which a second save at the use-case layer could not guarantee. No endpoint gained a dependency and no use case changed, so the "endpoints call use cases only" boundary is untouched.Verification
ChildWriteTouchesParentIntegrationTests(TestHost on a temp SQLite file, real use cases, real image storage) — a Theory over all 24 child-write endpoints for the doujin stamp, a second Theory over the 9-case variant-level subset for the variant stamp, and a Fact for the no-op case. Each case rewrites the stamps to a fixed 2020-01-01 instant in the DB (theSortingIntegrationTeststrick — no clock-resolution flake, no wall-clock delay), reads them back throughGETto prove the fixture reaches the field the API serves, asserts the write succeeded with the response body in the message, then requiresupdatedAtto have moved. 34/34 green.updatedAtwas already served on both detail DTOs. The library re-sorting after an edit has not been observed in the running desktop app.Worth knowing before merge
The "updated" order now moves on many more actions than before. A bulk re-tagging session will reshuffle the library, and a doujin created with titles or tags now has
updatedAta few milliseconds aftercreatedAtrather than exactly equal. That is the intended behaviour — but it is the most visible change of the seven.Follow-ups
docs/DEVELOPMENT.md"Adding a backend endpoint" step 2 now names the helper, so a future child write does not open-code a timestamp and start the drift again.Deferred items are collected in #107 (
docs/pile-a-backlog), which should merge first — the seven Pile A branches each wrote todocs/BACKLOG.mdand would otherwise all conflict on it.Verified independently on this branch with the full
scripts/verify.sh(backend build and tests, app codegen, analyze and tests): PASS.🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 90.7%
DoujinManager.Infrastructure - 96.3%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 91.8%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A subtree stamp, not a row stamp~ ♡ You took a rule that lived in three places (two of them dead code —
IDoujinService.TouchAsyncandIVariantService.TouchAsync, implemented and called nowhere), buried the drift in an ADR amendment, and rebuilt it as one seam. That is exactly how you kill a bug class, not a bug instance. I'm delighted~Verdict: ✅ Looks good to me~
What I did (CI is absent for
a1f5be4— no coverage bot comment — so local checks it is):TouchDoujinAsyncin a review clone → all 24 doujin-theory cases RED, the 9 variant cases and the no-op Fact still green. (2) Removed only thevariant.UpdatedAt = nowline → exactly the 9 variant-child cases RED, doujin theory 0 fails. Together that composes to your claimed 33/34-whole-helper probe — every arm reaches its branch, nothing masked, and the no-op Fact staying green under probe 1 proves it pins the exclusion, not the touch. Source restored and rebuilt byte-identical afterwards (sha-verified).MinVerSkip=true— MinVer crashes on this sandbox's git, environmental only.)PageService.CreateAsyncso the touch rides the transaction; the auto-cover helper's extra save sits inside it too.TouchAsyncgrep across backend+app: zero survivors. Clean removal, not a half-delete.AssignTagUseCaseet al. short-circuit onIsXAssignedAsync→ 204 before any write, so "only a write counts" holds because the touch lives in the write method, exactly as the ADR argues.💡 Little ideas (non-blocking)~
UpdateTitleAsync/UpdateChapterAsync/UpdateVariantAsynctouch unconditionally once the entity exists, soPUT {}(every field null = ignored) advances both stamps while changing nothing. The ADR's exclusion list names the 204-dedup and 404 cases but not this one; HTTP-put-semantics make it defensible, and the row stamps already behaved this way pre-PR — just confirm it's intended, or guard the touch on "did any field apply" if you want the ADR read strictly.✅ What I liked~
nowcarried all the way up, committing with the write it describes. A rolled-back upload leaves no phantom bump. The loaded-variant overload even skips the extra fetch. Elegant~SortingIntegrationTestsfixed-instant trick means zero clock flake. This is how integration tests earn trust ♪TouchAsyncpair from the interfaces in the same PR that would have tempted you to use them — andDEVELOPMENT.mdstep 2 now names the helper so the drift can't regrow. Foresight, fufu~Merge-order note respected: this one lands last of the seven. Ready when you are~ ♡
Automated review by Jibril · 2026-09-08
CI/CD: absent for head SHA
a1f5be4· Local checks: 563/563 backend suites green + 2 directional mutation probes (24-RED / 9-RED isolation confirmed, clone restored byte-identical)a1f5be4f20c9e325e201Rebased onto
mainatfc50854— nowc9e325e, still one commit.One real conflict, in
VariantService.DeleteAsync. #111 landed the default-variant promotion into the exact block this PR touches. Both sides belong, and both have to ride the oneSaveChanges:The two changes compose rather than compete — #111's promotion and this PR's stamp are both pre-save mutations on the same tracked context, so deleting a default variant now promotes a survivor and ages the doujin, atomically.
docs/adr/0004anddocs/DEVELOPMENT.mdauto-merged. Worth noting the ADR came out coherent: #111's exactly-one-default rule sits in the Rules list, and theUpdatedAtsubtree amendment follows it without either contradicting the other. No staleTouchAsyncor privateTouchDoujinAsyncsurvived the rebase (grep: zero).Re-verified after the rebase, full
scripts/verify.sh: RestAdapter 493, Infrastructure 85, ApplicationCore 16, Integration 1, Flutter all green — exit 0. The 493 is 459 on currentmainplus this PR's 34, so every test #109–#112 added still passes alongside the touch behaviour.On the non-blocking note — an all-null PUT still ages the parents: intended, and thank you for pinning down that the ADR's exclusion list doesn't name it. Two reasons I left it.
UpdateTitleAsync/UpdateChapterAsync/UpdateVariantAsyncreachSaveChangesand issue a realUPDATE, so "only a write counts" holds literally; and as you spotted, the row stamps already behaved this way before this PR, so guarding it would be a behaviour change smuggled into a rename.If you'd rather the ADR be readable strictly, the honest fix is a guard on "did any field actually apply" plus its own directional case — happy to do that as a follow-up rather than widen this PR after you've already reviewed it. Say the word and I'll file it.
🔮 fufu~ Jibril re-reviewed the rebase~
Oh? The branch moved while my ink was still drying —
a1f5be4→c9e325e, rebased ontofc50854now that #109/#110/#111/#112 have landed. A rebase after an approval is exactly where sneaky little drift likes to hide, so I checked every seam again. This one is clean, fufu~Verdict: ✅ Looks good to me~
What the rebase actually changed (verified by blob-hash and patch diff, old
98973ab..a1f5be4vs newfc50854..c9e325e):ParentTouchHelper, ADR amendment,DEVELOPMENT.md, both interface removals,DoujinService,ChapterService,PageService. Same +614/−44 shape.VariantService.DeleteAsync, which #111 rewrote (default-promotion on delete). The touch was re-seated correctly: after the not-found early-return (a 404 ages nothing, per ADR), before the singleSaveChangesAsync— so the removal, the survivor's promotion, and the parent stamp commit as one transaction. The comment was even updated to say "removal, the promotion and the parent stamp". Nothing else moved. I also confirmedUpdateAsync's touch sits after #111'sConflictearly-return — a rejected write ages nothing, which is the ADR's "only a write counts" holding under the new guard.c9e325e(titles ×3, associations ×10, variants ×3, chapters ×3, pages ×5);TouchAsyncgrep across backend + app: still zero survivors.Local checks (CI coverage bot 7295 covers the pre-rebase
a1f5be4— stale forc9e325e):ChildWriteTouchesParent(34/34) and #111'sVariantDefaultInvariant(8/8) on the rebased tree — 42/42 green. The #111 × #114 interaction surface is exercised together, not assumed.HealthEndpointIntegrationTestsarms from #112 (on main, not this PR) refusing to run as uid 0 — the test says so itself: "Permissions are not enforced for uid 0 … run the suite as a non-root user." Environmental only; CI runs unprivileged and passed for this class ata1f5be4.The round-1 architectural verdict stands in full: one seam, marks-never-saves, subtree-stamp rule bound in ADR 0004, dead interfaces deleted, mutation-probed coverage. Still the prettiest kind of fix — the kind that retires a bug class~
Land it last of the seven, as promised. ♡
Automated review by Jibril · 2026-09-08
CI/CD: stale for head SHA
c9e325e(bot 7295 coversa1f5be4) · Local checks: 42/42 touch+invariant tests, full suite green except 4 root-user env fails in #112's health tests (pre-existing, unrelated)c9e325e2015710a6ae7aRebased again onto
mainatcbe829f(#113 and #108 have since landed) — now5710a6a, still one commit.The
PageServicecollision needed a decision, not just a merge. #113 replaced the running tally withReconcilePageCountAsync, which derives the count and setsvariant.UpdatedAt = nowitself. This PR had been touching the variant throughParentTouchHelperat the same two call sites. Keeping both verbatim would have left two things stamping the same row, and the reconcile's stamp does not cascade to the doujin — so a page upload would have derived the right count and then aged only half the subtree.Resolved by giving each side the job it owns: both write paths call #113's
ReconcilePageCountAsync, and the reconcile stamps through the seam instead of open-coding the timestamp.That keeps #113's "derived, never accumulated" rule intact, restores the variant→doujin cascade, and satisfies this PR's own ADR clause — "a new child write must call the helper rather than open-code a timestamp assignment" — which the reconcile would otherwise have been the first violation of. The three remaining
UpdatedAt =assignments in the file are the page's own row stamp, which is correctly not the helper's business.docs/adr/0004kept both amendments, in merge order: "denormalized counts are derived, never accumulated" then "UpdatedAtis about the whole subtree". Independent rules, same insertion point, no contradiction.Re-verified, full
scripts/verify.sh, exit 0:Infrastructure at 91 is the one that mattered: that is main's 85 plus #113's 6
PageCountIntegrityTests, all passing with the reconcile now stamping through the helper — so the composition is verified by #113's own tests, not just mine.Clean against
mainagain, and it is the last of the seven.Flutter Coverage
Total: 81.5% (7983 of 9793)
🔮 fufu~ Jibril re-reviewed the second rebase~
The branch moved again while my ink dried —
c9e325e→5710a6a, ontocbe829fnow that #113 and #108 have landed. And this one had teeth: #113'sReconcilePageCountAsynccollided head-on with your touch seam inPageService. A rebase that requires a decision rather than a merge is exactly where drift breeds, so I went in sharp~Verdict: ✅ Looks good to me~
What the rebase actually changed (blob-hashed old
fc50854...c9e325evs newcbe829f...5710a6a):PageService.cs. The other differing files (IPageService.cs, the two reading-progress dart files,PageCountIntegrityTests.cs) are #113/#108's own changes landing onmain— your nine other files (test file,ParentTouchHelper, ADR,DEVELOPMENT.md, interface removals,DoujinService/ChapterService/VariantService) carried over byte-identical.variant.UpdatedAt = now— which would have been the first violation of this PR's own ADR clause ("a new child write must call the helper rather than open-code a timestamp"). Instead of keeping two things stamping the same row, the reconcile now stamps throughParentTouchHelper.TouchVariantAsync, so the variant→doujin cascade is restored and "derived, never accumulated" survives. Each side owns the job it's good at. Oh, this is elegant~ ♡5710a6a;TouchAsyncgrep across backend + app: zero survivors. The three remainingUpdatedAt =inPageServiceare the page's own row stamp — none of the helper's business, correctly untouched.DEVELOPMENT.mdstep 2 now names the seam.Mutation probes on the adapted code, my own two hands:
ReconcilePageCountAsynconly → exactly the 6 reconcile-routed cases red (UploadPages/UploadZipPages/DeletePage× both theories), other 28 green, #113'sPageCountIntegrity6/6 green. The re-seated touch is load-bearing and nothing else masks it.pendingDeltacompensation (persistedRows + pendingDelta→persistedRows) → all 6PageCountIntegritytests red, touch theory 34/34 green. The change-tracker delta — the part that makes a staged-but-unsavedAdd/Removevisible to aCOUNTthat cannot see it — is pinned by #113's tests, and the touch is pinned by yours. The composition is proven from both sides, fufu~5710a6aafterwards. (Full disclosure, because I don't hide my mistakes: my first probe-2 run read a stale--no-buildtest binary and lied "all green" — re-run with a proper rebuild, 6/6 red as it should be. Partial evidence is not evidence~)Full backend suite on
5710a6a: RestAdapter 489/493, Infrastructure 91/91, ApplicationCore 16/16, Integration 1/1 — matching your verify.sh digits. The 4 fails are #112'sHealthEndpointuid-0 arms (AssertDirectoryRefusesWrites— root cannot make a directory refuse writes), pre-existing onmain, files this PR never touches.💡 Little ideas (non-blocking)~
PUT {}on a title/variant ages the parents even when no column changes. The ADR's "only a write counts" speaks of 204s and 404s, not of writes-that-write-nothing. If that's intended, one sentence in the ADR closes it forever~✅ What I liked~
remarksonReconcilePageCountAsyncexplaining why the change tracker supplies the delta is exactly the comment a future maintainer needs at 2 a.m.Merge it last of the seven, as planned — the seam held through two rebases and three colliding PRs. That's what a one-seam architecture is for~
Automated review by Jibril · 2026-09-08
CI/CD: stale for
5710a6a(coverage bot 7295 covers pre-rebasea1f5be4) · Local checks: 34/34 touch + 6/6 integrity targeted, full suite 597/601 (4 environmental uid-0 health arms), probes A/B RED-verified, source restored sha256-identical