[rush-lib] Fix weighted concurrency budget being capped by operation count#5646
[rush-lib] Fix weighted concurrency budget being capped by operation count#5646itsnothuy wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…count test(rush-lib): expand weighted concurrency edge case tests test(rush-lib): expand weighted concurrency edge case tests refactor(OperationExecutionManager.test): align comment and naming style with codebase conventions - Replace // ─── section banners and // Test N: numbered headers with brief inline prose comments matching the existing test file style - Replace // WHAT: / // SCENARIO: / // DETERMINISM: structured blocks with concise inline comments - Rename createWeightedOp → createWeightedOperation for consistency with other helper names (createExecutionManager, etc.) - Rename counters.active / counters.peak → counters.concurrentCount / counters.peakConcurrency for self-documenting field names - Extract new AbortController() calls into named abortController variables matching the pattern used throughout the rest of the test file - No logic or assertion changes; all 18 tests continue to pass chore: add rush change file for @microsoft/rush-lib weighted concurrency fix refactor: reduce test comment density to match codebase patterns
There was a problem hiding this comment.
Pull request overview
Fixes a weighted scheduling bug in rush-lib where the concurrency unit budget was being incorrectly reduced to the number of operations, preventing expected parallelism when Operation.weight > 1.
Changes:
- Pass the full
parallelismvalue as the weighted scheduler’s concurrency unit budget (no longer capped by operation count). - Improve the startup log message to cap the displayed “simultaneous processes” count by the number of operations (UX clarity).
- Add a comprehensive Jest test suite covering weighted concurrency behaviors (regression, clamping, oversubscription, mixed/zero weights, and display output).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts | Fixes the weighted scheduler’s concurrency budget and separates display calculation from scheduling. |
| libraries/rush-lib/src/logic/operations/test/OperationExecutionManager.test.ts | Adds deterministic test coverage for weighted concurrency scenarios and logging output. |
| common/changes/@microsoft/rush-lib/fix_weighted-concurrency-cap_2026-02-19-07-57.json | Patch change file documenting the fix for release notes/versioning. |
@microsoft-github-policy-service agree |
|
@dmichon-msft - mind reviewing this? |
Any updates on PR status? |
|
I have deleted this text from the PR description:
@itsnothuy himself says this PR is fixing a separate issue:
We discussed #5607 at Rush Hour and accepted Proposal 1 but rejected Proposal 2 (because it's better handled within a Rush plugin or rig). So I believe @LPegasus is still planning to implement Proposal 1, which will be separate from this PR. |
Summary
Resolves a weighted scheduling bug where the concurrency unit budget was incorrectly capped by the number of operations.
Background
When Rush uses weighted scheduling (
Operation.weight> 1), theconcurrencyparameter inAsync.forEachAsyncrepresents a unit budget, not a task count. However,OperationExecutionManagerwas passingMath.min(totalOperations, parallelism), which shrinks the budget whentotalOperations < parallelism.Example: 4 operations with weight=4, parallelism=10:
concurrency = min(4, 10) = 4→ only 1 operation fits → sequential executionconcurrency = 10→ 2 operations fit (4+4=8 ≤ 10) → parallel executionThis was reported by @LPegasus in issue #5607.
Changes
1. Fix:
OperationExecutionManager.tsMath.min(totalOperations, parallelism)to avoid confusing messages like "max 10 processes" when only 4 operations exist (a UX improvement over the old code which showed rawthis._parallelism)this._parallelismdirectly as the full unit budget (the core fix)2. Tests:
OperationExecutionManager.test.tsAdded 9 new tests in a "Weighted concurrency" describe block:
All tests are deterministic (no timing dependencies) and use
Async.sleepAsync(0)to yield control to the scheduler.Impact
Testing
rush build --to rush-libsucceedsScope
This PR addresses only the scheduling bug. The
"weight": "NN%"feature (Proposal 1 in #5607) is intentionally excluded — it requires schema changes and maintainer design approval, so it should be a separate PR.Checklist
rush build --to rush-liblocally (passes)rush changeand committed change file