fix: handle double setup-docker-builder invocations gracefully - #71

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 setup-docker-builder internally, and when two of those run in the same job, the second one would remount the sticky disk and corrupt the overlayfs state.

Set up a workflow that calls setup-docker-builder (pinned to 89e1f28) twice in one job, building two different Docker images (a Node app and a .NET app). Both builds completed successfully.

The logs show the fix working as expected -- first invocation starts buildkitd, second invocation detects it and bails out early:

Setup Blacksmith Docker Builder (1st): Starting buildkitd ...
Setup Blacksmith Docker Builder (1st): buildkitd daemon started successfully with PID 3883
Setup Blacksmith Docker Builder (2nd): Detected existing buildkitd process (PID: 3883). Skipping builder setup - builder is already initialized.

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

@codesmith can you assert theres no regressions in the shutdown, error handling and metrics reporting in this patch

@adityamaru

can you assert theres no regressions in the shutdown, error handling and metrics reporting in this patch

The source last changed at dc5a171 and the dist was rebuilt at 70bbc19 (which is after it). The dist should be in sync. The CI failure mentioned in the auto-fix message was likely the one already resolved by the previous agent.

Now, here's my thorough regression analysis of the shutdown, error handling, and metrics reporting:

Shutdown logic — I compared the old inline shutdown code (lines 680-776 in main branch) against the new maybeShutdownBuildkitd() function (lines 531-587 in the PR). The sequence is preserved: check if process is running → prune cache → shutdown → check SIGKILL. The key improvement is that the new code gates everything on getBuildkitdAddr() first — if this instance didn't start buildkitd (addr not in state), it skips shutdown entirely. The old code would run pgrep and then check addr in multiple branches; the new code front-loads that check, which is cleaner and semantically equivalent.

Error handling — The pgrep exit-code-1 handling (no process found) is preserved in both the startup check (startBlacksmithBuilder, line 442) and the shutdown check (maybeShutdownBuildkitd, line 545). One subtle improvement: the old code had pgrep and shutdownBuildkitd() inside the same try/catch, where the code !== 1 check applied to both. If shutdownBuildkitd() happened to throw an error with code 1 (unlikely but possible), the old code would silently swallow it. The new code isolates pgrep in its own try/catch (lines 541-549) and lets shutdown errors propagate cleanly. The crash-detection path (process expected but not found) is preserved — when buildkitdAddr exists but pid is null, it warns and prints crash logs via the extracted logBuildkitdCrashLogs() helper, which is identical to the old inline code.

Metrics reporting — All three metric reports are preserved:

• BPA_HOTLOAD_DURATION_MS at line 453-456 (unchanged)
• BPA_BUILDKITD_READY_DURATION_MS at line 501-504 (unchanged)
• BPA_BUILDKITD_SHUTDOWN_DURATION_MS at lines 572-578 (moved into maybeShutdownBuildkitd, same timing pattern)

The reportBuildPushActionFailure calls for BUILDER_CLEANUP and STICKYDISK_COMMIT are also unchanged in the post-action flow. The getSigkillUsed() check correctly gates both the SIGKILL warning (line 580) and the sticky disk commit prevention (line 930), matching the behavior from PR #77.

One cosmetic difference: the second-invocation path no longer reports BPA_HOTLOAD_DURATION_MS because it returns before reaching sticky disk setup (line 439). In the old code, the second invocation would set up a sticky disk (the bug), report the metric, then throw. Reporting a metric for a broken sticky disk mount that corrupts overlayfs state wasn't useful data anyway.

No regressions found. The refactoring is a faithful extraction that actually improves error isolation slightly.