Skip to content

fix(toolchain): narrow the sweep, and stop spawning cl.exe to ask a yes/no - #444

Merged
Sunrisepeak merged 1 commit into
mainfrom
fix/self-review-followups
Aug 16, 2026
Merged

fix(toolchain): narrow the sweep, and stop spawning cl.exe to ask a yes/no#444
Sunrisepeak merged 1 commit into
mainfrom
fix/self-review-followups

Conversation

@Sunrisepeak

Copy link
Copy Markdown
Member

Both found by reviewing the v2026.8.16.3 diff after tagging it.

1. The sweep could delete another version mid-install 🔴

sweep_parked_payloads 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 exactly 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:

xim-x-msvc/
  14.44.35207/        ← this command's skeleton, safe to sweep
  14.52.36629/        ← another process, 200ms into extraction, no files yet

Two mcpp processes against one MCPP_HOME is 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 full installation_at(), which runs cl.exe and 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

TheSweepDoesNotTouchAnotherVersionThatHasNoFilesYet was run against the family-wide sweep and fails there. Full suite 84/84.

Shipped in v2026.8.16.3; these go into the next release.

…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.
@Sunrisepeak
Sunrisepeak merged commit 5c3abfb into main Aug 16, 2026
19 checks passed
@Sunrisepeak
Sunrisepeak deleted the fix/self-review-followups branch August 16, 2026 14:38
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.

2 participants