Skip to content

fix(templatecenter): reclaim leaked loop devices instead of silently degrading ext4 builds #1295

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.

Sign up for GitHub

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

Open
dushulin wants to merge 1 commit into
base: master
Choose a base branch
from
+505 −17
Open

fix(templatecenter): reclaim leaked loop devices instead of silently degrading ext4 builds #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 153 additions & 17 deletions CubeMaster/pkg/templatecenter/image/disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"strconv"
"strings"
"sync"
"time"
)

const (
Expand Down Expand Up @@ -68,12 +69,57 @@ func createExt4ImageStreaming(ctx context.Context, source *PreparedSource, workD

// Use context.Background() for cleanup so it runs even after request cancellation.
cleanupCtx := context.Background()
var loopDevice string
var mounted bool
var mountStuck bool
var unmountOnce sync.Once
var detachOnce sync.Once
cleanup := func() {
unmountOnce.Do(func() {
_ = runCommand(cleanupCtx, "", "umount", "--", mountPoint)
detachLoop := func() {
if loopDevice == "" {
return
}
detachOnce.Do(func() {
// A lazy umount releases the mount asynchronously, so the detach can fail
// EBUSY for a short window afterwards; retry briefly. When even the lazy
// umount failed the mount is still there and every attempt is guaranteed to
// fail, so don't spend the budget.
attempts := 5
if mountStuck {
attempts = 1
}
var err error
for attempt := 0; attempt < attempts; attempt++ {
if attempt > 0 {
time.Sleep(200 * time.Millisecond)
}
if err = runCommand(cleanupCtx, "", "losetup", "--detach", "--", loopDevice); err == nil {

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PR description says: "before retrying, the code asks losetup what the device is backed by, and stops when it is no longer this build's image." That check is not here — the loop retries losetup --detach unconditionally on any error. Permanent errors (device already gone/ENXIO, EPERM, or a concurrent reclaim having freed the device) burn the full ~1s budget and end in a "device stays attached" warning that is misleading for an already-released device. Consider checking losetup --list for the device before spending the budget, or at least distinguishing EBUSY (retryable) from non-retryable errors.

return
}
}
// Be precise about how narrow recovery is from here: reclaim only picks a
// device up once its backing file has been unlinked — which happens on the
// build's failure path only — and the mount is gone. On the success path the
// image stays, so the device stays attached until the host reboots.
log.G(ctx).Warnf("losetup --detach %s failed; device stays attached, reclaimable later only if its backing file is unlinked and the mount released: %v", loopDevice, err)

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PR description says a device that still could not be detached "now fails the build", but this branch only logs a warning — detachLoop returns nothing, cleanup() (lines 110–125) never propagates it, and the step-5 cleanup() call is ignored. On the success path, a detach that exhausts the 5×200ms budget leaves the device attached with a live backing file forever: the build reports success, the image is kept, the backing file is never unlinked, and the reclaim backstop (which only detaches (deleted) candidates) cannot heal it. That is exactly the permanent leak this PR sets out to fix. TestStreamingSkipsDetachRetriesWhenMountStuck even asserts this build succeeds, contradicting the PR text. Consider returning an error from detachLoop/cleanup and propagating it from the step-5 call.

})
}
// A single cleanup closure, so the detach always runs *after* the umount:
// detaching a still-mounted device fails EBUSY and pins the loop device — and,
// once the backing file is unlinked, its disk space too — until the host
// reboots. Two separate defers would run in LIFO order and get this backwards.
cleanup := func() {
if mounted {
unmountOnce.Do(func() {
if err := runCommand(cleanupCtx, "", "umount", "--", mountPoint); err != nil {
log.G(ctx).Warnf("umount %s failed, retrying lazily: %v", mountPoint, err)
if lazyErr := runCommand(cleanupCtx, "", "umount", "-l", "--", mountPoint); lazyErr != nil {

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When the lazy umount also fails, the mount is still active and the 5-attempt detachLoop retry below is guaranteed to fail EBUSY on every try, adding ~1s of cleanup latency while never succeeding. The retry budget only helps the "lazy umount succeeded but the device is briefly busy" window — in this branch it is wasted. Consider skipping the detach retry here (log-and-return), since the device also cannot be recovered by reclaim while the mount persists.

@dushulin dushulin Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right — that branch can never win. The retry budget is now skipped there:

attempts := 5
if mountStuck {
    attempts = 1
}

mountStuck is set in the lazy-umount failure branch, so the guaranteed-EBUSY case makes exactly one attempt and logs, while the "lazy umount succeeded, device briefly busy" window keeps the full budget.

mountStuck = true

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same gap on the stuck-mount branch: mountStuck = true only shortens the detach retries; the build still returns nil. The PR body says the code "refuses an image whose resize2fs -M and pmem alignment would have run against a still-mounted filesystem". On a real host, resize2fs -M and the pmem-align truncate both fail against the still-mounted file, so the image is served unshrunk and unaligned and the microVM rejects it with PmemSizeNotAligned. This case needs to fail the build (or at least not serve the image), not log and continue — and TestStreamingSkipsDetachRetriesWhenMountStuck currently asserts the build succeeds here.

log.G(ctx).Warnf("lazy umount %s also failed, loop device will leak: %v", mountPoint, lazyErr)

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When both umount and umount -l fail, the mount is still attached, yet the build proceeds to step 6/7 and returns success. resize2fs -M and the pmem-alignment truncate run against a still-attached device, so the image can be left un-shrunk and not 2 MiB-aligned — downstream pmem boot may reject it ("PmemSizeNotAligned") — while the device + disk-space leak persists. Since BuildExt4's failure path unlinks the file (making the device a future reclaim candidate), failing the build here would self-heal the scenario; returning success keeps a possibly-bad image and an unreclaimable leak. This behavior is pre-existing but now detectable — worth deciding deliberately.

}
}
})
}

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After a successful umount -l, the loop device can remain busy for a short window, so this immediate losetup --detach can fail EBUSY and log "device stays pinned until reclaimed". That message overpromises for the success path: the backing .ext4 is never unlinked there, so the device is never a (deleted) candidate for reclaimOrphanLoopDevices and stays attached until host reboot — the exact leak class this PR is meant to eliminate. Consider a short bounded retry of the detach before declaring the device leaked, or wording the message to distinguish "reclaimable later" (backing file gone) from "permanent until reboot" (backing file still exists).

@dushulin dushulin Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed with a short bounded retry (5 attempts, 200 ms apart) inside the detach, so a lazy umount that has not finished releasing the mount no longer abandons the device:

detachOnce.Do(func() {
    // A lazy umount releases the mount asynchronously, so the detach can fail
    // EBUSY for a short window afterwards. Retry briefly: on the success path
    // the backing file is never unlinked, so a device abandoned here would not
    // become a (deleted) candidate for reclaim and would stay attached until
    // the host reboots.
    var err error
    for attempt := 0; attempt < 5; attempt++ {
        if attempt > 0 {
            time.Sleep(200 * time.Millisecond)
        }
        if err = runCommand(cleanupCtx, "", "losetup", "--detach", "--", loopDevice); err == nil {
            return
        }
    }
    log.G(ctx).Warnf("losetup --detach %s still failing, device stays attached until its backing file is removed and reclaimed: %v", loopDevice, err)
})

The retry only runs on a path that has already failed, so the happy path pays nothing and the worst case is bounded at ~800 ms. The WARN wording now says the device stays attached until its backing file is removed and reclaimed, instead of implying reclaim would pick it up as-is — which, as you point out, it will not while the file still exists.

detachLoop()
Comment on lines +110 to +122

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The substance of this fix — umount-before-detach ordering, the lazy-umount fallback, mountStuck short-circuiting the retry budget, and the detach retry loop — is untested. The new tests only cover orphanLoopCandidates/artifactStoreDirOf. image_test.go already provides installFakeCommand, so faking losetup/umount/mount would let a test assert (a) detach is attempted only after umount, (b) an EBUSY detach is retried, (c) mountStuck reduces attempts to 1, and (d) allocation is retried after a successful reclaim — locking in the exact ordering this PR fixes.

if err := os.RemoveAll(mountPoint); err != nil {
log.G(ctx).Warnf("cleanup mount point %s failed: %v", mountPoint, err)
}
Expand All @@ -86,25 +132,47 @@ func createExt4ImageStreaming(ctx context.Context, source *PreparedSource, workD
// parsed device path — corrupting the loopDevice string so every later mount
// and detach receives a garbage argument (mount fails "bad option", detach
// fails → loop device leaks).
losetupCmd := exec.CommandContext(ctx, "losetup", "--find", "--show", "--", ext4Path)
var losetupErr bytes.Buffer
losetupCmd.Stderr = &losetupErr
loopOut, err := losetupCmd.Output()
if err != nil {
return fmt.Errorf("losetup --find --show %s failed: %w: %s", ext4Path, err, strings.TrimSpace(losetupErr.String()))
findLoop := func() ([]byte, error) {
cmd := exec.CommandContext(ctx, "losetup", "--find", "--show", "--", ext4Path)
var stderr bytes.Buffer
cmd.Stderr = &stderr
out, err := cmd.Output()
if err != nil {
return nil, fmt.Errorf("losetup --find --show %s failed: %w: %s", ext4Path, err, strings.TrimSpace(stderr.String()))
}
return out, nil
}
loopDevice := strings.TrimSpace(string(loopOut))
detachLoop := func() {
detachOnce.Do(func() {
_ = runCommand(cleanupCtx, "", "losetup", "--detach", "--", loopDevice)
})
loopOut, err := findLoop()
if err != nil {
// Running out of loop devices can be self-inflicted: a detach that could not
// be completed (e.g. the umount above only succeeded lazily) keeps one pinned
// for the lifetime of the host, so the fast path would be lost permanently
// once enough builds have leaked. Reclaim devices whose backing file is
// already gone and retry before degrading to the much slower phase-1 build.
// This is best-effort and only meaningful for exhaustion; losetup reports
// every failure the same way, so other causes (cancellation, permissions)
// pay one extra `losetup --list` and then fail the retry as they should.
// It also only frees devices this process managed to attach, i.e. ones whose
// /dev node exists here — it cannot create the node that LOOP_CTL_GET_FREE
// returned, so a container whose /dev tmpfs is short of loop nodes and has no
// orphans to reclaim still degrades to phase-1 (see the deployment note).
if n := reclaimOrphanLoopDevices(cleanupCtx, artifactStoreDirOf(ext4Path)); n > 0 {

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The reclaim + retry fires on any losetup --find failure, not just device exhaustion — e.g. a request cancellation, a missing/odd backing file, or a permission error also triggers a losetup --list and may detach orphaned devices. It's safe (only (deleted) store devices are touched) and self-limiting, but two things are worth noting:

  • The retry reuses the request ctx, so if the original failure was a cancellation, the retry fails immediately no matter how many devices were reclaimed — the reclaim work is wasted.
  • If reclaim succeeds but the retry also fails, the returned error is the retry's, dropping the original error's context.

Gating on the underlying error type (exhaustion vs. others) is fragile since losetup errors come from parsing stderr, but a short comment clarifying this is best-effort-and-only-meaningful-for-exhaustion would help future readers.

@dushulin dushulin Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both points are real; the reclaim is deliberately not gated on the error type, so I documented that and stopped dropping the original error.

The call-site comment now says the retry is best-effort and only meaningful for exhaustion — other causes (cancellation, permissions) pay one extra losetup --list and then fail the retry as they should. And when the retry fails, both errors are surfaced:

retryOut, retryErr := findLoop()
if retryErr != nil {
    return fmt.Errorf("%w (retry after reclaiming %d device(s) also failed: %v)", err, n, retryErr)
}

%w keeps the original as the wrapped cause, since that is the one that says why allocation failed in the first place.

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This backstop does not self-heal the container scenario that motivates the PR. When exhaustion is caused by a container whose /dev tmpfs lacks the loopN node that LOOP_CTL_GET_FREE returned, losetup --detach fails ENOENT (the node does not exist) — both here and inside reclaimOrphanLoopDevices, which logs at Debug and skips. So in a container the retry also fails and the build still permanently degrades to phase‑1. Reclaim only recovers devices on a host with real, present /dev/loopN nodes. Acknowledged in the PR body as a deployment concern, but worth a comment here so the backstop's scope is not over-read.

@dushulin dushulin Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Half of this is right, but the conclusion is too pessimistic — the reclaim does recover the observed container case, and here is why.

The devices reclaim detaches are ones this process previously attached successfully, so their /dev/loopN nodes exist inside the container by construction; the ENOENT only ever applies to the node LOOP_CTL_GET_FREE just handed back. And because LOOP_CTL_GET_FREE returns the lowest free index, detaching those leaked low-index devices makes the next losetup --find return one of them again instead of climbing past the container's highest node. That is exactly the situation I hit: loop0..loop7 all attached, none mounted, so --find returned /dev/loop8 → ENOENT; after freeing them, allocation succeeded on a low index.

What reclaim genuinely cannot fix is the case with no orphans — eight builds legitimately holding loop0..loop7, where the missing node is the only problem. Added a comment at the call site saying just that:

// It also only frees devices this process managed to attach, i.e. ones whose
// /dev node exists here — it cannot create the node that LOOP_CTL_GET_FREE
// returned, so a container whose /dev tmpfs is short of loop nodes and has no
// orphans to reclaim still degrades to phase-1 (see the deployment note).

log.G(ctx).Warnf("loop device allocation failed (%v); reclaimed %d orphaned loop device(s), retrying", err, n)
retryOut, retryErr := findLoop()
if retryErr != nil {
return fmt.Errorf("%w (retry after reclaiming %d device(s) also failed: %v)", err, n, retryErr)
}
loopOut = retryOut
} else {
return err
}
}
defer detachLoop()
loopDevice = strings.TrimSpace(string(loopOut))

if err := runCommand(ctx, "", "mount", "-o", "nosuid,noexec,nodev,noatime", "--", loopDevice, mountPoint); err != nil {
detachLoop() // explicit detach on mount failure (defer will be a no-op via sync.Once)
return fmt.Errorf("mount loop device %s: %w", loopDevice, err)
}
mounted = true

// 4. Stream export directly into the mounted ext4.
if source.ExportMode == ExportModeNative {
Expand Down Expand Up @@ -133,7 +201,7 @@ func createExt4ImageStreaming(ctx context.Context, source *PreparedSource, workD
}
}

// 5. Unmount (via cleanup).
// 5. Unmount and detach (via cleanup).
cleanup()

// 6. Shrink the ext4 filesystem to minimum size (best-effort).
Expand Down Expand Up @@ -176,6 +244,74 @@ func createExt4ImageStreaming(ctx context.Context, source *PreparedSource, workD
return nil
}

// artifactStoreDirOf returns the artifact store root for an ext4 image path,
// mirroring the <store>/<artifact>/<artifact>.ext4 layout BuildExt4 constructs
// (including when it falls back to ArtifactFallbackStoreRootDir, so a fallback
// build only ever reclaims fallback-store orphans). A path of another shape just
// yields a prefix that matches nothing in orphanLoopCandidates, i.e. reclaim
// no-ops rather than reaching outside the store. Nested store roots are the one
// case where the scope is wider than the build's own store: the shallower root's
// prefix also covers the deeper one's orphans, still bounded to devices whose
// backing file is already gone.
func artifactStoreDirOf(ext4Path string) string {

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dir(Dir(ext4Path)) implicitly encodes the <store>/<artifactID>/<artifactID>.ext4 layout constructed in BuildExt4 — correct today, but the coupling is only visible by cross-referencing BuildExt4. If a caller ever passes a different-depth or relative path, the prefix match in orphanLoopCandidates silently no-ops (safe, but silent). Two suggestions:

  • Add a one-line comment here tying this to the BuildExt4 layout, and/or guard on filepath.IsAbs(ext4Path) so a non-absolute path doesn't silently disable reclaim.
  • Note the fallback-store interaction: when a build runs on ArtifactFallbackStoreRootDir, only fallback-root orphans are reclaimed, so leaks in the primary store go un-reclaimed until a primary-store build hits exhaustion.

@dushulin dushulin Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the coupling to the doc comment rather than an IsAbs guard, since a wrong-shaped path already fails safe — it produces a prefix that matches nothing, so reclaim no-ops instead of reaching outside the store:

// artifactStoreDirOf returns the artifact store root for an ext4 image path,
// mirroring the <store>/<artifact>/<artifact>.ext4 layout BuildExt4 constructs
// (including when it falls back to ArtifactFallbackStoreRootDir, so a fallback
// build only ever reclaims fallback-store orphans). A path of another shape just
// yields a prefix that matches nothing in orphanLoopCandidates, i.e. reclaim
// no-ops rather than reaching outside the store.

The fallback-store asymmetry you describe is accurate and I left the behaviour as is: each store's own exhaustion is what makes that store's orphans worth reclaiming, and widening the scan to both roots would have a build detaching devices backed by a store it is not using.

return filepath.Dir(filepath.Dir(ext4Path))
}

// orphanLoopCandidates parses the output of
// `losetup --list --noheadings --raw --output NAME,BACK-FILE` and returns the
// devices whose backing file lives under storeDir and has already been unlinked
// (losetup marks those with a trailing " (deleted)").
//
// Requiring the unlink is what makes the reclaim safe against a build running
// concurrently: the image file is only unlinked once a build has given up on it,
// so a live build's device can never become a candidate — not even in the
// attach→mount window, where the device is not yet in the mount table and the
// kernel's EBUSY refusal would not protect it either.
func orphanLoopCandidates(losetupList, storeDir string) []string {
if storeDir == "" || storeDir == "/" {
return nil
}
prefix := strings.TrimSuffix(storeDir, "/") + "/"
var names []string
for _, line := range strings.Split(losetupList, "\n") {
name, backing, ok := strings.Cut(strings.TrimSpace(line), " ")
if !ok || name == "" {
continue
}
backing = strings.TrimSpace(backing)
if !strings.HasSuffix(backing, " (deleted)") {
continue
}
backing = strings.TrimSpace(strings.TrimSuffix(backing, " (deleted)"))
if strings.HasPrefix(backing, prefix) {
names = append(names, name)
}
}
return names
}

// reclaimOrphanLoopDevices detaches loop devices whose backing file under
// storeDir has already been unlinked, i.e. devices no build can still be using.
// Returns how many were freed.
func reclaimOrphanLoopDevices(ctx context.Context, storeDir string) int {
out, err := exec.CommandContext(ctx, "losetup", "--list", "--noheadings", "--raw",
"--output", "NAME,BACK-FILE").Output()
if err != nil {
log.G(ctx).Warnf("losetup --list for orphan reclaim failed: %v", err)
return 0
}
reclaimed := 0
for _, name := range orphanLoopCandidates(string(out), storeDir) {
if err := runCommand(ctx, "", "losetup", "--detach", "--", name); err != nil {

@cubesandboxbot cubesandboxbot Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Each orphan candidate gets exactly one losetup --detach with no retry. A (deleted) orphan whose lazy umount is still draining (the last reference still closing) fails EBUSY here and is skipped; the single findLoop retry then also fails, and the build degrades to a full phase-1 rebuild even though the device would have freed within a second or two.

Compounding this, both outcomes are logged at Debugf, which production drops (the level comes from conf.Level, normally INFO+). When reclaim finds candidates but frees none, the operator sees nothing except the phase-1 fallback warning — the reclaim attempt itself is invisible.

Suggest: a short bounded retry on the orphan detach, and warn-level logging when candidates existed but none were freed, so a near-miss reclaim is both self-healing and diagnosable.

log.G(ctx).Debugf("orphan loop device %s not reclaimable (likely still in use): %v", name, err)
continue
}
log.G(ctx).Debugf("reclaimed orphaned loop device %s", name)
reclaimed++
}
return reclaimed
}

// pipeExportToDir streams the docker export of a container directly into a target
// directory via tar -xf -.
func pipeExportToDir(ctx context.Context, containerID, destDir string) error {
Expand Down
Loading
Loading