Make build/test worktree-aware with same-worktree lock

johnml1135 · johnml1135 · commit b738d4139d56 · 2026-03-19T16:34:34.000-04:00
diff --git a/.github/instructions/build.instructions.md b/.github/instructions/build.instructions.md
@@ -44,6 +44,23 @@ Key ordering constraint:
 ## Worktrees and concurrent builds
 This repo supports multiple concurrent builds across git worktrees. Prefer the scripts because they handle environment setup and avoid cross-worktree process conflicts.
 
+### Worktree-aware process cleanup
+- `build.ps1` now scopes process cleanup to the current repository root (`$PSScriptRoot`).
+- This avoids killing active `msbuild`/`dotnet`/test processes started from other worktrees.
+- The same worktree is intentionally exclusive: `build.ps1`/`test.ps1` acquire a named lock and fail fast if another build/test workflow is already running in that worktree.
+- Lock metadata is written to `Output/WorktreeRun.lock.json` while the workflow is active.
+- Optional actor tagging: set `FW_BUILD_STARTED_BY=user|agent` (or pass `-StartedBy`) to record who owns the lock.
+
+### MSBuild node reuse default
+- `build.ps1` defaults `-NodeReuse` to `auto`.
+- In `auto` mode, MSBuild node reuse is enabled when the repository has a single local worktree and disabled when multiple local worktrees are detected, reducing cross-worktree lock contention from shared worker nodes.
+- You can still override the policy explicitly with:
+
+```powershell
+.\build.ps1 -NodeReuse $true
+.\build.ps1 -NodeReuse $false
+```
+
 ## Troubleshooting (common)
 
 ### “Native artifacts missing” / code-generation failures
diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md
@@ -65,6 +65,13 @@ The testing infrastructure relies on shared PowerShell modules for consistency:
 -   **`Build/scripts/Invoke-CppTest.ps1`**: Backend for native C++ tests (MSBuild/NMake).
 -   **`Build/Agent/FwBuildHelpers.psm1`**: Shared logic for VS environment, and process cleanup.
 
+## Worktree-aware behavior
+- `test.ps1` scopes process cleanup to the current repository root (`$PSScriptRoot`) so concurrent runs in other worktrees are not terminated.
+- `build.ps1` and `test.ps1` in the same worktree both target `Output/<Configuration>/`; a same-worktree lock prevents concurrent runs and fails fast with owner details.
+- `build.ps1 -RunTests` is supported in the same worktree: the child `test.ps1` run reuses the parent workflow lock.
+- If you need concurrency, use separate git worktrees and run one scripted workflow per worktree.
+- Optional lock ownership tagging: set `FW_BUILD_STARTED_BY=user|agent` (or pass `-StartedBy`) before running scripts.
+
 ## Debugging & Logs
 -   **Logs**: Build logs are written to `Output/Build.log` (if configured) or standard output.
 -   **Verbosity**: Use `-Verbosity detailed` with `test.ps1` for more output.
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -40,6 +40,14 @@
     "/^(&\\s+)?git\\s+(worktree\\s+list|remote(\\s+(-v|show))?|config\\s+--get)(?!.*[;&|<>])/": true,
     "/^(&\\s+)?git\\s+stash(\\s+(push|apply|pop|list|show|drop|branch)(\\s+[^;&|<>]+)*)?(?!.*[;&|<>])/": true,
 
+    // GitHub CLI read-only operations only
+    // Explicitly allow read verbs (view/list/status/search) and block chained/redirection patterns.
+    "/^(&\\s+)?gh\\s+(pr|issue|repo|run|release|workflow|label|milestone|project|discussion|gist)\\s+(view|list|status)\\b(?!.*[;&|<>])/i": true,
+    "/^(&\\s+)?gh\\s+search\\s+(prs|issues|repos|commits|code)\\b(?!.*[;&|<>])/i": true,
+    // gh api is approved only for non-GraphQL calls when no explicit mutating method is specified.
+    // gh api graphql always uses POST, so require manual approval for any graphql invocation.
+    "/^(&\\s+)?gh\\s+api\\b(?!.*\\bgraphql\\b)(?!.*\\s(-X|--method)\\s*(POST|PUT|PATCH|DELETE)\\b)(?!.*[;&|<>])/i": true,
+
     // Python/scripting (consolidated)
     "/^(&\\s+)?python\\s+(scripts\\/|-m|\\.github\\/)(?!.*[;&|<>])/": true,
     "/^(&\\s+)?\\.[\\\\/]scripts[\\\\/](?!.*[;&|<>])/": true,
