diff --git a/durable0011-warnings.png b/durable0011-warnings.png new file mode 100644 index 000000000..b87a641a3 Binary files /dev/null and b/durable0011-warnings.png differ diff --git a/src/Analyzers/AnalyzerReleases.Unshipped.md b/src/Analyzers/AnalyzerReleases.Unshipped.md index 74657c2a1..d61f18daa 100644 --- a/src/Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Analyzers/AnalyzerReleases.Unshipped.md @@ -5,3 +5,4 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- +DURABLE0011 | Orchestration | Warning | ContinueAsNewOrchestrationAnalyzer diff --git a/src/Analyzers/Orchestration/ContinueAsNewOrchestrationAnalyzer.cs b/src/Analyzers/Orchestration/ContinueAsNewOrchestrationAnalyzer.cs new file mode 100644 index 000000000..baa968637 --- /dev/null +++ b/src/Analyzers/Orchestration/ContinueAsNewOrchestrationAnalyzer.cs @@ -0,0 +1,109 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using static Microsoft.DurableTask.Analyzers.Orchestration.ContinueAsNewOrchestrationAnalyzer; + +namespace Microsoft.DurableTask.Analyzers.Orchestration; + +/// +/// Analyzer that reports a warning when an orchestration contains an unconditional while loop +/// that calls any TaskOrchestrationContext method (e.g. CallActivityAsync, WaitForExternalEvent, +/// CallSubOrchestratorAsync, CreateTimer) but no ContinueAsNew call within that loop. +/// Every orchestration API call adds to the replay history, so unbounded loops without +/// ContinueAsNew lead to unbounded history growth. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ContinueAsNewOrchestrationAnalyzer : OrchestrationAnalyzer +{ + /// + /// Diagnostic ID supported for the analyzer. + /// + public const string DiagnosticId = "DURABLE0011"; + + static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.ContinueAsNewOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources)); + static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.ContinueAsNewOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources)); + + static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + Title, + MessageFormat, + AnalyzersCategories.Orchestration, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/durabletask-analyzers"); + + /// + public override ImmutableArray SupportedDiagnostics => [Rule]; + + /// + /// Visitor that inspects orchestration methods for unbounded loops without ContinueAsNew. + /// Only direct invocations within the loop body are considered; calls made through helper + /// methods invoked from the loop are not tracked back to the loop context. + /// + public sealed class ContinueAsNewOrchestrationVisitor : MethodProbeOrchestrationVisitor + { + /// + public override bool Initialize() + { + return this.KnownTypeSymbols.TaskOrchestrationContext is not null; + } + + /// + protected override void VisitMethod(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + { + foreach (WhileStatementSyntax whileStatement in methodSyntax.DescendantNodes().OfType()) + { + if (!IsAlwaysTrueCondition(whileStatement.Condition)) + { + continue; + } + + IOperation? whileOperation = semanticModel.GetOperation(whileStatement); + if (whileOperation is not IWhileLoopOperation whileLoop || whileLoop.Body is null) + { + continue; + } + + bool hasHistoryGrowingCall = false; + bool hasContinueAsNew = false; + + foreach (IInvocationOperation invocation in whileLoop.Body.Descendants().OfType()) + { + IMethodSymbol targetMethod = invocation.TargetMethod; + + if (targetMethod.IsEqualTo(this.KnownTypeSymbols.TaskOrchestrationContext, "ContinueAsNew")) + { + hasContinueAsNew = true; + } + else if (SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, this.KnownTypeSymbols.TaskOrchestrationContext) || + SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType?.OriginalDefinition, this.KnownTypeSymbols.TaskOrchestrationContext)) + { + hasHistoryGrowingCall = true; + } + + if (hasHistoryGrowingCall && hasContinueAsNew) + { + break; + } + } + + if (hasHistoryGrowingCall && !hasContinueAsNew) + { + reportDiagnostic(Diagnostic.Create(Rule, whileStatement.WhileKeyword.GetLocation(), orchestrationName)); + } + } + } + + static bool IsAlwaysTrueCondition(ExpressionSyntax condition) + { + return condition is LiteralExpressionSyntax literal + && literal.IsKind(SyntaxKind.TrueLiteralExpression); + } + } +} diff --git a/src/Analyzers/Resources.resx b/src/Analyzers/Resources.resx index 4638ae98d..0389ac749 100644 --- a/src/Analyzers/Resources.resx +++ b/src/Analyzers/Resources.resx @@ -222,4 +222,10 @@ Input parameter binding can be used instead of GetInput + + Orchestration '{0}' contains an unbounded while(true) loop that calls TaskOrchestrationContext methods but no ContinueAsNew within that loop. This can cause unbounded history growth leading to performance degradation and persistence failures. + + + Long-running orchestration loops should call ContinueAsNew to bound history growth + \ No newline at end of file diff --git a/test/Analyzers.Tests/Orchestration/ContinueAsNewOrchestrationAnalyzerTests.cs b/test/Analyzers.Tests/Orchestration/ContinueAsNewOrchestrationAnalyzerTests.cs new file mode 100644 index 000000000..d6f5f2ee6 --- /dev/null +++ b/test/Analyzers.Tests/Orchestration/ContinueAsNewOrchestrationAnalyzerTests.cs @@ -0,0 +1,229 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.CodeAnalysis.Testing; +using Microsoft.DurableTask.Analyzers.Orchestration; + +using VerifyCS = Microsoft.DurableTask.Analyzers.Tests.Verifiers.CSharpAnalyzerVerifier; + +namespace Microsoft.DurableTask.Analyzers.Tests.Orchestration; + +public class ContinueAsNewOrchestrationAnalyzerTests +{ + [Fact] + public async Task EmptyCodeHasNoDiag() + { + string code = @""; + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueWithExternalEventNoContinueAsNew_ReportsDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, object input) + { + {|#0:while|} (true) + { + var item = await context.WaitForExternalEvent(""new-work""); + await context.CallActivityAsync(""ProcessItem"", item); + } + } +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueWithSubOrchestratorNoContinueAsNew_ReportsDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, object input) + { + {|#0:while|} (true) + { + await context.CallSubOrchestratorAsync(""ProcessItem"", null); + } + } +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueWithContextCallsNoContinueAsNew_ReportsDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, object input) + { + {|#0:while|} (true) + { + await context.CallActivityAsync(""DoWork"", ""input""); + await context.CreateTimer(TimeSpan.FromSeconds(30), CancellationToken.None); + } + } +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueWithContinueAsNew_NoDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, object input) + { + int count = 0; + while (true) + { + var item = await context.WaitForExternalEvent(""new-work""); + await context.CallSubOrchestratorAsync(""ProcessItem"", item); + count++; + if (count >= 100) + { + context.ContinueAsNew(count); + return null; + } + } + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueWithOnlyActivitiesAndContinueAsNew_NoDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, object input) + { + while (true) + { + await context.CallActivityAsync(""DoWork"", ""input""); + await context.CreateTimer(TimeSpan.FromSeconds(30), CancellationToken.None); + context.ContinueAsNew(null); + return null; + } + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorNoWhileTrue_NoDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, string input) + { + var result = await context.CallActivityAsync(""DoWork"", input); + return result; + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorWhileTrueNoContextCalls_NoDiagnostic() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, string input) + { + int i = 0; + while (true) + { + i++; + if (i > 10) return ""done""; + } + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task DurableFunctionWhileTrueWithExternalEventNoContinueAsNew_ReportsDiagnostic() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +async Task Method([OrchestrationTrigger] TaskOrchestrationContext context) +{ + {|#0:while|} (true) + { + var item = await context.WaitForExternalEvent(""new-work""); + await context.CallActivityAsync(""ProcessItem"", item); + } +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task FuncOrchestratorWhileTrueWithContextCallsNoContinueAsNew_ReportsDiagnostic() + { + string code = Wrapper.WrapFuncOrchestrator(@" +tasks.AddOrchestratorFunc(""Run"", async context => +{ + {|#0:while|} (true) + { + var item = await context.WaitForExternalEvent(""new-work""); + await context.CallActivityAsync(""ProcessItem"", item); + } +}); +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task FuncOrchestratorWhileTrueNoContextCalls_NoDiagnostic() + { + string code = Wrapper.WrapFuncOrchestrator(@" +tasks.AddOrchestratorFunc(""Run"", async context => +{ + int i = 0; + while (true) + { + i++; + if (i > 10) return ""done""; + } +}); +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + static DiagnosticResult BuildDiagnostic() + { + return VerifyCS.Diagnostic(ContinueAsNewOrchestrationAnalyzer.DiagnosticId); + } +}