Repository navigation
fix(sandbox): remove a sandbox directory whose build failed - #2357
Conversation
Config::build creates the sandbox's directory, then any later failure (fs-mapping validation, the synth user/group config, any step of Sandbox::new, including hardlinking or copying inputs) returned before a Sandbox existed, so its Drop never removed the directory: a failed build left a partial tree behind, and the next one made a new directory. A RemoveUnlessKept guard removes the directory unless the Sandbox was built (its Drop owns cleanup from then on), and stays out of the way when keep_dirs is set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: removing sandbox directories when a build fails. Description check The description explains the failure scenario, implementation, keep_dirs behavior, and testing results. It includes the required summary and testing information, although it does not reproduce the che… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the failure scenario, implementation, keep_dirs behavior, and testing results. It includes the required summary and testing information, although it does not reproduce the checklist section or provide command output.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the sandbox door,
The failed build leaves no crumbs on the floor.
A guard keeps watch, then steps aside,
When Sandbox is safely inside.
The rabbit hops along, content once more.
Comment @coderabbitai help to get the list of available commands.
|
Approved: no findings at Nothing to do. Clobber: none seen. |
|
recheck |
Config::buildcreates the sandbox's directory, then can still fail before aSandboxexists to own it: fs-mapping validation, the synth user/group config, or any step ofSandbox::new(hardlinking inputs, and copying them since #2234). Every one of those returns with the directory still there, because onlySandbox'sDropremoves it. A failed build left a partial tree behind, and the next build made a new uniquely-named directory instead of reusing it. CodeRabbit raised this on #2234 for the new copy path, but it applies to all of them.Change: a
RemoveUnlessKeptguard is armed right after the directory is created. Any early return removes the directory (and logs if removal fails). OnceSandbox::newsucceeds, the guard is disarmed and theSandbox's ownDropowns cleanup, as before. Withkeep_dirsset, the guard is never armed, so a failed build's directory is kept for inspection, matching the debugging option's intent.Testing (Linux): fmt is clean,
clippy -p sandbox --all-targets -D warningsis clean, and the new testa_failed_build_removes_its_directorypasses. It drivesConfig::buildintoSandbox::new'sMappedFilerejection, which happens after the directory and its synth config exist, and asserts the base directory is empty afterwards.Follow-up to a review thread on #2234.
🤖 Generated with Claude Code
Summary by CodeRabbit