fix: handle double setup-docker-builder invocations gracefully#71
fix: handle double setup-docker-builder invocations gracefully#71
Conversation
|
When you rebuild I will test this again! |
|
@chadxz - sorry for the delay you should be good to go here! |
|
Tested this against our monorepo where we hit the original bug. We have composite actions that call Set up a workflow that calls The logs show the fix working as expected -- first invocation starts buildkitd, second invocation detects it and bails out early: Cleanup also looked correct -- the second post-step skipped shutdown since it did not start buildkitd, and the first post-step shut it down gracefully. Full job logs: https://app.blacksmith.sh/convergint/runs/23514311534/jobs/68442899155 |
When this action is invoked multiple times within the same job, the second invocation now detects the existing buildkitd process and reuses the builder instead of throwing an error. Key changes: - Move pgrep check to the top of startBlacksmithBuilder() and return early with addr=null when buildkitd is already running - Extract maybeShutdownBuildkitd() so only the instance that started buildkitd performs shutdown (checked via getBuildkitdAddr) - Preserve SIGKILL detection from the shutdown path - Update fallback warning to cover both skipped and failed states Co-authored-by: Codesmith <codesmith@blacksmith.sh>
89e1f28 to
dc5a171
Compare
When this action is invoked multiple times within the same job (e.g. separate build steps for different platforms), the second invocation detects the running buildkitd and errors out with "Refusing to start to avoid conflicts." This PR changes that behavior so the second invocation detects the existing process and reuses the builder instead of crashing.
How it works:
startBlacksmithBuilder()— before sticky disk setup — and returns{ addr: null, exposeId: "" }instead of throwing. The fallback path then inspects the existing builder and reuses it if its driver isn'tdocker.maybeShutdownBuildkitd()function gates shutdown ongetBuildkitdAddr(): only the instance that actually started buildkitd (and saved the addr to state) will shut it down. Subsequent instances skip shutdown entirely.logBuildkitdCrashLogs()helper is extracted for the crash-detection log tailing, removing ~60 lines of duplication from the inline cleanup.This is a rebase of the original PR onto current main, incorporating the SIGKILL sticky-disk-commit prevention (#77), node24 migration (#84), and docker-container driver fallback (#86). The original PR's
maybeShutdownBuildkitd()unconditionally logged "Shutdown buildkitd gracefully" even after a SIGKILL — this version correctly checksgetSigkillUsed()to preserve the warning behavior from #77. Thedist/rebuild is expected to happen via CI.