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

[dushulin]

Problem

createExt4ImageStreaming (the loop-mount fast path from #472, hardened in #958) allocates a loop device, mounts it, and relies on two deferred cleanups to release it. Both cleanups discard their errors:

_ = runCommand(cleanupCtx, "", "umount", "--", mountPoint)
...
_ = runCommand(cleanupCtx, "", "losetup", "--detach", "--", loopDevice)

They are also registered as two separate defers — the umount first, the detach second — so LIFO ordering runs the detach before the umount. A detach attempted while the filesystem is still mounted fails with EBUSY, and nothing is logged. The consequences are permanent:

1. The loop device stays attached for the lifetime of the host.
2. If the backing .ext4 is unlinked afterwards (the build's own failure cleanup does exactly that), the loop device keeps the inode alive and its disk space is never reclaimed — the same class of permanent disk leak as [Bug Report] Template delete leaks 1.1GB rootfs ext4 in cubebox_os_image (permanent disk leak, root partition exhaustion risk) #822. losetup -a shows these as ... .ext4 (deleted).
3. Once enough devices have leaked, losetup --find fails and every subsequent build falls back to phase-1 behind a single WARN at ext4.go:141. The fast path never returns without operator intervention, and the only symptom is that template creation has become drastically slower.

Containerized deployments reach (3) far earlier than the kernel's loop limit would suggest. A container's /dev is a tmpfs whose node set is fixed when the container starts — typically loop-control plus loop0..loop7 — while LOOP_CTL_GET_FREE hands out node-global indexes. As soon as it returns 8 or above, that node does not exist inside the container:

losetup: /.../rfs-XXXXXXXX.ext4: failed to set up loop device: No such file or directory

Observed in practice: 8 devices attached, none of them mounted, three with (deleted) backing files, and every template build after that point taking the slow path.

Change

• The detach now runs from the same cleanup closure as the umount, immediately after it, so it can no longer be ordered before the umount and hit EBUSY. This is the primary fix.
• umount failure is logged and retried lazily (umount -l), so a busy mount point no longer cascades into a permanently pinned device.
• losetup --detach is retried briefly (5 attempts, 200 ms apart) before giving up, so a lazy umount that has not finished releasing the mount does not abandon the device, and the final failure is logged instead of discarded.
• A device that still could not be detached now fails the build instead of being reported as success. This is the one leak the backstop below cannot heal on its own: a successful build keeps its image, so the backing file is never unlinked and the device can never become a (deleted) reclaim candidate. Returning the error makes BuildExt4 unlink the image (ext4.go:141-143) — turning the pinned device into a candidate for the next allocation failure — and rebuild via phase-1, so no image is lost. It also refuses an image whose resize2fs -M and pmem alignment would have run against a still-mounted filesystem (the stuck-mount case), which would otherwise serve an unshrunk, unaligned image that the microVM rejects with PmemSizeNotAligned.
• A detach that fails only because the device is already gone is not treated as a leak: before retrying, the code asks losetup what the device is backed by, and stops when it is no longer this build's image. Attaches with autoclear semantics are released by the umount itself, and without this check every such host would burn the whole backoff budget and then fail every build into phase-1. The check queries losetup rather than matching its error text, which is localized.
• As a backstop for devices already leaked (by an earlier build, or by a lazy umount that had not completed yet), reclaimOrphanLoopDevices runs when losetup --find fails and retries once, so exhaustion self-heals instead of permanently degrading every later build.

Reclaim is narrow by construction: a device is only detached when its backing file lives under the artifact store and has already been unlinked (losetup --list reports it as (deleted)). A build unlinks its image only after it has given up on it, so a build in flight elsewhere 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. Devices that still cannot be detached are logged at debug level and skipped.

When the retry after a reclaim also fails, both errors are surfaced (%w on the original) so the reason allocation failed in the first place is not lost.

The happy path is unchanged — no additional command runs unless a cleanup fails or allocation is already exhausted.

Note for containerized deployments

Even with zero leaks, a container that only has loop0..loop7 can exhaust them with eight concurrent builds. Making more nodes available is a deployment concern (pre-create them, or share the host /dev) and is out of scope here; this change makes the failure recoverable and diagnosable rather than silent and permanent.

Tests

New disk_test.go:

• Behaviour of the fix itself, driven through createExt4ImageStreaming with fake losetup/mount/umount on PATH that record their argv: the umount always precedes the detach (reverting the ordering fails this test), a busy detach is retried until it succeeds, the plain-umount-fails/lazy-umount-succeeds case releases the device on the retry, a mount point that survives even the lazy umount stops after one detach attempt instead of spending the whole budget, a failed mount still detaches the allocated device (which now happens only through the deferred cleanup), and an exhausted losetup --find is retried after the orphan reclaim frees a device — while an allocation failure with no orphan to reclaim returns the original error with no retry, and one whose retry fails too reports both failures.
• losetup --list parsing and store-prefix matching: the (deleted) suffix is required (a live backing file under the store is left alone), devices outside the store, sibling directories sharing a prefix (storage-backup vs storage), malformed and blank lines, a backing path containing spaces, and a / store dir (refused rather than detaching everything).

gofmt, go build ./pkg/..., go vet ./pkg/templatecenter/image/ and the package tests (-short, as CI runs them) all pass.

[cubesandboxbot]

Moderate — the deferred detach still runs before the deferred unmount on error returns, so the direct detach never succeeds on the common streaming-failure path.

defer cleanup() (the umount) is registered at line 89 and defer detachLoop() at line 129. Defers run LIFO, so on any error return after the mount succeeds (native/docker streaming failure, docker create/export failure, or post-export hook failure at lines 110–134), detachLoop() executes while the loop device is still mounted → losetup --detach fails with EBUSY, and sync.Once prevents any retry once cleanup() finally unmounts. The device then stays pinned until a later build hits allocation failure and the reclaim runs — the newly-logged detach error is real, but the detach itself effectively never succeeds on the error path.

Since this PR is specifically about not silently pinning loop devices, consider registering defer detachLoop() before defer cleanup() (or folding the detach into cleanup after the umount) so unmount precedes detach on error returns too. The reclaim would then be a backstop rather than the primary fix for the error path.

[dushulin]

Correct, and this was the actual cause of the leak I observed — thanks. The five pinned devices whose backing files were still live had no other explanation: their umount had succeeded, so only a detach that ran before it could have failed.

Fixed by folding the detach into the same closure as the umount, so ordering is explicit rather than a consequence of defer LIFO:

detachLoop := func() {
    if loopDevice == "" {
        return
    }
    detachOnce.Do(func() { ... losetup --detach ... })
}
cleanup := func() {
    if mounted {
        unmountOnce.Do(func() { ... umount, then umount -l ... })
    }
    detachLoop()
    os.RemoveAll(mountPoint)
}
defer cleanup()

loopDevice and mounted are now declared before cleanup and assigned once the allocation and the mount succeed. The mounted guard keeps the mount-failure path from emitting a spurious "umount failed" WARN, and the loopDevice == "" guard covers the case where allocation itself failed. The standalone defer detachLoop() and the explicit detachLoop() in the mount-failure branch are both gone — one defer cleanup() now covers every return.

This makes the reclaim a backstop for devices leaked by earlier builds rather than the primary fix, which is how it should have been in the first place.

[cubesandboxbot]

Moderate — a device in the attach→mount window of a concurrent build can be pulled out from under it.

The "cannot be pulled out from under" claim holds for mounted devices (EBUSY), but not for the window between another build's losetup --find (device attached, backing file live) and its mount. Because orphanLoopCandidates matches live backing files — the (deleted) suffix is optional — such a device is an indistinguishable candidate. Reclaim detaches it, and the innocent build's mount then fails with ENOENT, silently degrading it to phase-1: exactly the failure mode this PR is meant to eliminate. This is most likely to trigger in the PR's own motivating scenario (small container loop pool + concurrent builds), since reclaim only runs when allocation is already exhausted.

Note that a live build's backing file is only unlinked in BuildExt4's failure cleanup after createExt4ImageStreaming returns, so a (deleted) backing file always denotes a finished/failed build, whereas a non-(deleted) store-backed device is either an in-flight build or a rare lazy-umount leak. Consider restricting reclaim to (deleted) backing files (and/or an explicit liveness check) to close the race while still catching the #822-style disk-leak orphans.

[dushulin]

Agreed — the EBUSY argument does not cover the attach→mount window, and a device is not in the mount table there.

Restricted to (deleted) entries only:

backing = strings.TrimSpace(backing)
if !strings.HasSuffix(backing, " (deleted)") {
    continue
}
backing = strings.TrimSpace(strings.TrimSuffix(backing, " (deleted)"))

Your reasoning about why this is safe is the reasoning I should have used: the image file is unlinked only by the failure cleanup that runs after createExt4ImageStreaming returns, so (deleted) implies the build is over. A leaked device whose file still exists is now left alone; it costs one loop slot, which the ordering fix above prevents from happening in the first place.

Tests updated: the "existing backing file under the store" case now expects no candidates, and I added a (deleted) path outside the store and a backing path containing spaces (the parser cuts on the first space, which only has to hold for NAME).

Also took the logging note — per-device reclaim is Debugf now, with the single summary Warnf staying at the call site.

[cubesandboxbot]

AI-generated review — not reviewed or approved by a human.

Summary

The diagnosis is excellent and the primary fix is correct: the two separate defers ran the loop detach before the umount, the detach hit EBUSY, and the error was discarded — pinning devices (and, once the backing .ext4 was unlinked, their disk space) until host reboot. Collapsing umount+detach into a single cleanup closure fixes the ordering, the lazy-umount + bounded detach retry is a reasonable mitigation for the busy window, the (deleted)-only orphan reclaim is a sound safety bound, and the tests are thorough and cleverly drive the real code through fake binaries on PATH.

However, the code does not implement two behaviors the PR description explicitly claims, and the tests enshrine the discrepancy:

1. "A device that still could not be detached now fails the build." — It does not. detachLoop/cleanup only log and never return an error, so the step-5 cleanup() cannot fail the build. On the success path, a detach that exhausts the 5×200 ms budget (lazy umount still releasing, or a stuck mount) leaves the device attached with a live backing file forever — the exact permanent leak this PR sets out to close, and the one the reclaim backstop explicitly cannot heal (the backing file is never unlinked on success).
2. "It also refuses an image whose resize2fs -M and pmem alignment would have run against a still-mounted filesystem." — It does not. The stuck-mount case (mountStuck = true) only shortens the detach retries; the build still returns nil.

TestStreamingSkipsDetachRetriesWhenMountStuck explicitly asserts the build succeeds with a stuck mount and a failed detach, which is the opposite of the PR body's claims.

Findings

F1 (High) — Detach failure is logged, not propagated; the success path can still leak permanently

disk.go:103 (the detach-failure Warnf) is the terminal action of the retry loop: detachLoop returns nothing, cleanup() (disk.go:110-125) never surfaces it, and the step-5 cleanup() call (disk.go:202) ignores any failure. So when all five detach attempts fail on an otherwise successful build, createExt4ImageStreaming returns nil, BuildExt4 keeps the image, the backing file is never unlinked, and the device stays attached with a live backing file until the host reboots. The reclaim backstop is deliberately bounded to (deleted) backing files, so it can never pick this device up. This is precisely the leak the PR says it eliminates by failing the build ("Returning the error makes BuildExt4 unlink the image (ext4.go:141-143) … turning the pinned device into a candidate for the next allocation failure"). To match the description, detachLoop/cleanup need to return an error and the step-5 call needs to propagate it.

F2 (Medium) — Stuck mount still returns success and can serve an unshrunk, unaligned image

When even umount -l fails (disk.go:116, mountStuck = true), the code proceeds and the build returns nil. On a real host the subsequent resize2fs -M and the pmem-align truncate run against a still-mounted filesystem: the shrink fails, the image is served at its unshrunk, unaligned size, and the microVM rejects it with PmemSizeNotAligned. The PR body says the code "refuses" this case; it does not.

F3 (Low) — The PR-described "device already gone" detach check is not implemented

The body says: "before retrying, the code asks losetup what the device is backed by, and stops when it is no longer this build's image." The retry loop (disk.go:91-98) instead retries losetup --detach unconditionally on any error, so permanent errors (device already gone/ENXIO, EPERM, or a concurrent reclaim having freed the device) burn the full ~1 s budget and end in a "device stays attached, reclaimable later…" warning that is misleading for an already-released device.

F4 (Low) — Reclaim can be triggered by non-exhaustion failures, and the retry reuses the cancelled context

findLoop fails for any reason (including request cancellation and permissions), and the code then runs the orphan reclaim and retries findLoop() (disk.go:159-165). On cancellation the retry is guaranteed to fail again (the same cancelled ctx is reused), yet the reclaim side effect (detaching any (deleted) orphans) still happens. The orphan detach is safe and the original error is preserved via %w, but the "retry after reclaiming N device(s) also failed" message will be misleading when the real cause was a cancellation.

F5 (Info) — Narrow concurrent reclaim / detach-retry race on a reused index

detachLoop retries for up to ~1 s. If, in that window, another build's reclaim detaches the same device (possible once the backing file is unlinked) and a third build's losetup --find reuses the freed index, the first build's next losetup --detach could detach the third build's freshly-allocated device, causing a transient mount failure and a phase-1 fallback. The window is small and the consequence is recoverable, but the missing "already gone" check (F3) is what would close it.

F6 (Info) — artifactStoreDirOf comment overstates the "matches nothing" claim

For the actual BuildExt4 layout (<store>/<artifact>/<artifact>.ext4) the function is correct. But the comment's claim that "a path of another shape just yields a prefix that matches nothing" is not quite true: a two-level path (e.g. <store>/<artifact>.ext4) yields the grandparent as the store dir, widening the reclaim scope. The (deleted) bound keeps it safe, but the comment could be tightened.

Tests

The test strategy is strong: fake truncate/mkfs.ext4/mount/umount/losetup/resize2fs on PATH, argv trace files, and gomonkey stubs for canUseLoopMount and StreamRegistryToDir. TestStreamingUnmountsBeforeDetach guards the actual bug, the detach-retry tests cover the lazy-umount window, the mount-failure test verifies the device is still detached, and the orphanLoopCandidates table covers the tricky parsing cases (blank/malformed lines, sibling prefixes, (deleted) requirement, root store dir, spaces in paths).

The one test that enshrines the spec gap is TestStreamingSkipsDetachRetriesWhenMountStuck: it asserts the build succeeds with a stuck mount and a failed detach, which is the opposite of the PR description's "fails the build" / "refuses an image" claims. If the fail-the-build behavior is intended, this test (and TestStreamingDetachesAfterLazyUmount, which succeeds after a delayed detach) should be updated to assert an error.

Bottom line

The ordering fix and the reclaim backstop are worth shipping, but the PR as written does not deliver the guarantee its description promises on the success path. Recommend resolving the mismatch — either propagate the detach failure from step 5 (matching the description) or revise the description and add a test documenting the residual leak window.

[cubesandboxbot]

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]

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.

[cubesandboxbot]

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]

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]

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]

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.

