fix: a character save commits atomically or not at all

[erwan-joly]

Architecture-review PR 3: save atomicity.

Problem

SaveService.SaveAsync walks ~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 Autofac DbContext registration consults that slot before building a fresh context, so every DAO call on the same async flow lands in the transaction. Task.WhenAll save-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.
• SaveAsync wraps 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).
• Disposing without commit = rollback; the scope disposes both transaction and context.

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: Dao never disposes the contexts it builds.

Verification

Build clean; GameObject.Tests (incl. SaveService persistence specs), PacketHandlers.Tests, Database.Tests all pass.

Merge note: overlaps SaveService.cs with the upcoming LastSp-persistence PR — trivial rebase whichever lands second.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Reliability

  • Character saves now commit all changes together, preventing partial saves.
  • If any save operation fails, changes are rolled back and an error is logged.

• Bug Fixes

  • Improved consistency across item, quest, objective, and respawn save operations.
  • Respawn save failures are now handled consistently with other persistence errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 65ff7fe9-811d-43a1-a310-085ab58cda4e

📥 Commits

Reviewing files that changed from the base of the PR and between a5787ac and 1031bc5.

📒 Files selected for processing (5)

• src/NosCore.Core/Persistence/IDaoTransactionScope.cs
• src/NosCore.Database/Hosting/DaoTransactionScope.cs
• src/NosCore.Database/Hosting/PersistenceModule.cs
• src/NosCore.GameObject/Services/SaveService/SaveService.cs
• test/NosCore.GameObject.Tests/Services/SaveService/SaveServiceTests.cs

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

Walkthrough

SaveService 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.

Changes

Transactional character saves

Layer / File(s)	Summary
Transaction contract and runtime src/NosCore.Core/Persistence/IDaoTransactionScope.cs, src/NosCore.Database/Hosting/DaoTransactionScope.cs	The new interfaces define transaction creation and commit operations. DaoTransactionScope creates the database transaction, exposes its context through AsyncLocal, and disposes the transaction and context.
Transaction dependency registration src/NosCore.Database/Hosting/PersistenceModule.cs	The persistence module registers ambient DbContext resolution and exposes IDaoTransactionScope in both dependency-injection configurations.
Transactional SaveService flow src/NosCore.GameObject/Services/SaveService/SaveService.cs, test/NosCore.GameObject.Tests/Services/SaveService/SaveServiceTests.cs	SaveService checks DAO results, returns through a shared failure path when an operation fails, and commits after all operations succeed. Tests provide DaoTransactionScope and ignore unsupported in-memory transaction warnings.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🔵 Low · up to 1031b

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

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: character saves now commit atomically or roll back entirely.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch arch/atomic-save

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}