fix(storage): flush the filesystem before unmount/detach

.github/workflows/ci.yml

@@ -62,6 +62,17 @@ 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 -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'
 
   build:
     name: Build

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.
@@ -676,6 +682,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)

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,7 @@ 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 {
+	return mountsync.Unmount("awsebs", target)
 }

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,7 @@ 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 {
+	return mountsync.Unmount("dovolume", target)
 }

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,7 @@ 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 {
+	return mountsync.Unmount("gcepd", target)
 }

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,7 @@ 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
+// Unmount on Linux flushes the filesystem, then calls umount2(2).
+func (execMounter) Unmount(_ context.Context, target string) error {
+	return mountsync.Unmount("hcloudvolume", target)
 }

pkg/storage/driver/mountsync/mountsync.go

@@ -0,0 +1,49 @@
+// Package mountsync flushes a mounted filesystem and unmounts it, for the
+// cloud volume drivers that then detach the underlying disk.
+//
+// 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.
+//
+// Measured on loop-backed ext4, writing a 90-byte file without fsync and
+// then reading back the raw device (what a detach hands you):
+//
+//	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.
+//
+// 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
+// names the calling driver for error messages ("gcepd").
+//
+// 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.
+//
+// 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. 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)
+// 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)
+}