[cubesandboxbot]

The warning overpromises recovery. "Until its backing file is removed and reclaimed" only holds when the build later fails and BuildExt4 unlinks the file (ext4.go:143). On the success path the backing file is never removed, so the device never becomes a (deleted) reclaim candidate and stays attached until host reboot; and when the lazy umount also failed (mount still present), reclaim's own losetup --detach will hit EBUSY and skip it too. Consider stating the actual recovery conditions (only when the backing file is later unlinked by the failure path and the device is no longer mounted).

[dushulin]

Reworded to state the actual conditions:

losetup --detach %s failed; device stays attached, reclaimable later only if its backing file is unlinked and the mount released

with a comment above it spelling out that the unlink only happens on the build's failure path, so on the success path the device is attached until the host reboots.

[cubesandboxbot]

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]

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.

[cubesandboxbot]

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]

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).

[cubesandboxbot]

Residual leak on the success path: if umount only succeeds lazily and the loop device stays busy longer than the fixed 5×200 ms budget, this detach fails — and because the build succeeded, the backing image is kept and never unlinked, so reclaimOrphanLoopDevices (which only ever picks up (deleted) backings) can never free it. The device stays attached until host reboot. 800 ms is a thin budget for a lazily-released filesystem whose last reference is dropped by an external process; consider a longer/backoff budget.

