Reconciler: uid/gid and ntfsfix for NTFS secondary mounts.

QA on 10.0.0.8 (mount-issues.md findings 2 and 3) showed the reconciler
path missed both NTFS accommodations that mount_drive() and
add_fstab_entry() already have: secondary NTFS drives mounted owned by
root (writable only via ntfs-3g's permissive default), and a dirty NTFS
volume would fail to mount at boot with no cleanup attempted.

Inject uid/gid for ntfs/ntfs3 in the reconciler, and run ntfsfix -d in
SubprocessMountExecutor before NTFS mounts (translating UUID=/LABEL=
specs to /dev/disk/by-* paths, which ntfsfix needs).  Also return an
error MountResult instead of raising when a binary is missing, per the
executor protocol.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

controller/lib/mount_executor.py

@@ -59,6 +59,18 @@ class MountExecutor(Protocol):
 MOUNT_TIMEOUT_SECONDS = 30
 
 
+def _device_path(device: str) -> str:
+    """Translate a UUID=/LABEL= mount spec to a real device node.
+
+    mount(8) resolves these specs itself, but tools like ntfsfix need a
+    path.  udev maintains the by-uuid/by-label symlinks."""
+    if device.startswith("UUID="):
+        return f"/dev/disk/by-uuid/{device[len('UUID='):]}"
+    if device.startswith("LABEL="):
+        return f"/dev/disk/by-label/{device[len('LABEL='):]}"
+    return device
+
+
 class SubprocessMountExecutor:
     """Executes mount/umount via subprocess and parses /proc/mounts."""
 
@@ -72,6 +84,17 @@ class SubprocessMountExecutor:
         fstype: str,
         options: str,
     ) -> MountResult:
+        # NTFS volumes are frequently left "dirty" (Windows Fast Startup, or
+        # an unclean unplug), which makes the driver refuse to mount them.
+        # ntfsfix -d clears the dirty flag and replays the journal; it is a
+        # no-op on clean volumes.  Never fatal: the mount below reports the
+        # real error.  Matches mount_drive() in lib/disks.py.
+        if fstype in ("ntfs", "ntfs3"):
+            fix = self._run(["ntfsfix", "-d", _device_path(device)],
+                            label=f"ntfsfix {device}")
+            if not fix.ok:
+                logger.warning("ntfsfix failed for %s (mount will still be attempted): %s",
+                               device, fix.error)
         cmd = ["mount", "-t", fstype, "-o", options, device, mount_point]
         return self._run(cmd, label=f"mount {device} {mount_point}")
 
@@ -104,6 +127,10 @@ class SubprocessMountExecutor:
                 ok=False,
                 error=f"{label}: timed out after {self._timeout}s",
             )
