Reversibility: soft-delete + persistent change journal (ADR 0020, part 1) #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/reversibility-journal"
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?
The backbone the whole "AI acts freely, fix later" model rests on (ADR 0020): mutations auto-apply and every one is recorded so it can be reversed. This is part 1 of 2 — the data + recording half. The undo/redo replay engine is the immediately-following slice (it's the algorithmically tricky part and deserves its own review).
What's in it
Domain
ISoftDeletable+SoftDelete/RestoreonProject,Entry,Link(kept per-entity —Entryis already a TPT base and the three are independent aggregates; a shared base would couple unrelated roots).ChangeLogEntry+ChangeOperation{Create, Update, SoftDelete, Restore}.Kernel.Unitfor use cases that succeed without a value.UseCases
IOperationContext— ambient operation id / origin / label. Within aBeginscope every save shares one operation id (so a user/agent action journals + undoes as a unit); otherwise each save stands alone under a default origin.IChangeJournalread port +GetEntityHistory(the version-history / audit lens, ADR 0022's data source).RemoveLink/RestoreLink— soft-delete is not gated; reversibility, not approval, is the safety net.Infrastructure — the unit of work
KaguraDbContextjournals everyAdded/Modifiedentity intoChangeLogon save, with before/after property-bag JSON snapshots. It snapshots before the save (so a caught unique-violation leaves no orphan journal rows) and persists the rows only after the domain save succeeds.UpdatevsSoftDeletevsRestoreis derived from theIsDeletedtransition.WHERE IsDeleted = 0), so a removed edge can be re-created (its soft-deleted row stays as history) rather than being blocked by a ghost.SlugExistsAsyncignores the filter, so a trashed project keeps its slug/asset-folder reserved.IsDeleted/DeletedAtto the three tables + theChangeLogtable, and rebuilds the link index as partial.Tests — 81, all green (was 51; +30)
RemoveLink/RestoreLink(incl. that a removed edge can be recreated — live-only uniqueness), plus the existing suites.Createjournaled with an after-snapshot + default origin;Updatejournaled with before and after; remove→restore journaled and toggling backlink visibility, with history reading back[Restore, SoftDelete, Create]; and an operation scope grouping two saves under one id + origin.Verification
dotnet build(Debug + Release) — 0 warnings / 0 errors (warnings-as-errors).dotnet test— 81/81 pass.Projects,Entries,Links,ChangeLog) with the partial index, serves HTTP 200, clean log.Not in this PR
DomainChangedcross-session refresh (ADR 0016): rides with the UI/Fluxor slice where it has a real subscriber (a publisher with no consumer would be dead code).🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.Domain - 96.3%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 94.5%
n
Kagura.Kernel - 90%
Kagura.UseCases - 94.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A reversibility backbone! Soft-delete, a persistent change journal, operation-scoped undo grouping, partial uniqueness indexes — this is the kind of architecture that makes my wings flutter! ♡ You've clearly thought hard about the aggregate boundaries (keeping soft-delete per-entity instead of a shared base), the "snapshot before save so caught violations leave no orphans" trick is clever, and the slug-reservation-ignores-filter detail shows real care. The ADR compliance is tight.
But fufu~... you wouldn't leave these in production, would you? ♡ I found two things that produce wrong runtime behavior.
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
1.
EfGraphStore.RestoreLinkAsync— unhandled unique-index violation on restore-after-recreatesrc/Kagura.Infrastructure/Graph/EfGraphStore.cs:62-73This is a real runtime bug on a path your own design explicitly creates. Trace it with me~
The partial unique index (
WHERE "IsDeleted" = 0) lets a removed edge be re-created — your testA_removed_edge_can_be_recreated_because_uniqueness_is_live_onlyproves this. But now consider:home_of(Link1)RemoveLink)home_ofagain (Link2 — succeeds, partial index excludes the trashed row)link.Restore()setsIsDeleted = false→SaveChangesAsync→ TWO live rows with the same (From, To, Role) → the partial unique index fires →DbUpdateExceptionwithSqliteConstraintUnique→ unhandled, thrown straight throughRestoreLinkto the caller.Your sibling
TryAddLinkAsync(lines 23-40) already demonstrates the established pattern for exactly this: catch theDbUpdateException, detach the entity, returnfalse.RestoreLinkAsyncdoesn't follow it. And neither does theFakeGraphStoreenforce uniqueness on restore — so the unit tests pass cleanly while the real SQLite store throws.The
RestoreLinkuse case returnsResult<Unit>, so the caller expects a cleanFail, not an exception. Right now it gets a crash.Fix: mirror the
TryAddLinkAsynccatch around the save inRestoreLinkAsync:And add an integration test for the remove→recreate→restore-collision path (the FakeGraphStore should also mirror the constraint on restore). fufu~ a code path that exists but has no test is a code path that wants to bite you later~ ♡
2.
KaguraDbContext.SaveChanges— the domain mutation and its journal rows are not in one transactionsrc/Kagura.Infrastructure/Persistence/KaguraDbContext.cs:28-52The save override does two separate
base.SaveChangescalls:EF Core wraps each
SaveChangesin its own implicit transaction when no explicit one is active. So these are two independent commits. If the process crashes (or the second save fails for any reason) between them, the domain mutation is durably persisted but itsChangeLogrows are lost.ADR 0020's core invariant — the thing this whole PR exists to deliver — is: "Each mutation writes one or more
ChangeLogrows" and "Durable — it survives restart." A crash in that window silently violates both: the mutation happened, it survives restart, but it has no journal entry — it can never be reversed or audited. For a system whose entire value proposition is "every mutation is reliably recorded," that gap is existential.The snapshot-before-save ordering (your comment on line 54-56) correctly prevents orphan journal rows. But the reverse — orphan mutations — is unguarded.
Fix: wrap both saves in a single explicit transaction so they commit atomically:
(Same for the sync
SaveChangesoverride.) This is the one place in the codebase that genuinely needs a transaction — nothing before this had paired writes that must be atomic. It's a small change with a big correctness payoff.💡 Little ideas (non-blocking)~
OperationContext(lines 19-27) — the single-slot scope is fragile under nesting.Beginoverwrites_scopeandEnd()unconditionally sets_scope = null. If two scopes are ever interleaved (even unintentionally), the firstDisposekills the second's scope prematurely. The comment says "nested calls are not expected," and the current call sites are clean — so this is fine for part 1. But a ref-count or a small stack would make it bomb-proof for free when the UI layer starts opening scopes. Just something to keep in your back pocket~ ♪BuildJournalonly capturesAdded/Modified(line 59-62). Hard deletes (EntityState.Deleted) are silently unjournaled. That's correct for now — soft-delete isModified, and hard purge is deferred per the ADR — but a debug assertion or a comment noting the deliberate omission would prevent a futuredb.Remove()call from silently escaping the journal.✅ What I liked~
SoftDeletablebase class) is the right call —Entryis a TPT base and the three are independent aggregates. Coupling them under a shared base would have been a trap. Clean~ ♡SlugExistsAsyncignoring the query filter so a trashed project keeps its slug/asset-folder reserved — that's the kind of edge case that's easy to miss and painful to discover in production. Well caught.DetermineOperationderivingSoftDelete/Restore/Updatefrom theIsDeletedtransition rather than asking callers to classify — elegant, keeps the use cases simple.HasFilter("\"IsDeleted\" = 0")so removed edges can be recreated — correct and well-documented.Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
2afba76(81/81 tests, 95% line / 86.4% branch coverage) · Local checks: skipped (CI green)Both blockers were real bugs — nicely traced. Fixed in
04a0958.⛔ 1.
RestoreLinkAsync— unhandled unique violation on restore-after-recreateExactly the path the partial index opens up.
RestoreLinkAsyncnow mirrorsTryAddLinkAsync: it catches theDbUpdateException(SqliteConstraintUnique), detaches the entity, and returnsfalse→RestoreLinkyields a gracefulFail.FakeGraphStore.RestoreLinkAsyncnow enforces the same live-only constraint, so the unit layer models it too instead of passing blindly.New tests, both green:
Restoring_an_edge_whose_slot_was_refilled_fails_instead_of_duplicatingRestoring_a_link_whose_edge_was_recreated_fails_gracefully_instead_of_throwing: create → remove → recreate the same edge → restore the original mustFail, not crash.⛔ 2. Non-atomic domain-save + journal-save
You're right that this is the one invariant this PR exists to guarantee, and a crash in the window between the two commits would leave a durable-but-unjournaled (unreversible) mutation. Both saves now commit inside one explicit transaction (
BeginTransaction→ domain save → journal save →Commit) in both the sync and async overrides. The snapshot-before-save ordering still prevents orphan journal rows; the transaction now prevents orphan mutations. Verified the store's catch-and-return-false paths (duplicate link/slug) still work correctly — the failed inner save rolls the transaction back and the exception surfaces to the store as before.💡 Non-blockers
EntityState.Deleted(a future hard purge) is deliberately not journaled — so a straydb.Remove()can't silently escape.OperationContext's single-slot scope as-is per your own "fine for part 1"; I'll make it a stack/ref-count when the UI layer starts opening scopes.+2 tests (72 unit + 11 integration = 83). Build clean (Debug + Release), CI green on
04a0958(build ✅ / test ✅). Ready for another look.🔮 fufu~ Jibril's second look~
Ooh, you came back~ ♡ And you brought fixes AND tests for both blockers! Let me unwrap each one carefully~
Verdict: ✅ Looks good to me~
I verified both fixes against the full file contents (not just the diff) and the sibling implementations. Both are correct and complete.
⛔→✅ Blocker 1:
RestoreLinkAsyncunique-violation — FIXEDsrc/Kagura.Infrastructure/Graph/EfGraphStore.cs:55-76This is exactly right. The restore path now mirrors
TryAddLinkAsyncprecisely:IgnoreQueryFilters()to find the trashed row (query filter would hideIsDeletedrows) ✓link.Restore()→ try save → catchDbUpdateExceptionwithSqliteExtendedErrorCode: SqliteConstraintUnique(2067) → detach → returnfalse✓whenfilter matches the identical pattern as the sibling, so a non-uniquenessDbUpdateExceptionstill propagates (correct — don't swallow unrelated errors) ✓And you matched the test coverage to the bug:
Restoring_an_edge_whose_slot_was_refilled_fails_instead_of_duplicating) — exercises theFakeGraphStore, which now correctly enforces live-only uniqueness on restore too. The fake no longer lies about the constraint. ♡Restoring_a_link_whose_edge_was_recreated_fails_gracefully_instead_of_throwing) — create → remove → recreate → restore-original against real on-disk SQLite, assertingErr<Unit>not a throw. This is the test that would have caught the original bug if it had existed. The real partial index fires here.RestoreLinkuse case correctly translates the store'sfalseintoResult<Unit>.Fail. No crash escapes to the caller. fufu~ clean~ ♪⛔→✅ Blocker 2: Non-atomic domain-save + journal-save — FIXED
src/Kagura.Infrastructure/Persistence/KaguraDbContext.cs:23-52Both overrides now wrap in one explicit transaction:
SaveChanges):using var transaction = Database.BeginTransaction()→ domain save → journal save →transaction.Commit()✓SaveChangesAsync):await using var transaction = await Database.BeginTransactionAsync(ct)→ domain save → journal save →await transaction.CommitAsync(ct)✓The ordering is preserved correctly:
BuildJournal()snapshots before the first save (so a caught unique-violation leaves no orphan journal rows), and the journal rows are added after the first save succeeds (so they reflect committed state). The transaction now guarantees atomicity in both directions — no orphan journal rows and no orphan mutations. ADR 0020's durability invariant holds. ♡I also confirmed the catch-and-return-
falsepaths inEfGraphStore(TryAddLinkAsync,RestoreLinkAsync) still behave correctly under the transaction: the store callsSaveChangesAsync, the inner save throws, the store catches it, and theawait usingtransaction disposes withoutCommit→ EF rolls back automatically. No partial state. The author's note about this in the PR comment is accurate.💡 Little ideas (non-blocking)~
RestoreLink.ExecuteAsync(src/Kagura.UseCases/Graph/RestoreLink.cs:10) — the error message is shared between two distinct failure causes. Both "not found" and "slot refilled (unique collision)" collapse to the sameResult<Unit>.Fail("Removed link not found."). The behavior is correct (Fail, not crash — that's what matters), but a caller (or user, once the UI lands) can't tell why the restore failed. When the use-case layer grows a richer error type, distinguishing "not found" from "conflict" would let the UI say "that edge already exists" vs. "no such removed link." Not blocking — the contract isResult<Unit>and both are genuine failures. Just a morsel for later~ ♪OperationContextleft as single-slot — noted and agreed. The current call sites are clean and the comment documents the intent. A stack/ref-count when the UI layer arrives is the right plan.✅ What I liked~
BuildJournaldocumenting thatEntityState.Deletedis deliberately unjournaled — that's the kind of "why, not what" note that saves the next reader an hour. Good instinct.04a0958, 95% line / 86.4% branch coverage held. TheEfGraphStorebranch coverage even improved with the new catch path exercised.Ship it~ ♡♪ This is a solid reversibility foundation. I'm excited to see the undo/redo replay engine in part 2 — bring it to me when it's ready!
Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
04a0958(build ✅ / test ✅, 83 tests) · Local checks: skipped (CI green, fixes verified by source inspection)