[cubesandboxbot]

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.

[dushulin]

Thanks — pushed a follow-up for findings 1 and 2.

1. The core fix is untested (medium) — fixed. disk_test.go now drives createExt4ImageStreaming with fake losetup/mount/umount/truncate/mkfs.ext4/resize2fs on PATH, each recording its argv to a trace file:

• TestStreamingUnmountsBeforeDetach asserts the umount precedes the detach. Verified it bites: moving detachLoop() to the top of the cleanup closure (i.e. reintroducing the original LIFO ordering) fails it.
• TestStreamingRetriesDetachAfterBusy — a detach that fails EBUSY twice is retried until it succeeds (3 attempts).
• TestStreamingSkipsDetachRetriesWhenMountStuck — when both the plain and the lazy umount fail, exactly one lazy umount and one detach are attempted.
• TestStreamingReclaimsOrphansOnAllocationFailure — an exhausted losetup --find triggers the reclaim of the (deleted)-backed device and one retry of the allocation.

2. Success-path detach failure is an unreclaimable leak (low) — agreed, and the point that this one is genuinely unreclaimable is what makes the thin budget matter. The retry is now exponential (200 ms doubling, 6 attempts, ~6.2 s total) instead of a fixed 5×200 ms, and the comment says why the budget is spent here rather than elsewhere. The mountStuck short-circuit is unchanged, so a mount that survived even the lazy umount still costs one attempt.

