-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix for Build Requests with Different Flags Not Correctly Scheduled #11862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for Build Requests with Different Flags Not Correctly Scheduled #11862
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the scheduler to correctly handle duplicate build requests with the same configuration ID but different build flags by rescheduling the second request when the cache key differs. It also adjusts the reporting logic to mark completed requests earlier and enhances unit tests accordingly.
- In
Scheduler.ReportResult
, add logic to reschedule on cache miss and move the completion call into the cache-hit branch. - Fix a typo in a log comment and remove stale all-caps comment.
- Enable previously skipped scheduler tests, introduce a
DefaultConfigId
constant, broadenCreateBuildRequest
overloads, and add two new tests covering duplicate-request scenarios.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Build/BackEnd/Components/Scheduler/Scheduler.cs | Handle cache misses by rescheduling duplicate requests; fix comment |
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs | Un-skip many tests, introduce DefaultConfigId , extend helpers, add new duplicate-request tests |
Comments suppressed due to low confidence (3)
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs:872
- [nitpick] The new test names use an underscore and the prefix
ReportResultTest_
, which differs from the existingTestXxx
pattern. Consider renaming to match the established naming convention.
public void ReportResultTest_NoCacheHitForDupes()
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs:878
- The
responses
variable is assigned but never used or asserted against. Either remove it or add assertions to validate the blocked request behavior.
List<ScheduleResponse> responses = [.. _scheduler.ReportRequestBlocked(2, new BuildRequestBlocker(-1, Array.Empty<string>(), [duplicateRequest]))];
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs:878
- Using the C# 12 collection expression here produces an array, not a List, so this assignment will fail. Consider calling
.ToList()
on the enumerable or usingnew List<ScheduleResponse>(...)
.
List<ScheduleResponse> responses = [.. _scheduler.ReportRequestBlocked(2, new BuildRequestBlocker(-1, Array.Empty<string>(), [duplicateRequest]))];
…com/YuliiaKovalova/msbuild into dev/ykovalova/fix_hole_in_scheduler
/backport to vs17.14 |
Started backporting to vs17.14: https://github.com/dotnet/msbuild/actions/runs/16378306218 |
@rainersigwald backporting to "vs17.14" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: update the case in scheduler of reporting unscheduled overlapping results
Applying: adjust the handling of cache miss in scheduler
Using index info to reconstruct a base tree...
M .editorconfig
M src/Build.UnitTests/BackEnd/Scheduler_Tests.cs
M src/Build/BackEnd/BuildManager/BuildManager.cs
M src/Build/BackEnd/Components/Scheduler/Scheduler.cs
Falling back to patching base and 3-way merge...
Auto-merging .editorconfig
CONFLICT (content): Merge conflict in .editorconfig
Auto-merging src/Build.UnitTests/BackEnd/Scheduler_Tests.cs
Auto-merging src/Build/BackEnd/BuildManager/BuildManager.cs
Auto-merging src/Build/BackEnd/Components/Scheduler/Scheduler.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 adjust the handling of cache miss in scheduler
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Fixes #11701
Context
When two build requests with the same configuration ID and targets arrive in the scheduler, the second one is expected to be satisfied from the cache with results from the first request. However, the current implementation doesn't properly account for differing
BuildRequestDataFlags
between these requests, leading to potential deadlocks.Problem
If the first request builds successfully but isn't cached (due to exceptions or skipped targets), the second request can get "orphaned" in the scheduler - it's not executed and doesn't send a proper completion callback. This causes Visual Studio to hang indefinitely waiting for a response that never comes.
Changes Made
Testing