buildkit: rebind daemon to the current process on miren restart#885
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 6bf18d7d5ae24c0ec750e677c2d5989a81ba50b4 and ef7097e. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughChanges
Compact metadata
Related issues: None referenced. Related PRs: None referenced. Suggested labels: buildkit, reliability, testing Suggested reviewers: None specified. Poem Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
servers/build/build_saga_buildkit_test.go (1)
22-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the newly-added
bkc.Info()health check.
fakeBuildKitProvider.Clientalways returnsnil, f.clientErr, sorunBuildkitBuildshort-circuits at the pre-existingb.BuildKit.Client(ctx)error check (build_saga_buildkit.go lines 142-147) and never reaches the new pre-flightbkc.Info(ctx)health check (lines 150-161) that this PR actually introduces. The test validates real but pre-existing behavior, leaving the PR's core new safeguard (detecting a stale-but-reachable daemon) untested.Consider adding a case where
Client()succeeds (e.g., a client dialed against a non-listening/bogus unix socket) soInfo()itself fails, actually covering the new code path.Also applies to: 37-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/build/build_saga_buildkit_test.go` around lines 22 - 33, The current test setup only exercises the existing BuildKit client creation failure path and never reaches the new bkc.Info() pre-flight health check in runBuildkitBuild. Update fakeBuildKitProvider (or add a new variant) so Client() can succeed while Info() fails, allowing the test to cover the stale-but-reachable daemon path introduced by the new health check. Use the runBuildkitBuild and fakeBuildKitProvider symbols to locate the affected test cases and add coverage for the Info() failure branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/buildkit/buildkit.go`:
- Around line 177-189: Reset c.container to nil whenever task setup fails after
assigning the container, so later Stop() calls don’t act on a deleted container.
Update the failure paths around container.NewTask and c.startTaskAndMonitor to
clear c.container before returning after container.Delete(...), and apply the
same cleanup pattern in restartExistingContainer where the container is deleted
on startup failure.
---
Nitpick comments:
In `@servers/build/build_saga_buildkit_test.go`:
- Around line 22-33: The current test setup only exercises the existing BuildKit
client creation failure path and never reaches the new bkc.Info() pre-flight
health check in runBuildkitBuild. Update fakeBuildKitProvider (or add a new
variant) so Client() can succeed while Info() fails, allowing the test to cover
the stale-but-reachable daemon path introduced by the new health check. Use the
runBuildkitBuild and fakeBuildKitProvider symbols to locate the affected test
cases and add coverage for the Info() failure branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0397d09d-a9ff-42fe-86d0-e44413bbe841
📥 Commits
Reviewing files that changed from the base of the PR and between 5b9b37b and b6581c796aaaead39097749386d452b68c844858.
📒 Files selected for processing (6)
cli/commands/server_install.gocomponents/buildkit/buildkit.gocomponents/buildkit/buildkit_test.goservers/build/build.goservers/build/build_saga_buildkit.goservers/build/build_saga_buildkit_test.go
evanphx
force-pushed
the
mir-1303-buildkitd-stale-process-reuse-after-miren-restart
branch
from
b6581c7 to
6bf18d7
Compare
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
LGTM! Good catch and good fix 🛠️
| @@ -95,14 +105,14 @@ type Builder struct { | |||
|
|
|||
| // BuildKit is the persistent BuildKit component for container image builds. | |||
| // When set, uses the shared daemon instead of launching ephemeral sandboxes. | |||
| BuildKit *buildkit.Component | |||
| BuildKit BuildKitProvider | |||
buildkitd runs as a containerd-managed container. On a clean miren shutdown, Stop() SIGTERMs it and deletes the container, so the next boot creates a fresh daemon. But KillMode=process means an ungraceful miren exit (crash, OOM, a second SIGTERM that hits os.Exit, or a stop that overruns TimeoutStopSec and gets SIGKILLed) leaves buildkitd orphaned and still running. On the next boot restartExistingContainer() found that running task and reused it, only dialing the socket to confirm readiness. buildkitd's in-process session server was bound to the miren process that launched it, so the reused daemon can't serve the new process: builds fail with "NotFound: no such job". This is silent and every build fails until buildkitd is manually killed and miren restarted. Rare (needs an ungraceful exit) but fatal when it happens. Fixes MIR-1303: - restartExistingContainer() now always force-stops any existing task and creates a fresh one bound to the current process. - stopTask() no longer returns early when SIGTERM fails, so task.Delete() always runs and can't leak a task that blocks the next NewTask. The delete context keeps the containerd namespace while detaching from the parent's cancellation. - A task exit-monitor goroutine via task.Wait() clears c.running when buildkitd dies unexpectedly, so IsRunning()/Client() reflect reality; Stop() cancels it and tears down the container even after such a death. - The build pre-flight bkc.Info() check is fatal, failing a build fast on a dead/stale daemon instead of proceeding into a confusing error. - servers/build gains a BuildKitProvider interface so builds can be faked in unit tests; adds unit + integration-gated component tests.
evanphx
force-pushed
the
mir-1303-buildkitd-stale-process-reuse-after-miren-restart
branch
from
6bf18d7 to
ef7097e
Compare
evanphx
deleted the
mir-1303-buildkitd-stale-process-reuse-after-miren-restart
branch
2 participants
Problem (MIR-1303)
buildkitd runs as a containerd-managed container. On a clean miren shutdown, the first SIGTERM cancels the server context (
cli/commands/global.go), which unwindsServer()and runs the deferredbuildkitComponent.Stop()(cli/commands/server.go).Stop()SIGTERMs buildkitd and deletes the container, so the next boot'sLoadContainerreturns NotFound and creates a fresh daemon. That is the normal, working restart path — the one exercised bysystemctl restart, deploy-driven bounces, andmake dev-server-restart.KillMode=processin the systemd unit changes this on an ungraceful exit. If miren dies without itsStop()defer completing — a crash / panic / OOM-kill, a second SIGTERM (which hitsos.Exit(130)and bypasses defers), or a graceful stop that exceedsTimeoutStopSec=90sand gets SIGKILLed —KillMode=processkills only miren's own process and leaves buildkitd (in containerd's cgroup) orphaned and still running.On the next boot,
restartExistingContainer()finds that still-running task, setsc.running = true, and reuses it — only dialing the socket to confirm readiness. It never restarts the buildkitd process. But the reused daemon can't serve the new miren process: buildkit builds use a client-side session (the daemon streams local build context back from the client over a hijacked gRPC stream), and that session state was bound to the miren process that exited. The new process's solves fail withNotFound: no such job <ref>. This is silent — no error, no log — and every build fails until buildkitd is manually killed and miren restarted.It hit the luanchat cluster on 2026-06-30: all builds failed for ~2.5 hours, and buildkitd had been up ~3 days — i.e. it had survived across the restart via the ungraceful path above.
So the failure is rare but fatal: it needs an ungraceful exit to orphan a live daemon, which is why it went unnoticed for so long even though miren has been restarted many times — the vast majority of those restarts took the fresh-daemon path. But once it happens, every build fails until manual intervention.
Fix
Handle the orphaned daemon at the layer that owns buildkit's lifecycle — the containerd component, on start. When
Start()finds an existing container, reach the same known-good state a fresh boot produces: a buildkitd task freshly bound to the current process.restartExistingContainer()now always force-stops any existing task and starts a fresh one bound to the current process (the primary fix). It no longer reuses a running task. This evicts an orphaned daemon on the next start regardless of how miren exited.stopTask()no longer returns early when SIGTERM fails, sotask.Delete()always runs and can't leak a task that would block the nextNewTask. The delete context keeps the containerd namespace while detaching from the parent's cancellation.task.Wait(), established beforeStart()) clearsc.runningwhen buildkitd dies unexpectedly, soIsRunning()/Client()reflect reality.Stop()cancels it and now tears down the container even after such a death.bkc.Info()pre-flight is now fatal — a dead/stale daemon fails the build immediately instead of proceeding into a confusing downstream error.BuildKitProviderinterface inservers/buildlets builds be faked in unit tests (with a typed-nil guard preserving theBuildKit == nilcheck).Testing
hack/it components/buildkit— 10 tests pass, including newTestBuildkitRestartsubtests that prove the buildkitd PID actually changes across a simulated restart (old task evicted, new one created and serving), thatIsRunning()flips false on an out-of-band daemon death, and thatStop()cleans up the container even after a death.go test ./servers/build/— new fake-provider unit test for the connect-error fast-fail; full package green.golangci-lint/gofmt/go vetclean.