Skip to content

Extract toolchain archives into a staging folder and gate on the executable - #4940

Merged
pepone merged 2 commits into
icerpc:mainfrom
pepone:fix/build-atomic-extract
Sep 11, 2026
Merged

Extract toolchain archives into a staging folder and gate on the executable#4940
pepone merged 2 commits into
icerpc:mainfrom
pepone:fix/build-atomic-extract

Conversation

@pepone

@pepone pepone commented Sep 11, 2026

Copy link
Copy Markdown
Member

ExtractTask unzipped the slicec and protoc archives straight into their final obj/ folder, and the extraction step
was skipped whenever that folder existed. A build killed mid-extraction (hard kill, disk full, power loss) therefore
left a partial folder that was never re-extracted: the following Copy failed with MSB3030, or copied a truncated
executable, until the developer removed obj/ by hand.

This PR makes the extraction all-or-nothing and self-healing:

  • ExtractTask extracts into a sibling <folder>.extracting staging folder and moves it into place once every archive
    is extracted. The staging folder is removed on failure and replaced on the next run.
  • The slicec and protoc extraction steps are now skipped only when the extracted executable exists, so a folder left
    without its executable is re-extracted on the next build instead of failing the following Copy.

Verified locally on macOS: a zip with a corrupt entry no longer leaves a destination folder behind, Unix executable
permissions are still preserved, a slicec or protoc folder whose executable was deleted heals on rebuild, and the
full solution builds. The task only uses netstandard 2.0 APIs, which is what RoslynCodeTaskFactory compiles against.

Fixes #4800

What's Changed entry

None — build-system change to how this repository downloads its own toolchain; no user-facing impact.

…utable

ExtractTask now extracts into a sibling staging folder and moves it into place once every archive is extracted, so
a build killed mid-extraction leaves no destination folder behind. The slicec and protoc extraction steps are now
skipped only when the extracted executable exists, so a folder left without its executable is re-extracted on the
next build instead of failing the following Copy with MSB3030.

Fixes icerpc#4800
Copilot AI lite review requested due to automatic review settings September 11, 2026 08:50
@pepone pepone added this to the 0.6.1 milestone Sep 11, 2026
@pepone pepone added build-system ai-audit AI-generated audit finding — needs human triage labels Sep 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Orphaned .extracting folders can remain after an interrupted extraction.

Pull request overview

This PR hardens toolchain archive extraction against interrupted builds.

Changes:

  • Extracts archives into a staging directory before replacement.
  • Re-extracts when slicec or protoc is missing.
File summaries
File Summary
src/IceRpc.Slice.Tools/IceRpc.Slice.Tools.csproj Gates extraction on slicec and needs staging cleanup handling.
src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.csproj Gates extraction on protoc and needs staging cleanup handling.
build/ExtractTask.cs Implements staged extraction and cleanup.
Review details

Suppressed comments (2)

src/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.csproj:97

  • If the process is hard-killed after extraction has completed into <destination>.extracting but before Directory.Move, the previous destination can still contain protoc. On the next build this condition is false, so ExtractTask never runs its staging cleanup and the complete or partial duplicate .extracting folder remains indefinitely. Include the staging path in the condition or run a cleanup target independently of the executable check.
      Condition="!Exists('$(MSBuildThisFileDirectory)obj/protoc-$(ProtobufVersion)-osx-aarch_64/bin/protoc')" />

src/IceRpc.Slice.Tools/IceRpc.Slice.Tools.csproj:110

  • If the process is hard-killed after extraction has completed into <destination>.extracting but before Directory.Move, the previous destination can still contain slicec. On the next build this condition is false, so ExtractTask never runs its staging cleanup and the complete or partial duplicate .extracting folder remains indefinitely. Include the staging path in the condition or run a cleanup target independently of the executable check.
      Condition="!Exists('$(MSBuildThisFileDirectory)obj/slicec-$(SlicecVersion)-%(SlicecPlatform.Identity)/%(SlicecPlatform.Exe)')" />
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@pepone

pepone commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

🤖 Claude: On the "Orphaned .extracting folders can remain after an interrupted extraction" note (suppressed on both csproj guards):

The premise that the destination can still contain the executable while a stale staging folder exists does not hold. ExtractTask only runs when the executable is absent from the destination, nothing in the task writes that executable before Directory.Move, and the move is a single directory rename. So after a kill at any point inside the task the executable is still missing, the next build's condition is true, the task runs again, and its first statement removes the stale staging folder before re-extracting.

Verified with a build-level check that reproduces the exact state described (a complete slicec in obj/slicec-<version>-<platform>.extracting, destination without slicec): the next dotnet build src/IceRpc.Slice.Tools re-extracted the executable and left no staging folder behind. A Codex review of this commit reached the same conclusion: extraction runs only when the destination executable is absent, so the orphan-folder concern does not apply.

The only leftover still possible is a staging folder for a toolchain version that was bumped between the kill and the next build. It accumulates the same way the per-version zips and extracted folders already do in obj/.

@bernardnormier bernardnormier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doc comment nit.

Comment thread build/ExtractTask.cs Outdated
Co-authored-by: Bernard Normier <bernard@zeroc.com>
@pepone
pepone merged commit 82d8ec7 into icerpc:main Sep 11, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-audit AI-generated audit finding — needs human triage build-system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Audit-Low] Interrupted toolchain extraction leaves a partial directory that is never re-extracted

4 participants