Skip to content

kill locking processes before doing actions that write to artifacts directories #49868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

baronfel
Copy link
Member

It's pretty common that I do a ./build.cmd and the redist fails to overlay the SDK because something's holding on to dlls under artifacts still. I hate finding that out at the end, so I added scripts that should terminate such processes as a pre-check when running the repo build scripts.

@baronfel baronfel requested review from Copilot and a team July 19, 2025 22:05
Copy link
Contributor

@Copilot 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 adds process termination functionality to prevent build failures caused by lingering dotnet processes holding locks on artifacts directory files. The changes implement a Stop-ArtifactLockers function in both shell and PowerShell build scripts to identify and terminate dotnet processes that may interfere with cleaning or writing to the artifacts directory.

  • Adds Stop-ArtifactLockers function to both build.sh and build.ps1 scripts
  • Calls the function before cleaning artifacts directory and before main build execution
  • Implements cross-platform process detection and termination logic

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
eng/common/build.sh Adds bash implementation of Stop-ArtifactLockers function with process detection and graceful termination
eng/common/build.ps1 Adds PowerShell implementation of Stop-ArtifactLockers function with .NET process filtering
Comments suppressed due to low confidence (3)

eng/common/build.sh:265

  • The function name uses PowerShell naming convention with a hyphen, but this is a bash script. Consider using bash naming convention like 'stop_artifact_lockers' or 'StopArtifactLockers'.
function Stop-ArtifactLockers {

eng/common/build.sh:385

  • Function call uses PowerShell naming convention but this is bash. Should be consistent with the function definition naming.
    Stop-ArtifactLockers "$repo_root"

eng/common/build.sh:397

  • Function call uses PowerShell naming convention but this is bash. Should be consistent with the function definition naming.
Stop-ArtifactLockers "$repo_root"

$artifactsRoot = Join-Path $RepoRoot 'artifacts'
$dotnetExes = @()
try {
$dotnetExes = Get-Process -Name 'dotnet' -ErrorAction SilentlyContinue | Where-Object { $_.Path -eq $repoDotnetPath -and $_.CommandLine.StartsWith($artifactsRoot) }
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

The logic differs from the bash version - this checks if CommandLine starts with artifacts path, while bash checks if CommandLine contains artifacts DLL paths. This inconsistency could lead to different behavior between platforms.

Suggested change
$dotnetExes = Get-Process -Name 'dotnet' -ErrorAction SilentlyContinue | Where-Object { $_.Path -eq $repoDotnetPath -and $_.CommandLine.StartsWith($artifactsRoot) }
$dotnetExes = Get-Process -Name 'dotnet' -ErrorAction SilentlyContinue | Where-Object { $_.Path -eq $repoDotnetPath -and $_.CommandLine -match [regex]::Escape($artifactsRoot) }

Copilot uses AI. Check for mistakes.

if [[ "$path" == "$repo_dotnet_path" ]]; then
dotnet_pids+=("$pid")
fi
done < <(pgrep -f dotnet -l 2>/dev/null | grep -E "^[0-9]+ .*dotnet$" || true)
Copy link
Preview

Copilot AI Jul 19, 2025

Choose a reason for hiding this comment

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

The regex pattern .*dotnet$ will match any process ending with 'dotnet', not necessarily the exact dotnet executable path. This could match unrelated processes with 'dotnet' in their name.

Suggested change
done < <(pgrep -f dotnet -l 2>/dev/null | grep -E "^[0-9]+ .*dotnet$" || true)
done < <(pgrep -f dotnet -l 2>/dev/null | grep -E "^[0-9]+ dotnet$" || true)

Copilot uses AI. Check for mistakes.

…ses holding on to things in artifacts before performing actions that write to artifacts.
@baronfel baronfel force-pushed the prevent-artifacts-file-locs branch from 3577b49 to 35da738 Compare July 21, 2025 21:10
Copy link
Member

@joeloff joeloff left a comment

Choose a reason for hiding this comment

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

You should make this change in Arcade. These files will be overwritten the next time an Arcade update flows through to the SDK.

@baronfel
Copy link
Member Author

good call @joeloff - there's also something going on in the generated script where the .Count isn't available on the powershell array for the discovered processes. I couldn't repro on windows powershell or pwsh, though - so I don't know what's really going on there. back to the drawing board!

@baronfel baronfel closed this Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants