fix(storage): flush the filesystem before unmount/detach - #271
Merged
Conversation
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.
This was referenced Aug 29, 2026
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.
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.
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.
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.
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'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #270.
Detaching a cloud volume discards anything still in the page cache, so the unmount has to flush first.
umount(2)only does that when it releases the last reference to the superblock — and a running container holds a second one, because the runtime binds the mount into its own namespace. So the unmount returned success having flushed nothing, and the detach threw the writes away:On disk that is files with the right name, owner and mode, and zero length. Any service relying on ordinary write-back is exposed; databases escape because they fsync their own journals.
All four cloud drivers carried near-identical copies of this routine, so all four lost data the same way.
Changes
pkg/storage/driver/mountsync:syncfs(2)the mount point, thenumount(2). One copy instead of four.findmntprobe from the unmount path. It ran viaexec.CommandContext, could not start on a cancelled context, and that failure was read as "not mounted" — skipping the unmount entirely, during shutdown, which is where contexts expire.EPERMis not treated as "nothing mounted": unprivilegedumount(2)fails before the kernel looks at the target, so folding it in would restore the silent no-op.Tests
TestUnmountFlushesWhenAnotherMountHoldsTheSuperblockbuilds the production shape on loop-backed ext4 and asserts a bareumount(2)loses the write wheremountsync.Unmountkeeps it. It fails against the code this replaces.umount(2)and loop mounts needCAP_SYS_ADMIN, which the CI runner lacks, so a privileged step runs the package withRUNE_REQUIRE_PRIVILEGED_MOUNT=1— without it a degraded runner would skip every test and still go green.Does not address the container/teardown ordering (#272), or the provisioning path #270 reports.