Skip to content

Conversation

@rkargMsft
Copy link
Contributor

@rkargMsft rkargMsft commented Feb 2, 2026

Adjusts the log levels so that:

  • Proper use of the cancellationToken and an intentional Deactivate during OnActivteAsync is only an Info level (this is a proper implementation handling the cancel properly).
  • Ignored cancellation token (not passed to downstream calls or not handled by downstream calls) that accesses an object disposed after Deactivate (this is often the IServiceProvider) are logged as Warning since the end behavior is acceptable (the activation is gone) but there is an error in the handling (or not handling) of the cancellationToken
  • Existing Error log for any of the above exceptions but without a triggered Deactivate
  • Existing Error log for any other exceptions

This will clean up logs for proper implementations, add new warning for improper but benign implementations, and keep the Error logs for real errors.


I'll rebase this on top of #9870 once that's merged in so we get proper activity participation in these error/not-error scenarios.

Microsoft Reviewers: Open in CodeFlow

Clean up log levels for intentional cancellation during OnActivateAsync execution.
- Ensures only unexpected activation failures are logged as errors, reducing log noise and improving clarity
- Add new error code and log message for disposed object access during activation cancellation
- Adjust log levels: cancellation-related exceptions now log at Info/Warning, not Error
- Add interfaces and grains to simulate various activation cancellation scenarios
- Introduce comprehensive tests to verify correct logging behavior for cancellation vs. non-cancellation exceptions
@ReubenBond
Copy link
Member

Doesn't .NET include a 'pass the CancellationToken where applicable' analyzer by default?

@ReubenBond
Copy link
Member

ReubenBond commented Feb 2, 2026

I'm not sure about the validity of a WaitAsync(ct) codefix, as in it might not be the right option in all cases. IMO we should split the analzyer+codefix out into a separate PR

@rkargMsft
Copy link
Contributor Author

I'm not sure about the WaitAsync(ct) codefix. IMO we should split the analzyer+codefix out into a separate PR

The .WaitAsync is for cases where the call doesn't accept a CancellationToken, like

  var reminder = await this.GetReminder("Name").WaitAsync(cancellationToken);

This would also apply to other user code that has async calls that don't take a CancellationToken parameter.

Without the WaitAsync, OnActivateAsync will continue to execute and this is where the disposed IServiceProvider exception gets propagated up and logged as an Error (or as a Warning with this PR). The only thing that a user can do in that scenario (non-CT accepting async method) is to add .WaitAsync(ct). The underlying method will continue to execute to completion in any scenario, that can't be fixed. But we don't care about it hitting a disposed object if we've already cancelled the activation.

@rkargMsft
Copy link
Contributor Author

rkargMsft commented Feb 2, 2026

Doesn't .NET include a 'pass the CancellationToken where applicable' analyzer by default?

There is but if a different CT is passed then it will not trigger that analyzer but still block the ActivationData await on OnActivateAsync.

        public override async Task OnActivateAsync(CancellationToken cancellationToken)
        {
            await base.OnActivateAsync(cancellationToken);
            _counter.State.Counter = random.Next(100);
            _counter.State.CurrentDateTime = DateTime.UtcNow;
            // this call will not trigger the .NET analyzer but continue to block OnActivateAsync as it executes
            await _counter.WriteStateAsync(CancellationToken.None);
        }

Though that would trigger the WaitAsync analyzer, so that's the only one that appears to be necessary (it check for explicitly the OnActivateAsync parameter to be passed in and will suggest WaitAsync in that case).

@rkargMsft
Copy link
Contributor Author

The analyzer can get split out, too

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.

3 participants