3. losetup --list parsing is coupled to the raw output format (low) — acknowledged. I can't validate against the deployment's util-linux from here (the environment I have available has no loop nodes at all, so nothing to list). The behaviour is the fail-safe one you describe: an unrecognised format yields no candidates, so reclaim no-ops and the build degrades exactly as it does today. Worth revisiting if the parser is ever asked to do more than "is this backing file under the store and already unlinked".

4. mounted = true assumes a failed mount leaves the device unmounted (low, pre-existing) — agreed on the analysis, leaving it as is. Setting the flag before mount would make the partial-failure case slightly better and the common case worse: on an ordinary mount failure both umounts would fail ("not mounted"), which sets mountStuck and cuts the detach to a single attempt — trading a rare partial failure for a regression on the frequent one. The flag only makes an assumption explicit that the previous code made silently.

On the note about reclaim running for non-exhaustion failures: kept deliberately, for the reason you list — one extra losetup --list on a path that is already failing.

[cubesandboxbot]

This is the one leak the new reclaim backstop can never recover, yet it is reported as success. The "reclaimable later" promise in this warning does not hold on the success path: by the time this runs the umount has already succeeded, so the image is valid, the build returns nil, and BuildExt4 keeps the file (keepStoreDir = true) — its backing file is therefore never unlinked and the device can never become a (deleted) candidate. It stays pinned to a live file until the host reboots, and the retry budget is already exhausted here, so there is no "give it more time" fallback.

