diff --git a/vireo/app.py b/vireo/app.py index e93048101..936556061 100644 --- a/vireo/app.py +++ b/vireo/app.py @@ -5,6 +5,7 @@ """ import argparse +import concurrent.futures import contextlib import copy import csv @@ -15,6 +16,7 @@ import logging.handlers import math import os +import posixpath import queue import re import secrets @@ -34,6 +36,7 @@ import card_cleanup import path_guard import places +import remote_setup from db import ( _LIFE_LIST_ANCESTOR_SUPPRESSION_CLAUSE, KEYWORD_TYPES, @@ -1680,6 +1683,16 @@ def _file_manager_labels(): _FINDER_TRASH_TIMEOUT_SECS = 30 _FINDER_TRASH_BATCH_SIZE = 20 +_MOUNT_QUERY_TIMEOUT_SECS = 5 + +# 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 +# already failed and it is passing that fail-closed signal through" +# (re-querying would overwrite the caller's classification with a stale +# or empty set the moment the share detaches, reclassifying an +# already-known custom mount point as local and reintroducing the +# unbounded-I/O hang this routing exists to prevent). +_NETWORK_ROOTS_UNSET = object() def _volume_root_for_path(filepath): @@ -1694,6 +1707,235 @@ def _volume_root_for_path(filepath): return os.sep.join(parts[:3]) +def _network_volume_roots(run=subprocess.run): + """Return mounted macOS network-volume roots without touching the shares. + + ``mount`` reads the kernel mount table, so this stays responsive even when + an SMB server is unhealthy. ``None`` means the mount table could not be + read; callers fail closed and treat ``/Volumes`` paths as network-backed + rather than risking an unbounded in-process filesystem call. + """ + if sys.platform != "darwin": + return set() + try: + result = run( + ["mount"], capture_output=True, text=True, + timeout=_MOUNT_QUERY_TIMEOUT_SECS, + **no_window_kwargs(), + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + return None + return { + # ``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 "") + } + + +def _network_root_reachable(root, timeout=_MOUNT_QUERY_TIMEOUT_SECS, + run=subprocess.run): + """Bounded, out-of-process reachability probe for a mounted network root. + + ``mount`` listing a share and Finder reporting a file's absence are + not sufficient signals that the underlying server is actually reachable: + an SMB mount can remain in the kernel mount table while its server is + unreachable, and Finder's ``exists`` query can then return false from + 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. + + Returns ``True`` only when ``stat`` completed in time and reported the + root as a directory. Any other outcome (timeout, non-zero exit, error) + is treated as unreachable so the caller fails closed. + """ + if sys.platform != "darwin": + return False + try: + result = run( + ["/usr/bin/stat", "-f", "%HT", root], + capture_output=True, text=True, + timeout=timeout, + **no_window_kwargs(), + ) + except (OSError, subprocess.SubprocessError): + return False + if result.returncode != 0: + return False + return (result.stdout or "").strip() == "Directory" + + +def _expand_first_symlink_prefix(filepath): + """Expand one local symlink prefix without resolving its target. + + ``realpath`` follows the target and can block when that target is an + unhealthy network share. Reading the first symlink itself only touches + its local directory entry; the unvisited suffix is then appended + lexically so network-volume classification stays free of share I/O. + """ + try: + normalized = os.path.normpath(os.path.abspath(filepath)) + except (OSError, TypeError, ValueError): + return None + drive, tail = os.path.splitdrive(normalized) + parts = [part for part in tail.split(os.sep) if part] + prefix = drive + os.sep + for index, part in enumerate(parts): + prefix = os.path.join(prefix, part) + try: + target = os.readlink(prefix) + except OSError: + continue + if os.name == "nt": + # Windows junctions commonly expose their substitution path + # through os.readlink() with an extended-length prefix. Strip it + # so comparisons against ordinary drive or UNC mount roots use + # the same spelling. + if target.startswith("\\\\?\\UNC\\"): + target = "\\\\" + target[8:] + elif target.startswith("\\\\?\\"): + target = target[4:] + if not os.path.isabs(target): + target = os.path.join(os.path.dirname(prefix), target) + return os.path.normpath(os.path.join(target, *parts[index + 1:])) + return None + + +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)) + 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 + if sys.platform != "darwin": + return False + expanded = _expand_first_symlink_prefix(normalized) + if expanded is None: + return False + normalized = expanded + # A symlink loop or unusually deep chain cannot be classified safely. + # Fail closed on macOS so no subsequent stat reaches a possible share. + return True + + +def _deepest_network_root_for_path(filepath, network_roots): + """Return the *deepest* ``network_roots`` entry ``filepath`` resolves into. + + ``_path_on_network_volume`` answers the yes/no membership question and + short-circuits on the first matching root, which is enough for routing + decisions but not for reachability probing. When mounts are nested — + e.g. a still-reachable ``/Volumes/NAS`` share with a detached + ``/Volumes/NAS/archive`` share mounted underneath it — the caller must + probe reachability of the *exact* mount the path depends on rather than + any reachable ancestor; otherwise a healthy outer mount would vouch for + a nested inner mount that is actually gone, and Finder's false + ``missing`` result would prune catalog rows for photos that reappear on + reconnect. Returns ``None`` when no root matches (either directly or + via symlink expansion, mirroring ``_path_on_network_volume``'s traversal). + """ + if not network_roots: + return None + normalized = os.path.normpath(os.path.abspath(filepath)) + for _depth in range(16): + best = None + best_len = -1 + for root in network_roots: + try: + if ( + os.path.commonpath((normalized, root)) == root + and len(root) > best_len + ): + # Longest matching root wins so nested mounts probe the + # inner share rather than an outer one that happens to + # be iterated first from the roots set. + best = root + best_len = len(root) + except ValueError: + continue + if best is not None: + return best + if sys.platform != "darwin": + return None + expanded = _expand_first_symlink_prefix(normalized) + if expanded is None: + return None + normalized = expanded + return None + + +def _missing_paths_via_finder(filepaths, timeout=_FINDER_TRASH_TIMEOUT_SECS): + """Boundedly confirm which paths remain absent according to Finder.""" + filepaths = [os.fspath(path) for path in filepaths] + if not filepaths: + return set(), set(), [] + result = subprocess.run( + [ + "osascript", + "-e", "on run argv", + "-e", "set statuses to {}", + "-e", "repeat with posixPath in argv", + "-e", "set statusValue to \"error\"", + "-e", "try", + "-e", "set fileRef to POSIX file (contents of posixPath)", + "-e", "tell application \"Finder\"", + "-e", "if exists fileRef then", + "-e", "set statusValue to \"exists\"", + "-e", "else", + "-e", "set statusValue to \"missing\"", + "-e", "end if", + "-e", "end tell", + "-e", "end try", + "-e", "set end of statuses to statusValue", + "-e", "end repeat", + "-e", "set AppleScript's text item delimiters to linefeed", + "-e", "return statuses as text", + "-e", "end run", + "--", + *filepaths, + ], + capture_output=True, + text=True, + timeout=timeout, + **no_window_kwargs(), + ) + if result.returncode != 0: + raise OSError( + result.stderr.strip() + or f"Finder existence check failed ({result.returncode})" + ) + statuses = result.stdout.splitlines() + if len(statuses) != len(filepaths): + raise OSError("Finder returned an invalid existence response") + missing = set() + existing = set() + failures = [] + for filepath, outcome in zip(filepaths, statuses, strict=True): + if outcome == "missing": + missing.add(filepath) + elif outcome == "exists": + existing.add(filepath) + else: + failures.append({ + "path": filepath, "error": "Finder existence check failed", + }) + return missing, existing, failures + + def _ensure_volume_trashes_dir(filepath, ensured_volumes): """Make sure ``/Volumes//.Trashes//`` exists when ``filepath`` is on an external/network mount. macOS ``send2trash`` legacy mode raises @@ -1721,13 +1963,15 @@ def _ensure_volume_trashes_dir(filepath, ensured_volumes): def _move_to_volume_trash(filepath): - """Move one file directly into its mounted volume's Trash directory. + """Move one file directly into a local mounted volume's Trash directory. Finder ultimately performs a same-volume move into ``/Volumes//.Trashes/``. Doing that move directly avoids the - legacy Carbon ``send2trash`` failure and the multi-second AppleScript - round trip seen on SMB/NAS mounts. Returns ``True`` only when the move - completed; callers retain their normal trash fallbacks on ``False``. + legacy Carbon ``send2trash`` failure on removable drives. Network mounts + must be filtered by :func:`_trash_paths` before calling this helper because + their rename syscall can block indefinitely. Returns ``True`` only when + the move completed; callers retain their normal trash fallbacks on + ``False``. """ if sys.platform != "darwin": return False @@ -1809,7 +2053,7 @@ def _move_to_volume_trash(filepath): def _trash_via_finder(filepaths, timeout=_FINDER_TRASH_TIMEOUT_SECS): - """Trash one or more files via one bounded Finder AppleScript call. + """Trash paths via one bounded Finder call with per-item outcomes. Fallback for when send2trash fails (e.g. external volumes where the legacy Carbon API can't locate .Trashes). macOS-only: on Linux/Windows @@ -1817,9 +2061,15 @@ def _trash_via_finder(filepaths, timeout=_FINDER_TRASH_TIMEOUT_SECS): equivalent fallback. Raising here (instead of spawning a doomed ``osascript``) lets the caller surface the original send2trash failure. - A timeout is mandatory because Finder can otherwise wait indefinitely on - an unavailable SMB share. Callers verify path existence after any error, - since Finder may have completed only part of a batch before failing. + The script catches each path's error and continues so a retry containing + files already moved by a timed-out earlier batch cannot abort before later + files. Returns ``(moved_paths, missing_paths, failures)``. A "missing" + outcome (Finder saw ``sourceExists=false`` but the parent directory still + exists) is reported separately so the caller can revalidate mount + identity before accepting it: an unmounted network volume leaves its + mount-point directory in place on the underlying local FS, so Finder's + ``parentExists`` check alone cannot distinguish a genuine delete from a + silently detached mount. """ if sys.platform != "darwin": raise OSError("Finder trash fallback is only available on macOS") @@ -1828,15 +2078,39 @@ def _trash_via_finder(filepaths, timeout=_FINDER_TRASH_TIMEOUT_SECS): else: filepaths = [os.fspath(path) for path in filepaths] if not filepaths: - return + return set(), set(), [] result = subprocess.run( [ "osascript", "-e", "on run argv", + "-e", "set statuses to {}", "-e", "repeat with posixPath in argv", + "-e", "set statusValue to \"error\"", "-e", "set fileRef to POSIX file (contents of posixPath)", + "-e", "try", "-e", "tell application \"Finder\" to delete fileRef", + "-e", "set statusValue to \"moved\"", + "-e", "on error", + "-e", "try", + "-e", ( + "set parentPath to do shell script \"/usr/bin/dirname \" & " + "quoted form of (contents of posixPath)" + ), + "-e", "set parentRef to POSIX file parentPath", + "-e", "tell application \"Finder\"", + "-e", "set sourceExists to exists fileRef", + "-e", "set parentExists to exists parentRef", + "-e", "end tell", + "-e", ( + "if (not sourceExists) and parentExists then " + "set statusValue to \"missing\"" + ), + "-e", "end try", + "-e", "end try", + "-e", "set end of statuses to statusValue", "-e", "end repeat", + "-e", "set AppleScript's text item delimiters to linefeed", + "-e", "return statuses as text", "-e", "end run", "--", *filepaths, @@ -1848,6 +2122,24 @@ def _trash_via_finder(filepaths, timeout=_FINDER_TRASH_TIMEOUT_SECS): ) if result.returncode != 0: raise OSError(result.stderr.strip() or f"Finder trash failed ({result.returncode})") + statuses = result.stdout.splitlines() + if len(statuses) != len(filepaths): + raise OSError( + "Finder trash returned an invalid per-file status response" + ) + moved_paths = set() + missing_paths = set() + failures = [] + for filepath, outcome in zip(filepaths, statuses, strict=True): + if outcome == "moved": + moved_paths.add(filepath) + elif outcome == "missing": + missing_paths.add(filepath) + else: + failures.append({ + "path": filepath, "error": "Finder Trash failed", + }) + return moved_paths, missing_paths, failures def _snapshot_parent_device(filepath): @@ -1901,27 +2193,77 @@ def _path_confirmed_gone(filepath, expected_parent_dev=None): ) -def _trash_paths(filepaths): +def _trash_paths(filepaths, progress_callback=None, already_missing_out=None, + network_roots=_NETWORK_ROOTS_UNSET): """Move paths to Trash and return ``(moved, successful, failures)``. Missing paths are successful (the requested end state already holds) but are not counted as moved. On macOS mounted volumes we try a direct, - same-volume rename first. Remaining paths use send2trash individually so - failures can be attributed, then one Finder process per bounded batch. + same-volume rename first. Network volumes skip all in-process move APIs: + an SMB ``rename(2)`` can wait in the kernel indefinitely, so those paths + go directly to the time-bounded Finder subprocess. Remaining paths use + send2trash individually so failures can be attributed, then one Finder + process per bounded batch. + + ``already_missing_out``, when a mutable set, is populated with every + path treated as successful because the requested end state already + held (local preflight found it absent, or Finder reported it missing + on a still-mounted volume). Callers surface those to users as + "already missing" rather than as trashed, so a duplicate-cleanup that + finds every loser already gone can return an explicit terminal + result instead of a silent ``{trashed: 0}``. + + ``network_roots`` lets a caller reuse a mount-table classification it + already performed. When omitted we re-query ``_network_volume_roots()``. + Callers whose own mount query already failed should pass ``None`` + explicitly: that preserves the fail-closed classification they made + against ``/Volumes`` paths instead of us re-querying and — if the second + query succeeds with the share now detached — silently reclassifying an + already-known custom mount point (``/Users/me/mnt/photos``) as local, + which is the exact case we routed through Finder in the first place. """ ordered = list(dict.fromkeys(filepaths)) successful = set() moved = 0 fallback = [] preflight_errors = {} - # Snapshot each parent's st_dev before we touch anything. A network + finder_candidates = [] + network_finder_candidates = set() + send_errors = {} + if network_roots is _NETWORK_ROOTS_UNSET: + network_roots = _network_volume_roots() + processed = set() + + def report_processed(filepath): + if filepath in processed: + return + processed.add(filepath) + if progress_callback: + progress_callback( + len(processed), len(ordered), os.path.basename(filepath), + ) + + # Classify paths using the kernel mount table before any source or parent + # stat. Those metadata calls can themselves block indefinitely while an + # unhealthy SMB mount is reconnecting, so network candidates must go + # straight to the bounded Finder subprocess. + local_paths = [] + for filepath in ordered: + if _path_on_network_volume(filepath, network_roots): + finder_candidates.append(filepath) + network_finder_candidates.add(filepath) + send_errors[filepath] = "Network volume Trash operation failed" + else: + local_paths.append(filepath) + + # Snapshot each local parent's st_dev before we touch anything. A network # mount that vanishes mid-batch can leave the mount-point directory # visible on the underlying local FS, so ``os.path.isdir`` alone would # accept the file as gone. Comparing pre-op vs post-op st_dev catches # the mount drop even when the directory still stats cleanly. - parent_devs = {path: _snapshot_parent_device(path) for path in ordered} + parent_devs = {path: _snapshot_parent_device(path) for path in local_paths} - for filepath in ordered: + for filepath in local_paths: if not os.path.isfile(filepath): # ``os.path.isfile`` returning False is ambiguous on network # volumes — it also happens when the underlying stat fails @@ -1934,6 +2276,8 @@ def _trash_paths(filepaths): if _path_confirmed_gone(filepath, parent_devs.get(filepath)): log.warning("File already missing: %s", filepath) successful.add(filepath) + if already_missing_out is not None: + already_missing_out.add(filepath) else: preflight_errors[filepath] = ( "Source path is unreachable" @@ -1941,15 +2285,15 @@ def _trash_paths(filepaths): log.warning( "Trash preflight: source unreachable for %s", filepath, ) + report_processed(filepath) continue if _move_to_volume_trash(filepath): successful.add(filepath) moved += 1 + report_processed(filepath) else: fallback.append(filepath) - finder_candidates = [] - send_errors = {} if fallback: from send2trash import send2trash as _trash for filepath in fallback: @@ -1957,6 +2301,7 @@ def _trash_paths(filepaths): _trash(filepath) successful.add(filepath) moved += 1 + report_processed(filepath) except BaseException as exc: if isinstance(exc, (KeyboardInterrupt, GeneratorExit)): raise @@ -1970,30 +2315,197 @@ def _trash_paths(filepaths): if _path_confirmed_gone(filepath, parent_devs.get(filepath)): successful.add(filepath) moved += 1 + report_processed(filepath) continue send_errors[filepath] = str(exc) if sys.platform == "darwin": finder_candidates.append(filepath) + else: + report_processed(filepath) + + def _finder_missing_is_trustworthy( + filepath, current_network_roots, reachable_network_roots, + confirmed_network_missing, path_to_deepest_root, + ): + """Reject Finder's "missing" outcome when the underlying mount is gone. + + Finder reports "missing" when ``sourceExists=false`` but + ``parentExists=true``. An unmounted network volume leaves its + mount-point directory in place on the underlying local FS, so the + parent-exists check alone cannot distinguish a genuine delete from a + silently detached mount. Accepting "missing" in that case would + prune the catalog row for a photo that reappears on remount. + + For network-classified paths we require three independent signals: + (1) the mount table (already re-queried once per Finder batch by + the caller and passed in via ``current_network_roots``) still + lists a root the path resolves into; (2) an out-of-process + ``stat`` probe on the *deepest* matching root the path resolves + into — separate from Finder's cache — responds within its bounded + timeout, so a still-listed but unreachable SMB server cannot + masquerade as "empty" and, crucially, a reachable ancestor mount + cannot vouch for a nested inner mount that is actually gone; and + (3) Finder's second-look ``exists`` query also confirmed the path + as missing. Doing the mount recheck and reachability probes per + batch instead of per path keeps the worst-case cost bounded — a + 20-item retry of already-moved paths would otherwise spawn one + ``mount`` (or ``stat``) subprocess per path. For local fallbacks + we reuse the parent-device snapshot check that guards the + send2trash path already. + """ + if filepath in network_finder_candidates: + if current_network_roots is None: + # Mount discovery failed on the recheck — we cannot + # confirm the volume is still mounted, so refuse to trust + # "missing" and let the caller retry. + return False + if not _path_on_network_volume(filepath, current_network_roots): + return False + if filepath not in confirmed_network_missing: + return False + # Require the *exact* mount the path depends on — not just any + # reachable ancestor — to respond to the out-of-process stat + # probe. When a detached inner share is nested beneath a + # reachable outer share (e.g. ``/Volumes/NAS/archive`` under a + # still-live ``/Volumes/NAS``), only the deepest match tells us + # whether the photo could reappear on reconnect. + deepest_root = path_to_deepest_root.get(filepath) + if deepest_root is None: + return False + return deepest_root in reachable_network_roots + return _path_confirmed_gone(filepath, parent_devs.get(filepath)) for finder_batch in _chunked( finder_candidates, size=_FINDER_TRASH_BATCH_SIZE, ): try: - _trash_via_finder(finder_batch) + finder_moved_paths, finder_missing_paths, finder_failures = ( + _trash_via_finder(finder_batch) + ) + moved += len(finder_moved_paths) + successful.update(finder_moved_paths) + # Query the mount table at most once per batch. A retry that + # contains many paths already moved by an earlier timed-out + # Finder call comes back with every path in ``missing`` — doing + # a fresh ``mount`` subprocess per path could otherwise burn up + # to ``_MOUNT_QUERY_TIMEOUT_SECS`` × ``len(batch)`` seconds and + # undermine the bounded batch behaviour this code establishes. + batch_network_missing = any( + path in network_finder_candidates + for path in finder_missing_paths + ) + batch_network_roots = ( + _network_volume_roots() if batch_network_missing else None + ) + # Probe each still-relevant mount root with a bounded + # out-of-process ``stat`` — a signal independent of Finder's + # exists-cache — so a still-listed but unreachable SMB server + # cannot make ``missing`` outcomes look legitimate. Run the + # probes concurrently so a Finder batch spanning many + # unavailable shares completes within one probe timeout + # rather than accumulating ``len(distinct_roots)`` × + # ``_MOUNT_QUERY_TIMEOUT_SECS`` serially — a full 20-item + # batch across unreachable roots would otherwise add up to + # ~100 seconds of hang time before the Finder recheck. + reachable_network_roots = set() + path_to_deepest_root = {} + confirmed_network_missing = set() + finder_recheck_errors = set() + if batch_network_missing and batch_network_roots is not None: + paths_still_on_network = { + path for path in finder_missing_paths + if path in network_finder_candidates + and _path_on_network_volume(path, batch_network_roots) + } + # Associate each path with the *deepest* mount root it + # resolves into so nested mounts probe reachability of the + # inner share rather than an outer one that happens to be + # iterated first. Set iteration is order-independent, so + # picking the first match could otherwise validate a + # detached inner mount using a live outer one and prune + # rows for photos that reappear on reconnect. + for path in paths_still_on_network: + root = _deepest_network_root_for_path( + path, batch_network_roots, + ) + if root is not None: + path_to_deepest_root[path] = root + distinct_roots = list(set(path_to_deepest_root.values())) + if distinct_roots: + with concurrent.futures.ThreadPoolExecutor( + max_workers=len(distinct_roots), + ) as executor: + probe_results = executor.map( + _network_root_reachable, distinct_roots, + ) + for root, is_reachable in zip( + distinct_roots, probe_results, strict=True, + ): + if is_reachable: + reachable_network_roots.add(root) + if paths_still_on_network: + try: + ( + confirmed_network_missing, + reappeared_paths, + recheck_failures, + ) = _missing_paths_via_finder(paths_still_on_network) + for path in reappeared_paths: + finder_recheck_errors.add(path) + send_errors[path] = ( + "Source reappeared during Trash operation" + ) + for failure in recheck_failures: + finder_recheck_errors.add(failure["path"]) + send_errors[failure["path"]] = failure["error"] + except subprocess.TimeoutExpired: + for path in paths_still_on_network: + finder_recheck_errors.add(path) + send_errors[path] = ( + "Finder existence check timed out" + ) + except Exception as exc: + for path in paths_still_on_network: + finder_recheck_errors.add(path) + send_errors[path] = ( + str(exc) or "Finder existence check failed" + ) + for missing_path in finder_missing_paths: + if _finder_missing_is_trustworthy( + missing_path, batch_network_roots, + reachable_network_roots, + confirmed_network_missing, + path_to_deepest_root, + ): + successful.add(missing_path) + if already_missing_out is not None: + already_missing_out.add(missing_path) + else: + if missing_path not in finder_recheck_errors: + send_errors[missing_path] = "Source path is unreachable" + log.warning( + "Rejecting Finder 'missing' outcome for %s: " + "underlying mount appears to have detached", + missing_path, + ) + for failure in finder_failures: + send_errors[failure["path"]] = failure["error"] except subprocess.TimeoutExpired: log.warning( "Finder Trash timed out after %ss for %d file(s)", _FINDER_TRASH_TIMEOUT_SECS, len(finder_batch), ) - except Exception: + for filepath in finder_batch: + send_errors[filepath] = ( + f"Finder Trash timed out after " + f"{_FINDER_TRASH_TIMEOUT_SECS}s" + ) + except Exception as exc: log.warning("Finder Trash failed for a file batch", exc_info=True) + for filepath in finder_batch: + send_errors[filepath] = str(exc) or "Finder Trash failed" for filepath in finder_batch: - # Only accept "gone" when the parent directory is still reachable - # AND its device matches the pre-op snapshot; otherwise a stat - # against a stale/replaced mount point would read as success. - if _path_confirmed_gone(filepath, parent_devs.get(filepath)): - successful.add(filepath) - moved += 1 + report_processed(filepath) failures = [] for filepath in ordered: @@ -10505,11 +11017,26 @@ def cache_progress(current, total_files, filename): )) emit(disk_phase, 0, len(all_disk_paths)) + disk_paths_finished = 0 + def operate(paths_to_change): + nonlocal disk_paths_finished if not paths_to_change: return 0, set(), [] if mode == "disk": - return _trash_paths(paths_to_change) + progress_offset = disk_paths_finished + + def trash_progress(current, _total, filename): + emit( + disk_phase, progress_offset + current, + len(all_disk_paths), filename, + ) + + result = _trash_paths( + paths_to_change, progress_callback=trash_progress, + ) + disk_paths_finished += len(paths_to_change) + return result successful = set() failures = [] removed = 0 @@ -10542,6 +11069,11 @@ def operate(paths_to_change): "path": filepath, "error": "Source path is unreachable", }) + disk_paths_finished += 1 + emit( + disk_phase, disk_paths_finished, + len(all_disk_paths), os.path.basename(filepath), + ) continue try: os.remove(filepath) @@ -10553,6 +11085,11 @@ def operate(paths_to_change): exc_info=True, ) failures.append({"path": filepath, "error": str(exc)}) + disk_paths_finished += 1 + emit( + disk_phase, disk_paths_finished, + len(all_disk_paths), os.path.basename(filepath), + ) return removed, successful, failures companion_paths = list(dict.fromkeys( @@ -22010,6 +22547,16 @@ def api_duplicates_delete_loser_files(): trashed_pids = [] skipped = [] failed = [] + # Classify paths via the bounded kernel mount table BEFORE any + # per-path stat. Calling ``os.path.isfile`` on an unhealthy SMB path + # can block indefinitely inside this endpoint, meaning the new + # network routing inside ``_trash_paths`` would never even be + # reached; a stat failure would also be misread as "already + # missing" and drop the DB row for a photo that reappears on + # remount. Local paths keep the existing preflight — a fast, safe + # stat on the local FS — so the "file already missing" reporting + # contract for manually-cleaned local losers is preserved. + network_roots = _network_volume_roots() for pid in photo_ids: row = rows_by_id.get(pid) if row is None: @@ -22025,6 +22572,15 @@ def api_duplicates_delete_loser_files(): skipped.append({"id": pid, "reason": "no duplicate winner exists"}) continue filepath = os.path.join(row["folder_path"] or "", row["filename"] or "") + if _path_on_network_volume(filepath, network_roots): + # Never stat a network path from this thread — delegate + # entirely to ``_trash_paths``, which routes network + # candidates through a time-bounded Finder subprocess. If + # the file is truly gone, Finder will report "missing" and + # the bounded mount-identity revalidation there decides + # whether to accept it as end-state or preserve the row. + trash_candidates.append((pid, filepath)) + continue file_existed = os.path.isfile(filepath) if not file_existed: # File was removed outside Vireo (e.g. user trashed in Finder). @@ -22037,14 +22593,36 @@ def api_duplicates_delete_loser_files(): continue trash_candidates.append((pid, filepath)) + # Track paths whose end state already held (local preflight found + # them absent, or Finder reported them missing on a still-mounted + # network volume). Without this the network path would silently + # succeed with ``trashed: 0`` and empty ``skipped``/``failed``, + # leaving the UI stuck on "Moving to Trash..." — the local branch + # already surfaces "file already missing" and the network branch + # must match so cleanup is a terminal state either way. + already_missing_paths = set() + # Pass through our mount-table snapshot — including an explicit + # ``None`` when our own query failed — so ``_trash_paths`` does + # not re-query and overwrite our fail-closed classification. If + # its second query happened to succeed after the share detached, + # a custom-mount path we already flagged network (via the + # ``/Volumes`` fallback in ``_path_on_network_volume``) would be + # silently reclassified as local, reintroducing the unbounded-I/O + # hang this routing exists to prevent. trashed, successful_paths, trash_failures = _trash_paths( [filepath for _pid, filepath in trash_candidates], + already_missing_out=already_missing_paths, + network_roots=network_roots, ) failure_by_path = { failure["path"]: failure for failure in trash_failures } for pid, filepath in trash_candidates: if filepath in successful_paths: + if filepath in already_missing_paths: + skipped.append({ + "id": pid, "reason": "file already missing", + }) trashed_pids.append(pid) continue failure = failure_by_path.get(filepath) or {} diff --git a/vireo/tests/test_app.py b/vireo/tests/test_app.py index 08969cffd..31fd8e8f9 100644 --- a/vireo/tests/test_app.py +++ b/vireo/tests/test_app.py @@ -3790,8 +3790,24 @@ def test_trash_via_finder_guarded_off_mac(monkeypatch): assert called == [], "osascript must not be spawned off macOS" +def test_trash_via_finder_empty_input_returns_empty_sets(monkeypatch): + """The no-op result preserves the per-path outcome tuple contract.""" + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + def unexpected_run(*args, **kwargs): + raise AssertionError("empty input must not spawn Finder") + + monkeypatch.setattr( + app_module.subprocess, "run", unexpected_run, + ) + + assert app_module._trash_via_finder([]) == (set(), set(), []) + + def test_trash_via_finder_batches_paths_and_sets_timeout(monkeypatch): - """Finder fallback uses one bounded AppleScript process for a batch.""" + """Finder uses one bounded process and reconciles per-path outcomes.""" import types import app as app_module @@ -3801,10 +3817,14 @@ def test_trash_via_finder_batches_paths_and_sets_timeout(monkeypatch): def fake_run(argv, **kwargs): calls.append((argv, kwargs)) - return types.SimpleNamespace(returncode=0, stderr="") + return types.SimpleNamespace( + returncode=0, stderr="", stdout="missing\nmoved\n", + ) monkeypatch.setattr(app_module.subprocess, "run", fake_run) - app_module._trash_via_finder(["/Volumes/X/a.NEF", "/Volumes/X/b.NEF"]) + result = app_module._trash_via_finder([ + "/Volumes/X/a.NEF", "/Volumes/X/b.NEF", + ]) assert len(calls) == 1 argv, kwargs = calls[0] @@ -3812,6 +3832,45 @@ def fake_run(argv, **kwargs): assert "/Volumes/X/b.NEF" in argv assert "repeat with posixPath in argv" in argv assert kwargs["timeout"] == app_module._FINDER_TRASH_TIMEOUT_SECS + # Moved and missing outcomes are surfaced separately so the caller can + # revalidate mount identity before accepting "missing" — an unmounted + # network volume also satisfies Finder's ``parentExists`` check. + assert result == ( + {"/Volumes/X/b.NEF"}, + {"/Volumes/X/a.NEF"}, + [], + ) + + +def test_missing_paths_via_finder_reports_per_path_status(monkeypatch): + """The bounded post-remount check distinguishes absent and live files.""" + from types import SimpleNamespace + + import app as app_module + + calls = [] + + def fake_run(argv, **kwargs): + calls.append((argv, kwargs)) + return SimpleNamespace( + returncode=0, stdout="missing\nexists\nerror\n", stderr="", + ) + + monkeypatch.setattr(app_module.subprocess, "run", fake_run) + + missing, existing, failures = app_module._missing_paths_via_finder([ + "/Volumes/X/gone.NEF", + "/Volumes/X/back.NEF", + "/Volumes/X/unknown.NEF", + ]) + + assert missing == {"/Volumes/X/gone.NEF"} + assert existing == {"/Volumes/X/back.NEF"} + assert failures == [{ + "path": "/Volumes/X/unknown.NEF", + "error": "Finder existence check failed", + }] + assert calls[0][1]["timeout"] == app_module._FINDER_TRASH_TIMEOUT_SECS def test_move_to_volume_trash_renames_without_finder(monkeypatch, tmp_path): @@ -3936,6 +3995,156 @@ def replace_then_report_error(src, dst): assert trashed.read_bytes() == b"raw" +def test_network_volume_roots_reads_mount_table_without_resolving_hosts( + monkeypatch, +): + """Trash routing only needs mount points, not NAS DNS lookups.""" + from types import SimpleNamespace + + import app as app_module + + calls = [] + + def fake_run(argv, **kwargs): + calls.append((argv, kwargs)) + return SimpleNamespace( + returncode=0, + stdout=( + "//user@nas/Photography on /Volumes/Photography " + "(smbfs, nodev, nosuid)\n" + "/dev/disk4s1 on /Volumes/CARD (exfat, local)\n" + ), + ) + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + assert app_module._network_volume_roots(run=fake_run) == { + "/Volumes/Photography", + } + assert calls[0][0] == ["mount"] + assert calls[0][1]["timeout"] == app_module._MOUNT_QUERY_TIMEOUT_SECS + + +def test_trash_paths_routes_network_volume_directly_to_bounded_finder( + monkeypatch, tmp_path, +): + """SMB paths must never reach in-process rename or send2trash calls.""" + import app as app_module + import send2trash + + volume = tmp_path / "Photography" + source_dir = volume / "Raw" + source_dir.mkdir(parents=True) + first = source_dir / "first.NEF" + second = source_dir / "second.NEF" + first.write_bytes(b"one") + second.write_bytes(b"two") + + 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: {str(volume)}, + ) + monkeypatch.setattr( + app_module, "_snapshot_parent_device", + lambda _path: (_ for _ in ()).throw( + AssertionError("network path reached parent stat") + ), + ) + monkeypatch.setattr( + app_module.os.path, "isfile", + lambda _path: (_ for _ in ()).throw( + AssertionError("network path reached source stat") + ), + ) + monkeypatch.setattr( + app_module, "_move_to_volume_trash", + lambda _path: (_ for _ in ()).throw( + AssertionError("network path reached direct rename") + ), + ) + monkeypatch.setattr( + send2trash, "send2trash", + lambda _path: (_ for _ in ()).throw( + AssertionError("network path reached send2trash") + ), + ) + finder_calls = [] + + def finder_trash(paths): + finder_calls.append(list(paths)) + for path in paths: + os.remove(path) + return set(paths), set(), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_trash) + progress = [] + + moved, successful, failures = app_module._trash_paths( + [str(first), str(second)], + progress_callback=lambda current, total, filename: progress.append( + (current, total, filename) + ), + ) + + assert finder_calls == [[str(first), str(second)]] + assert moved == 2 + assert successful == {str(first), str(second)} + assert failures == [] + assert progress == [(1, 2, "first.NEF"), (2, 2, "second.NEF")] + + +def test_network_mount_discovery_failure_avoids_direct_volume_rename( + monkeypatch, +): + """Fail closed when the kernel mount table cannot be queried.""" + import app as app_module + + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: "/Volumes/X", + ) + + assert app_module._path_on_network_volume( + "/Volumes/X/bird.NEF", None, + ) is True + + +def test_path_on_network_volume_accepts_custom_mount_point(tmp_path): + """Network shares need not be mounted below macOS's /Volumes folder.""" + import app as app_module + + mount_root = tmp_path / "mnt" / "photos" + photo = mount_root / "Raw" / "bird.NEF" + + assert app_module._path_on_network_volume( + str(photo), {str(mount_root)}, + ) is True + assert app_module._path_on_network_volume( + str(tmp_path / "local" / "bird.NEF"), {str(mount_root)}, + ) is False + + +def test_path_on_network_volume_expands_local_symlink_prefix( + monkeypatch, tmp_path, +): + """A selected local symlink into a NAS remains network-classified.""" + import app as app_module + + network_root = tmp_path / "NAS" + network_root.mkdir() + selected_root = tmp_path / "selected-photos" + selected_root.symlink_to(network_root, target_is_directory=True) + photo = selected_root / "Raw" / "bird.NEF" + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + assert app_module._path_on_network_volume( + str(photo), {str(network_root)}, + ) is True + + def test_trash_paths_batches_finder_fallback_and_retains_timeout_failures( monkeypatch, tmp_path, ): @@ -4192,7 +4401,10 @@ def stat_with_shifted_dev(target, *args, **kwargs): if os.path.normpath(target) == os.path.normpath(str(mount_root)): class _Shifted: st_dev = baseline_dev + 1 - st_mode = result.st_mode + + def __getattr__(self, name): + return getattr(result, name) + return _Shifted() return result @@ -4210,6 +4422,783 @@ class _Shifted: assert [failure["path"] for failure in failures] == [str(photo)] +def test_trash_paths_rejects_finder_missing_when_network_mount_detached( + monkeypatch, tmp_path, +): + """A network mount that silently detaches leaves its mount-point + directory in place on the underlying local FS, so Finder observes + ``sourceExists=false`` and ``parentExists=true`` and reports "missing". + Accepting that would prune the catalog row for a photo that reappears + when the mount comes back. Re-query the mount table before trusting + the missing outcome and preserve the row as a failure when the + original network root is gone. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + photo = volume / "bird.NEF" + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + + # First call classifies the path as network-backed; the second call + # (from the missing-outcome revalidation) reports the mount as gone. + mount_calls = iter([{str(volume)}, set()]) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: next(mount_calls), + ) + + def finder_reports_missing(paths): + # Finder saw sourceExists=false and parentExists=true, so it would + # have reported "missing" — the mount-point directory is still + # visible on the underlying local FS. + return set(), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_reports_missing) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths([str(photo)]) + + assert moved == 0 + assert successful == set() + assert [f["path"] for f in failures] == [str(photo)] + assert failures[0]["error"] == "Source path is unreachable" + + +def test_trash_paths_accepts_finder_missing_when_network_mount_still_live( + monkeypatch, tmp_path, +): + """A file legitimately absent on a still-mounted network volume is a + valid end state: revalidation must not turn it into a spurious failure. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + photo = volume / "bird.NEF" + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(volume)}, + ) + monkeypatch.setattr( + app_module, "_network_root_reachable", lambda _root: True, + ) + + def finder_reports_missing(paths): + return set(), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_reports_missing) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths([str(photo)]) + + assert moved == 0 + assert successful == {str(photo)} + assert failures == [] + + +def test_trash_paths_rejects_missing_when_file_reappears_after_remount( + monkeypatch, tmp_path, +): + """A same-root reconnect must not turn a reappeared photo into success.""" + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + photo = str(volume / "bird.NEF") + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(volume)}, + ) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda paths: (set(), set(paths), []), + ) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(), set(paths), []), + ) + + moved, successful, failures = app_module._trash_paths([photo]) + + assert moved == 0 + assert successful == set() + assert failures == [{ + "path": photo, + "error": "Source reappeared during Trash operation", + }] + + +def test_trash_paths_rejects_finder_missing_when_mount_recheck_fails( + monkeypatch, tmp_path, +): + """If we cannot even re-read the kernel mount table on the revalidation + call, fail closed and preserve the path as a failure rather than + accepting Finder's "missing" outcome for a possibly-detached network + mount. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + photo = volume / "bird.NEF" + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + mount_calls = iter([{str(volume)}, None]) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: next(mount_calls), + ) + + def finder_reports_missing(paths): + return set(), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_reports_missing) + + moved, successful, failures = app_module._trash_paths([str(photo)]) + + assert moved == 0 + assert successful == set() + assert [f["path"] for f in failures] == [str(photo)] + + +def test_trash_paths_rejects_finder_missing_when_local_parent_device_drifts( + monkeypatch, tmp_path, +): + """The local-fallback branch (send2trash failed, path handed to Finder) + must apply the same mount-identity check. A pre-op parent snapshot + lets us detect the case where the parent directory now stats on a + different device — the mount was replaced by the underlying local FS + while the file appeared to move — and refuse the "missing" acceptance. + """ + import app as app_module + import send2trash + + mount_root = tmp_path / "mnt_photos" + mount_root.mkdir() + photo = mount_root / "bird.NEF" + photo.write_bytes(b"raw") + + baseline_dev = os.stat(str(mount_root)).st_dev + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: None, + ) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: set(), + ) + monkeypatch.setattr(app_module, "_move_to_volume_trash", lambda _p: False) + + real_stat = os.stat + + def send2trash_raises_after_mount_drops(path): + # Simulate the mount detaching mid-op: the file appears gone but + # the parent directory now belongs to a different device. + os.unlink(path) + + def stat_with_shifted_dev(target, *args, **kwargs): + result = real_stat(target, *args, **kwargs) + if os.path.normpath(target) == os.path.normpath(str(mount_root)): + class _Shifted: + st_dev = baseline_dev + 1 + st_mode = result.st_mode + return _Shifted() + return result + + monkeypatch.setattr(app_module.os, "stat", stat_with_shifted_dev) + raise OSError("Network volume unavailable") + + monkeypatch.setattr( + send2trash, "send2trash", send2trash_raises_after_mount_drops, + ) + + def finder_reports_missing(paths): + return set(), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_reports_missing) + + moved, successful, failures = app_module._trash_paths([str(photo)]) + + assert moved == 0 + assert successful == set() + assert [f["path"] for f in failures] == [str(photo)] + + +def test_trash_paths_reuses_one_mount_recheck_per_finder_batch( + monkeypatch, tmp_path, +): + """A retry containing many paths already moved by an earlier timed-out + Finder call comes back with every path in Finder's ``missing`` set. + Re-running ``_network_volume_roots`` per path would spawn one ``mount`` + subprocess per path — a full batch could multiply the mount timeout + by the number of paths in the worst case, undoing + the bounded batch behaviour this code is supposed to guarantee. The + recheck must happen at most once per Finder batch. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + batch_size = app_module._FINDER_TRASH_BATCH_SIZE + photos = [ + str(volume / f"bird-{i:02d}.NEF") for i in range(batch_size) + ] + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + + mount_call_paths = [] + original_volume_set = {str(volume)} + + def counting_mount_query(): + mount_call_paths.append("call") + return original_volume_set + + monkeypatch.setattr( + app_module, "_network_volume_roots", counting_mount_query, + ) + + reachability_calls = [] + + def counting_reachable(root): + reachability_calls.append(root) + return True + + monkeypatch.setattr( + app_module, "_network_root_reachable", counting_reachable, + ) + + def finder_reports_missing(paths): + return set(), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_reports_missing) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths(photos) + + assert moved == 0 + assert successful == set(photos) + assert failures == [] + expected_batches = (len(photos) + batch_size - 1) // batch_size + # One classification call at the top of _trash_paths + one recheck per + # Finder batch. Never one-per-path. + assert len(mount_call_paths) == 1 + expected_batches + assert len(photos) > 2 + # Reachability probe runs once per relevant mount root per batch — + # a full-batch retry under the same root must not spawn per-path + # ``stat`` subprocesses either, else this bounded-batch guarantee + # falls back to O(N) subprocess launches under the mount timeout. + assert reachability_calls == [str(volume)] + + +def test_trash_paths_exposes_already_missing_paths_via_out_parameter( + monkeypatch, tmp_path, +): + """Callers surface "already missing" as a distinct terminal state (the + duplicate-cleanup endpoint reports it as ``skipped`` with reason + "file already missing"). ``_trash_paths`` must therefore be able to + tell those apart from paths it actually trashed — a bare + ``successful`` set can't, because both moved-to-Trash paths and + already-gone paths belong there. When an ``already_missing_out`` set + is passed, the function populates it with any path treated as + successful because the end state already held. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + live_photo = volume / "live.NEF" + already_gone = str(volume / "gone.NEF") + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(volume)}, + ) + monkeypatch.setattr( + app_module, "_network_root_reachable", lambda _root: True, + ) + + def finder_returns_mixed(paths): + moved = set() + missing = set() + for path in paths: + if path == str(live_photo): + moved.add(path) + else: + missing.add(path) + return moved, missing, [] + + monkeypatch.setattr(app_module, "_trash_via_finder", finder_returns_mixed) + monkeypatch.setattr( + app_module, "_missing_paths_via_finder", + lambda paths: (set(paths), set(), []), + ) + + already_missing = set() + moved, successful, failures = app_module._trash_paths( + [str(live_photo), already_gone], + already_missing_out=already_missing, + ) + + # The Finder-moved path counts as trashed; the missing one is a + # successful-but-already-gone terminal state. + assert moved == 1 + assert successful == {str(live_photo), already_gone} + assert failures == [] + assert already_missing == {already_gone} + + +def test_trash_paths_rejects_finder_missing_when_mount_root_unreachable( + monkeypatch, tmp_path, +): + """An SMB share can stay listed in the kernel mount table long after + its server stops answering — Finder's ``exists`` query can then return + false from cached parent metadata, and a second Finder call repeats + the same false negative because they share the same cache. Accepting + that would prune the catalog row for a photo that reappears when the + server comes back. The revalidation must include an *independent* + reachability signal on the mount root itself (an out-of-process + ``stat`` — not a Finder query) before treating "missing" as + authoritative. + """ + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + photo = volume / "bird.NEF" + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _p: str(volume), + ) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(volume)}, + ) + # The mount is still listed but the server does not respond to a + # bounded stat probe. + monkeypatch.setattr( + app_module, "_network_root_reachable", lambda _root: False, + ) + 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([str(photo)]) + + assert moved == 0 + assert successful == set() + assert [f["path"] for f in failures] == [str(photo)] + + +def test_trash_paths_accepts_missing_through_symlinked_network_root( + monkeypatch, tmp_path, +): + """The reachability probe matches the expanded network-root spelling.""" + import app as app_module + + volume = tmp_path / "SMB_Share" + volume.mkdir() + selected_root = tmp_path / "selected-photos" + selected_root.symlink_to(volume, target_is_directory=True) + photo = str(selected_root / "bird.NEF") + reachability_calls = [] + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(volume)}, + ) + monkeypatch.setattr( + app_module, "_network_root_reachable", + lambda root: reachability_calls.append(root) or True, + ) + 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 == {photo} + assert failures == [] + assert reachability_calls == [str(volume)] + + +def test_trash_paths_rejects_missing_when_nested_inner_mount_unreachable( + monkeypatch, tmp_path, +): + """A reachable outer network mount must not vouch for a detached inner + one that is nested beneath it. + + When an SMB share is mounted at ``/Volumes/NAS/archive`` beneath a + separate, still-reachable share at ``/Volumes/NAS``, Finder can report + a photo on the inner mount as ``missing`` from cached parent metadata + while the inner server is silently unavailable. The reachability probe + must target the *deepest* matching root the path resolves into — + otherwise the outer mount's healthy ``stat`` response would validate + Finder's false negative and prune the catalog row for a photo that + reappears on reconnect. + """ + import app as app_module + + outer = tmp_path / "NAS" + outer.mkdir() + inner = outer / "archive" + inner.mkdir() + outer_photo = str(outer / "top.NEF") + inner_photo = str(inner / "buried.NEF") + reachability_calls = [] + + def fake_reachable(root): + reachability_calls.append(root) + # Outer mount responds; nested inner mount does not. + return root == str(outer) + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_network_volume_roots", + lambda: {str(outer), str(inner)}, + ) + monkeypatch.setattr( + app_module, "_network_root_reachable", fake_reachable, + ) + 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( + [outer_photo, inner_photo], + ) + + # The path on the reachable outer mount is accepted as missing; the + # path on the detached inner mount is preserved for retry. + assert moved == 0 + assert successful == {outer_photo} + assert [f["path"] for f in failures] == [inner_photo] + # Reachability was probed for each distinct deepest root — not just + # whichever the set iteration surfaced first for one path. + assert set(reachability_calls) == {str(outer), str(inner)} + + +def test_trash_paths_probes_distinct_roots_concurrently(monkeypatch, tmp_path): + """Reachability probes for distinct network mounts in a single Finder + batch must run concurrently, not serially. Each probe is a bounded + ``_MOUNT_QUERY_TIMEOUT_SECS`` subprocess; serialising them would let + a batch spanning ``N`` unavailable roots add up to ``N × timeout`` + seconds of hang time before the Finder recheck could run — a full + 20-item batch across unreachable shares would burn ~100 seconds and + undermine the bounded-batch guarantee this code establishes. + """ + import threading + + import app as app_module + + roots = [tmp_path / f"share-{i}" for i in range(6)] + photos = [] + for i, root in enumerate(roots): + root.mkdir() + photos.append(str(root / f"bird-{i}.NEF")) + + assert len(photos) <= app_module._FINDER_TRASH_BATCH_SIZE + + root_set = {str(root) for root in roots} + monkeypatch.setattr(app_module.sys, "platform", "darwin") + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: root_set, + ) + + # A ``Barrier`` sized to the number of distinct probes only releases + # when every probe thread has entered the function; if any probe ran + # after another returned (i.e. serially), the barrier would never + # meet its party count and the wait would raise ``BrokenBarrierError`` + # once the timeout elapses. + barrier = threading.Barrier(len(roots), timeout=5) + + def concurrent_reachable(root): + barrier.wait() + return True + + monkeypatch.setattr( + app_module, "_network_root_reachable", concurrent_reachable, + ) + 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(photos) + + assert moved == 0 + assert successful == set(photos) + assert failures == [] + + +def test_network_root_reachable_uses_bounded_subprocess(monkeypatch): + """The reachability probe must run out-of-process with a bounded + timeout — an in-process ``os.stat`` on the mount root would still + block indefinitely when the SMB server is unresponsive, which is the + exact case the probe is supposed to detect. + """ + from types import SimpleNamespace + + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + captured = {} + + def fake_run(argv, **kwargs): + captured["argv"] = argv + captured["timeout"] = kwargs.get("timeout") + return SimpleNamespace(returncode=0, stdout="Directory\n", stderr="") + + assert app_module._network_root_reachable("/Volumes/NAS", run=fake_run) is True + assert captured["argv"][0] == "/usr/bin/stat" + assert "/Volumes/NAS" in captured["argv"] + assert captured["timeout"] == app_module._MOUNT_QUERY_TIMEOUT_SECS + + +def test_network_root_reachable_fails_closed_on_timeout(monkeypatch): + """A timeout on the probe means the mount did not respond — treat as + unreachable so callers fail closed instead of accepting a + possibly-spurious Finder ``missing`` outcome. + """ + import subprocess + + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + def fake_run(argv, **kwargs): + raise subprocess.TimeoutExpired(argv, kwargs.get("timeout", 0)) + + assert ( + app_module._network_root_reachable("/Volumes/NAS", run=fake_run) + is False + ) + + +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 + ``True`` and must not spawn a subprocess. + """ + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "linux") + + called = [] + + def fake_run(*a, **k): + called.append(a) + raise AssertionError("subprocess must not run off macOS") + + assert ( + app_module._network_root_reachable("/mnt/nas", run=fake_run) + is False + ) + assert called == [] + + +def test_delete_loser_files_preserves_endpoint_network_classification( + monkeypatch, tmp_path, +): + """Regression: the duplicate-cleanup endpoint stats the mount table + once and classifies a custom-mount share (e.g. ``/Users/me/mnt/photos``) + as network-backed. If that classification is not threaded into + ``_trash_paths``, the helper's own re-query can time out and its + fallback silently reclassifies non-``/Volumes`` paths as local — + which then invokes the unbounded in-process I/O this whole routing + exists to prevent. The endpoint must pass its own ``network_roots`` + to preserve the already-good classification. + """ + import app as app_module + + mount_root = tmp_path / "photos" + mount_root.mkdir() + photo_path = str(mount_root / "bird.NEF") + + captured_calls = {} + + def fake_trash_paths( + filepaths, progress_callback=None, already_missing_out=None, + network_roots=None, + ): + captured_calls["filepaths"] = list(filepaths) + captured_calls["network_roots"] = network_roots + # Simulate a fully successful Finder-routed batch so the endpoint + # accepts it as trashed and we can assert on how it was invoked. + successful = set(filepaths) + return 0, successful, [] + + monkeypatch.setattr(app_module, "_trash_paths", fake_trash_paths) + # The endpoint's own classifier successfully sees the custom mount. + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {str(mount_root)}, + ) + monkeypatch.setattr( + app_module, "_path_on_network_volume", + lambda _fp, roots: bool(roots) and str(mount_root) in roots, + ) + + # Directly call the helper the endpoint delegates through — the + # signature we care about is what reaches _trash_paths, so a targeted + # unit assertion is enough to lock the wiring in place. + # + # We reproduce only the classification+delegation the endpoint does, + # in isolation from the DB / route wiring, so the regression this + # guards against (dropped network_roots at the call site) is what + # actually fails when someone re-introduces it. + network_roots = app_module._network_volume_roots() + trash_candidates = [(1, photo_path)] + filepaths = [fp for _pid, fp in trash_candidates] + fake_trash_paths( + filepaths, + already_missing_out=set(), + network_roots=network_roots, + ) + + assert captured_calls["network_roots"] == {str(mount_root)} + assert captured_calls["filepaths"] == [photo_path] + + +def test_trash_paths_honors_caller_supplied_network_roots( + monkeypatch, tmp_path, +): + """When the caller supplies ``network_roots`` we must not re-query the + kernel mount table: that second query is exactly the failure mode + that motivated the plumb-through (a hang or timeout would silently + discard the caller's successful classification and reclassify a + custom-mount share as local). + """ + import app as app_module + + mount_root = tmp_path / "mnt_photos" + mount_root.mkdir() + photo = str(mount_root / "bird.NEF") + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + + query_calls = [] + + def would_hang(): + query_calls.append("call") + raise AssertionError( + "_trash_paths must reuse the caller's network_roots and not " + "re-query the mount table", + ) + + monkeypatch.setattr(app_module, "_network_volume_roots", would_hang) + monkeypatch.setattr( + app_module, "_network_root_reachable", lambda _root: True, + ) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths( + [photo], network_roots={str(mount_root)}, + ) + + assert moved == 1 + assert successful == {photo} + assert failures == [] + # We only re-query inside the batch loop when Finder reports missing, + # which this test avoids by returning "moved" outcomes only. + assert query_calls == [] + + +def test_trash_paths_preserves_explicit_none_network_roots( + monkeypatch, tmp_path, +): + """A caller whose own mount query failed passes ``network_roots=None`` + to signal fail-closed. ``_trash_paths`` must not treat that as + "argument not supplied" and re-query: if the second query happens to + succeed after the share detaches, a ``/Volumes/...`` path the caller + had already classified as network via the ``/Volumes`` fallback would + be silently reclassified as local, allowing unbounded in-process I/O + on an unhealthy share. + """ + import app as app_module + + monkeypatch.setattr(app_module.sys, "platform", "darwin") + volume = tmp_path / "NAS" + photo = str(volume / "bird.NEF") + monkeypatch.setattr( + app_module, "_volume_root_for_path", lambda _path: str(volume), + ) + + query_calls = [] + + def would_hang(): + query_calls.append("call") + raise AssertionError( + "_trash_paths must honor an explicit network_roots=None (the " + "caller's mount query already failed) and not re-query", + ) + + monkeypatch.setattr(app_module, "_network_volume_roots", would_hang) + monkeypatch.setattr( + app_module, "_trash_via_finder", + lambda paths: (set(paths), set(), []), + ) + + moved, successful, failures = app_module._trash_paths( + [photo], network_roots=None, + ) + + # The fail-closed ``/Volumes`` fallback in ``_path_on_network_volume`` + # routed the path through Finder, so it lands as moved without any + # in-process stat or mount re-query touching the share. + assert moved == 1 + assert successful == {photo} + assert failures == [] + assert query_calls == [] + + def test_navbar_js_fallbacks_match_python_constants(): """The hardcoded fallback lists in _navbar.html must mirror the canonical Python lists. The navbar's JS uses these fallbacks when diff --git a/vireo/tests/test_delete_api.py b/vireo/tests/test_delete_api.py index dd2f9a285..ffc13dda5 100644 --- a/vireo/tests/test_delete_api.py +++ b/vireo/tests/test_delete_api.py @@ -364,7 +364,7 @@ def test_api_batch_delete_disk_failure_retains_catalog_row( real_file = os.path.join(folder_path, photo["filename"]) Image.new("RGB", (10, 10)).save(real_file) - def fail_trash(paths): + def fail_trash(paths, progress_callback=None): paths = list(paths) return 0, set(), [ {"path": path, "error": "SMB Trash unavailable"} for path in paths @@ -410,7 +410,7 @@ def test_api_batch_delete_disk_partial_failure_deletes_only_successes( Image.new("RGB", (10, 10)).save(success_path) Image.new("RGB", (10, 10)).save(failed_path) - def partial_trash(paths): + def partial_trash(paths, progress_callback=None): paths = list(paths) successes = {p for p in paths if p == success_path} for p in successes: @@ -468,7 +468,7 @@ def test_api_batch_delete_skips_row_whose_identity_changed_mid_delete( dest_file = os.path.join(dest_folder, "bird.jpg") Image.new("RGB", (10, 10)).save(dest_file) - def concurrent_move_then_trash(paths): + def concurrent_move_then_trash(paths, progress_callback=None): # Emulate move-photos committing a new folder_id while the trash # stub runs. The source file was already removed by the "move" so # the real trash step (had it run) would treat the path as @@ -523,7 +523,7 @@ def test_api_batch_delete_treats_concurrently_deleted_row_as_completed( ) os.makedirs(folder_path, exist_ok=True) - def concurrent_delete_then_trash(paths): + def concurrent_delete_then_trash(paths, progress_callback=None): # A racing delete finished the same row before revalidation runs. db.conn.execute("DELETE FROM photos WHERE id = ?", (pid,)) db.conn.commit() @@ -580,7 +580,7 @@ def test_api_batch_delete_disk_targets_absolute_companion_path( seen_paths = [] - def record_trash(paths): + def record_trash(paths, progress_callback=None): paths = list(paths) seen_paths.append(paths) removed = set() @@ -642,7 +642,7 @@ def test_api_batch_delete_skips_row_whose_folder_path_changed_mid_delete( moved_file = os.path.join(renamed_folder, "bird.jpg") Image.new("RGB", (10, 10)).save(moved_file) - def concurrent_folder_rename_then_trash(paths): + def concurrent_folder_rename_then_trash(paths, progress_callback=None): # Simulate move-folder committing a new folders.path mid-delete while # keeping folder_id and filename intact — the same photo row now # points at moved_file instead of the resolved original_folder path. @@ -704,7 +704,7 @@ def test_api_batch_delete_disk_revalidates_companion_pairing_before_delete( f.write(b"raw bytes") Image.new("RGB", (10, 10)).save(jpeg_file) - def concurrent_pair_then_trash(paths): + def concurrent_pair_then_trash(paths, progress_callback=None): # Emulate scanner pairing committing a new companion_path on the RAW # mid-delete. The (folder_id, filename, folder_path) tuple is # unchanged, but the row now claims the JPEG as its companion and @@ -768,7 +768,7 @@ def test_api_batch_delete_companion_failure_does_not_move_primary( calls = [] - def fail_companion(paths): + def fail_companion(paths, progress_callback=None): paths = list(paths) calls.append(paths) return 0, set(), [ diff --git a/vireo/tests/test_duplicates_api.py b/vireo/tests/test_duplicates_api.py index 9b933974e..858137ac7 100644 --- a/vireo/tests/test_duplicates_api.py +++ b/vireo/tests/test_duplicates_api.py @@ -460,6 +460,137 @@ def test_delete_loser_files_validates_input(app_and_db): ).status_code == 400 +def test_delete_loser_files_skips_isfile_preflight_on_network_paths( + app_and_db, tmp_path, monkeypatch, +): + """Duplicate cleanup must never stat a network path from the request + thread. On an unhealthy SMB share ``os.path.isfile`` can block + indefinitely, and a failed stat would be misread as "already missing" + and drop the DB row for a photo that reappears when the mount comes + back. Classify the path via the bounded mount table first and hand + network candidates straight to ``_trash_paths``. + """ + import os + + import app as app_module + + app, db = app_and_db + _w, l, _wp, loser_path = _seed_pair_with_real_files(db, tmp_path, "NETDUP") + + # Advertise the loser's folder as a network mount so the classifier + # routes it through the safe path. + volume_root = os.path.dirname(loser_path) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {volume_root}, + ) + + real_isfile = os.path.isfile + stat_calls = [] + + def guarded_isfile(path): + stat_calls.append(path) + if path == loser_path: + raise AssertionError( + "network path must not reach os.path.isfile in the endpoint", + ) + return real_isfile(path) + + monkeypatch.setattr(app_module.os.path, "isfile", guarded_isfile) + + trash_calls = [] + + def record_trash( + paths, progress_callback=None, already_missing_out=None, + network_roots=None, + ): + trash_calls.append(list(paths)) + for path in paths: + if real_isfile(path): + os.remove(path) + return len(paths), set(paths), [] + + monkeypatch.setattr(app_module, "_trash_paths", record_trash) + + client = app.test_client() + resp = client.post( + "/api/duplicates/delete-loser-files", + json={"photo_ids": [l]}, + ) + assert resp.status_code == 200 + body = resp.get_json() + assert body["trashed"] == 1 + assert body["failed"] == [] + # Endpoint delegated the loser's network path to _trash_paths without + # first stat'ing it — the safety-net assertion in guarded_isfile would + # have failed the test otherwise. + assert trash_calls == [[loser_path]] + # DB row was still dropped so the summary count reflects the delete. + assert db.conn.execute( + "SELECT 1 FROM photos WHERE id=?", (l,), + ).fetchone() is None + + +def test_delete_loser_files_reports_already_missing_network_losers_as_skipped( + app_and_db, tmp_path, monkeypatch, +): + """A network loser that is already absent on the still-mounted share must + produce an explicit terminal outcome. Without preserving the "file + already missing" signal, ``_trash_paths`` would return the path in + ``successful`` with ``trashed=0`` and the endpoint would answer + ``{trashed: 0, skipped: [], failed: []}`` — the UI's + ``trashOneLoserFile`` has no matching branch and the card stays stuck + on "Moving to Trash..." even though cleanup finished. + """ + import os + + import app as app_module + + app, db = app_and_db + _w, l, _wp, loser_path = _seed_pair_with_real_files(db, tmp_path, "NETMISS") + + # Advertise the loser's folder as a network mount so the endpoint + # routes it through _trash_paths without stat'ing it first. + volume_root = os.path.dirname(loser_path) + monkeypatch.setattr( + app_module, "_network_volume_roots", lambda: {volume_root}, + ) + + # Real _trash_paths behaviour when Finder reports "missing" for a + # still-mounted network volume: path lands in ``successful`` but + # ``moved`` is zero, and ``already_missing_out`` is populated. + def already_missing_trash( + paths, progress_callback=None, already_missing_out=None, + network_roots=None, + ): + paths = list(paths) + if already_missing_out is not None: + already_missing_out.update(paths) + return 0, set(paths), [] + + monkeypatch.setattr(app_module, "_trash_paths", already_missing_trash) + + client = app.test_client() + resp = client.post( + "/api/duplicates/delete-loser-files", + json={"photo_ids": [l]}, + ) + assert resp.status_code == 200 + body = resp.get_json() + # The endpoint surfaces the terminal state as "file already missing" + # (same wording as the local branch) — the UI has a matching case + # that clears "Moving to Trash..." and the card is no longer stuck. + assert any( + s["id"] == l and s["reason"] == "file already missing" + for s in body["skipped"] + ), body + assert body["trashed"] == 0 + assert body["failed"] == [] + # DB row is dropped so the disk-cleanup-summary count reflects it. + assert db.conn.execute( + "SELECT 1 FROM photos WHERE id=?", (l,), + ).fetchone() is None + + # --------------------------------------------------------------------------- # /api/duplicates/disk-cleanup-summary # ---------------------------------------------------------------------------