Skip to content

feat(journaling)!: require operation prerequisite and completion hooks - #11465

Open
ReubenBond wants to merge 18 commits into
dotnet:mainfrom
ReubenBond:rb-journal-operation-hooks
Open

ReubenBond wants to merge 18 commits into
dotnet:mainfrom
ReubenBond:rb-journal-operation-hooks

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Problem

Journal-backed features need prerequisites before capture and completion work after storage ACK. They also need to install or remove hooks from state lifecycle callbacks while keeping each persistence operation's callback set stable.

Solution

Every IJournaledStateManager provides AddHookAsync(hook, index, cancellationToken) and RemoveHookAsync(hook, cancellationToken). Registrations are private to the manager; features retain their own hook references. The backing collection is a lazily allocated list.

Registration completes synchronously before initialization, supporting constructors. After initialization begins, idle changes apply synchronously; busy changes enter the work queue and apply after the current operation's after callbacks finish. Admission uses the manager's existing lock because standalone managers also run outside grain schedulers. Once dequeued, the work loop owns the hook set and applies queued changes directly. Failed recovery publishes idle state and queued failures together under the admission lock.

Queued changes separate write-coalescing groups. Caller cancellation ends its wait while the manager owns admitted changes. Shutdown cancels queued changes; persistence failures preserve their original cause. Invalid queued changes are logged and fault their own completion while the owner stays usable.

Before callbacks run in reverse registration order; after callbacks run forward. Register the final prerequisite with index: 0. Every before callback is awaited. Prerequisites remain valid through capture/deletion, including changes staged during asynchronous preparation or resumed turns. The completion-to-capture boundary retains its documented non-atomic behavior.

Synchronous state callbacks can request a change and observe its queued completion outside the callback. Features await registration before staging changes which depend on its effect. Constructor registration uses the guaranteed synchronous setup path; features created after initialization observe completion during asynchronous setup.

Append and snapshot requests use one WriteStateWorkItem. Operation selection consumes _snapshotRequired, retaining the decision locally; newer requests survive earlier ACKs. Before-hook failure restores the requirement. Snapshot writers validate preserved-entry formats before storage I/O. A tuple-returning helper gathers storage diagnostics, with status and disposal owned by the work loop.

JournaledStatePreCommitException reports retained pending state; JournaledStatePostCommitException reports completed persistence. Every after hook runs despite cleanup failures. Storage failures retain fencing and original-error precedence. Disposal drains owned hooks and I/O before releasing resources, and concurrent or reentrant disposal shares one completion. Callback dependencies between journal owners remain acyclic.

Consumer adoption

Custom owners implement the two registration methods; delegating owners forward them. Replace direct collection mutation with these methods and retain hook references for removal. Remove forwarding or inspection of the former Hooks property. Final prerequisites register using AddHookAsync(hook, index: 0) with completion established before dependent work.

This verified successor is adopted by the coordinated Messaging chain through an explicit pin. Parent and sibling worktrees retain their current pins until that integration is sequenced.

Package validation remains enabled. Hook-specific compatibility suppressions cover the two required registration methods on net8.0/net10.0; obsolete property suppressions are removed. Normal Release pack passes, and the public surface is regenerated using the official GenAPI target.

Rebase and scope

Based on main 79873eb26a59742bc520fcae87c19fe7dc23c7df, preserving merged timer, caching, Arc-lifetime and volatile-snapshot work. The extraction originated from 48f3aaa47fc18123db4109063af591f315e6f9cb..9c1ef0f2feb258c1764de5f408d89c53e7e63f9f.

Storage borrowing contracts, V0/V1 framing/readers, retained storage and dictionary value ownership remain intact. Composition preserves the shared shutdown cancellation/drain correction and dequeue gate once.

Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings October 10, 2026 15:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The work loop can execute queued non-hook work after shutdown cancellation.

1 open finding
What changed in this PR

Adds lifecycle hooks around journal capture, persistence acknowledgement, and deletion.

Changes:

  • Adds hook interfaces, delegate adapter, operation types, and failure exceptions.
  • Integrates ordered hooks, cancellation, disposal, and re-entry safeguards into the state manager.
  • Adds API surfaces, documentation, and comprehensive tests.
