Studio: keep the working llama.cpp install when a prebuilt update can't move it aside - #9221
Conversation
…during a prebuilt update
|
mahiatlinux
left a comment
There was a problem hiding this comment.
Review posted as a comment above.
…annot be moved aside
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Confirmed against activate_install_tree in studio/install_llama_prebuilt.py, where the recovery block still passes rollback_dir to cleanup and removes the only remaining copy of the working install; the failed-move retry and the no-previous-install test both look right now. |
…ng a copy of it
Fault injection over the failure paths of activate_install_tree showed the
retained rollback tree is preserved but unreachable: no loader looks under
.staging, so a run that keeps one still leaves the user with nothing at the
install path, and nothing in the tree ever removes those trees again, so a
machine that keeps failing parks another multi-gigabyte copy every time.
- Retry the aside-move and the rollback restore against transient Windows
sharing violations (WinError 5/32/145), the way the Node installer's
_replace_with_retry already does. A scanner holding a freshly extracted
DLL clears in a second or two; without a backoff it aborted the update.
- Copy the previous install back when the restore rename cannot run at all,
so the choice stops being between a rename and an empty install directory.
A linked install is never copied, because copytree follows its source root
even with symlinks = True and that would turn a --with-llama-cpp-dir link
into a real duplicate of a checkout this installer does not own.
- Keep exactly one retained copy, and reclaim every retained tree once an
activation has been confirmed, which is the one moment they are provably
no longer the last llama.cpp on the machine.
- Clear the read-only attribute during cleanup on Windows, where it blocks
an unlink outright. Not on POSIX: there the permission that decides an
unlink lives on the parent directory, so the chmod cannot help and would
leave an unreadable directory at 0o200. The handler also refuses the
non-removal callbacks rmtree routes through it and never chmods through a
link or a junction.
- Escape the install directory name before globbing the staging root, since
it comes from UNSLOTH_LLAMA_CPP_PATH and a bracket in it would reach the
side paths of a different install holding a different lock.
for more information, see https://pre-commit.ci
Running as root, 0o500 does not stop shutil.rmtree from deleting the directory, so the pytest.raises fails and the finally block then raises FileNotFoundError on a directory that is already gone. Guard with the os.access check this file already uses for the read-only marker tests, and only restore the mode if the directory survived.
glob.escape neutralizes * ? and [ but does not anchor the trailing *, so the pattern for foo also matched the side paths of a co-located install literally named foo.rollback-something: its retained rollback copy, its failed tree and its live staging dir. A successful update of foo deleted all three while holding only foo's lock, and that copy can be the other install's last llama.cpp. The comment above this glob already stated the invariant and there is already a test for the bracket variant; this is the same invariant for the prefix variant. Checked by the tail shape rather than a per-install staging namespace, which would strand every tree a shipped version has already parked under .staging.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 copytree fallback is the first and only step in this recovery that needs free space: extraction into staging happens earlier, and everything before the copy renames or deletes. So an ENOSPC it hits is not implied by the activation error, and often is the only place the full disk shows up at all. The handler bound the recovery failure to a name the except clause then unbound, and the raise below chains from the activation error, so _causal_chain, which follows __cause__ ahead of __context__, never saw it. The helper exited EXIT_FALLBACK instead of EXIT_NO_SPACE and setup.sh began a source build needing far more room than the copy that just failed. Swap the cause only when the activation error is not already environment fatal and the recovery error is.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d804aa4a2
ℹ️ 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".
| superseded = prune_stale_install_side_paths( | ||
| install_dir, keep = (rollback_dir, failed_dir) | ||
| ) |
There was a problem hiding this comment.
Preserve older rollback until the new one is known good
When a failed restore copy leaves a partial install_dir and cleanup cannot remove it, the valid previous install remains in an older rollback path. On the next attempt, that partial tree is moved into the new rollback_dir; if activation and restoration fail again, this call deletes the older known-good rollback as “superseded” while retaining only the partial tree. Avoid pruning earlier rollbacks on this failure path unless the newly retained rollback has been validated, or defer pruning until a successful activation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct catch, and the most serious item on this PR since it is the exact failure this PR exists to prevent. Fixed in 77659e3. Confirmed there are two prunes, not one: the success-path prune runs only after confirm_install_tree passes, and its comment is right that this is the only point where a retained tree is provably not the last copy, but the failure-path prune is gated on keep_rollback, which is existence only. Nothing validated the tree being retained, and rollback_dir is just whatever sat at install_dir when the attempt started, which the failure path itself can leave unusable when the remove after a failed aside-move raises or the copytree restore dies partway and the cleanup of that directory fails for the same reason. Reproduced with real directories and a read-only subdirectory so rmtree genuinely fails, no mocking of the failure. Attempt one leaves a broken install_dir and a valid rollback; attempt two moves the broken tree aside and keeps it, and the prune reports removing one superseded tree. A and B on the same host, identical injection: before the fix the marked good bytes survive in 0 files, after it they survive in 5. The retention bound is unchanged: the keep set is still exactly one tree, so worst case retained bytes stay at one install rather than one per attempt; only which tree is exempt has changed, and when nothing confirms the behaviour is byte-identical to before. Suite 226 to 227 with an empty failure-set diff, and the fault-injection matrix still reports 0 strict and 0 weak over 2160 cells. Worth noting the matrix does not catch this class, structurally: each cell builds one fresh install and activates once, so it cannot express an older good rollback already existing in the staging root. The existing retention test also hand-repairs install_dir to a good tree at the top of every attempt, which is precisely the state the bug lives in.
The failure-path prune kept rollback_dir on the strength of it existing, and rollback_dir is only whatever sat at install_dir when the attempt started. An earlier failed update can leave an unusable tree there: the remove after a failed aside-move can raise, or the copytree restore can die partway, and the cleanup of that same directory fails for the same reason. The next attempt then moves that broken tree aside, keeps it, and deletes the last known-good install as superseded. Confirm the tree about to be retained, and fall back to the newest older rollback that confirms when it does not. The keep set is still exactly one tree, so the retention bound is unchanged; only which tree is exempt has changed. When nothing confirms the behaviour is identical to before.
|
@codex review |
1 similar comment
|
@codex review |

On filesystems where renaming the install directory fails (e.g. overlayfs in containers, where a lower-layer directory rename raises EXDEV), activate_install_tree failed at the rollback aside-move with the working install untouched then its error handler retried a rename of the same directory, failed again, and passed the still-good install to cleanup_install_side_paths, deleting it before falling back to a source build. Track whether the aside-move actually succeeded; when it didn't, report the failure (BusyInstallConflict for busy/lock errors, PrebuiltFallback otherwise) and leave the existing install in place. Adds regression tests for the EXDEV and EBUSY cases.