fix(journal): an undo or a redo replays under the write lock — from its first read to its commit #63
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/undo-replay-write-lock"
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?
Summary
EfUndoStorereplays journal rows for every undo and redo: Ctrl+Z, Ctrl+Y, Revert to here, History's Redo, Revert run and Redo run. It read the rows it changes tracked (FindAsync, then the ChangeLog rows it marks), outside any transaction, and saved them in the save's own transaction. So another session's write could land between the replay's read and its save and vanish under the row the replay read. AGENTS.md and ADR 0022/0028 require every document and category write to hold the database's write lock from before its tracked read. #61 left this as a follow-up.TsumugiDbContext.WriteAsync. ItsBEGIN IMMEDIATEcomes before the first read, the save joins that transaction, andWriteAsyncpublishes the replay's notifications once after the commit and leaves nothing tracked on any exit. The replay's ownChangeTracker.Clear()is gone becauseWriteAsync's finally does it.WriteAsyncwould refuse to nest.Settings writes were checked for the same concern and are fine.
TsumugiDbContext's save overrides clear the tracker in afinally, even when the save throws. Nothing inEfAppSettingsStorecan throw between its tracked read and its save, because its guards repeat checks the store has already made. So a failed key save or removal cannot be committed by a later write, and that suspected follow-up is not real.Verification
UndoReplayWriteLockTests, one Theory covering an undo and a redo of one paragraph edit.BEGIN IMMEDIATE(withbusy_timeout = 0) getsSQLITE_BUSY.v1after the undo andv2after the redo, the row's undone flag matches, and nothing is left tracked.WriteAsync: both cases fail, as does #61'sThe_store_refuses_a_redo_whose_edit_finds_what_its_own_delete_took(the tracker clear).EfUndoStore: both new cases fail.WhenAnywith a 10 s timeout.DbContextOptions, so the pool key and interceptors match.WriteAsyncthat does).🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 95.8%
Tsumugi.Domain - 96.3%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 98.3%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.5%
663A4576626E1322FB9C6CE472CAC0A2D2F3DEFF8CB40D9F7__LineBreaks_0
🔮 fufu~ Jibril reviewed your code!
Oh? A PR whose entire production change is removing a
ChangeTracker.Clear()and wrapping one method inWriteAsync— and whose test class intercepts the context's own queries to prove, with a second SQLite connection'sBEGIN IMMEDIATEbouncing offSQLITE_BUSY, that the lock is held at the exact two reads that matter. That's not a test, that's a precision instrument. I got genuinely giddy tracing it~ ♡Verdict: ✅ Looks good to me~
Every load-bearing claim in the body, verified mechanically on a pristine clone at
6491967(basef8345aa= merge-base, clean stack):The fix —
ReplayAsyncnow delegates todb.WriteAsync(() => ReplayLockedAsync(...)). I readTsumugiDbContext.WriteAsyncin full:BeginTransactionAsyncbeforework(), saves join the transaction, notifications deferred to_deferredand published only after commit,ChangeTracker.Clear()in thefinallyon every exit. Which is exactly what licenses deleting the replay's own clear — and the empty-rowsTask.CompletedTaskearly return is behavior-identical to the oldreturn;(no lock needed, nothing read). Sibling pattern match confirmed:EfDocumentStorewraps inWriteAsync10×;EfSegmentStore.WriteChapterAsyncis the same load-under-lock shape; AGENTS.md:77-78 and ADR 0022:77-78 state the rule this now honors for the replay.No nesting —
WriteAsyncisinternalto Infrastructure; the six journal use cases are the onlyIUndoStoreconsumers (DI: oneAddScopedline), none calls it inside anotherWriteAsync. RedoRun's forced discard + redo remains two sequential transactions, exactly as the body says.My three probes, all killed on exactly their named pins (each restored pristine,
git diff --quietat head):WriteAsyncstripped from the replay → 3 red: both new cases + #61'sThe_store_refuses_a_redo_whose_edit_finds_what_its_own_delete_took(the tracker-clear pin survives viaWriteAsync's finally — the removal is safe, proven, not assumed).WhenAnyguard means a faulting replay can never hang the suite.EfUndoStore(basef8345aaversion) → both cases red onAssert.Throws() Failure: No exception was thrown— the probe connection'sBEGIN IMMEDIATEsucceeded, so no lock was held. The bug is real, and this test catches it. Bonus: this also mechanically provesMicrosoft.Data.SqliteissuesBEGIN IMMEDIATEas the doc comment claims — a deferred begin would have let the probe through.Test quality details I checked and liked~ — it reuses the composition's own
DbContextOptions(same connection string → the teardown'sClearPoolOfsweeps this pool too; theSqlitePragmaInterceptorsurvives the copy); the probe connection isPooling=False+busy_timeout = 0so it pollutes nothing and fails fast; gates consume in order (paragraph read, then the ChangeLog read before the save) and onlyReaderExecutingAsyncis intercepted, so saves aren't held; the redo arm's finalIsUndonere-read pins the flag both directions.Suites, digit-exact vs the body: Release build 0w/0e; Integration 580/580, BlazorAdapter 528/528, UseCases 321/321, Domain 486/486; the new class passed 5 consecutive runs here (2 + 3 stability), ~6 s each, zero flake. One
DocumentsStoreFailureTestsbUnit wait-timeout appeared in my first concurrent BlazorAdapter run while Integration hammered the same disk — it passes 19/19 in isolation and 528/528 on the clean re-run; a load artifact of my sandbox, not the PR (the diff touches no adapter surface).💡 Little ideas (non-blocking)~
EditAsync/ReplaceAsync) — tracked read →SaveChangesAsync, with the read outside the save's transaction: the same lost-write shape this PR's comment describes. It predates this diff (untouched here) and AGENTS.md's rule names document/category mutators, so it's no blocker for this PR — but it looks like the next resident of this bug class, worth a named follow-up so it doesn't live here unrecorded. ♡✅ What I liked~
Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (PR minutes old at fetch, 0 comments) · Local checks: all four suites green, digit-exact vs body; 3/3 author probes reproduced killed
Taking the little idea as a named follow-up rather than into this PR. It is real, and it is wider than
MutateAsync.Follow-up: every story, chapter and segment mutator under the write lock. These do a tracked read outside the save's transaction, the same lost-write shape this PR fixes for the replay:
EfSegmentStore:MutateAsync(behindEditAsync/ReplaceAsync),AddRangeAsync,ReorderAsync,MoveToChapterAsync,DeleteRangeAsync. OnlyWriteChapterAsync, and the incomingAppendAsync, take the lock.EfChapterStore: every mutator. None usesWriteAsync, andDeleteChapter's last-chapter check andMergeChapters' two saves are check-then-act across transactions.EfStoryStore.MutateAsync.Why not now: Phase 2's
feat/commit-nodeis editingEfSegmentStore/ISegmentStore(its newAppendAsync), and doing one mutator here would leave the rest. It lands as one arc after commit-node merges, together with extending AGENTS.md's "document or category store mutator runs insideWriteAsync" rule to stories, chapters and segments. Before wrapping, it needs to check that no caller runs these inside anotherWriteAsync(it does not nest).