Prevent SMB photo deletes from hanging - #1446
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe trash workflow detects macOS network volumes, routes them through Finder, reconciles per-file results, validates detached mounts, and reports progress. Duplicate cleanup now handles network paths and already-missing files explicitly. Tests cover routing, Finder outcomes, mount changes, and callback compatibility. ChangesmacOS trash workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DiskOperation
participant _trash_paths
participant mount
participant Finder
DiskOperation->>_trash_paths: process paths with progress callback
_trash_paths->>mount: discover mounted volumes
mount-->>_trash_paths: classify network paths
_trash_paths->>Finder: process network-volume paths
Finder-->>_trash_paths: return per-path statuses
_trash_paths-->>DiskOperation: report progress and missing paths
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5237465148
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| report_processed(filepath) | ||
| continue | ||
| if _path_on_network_volume(filepath, network_roots): |
There was a problem hiding this comment.
When the SMB share is already unhealthy, _snapshot_parent_device() and os.path.isfile() run for every path before this network-volume branch is reached. Both issue in-process filesystem calls against the share and can block indefinitely during SMB reconnection, so a delete can still hang before reaching the time-bounded Finder subprocess. Classify network candidates first and avoid all source and parent stat calls for them.
Useful? React with 👍 / 👎.
| volume_root = _volume_root_for_path(filepath) | ||
| if volume_root is None: | ||
| return False |
There was a problem hiding this comment.
When an SMB/NFS share is mounted at a custom mount point such as /Users/me/mnt/photos, _network_volume_roots() discovers that root, but this early /Volumes check returns False without comparing it. Such paths therefore fall through to send2trash() in the server worker and retain the unbounded-I/O behavior this routing is meant to prevent. Compare paths against discovered network roots regardless of their mount-point prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25b772f698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # After an error or timeout, retain every catalog row even if | ||
| # Finder may have moved a prefix of the batch. Rechecking those | ||
| # paths here would put the worker straight back into potentially | ||
| # unbounded SMB stat calls; a retry safely reconciles files that | ||
| # were already moved. |
There was a problem hiding this comment.
When Finder moves a prefix of an SMB batch and then times out, retaining every row makes the next retry include those now-missing paths. Because _trash_via_finder() has no per-item error handling, Finder errors on the first already-missing item and aborts the repeat loop before later files are attempted; network candidates also skip the absence preflight, so subsequent retries can remain stuck indefinitely. The bounded Finder operation needs to report/reconcile individual outcomes or tolerate already-missing items during retries.
Useful? React with 👍 / 👎.
|
Both Codex findings are addressed by 25b772f:
Related trash/network tests pass locally (10/10 in Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8fd1f55cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "-e", ( | ||
| "if (not sourceExists) and parentExists then " | ||
| "set statusValue to \"missing\"" |
There was a problem hiding this comment.
When a mounted volume disconnects but leaves its mount-point directory on the underlying filesystem, Finder observes sourceExists = false and parentExists = true, so this marks the path successful and the caller removes its catalog row even though the photo reappears after remounting. The previous post-Finder _path_confirmed_gone(..., expected_parent_dev) check explicitly rejected this device-change case; preserve equivalent mount-identity validation before accepting a missing outcome.
Useful? React with 👍 / 👎.
| for filepath in ordered: | ||
| if _path_on_network_volume(filepath, network_roots): | ||
| finder_candidates.append(filepath) |
There was a problem hiding this comment.
The separate /api/duplicates/delete-loser-files endpoint is fresh evidence beyond the earlier helper-level preflight issue: it still calls os.path.isfile(filepath) at line 22186 before invoking _trash_paths. For an unhealthy SMB share, that call can block indefinitely, so this new bounded network routing is never reached during duplicate cleanup; a failed stat can also be mistaken for a missing file and cause the row to be deleted. Route or classify these candidates before that endpoint performs filesystem I/O.
Useful? React with 👍 / 👎.
Addresses two P1 Codex review comments on the SMB-safe trash routing: 1. _trash_via_finder previously trusted its own "missing" outcome whenever Finder reported sourceExists=false and parentExists=true. An unmounted network volume leaves its mount-point directory in place on the underlying local FS, so parentExists=true does not prove the volume is still there — accepting missing would prune the catalog row for a photo that reappears on remount. Split its return shape into (moved, missing, failures) so the caller can revalidate mount identity. In _trash_paths, network-classified paths are rechecked against a fresh mount table (bounded by the mount subprocess timeout) and local fallbacks reuse the existing parent-device snapshot; a mount that disappeared or a stat we cannot perform is refused as unreachable. 2. /api/duplicates/delete-loser-files called os.path.isfile on every candidate before delegating to _trash_paths. On an unhealthy SMB share that stat can block indefinitely inside the request thread, never reaching the network-aware routing; a stat failure was also misread as "already missing" and dropped the DB row. Classify paths via the mount table first and hand network candidates straight through — local paths keep the existing preflight so manually-cleaned local losers still surface as "file already missing". Regression coverage: - _trash_paths rejects Finder's "missing" outcome when the mount table no longer lists the network root, when the recheck fails outright, and (for local fallbacks) when the parent's st_dev has drifted; accepts it when the mount is still healthy. - /api/duplicates/delete-loser-files never calls os.path.isfile for a network-classified path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b292a15bc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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)) |
There was a problem hiding this comment.
When a loser is already absent from a still-mounted network share, Finder returns missing and _trash_paths includes the path in successful_paths while leaving trashed at zero. This branch therefore deletes the catalog row but returns trashed: 0 with empty skipped and failed lists; trashOneLoserFile has no matching UI branch, so the card remains disabled with “Moving to Trash...” even though cleanup completed. Preserve the existing file already missing outcome for this network path, or otherwise return an explicit terminal result.
Useful? React with 👍 / 👎.
| if filepath in network_finder_candidates: | ||
| current_roots = _network_volume_roots() | ||
| if current_roots is None: |
There was a problem hiding this comment.
When a retry contains many paths already moved by an earlier timed-out Finder call, every path is returned as missing and this invokes a new mount subprocess for each one. A full 20-item batch can consequently spend up to another 100 seconds in the five-second mount-query timeout, and large cleanups spawn one process per missing file, undermining the bounded batch behavior this change introduces. Query the current roots once per Finder batch and reuse that result for all missing outcomes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vireo/app.py`:
- Around line 10644-10663: Update the permanent-delete branch of operate to
classify each path with _path_on_network_volume using _network_volume_roots
before any os.stat, os.path.isfile, or os.remove call. Skip network paths
entirely (fail closed), while preserving the existing permanent-delete behavior
for local paths and its progress accounting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03bb468e-84d0-48c1-97d9-5c77dd01dbf2
📒 Files selected for processing (3)
vireo/app.pyvireo/tests/test_app.pyvireo/tests/test_delete_api.py
Addresses two P2 Codex review comments on the SMB-safe trash routing:
1. When every duplicate loser is already absent on a still-mounted
network share, `_trash_paths` places each path in `successful` but
leaves `moved` at zero, and the duplicate-cleanup endpoint answered
`{trashed: 0, skipped: [], failed: []}` — a silent success the
frontend `trashOneLoserFile` has no matching branch for, so the card
stays disabled on "Moving to Trash..." forever. `_trash_paths` now
accepts an opt-in `already_missing_out` set that is populated for
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). The delete-loser-files endpoint
passes this in and surfaces those paths as
`{reason: "file already missing"}` in `skipped`, matching the
existing local-branch wording so the UI's terminal branch fires and
the card clears.
2. `_finder_missing_is_trustworthy` previously called
`_network_volume_roots()` for every missing path, spawning one
`mount` subprocess per path. A retry containing 20 already-moved
paths could therefore burn up to `_MOUNT_QUERY_TIMEOUT_SECS × 20`
seconds in the worst case, undoing the bounded-batch guarantee the
Finder routing is meant to give. The recheck now runs at most once
per Finder batch (and only when the batch actually contains a
network path with a `missing` outcome); the shared roots are passed
into the trust check for every missing path in that batch.
Regression coverage:
- `_trash_paths` populates `already_missing_out` with the Finder
`missing` paths on a still-mounted volume while leaving actually
moved paths out of it.
- `_trash_paths` calls `_network_volume_roots` at most twice for a
20-path all-missing batch (initial classification + one batch
recheck), never one call per path.
- `/api/duplicates/delete-loser-files` returns the network loser as
`skipped` with reason "file already missing" (and drops the orphan
row) when the file is already absent on the still-mounted share.
|
Both P2 Codex findings on
Regression coverage:
Related trash/duplicates suites pass locally (19/19 trash tests in Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed82b68e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| normalized = os.path.normpath(os.path.abspath(filepath)) | ||
| for root in network_roots: | ||
| try: | ||
| if os.path.commonpath((normalized, root)) == root: | ||
| return True |
There was a problem hiding this comment.
On macOS, when a catalog folder is a symlink such as /Users/me/photos -> /Volumes/NAS/photos, abspath preserves the symlink spelling, so this comparison does not recognize the path as belonging to the discovered NAS root. Vireo supports directly selected symlink roots in scanner.py:1584-1598; once the NAS is unhealthy, _trash_paths therefore classifies such photos as local and can block indefinitely in _snapshot_parent_device() or os.path.isfile() before reaching the bounded Finder subprocess.
Useful? React with 👍 / 👎.
| # confirm the volume is still mounted, so refuse to trust | ||
| # "missing" and let the caller retry. | ||
| return False | ||
| return _path_on_network_volume(filepath, current_network_roots) |
There was a problem hiding this comment.
If Finder reports missing while an SMB volume is detached, but the volume reconnects at the same mount point before this recheck, _path_on_network_volume() returns true solely because that root is listed again and the caller deletes the catalog row even though the photo has reappeared. Fresh evidence in this revision is that the new validation checks only lexical mount-root membership, not the original mount instance or the path's current absence; preserve mount identity or perform a bounded existence recheck before accepting this result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80e1c3924e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trashed, successful_paths, trash_failures = _trash_paths( | ||
| [filepath for _pid, filepath in trash_candidates], | ||
| already_missing_out=already_missing_paths, | ||
| ) |
There was a problem hiding this comment.
When a share uses a custom mount point and the endpoint's first mount query succeeds but _trash_paths' immediate second query times out or fails, the helper receives network_roots=None; _path_on_network_volume then fails closed only for /Volumes paths and reclassifies the already-known custom share as local, allowing _snapshot_parent_device or os.path.isfile to block indefinitely on the unhealthy share. Fresh evidence beyond the earlier duplicate-cleanup preflight concern is that this call discards the successful classification made at lines 22392-22415 instead of passing it into _trash_paths.
Useful? React with 👍 / 👎.
| return ( | ||
| _path_on_network_volume(filepath, current_network_roots) | ||
| and filepath in confirmed_network_missing |
There was a problem hiding this comment.
When an SMB mount remains listed while its server is unreachable, Finder can report the source as absent while cached parent metadata still reports the parent as present; _missing_paths_via_finder merely repeats the same Finder exists query, so two false-negative checks put the path in confirmed_network_missing and this condition accepts it as successful, causing callers to remove the catalog row even though the photo reappears when connectivity returns. Fresh evidence beyond the earlier detach/remount concerns is that the new confirmation adds no independent reachability or mount-identity signal.
Useful? React with 👍 / 👎.
…robe Addresses two P1 findings from the Codex review on commit 80e1c39: 1. **Preserve endpoint's network_roots into `_trash_paths`.** The duplicates delete-loser-files endpoint stats the mount table once (line 22392) and classifies custom-mount shares (`/Users/me/mnt/photos`) as network-backed. It then discarded that result and let `_trash_paths` re-query the table — if that second query timed out or failed, the helper's `_path_on_network_volume` fallback only catches `/Volumes` paths, so an already-known custom mount would be silently reclassified as local and trigger the unbounded in-process I/O this whole routing exists to prevent. Thread `network_roots` through as a parameter and pass the endpoint's successful result. 2. **Add an independent reachability signal before accepting Finder "missing" outcomes.** An SMB mount can stay listed in the kernel mount table while its server is unreachable; both Finder queries (`_trash_via_finder` and `_missing_paths_via_finder`) share the same exists-cache, so two false negatives can put a path in `confirmed_network_missing` even though the photo will reappear on reconnect. Add `_network_root_reachable`, a bounded out-of-process `stat` probe on the mount root that is independent of Finder's cache and cannot hang the caller (subprocess is killed on timeout). Require it, in addition to a still-listed mount and a Finder confirmation, before accepting a network path as trustworthy-gone. Runs once per relevant mount root per Finder batch to preserve the bounded-batch behaviour. Tests: adds coverage for the reachability probe (bounded subprocess, timeout fail-closed, platform gating), for `_trash_paths` rejecting missing when the mount root is unreachable, for the endpoint plumbing its own `network_roots` through, and for `_trash_paths` honouring a caller-supplied classification without re-querying. Existing tests that rely on missing outcomes being accepted are updated to mock the new probe. Full suite: 2094 passed, 16 skipped, 1 unrelated failure (`test_failed_import_does_not_chain` — missing local exiftool binary, same as noted in the PR description).
There was a problem hiding this comment.
🧹 Nitpick comments (5)
vireo/app.py (1)
1981-1982: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the empty-input return with the new tuple shape.
The docstring and the terminal return now yield
(moved_paths, missing_paths, failures), where the first element is a set. The empty-input branch still returns0for that element. The caller at Line 2257 computeslen(finder_moved_paths), which raisesTypeErroron the int form._chunkednever yields an empty batch today, so this is currently unreachable from_trash_paths, but the contract mismatch will break the next direct caller.♻️ Proposed fix
if not filepaths: - return 0, set(), [] + return set(), set(), []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/app.py` around lines 1981 - 1982, Update the empty-input branch in the function containing the `filepaths` check to return an empty set as `moved_paths`, matching the documented and terminal `(moved_paths, missing_paths, failures)` tuple shape. Preserve the existing empty values for missing paths and failures, and ensure callers such as the `finder_moved_paths` length calculation can handle the result.vireo/tests/test_duplicates_api.py (2)
502-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
andexpression statement with anif.Line 505 uses a boolean expression for control flow. An explicit
ifstates the intent and avoids a lint warning for a useless expression.♻️ Proposed change
def record_trash(paths, progress_callback=None, already_missing_out=None): trash_calls.append(list(paths)) for path in paths: - real_isfile(path) and os.remove(path) + if real_isfile(path): + os.remove(path) return len(paths), set(paths), []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/tests/test_duplicates_api.py` around lines 502 - 506, Update record_trash so the real_isfile(path) check and os.remove(path) call use an explicit if statement instead of a boolean-expression statement, preserving the existing deletion behavior for each path.
572-581: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the
trashedcounter.The test proves the skipped reason but leaves
body["trashed"]unchecked. A regression that counts an already-missing loser as trashed would still pass. Add the counter assertion so the user-visible count stays correct.♻️ Proposed change
assert resp.status_code == 200 body = resp.get_json() + assert body["trashed"] == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/tests/test_duplicates_api.py` around lines 572 - 581, Extend the duplicate-file API test assertions after validating body["skipped"] to also verify body["trashed"] has the expected count for an already-missing loser, while preserving the existing empty body["failed"] assertion.vireo/tests/test_app.py (2)
4636-4670: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the batch expectation from
_FINDER_TRASH_BATCH_SIZE.The test hard-codes 20 paths and expects exactly 2 mount queries. That holds only while
_FINDER_TRASH_BATCH_SIZE >= 20. If the constant is lowered,_chunkedyields more batches and the assertion fails for a reason unrelated to the behaviour under test.♻️ Proposed change to bind the test to the constant
- photos = [str(volume / f"bird-{i:02d}.NEF") for i in range(20)] + batch_size = app_module._FINDER_TRASH_BATCH_SIZE + photos = [str(volume / f"bird-{i:02d}.NEF") for i in range(batch_size)]- assert len(mount_call_paths) == 2 + # One classification call at the top of _trash_paths + one recheck for + # the single Finder batch. Never one-per-path. + assert len(mount_call_paths) == 2 + assert len(photos) > 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/tests/test_app.py` around lines 4636 - 4670, Update the test around _trash_paths to generate photos using _FINDER_TRASH_BATCH_SIZE and assert the expected mount-query count based on the resulting Finder batch count, rather than hard-coding 20 paths and 2 calls. Preserve the expectation of one initial classification query plus one query per Finder batch.
4593-4600: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
_Shiftedstat stub exposes only two attributes.
stat_with_shifted_devreturns an object withst_devandst_modeonly. Any future access to another field, for examplest_inoorst_nlink, raisesAttributeErrorinstead of failing the assertion clearly. Copy the real result and overridest_devinstead, for example with a smallos.stat_result-backed wrapper orunittest.mock.Mock(spec=result, st_dev=baseline_dev + 1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/tests/test_app.py` around lines 4593 - 4600, Update stat_with_shifted_dev so the shifted result preserves the full real_stat result interface while overriding only st_dev; use an os.stat_result-backed wrapper or a spec-constrained mock based on result, and retain the original st_mode and all other stat attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vireo/app.py`:
- Around line 1981-1982: Update the empty-input branch in the function
containing the `filepaths` check to return an empty set as `moved_paths`,
matching the documented and terminal `(moved_paths, missing_paths, failures)`
tuple shape. Preserve the existing empty values for missing paths and failures,
and ensure callers such as the `finder_moved_paths` length calculation can
handle the result.
In `@vireo/tests/test_app.py`:
- Around line 4636-4670: Update the test around _trash_paths to generate photos
using _FINDER_TRASH_BATCH_SIZE and assert the expected mount-query count based
on the resulting Finder batch count, rather than hard-coding 20 paths and 2
calls. Preserve the expectation of one initial classification query plus one
query per Finder batch.
- Around line 4593-4600: Update stat_with_shifted_dev so the shifted result
preserves the full real_stat result interface while overriding only st_dev; use
an os.stat_result-backed wrapper or a spec-constrained mock based on result, and
retain the original st_mode and all other stat attributes.
In `@vireo/tests/test_duplicates_api.py`:
- Around line 502-506: Update record_trash so the real_isfile(path) check and
os.remove(path) call use an explicit if statement instead of a
boolean-expression statement, preserving the existing deletion behavior for each
path.
- Around line 572-581: Extend the duplicate-file API test assertions after
validating body["skipped"] to also verify body["trashed"] has the expected count
for an already-missing loser, while preserving the existing empty body["failed"]
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f52609f4-8c4d-42f3-bb83-ee29a48d8cff
📒 Files selected for processing (3)
vireo/app.pyvireo/tests/test_app.pyvireo/tests/test_duplicates_api.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4fcf678bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # respond to a bounded out-of-process stat, treat "missing" | ||
| # as a false negative from an unreachable server rather than | ||
| # as evidence the photo is gone. | ||
| return _path_on_network_volume(filepath, reachable_network_roots) |
There was a problem hiding this comment.
When network mounts are nested (for example, an unavailable SMB mount at /Volumes/NAS/archive beneath a reachable mount at /Volumes/NAS), this accepts the path if any reachable ancestor root matches it. Because relevant_roots also selects an arbitrary first match from a set, the outer root may be the only root probed; it can therefore validate Finder's false missing result for the unavailable inner mount, causing the catalog row to be removed before the photo reappears. Associate each path with its deepest matching mount and require that exact root to be reachable.
Useful? React with 👍 / 👎.
Codex flagged that ``_trash_paths`` was accepting a Finder ``missing`` outcome as trustworthy whenever *any* reachable network root contained the path — an outer share could vouch for a detached inner share mounted underneath it (e.g. ``/Volumes/NAS/archive`` beneath ``/Volumes/NAS``). Iterating ``batch_network_roots`` (a set) with a first-match ``break`` also picked the reachable-root probe target in arbitrary order, so the outer root was often the only one probed at all. That would prune the catalog row for a photo that reappears on reconnect. Introduce ``_deepest_network_root_for_path``: longest-common-prefix matching over ``network_roots`` (with the same one-symlink-prefix expansion loop as ``_path_on_network_volume``) that returns the deepest matching root. ``_trash_paths`` now builds a per-batch ``path_to_deepest_root`` mapping, probes reachability of each distinct deepest root, and passes the mapping into ``_finder_missing_is_trustworthy``; trust now requires the path's *deepest* matching root to be in ``reachable_network_roots`` rather than merely being any-reachable-root-membership. Add ``test_trash_paths_rejects_missing_when_nested_inner_mount_unreachable``: two paths on nested mounts (outer reachable, inner detached); asserts the outer path is accepted, the inner is preserved for retry, and reachability was probed for *both* deepest roots — proving we no longer rely on set iteration order picking a single winner.
|
The P1 Codex finding on
Regression coverage:
The other Codex findings on earlier commits (line 1798 symlink, line 2042 mount-identity, line 2189 duplicate-cleanup preflight, line 22487 already-missing losers, line 22519 preserve endpoint classification) were already addressed by later commits in this branch — verified against current HEAD. Full trash/network suite passes (28/28 in Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38f80265b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if network_roots is None: | ||
| network_roots = _network_volume_roots() |
There was a problem hiding this comment.
When the duplicate-cleanup endpoint's first mount query fails, it passes network_roots=None after correctly classifying /Volumes/... paths via the fail-closed fallback, but this branch interprets that explicit failure as “not supplied” and queries again. If the second query succeeds after the share has detached and returns no matching root, the path is reclassified as local; the underlying mount-point directory can then make the absence preflight succeed, causing the catalog row to be deleted even though the photo reappears on remount. Fresh evidence beyond the earlier classification comment is the new parameter's conflation of its default with the mount-query failure value; use a distinct sentinel so an explicitly passed None remains fail-closed.
Useful? React with 👍 / 👎.
Codex flagged (P1) that _trash_paths(..., network_roots=None) treats the fail-closed signal None — passed by callers whose own _network_volume_roots() query failed — as if the caller had omitted the argument, then re-queries the mount table. If that second query succeeds after the share has detached, the caller's already-correct classification of a custom-mount path (e.g. /Users/me/mnt/photos, which the /Volumes fallback in _path_on_network_volume had flagged network) is silently overwritten with an empty or mismatched result and the path is reclassified as local — reintroducing the unbounded in-process I/O this routing exists to prevent. Add a module-level _NETWORK_ROOTS_UNSET sentinel as the default for the network_roots parameter so None remains distinguishable from 'not supplied'. Only re-query when the caller actually omitted the argument; an explicit None passes straight through and preserves the fail-closed classification. Regression test locks the behaviour: an explicit network_roots=None routes a /Volumes/... path through Finder without touching _network_volume_roots again.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4966747247
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for root in set(path_to_deepest_root.values()): | ||
| if _network_root_reachable(root): | ||
| reachable_network_roots.add(root) |
There was a problem hiding this comment.
When one Finder batch contains missing paths from several network mounts, this loop runs each five-second _network_root_reachable() probe serially. With the configured 20-item batch size, 20 unavailable roots add up to 100 seconds before the subsequent Finder recheck, so a bulk delete spanning multiple shares can still appear hung despite each individual subprocess having a timeout. Run these per-root probes concurrently or enforce one timeout budget for the entire batch.
Useful? React with 👍 / 👎.
Addresses Codex P2 review on #1446 (commit 4966747): the per-batch reachability loop ran each `_network_root_reachable()` probe serially, so a Finder batch spanning many unavailable roots could accumulate `len(distinct_roots) × _MOUNT_QUERY_TIMEOUT_SECS` seconds of hang time before the subsequent Finder recheck. With `_FINDER_TRASH_BATCH_SIZE = 20` and a 5s probe timeout, a full batch of unreachable-root paths would add up to ~100s and undermine the bounded-batch behaviour this code exists to establish. Run the distinct-root probes concurrently via a `ThreadPoolExecutor` sized to the batch's root count. Each probe still enforces its individual timeout, so wall-clock is bounded by one probe timeout rather than the sum. Regression test uses a `threading.Barrier` to prove the probes run concurrently — a serial implementation would never satisfy the barrier's party count and the wait would raise.
|
The P2 Codex finding on
Regression coverage:
The remaining unresolved Codex threads on earlier commits (line 1807 symlink, line 2096 mount-identity, line 2243 duplicate-cleanup preflight, line 22556 already-missing losers, line 22590 preserve endpoint classification) were already addressed by later commits — verified against current HEAD. Full mandated suite: 2099 pass, 16 skip. The pre-existing unrelated Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508773ed74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = run( | ||
| ["/usr/bin/stat", "-f", "%HT", root], | ||
| capture_output=True, text=True, | ||
| timeout=timeout, | ||
| **no_window_kwargs(), |
There was a problem hiding this comment.
When /usr/bin/stat becomes stuck in an uninterruptible SMB filesystem call, subprocess.run(..., timeout=...) is not a hard deadline: after the timeout Python sends SIGKILL and then synchronously calls wait(), which can remain blocked until the kernel call returns. The executor context subsequently waits for that worker as well, so deleting from the exact kind of unavailable share addressed here can still hang indefinitely. Fresh evidence beyond the earlier batch-budget concern is that the new reachability implementation uses this kill-and-wait path; use a probe design that can abandon a timed-out child without synchronously reaping it.
Useful? React with 👍 / 👎.
| else: | ||
| for root in network_roots: | ||
| try: | ||
| if os.path.commonpath((normalized, root)) == root: | ||
| return True |
There was a problem hiding this comment.
When a NAS has already detached before mount discovery, mount succeeds but returns a set with no matching root, so /Volumes/NAS/photo.jpg falls through this loop and is classified as local. The subsequent parent snapshot and missing-file checks both inspect the underlying local mount-point directory, see the same device, and accept the photo as already gone, causing callers to delete its catalog row even though it reappears after remounting. Fresh evidence beyond the earlier remount findings is the successful-empty-set classification path; a missing current mount-table entry must not by itself prove that a catalog path is local.
Useful? React with 👍 / 👎.
| 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 |
There was a problem hiding this comment.
When the bounded mount query fails and returns None, this fallback protects only paths under /Volumes. A network share mounted at a supported custom location such as /Users/me/mnt/photos is therefore classified as local and proceeds to _snapshot_parent_device() and os.path.isfile(), either of which can block the request indefinitely on an unhealthy share. Treat all macOS candidates conservatively when discovery itself failed, rather than interpreting non-/Volumes paths as local.
Useful? React with 👍 / 👎.
|
The three P1 Codex findings on
Regression coverage:
The three existing Full Generated by Claude Code |
2 participants
Vireo could hang indefinitely while moving photos to Trash on an unhealthy macOS SMB mount because
os.replaceran inside the server worker.This routes network-volume deletions through the existing time-bounded Finder subprocess, fails closed when mount discovery fails, retains catalog rows on timeout or error, and preserves the atomic rename fast path for local removable volumes.
It also reports file-level batch-delete progress and adds regression coverage for mount classification, bounded routing, failure behavior, and the progress callback.
Validation: 42 delete API tests, 13 focused Trash tests, and 32 duplicate-cleanup tests pass; the full app module has 426 passes and one unrelated local ExifTool availability failure.
Summary by CodeRabbit
Bug Fixes
Improvements