Consider propagating the error instead: a detach failure after a successful umount means the image itself is fine, but returning an error would make BuildExt4's failure path unlink the file, turning this exact scenario into a future (deleted) reclaim candidate. The tradeoff (discarding a valid image on a rare cleanup failure) seems worth it for the class of permanent leak this PR exists to eliminate.

[cubesandboxbot]

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.

[dushulin]

Findings 1, 2, 3 and 4 are addressed; 5 left as is with reasoning.

1 (medium) + 2 (low–medium) — you were right that reporting success is what makes these two unhealable, and they turn out to have the same fix. A detach that ultimately failed now fails the build:

if detachErr != nil {
    return fmt.Errorf("release loop device %s: %w", loopDevice, detachErr)
}

The tradeoff you flagged ("discarding a valid image") does not actually apply: BuildExt4 treats a streaming error as a fallback, not a build failure — it unlinks the image and rebuilds through phase-1 (ext4.go:139-143). So the outcome is a slow build instead of a fast one, plus the pinned device becoming a (deleted) candidate that the next allocation failure reclaims. Nothing is lost, and the leak becomes self-healing.

The same return also covers finding 2: the stuck-mount case is exactly a case where the detach keeps failing, so the build no longer proceeds to resize2fs -M and the pmem alignment against a mounted device, and can no longer serve an unshrunk, unaligned image. The test asserts resize2fs does not run in that case.

3 (low, test gaps) — added two of the three:

• TestStreamingDetachesWhenMountFails — a failing mount still detaches the allocated device (and attempts no umount), which is the regression guard for the removed explicit detachLoop().
• TestStreamingDetachesAfterLazyUmount — plain umount fails, umount -l succeeds, the first detach hits EBUSY and the retry releases the device. This is the combination the budget exists for; TestStreamingRetriesDetachAfterBusy keeps covering the retry loop itself.

