From ff7d48c56186b7cacbacda256f54d431b8d7f915 Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 03:04:00 +0200 Subject: [PATCH 1/8] Fail closed on unreachable macOS network mounts --- vireo/app.py | 95 ++++++++++++++++++++------- vireo/tests/test_app.py | 139 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+), 23 deletions(-) diff --git a/vireo/app.py b/vireo/app.py index 936556061..01b92c09c 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -1735,8 +1735,30 @@ def _network_volume_roots(run=subprocess.run): } +def _reap_abandoned_network_probe(process): + """Reap a timed-out probe away from the request path.""" + try: + process.communicate() + except (OSError, ValueError, subprocess.SubprocessError): + pass + + +def _abandon_network_probe(process): + """Kill a timed-out probe without synchronously waiting for it.""" + try: + process.kill() + except OSError: + pass + threading.Thread( + target=_reap_abandoned_network_probe, + args=(process,), + name="vireo-network-probe-reaper", + daemon=True, + ).start() + + def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, - run=subprocess.run): + run=None, popen=subprocess.Popen): """Bounded, out-of-process reachability probe for a mounted network root. ``mount`` listing a share and Finder reporting a file's absence are @@ -1746,9 +1768,10 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, cached parent metadata even though the photo will reappear on reconnect. Repeating the same Finder query does not detect this — it reuses the same cache. A ``stat`` on the mount root in a bounded - subprocess is an *independent* signal: it does not touch Finder, and - the subprocess is killed on timeout so an unresponsive share cannot - hang the caller. + subprocess is an *independent* signal: it does not touch Finder. On + timeout the child is killed and reaped on a daemon thread so an + uninterruptible filesystem call cannot hold the request thread in + Python's usual synchronous kill-and-wait timeout cleanup. Returns ``True`` only when ``stat`` completed in time and reported the root as a directory. Any other outcome (timeout, non-zero exit, error) @@ -1756,18 +1779,36 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, """ if sys.platform != "darwin": return False + argv = ["/usr/bin/stat", "-f", "%HT", root] + if run is not None: + try: + result = run( + argv, capture_output=True, text=True, timeout=timeout, + **no_window_kwargs(), + ) + except (OSError, subprocess.SubprocessError): + return False + return ( + result.returncode == 0 + and (result.stdout or "").strip() == "Directory" + ) try: - result = run( - ["/usr/bin/stat", "-f", "%HT", root], - capture_output=True, text=True, - timeout=timeout, + process = popen( + argv, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, **no_window_kwargs(), ) - except (OSError, subprocess.SubprocessError): + stdout, _stderr = process.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + _abandon_network_probe(process) return False - if result.returncode != 0: + except (OSError, subprocess.SubprocessError): + if "process" in locals(): + _abandon_network_probe(process) return False - return (result.stdout or "").strip() == "Directory" + return process.returncode == 0 and (stdout or "").strip() == "Directory" def _expand_first_symlink_prefix(filepath): @@ -1809,19 +1850,27 @@ def _expand_first_symlink_prefix(filepath): def _path_on_network_volume(filepath, network_roots): """Whether ``filepath`` should avoid in-process mounted-volume I/O.""" normalized = os.path.normpath(os.path.abspath(filepath)) + if sys.platform == "darwin" and network_roots is None: + # Discovery failed, so there is no trustworthy evidence that any + # candidate is local. Route every macOS path through bounded Finder + # handling instead of risking an in-process stat on a custom mount. + return True for _depth in range(16): - if network_roots is None: - # Mount discovery failed. Prefer bounded Finder handling for any - # /Volumes path, including one reached through a local symlink. - if _volume_root_for_path(normalized) is not None: - return True - else: - for root in network_roots: - try: - if os.path.commonpath((normalized, root)) == root: - return True - except ValueError: - continue + # A detached network mount disappears from a successful mount-table + # result but leaves its /Volumes directory behind. That absence cannot + # prove the catalog path is local, so keep all such paths off in-process + # filesystem calls even when no current network root matches. + if ( + sys.platform == "darwin" + and _volume_root_for_path(normalized) is not None + ): + return True + for root in network_roots: + try: + if os.path.commonpath((normalized, root)) == root: + return True + except ValueError: + continue if sys.platform != "darwin": return False expanded = _expand_first_symlink_prefix(normalized) diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index 31fd8e8f9..7a66ba84c 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -5020,6 +5020,40 @@ def fake_run(argv, **kwargs): ) +def test_network_root_reachable_does_not_wait_to_reap_timeout(monkeypatch): + """A wedged probe is reaped off-thread after the caller returns.""" + import subprocess + import threading + + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + reaper_started = threading.Event() + release_reaper = threading.Event() + reaper_finished = threading.Event() + + class WedgedProcess: + returncode = None + + def communicate(self, timeout=None): + if timeout is not None: + raise subprocess.TimeoutExpired(["stat"], timeout) + reaper_started.set() + release_reaper.wait(timeout=2) + reaper_finished.set() + return "", "" + + def kill(self): + return None + + assert app_module._network_root_reachable( + "/Volumes/NAS", popen=lambda *args, **kwargs: WedgedProcess(), + ) is False + assert reaper_started.wait(timeout=1) + release_reaper.set() + assert reaper_finished.wait(timeout=1) + + def test_network_root_reachable_returns_false_off_mac(monkeypatch): """The probe only runs on macOS. On other platforms callers already do not route through Finder, so the probe never needs to answer @@ -5042,6 +5076,111 @@ def fake_run(*a, **k): assert called == [] +def test_path_on_network_volume_fails_closed_for_detached_volume(monkeypatch): + """An absent mount-table entry does not make /Volumes paths local.""" + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: "/Volumes/NAS", + ) + + assert app_module._path_on_network_volume( + "/Volumes/NAS/bird.NEF", set(), + ) is True + + +def test_path_on_network_volume_fails_closed_when_discovery_fails(monkeypatch): + """A failed mount query protects custom macOS mount locations too.""" + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: None, + ) + + assert app_module._path_on_network_volume( + "/Users/me/mnt/photos/bird.NEF", None, + ) is True + + +def test_trash_paths_preserves_detached_volume_missing_from_mount_table( + monkeypatch, tmp_path, +): + """Initial discovery cannot mistake a detached /Volumes path for local.""" + import app as app_module + + volume = tmp_path / "NAS" + photo = str(volume / "bird.NEF") + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: str(volume), + ) + monkeypatch.setattr(app_module, "_network_volume_roots", lambda: set()) + monkeypatch.setattr( + app_module, "_snapshot_parent_device", + lambda _path: (_ for _ in ()).throw( + AssertionError("detached network path reached parent stat"), + ), + ) + monkeypatch.setattr( + app_module.os.path, "isfile", + lambda _path: (_ for _ in ()).throw( + AssertionError("detached network path reached source stat"), + ), + ) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda paths: (set(), set(paths), []), + ) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths([photo]) + + assert moved == 0 + assert successful == set() + assert failures == [{"path": photo, "error": "Source path is unreachable"}] + + +def test_trash_paths_routes_custom_path_when_mount_discovery_fails( + monkeypatch, tmp_path, +): + """A failed mount query keeps custom mounts off in-process I/O.""" + import app as app_module + + photo = str(tmp_path / "custom-mount" / "bird.NEF") + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr(app_module, "_network_volume_roots", lambda: None) + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: None, + ) + monkeypatch.setattr( + app_module, "_snapshot_parent_device", + lambda _path: (_ for _ in ()).throw( + AssertionError("custom network path reached parent stat"), + ), + ) + monkeypatch.setattr( + app_module.os.path, "isfile", + lambda _path: (_ for _ in ()).throw( + AssertionError("custom network path reached source stat"), + ), + ) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths([photo]) + + assert moved == 1 + assert successful == {photo} + assert failures == [] + + def test_delete_loser_files_preserves_endpoint_network_classification( monkeypatch, tmp_path, ): From abed1be31eb83beceb4c61334bcb7d438612f865 Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 03:15:08 +0200 Subject: [PATCH 2/8] Preserve discovery fallback on test hosts --- vireo/app.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vireo/app.py b/vireo/app.py index 01b92c09c..b5115dc4b 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -1850,11 +1850,15 @@ def _expand_first_symlink_prefix(filepath): def _path_on_network_volume(filepath, network_roots): """Whether ``filepath`` should avoid in-process mounted-volume I/O.""" normalized = os.path.normpath(os.path.abspath(filepath)) - if sys.platform == "darwin" and network_roots is None: + if network_roots is None: # Discovery failed, so there is no trustworthy evidence that any # candidate is local. Route every macOS path through bounded Finder # handling instead of risking an in-process stat on a custom mount. - return True + # Preserve the explicit /Volumes fallback on non-macOS test hosts. + return ( + sys.platform == "darwin" + or _volume_root_for_path(normalized) is not None + ) for _depth in range(16): # A detached network mount disappears from a successful mount-table # result but leaves its /Volumes directory behind. That absence cannot From 7d5ede5c375bc60a031006cef69715c038eb78d0 Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 03:18:47 +0200 Subject: [PATCH 3/8] Preserve live local volume trash routing --- vireo/app.py | 62 +++++++++++++++++++++++++++++++---------- vireo/tests/test_app.py | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 14 deletions(-) diff --git a/vireo/app.py b/vireo/app.py index b5115dc4b..d982abfe3 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -1695,6 +1695,31 @@ def _file_manager_labels(): _NETWORK_ROOTS_UNSET = object() +class _NetworkVolumeRoots(set): + """Network roots plus the live /Volumes roots from one mount snapshot.""" + + def __init__(self, network_roots=(), mounted_volume_roots=()): + super().__init__(network_roots) + self.mounted_volume_roots = frozenset(mounted_volume_roots) + + +def _mounted_volume_roots_from_mount_output(text): + """Return live top-level ``/Volumes/`` mount points.""" + roots = set() + for line in text.splitlines(): + before_options, separator, _options = line.strip().rpartition(" (") + if not separator: + continue + _source, separator, mount_point = before_options.partition(" on ") + if not separator: + continue + normalized = posixpath.normpath(mount_point) + parts = normalized.split("/") + if len(parts) == 3 and parts[1] == "Volumes" and parts[2]: + roots.add(normalized) + return roots + + def _volume_root_for_path(filepath): """Return ``/Volumes/`` for a path on a macOS mounted volume.""" try: @@ -1727,12 +1752,16 @@ def _network_volume_roots(run=subprocess.run): return None if result.returncode != 0: return None - return { + output = result.stdout or "" + return _NetworkVolumeRoots( # ``mount`` always reports macOS/POSIX paths. Keep parsing independent # of the host running the test suite (notably Windows' ``ntpath``). - posixpath.normpath(row["mount_point"]) - for row in remote_setup.parse_mount_output(result.stdout or "") - } + ( + posixpath.normpath(row["mount_point"]) + for row in remote_setup.parse_mount_output(output) + ), + _mounted_volume_roots_from_mount_output(output), + ) def _reap_abandoned_network_probe(process): @@ -1792,6 +1821,7 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, result.returncode == 0 and (result.stdout or "").strip() == "Directory" ) + process = None try: process = popen( argv, @@ -1805,7 +1835,7 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, _abandon_network_probe(process) return False except (OSError, subprocess.SubprocessError): - if "process" in locals(): + if process is not None: _abandon_network_probe(process) return False return process.returncode == 0 and (stdout or "").strip() == "Directory" @@ -1860,21 +1890,25 @@ def _path_on_network_volume(filepath, network_roots): or _volume_root_for_path(normalized) is not None ) for _depth in range(16): - # A detached network mount disappears from a successful mount-table - # result but leaves its /Volumes directory behind. That absence cannot - # prove the catalog path is local, so keep all such paths off in-process - # filesystem calls even when no current network root matches. - if ( - sys.platform == "darwin" - and _volume_root_for_path(normalized) is not None - ): - return True for root in network_roots: try: if os.path.commonpath((normalized, root)) == root: return True except ValueError: continue + if sys.platform == "darwin": + volume_root = _volume_root_for_path(normalized) + if volume_root is not None: + mounted_roots = getattr( + network_roots, "mounted_volume_roots", None, + ) + # A detached network mount disappears from the snapshot but + # leaves its /Volumes directory behind. Conversely, a live + # local USB/APFS root in the same snapshot must retain the + # local-trash path. Plain sets from older callers/tests carry + # no liveness evidence, so continue to fail closed for them. + if mounted_roots is None or volume_root not in mounted_roots: + return True if sys.platform != "darwin": return False expanded = _expand_first_symlink_prefix(normalized) diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index 7a66ba84c..c8703874a 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -4021,6 +4021,11 @@ def fake_run(argv, **kwargs): assert app_module._network_volume_roots(run=fake_run) == { "/Volumes/Photography", } + assert app_module._network_volume_roots( + run=fake_run, + ).mounted_volume_roots == { + "/Volumes/Photography", "/Volumes/CARD", + } assert calls[0][0] == ["mount"] assert calls[0][1]["timeout"] == app_module._MOUNT_QUERY_TIMEOUT_SECS @@ -5049,6 +5054,7 @@ def kill(self): assert app_module._network_root_reachable( "/Volumes/NAS", popen=lambda *args, **kwargs: WedgedProcess(), ) is False + assert not reaper_finished.is_set() assert reaper_started.wait(timeout=1) release_reaper.set() assert reaper_finished.wait(timeout=1) @@ -5090,6 +5096,57 @@ def test_path_on_network_volume_fails_closed_for_detached_volume(monkeypatch): ) is True +def test_path_on_network_volume_preserves_live_local_volume(monkeypatch): + """A live USB/APFS volume keeps the direct local-trash route.""" + import app as app_module + + volume = "/Volumes/CARD" + roots = app_module._NetworkVolumeRoots((), {volume}) + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: volume, + ) + + assert app_module._path_on_network_volume( + f"{volume}/bird.NEF", roots, + ) is False + + +def test_trash_paths_preserves_live_local_volume_fast_path(monkeypatch, tmp_path): + """Healthy local volumes still use the direct mounted-volume Trash path.""" + import app as app_module + + volume = tmp_path / "CARD" + volume.mkdir() + photo = volume / "bird.NEF" + photo.write_bytes(b"raw") + roots = app_module._NetworkVolumeRoots((), {str(volume)}) + direct_calls = [] + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr(app_module, "_network_volume_roots", lambda: roots) + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: str(volume), + ) + monkeypatch.setattr( + app_module, "_move_to_volume_trash", + lambda path: direct_calls.append(path) or True, + ) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda _paths: (_ for _ in ()).throw( + AssertionError("live local volume reached Finder"), + ), + ) + + moved, successful, failures = app_module._trash_paths([str(photo)]) + + assert moved == 1 + assert successful == {str(photo)} + assert failures == [] + assert direct_calls == [str(photo)] + + def test_path_on_network_volume_fails_closed_when_discovery_fails(monkeypatch): """A failed mount query protects custom macOS mount locations too.""" import app as app_module From 3afeaa5c1820d29a5461ae1fc85af5812aca67c0 Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 03:40:38 +0200 Subject: [PATCH 4/8] Bound stuck network reachability probes --- vireo/app.py | 55 +++++++++++++++++--- vireo/tests/test_app.py | 112 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 6 deletions(-) diff --git a/vireo/app.py b/vireo/app.py index d982abfe3..5bd69590b 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -1684,6 +1684,10 @@ def _file_manager_labels(): _FINDER_TRASH_TIMEOUT_SECS = 30 _FINDER_TRASH_BATCH_SIZE = 20 _MOUNT_QUERY_TIMEOUT_SECS = 5 +_MAX_NETWORK_PROBES = 8 +_NETWORK_PROBE_RESERVED = object() +_NETWORK_PROBE_LOCK = threading.Lock() +_NETWORK_PROBES = {} # Distinct from ``None`` so ``_trash_paths`` can tell "caller didn't pass # network_roots" (re-query is safe) apart from "caller's own mount query @@ -1764,15 +1768,35 @@ def _network_volume_roots(run=subprocess.run): ) -def _reap_abandoned_network_probe(process): +def _reserve_network_probe(root): + """Reserve a bounded probe slot, reusing one already active per root.""" + with _NETWORK_PROBE_LOCK: + if root in _NETWORK_PROBES: + return False + if len(_NETWORK_PROBES) >= _MAX_NETWORK_PROBES: + return False + _NETWORK_PROBES[root] = _NETWORK_PROBE_RESERVED + return True + + +def _release_network_probe(root, owner): + """Release ``root`` only when it is still owned by this probe.""" + with _NETWORK_PROBE_LOCK: + if _NETWORK_PROBES.get(root) is owner: + _NETWORK_PROBES.pop(root, None) + + +def _reap_abandoned_network_probe(root, process): """Reap a timed-out probe away from the request path.""" try: process.communicate() except (OSError, ValueError, subprocess.SubprocessError): pass + finally: + _release_network_probe(root, process) -def _abandon_network_probe(process): +def _abandon_network_probe(root, process): """Kill a timed-out probe without synchronously waiting for it.""" try: process.kill() @@ -1780,7 +1804,7 @@ def _abandon_network_probe(process): pass threading.Thread( target=_reap_abandoned_network_probe, - args=(process,), + args=(root, process), name="vireo-network-probe-reaper", daemon=True, ).start() @@ -1800,7 +1824,9 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, subprocess is an *independent* signal: it does not touch Finder. On timeout the child is killed and reaped on a daemon thread so an uninterruptible filesystem call cannot hold the request thread in - Python's usual synchronous kill-and-wait timeout cleanup. + Python's usual synchronous kill-and-wait timeout cleanup. Active probes + are reused per root and globally capped so retries cannot accumulate an + unbounded number of stuck children and reaper threads. Returns ``True`` only when ``stat`` completed in time and reported the root as a directory. Any other outcome (timeout, non-zero exit, error) @@ -1821,7 +1847,15 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, result.returncode == 0 and (result.stdout or "").strip() == "Directory" ) + try: + root_key = os.path.normcase(os.path.normpath(os.fspath(root))) + except (TypeError, ValueError): + return False + if not _reserve_network_probe(root_key): + return False + process = None + abandoned = False try: process = popen( argv, @@ -1830,14 +1864,23 @@ def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, text=True, **no_window_kwargs(), ) + with _NETWORK_PROBE_LOCK: + _NETWORK_PROBES[root_key] = process stdout, _stderr = process.communicate(timeout=timeout) except subprocess.TimeoutExpired: - _abandon_network_probe(process) + abandoned = True + _abandon_network_probe(root_key, process) return False except (OSError, subprocess.SubprocessError): if process is not None: - _abandon_network_probe(process) + abandoned = True + _abandon_network_probe(root_key, process) + else: + _release_network_probe(root_key, _NETWORK_PROBE_RESERVED) return False + finally: + if process is not None and not abandoned: + _release_network_probe(root_key, process) return process.returncode == 0 and (stdout or "").strip() == "Directory" diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index c8703874a..d34426a99 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -5060,6 +5060,118 @@ def kill(self): assert reaper_finished.wait(timeout=1) +def test_network_root_reachable_reuses_abandoned_probe(monkeypatch): + """Retries for a wedged root must not spawn more children or reapers.""" + import subprocess + import threading + import time + + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + release_reaper = threading.Event() + popen_calls = [] + + class WedgedProcess: + returncode = None + + def communicate(self, timeout=None): + if timeout is not None: + raise subprocess.TimeoutExpired(["stat"], timeout) + release_reaper.wait(timeout=2) + return "", "" + + def kill(self): + return None + + class HealthyProcess: + returncode = 0 + + def communicate(self, timeout=None): + return "Directory\n", "" + + def fake_popen(*args, **kwargs): + popen_calls.append(args) + if len(popen_calls) == 1: + return WedgedProcess() + return HealthyProcess() + + root = "/Volumes/NAS" + try: + assert app_module._network_root_reachable(root, popen=fake_popen) is False + assert app_module._network_root_reachable(root, popen=fake_popen) is False + assert len(popen_calls) == 1 + finally: + release_reaper.set() + + deadline = time.monotonic() + 1 + while time.monotonic() < deadline: + with app_module._NETWORK_PROBE_LOCK: + if root not in app_module._NETWORK_PROBES: + break + time.sleep(0.01) + + assert app_module._network_root_reachable(root, popen=fake_popen) is True + assert len(popen_calls) == 2 + + +def test_network_root_reachable_caps_abandoned_probes(monkeypatch): + """Distinct wedged roots cannot grow the process/thread count forever.""" + import subprocess + import threading + import time + + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr(app_module, "_MAX_NETWORK_PROBES", 2) + release_reapers = threading.Event() + popen_calls = [] + + class WedgedProcess: + returncode = None + + def communicate(self, timeout=None): + if timeout is not None: + raise subprocess.TimeoutExpired(["stat"], timeout) + release_reapers.wait(timeout=2) + return "", "" + + def kill(self): + return None + + def fake_popen(*args, **kwargs): + popen_calls.append(args) + return WedgedProcess() + + roots = ["/Volumes/NAS-1", "/Volumes/NAS-2"] + try: + for root in roots: + assert ( + app_module._network_root_reachable(root, popen=fake_popen) + is False + ) + assert ( + app_module._network_root_reachable( + "/Volumes/NAS-3", popen=fake_popen, + ) + is False + ) + assert len(popen_calls) == 2 + finally: + release_reapers.set() + + deadline = time.monotonic() + 1 + while time.monotonic() < deadline: + with app_module._NETWORK_PROBE_LOCK: + if not any(root in app_module._NETWORK_PROBES for root in roots): + break + time.sleep(0.01) + + with app_module._NETWORK_PROBE_LOCK: + assert not any(root in app_module._NETWORK_PROBES for root in roots) + + def test_network_root_reachable_returns_false_off_mac(monkeypatch): """The probe only runs on macOS. On other platforms callers already do not route through Finder, so the probe never needs to answer From 634e529f5819ca07af99b446170257a59366cd88 Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 04:09:56 +0200 Subject: [PATCH 5/8] Make probe cleanup tests path-portable --- vireo/tests/test_app.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index d34426a99..7cb050162 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -5097,6 +5097,7 @@ def fake_popen(*args, **kwargs): return HealthyProcess() root = "/Volumes/NAS" + root_key = app_module.os.path.normcase(app_module.os.path.normpath(root)) try: assert app_module._network_root_reachable(root, popen=fake_popen) is False assert app_module._network_root_reachable(root, popen=fake_popen) is False @@ -5107,7 +5108,7 @@ def fake_popen(*args, **kwargs): deadline = time.monotonic() + 1 while time.monotonic() < deadline: with app_module._NETWORK_PROBE_LOCK: - if root not in app_module._NETWORK_PROBES: + if root_key not in app_module._NETWORK_PROBES: break time.sleep(0.01) @@ -5145,6 +5146,10 @@ def fake_popen(*args, **kwargs): return WedgedProcess() roots = ["/Volumes/NAS-1", "/Volumes/NAS-2"] + root_keys = [ + app_module.os.path.normcase(app_module.os.path.normpath(root)) + for root in roots + ] try: for root in roots: assert ( @@ -5164,12 +5169,12 @@ def fake_popen(*args, **kwargs): deadline = time.monotonic() + 1 while time.monotonic() < deadline: with app_module._NETWORK_PROBE_LOCK: - if not any(root in app_module._NETWORK_PROBES for root in roots): + if not any(key in app_module._NETWORK_PROBES for key in root_keys): break time.sleep(0.01) with app_module._NETWORK_PROBE_LOCK: - assert not any(root in app_module._NETWORK_PROBES for root in roots) + assert not any(key in app_module._NETWORK_PROBES for key in root_keys) def test_network_root_reachable_returns_false_off_mac(monkeypatch): From c334ec84b42710d4fbade4ee0681811f873cb5ae Mon Sep 17 00:00:00 2001 From: Julius Simonelli Date: Mon, 10 Aug 2026 04:28:48 +0200 Subject: [PATCH 6/8] Fail closed on unknown macOS mount types --- vireo/app.py | 24 +++++++++----------- vireo/remote_setup.py | 38 +++++++++++++++++++++++++++----- vireo/tests/test_app.py | 5 +++-- vireo/tests/test_remote_setup.py | 23 +++++++++++++++++++ 4 files changed, 69 insertions(+), 21 deletions(-) diff --git a/vireo/app.py b/vireo/app.py index 5bd69590b..b9da2bc25 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -1707,17 +1707,11 @@ def __init__(self, network_roots=(), mounted_volume_roots=()): self.mounted_volume_roots = frozenset(mounted_volume_roots) -def _mounted_volume_roots_from_mount_output(text): - """Return live top-level ``/Volumes/`` mount points.""" +def _mounted_volume_roots(mounts): + """Return live top-level ``/Volumes/`` roots from parsed mounts.""" roots = set() - for line in text.splitlines(): - before_options, separator, _options = line.strip().rpartition(" (") - if not separator: - continue - _source, separator, mount_point = before_options.partition(" on ") - if not separator: - continue - normalized = posixpath.normpath(mount_point) + for mount in mounts: + normalized = posixpath.normpath(mount["mount_point"]) parts = normalized.split("/") if len(parts) == 3 and parts[1] == "Volumes" and parts[2]: roots.add(normalized) @@ -1757,14 +1751,18 @@ def _network_volume_roots(run=subprocess.run): if result.returncode != 0: return None output = result.stdout or "" + mounts = remote_setup.parse_mount_table(output) return _NetworkVolumeRoots( # ``mount`` always reports macOS/POSIX paths. Keep parsing independent # of the host running the test suite (notably Windows' ``ntpath``). ( - posixpath.normpath(row["mount_point"]) - for row in remote_setup.parse_mount_output(output) + posixpath.normpath(mount["mount_point"]) + for mount in mounts + if remote_setup.mount_type_is_network_or_unknown( + mount["fs_type"], + ) ), - _mounted_volume_roots_from_mount_output(output), + _mounted_volume_roots(mounts), ) diff --git a/vireo/remote_setup.py b/vireo/remote_setup.py index e61423894..62b6a1665 100644 --- a/vireo/remote_setup.py +++ b/vireo/remote_setup.py @@ -36,6 +36,10 @@ ) _NETWORK_FS = ("smbfs", "nfs", "afpfs", "webdav") +_LOCAL_FS = frozenset({ + "apfs", "autofs", "cd9660", "devfs", "exfat", "fdesc", "hfs", + "msdos", "nullfs", "procfs", "tmpfs", "udf", "union", +}) def platform_supported(): @@ -51,17 +55,39 @@ def platform_supported(): return sys.platform == "darwin" -def parse_mount_output(text): - """Parse ``mount`` output into network-share rows the wizard can offer.""" +def parse_mount_table(text): + """Parse macOS ``mount`` output without classifying its filesystems.""" rows = [] for line in text.splitlines(): - m = _MOUNT_RE.match(line.strip()) - if not m: + match = _MOUNT_RE.match(line.strip()) + if not match: continue - fs_type = (m.group("opts").split(",")[0] or "").strip() + rows.append({ + "source": match.group("src"), + "mount_point": match.group("mp"), + "fs_type": (match.group("opts").split(",")[0] or "").strip(), + }) + return rows + + +def mount_type_is_network_or_unknown(fs_type): + """Fail closed for mount types that are not explicitly known-local. + + New network filesystem implementations must not silently gain local-file + semantics in safety-sensitive callers such as Trash routing. Unknown local + filesystems may take a slower Finder path until added to ``_LOCAL_FS``. + """ + return (fs_type or "").strip().lower() not in _LOCAL_FS + + +def parse_mount_output(text): + """Parse ``mount`` output into network-share rows the wizard can offer.""" + rows = [] + for mount in parse_mount_table(text): + fs_type = mount["fs_type"] if fs_type not in _NETWORK_FS: continue - src, mount_point = m.group("src"), m.group("mp") + src, mount_point = mount["source"], mount["mount_point"] if fs_type == "nfs": n = _NFS_SRC_RE.match(src) if not n: diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index 7cb050162..b9cdc2df5 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -4013,18 +4013,19 @@ def fake_run(argv, **kwargs): "//user@nas/Photography on /Volumes/Photography " "(smbfs, nodev, nosuid)\n" "/dev/disk4s1 on /Volumes/CARD (exfat, local)\n" + "nas:/archive on /Volumes/Archive (sshfs, nodev)\n" ), ) monkeypatch.setattr(app_module.sys, "platform", "darwin") assert app_module._network_volume_roots(run=fake_run) == { - "/Volumes/Photography", + "/Volumes/Photography", "/Volumes/Archive", } assert app_module._network_volume_roots( run=fake_run, ).mounted_volume_roots == { - "/Volumes/Photography", "/Volumes/CARD", + "/Volumes/Photography", "/Volumes/CARD", "/Volumes/Archive", } assert calls[0][0] == ["mount"] assert calls[0][1]["timeout"] == app_module._MOUNT_QUERY_TIMEOUT_SECS diff --git a/vireo/tests/test_remote_setup.py b/vireo/tests/test_remote_setup.py index bafe96f62..ed318c729 100644 --- a/vireo/tests/test_remote_setup.py +++ b/vireo/tests/test_remote_setup.py @@ -56,6 +56,29 @@ def test_parse_ignores_non_network_and_garbage(): assert remote_setup.parse_mount_output(LOCAL + "\nnot a mount line\n") == [] +def test_parse_mount_table_preserves_unclassified_and_complex_mount_points(): + unusual = ( + "server:/archive on /Volumes/Photo on Film (Archive) " + "(sshfs, nodev)" + ) + + assert remote_setup.parse_mount_table(LOCAL + "\n" + unusual) == [ + {"source": "/dev/disk3s1s1", "mount_point": "/", "fs_type": "apfs"}, + { + "source": "server:/archive", + "mount_point": "/Volumes/Photo on Film (Archive)", + "fs_type": "sshfs", + }, + ] + + +def test_unknown_mount_types_fail_closed_as_possibly_network_backed(): + assert remote_setup.mount_type_is_network_or_unknown("apfs") is False + assert remote_setup.mount_type_is_network_or_unknown("exfat") is False + assert remote_setup.mount_type_is_network_or_unknown("smbfs") is True + assert remote_setup.mount_type_is_network_or_unknown("sshfs") is True + + def test_parse_afpfs_and_ipv6_hosts(): afp = "//julius@mynas._afpovertcp._tcp.local/Media on /Volumes/Media (afpfs, nodev, nosuid, mounted by julius)" v6 = "//admin@[fe80::1%25en0]/Backup on /Volumes/Backup (smbfs, nodev, nosuid, mounted by julius)" From b5e26ebb9efa0037cc3675da636aa852717de450 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:42:39 +0000 Subject: [PATCH 7/8] Fail closed on autofs mounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit autofs entries are automount triggers, not local storage: the target they resolve to can be an unavailable network share, so any in-process `stat`/`isfile` call under an autofs mount trips the automount and can block indefinitely — the exact failure mode this PR's bounded probe path is designed to avoid. Move `autofs` out of `_LOCAL_FS` so `mount_type_is_network_or_unknown` returns True for it and callers route through the bounded, out-of-process reachability probe instead of a synchronous kernel call. Addresses Codex P1 review on PR #1448 (remote_setup.py:40). --- vireo/remote_setup.py | 8 +++++++- vireo/tests/test_remote_setup.py | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/vireo/remote_setup.py b/vireo/remote_setup.py index 62b6a1665..35b990503 100644 --- a/vireo/remote_setup.py +++ b/vireo/remote_setup.py @@ -36,8 +36,14 @@ ) _NETWORK_FS = ("smbfs", "nfs", "afpfs", "webdav") +# autofs is deliberately excluded: an autofs entry is an automount trigger, +# not local storage, and the target it resolves to can be an unavailable +# network share. Treating it as local would let ``_path_on_network_volume`` +# route callers through in-process ``stat``/``isfile`` calls that trip the +# automount and block indefinitely — the exact failure mode the fail-closed +# probe path is designed to avoid. _LOCAL_FS = frozenset({ - "apfs", "autofs", "cd9660", "devfs", "exfat", "fdesc", "hfs", + "apfs", "cd9660", "devfs", "exfat", "fdesc", "hfs", "msdos", "nullfs", "procfs", "tmpfs", "udf", "union", }) diff --git a/vireo/tests/test_remote_setup.py b/vireo/tests/test_remote_setup.py index ed318c729..f700712bc 100644 --- a/vireo/tests/test_remote_setup.py +++ b/vireo/tests/test_remote_setup.py @@ -77,6 +77,11 @@ def test_unknown_mount_types_fail_closed_as_possibly_network_backed(): assert remote_setup.mount_type_is_network_or_unknown("exfat") is False assert remote_setup.mount_type_is_network_or_unknown("smbfs") is True assert remote_setup.mount_type_is_network_or_unknown("sshfs") is True + # autofs is an automount trigger, not local storage: its target can be an + # unavailable network share, so callers must route through the bounded + # probe path rather than issuing in-process stat/isfile that would trip + # the automount and block. + assert remote_setup.mount_type_is_network_or_unknown("autofs") is True def test_parse_afpfs_and_ipv6_hosts(): From 9d1e8e4004fc17228a142c6a90c01f0fae4b0f40 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:58:54 +0000 Subject: [PATCH 8/8] Parse multiword autofs sources in the mount table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged that _MOUNT_RE's \S+ source rejected the standard macOS auto_home entry ("map auto_home on /System/Volumes/Data/home (autofs, ...)"). parse_mount_table silently dropped those lines, so the follow-up _LOCAL_FS check excluding autofs never ran for them and paths beneath the automount fell through to the local-classification branch — the same in-process stat/isfile blocking path the fail-closed probe was introduced to avoid. Allow multi-word sources by anchoring the split at the first " on /" boundary: the mount point is always an absolute path, so requiring it to start with / keeps the source/mount-point split unambiguous without adding fs_type-specific parsing. Existing coverage for the space-in- mount-point case (server:/archive on /Volumes/Photo on Film (Archive)) still passes because the greedy mount_point group only backs off far enough to satisfy the trailing options group. Adds a regression test asserting that both auto_home and -fstab autofs entries survive parsing and classify as network-or-unknown so callers route them through the bounded probe path. --- vireo/remote_setup.py | 15 ++++++++---- vireo/tests/test_remote_setup.py | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/vireo/remote_setup.py b/vireo/remote_setup.py index 35b990503..e2b0c446a 100644 --- a/vireo/remote_setup.py +++ b/vireo/remote_setup.py @@ -21,10 +21,17 @@ import urllib.parse # `mount` line: " on (, opt, ...)". -# The source is space-free for the network filesystems we accept (smbfs/afp -# URL-encode spaces; nfs is host:/path), while the mount point may contain -# spaces — so split at the FIRST " on " (non-greedy source). -_MOUNT_RE = re.compile(r"^(?P\S+?) on (?P.+) \((?P[^()]*)\)$") +# Sources for the network filesystems we accept happen to be space-free +# (smbfs/afp URL-encode spaces; nfs is host:/path), but macOS also emits +# multi-word sources for automount triggers such as ``map auto_home on +# /System/Volumes/Data/home (autofs, ...)``. Restricting the source to +# ``\S+`` silently dropped those lines and let paths beneath the automount +# be classified as local — the exact failure mode the fail-closed probe +# path is designed to avoid. Anchor the split by requiring the mount point +# to start with ``/`` instead: the non-greedy source stops at the FIRST +# ``" on /"`` boundary, which unambiguously separates ```` from the +# absolute mount path. +_MOUNT_RE = re.compile(r"^(?P.+?) on (?P/.*) \((?P[^()]*)\)$") # smbfs/afp source: //[user@]host/share (URL-encoded) _SMB_SRC_RE = re.compile(r"^//(?:(?P[^@/]+)@)?(?P[^/]+)/(?P.+)$") # nfs source: host:/export/path — host may be a hostname, IPv4, or a diff --git a/vireo/tests/test_remote_setup.py b/vireo/tests/test_remote_setup.py index f700712bc..4d18f0be2 100644 --- a/vireo/tests/test_remote_setup.py +++ b/vireo/tests/test_remote_setup.py @@ -72,6 +72,46 @@ def test_parse_mount_table_preserves_unclassified_and_complex_mount_points(): ] +def test_parse_mount_table_keeps_multiword_autofs_sources(): + # macOS emits automount entries like ``map auto_home on + # /System/Volumes/Data/home (autofs, ...)`` where the source itself + # contains a space. Dropping those rows would let paths beneath the + # trigger fall through to the local-classification branch and reach + # the blocking in-process ``stat``/``isfile`` path when the target is + # an unavailable network share — exactly the failure the fail-closed + # probe is meant to prevent. + auto_home = ( + "map auto_home on /System/Volumes/Data/home " + "(autofs, automounted, nobrowse)" + ) + fstab = ( + "map -fstab on /System/Volumes/Data/Network/Servers " + "(autofs, automounted, nobrowse)" + ) + + rows = remote_setup.parse_mount_table(auto_home + "\n" + fstab) + + assert rows == [ + { + "source": "map auto_home", + "mount_point": "/System/Volumes/Data/home", + "fs_type": "autofs", + }, + { + "source": "map -fstab", + "mount_point": "/System/Volumes/Data/Network/Servers", + "fs_type": "autofs", + }, + ] + # Retained rows must classify as network-or-unknown so callers route + # them through the bounded probe path rather than the in-process + # ``stat``/``isfile`` that trips the automount. + assert all( + remote_setup.mount_type_is_network_or_unknown(row["fs_type"]) + for row in rows + ) + + def test_unknown_mount_types_fail_closed_as_possibly_network_backed(): assert remote_setup.mount_type_is_network_or_unknown("apfs") is False assert remote_setup.mount_type_is_network_or_unknown("exfat") is False