Fix V2 dependency resolution thread-affinity failure#2008
Open
Gijsreyn wants to merge 2 commits into
Open
Conversation
Gijsreyn
requested review from
SydneyhSmith,
adityapatwardhan,
alerickson,
anamnavi and
shammu1
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes dependency discovery for V2 server protocol more deterministic/testable by introducing a test hook that controls when dependency lookups run in parallel, and by refactoring dependency-finding to aggregate output messages from concurrent execution.
Changes:
- Added an
InternalHooksfield to override the parallelization threshold for dependency queries. - Updated dependency discovery to pass shared concurrent message queues through recursive calls and flush once.
- Updated the V2 server protocol test to force parallel dependency resolution and restore the hook afterward.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/FindPSResourceTests/FindPSResourceV2Server.Tests.ps1 | Sets/resets a test hook to force parallel dependency resolution during the test. |
| src/code/InternalHooks.cs | Adds a new internal hook (FindDependencyPackagesParallelThreshold) with default -1. |
| src/code/FindHelper.cs | Refactors dependency resolution to propagate concurrent message queues and uses the new threshold hook for parallelization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1171
to
+1178
| ConcurrentQueue<ErrorRecord> errorMsgs = new ConcurrentQueue<ErrorRecord>(); | ||
| ConcurrentQueue<string> verboseMsgs = new ConcurrentQueue<string>(); | ||
| ConcurrentQueue<string> debugMsgs = new ConcurrentQueue<string>(); | ||
| ConcurrentQueue<string> warningMsgs = new ConcurrentQueue<string>(); | ||
|
|
||
| _cmdletPassedIn.WriteDebug($"In FindHelper::FindDependencyPackages() - {currentPkg.Name}"); | ||
| FindDependencyPackagesHelper(currentServer, currentResponseUtil, currentPkg, repository, errorMsgs, warningMsgs, debugMsgs, verboseMsgs); | ||
| Utils.WriteOutConcurrentQueue(_cmdletPassedIn, errorMsgs, warningMsgs, debugMsgs, verboseMsgs); |
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
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.
PR Summary
Fixes a regression in V2 dependency resolution where
Install-PSResourceandFind-PSResource -IncludeDependenciescould call PowerShell cmdlet output APIs from worker threads while resolving large dependency graphs.The fix changes recursive dependency discovery so parallel workers only enqueue errors and stream messages. The outer dependency discovery entry point now flushes those queues once it is back on the cmdlet pipeline thread.
Small test added to cover regression so no full
Azdependency graph needs to be resolved.PR Context
Fixes #2006.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title and remove the prefix when the PR is ready.