File Description
test/​Orleans.Journaling.Tests/​StateManagerHookTests.cs Tests hook ordering, failures, cancellation, disposal, and serialization.
src/​Orleans.Journaling/​README.md Documents hook semantics.
src/​Orleans.Journaling/​JournaledStatePreCommitException.cs Adds prerequisite-failure reporting.
src/​Orleans.Journaling/​JournaledStatePostCommitException.cs Adds post-persistence failure reporting.
src/​Orleans.Journaling/​JournaledStateManager.cs Implements hook execution and lifecycle handling.
src/​Orleans.Journaling/​JournaledStateHook.cs Adapts delegates to hooks.
src/​Orleans.Journaling/​IJournaledStateManager.cs Exposes hook registration.
src/​Orleans.Journaling/​IJournaledStateHook.cs Defines hook callbacks and operation types.
src/​Orleans.Journaling/​IJournaledStateCaptureHook.cs Defines the final capture prerequisite.
src/​api/​Orleans.Journaling/​Orleans.Journaling.cs Regenerates the public API surface.
docs/​site/​src/​content/​docs/​grains/​journaling/​runtime-behavior.md Documents runtime behavior.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Orleans.Journaling/JournaledStateManager.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 18:45
@ReubenBond
ReubenBond force-pushed the rb-journal-operation-hooks branch from c9520d3 to f14c07f Compare October 10, 2026 18:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The concurrency-sensitive work-loop, cancellation, and disposal changes warrant final human validation despite strong test coverage.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Hook mutation remains possible after operation admission but before work-loop dequeue.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject hook mutations while persistence is queued or running

src/​Orleans.Journaling/​JournaledStateManager.cs:815

Reject hook-list mutations while a persistence operation is queued as well as while it is executing. On the owner's single-threaded context, WriteStateAsync can enqueue and return an incomplete task before the work loop gets to set _hookOperationRunning; a subsequent Hooks.Add/Clear therefore succeeds and changes the hook set of an already-admitted operation, contradicting the operation-stable/in-flight mutation contract. Checking the queue under this same lock closes that admission-to-dequeue gap.

🧠 Review effort: Balanced

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request Current main Variance
Lines 83.49% (118,807 / 142,293) 83.48% (118,623 / 142,096) +0.0137 pp
Branches 73.09% (35,348 / 48,360) 73.09% (35,306 / 48,308) +0.0083 pp

Report-only conclusion: improved.

The current-main baseline is commit 79873eb26a and uses the same reviewed coverage matrix.

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

Copilot AI balanced review requested due to automatic review settings October 11, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Indirect cross-owner hook recursion can deadlock journal work loops.

1 open finding

🧠 Review effort: Balanced

Comment thread src/Orleans.Journaling/JournaledStateManager.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 11, 2026 01:31
@ReubenBond ReubenBond changed the title feat(journaling): add operation prerequisite and completion hooks feat(journaling)!: require operation prerequisite and completion hooks Oct 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reentrant disposal during cancellation can start duplicate drains and dispose shared resources twice.

2 open findings

🧠 Review effort: Balanced

Comment thread src/Orleans.Journaling/JournaledStateManager.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 11, 2026 02:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The public API break and concurrency-sensitive persistence, cancellation, and disposal semantics warrant final human review.

2 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 15:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes central persistence, failure, cancellation, and disposal semantics and warrants final human concurrency review.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 15:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes public contracts and concurrency-sensitive persistence, cancellation, and disposal paths requiring final human review.

0 open findings

🧠 Review effort: Balanced

@ReubenBond
ReubenBond force-pushed the rb-journal-operation-hooks branch from 9627bea to 470ec86 Compare October 11, 2026 23:08
Copilot AI balanced review requested due to automatic review settings October 11, 2026 23:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes intertwined persistence, cancellation, disposal, and public API contracts that warrant final human validation.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 23:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Snapshot trigger telemetry loses its storage or migration cause and reports an incorrect compaction reason.

1 open finding

🧠 Review effort: Balanced

Comment on lines +324 to +326
compactionReason = _snapshotRequired
? JournalingInstruments.CompactionReasonUserSnapshot
: JournalingInstruments.CompactionReasonStorageRequested;
Copilot AI balanced review requested due to automatic review settings October 12, 2026 00:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Snapshot telemetry is misclassified, and two changed diagnostics provide contradictory or insufficient guidance.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify registration freeze applies only to state registration

src/​Orleans.Journaling/​JournaledStateManager.cs:137

This message says all journal registrations are frozen after initialization, but the newly added hook-registration contract explicitly permits idle or queued changes after initialization. This guard only protects state-machine/durable-state registration, so the broader wording gives callers contradictory guidance; name state registration specifically and update the matching assertion.

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants