Repository navigation
Plan analysis: parsing and analysis stop when the caller cancels, and the async path enforces the size limit (#4512) - #4560
Merged
Conversation
…e depth guard actually fires before the crash it prevents (#4512)
…a 1 MB caller thread (#4512) PlanAnalyzer.Analyze, BenefitScorer.Score, and PlanLayoutEngine.Layout all walk trees ShowPlanParser.Parse can now return up to 1,000 levels deep. Measured directly (a crash-safe child-process harness, binary-searched, never run against the test host): all three survive a depth-999 tree on both a 1 MB caller thread (the plan viewer's WPF UI thread, the smallest real caller) and a 1.5 MB caller (the Darling service's analysis pass and the MCP/web tools that front it) with well over 2x margin below their measured floors. None of the three need the parser's dedicated-thread remedy; a doc comment on each records the measured floor.
…lan XML (#4551) ScopedDescendants walked the plan tree recursively; a plan with a very deep run of non-RelOp elements inside one operator could overflow the stack before MaxParseDepth's own check ever saw it, since that guard bounds ParseRelOp/ParseStatementAndChildren, not this helper. It now uses an explicit stack for the same pre-order walk. XDocument.Parse's catch block silently returned on any exception. It now catches XmlException specifically and records a ParseError.
…or (#4551) Two pins for the review fix in the previous commit: a plan whose single operator holds 100,000 nested non-RelOp elements, parsed on a 1 MB thread, completes without crashing; and malformed plan XML sets ParsedPlan.ParseError with a readable message instead of leaving it null. A crash-safe harness that runs the parser as a separate process against the pre-fix DLL couldn't be made to distinguish a fast crash from the walk's own slowness within the time available; reverting the iterative walk locally and running the pin in-process showed a severe slowdown at this depth rather than a fast, clean crash, so that comparison isn't included as evidence. The class-level in-process 1,500-level RelOp case already documents the same crash-vs-run-time tradeoff for the sibling recursion guard.
…/4512-parse-depth
…epth # Conflicts: # Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Plans.cs # Lite/Analysis/DrillDownCollector.Plans.cs
…nd missing indexes exist without analysis (#4512)
… plan-sync/4514-sub-plans # Conflicts: # PerformanceMonitor.PlanAnalysis/BenefitScorer.cs # PerformanceMonitor.Ui/PlanViewerControl.xaml.cs
PlanStatements.PushAll's parameter was typed IReadOnlyList<PlanStatement>, but every caller passes a PlanBatch or PlanStatement's concrete List<PlanStatement> Statements property, tripping CA1859. Declare the concrete type. Also updates PlanSync4512ParseErrorSurfacingTests, which still searched for the pre-#4514 plan.Batches read; the drill-down collectors now walk PlanStatements.EnumerateAll(plan) instead.
# Conflicts: # Darling/Darling.Tests/PlanSync4512ParseErrorSurfacingTests.cs # PerformanceMonitor.PlanAnalysis/PlanModels.cs # PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs # PerformanceMonitor.Ui/PlanViewerControl.xaml.cs
… the async path enforces the size limit (#4512)
…lan-sync/4512-cancellation
…wns, and Lite's MCP plan tools (#4512) - PlanAdvisoryAggregator.Extract/Summarize take a CancellationToken and pass it to ShowPlanParser.Parse and PlanAnalysisPipeline.Run, checking it once per plan in the loop. - PgFactCollector.QueryPerf.cs and DuckDbFactCollector.QueryPerf.cs pass context.CancellationToken into Summarize. - PgDrillDownCollector.Plans.cs and DrillDownCollector.Plans.cs pass context.CancellationToken into Parse/Run/Extract, and their bare catch clauses around the parse step now let OperationCanceledException through instead of swallowing it as a parse failure. - Lite/Mcp/McpPlanTools.cs: the four plan tools (analyze_query_plan, analyze_procedure_plan, analyze_query_store_plan, analyze_plan_xml) take a CancellationToken, mirroring Darling's MCP plan tools, and pass it into BuildAnalysisResult; their catch clauses exclude OperationCanceledException. - Fixed the erikdarlingdata/PerformanceStudio issue references in PlanSync4527Tests.cs's doc comment.
…ation # Conflicts: # PerformanceMonitor.PlanAnalysis/BenefitScorer.cs # PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs # PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs
…d cancellable siblings Restores Extract(IEnumerable<string>) and Summarize(IEnumerable<string>) to their pre-cancellation signatures so the deprecated dashboard test project (no longer editable) keeps compiling clean. The cancellation-aware behavior moves to new methods, ExtractCancellable and SummarizeCancellable, which the four live production callers (Darling and Lite fact collectors and drill-down enrichers) now call directly with their CancellationToken. The token-free overloads delegate with CancellationToken.None. Also splits three statements that had been joined onto one line in PlanAnalyzer.cs and ShowPlanParser.cs back to one statement per line.
The mid-walk cancellation test cancelled the token on a timer racing against XML parsing, so it could pass without actually cancelling during the tree walk. ShowPlanParser now exposes a test-only hook invoked after each statement the walk finishes; the test cancels the token from inside that hook and checks that the walk saw no statement past the cancellation point. Also renamed the abandon-classifier pin to match what it actually covers.
…ad parse ParseAsync used to walk the parsed tree on its own LoadAsync continuation instead of the dedicated 32 MB thread Parse runs on, so a deeply nested plan could overflow the continuation thread's stack before the depth guard ever fired. ParseAsync now starts the same dedicated thread Parse uses and completes a TaskCompletionSource from it, so it never walks the tree on the caller's own stack. Parse and ParseAsync share one xml.Length > MaxParseCharacters check and the same refusal message. The dedicated-thread lambda now has a catch-all after the cancellation catch, so a non-XmlException failure (for example an out-of-memory error building the XDocument) sets ParseError instead of going unhandled on a non-pool thread. The captured OperationCanceledException is now routed through TaskCompletionSource.TrySetException, which preserves its original stack for both Parse's synchronous unwrap and ParseAsync's await, rather than losing it to a freshly constructed exception.
… classes can't pollute it
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 06:57
This was referenced Sep 28, 2026
A deeply nested plan overflows the stack in ShowPlanParser and ends the process that parses it
#4512
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4512
Part of #4511
#4557 (#4514) is already merged; this branch contains it.
Why
A plan-analysis pass that keeps parsing and analyzing after the caller has already given up wastes work on a plan nobody is waiting for. The MCP plan tools, the Darling and Lite analysis passes, and the drill-down collectors all have a cancellation token in scope; the parser, the analyzer, the benefit scorer and the pipeline that chains them together did not check it, so a cancelled request or a shutting-down analysis pass ran the full parse/analyze/score sequence to completion regardless.
What changes
ShowPlanParser.Parseand the newParseAsynctake aCancellationToken. A cancellation surfaces asOperationCanceledException, never asParsedPlan.ParseError(callers treat aParseErroras "this plan is bad", not "the caller gave up").ParseAsyncruns the same dedicated 32 MB-stack thread parse asParse(the thread completes the returned task), so a deeply nested plan can't overflow the caller's stack on either path, and both share one up-front size check with the same message. The production callers use the synchronousParse.OperationCanceledException, with its stack.PlanAnalyzer.AnalyzeandBenefitScorer.Scorecheck the token per statement and down their node-tree walks.PlanAnalysisPipeline.Run(and the newRunAsync) check the token before, between and after the analyze and score stages, so a cancellation stops the pipeline before the next stage instead of finishing.McpPlanAnalysisFormatter.BuildAnalysisResulttakes and forwards the token.analyze_query_plan,analyze_procedure_plan,analyze_query_store_plan,analyze_plan_xml),PlanAdvisoryAggregator.ExtractCancellable/SummarizeCancellable(new names that carry the token; the token-freeExtract/Summarizekeep their signatures), and both drill-down collectors (Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Plans.cs,Lite/Analysis/DrillDownCollector.Plans.cs). Each catch clause that used to swallow every exception is narrowed so a cancellation propagates instead of being reported as a parse or analysis failure (the drill-downs already had abandon-classifier filters —AnalysisShutdown.IsExpectedAbandon/AnalysisAbandon.IsExpected— which recognize a cancelled token'sOperationCanceledExceptionas an expected abandonment, not a fault).PlanSession,PlanOperations,InMemoryPlanCatalog,PlanXmlPreflight, the CLI/text formatters, and the shared resource-budget machinery.Test plan
New
Darling/Darling.Tests/PlanSync4512CancellationTests.cs, 8 pins:PlanSync4512AsyncParseTests: a 500-level plan throughParseAsync, awaited from a real 1 MB thread, returns normally (501 operators, noParseError). On the previous code this crashes the process with a stack overflow, so its RED was shown locally in a separate process (exit 134, "Stack overflow."; exit 0 with the fix; 134 again withParseAsyncwalking the tree itself). The async size refusal gives the same message as the sync path.ParseandParseAsyncthrowsOperationCanceledException, not aParseError.AsyncLocal, so parallel tests can't interfere) cancels after statement 3 of 50; the test asserts theOperationCanceledExceptionand that parsing stopped there. Removing the in-walk checks turns it RED (an assertion, no crash).PlanAnalysisPipeline.Runwith a cancelled token throws before either the analyzer or the scorer adds anything (ParsedPlan.AllWarningsstays empty).ParseAsyncon XML over the size limit setsParseErrorwith no exception and no cancellation — a size refusal is a parse failure, not a cancellation.Parse(xml),Run(plan)) give the same result as the token overloads called withCancellationToken.None, on a normal plan.AbandonClassifier_TreatsCancelledParseAsExpected):AnalysisShutdown.IsExpectedAbandonreturns true for anOperationCanceledExceptionagainst a cancelled token — the half of the drill-down's catch-filter guard bothPgDrillDownCollector.Plans.csandLite/Analysis/DrillDownCollector.Plans.csshare, which is why a cancellation fromShowPlanParser.Parseinside a drill-down propagates instead of being caught and reported as a bad plan.Mutation: removed
Parse's dedicated-thread catch that captures anOperationCanceledExceptionand rethrows it on the caller's thread. On the pre-fix behavior this is fatal to the whole test process — the parser's own comment says an unhandled exception on that non-pool thread has nowhere to go — so the RED here is the pin-runner process itself terminating instead of printing a[FAIL]line, not a failing assertion. Reverted; the 8 pins are green again after rebuilding.Build (Release,
-p:EnableWindowsTargeting=true):Darling.Tests,Lite,Lite.Tests,PerformanceMonitor.Ui,Darling/PerformanceMonitor.Darling.ServiceandDashboard.Tests— all 0 Warning(s), 0 Error(s).grep -E "warning (CA|CS|IDE)[0-9]+"across all four build logs: empty.Run (in-process on macOS,
Microsoft.WindowsDesktop.Appstripped fromDarling.Tests.runtimeconfig.json), the plan-analysis classes plus the new ones:Total: 274, Errors: 0, Failed: 0, Skipped: 2, Not Run: 0(after the review fixes; 362 on the first cut)The 2 skipped are the live-Postgres classes (
DarlingMcpPlanToolsLivePostgresTests, and the live fixture insideDarlingAnalysisPipelineTests) — they need a live store and run in CI.Lite.Tests builds (0/0) but cannot run here (
net10.0-windows, discovery dies onWindowsBaseon macOS); CI decides it.CHANGELOG
SECTION: Fixed
ENTRY: - Plan analysis stops when its caller cancels ([#4560]) - The MCP plan tools stop parsing and analysing a plan when the client cancels the request, and the Darling and Lite analysis passes stop on shutdown instead of finishing a large plan first.
REF: [#4560]: #4560