I skipped the error-path ordering test: after the consolidation there is only one cleanup closure and both paths run the identical code, the only difference being whether it is reached via the explicit call or the defer. A regression in the ordering is caught by TestStreamingUnmountsBeforeDetach regardless of which path invoked it. Happy to add it if you would rather have the path itself pinned.

4 (low) — PR description fixed (6 attempts, exponential backoff, ~6.2 s).

5 (low, quoted --raw output) — leaving the parse as is. It fails safe in the direction that matters (an unparsed line is skipped, never wrongly detached), and a tolerant parse would have to guess at a quoting convention I cannot verify against the deployed util-linux from here. If losetup ever starts quoting, the symptom is that reclaim stops finding candidates and builds degrade exactly as they do today — visible in the existing WARN, not silent corruption.

[cubesandboxbot]

Failing the build assumes a failed losetup --detach always means the device is still attached. That isn't guaranteed: losetup --find --show -- <file> sets LO_FLAGS_AUTOCLEAR by default on util-linux ≥ 2.24, so on such a host a clean umount releases the last reference and the kernel auto-detaches the device before this code runs — losetup --detach then fails with "No such device or address" (ENXIO) even though nothing leaked. The result here would be 6.2s of pointless backoff retries (lines ~100-103) followed by a failed build → phase-1 fallback on every build, which is the same silent-permanent-degradation failure mode this PR is trying to remove, now on autoclear-enabled hosts.

Failing the build is the right policy only when the device is genuinely still attached (EBUSY). Consider treating an "already gone" detach — e.g. error text matching "No such device or address" / "No such file or directory" — as success, since the goal of releasing the device is already achieved. This may not reproduce in the environment described in the PR (the observed leaks imply autoclear is inactive there), but this code will run in many deployments, and the old code tolerated this case by discarding the error.

[cubesandboxbot]

stillOurs() treats any losetup --list failure as "device already gone" — but it's only safe to stop retrying when the device is actually no longer ours. If the first losetup --detach fails with EBUSY (e.g. the lazy-umount window) and losetup --list then fails for a transient reason that is not "device does not exist" (permission, EIO, a briefly inconsistent loop state), this returns false, the retry loop exits without setting detachErr, and the build reports success while the loop device stays attached. Because the build succeeded, the backing file is never unlinked, so the device can never become a (deleted) reclaim candidate — the exact permanent, silent leak this PR is designed to eliminate, just on a different trigger.

The intended "already released by the umount" case is distinguishable by which error the --list/--detach returns (device absent: ENOENT/ENODEV/no-such-device), rather than by "any error at all". Suggest checking the error for the device-gone case before returning false here, and letting an ambiguous error fail the detach (set detachErr) so the build falls back to phase-1 and the image gets unlinked.

[dushulin]

Thanks — the medium is fixed, and both test gaps are now covered.

Medium: a failed detach is not always a leak — agreed, and this was the right catch: with the new fail-the-build policy, a detach that fails because the device is already gone would turn a healthy build into a permanent phase-1 fallback, which is exactly the failure mode this PR set out to remove.

I implemented the tolerance by asking losetup instead of matching its error text:

stillOurs := func() bool {
    out, err := exec.CommandContext(cleanupCtx, "losetup", "--noheadings", "--raw",
        "--output", "BACK-FILE", "--list", "--", loopDevice).Output()
    if err != nil {
        return false
    }
    return strings.HasPrefix(strings.TrimSpace(string(out)), ext4Path)
}

The loop now stops immediately — no further retries, no detachErr, build succeeds — as soon as the device is no longer attached to this build's backing file. The reason for not keying on No such device or address / No such file or directory is that those strings come from strerror(3) and are localized: on a host with a non-English locale the match silently stops working and we are back to failing healthy builds, with no signal that the special case has stopped firing. Querying the device state covers the same condition without depending on the message, and it also covers the case where the device was reused by something else in the meantime (attached, but no longer ours), which a text match would not.

One thing I could not confirm and did not want to overclaim: I don't believe losetup --find --show sets LO_FLAGS_AUTOCLEAR by itself — my reading is that mount -o loop is what sets autoclear on the device it allocates, and a bare losetup attach stays until it is detached. But the tolerance does not depend on which is true: any cause of "the device is already released" (autoclear, an unrelated detach, a reused device) reaches the same check, so the behaviour is right either way.

