fix(journaling)!: recover failed operations in new activations - #11276
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical issues remain in stale-write coalescing and concurrent revert queue handling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR hardens journaling recovery boundaries and prevents stale mutations after recovery failures.
Changes:
- Adds recovery fencing and generation tracking.
- Rebinds states missing after journal replay.
- Expands regression coverage for recovery failures and stale writes.
The review found two critical recovery-concurrency issues and one minor wording issue in JournaledStateManager.cs.
| File | Summary |
|---|---|
test/Orleans.Journaling.Tests/StateManagerTests.cs |
Adds coverage for recovery fencing and stale writes. |
src/Orleans.Journaling/JournaledStateManager.cs |
Implements recovery fencing, generation tracking, and state rebinding. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage
Report-only conclusion: current-main baseline stale. The newest successful coverage run tested 1133353, not current main 02ae387. Coverage combines every CI test matrix job, including providers, CodeGen, .NET 8/10, Linux, Windows, and macOS, using canonical physical source and branch identities. The comparison remains report-only while normal line and branch variance is calibrated. Coverage details |
Stage only safe-to-commit mutations. Permanently fence failed managers and deactivate grain owners instead of resetting live state beneath interleaved methods. BREAKING CHANGE: Remove IJournaledStateManager.RevertPendingChangesAsync. Recovery after failure requires a fresh activation or a new standalone manager with new state instances.
ReubenBond
changed the title
Problem
Reloading a live journaled state manager can reset another interleaved grain method's mutations while its local decisions and references still describe the earlier state. A subsequent write can then acknowledge an empty or unrelated batch. Recovery-generation checks cover already queued writes, but cannot account for methods which enqueue their write after the reset.
Solution
Adopt a safe-to-commit staging contract: prepare fallible work and external acknowledgements in operation-local data, then apply the resulting mutations to shared durable state and await a write.
Remove
IJournaledStateManager.RevertPendingChangesAsyncand automatic in-place recovery after journal failures. A failed initialization, write, snapshot, or deletion permanently fences that manager, faults queued work, and requests grain deactivation. A new activation replays the durable journal. Owners of factory-created managers dispose the failed instance and create a new manager for the sameJournalIdwith new state instances.Preserve cancellation's wait-only behavior for queued writes and initial replay's registered-state rebinding and unknown-stream preservation. Include regression coverage for interleaved callers, queued operations, lost commit acknowledgements, grain deactivation, and fresh standalone-manager recovery. Regenerate the public API surface and scope compatibility suppressions to the intentional removal.
Rationale
Recovery belongs to a fresh execution context, where application fields, local work, and durable state are reconstructed together. An uncertain write outcome is reconciled through application idempotency rather than rewinding objects still referenced by executing methods.
This replaces the earlier recovery-and-resume design in this PR using a forward commit, preserving published dependency pins. The Durable Messaging consumer in #10693 will adopt operation-local inbox/outbox staging in its owning layer; participant and observer layers must follow the new terminal-failure boundary.