From 061eb7c0045b0bfc43931a8f19b4d1786ecdf0a3 Mon Sep 17 00:00:00 2001
From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com>
Date: Mon, 10 Aug 2026 05:56:06 +0100
Subject: [PATCH] =?UTF-8?q?volume-manager:=20drop=20the=20medium-event=20d?=
 =?UTF-8?q?edup=20=E2=80=94=20it=20only=20ever=20misfired=20(S5=20review)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The adversarial S5 review found a real, unrecoverable defect: onMediumEvent
deduped medium_changed events on a module-global last_medium_change compared by
equality against the event's change_count. But change_count is a PER-DRIVER
counter that restarts at 0 in every usb-storage instance — it is bumped +%=1 and
published only on a real medium transition, so within one instance every count
is unique and monotonic and an equality dedup can never legitimately fire.

The global was carried across a driver restart — S5's OWN crash-rebuild path —
so a fresh instance's first eject (count=1) collided with a stale last==1 and was
dropped. removeDevice never ran; the filesystem kept serving I/O against absent
media forever, and nothing else recovered it: onGeometry answers from a cached
block_count so channelAlive stays true, and isDevicePresent stays true (the
device never left the tree). medium_changed is the sole eject oracle there.

The dedup guarded a re-delivery the driver already makes impossible, and its
only observable effect was this bug. Remove it: react to each present-edge
directly. Both branches are idempotent (a freed device stops matching dev.used)
and the poll reconciles, so acting on every genuine edge is safe — and a fresh
driver instance's counter can no longer be mistaken for the previous one's.

Verified: build + bounds green; volume-removal, volume-medium-change and
volume-driver-restart all pass, so both removal paths survive the change. The
driver-restart-then-eject intersection that triggered the collision cannot be
staged in QEMU — the internal driver-kill cannot be ordered against a QMP eject,
and a usb-storage device_add is not re-presented — so the guarantee rests on the
driver's one-publish-per-transition-with-unique-count contract.
---
 system/services/volume-manager/volume-manager.zig | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/system/services/volume-manager/volume-manager.zig b/system/services/volume-manager/volume-manager.zig
index 2235d40..fea4913 100644
--- a/system/services/volume-manager/volume-manager.zig
+++ b/system/services/volume-manager/volume-manager.zig
@@ -555,8 +555,6 @@ fn onNotification(badge: u64) void {
     }
 }
 
-var last_medium_change: u32 = 0;
-
 fn anyVolumeOn(device_id: u64) bool {
     for (&volumes) |*v| if (v.used and v.device_id == device_id) return true;
     return false;
@@ -570,10 +568,18 @@ fn anyVolumeOn(device_id: u64) bool {
 /// no device, so `absent` retires every adopted device (its volumes unmount and
 /// the poll re-adopts the still-present device with its now-empty medium), and
 /// `present` frees any empty adopted device so the poll re-probes and remounts it.
+///
+/// We act on every edge and do NOT dedup on `change_count`. The driver publishes
+/// exactly once per transition, each with a unique monotonic count, so a count is
+/// never legitimately re-sent within one subscription — an equality dedup could
+/// only ever fire spuriously, and it did: `change_count` restarts at 0 in each
+/// driver instance (usb-storage.zig), so a global "last count" carried across a
+/// driver restart (S5's own crash-rebuild) mistook the fresh instance's first
+/// edge for a re-delivery and dropped a real eject, wedging a mount over absent
+/// media. Both branches are idempotent (a freed device stops matching `dev.used`)
+/// and the poll reconciles, so reacting to each genuine edge is safe.
 fn onMediumEvent(payload: []const u8) void {
     const event = block.decodeMediumChanged(payload) orelse return;
-    if (event.change_count == last_medium_change) return; // a coalesced or re-delivered edge
-    last_medium_change = event.change_count;
     if (event.present == 0) {
         std.log.info("medium left a storage device; unmounting its volume(s)", .{});
         for (&devices) |*dev| {