Skip to content

Fix RequestCores/ReleaseCores fallback in OOP TaskHost: throw NotImplementedException instead of logging error#13345

Merged
JanProvaznik merged 2 commits intodotnet:mainfrom
JanProvaznik:fix/requestcores-fallback
Mar 10, 2026
Merged

Fix RequestCores/ReleaseCores fallback in OOP TaskHost: throw NotImplementedException instead of logging error#13345
JanProvaznik merged 2 commits intodotnet:mainfrom
JanProvaznik:fix/requestcores-fallback

Conversation

@JanProvaznik
Copy link
Member

@JanProvaznik JanProvaznik commented Mar 9, 2026

Context

Fixes #13333

PR #13306 added RequestCores/ReleaseCores IPC callback support to OutOfProcTaskHostNode, correctly gating the new callback path behind CallbacksSupported. However, it also changed the fallback behavior (when callbacks are not supported) from hrow NotImplementedException() to LogErrorFromResource() + return 0.

This broke all callers. All known consumers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase in dotnet/runtime) use this pattern:

try {
    allowedParallelism = be9.RequestCores(N);
} catch (NotImplementedException) {
    be9 = null; // fall back to own parallelism estimate
}
// ...
finally { be9?.ReleaseCores(allowedParallelism); }

After #13306, no exception is thrown, so the \catch\ block never fires. Instead:

  1. *\LogErrorFromResource* silently pushes a \BuildErrorEventArgs\ — the build is doomed to fail and the task can't prevent it
  2. *
    eturn 0*
    violates the API contract (\RequestCores\ guarantees return ∈ [1, requestedCores])

Changes Made

**\OutOfProcTaskHostNode.cs**: Restore \ hrow NotImplementedException()\ in both \RequestCores\ and \ReleaseCores\ fallback paths (CLR2 and !CallbacksSupported). This preserves the pre-#13306 behavior until the packet version is bumped to enable callbacks.

**\TaskHostTask.cs**: Default \grantedCores\ to \1\ (not \

…rror

The RequestCores/ReleaseCores fallback paths in OutOfProcTaskHostNode
(when CallbacksSupported is false or under CLR2) were logging a build
error via LogErrorFromResource and returning 0. This caused build
failures for any task calling RequestCores in the OOP TaskHost because:

1. LogErrorFromResource logs a BuildErrorEventArgs which makes the
   build fail - but RequestCores is advisory, not fatal.
2. Returning 0 violates the IBuildEngine9.RequestCores contract which
   guarantees return value in [1, requestedCores].

All known callers (MonoAOTCompiler, EmccCompile, ILStrip, EmitBundleBase)
catch NotImplementedException from the old code path and fall back to
their own parallelism estimate. The new code no longer threw, so the
catch blocks didn't fire, and the error event silently poisoned the build.

Fix:
- Restore throw NotImplementedException in fallback paths to match
  pre-PR behavior and what all callers expect
- Fix TaskHostTask.HandleCoresRequest to default grantedCores to 1
  (not 0) when the build engine doesn't support IBuildEngine9

Fixes dotnet#13333

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 9, 2026 13:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores the historical fallback semantics for IBuildEngine9.RequestCores/ReleaseCores when OOP TaskHost callbacks are unavailable, ensuring real-world tasks that rely on catching NotImplementedException continue to work in cross-version scenarios.

Changes:

  • Reverted OOP TaskHost RequestCores/ReleaseCores fallback from “log error + return 0” back to throwing NotImplementedException.
  • Adjusted worker-side callback handling to default grantedCores to 1 for RequestCores (and 0 for ReleaseCores acknowledgments).
  • Updated/added unit tests to cover the regression scenario and the common “catch + skip release” caller pattern.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/MSBuild/OutOfProcTaskHostNode.cs Restores NotImplementedException fallback behavior for unsupported callback scenarios.
src/Build/Instance/TaskFactories/TaskHostTask.cs Ensures a contract-friendly default for RequestCores responses when handling callback packets.
src/Build.UnitTests/BackEnd/TaskHostCallback_Tests.cs Adds regression coverage for the cross-version callback-disabled behavior and the common caller pattern.
src/Build.UnitTests/BackEnd/RequestCoresWithFallbackTask.cs Introduces a test task that mirrors real consumer fallback logic to validate behavior end-to-end.

You can also share your feedback on Copilot code review. Take the survey.

@rainersigwald
Copy link
Member

fyi @lewing, this should be the durable fix for p3.

Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@JanProvaznik JanProvaznik enabled auto-merge (squash) March 10, 2026 09:41
@JanProvaznik JanProvaznik merged commit 6fa57bb into dotnet:main Mar 10, 2026
10 checks passed
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.

[NETSDKE2E][Regression] With 11.0.0-preview. 2.26154.117 sdk installed, publishing Blazor WebAssembly App failed after installing wasm-tools.

3 participants