Fail closed on unreachable macOS network mounts - #1448
Conversation
|
Warning Review limit reached
Next review available in: 10 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 (4)
📝 WalkthroughWalkthroughNetwork volume discovery now tracks mounted ChangesNetwork volume handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant macOSPathClassifier
participant _network_root_reachable
participant Popen
participant daemon_reaper
macOSPathClassifier->>_network_root_reachable: Check uncertain volume path
_network_root_reachable->>Popen: Start bounded reachability probe
Popen-->>_network_root_reachable: Return result or timeout
_network_root_reachable->>daemon_reaper: Kill and reap abandoned process
_network_root_reachable-->>macOSPathClassifier: Return bounded classification result
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: ff7d48c561
ℹ️ 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 ( | ||
| sys.platform == "darwin" | ||
| and _volume_root_for_path(normalized) is not None | ||
| ): | ||
| return True |
There was a problem hiding this comment.
When a photo is on a mounted local USB/APFS volume, _network_volume_roots() excludes that mount because remote_setup.parse_mount_output() only returns network filesystem types, but this condition now classifies every /Volumes/... path as network-backed anyway. A file already removed from such a healthy local volume is therefore sent through Finder; the subsequent deepest-network-root lookup finds no matching root and rejects Finder's missing result as unreachable, so delete and duplicate-cleanup operations retain the stale catalog row instead of accepting the already-missing file. This also bypasses the existing local-volume _move_to_volume_trash path for all external disks.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
vireo/app.py (1)
1795-1810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
locals()guard with an explicit sentinel.The
"process" in locals()check is correct, becausepopen()can raiseOSErrorbeforeprocessis bound. An explicitNoneinitialization states the same intent more directly and survives later refactoring.♻️ Proposed refactor
+ process = None try: process = popen( argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, **no_window_kwargs(), ) stdout, _stderr = process.communicate(timeout=timeout) except subprocess.TimeoutExpired: _abandon_network_probe(process) return False except (OSError, subprocess.SubprocessError): - if "process" in locals(): + if process is not None: _abandon_network_probe(process) return False🤖 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 1795 - 1810, Initialize process to None before the try block in the probe flow, then replace the locals() guard in the subprocess exception handler with an explicit process-is-not-None check before calling _abandon_network_probe(process).vireo/tests/test_app.py (1)
5049-5054: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the assertion so a synchronous regression fails the test.
The current assertions also pass if
_abandon_network_probebecomes synchronous. In that case_network_root_reachablewould block insiderelease_reaper.wait(timeout=2), returnFalseafter the 2-second timeout, and all four assertions would still hold. The test would only run slower.Add an assertion that the reaper has not finished at the moment the caller returns. That is the property this PR guarantees.
💚 Proposed test hardening
assert app_module._network_root_reachable( "/Volumes/NAS", popen=lambda *args, **kwargs: WedgedProcess(), ) is False + # The caller must return while the reaper is still blocked. A + # synchronous kill-and-wait would have completed it already. + assert not reaper_finished.is_set() assert reaper_started.wait(timeout=1) release_reaper.set() assert reaper_finished.wait(timeout=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 5049 - 5054, Strengthen the test around _network_root_reachable by asserting reaper_finished is not set immediately after the call returns, before waiting for completion. Preserve the existing assertions that the probe returns False, reaper_started is reached, and reaper_finished eventually becomes set after release_reaper is triggered.
🤖 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 1795-1810: Initialize process to None before the try block in the
probe flow, then replace the locals() guard in the subprocess exception handler
with an explicit process-is-not-None check before calling
_abandon_network_probe(process).
In `@vireo/tests/test_app.py`:
- Around line 5049-5054: Strengthen the test around _network_root_reachable by
asserting reaper_finished is not set immediately after the call returns, before
waiting for completion. Preserve the existing assertions that the probe returns
False, reaper_started is reached, and reaper_finished eventually becomes set
after release_reaper is triggered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cfe3f26-6113-410c-a270-e2d2488aa9e7
📒 Files selected for processing (2)
vireo/app.pyvireo/tests/test_app.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abed1be31e
ℹ️ 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".
| threading.Thread( | ||
| target=_reap_abandoned_network_probe, | ||
| args=(process,), | ||
| name="vireo-network-probe-reaper", | ||
| daemon=True, | ||
| ).start() |
There was a problem hiding this comment.
When a filesystem call remains uninterruptible even after kill()—the exact case this change is designed to handle—the new daemon thread blocks forever in process.communicate(), while the child also remains present until the syscall returns. Each retry of a Trash operation against the same wedged mount starts another probe and another reaper without any registry or concurrency cap, so repeated requests can exhaust the application's thread or process limits; reuse an outstanding probe or bound the number of abandoned probes.
Useful? React with 👍 / 👎.
|
The Codex P1 ( Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vireo/app.py (1)
1771-1811: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog network-probe reservation rejections.
_reserve_network_probereturnsFalseboth when a root already has an active probe and when_MAX_NETWORK_PROBESis reached._network_root_reachablethen returnsFalse(unreachable) with no log output in either case.This subsystem already fails closed and is hard to observe from the outside. A genuinely reachable share rejected only because of capacity or a concurrent duplicate probe is indistinguishable, from the logs, from a truly wedged mount. Add a log line (
log.debugorlog.warning) at the rejection point in_reserve_network_probe, including the root and the current probe count, so an operator can tell "probe capacity exhausted" from "the mount is actually stuck."🔍 Proposed logging addition
def _reserve_network_probe(root): """Reserve a bounded probe slot, reusing one already active per root.""" with _NETWORK_PROBE_LOCK: if root in _NETWORK_PROBES: + log.debug( + "Network probe for %s already active; skipping (unreachable " + "verdict does not necessarily mean the mount is wedged)", + root, + ) return False if len(_NETWORK_PROBES) >= _MAX_NETWORK_PROBES: + log.debug( + "Network probe capacity (%d) reached; skipping probe for %s", + _MAX_NETWORK_PROBES, root, + ) return False _NETWORK_PROBES[root] = _NETWORK_PROBE_RESERVED return True🤖 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 1771 - 1811, Update _reserve_network_probe to log a debug or warning message immediately before each False return, including the rejected root and current probe count; distinguish an already-active probe from reaching _MAX_NETWORK_PROBES so operators can identify the rejection cause.
🤖 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 1702-1726: Align _mounted_volume_roots_from_mount_output with
remote_setup.parse_mount_output so unlisted network filesystem types cannot be
treated as local; either centralize mount-line parsing and reuse the same
network classification, or consistently expand the supported network types in
both places. Preserve correct handling of mount-point names containing literal “
(” or “ on ” by avoiding divergent parsing logic.
---
Nitpick comments:
In `@vireo/app.py`:
- Around line 1771-1811: Update _reserve_network_probe to log a debug or warning
message immediately before each False return, including the rejected root and
current probe count; distinguish an already-active probe from reaching
_MAX_NETWORK_PROBES so operators can identify the rejection cause.
🪄 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: 8fe960d3-1edb-403e-841d-cb14f3ecbe80
📒 Files selected for processing (2)
vireo/app.pyvireo/tests/test_app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- vireo/tests/test_app.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c334ec84b4
ℹ️ 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".
|
|
||
| _NETWORK_FS = ("smbfs", "nfs", "afpfs", "webdav") | ||
| _LOCAL_FS = frozenset({ | ||
| "apfs", "autofs", "cd9660", "devfs", "exfat", "fdesc", "hfs", |
There was a problem hiding this comment.
When a catalog path is beneath a macOS auto_home/autofs mount whose target is an unavailable network share, this allowlist removes the mount from _network_volume_roots(). _path_on_network_volume() consequently selects the local path, and _snapshot_parent_device() or os.path.isfile() can trigger the automount and block indefinitely—the failure mode this change is intended to avoid. autofs indicates an automount trigger, not local storage, so it must remain network-or-unknown for Trash routing.
Useful? React with 👍 / 👎.
autofs entries are automount triggers, not local storage: the target they resolve to can be an unavailable network share, so any in-process `stat`/`isfile` call under an autofs mount trips the automount and can block indefinitely — the exact failure mode this PR's bounded probe path is designed to avoid. Move `autofs` out of `_LOCAL_FS` so `mount_type_is_network_or_unknown` returns True for it and callers route through the bounded, out-of-process reachability probe instead of a synchronous kernel call. Addresses Codex P1 review on PR #1448 (remote_setup.py:40).
|
Addressed the outstanding Codex P1 on The Codex P2 ( Local run of the required suite ( 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: b5e26ebb9e
ℹ️ 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 line in text.splitlines(): | ||
| m = _MOUNT_RE.match(line.strip()) | ||
| if not m: | ||
| match = _MOUNT_RE.match(line.strip()) |
There was a problem hiding this comment.
On macOS, the standard auto-home entry is formatted like map auto_home on /System/Volumes/Data/home (autofs, ...), but _MOUNT_RE still restricts the source to \S+, so this new generic parser silently drops the entry before mount_type_is_network_or_unknown() can classify it. Paths beneath that automount are therefore treated as local and can reach the blocking in-process stat/isfile path when the network target is unavailable. Fresh evidence beyond the earlier autofs comment is that removing autofs from _LOCAL_FS does not help because the actual multiword mount line never reaches that check.
Useful? React with 👍 / 👎.
Codex flagged that _MOUNT_RE's \S+ source rejected the standard macOS
auto_home entry ("map auto_home on /System/Volumes/Data/home
(autofs, ...)"). parse_mount_table silently dropped those lines, so the
follow-up _LOCAL_FS check excluding autofs never ran for them and paths
beneath the automount fell through to the local-classification branch —
the same in-process stat/isfile blocking path the fail-closed probe was
introduced to avoid.
Allow multi-word sources by anchoring the split at the first " on /"
boundary: the mount point is always an absolute path, so requiring it to
start with / keeps the source/mount-point split unambiguous without
adding fs_type-specific parsing. Existing coverage for the space-in-
mount-point case (server:/archive on /Volumes/Photo on Film (Archive))
still passes because the greedy mount_point group only backs off far
enough to satisfy the trailing options group.
Adds a regression test asserting that both auto_home and -fstab autofs
entries survive parsing and classify as network-or-unknown so callers
route them through the bounded probe path.
2 participants
Follow-up to #1446 addressing three late P1 review findings after that PR was automatically merged. Mount discovery failures now route all macOS candidates through bounded Finder handling, and detached paths under /Volumes remain network-classified even when the current mount table no longer lists them. Network-root stat probes now use Popen and hand timed-out children to a daemon reaper instead of synchronously waiting on an uninterruptible filesystem call. Regression tests cover wedged-probe abandonment, detached mount-table entries, and custom mount discovery failure.
Summary by CodeRabbit
Bug Fixes
Tests