fix: e2e cleanup sweeps all active mounts before rm#636
Conversation
The cleanup() function only unmounted the last tracked $MOUNT_POINT, leaving earlier mounts from multi-mount scenarios (sandbox_restore, fast_worktree) still active. When set -e triggered exit mid-scenario, rm -rf $RUN_ROOT hit "Device or resource busy" and in-flight HTTP requests got context-canceled. Now cleanup() sweeps RUN_ROOT for any remaining FUSE mounts and unmounts each before removing the directory. Also dumps all mount logs on failure, not just the last one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe smoke-test script now dumps all mount-related logs under ChangesSmoke-test teardown and log collection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/git-workspace-smoke-test.sh`:
- Around line 316-320: The log sweep in dump_mount_log() is traversing
still-mounted FUSE trees, which can hang and print unrelated workspace logs.
Update the find/read loop to avoid live mountpoints by pruning mounted
directories explicitly or restricting the search to the dedicated mount-log
location, and keep the existing MOUNT_LOG exclusion in place.
- Around line 666-674: The mount cleanup loop in the shell script can still
descend into active FUSE mounts before stop_mount runs, which can block cleanup
and leave rm -rf exposed to resource-busy failures. Update the find pipeline
used in the mount enumeration to avoid traversing inside mount directories (for
example by pruning candidate mount points) or switch to iterating the known
parent mount directory directly. Keep the fix localized around the while/read
loop and the is_mounted check so stop_mount only operates on discovered mount
roots.
🪄 Autofix (Beta)
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
Run ID: 0e9fa746-363f-4b19-acd9-6328a33fb226
📒 Files selected for processing (1)
e2e/git-workspace-smoke-test.sh
![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: 129a53c1c0
ℹ️ 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".
| fi | ||
| # Also dump any other mount logs under RUN_ROOT that weren't the last one. | ||
| if [ -n "${RUN_ROOT:-}" ] && [ -d "$RUN_ROOT" ]; then | ||
| find "$RUN_ROOT" -name '*.log' -type f 2>/dev/null | while read -r logf; do |
There was a problem hiding this comment.
When a scenario fails while its FUSE mount is still active, on_exit calls this before cleanup, so this recursive find can walk into $case_root/mount and traverse the mounted repo instead of only the case-root log files. I checked GNU find locally: --help lists -prune/-xdev as traversal controls, and find -D search confirms a normal recursive search visits children under a matched mount directory; in the failure mode this can block on the unhealthy FUSE mount or dump unrelated repo .log files before cleanup runs.
Useful? React with 👍 / 👎.
| [ -z "$mp" ] && continue | ||
| MOUNT_POINT="$mp" | ||
| stop_mount | ||
| done < <(find "$RUN_ROOT" -maxdepth 3 -name mount -type d 2>/dev/null | while read -r d; do |
There was a problem hiding this comment.
This sweep is meant to find mounted $RUN_ROOT/*/mount directories so it can unmount them, but find -maxdepth 3 still descends through each depth-2 mount directory to inspect depth-3 repo entries before the loop gets a chance to call stop_mount. In the exact stale/wedged FUSE case this cleanup handles, that traversal can hang or do remote I/O; use a shallower search or print-and-prune matching mount directories so discovery does not enter the mounts.
Useful? React with 👍 / 👎.
- dump_mount_log(): add -xdev to avoid crossing into mounted FUSE filesystems that may be wedged (D-state), preventing hangs and stray log file collection from mounted workspaces - cleanup(): reduce -maxdepth 3 to -maxdepth 2 with -prune so find discovers */mount dirs without descending into them, avoiding hangs on stuck FUSE mounts before stop_mount can run Addresses review comments from coderabbitai and codex on PR #636. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
cleanup()only unmounted the last tracked$MOUNT_POINT, leaving earlier mounts from multi-mount scenarios (sandbox_restore,fast_worktree) still activeset -etriggered exit mid-scenario,rm -rf $RUN_ROOThit "Device or resource busy" and in-flight HTTP requests got context-canceledcleanup()sweeps$RUN_ROOTfor any remaining FUSE mounts and unmounts each before removing the directoryContext
From e2e failure:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit