Make deletion of missing VMs idempotent - #474
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 01847e31f6
ℹ️ 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".
|
|
||
| _, err := os.Lstat(filepath.Join(tartHome, "vms", vm.id())) | ||
|
|
||
| return errors.Is(err, os.ErrNotExist) |
There was a problem hiding this comment.
When an intermediate component such as $TART_HOME or its vms directory is a dangling symlink, os.Lstat on the full VM path also returns ENOENT. This therefore treats Tart's exit code 2 as successful deletion even though the storage path is unavailable rather than the VM leaf being confirmed absent; if the symlink target is restored, the VM can reappear after the worker has discarded it. Check the storage ancestors separately before accepting ENOENT for the final VM component.
Useful? React with 👍 / 👎.
tart delete returns exit code 2 for both missing VMs and failures to open an existing VM's lock file. Preserve the original process error and treat this status as success only when Lstat proves that the VM path is absent. Use TART_HOME or the effective user's account home. Keep the deletion error when home resolution is uncertain or an override requires Foundation-specific path expansion or normalization. Add regression coverage for repeated deletion, inaccessible storage, ambiguous paths, missing executables, and retained exit statuses. Validation: vmmanager package tests and lint for changed lines pass.
01847e3 to
077bce1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 077bce1ccc
ℹ️ 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 overridden { | ||
| // Foundation interprets tilde and dot components differently from | ||
| // filepath.Join. Only check overrides that need no path normalization. | ||
| if !filepath.IsAbs(tartHome) || filepath.Clean(tartHome) != tartHome { |
There was a problem hiding this comment.
When TART_HOME is an absolute path with a trailing separator (for example, /Users/worker/.tart/), filepath.Clean(tartHome) != tartHome causes this helper to return false even though removing the separator cannot change which directory Tart uses. Consequently, deleting an already-missing VM still returns ErrVMFailed for this valid configuration, defeating the idempotency added here; reject genuinely ambiguous components such as .. while allowing harmless trailing separators.
Useful? React with 👍 / 👎.
1 participant
Deleting an already-removed Tart VM currently returns an error, preventing a repeated cleanup attempt from succeeding. Tart also uses the same exit code for some failures to open an existing VM, so the exit code alone cannot establish that deletion succeeded.
Treat exit code 2 as success only when a filesystem check confirms that the local VM directory is absent. Preserve permission failures, other command errors, and ambiguous storage-path errors. Keep wrapped process errors available to callers through
errors.Isanderrors.As.Regression tests cover repeated deletion, inaccessible VM files and directories, ambiguous
TART_HOMEpaths, process startup failures, and exit-status preservation.Validation:
go test -count=1 -timeout=120s ./internal/worker/vmmanager/....