Repository navigation
fix(runtime): honor successful activation after cancellation - #11650
ReubenBond wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical registration/deactivation race can overwrite the Deactivating state and trigger duplicate teardown, calling OnDeactivateAsync after the grain and its services have been disposed.
1 open issue
What changed in this PR
This PR addresses #8812 by honoring a successful OnActivateAsync return after cancellation, allowing Orleans to invoke OnDeactivateAsync for resource cleanup.
Changes:
- Tracks successful activation independently of routing state.
- Adds regression coverage for callback outcomes, cleanup ordering, and replacement activation.
- Documents activation cancellation and completion behavior.
| File | Description |
|---|---|
test/Orleans.Runtime.Internal.Tests/ActivationsLifeCycleTests/LifecycleCallbackCancellationTests.cs |
Tests cancellation outcomes and deactivation callback ordering. |
src/Orleans.Runtime/Catalog/ActivationData.cs |
Tracks activation completion and pairs successful activation with deactivation. |
docs/site/src/content/docs/grains/grain-lifecycle.md |
Clarifies cancellation semantics and resource cleanup. |
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
A critical registration race can overwrite the deactivating state and run teardown twice after disposal, while an existing retirement test still expects no migration despite the changed successful-activation behavior.
2 open issues
Review effort: Balanced
tomachristian
left a comment
There was a problem hiding this comment.
Hi Reuben, I'm Claude. Cristi asked me to give this a review round as well. I've left out what Copilot already raised.
The direction is right: when OnActivateAsync returns normally, the grain has done its setup and should get OnDeactivateAsync to undo it. I read it on top of #11514 and followed each way an activation's token gets cancelled: a deactivation request, the silo stopping, ActivationTimeout, and a migration requested during activation.
- It changes what a normal return means for code that already exists: a grain that swallows the cancellation and returns used to count as cancelled, and now counts as activated (docs, line 20).
- When only
ActivationTimeoutcancelled the token, the activation now succeeds and is then deactivated, and its queued calls are forwarded instead of rejected (line 2124). - A migration requested during activation still migrates an activation that didn't complete (line 2268).
- The deactivation timeout starts at the request, so
OnDeactivateAsynccan start with its token already cancelled, as it does in this PR's own tests (line 764).
There was a problem hiding this comment.
🟡 Changes recommended
The public ActivationTimeout remarks still contradict the cancellation behavior by failing to distinguish a successful OnActivateAsync return from cancellation during lifecycle startup.
1 open issue
2 resolved since last review
Review effort: Balanced
| // A normal return completes activation. Deactivation requested during activation follows it. When only the | ||
| // activation timeout canceled the token, the activation remains valid. |
There was a problem hiding this comment.
🟡 Changes recommended
Public timeout XML remarks still incorrectly imply that cancellation always fails activation and omit that the deactivation callback timeout starts after activation and running requests complete.
2 open issues
Review effort: Balanced
| ## Activation and deactivation timeouts | ||
|
|
||
| <xref:Orleans.Configuration.GrainCollectionOptions.ActivationTimeout> and <xref:Orleans.Configuration.GrainCollectionOptions.DeactivationTimeout> both default to 30 seconds. When a timeout elapses, Orleans cancels the <xref:System.Threading.CancellationToken> passed to <xref:Orleans.Grain.OnActivateAsync*>, <xref:Orleans.Grain.OnDeactivateAsync*>, and grain lifecycle participants, then waits for that code to complete. The activation keeps its grain directory registration and activation-scoped services until those callbacks return. If the activation timeout elapses, activation fails after the callbacks return. If the deactivation timeout elapses, deactivation continues after the callbacks return. | ||
| <xref:Orleans.Configuration.GrainCollectionOptions.ActivationTimeout> and <xref:Orleans.Configuration.GrainCollectionOptions.DeactivationTimeout> both default to 30 seconds. When a timeout elapses, Orleans cancels the <xref:System.Threading.CancellationToken> passed to <xref:Orleans.Grain.OnActivateAsync*>, <xref:Orleans.Grain.OnDeactivateAsync*>, and grain lifecycle participants, then waits for that code to complete. The activation keeps its grain directory registration and activation-scoped services until those callbacks return. If the activation timeout elapses, activation fails when a callback throws or Orleans stops before the next lifecycle stage or <xref:Orleans.Grain.OnActivateAsync*>. When <xref:Orleans.Grain.OnActivateAsync*> returns normally, the activation remains valid and Orleans logs a warning. The deactivation timeout starts when deactivation callbacks begin, after activation and running requests complete. If it elapses, deactivation continues after the callbacks return. |
Code coverage
Report-only conclusion: improved. The current-main baseline is commit 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 |
Replace ActivationState with a linear chain which is symmetric around Active. Each setup phase pairs with the teardown phase which undoes it, teardown starts at the mirror of the last completed setup phase, a deactivation request during setup is recorded intent which diverts setup at its next phase boundary, and forced disposal is an explicit transition to Disposing. SetState asserts each transition. Part of dotnet#8812.
303d8c3 to
177e6e4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The public ActivationTimeout documentation still promises activation failure on cancellation, contradicting the new successful-return behavior and potentially encouraging early returns that expose partially initialized grains.
3 open issues
Review effort: Balanced
| // A normal return from OnActivateAsync completes activation, including after its token was canceled. When deactivation | ||
| // was requested, teardown starts by running OnDeactivateAsync. When only the activation timeout canceled the token, | ||
| // the activation becomes active. |
Setup boundaries stop setup without entering teardown; the deactivation operation alone maps the last completed phase to its teardown entry. ActivateAsync and FinishDeactivating become sequences of phase methods, and teardown is one loop over its phases. Rehydration-context ownership moves to a single TakeRehydrationContext helper.
177e6e4 to
580c633
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The public ActivationTimeout XML remarks still contradict the implementation and updated guide by claiming token cancellation fails activation even when OnActivateAsync returns successfully.
3 open issues
Review effort: Balanced
…ed disposal Recheck Rehydrating under the lock before invoking migration participants, so forced disposal which preempts rehydration skips them. Allow an activation operation queued before a pre-start deactivation request to find the activation in None. Remove the forcibly disposed activation from the catalog in the disposal test, as ActivationDirectory disposal does.
580c633 to
6ea0109
Compare
| // The outcome of OnActivateAsync decides activation: a normal return completes it, including after its token was canceled. | ||
| await grainBase.OnActivateAsync(cancellationToken); |
…ctivation The process-wide activity listener now disposes only when the rehydrate span starts on the target activation's scheduler, and the test asserts the activation was Rehydrating at that point.
…ntrant observers Dispose an unconsumed rehydration context alongside the service scope, so a context whose disposal throws cannot prevent the grain instance and services from being disposed. Deactivate no longer asserts that the activation is not disposing, since lifecycle observers and diagnostic listeners run synchronously inside it and may force disposal.
…ion setup Deactivate and FailSetup record the request, the transition into teardown, and the scheduled deactivation before observers and cancellation callbacks run. RunOnActivateAsync reports observed cancellation, so forced disposal during OnActivateAsync is a cancellation, not a failure. The success tail runs after setup, the activation span ends on every path, a throwing rehydration context no longer fails activation, and teardown after a registration failure no longer receives the canceled activation token. Tests now fail on ActivationData assertion failures which the operation loop swallows, record lifecycle start, stop, and deactivation so teardown entries are exact, and observe directory calls for pre-start deactivation.
6ea0109 to
eb4428f
Compare
…r in teardown Dehydrating discards a migration requested during setup, so an activation whose setup stopped never sends partially initialized state, such as grain state whose storage read was canceled. Unregistering removes the activation's registration regardless of the deactivation reason: a registration which threw may have committed, and unregistration is conditional on the activation's identity. The Accordant model now gates directory registration and unregistration and covers migration requests and forced disposal.
eb4428f to
1db41ea
Compare
ActivationState is byte-backed with None = 0, Active = 127, and Destroyed = 254, so the mirror remains Destroyed - s and new phase pairs can be inserted at mirrored values without renumbering. Transitions use an explicit GetNext rather than arithmetic. Rehydration rechecks Rehydrating before each migration participant, so a participant which forces disposal stops the rest. An activation operation which finds setup stopped before registration ends the activation span. Fixes the missing final newline flagged by the whitespace check.
1db41ea to
98e5679
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The warning can misreport caller cancellation as an activation timeout, and the ActivationTimeout API remarks still contradict the documented activation behavior.
5 open issues
Cancellation test fixture no longer throws when activation is canceled Avoid reporting a timeout when the caller cancels activation · New Clarify activation timeout cancellation guidance Update timeout remarks to match the new activation behavior Update ActivationTimeout remarks for revised cancellation behavior
Review effort: Balanced
|
|
||
| [LoggerMessage( | ||
| Level = LogLevel.Warning, | ||
| Message = "Activation of {Grain} completed after its activation timeout of {ActivationTimeout} elapsed. The activation remains active.")] |
…e cases Accordant's transition-coverage generation at depth 10 produces about 9,400 overlapping sequences covering 51 model transitions, which took minutes to execute. The test now greedily selects a deterministic subset which covers every transition the generated cases cover (19 cases), running in about a second.
A normal return from OnActivateAsync now completes activation even if its cancellation token was canceled; stopping early requires throwing. A deactivation requested during activation enters teardown at Deactivating once OnActivateAsync returns, so OnDeactivateAsync runs. When only the activation timeout canceled the token, the activation becomes active and Orleans logs a warning. Only completed activations migrate, and the deactivation timeout starts when teardown begins. Fixes: dotnet#8812
98e5679 to
f36cf28
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The shutdown test can release the activation callback before cancellation is observed, causing intermittent migration assertion failures, and the public API remarks still describe outdated cancellation behavior.
6 open issues
Cancellation test fixture no longer throws when activation is canceled Wait for activation cancellation before releasing the held callback · New Avoid reporting a timeout when the caller cancels activation Clarify activation timeout cancellation guidance Update timeout remarks to match the new activation behavior Update ActivationTimeout remarks for revised cancellation behavior
Review effort: Balanced
| // Throwing on cancellation interrupts activation, which may hold partially initialized state, so its request is forwarded or rejected. | ||
| var stopping = cluster.StopSiloAsync(retiring, Cancellation); | ||
| await WaitUntilAsync(() => activation.State is ActivationState.Deactivating or ActivationState.Invalid); | ||
| await WaitUntilAsync(() => activation.IsDeactivationRequested); |



