fix: a character save commits atomically or not at all - #2356
erwan-joly wants to merge 1 commit into
Conversation
SaveService walked ten DAOs sequentially, each on its own context, so a crash or a swallowed DAO failure mid-save persisted half a character - gold without inventory, quests without objectives. DAO calls on the current async flow now share one transaction via IDaoTransactionScope: the DbContext registration consults an AsyncLocal slot an active scope fills, every operation's result is checked, and the commit happens only after all of them succeed. AsyncLocal keeps the parallel save-all path isolated per character. Begin is synchronous on purpose - an AsyncLocal written inside an awaited method does not reach the caller's flow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughSaveService now executes character persistence through one DAO transaction. New persistence interfaces, ambient database-context handling, dependency registrations, commit and rollback behavior, and in-memory test configuration support this flow. ChangesTransactional character saves
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Character saves now commit as one unit, preventing ordinary failures from leaving partially persisted state. Mergeability is otherwise reasonable, but transaction-start failures could retain database resources and nested save scopes could allow later work outside the intended transaction, so explicit owner awareness or follow-up is recommended. Sequence Diagram(s)sequenceDiagram
participant SaveService
participant IDaoTransactionScope
participant DAO
participant NosCoreContext
SaveService->>IDaoTransactionScope: Begin()
IDaoTransactionScope->>NosCoreContext: Start database transaction
IDaoTransactionScope-->>SaveService: Return transaction
SaveService->>DAO: Execute persistence operations
DAO->>NosCoreContext: Use ambient context
DAO-->>SaveService: Return operation result
alt All operations succeed
SaveService->>IDaoTransactionScope: CommitAsync()
IDaoTransactionScope->>NosCoreContext: Commit transaction
else An operation fails
SaveService->>IDaoTransactionScope: DisposeAsync()
IDaoTransactionScope->>NosCoreContext: Dispose without commit
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
erwan-joly
left a comment
erwan-joly
left a comment
There was a problem hiding this comment.
Reviewed the transaction wiring end-to-end (SaveService, PersistenceModule, Dao<> in NosCore.Dao, and the call sites). The Fail() + early-return restructuring is a clear improvement — every DAO result is now checked, which is what the old code got wrong, and rollback-on-dispose is the right shape. Four things I'd want resolved before this merges.
1. The ambient slot leaks to the caller under Task.WhenAll — and points at a disposed context
SaveAllSessionsHandler and WorldServer.cs:57 both do:
await Task.WhenAll(sessionRegistry.GetSessions().Select(saveService.SaveAsync));SaveAsync has no await before daoTransactionScope.Begin(), so the AsyncLocal write happens synchronously on the caller's execution context. Invoking an async method does not fork the EC — it only forks at the first suspension. So:
SaveAsync(s1)runs inline, sets the slot to ctx1, hits its first await, captures EC{ctx1}SaveAsync(s2)starts on the caller's EC (now holding ctx1), sets ctx2, captures EC{ctx2}
Each save is correctly isolated after its first await, so the comment's claim holds for the saves themselves. But the enumerating flow keeps whatever was written last, and each DaoTransaction.DisposeAsync clears the slot on its own flow, not the caller's. After the WhenAll completes, the handler's ambient slot still references the last-begun, now-disposed DbContext. Any DAO call later on that flow gets an ObjectDisposedException.
Today both call sites end right after the WhenAll, so it doesn't bite. It's a loaded gun for the next person who adds a line.
Cheapest fix: await Task.Yield(); immediately before Begin(), so the method suspends and forks the EC before writing the slot. Alternatively have the handlers wrap each save in Task.Run.
2. MirrorTo does not consult AmbientDbContext, so the transaction silently does nothing on that path
Load routes DbContext through the ambient slot:
builder.Register(c => AmbientDbContext.Current ?? (DbContext)c.Resolve<NosCoreContext>()).As<DbContext>()MirrorTo does not:
services.AddTransient<DbContext, NosCoreContext>();Dao<> takes Func<DbContext>, so anything resolving DAOs through the IServiceCollection path gets a fresh context per call while the scope holds a different one. The transaction then wraps nothing: CommitAsync commits an empty transaction and disposal rolls back nothing, with no error anywhere. The writes still land, so it fails open — you lose atomicity and nothing tells you.
Even if every host is Autofac-backed today, having two registration sites that differ on exactly the behaviour this PR exists to provide is the trap. Either mirror the ambient lookup or make MirrorTo throw for IDaoTransactionScope so the unsupported path is loud.
3. Singleton scope resolving a scoped NosCoreContext
services.AddSingleton<IDaoTransactionScope>(sp => new DaoTransactionScope(() => sp.GetRequiredService<NosCoreContext>()));NosCoreContext is registered with AddDbContext in all six bootstraps, which is scoped. A singleton capturing the root provider and resolving a scoped service is the classic captive-dependency bug — it throws from the root provider, or hands back an instance with the wrong lifetime. Same root cause as #2: this registration is written as if it were the Autofac one.
4. No test would fail if the transaction were deleted
The test suppresses InMemoryEventId.TransactionIgnoredWarning. That warning is EF telling you the in-memory provider ignores transactions entirely — so the atomicity this PR adds is not exercised. Worse, the test builds its DAOs as:
new Dao<ItemInstance, IItemInstanceDto?, Guid>(..., ContextBuilder)with a factory that returns a fresh context, bypassing AmbientDbContext even in principle. So the wiring that makes the feature work — the container indirection — is not under test either.
Suppressing the warning is the only way to make it run on InMemory, so this needs a different provider to be meaningful: a real Postgres (Testcontainers or the CI service) asserting that a mid-save failure leaves zero rows, and that the ambient slot is what the DAOs actually receive. Without that, the headline of this PR is unverified.
Smaller
AmbientDbContext.Set(null)on dispose discards rather than restores. Capturing the previous value and restoring it makes nesting safe and costs one field.Begin()uses the synchronousBeginTransaction(), which opens the Npgsql connection on the calling thread. The comment correctly explains whyBeginmust be sync, but the transaction start itself could still be async if the ambient write happened first.- Pre-existing, not this PR: nothing disposes the per-call contexts the DAOs create when no scope is active. Worth a follow-up.
1 participant
Architecture-review PR 3: save atomicity.
Problem
SaveService.SaveAsyncwalks ~10 DAOs sequentially — account, character, quicklist, inventory (two tables, FK-ordered), bonuses, titles, miniland, quests, objectives, respawns — and each DAO op runs on its own DbContext with its own SaveChanges. A crash mid-save, or any of the DAO-swallowed failures, persists half a character: gold updated but inventory not, quests without their objectives. Only two ops even checked their results (for FK-cascade noise, not consistency).Change
IDaoTransactionScope(NosCore.Core, no EF dependency) /DaoTransactionScope(NosCore.Database):Begin()opens one context + transaction and publishes the context in an AsyncLocal slot; the AutofacDbContextregistration consults that slot before building a fresh context, so every DAO call on the same async flow lands in the transaction.Task.WhenAllsave-all stays safe — AsyncLocal isolates concurrent flows per character.Begin()is deliberately synchronous: an AsyncLocal written inside an awaited method doesn't flow back to the caller.SaveAsyncwraps the whole walk in a scope, checks every DAO result (they swallow exceptions and report via return values), and commits only at the end; any failure or exception rolls the entire save back. FK insert/delete ordering preserved (Postgres validates per statement).DAO behavior outside a scope is unchanged (fresh context per op, as before). Tests construct DAOs with raw context builders so they bypass the ambient path; the InMemory provider ignores transactions via the standard warning suppression.
Noted for a NosCore.Dao follow-up:
Daonever disposes the contexts it builds.Verification
Build clean; GameObject.Tests (incl. SaveService persistence specs), PacketHandlers.Tests, Database.Tests all pass.
Merge note: overlaps
SaveService.cswith the upcoming LastSp-persistence PR — trivial rebase whichever lands second.🤖 Generated with Claude Code
Summary by CodeRabbit
Reliability
Bug Fixes