fix(toolchain): narrow the sweep, and stop spawning cl.exe to ask a yes/no - #444
Merged
Conversation
…es/no Both found by reviewing the 2026.8.16.3 diff after tagging it. **The sweep could delete another version mid-install.** It walked the whole family directory and removed any version directory containing no regular files. But an install populates a version directory over time -- which is why package_fetcher tracks completeness with a marker file rather than by existence -- so a DIFFERENT version being extracted right now is briefly indistinguishable from a leftover skeleton. Two mcpp processes against one MCPP_HOME is ordinary on a shared or self-hosted runner, and this feature exists for long-lived installs on exactly those machines. `.trash-*` is still swept family-wide: that name is only ever written by this code, so deleting one is safe whoever else is running. The file-less-skeleton rule now applies to the single version the command names. **`msvc_available_here()` ran a compiler to answer a yes/no.** It wanted to know whether a usable toolset exists and called the full `installation_at()`, which spawns cl.exe for its banner to identify the version. That question is asked on every build at the MSVC-ABI gate, so a machine with several installed toolsets paid several subprocess spawns per build -- and those are precisely the machines this predicate was added for. `installation_at(..., identifyVersion = false)` skips the banner. The layout still has exactly ONE implementation; this is a parameter, not a second copy of the path arithmetic. The new test was run against the family-wide sweep and fails there.
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.
Both found by reviewing the
v2026.8.16.3diff after tagging it.1. The sweep could delete another version mid-install 🔴
sweep_parked_payloadswalked the whole family directory and removed any version directory containing no regular files.But an install populates a version directory over time — which is exactly why
package_fetchertracks completeness with a marker file rather than by existence. So a different version being extracted right now is briefly indistinguishable from a leftover skeleton:Two mcpp processes against one
MCPP_HOMEis ordinary on a shared or self-hosted runner — and this whole park/sweep feature exists for long-lived installs on precisely those machines.Fix:
.trash-*is still swept family-wide (that name is only ever written by this code, so deleting one is safe whoever else is running). The file-less-skeleton rule now applies to the single version the command names.2. A yes/no question was spawning a compiler
msvc_available_here()wants to know whether a usable toolset exists. It called the fullinstallation_at(), which runscl.exeand parses its banner to identify the version — information it then discards.That question is asked on every build at the MSVC-ABI gate, so a machine with several installed toolsets paid several subprocess spawns per build. Those are the same machines the predicate was added for (managed toolsets, no Visual Studio).
Fix:
installation_at(..., identifyVersion = false)skips the banner. The layout still has exactly one implementation — this is a parameter, not a second copy of the path arithmetic (which is the sin the review's §1 is about).Testing
TheSweepDoesNotTouchAnotherVersionThatHasNoFilesYetwas run against the family-wide sweep and fails there. Full suite 84/84.