A grain can finish
OnActivateAsyncsuccessfully after its cancellation token is canceled. Orleans should honor that outcome and runOnDeactivateAsyncwhen deactivation was requested, so the grain can clean up resources.On the symmetric state machine from #11652, the outcome of
OnActivateAsyncnow decides theActivatingphase. A normal return completes it, including after cancellation. When deactivation was requested during activation, teardown then starts atDeactivating, the mirror of the completed phase, soOnDeactivateAsyncruns. When onlyActivationTimeoutcanceled the token, the activation becomesActive, and Orleans logs a warning. Throwing failsActivating, and teardown starts atStoppingwithout callingOnDeactivateAsync. A canceled lifecycle start still fails activation.#11652 migrates only activations which completed activation. With this change, a migration requested during
OnActivateAsynctherefore proceeds whenOnActivateAsyncreturns normally, and is discarded when it throws.DeactivationTimeoutnow starts when teardown begins rather than when deactivation is requested.Updates the lifecycle, timeout, and state-machine documentation. Adds regression coverage for callback outcomes, timeout-only cancellation, migration during activation, retirement, and deactivation-timeout timing, and updates the state-machine model.
Release notes
A normal return from
OnActivateAsyncnow completes activation even if its cancellation token was canceled. Code that stops early after observing cancellation must throw, for example withcancellationToken.ThrowIfCancellationRequested().DeactivationTimeoutnow starts when deactivation callbacks begin.Fixes: #8812
Microsoft Reviewers: Open in CodeFlow