fix: Stop builds hanging on a large HEAD commit message - #168
fix: Stop builds hanging on a large HEAD commit message#168tablackburn wants to merge 2 commits into
Conversation
BuildHelpers populates $env:BHCommitMessage by running
git log --format=%B -n 1
through Invoke-Git, which redirects git's output streams and then calls
WaitForExit() before reading them. Once git writes more than the pipe
buffer holds it blocks waiting for a reader while BuildHelpers blocks
waiting for the process, and the build stops with no output and no
timeout. Windows has the smaller buffer, so it hangs there first.
Measured on this repository: a 5498-byte commit message returns, 5757
bytes hangs. Squashed pull request bodies reach that size routinely, so
the merge of #162 hung both Windows CI legs on main.
Get-BuildVariable only shells out for the message when it cannot read one
from a known CI variable. Invoke-SetBuildEnvironment reads the message
itself -- PowerShell drains the pipe while the process writes, so it
cannot deadlock -- publishes it through the Azure Pipelines variable
BuildHelpers consumes verbatim, calls Set-BuildEnvironment, and removes
the variable again. Nothing else keys off that variable: the build
system, branch, commit hash, and build number are each detected from
different ones, so borrowing it does not make BuildHelpers report an
Azure Pipelines build. A real pipeline that already supplies it is left
alone.
All three call sites are covered: Initialize-PSBuild, the shipped
build.properties.ps1 that runs before any consumer task, and this
repository's own build.ps1.
The regression tests run each call in a child process that is killed if
it overruns, because without the fix they would not fail, they would
hang. A background job cannot be used: Invoke-Git deadlocks inside one
whatever the output size, so a job-based harness would hang even with the
fix in place.
The defect is upstream in BuildHelpers, whose last release predates this
by five years.
Closes #167
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018TJfFJGtUJY5CFu8MRRMYt
There was a problem hiding this comment.
Pull request overview
This PR prevents PowerShellBuild-based builds (especially on Windows) from hanging when the HEAD commit message is large by avoiding BuildHelpers’ Invoke-Git deadlock path and supplying the commit message via a CI environment variable that BuildHelpers already consumes.
Changes:
- Introduces a private wrapper (
Invoke-SetBuildEnvironment) that reads the HEAD commit message safely and injects it throughBUILD_SOURCEVERSIONMESSAGEso BuildHelpers does not invoke the problematicgit logcode path. - Updates key build entry points to use the wrapper instead of calling
Set-BuildEnvironmentdirectly. - Adds regression tests that run the call in a killable child process to validate “does not hang” behavior and message correctness.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/Invoke-SetBuildEnvironment.tests.ps1 |
Adds regression coverage for the deadlock using a large commit message fixture and a timeout-enforced child process runner. |
PowerShellBuild/Public/Initialize-PSBuild.ps1 |
Routes environment initialization through Invoke-SetBuildEnvironment to avoid the BuildHelpers hang. |
PowerShellBuild/Private/Invoke-SetBuildEnvironment.ps1 |
Implements the deadlock workaround by pre-populating the commit-message CI variable and restoring BHCommitMessage. |
PowerShellBuild/build.properties.ps1 |
Dot-sources and invokes the wrapper early so consumer builds do not hang before any task output. |
CHANGELOG.md |
Documents the fix and its rationale as part of the release notes. |
build.ps1 |
Updates the repo’s own build entry point to use the wrapper prior to invoking psake. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try { | ||
| if ($commitMessage) { | ||
| Set-Item -Path $commitMessageVariable -Value $commitMessage | ||
| } | ||
|
|
||
| BuildHelpers\Set-BuildEnvironment @Parameter | ||
|
|
||
| if ($commitMessage) { | ||
| # The Azure Pipelines path joins the message onto a single line. Restore the form the | ||
| # git path produces so the variable looks the same as it always has. | ||
| $env:BHCommitMessage = $commitMessage | ||
| } | ||
| } finally { | ||
| if ($commitMessage) { | ||
| Remove-Item -Path $commitMessageVariable -ErrorAction SilentlyContinue | ||
| } | ||
| } |
The first cut supplied the commit message through the Azure Pipelines variable and assumed BuildHelpers would use it. Get-BuildVariable picks the source with a switch over an unordered collection of environment variable names, so when a build system also publishes a commit SHA -- GITHUB_SHA, CI_COMMIT_SHA, GIT_COMMIT and four others -- whichever name the collection yields first wins. That is a coin toss, and CI called it the other way: the ubuntu and macOS legs took the GITHUB_SHA branch, which runs git for the message, and reported the runner's environment instead of the fixture's. Removing those seven variables for the duration of the call makes the choice deterministic, because no branch that shells out for the message can be selected. They are restored afterwards, and because they also supply the commit hash, the hash BuildHelpers derives from HEAD is replaced with the value the build system gave. The tests now clear inherited build system variables in the child before applying the ones each case asks for, so results no longer depend on where the suite runs, and a new case covers the GitHub Actions combination that hung main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018TJfFJGtUJY5CFu8MRRMYt
|
Follow-up after the first CI run, since the PR description no longer tells the whole story. CI caught a real hole in the first cut, not a flaky test. The original approach supplied the commit message through That is a coin toss, and it lands differently in different environments. Locally The fix now removes those seven variables for the duration of the call and restores them afterwards, which makes the choice deterministic rather than lucky. Because they also supply the commit hash, the hash is restored from the build system value rather than left as the HEAD-derived one. Two things a reviewer should know:
All legs green now: 465 tests, +13. |
|
Closing unmerged — not abandoned. The defect is upstream in BuildHelpers (RamblingCookieMonster/BuildHelpers#86, open since 2018), and BuildHelpers is in the process of transferring to PowerShellOrg, which puts a real fix there within reach. Shipping ~150 lines of environment juggling into the module days before the 1.0.0 API freeze — with the intent of deleting it once upstream lands — is not a good trade for a bug that needs a commit message over ~5.5 KB on Windows to trigger. The branch is left in place. Reopen if the upstream path stalls or 1.0.0 arrives without a fix. The mechanism, measurements, and reproducer are recorded on #167, which stays open. |
Summary
Builds hang — no output, no error, no timeout — when the HEAD commit message is large. This
happened on
maintwo hours ago: the#162merge hung both Windows CI legs for 27 minutesbefore I cancelled them, and every local
./build.ps1hung with it.BuildHelperspopulates$env:BHCommitMessageby runninggit log --format=%B -n 1throughInvoke-Git, which redirects git's output streams and then callsWaitForExit()beforereading them:
Once git writes more than the pipe buffer holds, git blocks waiting for a reader and
BuildHelpersblocks waiting for git. Neither moves. Windows has the smaller buffer, so it hangsthere first — ubuntu and macOS passed the same commit.
Measured against real commits on
main:f726f4864b72079720cb7Squashed pull request bodies in this repository cross that line routinely, so this is the house
style meeting an upstream defect, not an exotic edge case.
Approach
Get-BuildVariableonly shells out for the commit message when it cannot read one from a knownCI variable.
Invoke-SetBuildEnvironmentreads the message itself — PowerShell drains the pipewhile the process writes, so it cannot deadlock — publishes it through the one variable
BuildHelpersconsumes verbatim, callsSet-BuildEnvironment, and removes the variable again.The git command that hangs never runs.
Borrowing the Azure Pipelines variable is the part worth scrutinising, so: nothing else keys off
it. The build system is detected from
BUILD_DEFINITIONNAME, the branch fromBUILD_SOURCEBRANCHNAME, the commit hash fromBUILD_SOURCEVERSION, the build root fromSYSTEM_DEFAULTWORKINGDIRECTORY— all different variables. A test assertsBHBuildSystemstillreads
Unknown. A real Azure pipeline that already supplies the variable is left untouched, anda test covers that too.
All three call sites are fixed:
Initialize-PSBuild— the shipped entry pointPowerShellBuild/build.properties.ps1— runs before any consumer task, so the hang would landbefore a consumer's build printed a single line
build.ps1— this repository's own buildOne implementation, dot-sourced by the two callers that live outside the module's scope. No new
public API: the helper is private, which matters with the 1.0.0 freeze approaching, because this
is a workaround we want to delete rather than support.
Test Plan
completes instead of hangingfail after the 120s kill. Green with it, in 6sSet-BuildEnvironmentstill hangs on the same fixtureThe tests run each call in a child process that is killed if it overruns, because without the
fix they do not fail, they hang forever. A
Start-Jobharness cannot be used:Invoke-Gitdeadlocks inside a background job whatever the output size — an 85-byte commit message and a
5-byte
rev-parsestill hang — so a job-based test would hang even with the fix in place. Thatis a second face of the same defect and is worth knowing before anyone tries to call
Set-BuildEnvironmentfrom a job.Breaking Changes
None. The variables
Set-BuildEnvironmentproduces are unchanged, including$env:BHCommitMessage, which is normalized exactly the wayGet-BuildVariablenormalizes it(blank lines dropped, lines trimmed, joined with newlines) so the value is identical to what
consumers saw before.
Notes for the reviewer
BuildHelpers, whose last release(2.0.16) shipped in December 2020 — five years ago — so waiting for a fix there is not a plan.
Reporting it upstream and reconsidering the dependency both belong to [Tracking] PowerShellBuild v1.0.0 roadmap #120's dependency
question rather than to this PR
short
-b, ormain's Windows CI will hang on this very commit messageCloses #167