StackGuard: probe stack every 8 levels to fix flaky StackOverflow - #5656
Merged
Merged
Conversation
The deep-recursion stack guard (used by the expression/SQL visitors) only probed TryEnsureSufficientExecutionStack every 64 levels. Between two probes up to ~64 x per-level stack is consumed with no check; at ~220 bytes/level (x64) that is ~14KB, which can exceed the fixed margin TryEnsureSufficientExecutionStack guarantees on tighter configurations (net462 / Linux, where frames are larger). When that happens the stack overflows BETWEEN probes - an uncatchable StackOverflowException that crashes the process - instead of the intended graceful thread hop. This showed up as a flaky StackOverflow in StackUseTests on CI. Probe every 8 levels (named StackProbeInterval const), bounding the between-probe stack to ~2KB, comfortably inside the guaranteed margin on every TFM. The probe is a cheap JIT intrinsic and only matters for deep recursion (shallow queries probe ~once), so the cost is negligible. Hop semantics are unchanged - it just hops slightly sooner; StackUseTests' TestExpressionVisitorHops / TestSqlVisitorHops assertions (0/1/2 hops throw nested InsufficientExecutionStackException, 10 completes) still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/azp run test-all |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed. |
MaceWindu
marked this pull request as ready for review
June 26, 2026 06:31
MaceWindu
requested review from
Shane32,
igor-tkachev,
jods4,
sdanyliv and
viceroypenguin
as code owners
June 26, 2026 06:31
Contributor
Author
📝 Release-notes draft🤖 Auto-generated user-facing summary for this PR. Toggle the boxes to control how it ships; the text is regenerated when new commits land (the maintainer confirms every change).
Full release notes (wiki)
GitHub release highlight (brief)(none) Generated from commit |
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
2 participants
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.
Problem
The deep-recursion stack guard used by the expression / SQL visitors intermittently crashes CI with an uncatchable
StackOverflowException(e.g.StackUseTestson thenet462/ Linux legs) instead of recovering via its thread-hop mechanism.Root cause
StackGuard.EnterprobedRuntimeHelpers.TryEnsureSufficientExecutionStack()only every 64 recursion levels. Between two probes up to ~64 × per-level stack is consumed with no check. Captured at the hop point on x64 there are 3 frames per level (ExpressionVisitorBase.Visit→ExpressionVisitorUtils.VisitArguments→ExpressionVisitor.VisitMethodCall), depth 4737 before the first hop ⇒ ~220 bytes/level, so 64 levels ≈ ~14 KB.TryEnsureSufficientExecutionStackonly guarantees a fixed margin. On tighter configurations (net462/ Linux, larger frames) those ~14 KB can exceed the margin, so the stack overflows between probes — a hardStackOverflowExceptionthat crashes the process — rather than the intended graceful hop.Fix
Probe every 8 levels (named
StackProbeIntervalconstant), bounding the between-probe stack to ~2 KB — comfortably inside the guaranteed margin on every TFM. The probe is a cheap JIT intrinsic and only matters for deep recursion (shallow queries probe ~once), so the cost is negligible. Hop semantics are unchanged; it just hops slightly sooner.Verification
StackUseTestshop tests pass locally on net10.0 —TestExpressionVisitorHops/TestSqlVisitorHops(0/1/2 → nestedInsufficientExecutionStackException, 10 → completes) andTestPreserveExceptionOnHop. The hardStackOverflowonly reproduces on the tight-margin legs (net10.0/x64 hops gracefully at depth 4737), so CI onnet462/ Linux is the final confirmation.🤖 Generated with Claude Code