From 7c49348afed3006a98a87949396364100d53fe52 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Sun, 13 Sep 2026 18:16:38 -0700 Subject: [PATCH] fix(runtime): stop the reaper sweeping LIVE jobs' dind bind staging dirs Regression introduced by #187 (v0.2.8). Its periodic dead-container reaper called dind.SweepStagedBinds once per pass. That function is documented STARTUP ONLY, and its own comment says why: "it does not know which jobs are live, and unmounting a running job's staged bind would not break that job's already-running containers but would break any container it starts next." Which is exactly what happened on the fleet's Linux amd64 nodes: docker: Error response from daemon: bind mount /home/runner/_work/... -> /w rejected: creating bind staging mountpoint /var/lib/ephemerd/dind-binds/ephemerd-github-ephpm-sure_hopper/2: mkdir ...: no such file or directory A reaper tick reaps some unrelated dead container, sweeps the whole /dind-binds tree, and deletes the staging directory of every LIVE job with it. The running job's existing containers keep working (their mounts are already in their namespaces), so nothing fails loudly -- but its NEXT `docker run -v` cannot create its mountpoint, because ensureDirLocked has latched m.ready and will not recreate the parent. Hence the index /2: binds 0 and 1 staged fine before the tick landed. ephpm's release workflow is the heaviest user of `docker run -v "$PWD":/w" (since the node24 de-containerization), which is why it surfaced there. Fix: sweep only the staging dir of the container actually being reaped, via a new SweepStagedBindsForJob. The reaping motive is unchanged -- a leaked staged mount pins that container's rootfs so its snapshot cannot be deleted -- and scoping it keeps that without touching anyone else's. The startup-only whole-tree sweep in CleanOrphans is left alone; that one is correct, because nothing is live yet. --- pkg/dind/bindstage.go | 30 ++++++++++++++++++++ pkg/dind/bindstage_linux.go | 19 +++++++++++++ pkg/dind/bindstage_linux_test.go | 47 ++++++++++++++++++++++++++++++++ pkg/dind/bindstage_other.go | 7 +++++ pkg/runtime/runtime.go | 26 ++++++++++++------ 5 files changed, 120 insertions(+), 9 deletions(-) diff --git a/pkg/dind/bindstage.go b/pkg/dind/bindstage.go index df089ad..116dbef 100644 --- a/pkg/dind/bindstage.go +++ b/pkg/dind/bindstage.go @@ -75,3 +75,33 @@ func SweepStagedBinds(dataDir string, log *slog.Logger) { } sweepStagedBinds(stagingRootDir(dataDir), log) } + +// SweepStagedBindsForJob removes the bind staging mounts and directory of ONE +// job. Safe to call while other jobs are running, which is what separates it +// from SweepStagedBinds. +// +// This exists because #187's periodic dead-container reaper called the +// whole-tree SweepStagedBinds on its sweeper tick. That function is documented +// STARTUP ONLY for a precise reason, and the reaper violated it: sweeping the +// root unmounts and deletes the staging directory of every LIVE job too. The +// running job's already-started containers keep working (their mounts are +// already in their namespaces), so nothing fails loudly — but the job's NEXT +// `docker run -v` cannot create its mountpoint, because ensureDirLocked has +// already latched m.ready and will not recreate the parent: +// +// docker: Error response from daemon: bind mount ... rejected: +// creating bind staging mountpoint /dind-binds//2: +// mkdir ...: no such file or directory +// +// Observed on the fleet's Linux amd64 nodes against ephpm's release workflow, +// which runs several `docker run -v "$PWD":/w` steps per job — binds 0 and 1 +// stage fine, a reaper tick lands, and bind 2 fails. Scoping the sweep to the +// container actually being reaped keeps the benefit (releasing the mounts that +// pin that container's rootfs snapshot, which is why the reaper swept at all) +// without touching anyone else's. +func SweepStagedBindsForJob(dataDir, jobID string, log *slog.Logger) { + if dataDir == "" || jobID == "" { + return + } + sweepStagedBindsForJob(stagingRootDir(dataDir), jobID, log) +} diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go index 81475f4..c249606 100644 --- a/pkg/dind/bindstage_linux.go +++ b/pkg/dind/bindstage_linux.go @@ -232,6 +232,25 @@ func stagingRootParent(jobDir string) string { return filepath.Dir(jobDir) } // sweepStagedBinds is the startup half of the lifecycle: it unmounts and // removes staging directories left by a previous ephemerd process. See // SweepStagedBinds for why leaked staging mounts are worse than untidy. +// sweepStagedBindsForJob unmounts and removes exactly one job's staging +// directory. Unlike sweepStagedBinds it never reads the root, so a live job's +// directory sitting next to this one is untouched. +func sweepStagedBindsForJob(root, jobID string, log *slog.Logger) { + dir := filepath.Join(root, jobID) + if _, err := os.Lstat(dir); err != nil { + return // this job never staged a bind, or it is already gone + } + if err := unmountTreeAndRemove(dir); err != nil { + if log != nil { + log.Warn("failed to sweep dind bind staging dir for reaped job", "path", dir, "error", err) + } + return + } + if log != nil { + log.Info("swept dind bind staging dir for reaped job", "path", dir) + } +} + func sweepStagedBinds(root string, log *slog.Logger) { entries, err := os.ReadDir(root) if err != nil { diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go index 7657b37..83dcbe3 100644 --- a/pkg/dind/bindstage_linux_test.go +++ b/pkg/dind/bindstage_linux_test.go @@ -726,3 +726,50 @@ func TestBindStaging_MountpointTypeMatchesSource(t *testing.T) { t.Errorf("staged file reads %q (err %v), want %q", body, err, "body") } } + +// TestSweepStagedBindsForJob_SparesOtherJobs pins the fix for the regression +// #187 introduced: its periodic reaper called the whole-tree +// SweepStagedBinds, which is documented STARTUP ONLY because it cannot tell a +// live job's staging dir from a dead one. On the fleet that deleted running +// jobs' directories, and their next `docker run -v` failed with +// "creating bind staging mountpoint /dind-binds//2: +// +// mkdir ...: no such file or directory" +// +// because ensureDirLocked had already latched m.ready and would not recreate +// the parent. No mounts are needed to prove the scoping: plain directories +// exercise the same path selection. +func TestSweepStagedBindsForJob_SparesOtherJobs(t *testing.T) { + data := t.TempDir() + root := stagingRootDir(data) + + dead := filepath.Join(root, "job-dead") + live := filepath.Join(root, "job-live") + for _, d := range []string{filepath.Join(dead, "0"), filepath.Join(live, "0")} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + + SweepStagedBindsForJob(data, "job-dead", discardLog()) + + if _, err := os.Lstat(dead); !os.IsNotExist(err) { + t.Errorf("reaped job's staging dir still present (err=%v), want removed", err) + } + // The whole point: a live job sitting next to the reaped one must survive, + // mountpoint subdirectory and all, or its next docker run cannot stage. + if _, err := os.Lstat(filepath.Join(live, "0")); err != nil { + t.Errorf("LIVE job's staging dir was destroyed by a scoped sweep: %v — "+ + "this is the #187 regression: the next `docker run -v` in that job "+ + "fails with mkdir ENOENT", err) + } +} + +// TestSweepStagedBindsForJob_ToleratesMissing: a job that never staged a bind +// (no -v in the whole job) must not make the reaper log or fail. +func TestSweepStagedBindsForJob_ToleratesMissing(t *testing.T) { + data := t.TempDir() + SweepStagedBindsForJob(data, "never-staged", discardLog()) + SweepStagedBindsForJob("", "job", discardLog()) + SweepStagedBindsForJob(data, "", discardLog()) +} diff --git a/pkg/dind/bindstage_other.go b/pkg/dind/bindstage_other.go index 7895c67..dc05b25 100644 --- a/pkg/dind/bindstage_other.go +++ b/pkg/dind/bindstage_other.go @@ -5,6 +5,7 @@ package dind import ( "log/slog" "os" + "path/filepath" ) // newBindStager returns the off-Linux stager. There is no mount(2) here and, @@ -35,3 +36,9 @@ func (passthroughStager) teardown() {} func sweepStagedBinds(root string, log *slog.Logger) { _ = os.RemoveAll(root) } + +// sweepStagedBindsForJob is the one-job form; same "nothing to unmount" +// reasoning as above. +func sweepStagedBindsForJob(root, jobID string, log *slog.Logger) { + _ = os.RemoveAll(filepath.Join(root, jobID)) +} diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index a860683..65d25c4 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -563,12 +563,11 @@ func (r *Runtime) ReapDeadContainers(ctx context.Context) error { } now := time.Now() - // Release leaked dind bind-staging mounts lazily — only once, and only - // if we actually find something to reap. Each leaked staging mount pins - // the runner rootfs it was bound from, which makes WithSnapshotCleanup - // fail with "device or resource busy"; CleanOrphans sweeps them for the - // same reason. Skipped entirely on an idle node with nothing to reap. - stagedSwept := false + // Each leaked staging mount pins the runner rootfs it was bound from, + // which makes WithSnapshotCleanup fail with "device or resource busy", + // so a container being reaped needs its staging dir released first. That + // happens per-container below, NOT once per pass over the whole tree — + // see the comment at the call site. for _, c := range containers { id := c.ID() log := r.cfg.Log.With("id", id) @@ -592,9 +591,18 @@ func (r *Runtime) ReapDeadContainers(ctx context.Context) error { continue } - if !stagedSwept { - dind.SweepStagedBinds(r.cfg.DataDir, r.cfg.Log) - stagedSwept = true + // Sweep ONLY this container's staging dir. The whole-tree + // SweepStagedBinds used to run here once per pass, which broke live + // jobs: it is documented STARTUP ONLY precisely because it cannot + // tell a live job's staging dir from a dead one, and deleting a + // running job's dir makes its NEXT `docker run -v` fail with + // "creating bind staging mountpoint ...: no such file or directory" + // (ensureDirLocked has latched m.ready and will not recreate the + // parent). The reaping motive is unchanged — a leaked staged mount + // pins this container's rootfs so its snapshot cannot be deleted — + // and scoping to the container being reaped keeps that. + { + dind.SweepStagedBindsForJob(r.cfg.DataDir, id, r.cfg.Log) } log.Info("reaping dead job container", "reason", "task dead past grace, snapshot pinned", "grace", grace)