Surface critical graph build exceptions instead of hanging#14461
Surface critical graph build exceptions instead of hanging#14461AlesProkop wants to merge 1 commit into
Conversation
Route critical graph scheduler exceptions through the build manager's thread-exception path so submissions complete and the fatal error is surfaced instead of hanging. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62a2178b-372e-479c-a717-ed6e7af95980
There was a problem hiding this comment.
Pull request overview
This PR fixes a static graph build hang caused by critical exceptions escaping the detached graph scheduler worker, leaving graph submissions incomplete and synchronous callers blocked indefinitely. The change routes those critical exceptions through BuildManager’s thread-exception path so submissions complete and the fatal exception is preserved for EndBuild/Build(...) to surface.
Changes:
- Route all graph-scheduler worker exceptions (including critical) through a new
HandleGraphSubmissionExceptionhelper, which forwards critical exceptions toOnThreadException. - Ensure exceptions thrown while starting the detached graph worker are handled consistently with exceptions thrown inside it.
- Add a regression unit test covering a critical exception originating from a project cache plugin during graph build execution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Build/BackEnd/BuildManager/BuildManager.cs | Ensures critical exceptions from the detached graph scheduler complete submissions via OnThreadException instead of faulting the worker and hanging callers. |
| src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs | Adds a regression test asserting graph builds complete and surface a critical exception rather than hanging. |
There was a problem hiding this comment.
✅ 24/24 dimensions clean — no findings.
Summary: The fix correctly ensures graph build submissions complete on critical exceptions by routing them through OnThreadException, which safely completes all active submissions under _syncLock. Previously, the when (!ExceptionHandling.IsCriticalException(ex)) filter caused critical exceptions to bypass submission completion, leaving callers hanging indefinitely.
Key observations:
- Concurrency: Safe —
OnThreadExceptionacquires_syncLockand checksIsCompletedbefore completing submissions. - Correctness: Non-critical exception path unchanged. Critical exceptions now get proper handling.
- Compatibility: No breaking change — fixes a hang bug.
- Test quality: Good timeout-based assertion verifying submission completes.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
awmgmcpg
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
Generated by Expert Code Review (on open) for #14461 · 58.3 AIC · ⌖ 4.98 AIC · ⊞ 5K
Related to #14325. Split from #14395 so the generic graph-submission failure handling can be reviewed independently from the graph-edge fix.
Context
Graph scheduling runs on a detached worker task. Its exception handler only caught non-critical exceptions, so a critical exception faulted the worker without completing the active graph submission. Synchronous callers then waited indefinitely instead of receiving the fatal error.
Changes Made
OnThreadException.EndBuildto rethrow.HandleSubmissionExceptionbehavior for non-critical graph failures.Testing
build.cmd -verbosity quiet— 0 warnings and 0 errors.CriticalExceptionFromGraphBuildCompletesSubmissiononnet10.0— 1 passed, 0 failed.