diff --git a/Build/Agent/FwBuildHelpers.psm1 b/Build/Agent/FwBuildHelpers.psm1
@@ -81,7 +81,7 @@ function Stop-ConflictingProcesses {
     # Filter by RepoRoot (Smart Kill) - only kill processes locking files in this repo
     if ($RepoRoot) {
         $processesToKill = @()
-        $RepoRoot = $RepoRoot.TrimEnd('\').TrimEnd('/')
+        $RepoRoot = $RepoRoot.TrimEnd([char[]]@([System.IO.Path]::DirectorySeparatorChar, [System.IO.Path]::AltDirectorySeparatorChar))
 
         foreach ($p in $processes) {
             if ($p.Id -eq $PID) { continue } # Don't kill self
@@ -125,6 +125,173 @@ function Stop-ConflictingProcesses {
     }
 }
 
+function Get-WorktreeMutexName {
+    param([Parameter(Mandatory)][string]$RepoRoot)
+
+    if ([string]::IsNullOrWhiteSpace($RepoRoot)) {
+        throw "RepoRoot cannot be null or empty."
+    }
+
+    $normalizedRepoRoot = [System.IO.Path]::GetFullPath($RepoRoot).TrimEnd([char[]]@([System.IO.Path]::DirectorySeparatorChar, [System.IO.Path]::AltDirectorySeparatorChar))
+    $normalizedRepoRoot = $normalizedRepoRoot.ToLowerInvariant()
+
+    $bytes = [System.Text.Encoding]::UTF8.GetBytes($normalizedRepoRoot)
+    $sha = [System.Security.Cryptography.SHA256]::Create()
+    try {
+        $hashBytes = $sha.ComputeHash($bytes)
+    }
+    finally {
+        $sha.Dispose()
+    }
+
+    $hash = [System.BitConverter]::ToString($hashBytes).Replace('-', '')
+    $shortHash = $hash.Substring(0, 16)
+	# Global scope is intentional to allow mutex visibility across processes regardless of session or user (e.g. VS and command line)
+    return "Global\FieldWorks.Worktree.$shortHash"
+}
+
+function Enter-WorktreeLock {
+    <#
+    .SYNOPSIS
+        Acquires an exclusive same-worktree lock for build/test workflows.
+    .DESCRIPTION
+        Uses a named Windows mutex keyed by RepoRoot to prevent concurrent build/test
+        runs inside the same worktree. Also writes lock metadata to Output/ so users
+        can see who currently owns the lock.
+    .PARAMETER RepoRoot
+        Repository root path for this worktree.
+    .PARAMETER Context
+        Friendly text describing the operation (e.g. "FieldWorks build").
+    .PARAMETER StartedBy
+        Optional actor tag for diagnostics (for example: user, agent).
+    #>
+    param(
+        [Parameter(Mandatory)][string]$RepoRoot,
+        [Parameter(Mandatory)][string]$Context,
+        [string]$StartedBy = 'unknown'
+    )
+
+    $mutexName = Get-WorktreeMutexName -RepoRoot $RepoRoot
+    $mutex = New-Object System.Threading.Mutex($false, $mutexName)
+    $hasHandle = $false
+
+    try {
+        try {
+            $hasHandle = $mutex.WaitOne(0)
+        }
+        catch [System.Threading.AbandonedMutexException] {
+            $hasHandle = $true
+        }
+
+        $lockPath = Join-Path $RepoRoot 'Output' | Join-Path -ChildPath 'WorktreeRun.lock.json'
+
+        if (-not $hasHandle) {
+            $ownerDetails = $null
+            if (Test-Path -LiteralPath $lockPath -PathType Leaf) {
+                try {
+                    $ownerDetails = Get-Content -LiteralPath $lockPath -Raw | ConvertFrom-Json
+                }
+                catch {
+                    $ownerDetails = $null
+                }
+            }
+
+            if ($ownerDetails) {
+                throw "$Context is already running in this worktree (ownerPid=$($ownerDetails.Pid), startedBy=$($ownerDetails.StartedBy), startedAtUtc=$($ownerDetails.StartedAtUtc), context=$($ownerDetails.Context)). Run one build/test workflow at a time per worktree."
+            }
+
+            throw "$Context is already running in this worktree. Run one build/test workflow at a time per worktree."
+        }
+
+        $outputDir = Split-Path -Parent $lockPath
+        if (-not (Test-Path -LiteralPath $outputDir -PathType Container)) {
+            New-Item -Path $outputDir -ItemType Directory -Force | Out-Null
+        }
+
+        $metadata = [pscustomobject]@{
+            Context = $Context
+            StartedBy = $StartedBy
+            Pid = $PID
+            ProcessName = (Get-Process -Id $PID).ProcessName
+            RepoRoot = [System.IO.Path]::GetFullPath($RepoRoot)
+            MutexName = $mutexName
+            StartedAtUtc = (Get-Date).ToUniversalTime().ToString('o')
+        }
+
+        $metadata | ConvertTo-Json -Depth 4 | Set-Content -LiteralPath $lockPath -Encoding UTF8
+
+        return [pscustomobject]@{
+            Mutex = $mutex
+            MutexName = $mutexName
+            LockPath = $lockPath
+            HasHandle = $true
+            OwnerPid = $PID
+        }
+    }
+    catch {
+        if ($mutex) {
+            if ($hasHandle) {
+                try {
+                    $mutex.ReleaseMutex()
+                }
+                catch {
+                    # Ignore release failures on the error path.
+                }
+            }
+
+            $mutex.Dispose()
+        }
+        throw
+    }
+}
+
+function Exit-WorktreeLock {
+    <#
+    .SYNOPSIS
+        Releases a same-worktree lock acquired by Enter-WorktreeLock.
+    #>
+    param([Parameter(Mandatory)]$LockHandle)
+
+    if (-not $LockHandle) {
+        return
+    }
+
+    $mutex = $LockHandle.Mutex
+    $lockPath = $LockHandle.LockPath
+
+    try {
+        if ($lockPath -and (Test-Path -LiteralPath $lockPath -PathType Leaf)) {
+            try {
+                $owner = $null
+                try {
+                    $owner = Get-Content -LiteralPath $lockPath -Raw | ConvertFrom-Json
+                }
+                catch {
+                    $owner = $null
+                }
+
+                if (-not $owner -or ($owner.Pid -eq $PID)) {
+                    Remove-Item -LiteralPath $lockPath -Force -ErrorAction SilentlyContinue
+                }
+            }
+            catch {
+                # Best effort only
+            }
+        }
+    }
+    finally {
+        if ($mutex) {
+            try {
+                $mutex.ReleaseMutex()
+            }
+            catch {
+                # Ignore release failures
+            }
+            $mutex.Dispose()
+        }
+    }
+}
+
 function Test-IsFileLockError {
     param([Parameter(Mandatory)][System.Management.Automation.ErrorRecord]$ErrorRecord)
 
@@ -156,10 +323,28 @@ function Test-IsFileLockError {
 }
 
 function Invoke-WithFileLockRetry {
+    <#
+    .SYNOPSIS
+        Runs an action and retries once when a file lock conflict is detected.
+    .DESCRIPTION
+        On lock-related failures, performs the same process cleanup used by build/test scripts
+        before retrying. When RepoRoot is provided, cleanup remains worktree-aware.
+    .PARAMETER Action
+        Script block to execute.
+    .PARAMETER Context
+        Friendly text used in retry warnings.
+    .PARAMETER IncludeOmniSharp
+        Includes OmniSharp processes in cleanup candidates.
+    .PARAMETER RepoRoot
+        Optional repository root used to scope cleanup to the current worktree.
+    .PARAMETER MaxAttempts
+        Number of attempts (default 2).
+    #>
     param(
         [Parameter(Mandatory)][ScriptBlock]$Action,
         [Parameter(Mandatory)][string]$Context,
         [switch]$IncludeOmniSharp,
+        [string]$RepoRoot,
         [int]$MaxAttempts = 2
     )
 
@@ -173,7 +358,7 @@ function Invoke-WithFileLockRetry {
             if ($attempt -lt $MaxAttempts -and (Test-IsFileLockError -ErrorRecord $_)) {
                 $nextAttempt = $attempt + 1
                 Write-Host "[WARN] $Context hit a file lock. Cleaning and retrying (attempt $nextAttempt of $MaxAttempts)..." -ForegroundColor Yellow
-                Stop-ConflictingProcesses -IncludeOmniSharp:$IncludeOmniSharp
+                Stop-ConflictingProcesses -IncludeOmniSharp:$IncludeOmniSharp -RepoRoot $RepoRoot
                 Start-Sleep -Seconds 2
                 $retry = $true
             }
@@ -321,6 +506,8 @@ Export-ModuleMember -Function @(
     'Get-CvtresDiagnostics',
     # Local functions
     'Stop-ConflictingProcesses',
+    'Enter-WorktreeLock',
+    'Exit-WorktreeLock',
     'Remove-StaleObjFolders',
     'Test-IsFileLockError',
     'Invoke-WithFileLockRetry'
diff --git a/ReadMe.md b/ReadMe.md
@@ -36,6 +36,14 @@ FieldWorks uses the **MSBuild Traversal SDK** for declarative, dependency-ordere
 
 For detailed build instructions, see [.github/instructions/build.instructions.md](.github/instructions/build.instructions.md).
 
+### Concurrent worktree builds/tests
+
+`build.ps1` and `test.ps1` use worktree-aware process cleanup, so running scripted builds/tests in different git worktrees does not kill each other.
+
+Within a single worktree, builds and tests run one at a time: scripts acquire a worktree lock and fail fast if another scripted workflow is active.
+
+You can tag lock ownership for diagnostics with `FW_BUILD_STARTED_BY=user|agent` (or `-StartedBy user|agent`).
+
 ## Building Installers (WiX 3 default, WiX 6 opt-in)
 
 Installer builds default to **WiX 3** (legacy batch pipeline) using inputs in `FLExInstaller/` and `PatchableInstaller/`. The **Visual Studio WiX Toolset v3 extension** is required so `Wix.CA.targets` is available under the MSBuild extensions path. Use `-InstallerToolset Wix6` to opt into the WiX 6 SDK-style path (restored via NuGet).
diff --git a/build.ps1 b/build.ps1
diff --git a/test.ps1 b/test.ps1