+        except OSError as e:
+            # e.g. FileNotFoundError for a missing binary — the executor
+            # protocol forbids raising for I/O failures.
+            return MountResult(ok=False, error=f"{label}: {e}")
         if result.returncode != 0:
             return MountResult(
                 ok=False,

controller/lib/reconciler.py

@@ -149,11 +149,11 @@ class Reconciler:
         return None
 
     def _inject_uid_gid(self, entry: FstabEntry) -> str:
-        """exfat/vfat have no POSIX permissions; without uid/gid the
+        """exfat/vfat/ntfs have no POSIX permissions; without uid/gid the
         mounted directory belongs to root and the wrolpi user cannot write
-        to it."""
+        to it.  Covers both NTFS drivers, matching mount_drive()."""
         options = entry.options or "defaults"
-        if entry.fstype not in ("exfat", "vfat"):
+        if entry.fstype not in ("exfat", "vfat", "ntfs", "ntfs3"):
             return options
         if "uid=" in options or "gid=" in options:
             return options

controller/test/test_mount_executor.py

@@ -124,6 +124,67 @@ class TestSubprocessMountExecutor:
         assert calls == [["mount", "-t", "ext4", "-o", "defaults",
                           "UUID=1", "/media/x"]]
 
+    def test_ntfs_mount_runs_ntfsfix_first(self, monkeypatch):
+        """NTFS mounts run ntfsfix -d first to clear the Windows dirty flag;
+        UUID= specs are translated to /dev/disk/by-uuid for ntfsfix."""
+        calls = []
+
+        def fake_run(cmd, **kwargs):
+            calls.append(cmd)
+            return _FakeCompletedProcess(returncode=0)
+
+        monkeypatch.setattr(
+            "controller.lib.mount_executor.subprocess.run", fake_run,
+        )
+        executor = SubprocessMountExecutor()
+        result = executor.mount("UUID=ABC123", "/media/x", "ntfs", "defaults")
+        assert result == MountResult(ok=True)
+        assert calls[0] == ["ntfsfix", "-d", "/dev/disk/by-uuid/ABC123"]
+        assert calls[1][0] == "mount"
+
+    def test_ntfsfix_failure_does_not_block_mount(self, monkeypatch):
+        def fake_run(cmd, **kwargs):
+            if cmd[0] == "ntfsfix":
+                return _FakeCompletedProcess(returncode=1, stderr="dirty")
+            return _FakeCompletedProcess(returncode=0)
+
+        monkeypatch.setattr(
+            "controller.lib.mount_executor.subprocess.run", fake_run,
+        )
+        executor = SubprocessMountExecutor()
+        result = executor.mount("/dev/sda1", "/media/x", "ntfs3", "defaults")
+        assert result == MountResult(ok=True)
+
+    def test_ext4_mount_does_not_run_ntfsfix(self, monkeypatch):
+        calls = []
+
+        def fake_run(cmd, **kwargs):
+            calls.append(cmd)
+            return _FakeCompletedProcess(returncode=0)
+
+        monkeypatch.setattr(
+            "controller.lib.mount_executor.subprocess.run", fake_run,
+        )
+        executor = SubprocessMountExecutor()
+        executor.mount("UUID=1", "/media/x", "ext4", "defaults")
+        assert len(calls) == 1
+        assert calls[0][0] == "mount"
+
+    def test_missing_binary_returns_error_instead_of_raising(self, monkeypatch):
+        """The executor protocol forbids raising for I/O failures; a missing
+        binary (FileNotFoundError) must come back as MountResult(ok=False)."""
+
+        def fake_run(cmd, **kwargs):
+            raise FileNotFoundError(2, "No such file or directory", cmd[0])
+
+        monkeypatch.setattr(
+            "controller.lib.mount_executor.subprocess.run", fake_run,
+        )
+        executor = SubprocessMountExecutor()
+        result = executor.mount("UUID=1", "/media/x", "ext4", "defaults")
+        assert result.ok is False
+        assert "No such file" in result.error
+
     def test_mount_propagates_stderr(self, monkeypatch):
         def fake_run(cmd, **kwargs):
             return _FakeCompletedProcess(returncode=32, stderr="busy")

controller/test/test_reconciler.py

@@ -242,6 +242,17 @@ class TestUidGidInjection:
         assert "uid=2000" in opts
         assert "gid=2000" in opts
 
+    def test_injects_uid_gid_for_ntfs(self, tmp_path):
+        """ntfs/ntfs3 have no POSIX permissions either; secondary NTFS drives
+        must mount owned by the wrolpi user like exfat/vfat do."""
+        fstab = FstabFile(mounts=[_entry(fstype="ntfs")])
+        rec, fake = make_reconciler(tmp_path, fstab=fstab,
+                                    wrolpi_uid=2000, wrolpi_gid=2000)
+        rec.apply()
+        opts = fake.mount_calls[0].options
+        assert "uid=2000" in opts
+        assert "gid=2000" in opts
+
     def test_does_not_double_inject_if_user_already_supplied(self, tmp_path):
         fstab = FstabFile(mounts=[_entry(fstype="vfat",
                                          options="defaults,uid=1234,gid=1234")])