Fix: Windows: os.replace onto in-use cached binary raises PermissionError - #118
Open
pullapprove5[bot] wants to merge 1 commit into
Open
Fix: Windows: os.replace onto in-use cached binary raises PermissionError#118pullapprove5[bot] wants to merge 1 commit into
pullapprove5[bot] wants to merge 1 commit into
Conversation
…rror The finding is real: on Windows, os.replace(tmp_path, binary_path) in both plain-tailwind/plain/tailwind/core.py:173 and plain-code/plain/code/oxc.py:195 raises PermissionError (sharing violation) when a second checkout tries to install/download the same version binary while a first checkout is actively executing it from the shared machine-wide ~/.cache/plain cache. POSIX is unaffected because os.replace there succeeds even onto an open/executing file. I confirmed the exact behavior by exercising the real Tailwind.download() and OxcTool.download() code paths end-to-end (mocked network I/O, real os.replace monkeypatched to raise the actual Windows sharing-violation errno/message) with the destination binary pre-populated to simulate "another process already installed and is running it": on the pre-fix code this raised PermissionError; after the fix it succeeded and left the pre-existing binary in place. I fixed both call sites identically: wrap os.replace in try/except PermissionError, and only re-raise if the destination binary_path doesn't already exist (i.e. genuinely still missing, not just contended) — matching the deferred finding's suggested approach. Ran ./scripts/fix on both plain-tailwind and plain-code (after installing uv via pip since it wasn't preinstalled in this sandbox) — both passed clean with zero diff (ruff, oxlint/oxfmt, prettier all no-op). Could not run ./scripts/test: it requires a Postgres server via scripts/start-postgres, and docker isn't available in this sandbox; additionally neither plain-tailwind nor plain-code has a tests/ directory or appears in scripts/test's ALL_PACKAGES list, so the repo's own suite never covered this code before or after the change. One commit was made covering both fixes.
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.
The finding is real: on Windows, os.replace(tmp_path, binary_path) in both plain-tailwind/plain/tailwind/core.py:173 and plain-code/plain/code/oxc.py:195 raises PermissionError (sharing violation) when a second checkout tries to install/download the same version binary while a first checkout is actively executing it from the shared machine-wide ~/.cache/plain cache. POSIX is unaffected because os.replace there succeeds even onto an open/executing file. I confirmed the exact behavior by exercising the real Tailwind.download() and OxcTool.download() code paths end-to-end (mocked network I/O, real os.replace monkeypatched to raise the actual Windows sharing-violation errno/message) with the destination binary pre-populated to simulate "another process already installed and is running it": on the pre-fix code this raised PermissionError; after the fix it succeeded and left the pre-existing binary in place. I fixed both call sites identically: wrap os.replace in try/except PermissionError, and only re-raise if the destination binary_path doesn't already exist (i.e. genuinely still missing, not just contended) — matching the deferred finding's suggested approach. Ran ./scripts/fix on both plain-tailwind and plain-code (after installing uv via pip since it wasn't preinstalled in this sandbox) — both passed clean with zero diff (ruff, oxlint/oxfmt, prettier all no-op). Could not run ./scripts/test: it requires a Postgres server via scripts/start-postgres, and docker isn't available in this sandbox; additionally neither plain-tailwind nor plain-code has a tests/ directory or appears in scripts/test's ALL_PACKAGES list, so the repo's own suite never covered this code before or after the change. One commit was made covering both fixes.
Opened by a PullApprove implementation run (implement-finding v4) for:
Merging this is what closes them as fixed.