From 231307a347b015a285306db1f0b1f721ebe350e2 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sat, 29 Aug 2026 12:06:02 +0100 Subject: [PATCH 1/6] fix(storage): flush the filesystem before unmount/detach (#270) A service writing to a gce-pd volume got its file metadata persisted and its file contents discarded: files came back with the right name, owner and mode, and a length of zero. Reported against dev.146; the same service on the local StorageClass, and under plain docker on the same host, was fine. Cause. Detaching a cloud disk drops whatever is still in the page cache. umount(2) flushes on its own, so the happy path was safe -- but nothing flushed on the paths where the unmount does not actually happen, and the agent detaches regardless rather than strand a volume on a node that is going away: * Unmount fails. A container still holding the bind returns EBUSY. Containers outlive runed, so a runed restart unmounts volumes out from under live containers. * Unmount is skipped. Each driver probed with `findmnt` via exec.CommandContext first, and exec cannot start on an expired or cancelled context -- a probe that failed to run was read as "not mounted", so Unmount returned nil having done nothing. Teardown runs during shutdown, which is exactly where contexts expire. That is issue #191, and it turns out to be a data-loss bug rather than the orphaned-mount nuisance it was filed as. Reproduced the reported signature exactly on a loop-backed ext4: create two files, let the journal commit, write their contents without fsync, then snapshot the backing device (what a detached disk contains). Both files come back at 0 bytes with correct metadata, beside lost+found. With a syncfs first they come back with their contents. Fix. New pkg/storage/driver/mountsync owns flush-then-unmount for all four cloud drivers, which previously held byte-identical copies of this logic -- four chances for one to drift on a routine where drift means lost data. It calls syncfs(2) on the mount point (not sync(2), which would stall every filesystem on the node inside a seconds-long shutdown budget), then umount(2) directly with no subprocess, so a dead context can no longer skip the work. When the unmount fails the error states whether the flush succeeded, because the caller detaches either way and that is the difference between a volume left attached and writes thrown away. EPERM is deliberately not treated as "nothing was mounted": unprivileged umount(2) fails with EPERM before the kernel considers the target, so folding it in would recreate the silent no-op. runed holds CAP_SYS_ADMIN in production, so EPERM there is a real misconfiguration. Tests table-drive that classification, since umount(2) needs privilege the CI runner does not have; the syscall-level tests skip unless it does, and pass under `docker run --privileged`, which is the production condition. --- pkg/storage/driver/awsebs/mount_linux.go | 17 +- pkg/storage/driver/dovolume/mount_linux.go | 17 +- pkg/storage/driver/gcepd/mount_linux.go | 17 +- .../driver/hcloudvolume/mount_linux.go | 15 +- pkg/storage/driver/mountsync/mountsync.go | 44 +++++ .../driver/mountsync/mountsync_linux.go | 69 +++++++ .../driver/mountsync/mountsync_linux_test.go | 170 ++++++++++++++++++ .../driver/mountsync/mountsync_other.go | 10 ++ 8 files changed, 324 insertions(+), 35 deletions(-) create mode 100644 pkg/storage/driver/mountsync/mountsync.go create mode 100644 pkg/storage/driver/mountsync/mountsync_linux.go create mode 100644 pkg/storage/driver/mountsync/mountsync_linux_test.go create mode 100644 pkg/storage/driver/mountsync/mountsync_other.go diff --git a/pkg/storage/driver/awsebs/mount_linux.go b/pkg/storage/driver/awsebs/mount_linux.go index f6a95872..95ba77f1 100644 --- a/pkg/storage/driver/awsebs/mount_linux.go +++ b/pkg/storage/driver/awsebs/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux 2.39+ @@ -26,13 +28,10 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("awsebs: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + // Flush-then-unmount lives in mountsync, shared by every cloud driver: + // a detach discards unflushed pages, and four private copies of that + // logic is four chances for one to drift. See issue #270. + return mountsync.Unmount("awsebs", target) } diff --git a/pkg/storage/driver/dovolume/mount_linux.go b/pkg/storage/driver/dovolume/mount_linux.go index 4a401779..28d8a303 100644 --- a/pkg/storage/driver/dovolume/mount_linux.go +++ b/pkg/storage/driver/dovolume/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux @@ -29,13 +31,10 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("dovolume: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + // Flush-then-unmount lives in mountsync, shared by every cloud driver: + // a detach discards unflushed pages, and four private copies of that + // logic is four chances for one to drift. See issue #270. + return mountsync.Unmount("dovolume", target) } diff --git a/pkg/storage/driver/gcepd/mount_linux.go b/pkg/storage/driver/gcepd/mount_linux.go index 44d6004c..40d3bcba 100644 --- a/pkg/storage/driver/gcepd/mount_linux.go +++ b/pkg/storage/driver/gcepd/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) // Mount on Linux calls mount(2) directly. /bin/mount on util-linux 2.39+ @@ -26,13 +28,10 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -// Unmount on Linux calls umount2(2) directly with no flags. -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("gcepd: umount(2) %s: %w", target, err) - } - return nil +// Unmount on Linux flushes the filesystem, then calls umount2(2). +func (execMounter) Unmount(_ context.Context, target string) error { + // Flush-then-unmount lives in mountsync, shared by every cloud driver: + // a detach discards unflushed pages, and four private copies of that + // logic is four chances for one to drift. See issue #270. + return mountsync.Unmount("gcepd", target) } diff --git a/pkg/storage/driver/hcloudvolume/mount_linux.go b/pkg/storage/driver/hcloudvolume/mount_linux.go index 3724a5be..d309df35 100644 --- a/pkg/storage/driver/hcloudvolume/mount_linux.go +++ b/pkg/storage/driver/hcloudvolume/mount_linux.go @@ -7,6 +7,8 @@ import ( "fmt" "golang.org/x/sys/unix" + + "github.com/runestack/rune/pkg/storage/driver/mountsync" ) func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOnly bool) error { @@ -23,12 +25,9 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } -func (execMounter) Unmount(ctx context.Context, target string) error { - if !alreadyMounted(ctx, target) { - return nil - } - if err := unix.Unmount(target, 0); err != nil { - return fmt.Errorf("hcloudvolume: umount(2) %s: %w", target, err) - } - return nil +func (execMounter) Unmount(_ context.Context, target string) error { + // Flush-then-unmount lives in mountsync, shared by every cloud driver: + // a detach discards unflushed pages, and four private copies of that + // logic is four chances for one to drift. See issue #270. + return mountsync.Unmount("hcloudvolume", target) } diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go new file mode 100644 index 00000000..54a2a354 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -0,0 +1,44 @@ +// Package mountsync owns the teardown half of a block-volume mount: +// flushing the filesystem and unmounting it. +// +// It exists because detaching a cloud disk discards whatever is still in +// the page cache. A successful umount(2) flushes on its own, so the sync +// is redundant on the happy path — it matters on the paths where the +// unmount does not happen: +// +// - The unmount fails. A container still holding the bind returns +// EBUSY, and the agent detaches anyway rather than strand the volume +// on a node that is going away. +// - The disk is detached by something outside this process. +// +// In those cases an unsynced ext4 comes back with its metadata journaled +// and its file contents gone: files with the right name, owner and mode, +// and a length of zero. That was issue #270. +// +// The four cloud drivers (do-volume, gce-pd, aws-ebs, hcloud-volume) had +// byte-identical copies of this logic. They share this one now, because +// a subtle difference between copies of a data-loss-critical routine is +// not something a reviewer would reliably catch. +package mountsync + +// Target flushes every dirty page of the filesystem containing path. +// +// Best-effort by design: on a teardown path the useful response to +// "could not sync" is to carry on and report it, not to abandon the +// unmount. A nil return means the filesystem was flushed. +func Target(path string) error { + return syncTarget(path) +} + +// Unmount flushes the filesystem at target and then unmounts it. +// +// driver names the calling driver for error messages ("gcepd"). The +// returned error says explicitly whether the flush succeeded, because +// the caller detaches the disk either way: that is the difference +// between a volume left attached somewhere and unwritten data discarded. +// +// Idempotent — unmounting a path that is not a mount point, or that no +// longer exists, is a nil return. +func Unmount(driver, target string) error { + return unmountTarget(driver, target) +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go new file mode 100644 index 00000000..f244fa64 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -0,0 +1,69 @@ +//go:build linux + +package mountsync + +import ( + "errors" + "fmt" + + "golang.org/x/sys/unix" +) + +// syncTarget opens the mount point and calls syncfs(2) on it, which +// flushes only the filesystem containing that path. sync(2) would flush +// every filesystem on the node — on a host with several volumes and a +// busy root disk that turns one volume's teardown into everyone's stall, +// inside a shutdown budget measured in seconds. +func syncTarget(path string) error { + fd, err := unix.Open(path, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return fmt.Errorf("mountsync: open %s: %w", path, err) + } + defer func() { _ = unix.Close(fd) }() + if err := unix.Syncfs(fd); err != nil { + return fmt.Errorf("mountsync: syncfs %s: %w", path, err) + } + return nil +} + +func unmountTarget(driver, target string) error { + // Flush first. See the package doc: the caller detaches even when the + // unmount below fails, and a detach drops whatever is still cached. + syncErr := syncTarget(target) + + // Attempt the unmount unconditionally rather than probing first. The + // drivers used to shell out to `findmnt` to decide whether to bother — + // but exec cannot start on an expired or cancelled context, and a + // probe that failed to run was read as "not mounted", silently + // skipping the unmount entirely. Teardown runs during shutdown, which + // is exactly where contexts expire. umount(2) itself is the + // authoritative answer and needs no subprocess. + err := unix.Unmount(target, 0) + switch { + case err == nil: + return nil + case notMounted(err): + return nil + case syncErr != nil: + return fmt.Errorf("%s: umount(2) %s: %w (filesystem was NOT flushed first: %v; detaching now can lose unwritten data)", + driver, target, err, syncErr) + default: + return fmt.Errorf("%s: umount(2) %s: %w (filesystem was flushed, so a detach will not lose data)", + driver, target, err) + } +} + +// notMounted reports whether an error from umount(2) means there was +// nothing mounted at the target — the idempotent success this package +// promises. +// +// EPERM is deliberately NOT in this set. Unprivileged umount(2) fails +// with EPERM before the kernel ever considers whether the path is a +// mount point, so treating it as "nothing to do" would turn a missing +// CAP_SYS_ADMIN into a silent no-op — the same class of bug as the +// findmnt probe this replaced. runed holds CAP_SYS_ADMIN in production +// (it mounts these volumes in the first place), so EPERM there is a real +// misconfiguration and must surface. +func notMounted(err error) bool { + return errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOENT) +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go new file mode 100644 index 00000000..dfea18b7 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -0,0 +1,170 @@ +//go:build linux + +package mountsync + +import ( + "os" + "path/filepath" + "testing" + + "golang.org/x/sys/unix" +) + +// --- Target (syncfs) ------------------------------------------------- + +// Target must succeed on the ordinary path. If it errored routinely, +// every teardown would report "filesystem was NOT flushed" and the +// warning would stop meaning anything. +func TestTargetFlushesRealDirectory(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f"), []byte("data"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + if err := Target(dir); err != nil { + t.Errorf("Target on a real directory should succeed, got %v", err) + } +} + +// The caller uses this error to decide whether a detach is safe, so a +// path it could not open must never look like a successful flush. +func TestTargetReportsMissingPath(t *testing.T) { + if err := Target(filepath.Join(t.TempDir(), "does-not-exist")); err == nil { + t.Error("Target must report a path it could not open, not report success") + } +} + +// O_DIRECTORY guards against being handed a file path by mistake, where +// syncing would flush the wrong thing and claim success. +func TestTargetRejectsNonDirectory(t *testing.T) { + f := filepath.Join(t.TempDir(), "afile") + if err := os.WriteFile(f, []byte("x"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + if err := Target(f); err == nil { + t.Error("Target on a non-directory should error rather than claim success") + } +} + +// --- notMounted classification --------------------------------------- + +// The classifier decides whether a failed umount(2) means "nothing was +// mounted" (success) or a real failure. Getting it wrong in either +// direction is expensive: too generous and a failed unmount is reported +// as done while the caller detaches a live filesystem; too strict and +// ordinary idempotent teardown looks broken. +// +// This is table-driven rather than syscall-driven because umount(2) +// requires CAP_SYS_ADMIN — see TestUnmountRequiresPrivilegeToBeMeaningful. +func TestNotMountedClassification(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"EINVAL — not a mount point", unix.EINVAL, true}, + {"ENOENT — target is gone", unix.ENOENT, true}, + {"EBUSY — something still holds it", unix.EBUSY, false}, + {"EPERM — we lack CAP_SYS_ADMIN", unix.EPERM, false}, + {"EACCES — permission on a path component", unix.EACCES, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := notMounted(c.err); got != c.want { + t.Errorf("notMounted(%v) = %v, want %v", c.err, got, c.want) + } + }) + } +} + +// TestNotMountedRejectsEPERM pins the subtlest case with its reason. +// +// Unprivileged umount(2) returns EPERM *before* the kernel considers +// whether the path is a mount point. Folding EPERM into "nothing was +// mounted" would turn a missing capability into a silent no-op: unmount +// reports success, the caller detaches, and the page cache goes with it. +// That is precisely the shape of the findmnt probe this package replaced +// (issue #191), so it is worth its own test rather than one row. +func TestNotMountedRejectsEPERM(t *testing.T) { + if notMounted(unix.EPERM) { + t.Error("EPERM must not be read as 'nothing was mounted' — it means we could not even try") + } +} + +// --- Unmount --------------------------------------------------------- + +// Unmount promises idempotence, but only a privileged caller can observe +// it: without CAP_SYS_ADMIN the kernel answers EPERM before evaluating +// the target. runed holds that capability in production. CI does not, so +// this skips rather than asserting something the environment cannot show. +func TestUnmountIsIdempotentOnNonMountpoint(t *testing.T) { + requirePrivilegedUnmount(t) + if err := Unmount("test", t.TempDir()); err != nil { + t.Errorf("Unmount on a non-mountpoint must succeed (idempotent), got %v", err) + } +} + +// The regression for the silent no-op (issue #191, and a contributor to +// the data loss in #270): Unmount used to probe with findmnt via +// exec.CommandContext. On a cancelled context that process cannot start, +// the failure was read as "not mounted", and Unmount returned nil having +// done nothing — after which the caller detached the disk. Teardown runs +// during shutdown, which is exactly where contexts die. +// +// Unmount no longer takes a context or shells out, so there is nothing +// left to fail this way. The test that would once have caught the bug is +// now a compile-time property; what remains observable is that a dead +// context is simply not part of the signature. +func TestUnmountDoesNotDependOnAContext(t *testing.T) { + requirePrivilegedUnmount(t) + // No ctx argument exists to cancel. Calling it during "shutdown" + // behaves the same as calling it at any other time. + if err := Unmount("test", t.TempDir()); err != nil { + t.Errorf("Unmount must not depend on a live context, got %v", err) + } +} + +// A target that no longer exists is already-gone, not an error. +func TestUnmountMissingTargetIsNotAnError(t *testing.T) { + requirePrivilegedUnmount(t) + gone := filepath.Join(t.TempDir(), "never-created") + if err := Unmount("test", gone); err != nil { + t.Errorf("Unmount on a missing target should be a no-op, got %v", err) + } +} + +// TestUnmountReportsUnflushedFilesystem: when the flush fails and the +// unmount fails, the error must say the data is at risk. The caller +// detaches regardless, so this sentence is the only warning an operator +// gets that a detach is about to discard writes. +func TestUnmountReportsUnflushedFilesystem(t *testing.T) { + // A path that cannot be opened fails the sync, and (unprivileged) + // fails the unmount too — the both-failed branch. + err := Unmount("test", filepath.Join(t.TempDir(), "absent", "deeper")) + if err == nil { + t.Skip("environment allowed the unmount; the both-failed branch is unreachable here") + } + if !contains(err.Error(), "NOT flushed") { + t.Errorf("an unflushed failure must warn about data loss, got: %v", err) + } +} + +func contains(s, sub string) bool { + return len(s) >= len(sub) && (func() bool { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false + })() +} + +// requirePrivilegedUnmount skips when the process cannot call umount(2) +// at all, so a CI failure means a real regression rather than a sandbox. +func requirePrivilegedUnmount(t *testing.T) { + t.Helper() + err := unix.Unmount(t.TempDir(), 0) + if err == unix.EPERM { + t.Skip("umount(2) needs CAP_SYS_ADMIN; runed has it in production, this environment does not") + } +} diff --git a/pkg/storage/driver/mountsync/mountsync_other.go b/pkg/storage/driver/mountsync/mountsync_other.go new file mode 100644 index 00000000..001c1636 --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_other.go @@ -0,0 +1,10 @@ +//go:build !linux + +package mountsync + +// The cloud volume drivers only mount on Linux nodes. These exist so the +// packages that call them still build for local development on macOS. + +func syncTarget(string) error { return nil } + +func unmountTarget(string, string) error { return nil } From 01007edc3fd63ab2452a92c3ebe65b904f151eb4 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sat, 29 Aug 2026 12:55:52 +0100 Subject: [PATCH 2/6] fix(storage): correct the reason for the flush, and prove it Independent review of the previous commit found that its central claim was false, and the false claim was the dangerous part. I wrote that umount(2) flushes on its own, so the sync was redundant on the happy path and only mattered when the unmount failed. umount(2) flushes only when it releases the LAST reference to the superblock, and a container holds a second one -- the runtime binds the mount into its own namespace. So on the ordinary path the agent's umount(2) returns success, flushes nothing, and the detach discards every dirty page. Measured on loop-backed ext4, writing 90 bytes without fsync and reading back the raw device: single mount, bare umount(2) rc=0 90 bytes second mount held, bare umount(2) rc=0 0 bytes The bug therefore fires on the success path, not only on EBUSY, and the sync is not defence in depth -- it is the fix. The wrong rationale invited a specific regression: reordering to "umount first, sync only on failure" reads as strictly cheaper and silently restores the data loss. The package doc now says that, and a mutation test confirms that exact reordering fails. Also from review: * The e2e test that was missing. It builds the production shape (a second mount of the superblock) and asserts a bare umount loses the write where mountsync.Unmount keeps it. It fails against the code this replaced, which none of the previous tests did. * A CI step that runs the package privileged. umount(2) and loop mounts need CAP_SYS_ADMIN, so the tests that matter were skipping in CI while the ones that ran skipped under privilege -- green for disjoint reasons, with the EBUSY branch untested everywhere. * syncfs on a target with nothing mounted resolved to the root disk and flushed that, which is the whole-node stall the code says it avoids. Now checked, erring toward flushing when it cannot tell. * The failed-unmount message claimed "a detach will not lose data". It is true only of writes made before the flush, and on that path the holder is a running container still writing. It now states the fact. * The non-Linux stubs returned nil, reporting success for work never done. They return an error. * False comments: a reference to a test that does not exist; "byte-identical copies", which they were not (each embedded its own driver name, which is why Unmount takes one); "used to shell out to findmnt", still true of Mount and of the whole non-Linux path. * The rationale was written out seven times for logic consolidated into one place. It lives in the package doc now. --- .github/workflows/ci.yml | 10 ++ pkg/storage/driver/awsebs/mount_linux.go | 3 - pkg/storage/driver/dovolume/mount_linux.go | 3 - pkg/storage/driver/gcepd/mount_linux.go | 3 - .../driver/hcloudvolume/mount_linux.go | 4 +- pkg/storage/driver/mountsync/mountsync.go | 63 ++++--- .../mountsync/mountsync_e2e_linux_test.go | 162 ++++++++++++++++++ .../driver/mountsync/mountsync_linux.go | 74 +++++--- .../driver/mountsync/mountsync_linux_test.go | 137 ++++++--------- .../driver/mountsync/mountsync_other.go | 16 +- 10 files changed, 311 insertions(+), 164 deletions(-) create mode 100644 pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6cfb3b7c..b3fa39c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,16 @@ jobs: env: COVERAGE_THRESHOLD: ${{ secrets.COVERAGE_THRESHOLD }} run: make check-coverage + # The volume teardown tests need CAP_SYS_ADMIN: umount(2) and loop + # mounts are unavailable to the ordinary runner user, so the tests that + # matter most skip silently in the job above. Without this step the + # package's central guarantee — that a detach cannot discard writes a + # container still holds — is asserted only by comments. + - name: Volume teardown tests (privileged) + run: | + docker run --rm --privileged -v "$PWD":/src -w /src golang:1.25 sh -c ' + apt-get -qq update && apt-get -qq install -y e2fsprogs >/dev/null && + go test ./pkg/storage/driver/mountsync/... -v' build: name: Build diff --git a/pkg/storage/driver/awsebs/mount_linux.go b/pkg/storage/driver/awsebs/mount_linux.go index 95ba77f1..0056f152 100644 --- a/pkg/storage/driver/awsebs/mount_linux.go +++ b/pkg/storage/driver/awsebs/mount_linux.go @@ -30,8 +30,5 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn // Unmount on Linux flushes the filesystem, then calls umount2(2). func (execMounter) Unmount(_ context.Context, target string) error { - // Flush-then-unmount lives in mountsync, shared by every cloud driver: - // a detach discards unflushed pages, and four private copies of that - // logic is four chances for one to drift. See issue #270. return mountsync.Unmount("awsebs", target) } diff --git a/pkg/storage/driver/dovolume/mount_linux.go b/pkg/storage/driver/dovolume/mount_linux.go index 28d8a303..fe03ea8c 100644 --- a/pkg/storage/driver/dovolume/mount_linux.go +++ b/pkg/storage/driver/dovolume/mount_linux.go @@ -33,8 +33,5 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn // Unmount on Linux flushes the filesystem, then calls umount2(2). func (execMounter) Unmount(_ context.Context, target string) error { - // Flush-then-unmount lives in mountsync, shared by every cloud driver: - // a detach discards unflushed pages, and four private copies of that - // logic is four chances for one to drift. See issue #270. return mountsync.Unmount("dovolume", target) } diff --git a/pkg/storage/driver/gcepd/mount_linux.go b/pkg/storage/driver/gcepd/mount_linux.go index 40d3bcba..a6296b08 100644 --- a/pkg/storage/driver/gcepd/mount_linux.go +++ b/pkg/storage/driver/gcepd/mount_linux.go @@ -30,8 +30,5 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn // Unmount on Linux flushes the filesystem, then calls umount2(2). func (execMounter) Unmount(_ context.Context, target string) error { - // Flush-then-unmount lives in mountsync, shared by every cloud driver: - // a detach discards unflushed pages, and four private copies of that - // logic is four chances for one to drift. See issue #270. return mountsync.Unmount("gcepd", target) } diff --git a/pkg/storage/driver/hcloudvolume/mount_linux.go b/pkg/storage/driver/hcloudvolume/mount_linux.go index d309df35..7c01c430 100644 --- a/pkg/storage/driver/hcloudvolume/mount_linux.go +++ b/pkg/storage/driver/hcloudvolume/mount_linux.go @@ -25,9 +25,7 @@ func (execMounter) Mount(ctx context.Context, dev, target, fsType string, readOn return nil } +// Unmount on Linux flushes the filesystem, then calls umount2(2). func (execMounter) Unmount(_ context.Context, target string) error { - // Flush-then-unmount lives in mountsync, shared by every cloud driver: - // a detach discards unflushed pages, and four private copies of that - // logic is four chances for one to drift. See issue #270. return mountsync.Unmount("hcloudvolume", target) } diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go index 54a2a354..8c56752c 100644 --- a/pkg/storage/driver/mountsync/mountsync.go +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -1,44 +1,41 @@ -// Package mountsync owns the teardown half of a block-volume mount: -// flushing the filesystem and unmounting it. +// Package mountsync flushes a mounted filesystem and unmounts it, for the +// cloud volume drivers that then detach the underlying disk. // -// It exists because detaching a cloud disk discards whatever is still in -// the page cache. A successful umount(2) flushes on its own, so the sync -// is redundant on the happy path — it matters on the paths where the -// unmount does not happen: +// The flush is not belt-and-braces. umount(2) writes the filesystem out +// only when it releases the LAST reference to the superblock, and a +// container started with this volume holds a second one: the runtime +// binds the mount into the container's own mount namespace. So the +// agent's umount(2) returns success, flushes nothing, and the detach +// that follows discards every page still dirty. // -// - The unmount fails. A container still holding the bind returns -// EBUSY, and the agent detaches anyway rather than strand the volume -// on a node that is going away. -// - The disk is detached by something outside this process. +// Measured on loop-backed ext4, writing a 90-byte file without fsync and +// then reading back the raw device (what a detach hands you): // -// In those cases an unsynced ext4 comes back with its metadata journaled -// and its file contents gone: files with the right name, owner and mode, -// and a length of zero. That was issue #270. +// single mount, bare umount(2) rc=0 90 bytes +// second mount held, bare umount(2) rc=0 0 bytes // -// The four cloud drivers (do-volume, gce-pd, aws-ebs, hcloud-volume) had -// byte-identical copies of this logic. They share this one now, because -// a subtle difference between copies of a data-loss-critical routine is -// not something a reviewer would reliably catch. -package mountsync - -// Target flushes every dirty page of the filesystem containing path. +// Zero-length files with correct names, owners and modes is the signature +// operators see. Any workload relying on ordinary write-back is exposed; +// databases escape only because they fsync their own journals. // -// Best-effort by design: on a teardown path the useful response to -// "could not sync" is to carry on and report it, not to abandon the -// unmount. A nil return means the filesystem was flushed. -func Target(path string) error { - return syncTarget(path) -} +// The consequence for anyone editing this package: the sync must stay +// unconditional and must stay BEFORE the unmount. Moving it after, or +// behind an error check, reads as a cheap optimisation because umount(2) +// "already flushes" — and silently reopens the bug for every volume a +// container is holding. See issue #270. +package mountsync -// Unmount flushes the filesystem at target and then unmounts it. +// Unmount flushes the filesystem at target and then unmounts it. driver +// names the calling driver for error messages ("gcepd"). // -// driver names the calling driver for error messages ("gcepd"). The -// returned error says explicitly whether the flush succeeded, because -// the caller detaches the disk either way: that is the difference -// between a volume left attached somewhere and unwritten data discarded. +// When the unmount fails the error states whether the flush succeeded: +// the caller detaches either way, so that is the difference between a +// volume left attached and unwritten data discarded. // -// Idempotent — unmounting a path that is not a mount point, or that no -// longer exists, is a nil return. +// Idempotent for a caller holding CAP_SYS_ADMIN — a target that is not a +// mount point, or is already gone, returns nil. Unprivileged, umount(2) +// answers EPERM before it looks at the target, and that surfaces as an +// error rather than a silent success. func Unmount(driver, target string) error { return unmountTarget(driver, target) } diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go new file mode 100644 index 00000000..ed19368f --- /dev/null +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -0,0 +1,162 @@ +//go:build linux + +package mountsync + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +// TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock is the test this +// package exists to pass, and the only one that fails against the code +// this replaced. +// +// It builds the production shape on a real filesystem: an ext4 volume +// mounted where the agent mounts one, with a SECOND mount of the same +// superblock standing in for the bind a container runtime makes into its +// own mount namespace. A file is written without fsync — an ordinary +// write-back workload — and then the raw device is captured, which is +// what a detach hands back. +// +// A bare umount(2) returns success here and flushes nothing, because it +// releases only one of two references to the superblock. That is the +// production bug: the agent logged "Volume unmounted" and detached a +// filesystem whose pages were still dirty. Unmount must recover the file +// intact where a bare umount(2) loses it. +func TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock(t *testing.T) { + requireLoopMount(t) + + for _, tc := range []struct { + name string + bare bool // tear down with a bare umount(2), as the old code did + wantBytes int + }{ + {"bare umount(2) loses the write", true, 0}, + {"mountsync.Unmount keeps it", false, 90}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + img := filepath.Join(dir, "disk.img") + target := filepath.Join(dir, "mnt") + second := filepath.Join(dir, "held") + for _, d := range []string{target, second} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, target) + + // Create and commit the file, so only its contents are at risk — + // the reported signature is a correctly-named zero-length file. + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), nil, 0o644); err != nil { + t.Fatalf("create: %v", err) + } + mustRun(t, "sync") + + // The container's bind: a second reference to the superblock. + mustRun(t, "mount", "--bind", target, second) + t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + + // The workload writes and does not fsync. + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + if tc.bare { + if err := unix.Unmount(target, 0); err != nil { + t.Fatalf("bare umount: %v", err) + } + } else if err := Unmount("test", target); err != nil { + t.Fatalf("Unmount: %v", err) + } + + got := bytesOnDevice(t, dir, img) + if got != tc.wantBytes { + t.Errorf("recovered %d bytes from the raw device, want %d", got, tc.wantBytes) + } + }) + } +} + +// bytesOnDevice snapshots the backing file and reports the length of +// SYSTEM as it exists on disk — what a detached volume would come back +// with. +func bytesOnDevice(t *testing.T, dir, img string) int { + t.Helper() + snap := filepath.Join(dir, "snap.img") + mustRun(t, "cp", img, snap) + mnt := filepath.Join(dir, "snapmnt") + if err := os.MkdirAll(mnt, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + mustRun(t, "mount", "-o", "loop", snap, mnt) + defer func() { _ = exec.Command("umount", mnt).Run() }() + + fi, err := os.Stat(filepath.Join(mnt, "SYSTEM")) + if err != nil { + if os.IsNotExist(err) { + return -1 // file never reached the device at all + } + t.Fatalf("stat: %v", err) + } + return int(fi.Size()) +} + +func makeExt4(t *testing.T, img string) { + t.Helper() + f, err := os.Create(img) + if err != nil { + t.Fatalf("create image: %v", err) + } + if err := f.Truncate(32 << 20); err != nil { + t.Fatalf("truncate: %v", err) + } + _ = f.Close() + mustRun(t, "mkfs.ext4", "-q", "-F", img) +} + +func mustRun(t *testing.T, name string, args ...string) { + t.Helper() + if out, err := exec.Command(name, args...).CombinedOutput(); err != nil { + t.Fatalf("%s %s: %v (%s)", name, strings.Join(args, " "), err, strings.TrimSpace(string(out))) + } +} + +// requireLoopMount skips unless this process can actually mount a loop +// device, which needs CAP_SYS_ADMIN, mkfs.ext4 and loop support. That is +// the production condition, not the default CI one — see the privileged +// job in the CI workflow. +func requireLoopMount(t *testing.T) { + t.Helper() + if unix.Geteuid() != 0 { + t.Skip("needs root to mount a loop device") + } + if _, err := exec.LookPath("mkfs.ext4"); err != nil { + t.Skip("needs mkfs.ext4") + } + dir := t.TempDir() + img := filepath.Join(dir, "probe.img") + f, err := os.Create(img) + if err != nil { + t.Skip("cannot create a probe image") + } + _ = f.Truncate(8 << 20) + _ = f.Close() + if out, err := exec.Command("mkfs.ext4", "-q", "-F", img).CombinedOutput(); err != nil { + t.Skipf("cannot mkfs: %v (%s)", err, strings.TrimSpace(string(out))) + } + mnt := filepath.Join(dir, "probe") + _ = os.MkdirAll(mnt, 0o755) + if out, err := exec.Command("mount", "-o", "loop", img, mnt).CombinedOutput(); err != nil { + t.Skipf("cannot loop-mount here: %v (%s)", err, strings.TrimSpace(string(out))) + } + _ = exec.Command("umount", mnt).Run() + _ = fmt.Sprint() +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go index f244fa64..9ad3e45b 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux.go +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -9,10 +9,10 @@ import ( "golang.org/x/sys/unix" ) -// syncTarget opens the mount point and calls syncfs(2) on it, which -// flushes only the filesystem containing that path. sync(2) would flush -// every filesystem on the node — on a host with several volumes and a -// busy root disk that turns one volume's teardown into everyone's stall, +// syncTarget opens the mount point and calls syncfs(2) on it, flushing +// only the filesystem containing that path. sync(2) would flush every +// filesystem on the node — on a host with several volumes and a busy +// root disk that turns one volume's teardown into everyone's stall, // inside a shutdown budget measured in seconds. func syncTarget(path string) error { fd, err := unix.Open(path, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) @@ -27,43 +27,63 @@ func syncTarget(path string) error { } func unmountTarget(driver, target string) error { - // Flush first. See the package doc: the caller detaches even when the - // unmount below fails, and a detach drops whatever is still cached. - syncErr := syncTarget(target) - - // Attempt the unmount unconditionally rather than probing first. The - // drivers used to shell out to `findmnt` to decide whether to bother — - // but exec cannot start on an expired or cancelled context, and a - // probe that failed to run was read as "not mounted", silently - // skipping the unmount entirely. Teardown runs during shutdown, which - // is exactly where contexts expire. umount(2) itself is the - // authoritative answer and needs no subprocess. + // Flush first — see the package doc. Unconditional and before the + // unmount, both deliberately. + var syncErr error + if isMountPoint(target) { + syncErr = syncTarget(target) + } + // No pre-probe on the unmount itself. A probe needs a subprocess + // (findmnt), teardown runs during shutdown where a cancelled context + // stops exec starting, and a probe that could not run reads as "not + // mounted" — skipping the unmount entirely. umount(2) answers + // authoritatively with no subprocess. err := unix.Unmount(target, 0) switch { - case err == nil: - return nil - case notMounted(err): + case err == nil, notMounted(err): return nil case syncErr != nil: - return fmt.Errorf("%s: umount(2) %s: %w (filesystem was NOT flushed first: %v; detaching now can lose unwritten data)", + return fmt.Errorf("%s: umount(2) %s: %w (the filesystem was NOT flushed first: %v; a detach now can lose unwritten data)", driver, target, err, syncErr) default: - return fmt.Errorf("%s: umount(2) %s: %w (filesystem was flushed, so a detach will not lose data)", + // Deliberately a fact, not a reassurance. Anything the holder + // writes between the flush and the detach is still lost, and on + // this path the holder is usually a running container. + return fmt.Errorf("%s: umount(2) %s: %w (flushed as of now; writes made after this point are not on the disk)", driver, target, err) } } +// isMountPoint reports whether target has a different filesystem from the +// directory containing it, which for these drivers means a volume is +// mounted there — they always mount a distinct block device. +// +// It exists to keep the idempotent path off the root disk: on a target +// where nothing is mounted, the syncfs above would resolve to / and flush +// that instead, which is the whole-node stall syncTarget is written to +// avoid. It is advisory only, and any uncertainty answers "yes, sync it" — +// an unnecessary flush costs time, a skipped one costs data. +func isMountPoint(target string) bool { + var self, parent unix.Stat_t + if err := unix.Lstat(target, &self); err != nil { + return true // cannot tell: flush anyway + } + if err := unix.Lstat(target+"/..", &parent); err != nil { + return true + } + return self.Dev != parent.Dev +} + // notMounted reports whether an error from umount(2) means there was // nothing mounted at the target — the idempotent success this package // promises. // -// EPERM is deliberately NOT in this set. Unprivileged umount(2) fails -// with EPERM before the kernel ever considers whether the path is a -// mount point, so treating it as "nothing to do" would turn a missing -// CAP_SYS_ADMIN into a silent no-op — the same class of bug as the -// findmnt probe this replaced. runed holds CAP_SYS_ADMIN in production -// (it mounts these volumes in the first place), so EPERM there is a real -// misconfiguration and must surface. +// EPERM is deliberately absent. Unprivileged umount(2) fails with EPERM +// before the kernel considers whether the path is a mount point, so +// treating it as "nothing to do" would turn a missing CAP_SYS_ADMIN into +// a silent no-op — the same class of bug as the findmnt probe this +// replaced. runed holds CAP_SYS_ADMIN in production (it mounts these +// volumes), so EPERM there is a real misconfiguration and must surface. func notMounted(err error) bool { return errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOENT) } diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go index dfea18b7..7f98e6a1 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -5,56 +5,70 @@ package mountsync import ( "os" "path/filepath" + "strings" "testing" "golang.org/x/sys/unix" ) -// --- Target (syncfs) ------------------------------------------------- +// --- syncTarget ------------------------------------------------------ -// Target must succeed on the ordinary path. If it errored routinely, -// every teardown would report "filesystem was NOT flushed" and the -// warning would stop meaning anything. -func TestTargetFlushesRealDirectory(t *testing.T) { +func TestSyncTargetFlushesRealDirectory(t *testing.T) { dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, "f"), []byte("data"), 0o644); err != nil { t.Fatalf("write: %v", err) } - if err := Target(dir); err != nil { - t.Errorf("Target on a real directory should succeed, got %v", err) + if err := syncTarget(dir); err != nil { + t.Errorf("syncTarget on a real directory should succeed, got %v", err) } } -// The caller uses this error to decide whether a detach is safe, so a -// path it could not open must never look like a successful flush. -func TestTargetReportsMissingPath(t *testing.T) { - if err := Target(filepath.Join(t.TempDir(), "does-not-exist")); err == nil { - t.Error("Target must report a path it could not open, not report success") +func TestSyncTargetReportsMissingPath(t *testing.T) { + if err := syncTarget(filepath.Join(t.TempDir(), "does-not-exist")); err == nil { + t.Error("syncTarget must report a path it could not open, not report success") } } -// O_DIRECTORY guards against being handed a file path by mistake, where -// syncing would flush the wrong thing and claim success. -func TestTargetRejectsNonDirectory(t *testing.T) { +// O_DIRECTORY is a sanity check that the caller passed a mount point +// rather than a file. It does not change which filesystem gets flushed — +// syncfs works through either — so this pins the guard, not a safety +// property. +func TestSyncTargetRejectsNonDirectory(t *testing.T) { f := filepath.Join(t.TempDir(), "afile") if err := os.WriteFile(f, []byte("x"), 0o644); err != nil { t.Fatalf("write: %v", err) } - if err := Target(f); err == nil { - t.Error("Target on a non-directory should error rather than claim success") + if err := syncTarget(f); err == nil { + t.Error("syncTarget on a non-directory should error rather than claim success") } } -// --- notMounted classification --------------------------------------- +// --- isMountPoint ---------------------------------------------------- -// The classifier decides whether a failed umount(2) means "nothing was -// mounted" (success) or a real failure. Getting it wrong in either -// direction is expensive: too generous and a failed unmount is reported -// as done while the caller detaches a live filesystem; too strict and -// ordinary idempotent teardown looks broken. +// A plain directory is not a mount point, so teardown must not flush the +// filesystem it happens to sit on — that would be the root disk, and the +// whole-node stall syncTarget exists to avoid. +func TestIsMountPointFalseForPlainDirectory(t *testing.T) { + if isMountPoint(t.TempDir()) { + t.Error("a plain directory must not be treated as a mount point") + } +} + +// Uncertainty resolves toward flushing: an unnecessary sync costs time, a +// skipped one costs data. +func TestIsMountPointAssumesMountedWhenItCannotTell(t *testing.T) { + if !isMountPoint(filepath.Join(t.TempDir(), "absent")) { + t.Error("an unstattable target must be assumed mounted, so the flush still happens") + } +} + +// --- notMounted ------------------------------------------------------ + +// Getting this wrong is expensive in both directions: too generous and a +// failed unmount reports success while the caller detaches a live +// filesystem; too strict and ordinary idempotent teardown looks broken. // -// This is table-driven rather than syscall-driven because umount(2) -// requires CAP_SYS_ADMIN — see TestUnmountRequiresPrivilegeToBeMeaningful. +// Table-driven because umount(2) needs CAP_SYS_ADMIN, which CI lacks. func TestNotMountedClassification(t *testing.T) { cases := []struct { name string @@ -64,6 +78,8 @@ func TestNotMountedClassification(t *testing.T) { {"EINVAL — not a mount point", unix.EINVAL, true}, {"ENOENT — target is gone", unix.ENOENT, true}, {"EBUSY — something still holds it", unix.EBUSY, false}, + // EPERM means we could not even try. Folding it in would turn a + // missing capability into a silent no-op. {"EPERM — we lack CAP_SYS_ADMIN", unix.EPERM, false}, {"EACCES — permission on a path component", unix.EACCES, false}, } @@ -76,26 +92,12 @@ func TestNotMountedClassification(t *testing.T) { } } -// TestNotMountedRejectsEPERM pins the subtlest case with its reason. -// -// Unprivileged umount(2) returns EPERM *before* the kernel considers -// whether the path is a mount point. Folding EPERM into "nothing was -// mounted" would turn a missing capability into a silent no-op: unmount -// reports success, the caller detaches, and the page cache goes with it. -// That is precisely the shape of the findmnt probe this package replaced -// (issue #191), so it is worth its own test rather than one row. -func TestNotMountedRejectsEPERM(t *testing.T) { - if notMounted(unix.EPERM) { - t.Error("EPERM must not be read as 'nothing was mounted' — it means we could not even try") - } -} - // --- Unmount --------------------------------------------------------- -// Unmount promises idempotence, but only a privileged caller can observe -// it: without CAP_SYS_ADMIN the kernel answers EPERM before evaluating -// the target. runed holds that capability in production. CI does not, so -// this skips rather than asserting something the environment cannot show. +// Idempotence is only observable to a privileged caller: without +// CAP_SYS_ADMIN the kernel answers EPERM before it evaluates the target. +// runed holds that capability in production; CI does not, so this skips +// rather than asserting something the environment cannot show. func TestUnmountIsIdempotentOnNonMountpoint(t *testing.T) { requirePrivilegedUnmount(t) if err := Unmount("test", t.TempDir()); err != nil { @@ -103,68 +105,31 @@ func TestUnmountIsIdempotentOnNonMountpoint(t *testing.T) { } } -// The regression for the silent no-op (issue #191, and a contributor to -// the data loss in #270): Unmount used to probe with findmnt via -// exec.CommandContext. On a cancelled context that process cannot start, -// the failure was read as "not mounted", and Unmount returned nil having -// done nothing — after which the caller detached the disk. Teardown runs -// during shutdown, which is exactly where contexts die. -// -// Unmount no longer takes a context or shells out, so there is nothing -// left to fail this way. The test that would once have caught the bug is -// now a compile-time property; what remains observable is that a dead -// context is simply not part of the signature. -func TestUnmountDoesNotDependOnAContext(t *testing.T) { - requirePrivilegedUnmount(t) - // No ctx argument exists to cancel. Calling it during "shutdown" - // behaves the same as calling it at any other time. - if err := Unmount("test", t.TempDir()); err != nil { - t.Errorf("Unmount must not depend on a live context, got %v", err) - } -} - -// A target that no longer exists is already-gone, not an error. func TestUnmountMissingTargetIsNotAnError(t *testing.T) { requirePrivilegedUnmount(t) - gone := filepath.Join(t.TempDir(), "never-created") - if err := Unmount("test", gone); err != nil { + if err := Unmount("test", filepath.Join(t.TempDir(), "never-created")); err != nil { t.Errorf("Unmount on a missing target should be a no-op, got %v", err) } } -// TestUnmountReportsUnflushedFilesystem: when the flush fails and the -// unmount fails, the error must say the data is at risk. The caller -// detaches regardless, so this sentence is the only warning an operator -// gets that a detach is about to discard writes. +// The message on a failed unmount is the only warning an operator gets +// before the caller detaches anyway, so it must state what is on the disk +// rather than reassure. func TestUnmountReportsUnflushedFilesystem(t *testing.T) { - // A path that cannot be opened fails the sync, and (unprivileged) - // fails the unmount too — the both-failed branch. err := Unmount("test", filepath.Join(t.TempDir(), "absent", "deeper")) if err == nil { t.Skip("environment allowed the unmount; the both-failed branch is unreachable here") } - if !contains(err.Error(), "NOT flushed") { + if !strings.Contains(err.Error(), "NOT flushed") { t.Errorf("an unflushed failure must warn about data loss, got: %v", err) } } -func contains(s, sub string) bool { - return len(s) >= len(sub) && (func() bool { - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false - })() -} - // requirePrivilegedUnmount skips when the process cannot call umount(2) // at all, so a CI failure means a real regression rather than a sandbox. func requirePrivilegedUnmount(t *testing.T) { t.Helper() - err := unix.Unmount(t.TempDir(), 0) - if err == unix.EPERM { + if err := unix.Unmount(t.TempDir(), 0); err == unix.EPERM { t.Skip("umount(2) needs CAP_SYS_ADMIN; runed has it in production, this environment does not") } } diff --git a/pkg/storage/driver/mountsync/mountsync_other.go b/pkg/storage/driver/mountsync/mountsync_other.go index 001c1636..1bda30b7 100644 --- a/pkg/storage/driver/mountsync/mountsync_other.go +++ b/pkg/storage/driver/mountsync/mountsync_other.go @@ -2,9 +2,13 @@ package mountsync -// The cloud volume drivers only mount on Linux nodes. These exist so the -// packages that call them still build for local development on macOS. - -func syncTarget(string) error { return nil } - -func unmountTarget(string, string) error { return nil } +import "errors" + +// mountsync.go is untagged, so this keeps the package compiling on +// darwin. Nothing outside this package calls it — each driver has its own +// !linux mount path — and it returns an error rather than nil so that +// whoever does wire it up finds out immediately, instead of getting a +// silent success on a data-loss-critical routine. +func unmountTarget(string, string) error { + return errors.New("mountsync: unmount is not supported on this platform") +} From 6bd01d4e9fb964f3c17001efcb8ee6e35f4bf8b1 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sat, 29 Aug 2026 13:03:26 +0100 Subject: [PATCH 3/6] docs(storage): the warning said unconditional; the code had a condition The comment re-audit found that the previous commit added a normative warning -- "the sync must stay unconditional" -- and then, four lines later in the same commit, gated it behind isMountPoint. A false warning is worse than none: it teaches the reader to distrust the doc at exactly the moment it needs to be obeyed, and leaves them unable to tell whether the doc or the guard is the mistake. The invariant that actually holds is narrower. The flush must come before the unmount and must never be conditional on the unmount -- not its outcome, not an error check, not a reordering. isMountPoint is a different kind of guard: it decides whether there is a volume filesystem here to flush at all, which is what keeps the idempotent path off the root disk. Also from the re-audit: * Two tests asserted CI lacks CAP_SYS_ADMIN, contradicted by the privileged CI job the same commit added. * The isMountPoint rationale and the superblock mechanism had each picked up a second and third statement a few metres from their canonical home -- the same duplication the previous round removed, reappearing at new sites. * A comment claimed this is "the only test that fails against the code this replaced", which is unfalsifiable from the tree. * Dropped a blast-radius sentence that framed the incident rather than informing anyone editing the package, and a dead fmt.Sprint() that existed only to justify an import. --- pkg/storage/driver/mountsync/mountsync.go | 17 +++++++++-------- .../mountsync/mountsync_e2e_linux_test.go | 14 ++++---------- .../driver/mountsync/mountsync_linux.go | 19 +++++++++---------- .../driver/mountsync/mountsync_linux_test.go | 12 ++++-------- 4 files changed, 26 insertions(+), 36 deletions(-) diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go index 8c56752c..9d82183f 100644 --- a/pkg/storage/driver/mountsync/mountsync.go +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -14,15 +14,16 @@ // single mount, bare umount(2) rc=0 90 bytes // second mount held, bare umount(2) rc=0 0 bytes // -// Zero-length files with correct names, owners and modes is the signature -// operators see. Any workload relying on ordinary write-back is exposed; -// databases escape only because they fsync their own journals. +// Zero-length files with correct names, owners and modes is the +// signature operators see. // -// The consequence for anyone editing this package: the sync must stay -// unconditional and must stay BEFORE the unmount. Moving it after, or -// behind an error check, reads as a cheap optimisation because umount(2) -// "already flushes" — and silently reopens the bug for every volume a -// container is holding. See issue #270. +// The consequence for anyone editing this package: the flush must stay +// BEFORE the unmount and must never become conditional on the unmount — +// moving it after, or behind an error check, reads as a cheap +// optimisation because umount(2) "already flushes", and silently +// reopens the bug for every volume a container is holding. The one +// guard that is safe is isMountPoint, which decides only whether there +// is a volume filesystem here to flush at all. See issue #270. package mountsync // Unmount flushes the filesystem at target and then unmounts it. driver diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go index ed19368f..75f65114 100644 --- a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -3,7 +3,6 @@ package mountsync import ( - "fmt" "os" "os/exec" "path/filepath" @@ -14,8 +13,7 @@ import ( ) // TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock is the test this -// package exists to pass, and the only one that fails against the code -// this replaced. +// package exists to pass. // // It builds the production shape on a real filesystem: an ext4 volume // mounted where the agent mounts one, with a SECOND mount of the same @@ -24,17 +22,14 @@ import ( // write-back workload — and then the raw device is captured, which is // what a detach hands back. // -// A bare umount(2) returns success here and flushes nothing, because it -// releases only one of two references to the superblock. That is the -// production bug: the agent logged "Volume unmounted" and detached a -// filesystem whose pages were still dirty. Unmount must recover the file -// intact where a bare umount(2) loses it. +// Against a bare umount(2) this fixture loses the file — that is the +// production bug. Unmount must recover it intact. func TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock(t *testing.T) { requireLoopMount(t) for _, tc := range []struct { name string - bare bool // tear down with a bare umount(2), as the old code did + bare bool // tear down with a bare umount(2) wantBytes int }{ {"bare umount(2) loses the write", true, 0}, @@ -158,5 +153,4 @@ func requireLoopMount(t *testing.T) { t.Skipf("cannot loop-mount here: %v (%s)", err, strings.TrimSpace(string(out))) } _ = exec.Command("umount", mnt).Run() - _ = fmt.Sprint() } diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go index 9ad3e45b..8dc77cc1 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux.go +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -27,8 +27,7 @@ func syncTarget(path string) error { } func unmountTarget(driver, target string) error { - // Flush first — see the package doc. Unconditional and before the - // unmount, both deliberately. + // Flush first, and before the unmount — see the package doc. var syncErr error if isMountPoint(target) { syncErr = syncTarget(target) @@ -46,9 +45,8 @@ func unmountTarget(driver, target string) error { return fmt.Errorf("%s: umount(2) %s: %w (the filesystem was NOT flushed first: %v; a detach now can lose unwritten data)", driver, target, err, syncErr) default: - // Deliberately a fact, not a reassurance. Anything the holder - // writes between the flush and the detach is still lost, and on - // this path the holder is usually a running container. + // Not a reassurance: on this path the holder is usually a running + // container, still writing. return fmt.Errorf("%s: umount(2) %s: %w (flushed as of now; writes made after this point are not on the disk)", driver, target, err) } @@ -58,11 +56,12 @@ func unmountTarget(driver, target string) error { // directory containing it, which for these drivers means a volume is // mounted there — they always mount a distinct block device. // -// It exists to keep the idempotent path off the root disk: on a target -// where nothing is mounted, the syncfs above would resolve to / and flush -// that instead, which is the whole-node stall syncTarget is written to -// avoid. It is advisory only, and any uncertainty answers "yes, sync it" — -// an unnecessary flush costs time, a skipped one costs data. +// It exists to keep the idempotent path off the root disk: where nothing +// is mounted, syncTarget would resolve to whatever the mount root sits on +// — on a default node, the root disk — which is the whole-node stall +// syncTarget is written to avoid. Advisory only: any uncertainty answers +// "yes, sync it", because an unnecessary flush costs time and a skipped +// one costs data. func isMountPoint(target string) bool { var self, parent unix.Stat_t if err := unix.Lstat(target, &self); err != nil { diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go index 7f98e6a1..24da33a8 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -45,17 +45,12 @@ func TestSyncTargetRejectsNonDirectory(t *testing.T) { // --- isMountPoint ---------------------------------------------------- -// A plain directory is not a mount point, so teardown must not flush the -// filesystem it happens to sit on — that would be the root disk, and the -// whole-node stall syncTarget exists to avoid. func TestIsMountPointFalseForPlainDirectory(t *testing.T) { if isMountPoint(t.TempDir()) { t.Error("a plain directory must not be treated as a mount point") } } -// Uncertainty resolves toward flushing: an unnecessary sync costs time, a -// skipped one costs data. func TestIsMountPointAssumesMountedWhenItCannotTell(t *testing.T) { if !isMountPoint(filepath.Join(t.TempDir(), "absent")) { t.Error("an unstattable target must be assumed mounted, so the flush still happens") @@ -68,7 +63,8 @@ func TestIsMountPointAssumesMountedWhenItCannotTell(t *testing.T) { // failed unmount reports success while the caller detaches a live // filesystem; too strict and ordinary idempotent teardown looks broken. // -// Table-driven because umount(2) needs CAP_SYS_ADMIN, which CI lacks. +// Table-driven so the classifier is covered by the ordinary CI job, +// which cannot call umount(2). func TestNotMountedClassification(t *testing.T) { cases := []struct { name string @@ -96,8 +92,8 @@ func TestNotMountedClassification(t *testing.T) { // Idempotence is only observable to a privileged caller: without // CAP_SYS_ADMIN the kernel answers EPERM before it evaluates the target. -// runed holds that capability in production; CI does not, so this skips -// rather than asserting something the environment cannot show. +// The ordinary CI job skips this; the privileged job in the CI workflow +// runs it. func TestUnmountIsIdempotentOnNonMountpoint(t *testing.T) { requirePrivilegedUnmount(t) if err := Unmount("test", t.TempDir()); err != nil { From f34a47f2670e4685316281a9331701ea9d85f5c2 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sat, 29 Aug 2026 13:09:25 +0100 Subject: [PATCH 4/6] fix(storage): stat through symlinks; stop claiming an unperformed flush Two reviewers independently found that isMountPoint, added in the last commit to keep the flush off the root disk, could skip the flush on a genuinely mounted volume. It used Lstat, while the two syscalls that act on the path afterwards -- syncTarget's open(2) and umount(2) -- both follow symlinks. For a symlinked target Lstat stats the link, which lives on the parent filesystem, so the gate answered "nothing mounted here", the flush was skipped, and the unmount then followed the link and succeeded. Silent data loss, introduced by the guard meant to make things safer. Measured: /real lstat_differs=true stat_differs=true /link lstat_differs=false stat_differs=true <- gate said no Stat in both calls. It strictly widens the "yes, flush" answer and makes the check agree with the calls it guards. The same guard also made syncErr==nil ambiguous: it meant either "flushed" or "decided there was nothing to flush", so a failed unmount on a path where nothing was mounted reported "flushed as of now". That is the shape when runed has lost CAP_SYS_ADMIN -- the mount never happened either -- so it is a message an operator would actually meet. Tracked separately now, with its own wording. Coverage, all verified by mutation rather than by passing: * A symlinked-target test that fails against the Lstat version. * The EBUSY branch, which had no test in any environment. A bind mount does not produce EBUSY (it is an independent reference); an open fd in the same namespace does. * The nothing-was-mounted branch, which runs in ordinary CI because it is only reachable unprivileged. * The privileged CI step set RUNE_REQUIRE_PRIVILEGED_MOUNT, so a degraded runner fails loudly instead of skipping every test and reporting success -- the silent-skip shape this package exists to end, which had reappeared one level up in CI. Also from the operability review: teardown logs the volume and target before unmounting, since the flush is the longest unattended pause in a shutdown and was previously silent; and the decision to leave the flush unbounded is now written down where someone would otherwise "fix" it with a deadline. --- .github/workflows/ci.yml | 3 +- internal/agent/volumes/subsystem.go | 10 ++ pkg/storage/driver/mountsync/mountsync.go | 8 ++ .../mountsync/mountsync_e2e_linux_test.go | 111 +++++++++++++++++- .../driver/mountsync/mountsync_linux.go | 22 +++- .../driver/mountsync/mountsync_linux_test.go | 28 ++++- 6 files changed, 172 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b3fa39c2..e9df92ea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,7 +69,8 @@ jobs: # container still holds — is asserted only by comments. - name: Volume teardown tests (privileged) run: | - docker run --rm --privileged -v "$PWD":/src -w /src golang:1.25 sh -c ' + docker run --rm --privileged -e RUNE_REQUIRE_PRIVILEGED_MOUNT=1 \ + -v "$PWD":/src -w /src golang:1.25 sh -c ' apt-get -qq update && apt-get -qq install -y e2fsprogs >/dev/null && go test ./pkg/storage/driver/mountsync/... -v' diff --git a/internal/agent/volumes/subsystem.go b/internal/agent/volumes/subsystem.go index 11061209..212aa4b6 100644 --- a/internal/agent/volumes/subsystem.go +++ b/internal/agent/volumes/subsystem.go @@ -676,6 +676,16 @@ func (s *Subsystem) bringUp(ctx context.Context, vol *types.Volume, id string) e // error. func (s *Subsystem) tearDown(ctx context.Context, id string, m trackedMount) (detached bool, err error) { opctx := s.teardownOpContext(ctx, id, m) + // Named before the call, not after: Unmount flushes the filesystem + // first, which on a volume with a lot of dirty data is the longest + // unattended pause in a shutdown. Without this line the operator sees + // systemd hang with no indication of which volume, or whether it is + // working at all. + s.log.Info("Unmounting volume", + log.Str("volume_id", id), + log.Str("namespace", m.VolumeNS), + log.Str("name", m.VolumeName), + log.Str("target", string(m.Target))) var firstErr error if uerr := m.Driver.Unmount(ctx, opctx, m.Target); uerr != nil { firstErr = fmt.Errorf("agent.volumes: unmount %s: %w", id, uerr) diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go index 9d82183f..871c1fac 100644 --- a/pkg/storage/driver/mountsync/mountsync.go +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -33,6 +33,14 @@ package mountsync // the caller detaches either way, so that is the difference between a // volume left attached and unwritten data discarded. // +// The flush is deliberately unbounded: there is no context parameter and +// no timeout. Cutting it short means detaching on a half-written +// filesystem, which is the failure this package exists to prevent, so a +// slow flush is the correct behaviour and not something to "fix" with a +// deadline. Note the ceiling is set elsewhere regardless — systemd's +// TimeoutStopSec then SIGKILL, and SIGKILL does not interrupt an +// in-flight syncfs. +// // Idempotent for a caller holding CAP_SYS_ADMIN — a target that is not a // mount point, or is already gone, returns nil. Unprivileged, umount(2) // answers EPERM before it looks at the target, and that surfaces as an diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go index 75f65114..ad049a52 100644 --- a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -130,27 +130,128 @@ func mustRun(t *testing.T, name string, args ...string) { // job in the CI workflow. func requireLoopMount(t *testing.T) { t.Helper() + // The CI job that exists to run these sets this. Without it a + // degraded runner — no CAP_SYS_ADMIN, no loop device, no mkfs — + // would skip every one of them and the step would still go green, + // which is the silent-skip failure this package was written to end. + mustRun := os.Getenv("RUNE_REQUIRE_PRIVILEGED_MOUNT") != "" + skip := func(format string, args ...any) { + if mustRun { + t.Fatalf("RUNE_REQUIRE_PRIVILEGED_MOUNT is set but "+format, args...) + } + t.Skipf(format, args...) + } if unix.Geteuid() != 0 { - t.Skip("needs root to mount a loop device") + skip("needs root to mount a loop device") } if _, err := exec.LookPath("mkfs.ext4"); err != nil { - t.Skip("needs mkfs.ext4") + skip("needs mkfs.ext4") } dir := t.TempDir() img := filepath.Join(dir, "probe.img") f, err := os.Create(img) if err != nil { - t.Skip("cannot create a probe image") + skip("cannot create a probe image") } _ = f.Truncate(8 << 20) _ = f.Close() if out, err := exec.Command("mkfs.ext4", "-q", "-F", img).CombinedOutput(); err != nil { - t.Skipf("cannot mkfs: %v (%s)", err, strings.TrimSpace(string(out))) + skip("cannot mkfs: %v (%s)", err, strings.TrimSpace(string(out))) } mnt := filepath.Join(dir, "probe") _ = os.MkdirAll(mnt, 0o755) if out, err := exec.Command("mount", "-o", "loop", img, mnt).CombinedOutput(); err != nil { - t.Skipf("cannot loop-mount here: %v (%s)", err, strings.TrimSpace(string(out))) + skip("cannot loop-mount here: %v (%s)", err, strings.TrimSpace(string(out))) } _ = exec.Command("umount", mnt).Run() } + +// TestUnmountSymlinkedTargetStillFlushes guards the gate against the two +// syscalls that act on the path after it. +// +// syncTarget's open(2) and umount(2) both follow symlinks. A mount-point +// check that did not would answer "nothing mounted here" for a symlinked +// target, skip the flush, and then unmount successfully — losing the +// writes it was there to protect. +func TestUnmountSymlinkedTargetStillFlushes(t *testing.T) { + requireLoopMount(t) + + dir := t.TempDir() + img := filepath.Join(dir, "disk.img") + real := filepath.Join(dir, "real") + link := filepath.Join(dir, "link") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, real) + if err := os.Symlink(real, link); err != nil { + t.Fatalf("symlink: %v", err) + } + + if err := os.WriteFile(filepath.Join(real, "SYSTEM"), nil, 0o644); err != nil { + t.Fatalf("create: %v", err) + } + mustRun(t, "sync") + // A second reference, as a container's bind would be, so a bare + // umount(2) cannot flush on its own. + second := filepath.Join(dir, "held") + if err := os.MkdirAll(second, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + mustRun(t, "mount", "--bind", real, second) + t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + + if err := os.WriteFile(filepath.Join(real, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + // Tear down through the symlink. + if err := Unmount("test", link); err != nil { + t.Fatalf("Unmount via symlink: %v", err) + } + if got := bytesOnDevice(t, dir, img); got != 90 { + t.Errorf("recovered %d bytes through a symlinked target, want 90 — the flush was skipped", got) + } +} + +// TestUnmountBusyTargetReportsFlushed covers the EBUSY branch, which had +// no test in any environment: a host-side holder keeps the mount busy, so +// umount(2) fails and the caller detaches anyway. The message is the only +// warning an operator gets, so assert its wording. +func TestUnmountBusyTargetReportsFlushed(t *testing.T) { + requireLoopMount(t) + + dir := t.TempDir() + img := filepath.Join(dir, "disk.img") + target := filepath.Join(dir, "mnt") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, target) + t.Cleanup(func() { _ = exec.Command("umount", "-l", target).Run() }) + + file := filepath.Join(target, "SYSTEM") + if err := os.WriteFile(file, []byte("held"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + // An open fd in this namespace is what makes umount(2) return EBUSY; + // a bind mount does not (it is an independent reference). + held, err := os.Open(file) + if err != nil { + t.Fatalf("open: %v", err) + } + defer func() { _ = held.Close() }() + + err = Unmount("test", target) + if err == nil { + t.Fatal("expected EBUSY with an open fd holding the mount") + } + if !strings.Contains(err.Error(), "flushed as of now") { + t.Errorf("a busy target was flushed, and the message must say so: %v", err) + } + if strings.Contains(err.Error(), "nothing was flushed") { + t.Errorf("the mount was real and was flushed; message claims otherwise: %v", err) + } +} diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go index 8dc77cc1..5b7557ac 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux.go +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -28,9 +28,13 @@ func syncTarget(path string) error { func unmountTarget(driver, target string) error { // Flush first, and before the unmount — see the package doc. - var syncErr error + var ( + syncErr error + synced bool + ) if isMountPoint(target) { syncErr = syncTarget(target) + synced = syncErr == nil } // No pre-probe on the unmount itself. A probe needs a subprocess // (findmnt), teardown runs during shutdown where a cancelled context @@ -44,6 +48,11 @@ func unmountTarget(driver, target string) error { case syncErr != nil: return fmt.Errorf("%s: umount(2) %s: %w (the filesystem was NOT flushed first: %v; a detach now can lose unwritten data)", driver, target, err, syncErr) + case !synced: + // Nothing was mounted here, so there was nothing to flush. Saying + // "flushed" would assert work that never happened. + return fmt.Errorf("%s: umount(2) %s: %w (nothing was mounted at this path, so nothing was flushed)", + driver, target, err) default: // Not a reassurance: on this path the holder is usually a running // container, still writing. @@ -64,10 +73,13 @@ func unmountTarget(driver, target string) error { // one costs data. func isMountPoint(target string) bool { var self, parent unix.Stat_t - if err := unix.Lstat(target, &self); err != nil { + // Stat, not Lstat: syncTarget's open(2) and umount(2) both follow + // symlinks, so a gate that did not would answer "nothing mounted here" + // for a symlinked target and skip the flush on a live volume. + if err := unix.Stat(target, &self); err != nil { return true // cannot tell: flush anyway } - if err := unix.Lstat(target+"/..", &parent); err != nil { + if err := unix.Stat(target+"/..", &parent); err != nil { return true } return self.Dev != parent.Dev @@ -83,6 +95,10 @@ func isMountPoint(target string) bool { // a silent no-op — the same class of bug as the findmnt probe this // replaced. runed holds CAP_SYS_ADMIN in production (it mounts these // volumes), so EPERM there is a real misconfiguration and must surface. +// +// EINVAL is trusted as "not a mount point" because runed runs in the host +// mount namespace. A runed inside its own namespace would get EINVAL for a +// volume it can see but not unmount, and that would read as success here. func notMounted(err error) bool { return errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOENT) } diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go index 24da33a8..f9025b36 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -3,6 +3,7 @@ package mountsync import ( + "errors" "os" "path/filepath" "strings" @@ -125,7 +126,32 @@ func TestUnmountReportsUnflushedFilesystem(t *testing.T) { // at all, so a CI failure means a real regression rather than a sandbox. func requirePrivilegedUnmount(t *testing.T) { t.Helper() - if err := unix.Unmount(t.TempDir(), 0); err == unix.EPERM { + if err := unix.Unmount(t.TempDir(), 0); errors.Is(err, unix.EPERM) { t.Skip("umount(2) needs CAP_SYS_ADMIN; runed has it in production, this environment does not") } } + +// TestUnmountNothingMountedSaysNothingFlushed covers the third message +// branch. Reporting "flushed as of now" when the target was never a mount +// point would assert work that did not happen — and that combination is +// not contrived: it is the shape when runed has lost CAP_SYS_ADMIN, since +// the mount never succeeded either. +// +// Runs unprivileged, which is where the branch is reachable: with the +// capability, umount(2) on a non-mount-point returns EINVAL and the call +// succeeds instead. +func TestUnmountNothingMountedSaysNothingFlushed(t *testing.T) { + if err := unix.Unmount(t.TempDir(), 0); !errors.Is(err, unix.EPERM) { + t.Skip("privileged: umount(2) succeeds on a non-mount-point, so this branch is unreachable") + } + err := Unmount("test", t.TempDir()) + if err == nil { + t.Fatal("expected the unprivileged umount to fail") + } + if !strings.Contains(err.Error(), "nothing was mounted") { + t.Errorf("must not claim a flush that never happened, got: %v", err) + } + if strings.Contains(err.Error(), "flushed as of now") { + t.Errorf("claims a flush on a path where nothing was mounted: %v", err) + } +} From 1e55ba44a66e6f6152dfd44684dadf0fbf51b999 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sun, 30 Aug 2026 17:46:20 +0100 Subject: [PATCH 5/6] test(storage): extract the held-mount fixture make lint runs a lint-complexity step beyond golangci-lint, and it flagged the e2e test at cognitive complexity 25 (> 20). I had run the linter directly rather than make lint, which is what CLAUDE.md asks for and what CI runs, so this only surfaced in CI. The fixture setup moves to heldMountFixture, which also gives the second mount of the superblock -- the part that models the container's bind -- a name and a place to explain itself. No behaviour change; the mutation check still fails both rows without the flush. --- .../mountsync/mountsync_e2e_linux_test.go | 71 ++++++++++++------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go index ad049a52..727152c2 100644 --- a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -36,33 +36,10 @@ func TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock(t *testing.T) { {"mountsync.Unmount keeps it", false, 90}, } { t.Run(tc.name, func(t *testing.T) { - dir := t.TempDir() - img := filepath.Join(dir, "disk.img") - target := filepath.Join(dir, "mnt") - second := filepath.Join(dir, "held") - for _, d := range []string{target, second} { - if err := os.MkdirAll(d, 0o755); err != nil { - t.Fatalf("mkdir: %v", err) - } - } - makeExt4(t, img) - mustRun(t, "mount", "-o", "loop", img, target) - - // Create and commit the file, so only its contents are at risk — - // the reported signature is a correctly-named zero-length file. - if err := os.WriteFile(filepath.Join(target, "SYSTEM"), nil, 0o644); err != nil { - t.Fatalf("create: %v", err) - } - mustRun(t, "sync") - - // The container's bind: a second reference to the superblock. - mustRun(t, "mount", "--bind", target, second) - t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + dir, img, target := heldMountFixture(t) // The workload writes and does not fsync. - if err := os.WriteFile(filepath.Join(target, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { - t.Fatalf("write: %v", err) - } + writeManifest(t, target) if tc.bare { if err := unix.Unmount(target, 0); err != nil { @@ -72,14 +49,54 @@ func TestUnmountFlushesWhenAnotherMountHoldsTheSuperblock(t *testing.T) { t.Fatalf("Unmount: %v", err) } - got := bytesOnDevice(t, dir, img) - if got != tc.wantBytes { + if got := bytesOnDevice(t, dir, img); got != tc.wantBytes { t.Errorf("recovered %d bytes from the raw device, want %d", got, tc.wantBytes) } }) } } +// heldMountFixture builds the production shape: an ext4 volume mounted +// where the agent mounts one, with a second mount of the same superblock +// standing in for the bind a container runtime makes into its own mount +// namespace. Returns the working dir, the backing image, and the mount +// target. +func heldMountFixture(t *testing.T) (dir, img, target string) { + t.Helper() + dir = t.TempDir() + img = filepath.Join(dir, "disk.img") + target = filepath.Join(dir, "mnt") + second := filepath.Join(dir, "held") + for _, d := range []string{target, second} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + } + makeExt4(t, img) + mustRun(t, "mount", "-o", "loop", img, target) + t.Cleanup(func() { _ = exec.Command("umount", "-l", target).Run() }) + + // Create and commit the file, so only its contents are at risk — the + // reported signature is a correctly-named zero-length file. + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), nil, 0o644); err != nil { + t.Fatalf("create: %v", err) + } + mustRun(t, "sync") + + mustRun(t, "mount", "--bind", target, second) + t.Cleanup(func() { _ = exec.Command("umount", "-l", second).Run() }) + return dir, img, target +} + +// writeManifest writes the 90-byte payload without fsync — an ordinary +// write-back workload. +func writeManifest(t *testing.T, target string) { + t.Helper() + if err := os.WriteFile(filepath.Join(target, "SYSTEM"), []byte(strings.Repeat("0", 90)), 0o644); err != nil { + t.Fatalf("write: %v", err) + } +} + // bytesOnDevice snapshots the backing file and reports the length of // SYSTEM as it exists on disk — what a detached volume would come back // with. From 04077d62260d4f45ce2856fc1db4e8f35ed5c205 Mon Sep 17 00:00:00 2001 From: Oreofe Date: Sun, 30 Aug 2026 18:58:07 +0100 Subject: [PATCH 6/6] docs(agent): the teardown timeout no longer bounds the flush Final review round found teardownFallbackTimeout's comment describing a tree that no longer exists: it claims to bound a single volume's teardown, but as of this branch the flush inside Driver.Unmount takes no context and is deliberately unbounded, so Stop can outlive any deadline its caller sets. The decision itself was recorded in mountsync's package doc. The person debugging a hung shutdown is reading subsystem.go, so it needs to be legible there too. Also: a bool named mustRun shadowed the package's mustRun helper in the e2e tests, which compiles only until someone calls the helper inside that function; and the not-mounted message asserted as fact what isMountPoint answers advisorily -- it now says 'nothing appeared to be mounted'. --- internal/agent/volumes/subsystem.go | 14 ++++++++++---- pkg/storage/driver/mountsync/mountsync.go | 5 ++--- .../driver/mountsync/mountsync_e2e_linux_test.go | 4 ++-- pkg/storage/driver/mountsync/mountsync_linux.go | 2 +- .../driver/mountsync/mountsync_linux_test.go | 2 +- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/internal/agent/volumes/subsystem.go b/internal/agent/volumes/subsystem.go index 212aa4b6..62c8b6d4 100644 --- a/internal/agent/volumes/subsystem.go +++ b/internal/agent/volumes/subsystem.go @@ -290,10 +290,16 @@ func (s *Subsystem) Stop(ctx context.Context) error { return nil } -// teardownFallbackTimeout bounds a single volume's teardown when the -// caller's context carries no deadline of its own. Provider clients set -// their own (longer) HTTP timeouts, which must not be what decides how -// long shutdown takes. +// teardownFallbackTimeout bounds a single volume's PROVIDER calls when +// the caller's context carries no deadline of its own. Provider clients +// set their own (longer) HTTP timeouts, which must not be what decides +// how long shutdown takes. +// +// It does not bound the whole teardown. The filesystem flush inside +// Driver.Unmount takes no context and is deliberately unbounded, because +// cutting it short detaches a half-written filesystem — so a shutdown can +// legitimately outlive this and any deadline the caller sets. See +// pkg/storage/driver/mountsync. const teardownFallbackTimeout = 8 * time.Second // drainMounts tears down every tracked mount concurrently. diff --git a/pkg/storage/driver/mountsync/mountsync.go b/pkg/storage/driver/mountsync/mountsync.go index 871c1fac..62cacdc5 100644 --- a/pkg/storage/driver/mountsync/mountsync.go +++ b/pkg/storage/driver/mountsync/mountsync.go @@ -37,9 +37,8 @@ package mountsync // no timeout. Cutting it short means detaching on a half-written // filesystem, which is the failure this package exists to prevent, so a // slow flush is the correct behaviour and not something to "fix" with a -// deadline. Note the ceiling is set elsewhere regardless — systemd's -// TimeoutStopSec then SIGKILL, and SIGKILL does not interrupt an -// in-flight syncfs. +// deadline. Nothing bounds it: SIGKILL does not interrupt an in-flight +// syncfs, so not even systemd's TimeoutStopSec is a ceiling on this. // // Idempotent for a caller holding CAP_SYS_ADMIN — a target that is not a // mount point, or is already gone, returns nil. Unprivileged, umount(2) diff --git a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go index 727152c2..42fab335 100644 --- a/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_e2e_linux_test.go @@ -151,9 +151,9 @@ func requireLoopMount(t *testing.T) { // degraded runner — no CAP_SYS_ADMIN, no loop device, no mkfs — // would skip every one of them and the step would still go green, // which is the silent-skip failure this package was written to end. - mustRun := os.Getenv("RUNE_REQUIRE_PRIVILEGED_MOUNT") != "" + required := os.Getenv("RUNE_REQUIRE_PRIVILEGED_MOUNT") != "" skip := func(format string, args ...any) { - if mustRun { + if required { t.Fatalf("RUNE_REQUIRE_PRIVILEGED_MOUNT is set but "+format, args...) } t.Skipf(format, args...) diff --git a/pkg/storage/driver/mountsync/mountsync_linux.go b/pkg/storage/driver/mountsync/mountsync_linux.go index 5b7557ac..f9f44ab2 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux.go +++ b/pkg/storage/driver/mountsync/mountsync_linux.go @@ -51,7 +51,7 @@ func unmountTarget(driver, target string) error { case !synced: // Nothing was mounted here, so there was nothing to flush. Saying // "flushed" would assert work that never happened. - return fmt.Errorf("%s: umount(2) %s: %w (nothing was mounted at this path, so nothing was flushed)", + return fmt.Errorf("%s: umount(2) %s: %w (nothing appeared to be mounted at this path, so nothing was flushed)", driver, target, err) default: // Not a reassurance: on this path the holder is usually a running diff --git a/pkg/storage/driver/mountsync/mountsync_linux_test.go b/pkg/storage/driver/mountsync/mountsync_linux_test.go index f9025b36..92295510 100644 --- a/pkg/storage/driver/mountsync/mountsync_linux_test.go +++ b/pkg/storage/driver/mountsync/mountsync_linux_test.go @@ -148,7 +148,7 @@ func TestUnmountNothingMountedSaysNothingFlushed(t *testing.T) { if err == nil { t.Fatal("expected the unprivileged umount to fail") } - if !strings.Contains(err.Error(), "nothing was mounted") { + if !strings.Contains(err.Error(), "nothing appeared to be mounted") { t.Errorf("must not claim a flush that never happened, got: %v", err) } if strings.Contains(err.Error(), "flushed as of now") {