Stuck-mount devices are never reclaimable (low) — accepted as stated, and the bound is worth having in writing: the backstop heals only the unmounted case. A mount that survived both umount and umount -l keeps the device pinned until reboot; all this fix can do there is refuse to report success (so the image is not served unshrunk and unaligned) and log it.

Test-coverage gaps (low) — both added:

• TestStreamingAllocationFailureWithoutOrphans — losetup --list returns nothing, so the n == 0 path returns the original allocation error with no retry attempted (asserted on the trace, exactly one losetup --find). This is the path every non-exhaustion cause takes.
• TestStreamingReportsBothAllocationFailures — a candidate is reclaimed but the retry fails too; the error must contain both the original message and retry after reclaiming 1 device(s), so the reclaim cannot hide why the fast path was lost.

Also added last round: TestStreamingToleratesAlreadyReleasedDevice, which asserts exactly one detach attempt and a successful build when the device is gone.

Backoff blocking the request goroutine — agreed, no change: it is bounded, it only runs when a detach has already failed, and the happy path runs no extra commands.

[cubesandboxbot]

return false here conflates two very different situations: the device genuinely being gone (autoclear released it after the clean umount — correct to stop) and the probe itself failing (losetup --list exits non-zero for a reason other than "device absent"). Note this check is only reached after a losetup --detach has already failed, so at this point the device is more likely still attached to this build's image than not.

The problem: on the probe-error branch the loop treats the device as released — no retry, no warning — and because the build then reports success, the image is kept, so the device can never become a (deleted) reclaim candidate. That is the exact permanent-leak class this PR exists to fix, just reached through a different failure mode.

The two cases are distinguishable by exit status: a successful probe reporting a different BACK-FILE is a safe "not ours"; a failed probe is ambiguous. Suggest logging a warning and erring toward retry/fail on the probe-error branch, keeping the lenient path only for "probe succeeded but BACK-FILE differs".

[cubesandboxbot]

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.

[dushulin]

Correction to my previous comment: I have reverted the two policy changes it described, and the branch is now back to the behaviour of the first push. Sorry for the churn — this supersedes the stillOurs snippet above.

What is gone again:

• the if detachErr != nil { return ... } that failed the build when the device could not be detached;
• the stillOurs() query that had to be added to stop that policy from failing healthy builds;
• the exponential backoff (back to 5 attempts, 200 ms apart).

The reasoning is the one your round-6 medium pointed at, taken one step further. That finding was correct — but the regression it described exists only because of the fail-the-build policy I had added the round before. Removing the policy removes the need to special-case "the device is already gone" at all: a detach that fails for any reason is logged and the build succeeds, which is what the code did before this PR. Keeping the policy meant carrying a second mechanism whose only job was to protect against the first, and whose correctness depends on losetup --list output I cannot verify from here.

So the PR is back to the two things it set out to do:

1. Fix the ordering that causes the leak. The single cleanup closure guarantees umount runs before losetup --detach, instead of the LIFO defer pair that ran them backwards and pinned the device with EBUSY. The detach error is logged rather than discarded.
2. Reclaim devices already leaked. On an allocation failure, detach loop devices under the artifact store whose backing file is (deleted) and retry once, so an exhausted host self-heals instead of degrading to phase-1 forever.

The tradeoff I am accepting explicitly, rather than paying for it with more machinery: a device that cannot be detached on the success path stays attached until the host reboots and is not a reclaim candidate (the image is kept, so the backing file is never unlinked). That is exactly what happens today; the only difference is that it is now visible in the log instead of silently swallowed.

Tests are kept, minus the one that asserted the reverted semantics: the umount-before-detach ordering (verified to fail if the ordering is reverted), the busy-detach retry, the lazy-umount-then-detach case, the stuck-mount single-attempt case, the detach on a failed mount, and the reclaim path including "nothing to reclaim" and "retry after reclaim also failed".

[cubesandboxbot]

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.

[cubesandboxbot]

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.

[cubesandboxbot]

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.

[dushulin]

cc @fslongjin I wonder if you guys have encountered this issue when using K8s to deploy Cubemaster within your system. It seems that the loop devices are running out of resources. We encountered this issue during the deployment of version 0.5.1.