fix(cli): use os.replace to safely rebuild venv when hermes.exe is running (#37881) - #38797
Closed
ashishpatel26 wants to merge 1 commit into
Closed
fix(cli): use os.replace to safely rebuild venv when hermes.exe is running (#37881)#38797ashishpatel26 wants to merge 1 commit into
ashishpatel26 wants to merge 1 commit into
Conversation
…rch#37881) shutil.rmtree(ignore_errors=True) on a running hermes.exe venv deletes site-packages and certifi but silently fails on the locked python.exe, leaving a half-gutted venv that uv then refuses to overwrite — bricking the install with no recovery path. Fix: move the old venv aside atomically with os.replace before recreating. os.replace fails fast (without partial deletion) when the venv is in use, leaving the existing venv intact. If the rebuild fails, the backup is restored so Hermes never ends up with no venv.
Closed
2 tasks
Contributor
|
Thanks for this — the The underlying brick is already fixed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
shutil.rmtree(ignore_errors=True)inrebuild_venv()with an atomicos.replace()move-aside before recreating the venv.Root cause (fixes #37881)
When
hermes.exe/python.exeis running, Windows holds an exclusive lock onvenv\Scripts\python.exe.shutil.rmtree(ignore_errors=True)deletes everything it can (site-packages, certifi's cert bundle) but silently fails on the locked interpreter, leaving a half-gutted venv. The followinguv venvthen refuses to overwrite ("directory already exists") — bricking the install. Every later HTTPS call dies withFileNotFoundErrorfor the missing cert bundle. No automatic recovery: the venv must be rebuilt by hand.Fix
os.replace(venv_dir, backup)atomically renames the venv directory. On Windows, renaming a directory containing a running exe succeeds (only deletion fails withERROR_SHARING_VIOLATION). If the rename fails (venv is locked by an open handle on the dir itself),rebuild_venvaborts cleanly without touching the existing venv and tells the user to stop the gateway/desktop. If the rebuild fails, the backup is restored.Test plan
test_moves_old_venv_aside_and_creates_new— happy path, backup cleaned uptest_aborts_without_deleting_when_venv_in_use—os.replaceraises OSError →Falsereturned, existing venv intact,uv venvnever calledtest_restores_backup_on_rebuild_failure—uv venvexits non-zero → backup restored,Falsereturnedtest_rebuild_failure_returns_false— no existing venv,uv venvfails →False🤖 Generated with Claude Code