Skip to content

Parse deep plans on a large-stack thread (#589) - #591

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/589-deep-plan-stack
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/589-deep-plan-stack

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #589.

A plan with about 450 or more nested operators ended the process with a stack overflow. The parser walked the plan tree on the caller's thread. The app's UI thread and the CLI's main thread have 1 MB stacks. The walk ran out of stack at about 450 levels, so the depth limit of 1,000 (MaxParseDepth) never refused the plan. A .NET process cannot catch a stack overflow, so the app closed with no message.

The changes:

  • ShowPlanParser.Parse and ParseAsync walk the tree on a new thread with a 32 MB stack, and wait for it. The public signature does not change. Now the depth limit stops a deep plan: 1,000 levels parse, and 1,001 levels give a parse error. This is the same fix as Plan analysis: a deeply nested plan no longer crashes the process that parses it (#4512) PerformanceMonitor#4551.
  • The web viewer runs on Blazor WebAssembly, which cannot start a thread. It still walks the tree on the calling thread, so its limit depends on the browser's stack.
  • ScopedDescendants searches the elements inside one operator. It used a recursive iterator, and no depth guard counts those elements, so deep nesting there overflowed even the 32 MB stack. It now uses a loop with its own stack.
  • The steps after the parse also walk the tree on the caller's thread. A measurement of each step at 1,000 levels, in a separate process, found two steps near the limit. ResultMapper needed more than 768 KB of a 1 MB stack, and HtmlExporter needed 512 KB. ResultMapper now uses a loop. HtmlExporter writes each operator's line in a separate method, so its recursive method has a small frame. Now each step needs 384 KB or less.
  • JSON output has a depth limit (AnalysisJson.MaxDepth, 1,024). Each operator level uses two JSON levels, so the limit allows about 500 operator levels. Before this change, the parser crashed before a plan got that deep. Now a plan with more levels reaches the limit, and the serializer's message blames "a possible object cycle". A new AnalysisJson.TooDeepMessage gives the real cause. The CLI, Robot Advice, the MCP tools and the share button in the web viewer show it.
  • The CLI now stops with the parse error when a plan does not parse. Before, the offline path printed "Could not parse any statements from the plan XML". The live path and query-store wrote an empty or partial analysis and reported OK.
  • The MCP tool get_query_store_top reports a plan that does not parse in load_error. Before, it listed the plan as loaded, with no warnings.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

The web viewer (PlanViewer.Web) changes too, in its share error message.

How was this tested?

  • New DeepPlanStackTests, 11 cases. The depth cases run on a real thread with a small stack:
    • A plan at the depth limit parses on a 1 MB thread, with every level.
    • One level more gives a parse error on a 1 MB thread.
    • ParseAsync parses a plan at the depth limit on a 1 MB thread.
    • The search inside one operator handles 20,000 levels on a 256 KB thread.
    • Analysis, layout, mapping, text output and HTML output all run on a 1 MB thread, for a plan at the depth limit. JSON output stops with a JsonException.
    • The result mapper maps a plan at the depth limit on a 256 KB thread.
    • The HTML exporter writes a plan at the depth limit on a 384 KB thread. Measured at 1,000 levels, it now needs less than 320 KB in Debug and 256 KB in Release. The old exporter needed more than 384 KB in Release and more than 640 KB in Debug.
    • The search inside one operator returns its matches in document order and skips nested operators, as the old recursive version did.
    • The CLI's parse-failure message, and the too-deep messages from the CLI and the MCP tools.
    • Warning text from the parser still uses the caller's culture.
  • Against the unfixed code, each of the six original depth cases crashed the test host with a stack overflow. The culture case passed there, because the old parse ran on the caller's thread. With the old HTML exporter swapped back in, the 384 KB exporter case crashed the test host too.
  • The built planview.exe analyzed plans 150, 999, 1,000 and 1,001 levels deep:
    • Before: 999 levels and more crashed with a stack overflow.
    • After: -o text succeeds up to 1,000 levels. -o json exits 1 with the new message at 999 and 1,000 levels. At 1,001 levels, both formats exit 1 with the parse error.
  • The desktop app opened a 1,000-level plan. The plan rendered, Human Advice opened, and Robot Advice showed the new message. A 1,001-level plan showed "Couldn't Load Plan" with the parse error.
  • A first version returned a Task from the parse thread, so the caller resumed on the thread pool. Two MCP tests then timed out in the full suite. The parse now waits for its thread with Join, in the same way that the walk ran on the caller's thread before.
  • Full suite on Windows: 955 total, 954 passed, 0 failed, 1 skipped. The Release build has 0 warnings.

Review round 1

  • Fixed: get_query_store_top did not check the parse error (above).
  • Fixed: the share button blamed every JsonException on depth, but a reply from the server that is not JSON throws one too. Now only the serializer's exception gets the depth message.
  • Fixed: the tests above for the HTML exporter, the search order and the error messages.
  • Fixed: HtmlExporter.WriteOperatorNode passed down a parameter that it never used.
  • Not changed: the desktop app's UI thread is STA. So its wait for the parse thread handles COM calls and sent window messages, as every blocking wait on that thread does. The parse thread never calls the UI thread, so the wait cannot deadlock.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build, --no-incremental)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

erikdarlingdata and others added 2 commits September 28, 2026 12:43
A plan about 450 operators deep overflowed the 1 MB stack of the UI thread
and the CLI's main thread before MaxParseDepth (1,000) could refuse it, and
the process ended. Parse and ParseAsync now walk the tree on a 32 MB thread
and join it, so the depth limit stops a deep plan. Same fix as
PerformanceMonitor#4551. The web viewer (WebAssembly) still parses inline.

ScopedDescendants and ResultMapper.MapNode use loops with their own stacks,
and HtmlExporter keeps its per-node text out of the recursive method, so
every step after the parse fits a 1 MB thread at the depth limit.

JSON output, limited to about 500 operator levels, now says the plan is too
deeply nested instead of "a possible object cycle" (CLI, Robot Advice, MCP,
web share). The CLI stops with the parse error when a plan does not parse.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
- get_query_store_top reports a plan the parser refuses in load_error.
  It returned the plan as loaded, with no warnings.
- The web viewer's share button maps only the serializer's JsonException
  to TooDeepMessage. A reply from the server that isn't JSON gets the
  usual "Share failed" message again.
- HtmlExporter.WriteOperatorNode drops a parameter it never used.
- New tests: the HTML exporter at 1,000 levels on a 384 KB thread (the
  old exporter overflows there), the search's document order and RelOp
  skipping, the CLI's parse-failure message, and the too-deep messages
  from the CLI and the MCP tools.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 17:15
@erikdarlingdata
erikdarlingdata merged commit 0283ed0 into dev Sep 28, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/589-deep-plan-stack branch September 28, 2026 17:15
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the parse-thread rewrite and the traversal changes. No correctness issues found.

Specifically verified:

  • RunOnParseThread/Parse/ParseAsync: exceptions (including OperationCanceledException) are captured via ExceptionDispatchInfo on the worker thread and rethrown correctly on the joining thread; the OperatingSystem.IsBrowser() early-return pattern is the analyzer-recognized shape, so no new CA1416 warnings expected. ParseDocument's own internal try/catch means the WASM path (no outer try/catch around it) still can't leak an unhandled exception.
  • ScopedDescendants: traced the stack-based rewrite by hand against the old recursive iterator — pushing Elements().Reverse() and popping preserves the original pre-order/document-order traversal, including the "skip nested RelOp" behavior. Matches the new order/skip test.
  • ResultMapper.MapNode: the iterative version still appends each node's children in original document order (only the visiting order changes, not what's written to result.Children), so TreeDepth/ordering is preserved.
  • HtmlExporter: WriteOperatorLine split out with [MethodImpl(NoInlining)] correctly keeps the interpolated-string-heavy code out of the recursive frame; the <div class="op-node">/</div> open/close is still balanced across the two methods.
  • All CLI/MCP call sites that now check ParseFailure/catch the too-deep JsonException are wrapped by an existing outer per-item catch (Exception) in their loops, so a bad plan reports an error/load_error without aborting a batch run or leaving a half-registered session (McpQueryStoreTools skips CaptureSession/AdmitSnapshot on parse failure, as intended).

No untrusted-input or T-SQL concerns — this PR doesn't touch SQL generation. No Directory.Build.props/SSMS version-file or PlanViewer.Web linked-file convention concerns — this PR doesn't touch those files. Test coverage for the new behavior (deep-plan parsing on constrained stacks, per-step stack budgets, error-message plumbing) looks thorough and the PR description documents empirical measurements backing the stack-size choices.

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.

1 participant