fix: derive Variant.PageCount from page rows instead of accumulating #113
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/page-count-integrity"
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
Variant.PageCountwas a running tally:+= 1inPageService.CreateAsync,Math.Max(0, PageCount - 1)inDeleteAsync. Neither read the page rows, so a count that had drifted once — a concurrent writer through a secondDbContext, a hand-edited or partially restored database — stayed drifted for ever, and the clamp hid the low-drift case by reporting0instead of the negative number that would have been the only visible symptom.PageServicederives the count on both write paths. One privateReconcilePageCountAsyncsetsPageCountto the number of rows the variant will hold once the pending change set is saved: the persistedCOUNTfor that variant, folded with the change tracker's staged inserts and deletes for it. The stored value is never read, so the next create or delete repairs an already-drifted count instead of carrying it forward. TheMath.Maxclamp is gone with the arithmetic that needed it.SaveChanges. Doing it as a second round trip after saving would have opened a transaction that a crash could land between (page creates and deletes outside the upload use cases have no ambient transaction), trading one integrity bug for another.IPageService's contract docs stop promising increment/decrement semantics.No endpoint, DTO, hypermedia, config or migration surface changes; no new tunable (this is a correctness invariant, not a deployment knob).
Verification
backend/tests/DoujinManager.Infrastructure.Tests/PageCountIntegrityTests.cs, six cases over a real SQLiteDbContext: an ordinary add/delete editing session across two chapters; delete-with-drifted-counter (stored0and7against three real rows); create-with-drifted-counter (same two directions); and a per-variant scoping case that holds another variant's saved and staged-unsaved rows while creating a page. Every assertion first asserts the actual row count, so a fixture that never reached the branch fails loudly, and reads the stored count withAsNoTracking()so it cannot pass on the change-tracked instance the service just mutated.Added => 0(3 red),Deleted => 0(3 red),_ => 1(5 red), tracker variant filter dropped (1 red),COUNTvariant filter dropped (1 red), and the original accumulate-plus-clamp restored (4 red).DoujinManager.RestAdapter.Tests, 427 passed, which covers the existingpageCountassertions inImageEndpointsIntegrationTests(upload of 2, of 1, and the rolled-back upload that must leave0) and the ADR 0021 reflection test. FullInfrastructure.Tests91,ApplicationCore.Tests16,IntegrationTests1 — 535 passed, 0 failed.pageCountoff the variant DTO and is unaffected in shape. The cross-DbContextconcurrent-writer race is not closed by this change — see Follow-ups.Follow-ups
IHostedServicetier in this codebase and it should not grow one for this. Worth checking the live database, since existing drift is not repaired by merging this.DbContexts can still each compute before either commits. Closing it needs eitherUPDATE ... SET page_count = (SELECT COUNT(*) ...)or theBEGIN IMMEDIATEpatternImageServicealready uses for dedup, plus transaction management insidePageServiceand a temp-file-database contention test (:memory:is private per connection). Left out as its own piece of work; the window is now self-correcting on the next edit rather than permanent.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. This branch and #111 both amend ADR 0004, as doesfix/touch-parent-on-child-write, which should merge last of the seven.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
PageService kept PageCount as a running tally: '+= 1' on create and a clamped '- 1' on delete. Neither read the rows, so a count that had drifted once stayed drifted for ever, and the Math.Max(0, ...) clamp on the delete path hid the low-drift case by reporting 0 rather than a negative number - the one symptom that would have exposed it. Both write paths now call a single ReconcilePageCountAsync helper that sets the count to the number of page rows the variant will hold once the pending change set is saved: the persisted COUNT for that variant, folded with the change tracker's staged inserts and deletes for it. The stored value is never read, so an already-drifted count is repaired by the next create or delete, and the reconcile stays inside the SaveChanges the caller already makes - no extra round trip for a crash to land between. ADR 0004 gains the amendment this establishes ('a denormalized count is derived from the rows it counts, never accumulated'), and IPageService's contract docs stop promising increment/decrement semantics. A one-shot repair sweep for databases that already drifted is deferred to docs/BACKLOG.md - this codebase has no background tier and should not grow one for this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 89.2%
DoujinManager.Infrastructure - 95.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 89%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A denormalized counter that heals itself instead of rotting silently — you went and turned an accumulate-and-pray into derive-and-know~ ♡ I read the whole diff twice, then went looking for the trap. There wasn't one. How delicious.
Verdict: ✅ Looks good to me~
No blocking issues. And I tried, fufu~ — here is what I hunted and what I found:
1. The ADR's bold claim — "no other code path adds or removes page rows." I grepped the entire backend:
db.Pages.Add/Removeexist only inPageService; noExecuteDelete/ExecuteUpdateanywhere;VariantServicewritesPageCount = 0exactly once at variant creation and itsUpdateAsyncnever touches the field. The invariant is airtight, and the cascade delete claim matchesVariantConfiguration'sDeleteBehavior.Cascade. Claim survives~2. The change-tracker fold.
persisted COUNT+ stagedAdded/Deleteddelta, computed before the caller's singleSaveChanges— no second round trip, no crash window, and inside the ambient transactions the upload use cases open (ImageUseCases.cs:49/111). Theremarksdoc onReconcilePageCountAsyncexplaining why the COUNT can't see staged rows is exactly the kind of comment that saves the next reader an hour. ♪3. Mutation probes — independently re-run. You claimed six; I re-ran three from scratch (with proper test-project rebuilds, since my first attempt was tainted by a stale binary — your tests caught my sloppiness too, fufu~): tracker variant-filter dropped → exactly 1 red (the scoping test); COUNT variant-filter dropped → exactly 1 red (scoping again — correct, the drift tests hold only one variant with rows); delete path reverted to accumulate+clamp → exactly 2 red (both delete-drift arms, storedCount 0 and 7). These tests have teeth~
4. The scoping test is the quiet hero. It holds another variant's saved rows and a staged-unsaved fourth row while reconciling — the exact shape a shared request-scoped
DbContextcan produce mid-upload. Both leak paths pinned by one test.5.
AsNoTracking()assertions. Reading the stored count back un-tracked means the test can't pass off the in-memory instance the service just mutated. Most test authors forget this. You didn't. ♡✅ What I liked~
Math.Max(0, …)was hiding the only visible symptom of low drift. That's the difference between deleting a line and understanding a bug.BACKLOG.md(the cross-repo reference is real, not a promise).IPageServiceupdated to stop promising increment semantics — the interface no longer lies.Fufu~ derive, don't accumulate. The Flugel approves~ ♪
Automated review by Jibril · 2026-09-08
CI/CD: absent for head SHA
0b9c487· Local checks: build 0 errors (19 pre-existing warnings, none in changed files) · Infrastructure 91/91 · ApplicationCore 16/16 · RestAdapter 427/427 · Integration 1/1 — 535 passed, 0 failed, matches PR body exactly