Skip to content

Make build/test worktree-aware and add same-worktree lock#737

Open
johnml1135 wants to merge 2 commits intomainfrom
simultaneous_build
Open

Make build/test worktree-aware and add same-worktree lock#737
johnml1135 wants to merge 2 commits intomainfrom
simultaneous_build

Conversation

@johnml1135
Copy link
Contributor

@johnml1135 johnml1135 commented Mar 4, 2026

Summary

  • Scope process cleanup in build/test flows to the current worktree root.
  • Add lightweight same-worktree exclusivity using a named mutex plus Output/WorktreeRun.lock.json metadata.
  • Add optional actor tagging via FW_BUILD_STARTED_BY or -StartedBy.
  • Keep build.ps1 -RunTests working by having child test.ps1 reuse the parent lock (-SkipWorktreeLock).
  • Default build.ps1 -NodeReuse to false for safer multi-worktree behavior.
  • Document behavior in build/testing instructions and README.

Files

  • build.ps1
  • test.ps1
  • Build/Agent/FwBuildHelpers.psm1
  • .github/instructions/build.instructions.md
  • .github/instructions/testing.instructions.md
  • ReadMe.md

Validation

  • Ran diagnostics checks on changed PowerShell files.
  • No new diagnostics in build.ps1 or Build/Agent/FwBuildHelpers.psm1.
  • Existing pre-existing warning remains in test.ps1 (vstestOutput assigned but not used).

This change is Reviewable

Copilot AI review requested due to automatic review settings March 4, 2026 19:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the repo’s PowerShell build/test tooling to be worktree-aware, enabling safer concurrent workflows across multiple git worktrees while enforcing single active scripted workflow per worktree via a lock.

Changes:

  • Add same-worktree exclusivity using a named mutex + Output/WorktreeRun.lock.json metadata (with optional FW_BUILD_STARTED_BY / -StartedBy actor tagging).
  • Scope process cleanup and file-lock retry cleanup to the current worktree root ($PSScriptRoot) so other worktrees aren’t affected.
  • Change build.ps1 default MSBuild node reuse to -NodeReuse $false to reduce cross-worktree contention, and document the new behavior.

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
build.ps1 Acquires worktree lock, scopes cleanup by repo root, defaults -NodeReuse to false, and passes -SkipWorktreeLock to child test.ps1 under -RunTests.
test.ps1 Adds optional -StartedBy, supports -SkipWorktreeLock, scopes cleanup by repo root, and releases lock in finally.
Build/Agent/FwBuildHelpers.psm1 Implements Enter-WorktreeLock/Exit-WorktreeLock, adds worktree-aware filtering to Stop-ConflictingProcesses, and extends Invoke-WithFileLockRetry with optional -RepoRoot.
.github/instructions/build.instructions.md Documents worktree-aware cleanup, same-worktree lock behavior, and -NodeReuse default change.
.github/instructions/testing.instructions.md Documents worktree-aware behavior and lock reuse for build.ps1 -RunTests.
ReadMe.md Adds a concise overview of concurrent worktree behavior and lock tagging.

}
}
catch {
if (-not $hasHandle -and $mutex) {
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

In Enter-WorktreeLock, if the mutex has already been acquired ($hasHandle = $true) and an exception occurs after that (e.g., failing to create Output/ or write WorktreeRun.lock.json), the catch block rethrows without releasing/disposing the mutex. In a long-lived PowerShell session this can leave the mutex held and block subsequent build/test runs. Ensure the error path releases the mutex (when owned) and disposes it before throwing (or wrap the whole acquisition/write flow in a try/finally that always cleans up on failure).

Suggested change
if (-not $hasHandle -and $mutex) {
if ($mutex) {
if ($hasHandle) {
try {
$mutex.ReleaseMutex()
}
catch {
# Ignore release failures on error path
}
}

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Mar 4, 2026

NUnit Tests

    1 files  ±0      1 suites  ±0   5m 50s ⏱️ +2s
4 407 tests ±0  4 320 ✅ ±0  87 💤 ±0  0 ❌ ±0 
4 416 runs  ±0  4 329 ✅ ±0  87 💤 ±0  0 ❌ ±0 

Results for commit f14a7c6. ± Comparison against base commit fe415b8.

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