[sean0407]

Summary

A persistent venv can retain a dangling package-directory symlink after the Python runtime changes. When initialization then creates a runtime file link beneath that path, os.makedirs(..., exist_ok=True) raises FileExistsError instead of recovering.

Remove the parent symlink only when it is confirmed dangling, then create the directory and the current runtime file link. Fixes #4269.

Checks

• In a credential-isolated sandbox, the issue reproduction raised FileExistsError before the fix.
• In the same sandbox, cached_venv_replaces_dangling_package_symlink_test passed (1 test), verifying that the stale package link becomes a directory and the child link resolves to the current runfiles file.
• git diff --check passed.

[aignas]

I see the CI is failing in quite a few places, could you please check it?

[sean0407]

Thanks for flagging this. I checked Buildkite build #18004 and found a Buildifier formatting failure in the new BUILD target (tests/bootstrap_impls/BUILD.bazel); I reordered the attributes and pushed commit 3d94855 to fix that. The Ubuntu/RBE Bazel jobs in #18004 also failed, but their visible log output only reports that the Bazel build did not complete, without identifying the failing target. The new commit is now pushed, and I am checking its CI result before calling the remaining failures resolved.

[sean0407]

Correction to my update above: build #18005 still flagged the same Buildifier check, so my first attribute reorder was incomplete. I moved main after data to match the existing BUILD target ordering and pushed e6da0b7. Buildkite #18006 and the Read the Docs build for this commit are now running; I will report their results when available.

[sean0407]

Update: build #18006 no longer reports the Buildifier formatting annotation (the only current annotation is a Debian 11 end-of-life warning). The overall Buildkite check still fails: the visible failed jobs are Ubuntu/Bazel 9 and Debian/Bazel 8 and 9; their logs end with a generic Bazel build failure and do not identify a failing target. Read the Docs is still running. I have not treated the remaining jobs as passing.

[sean0407]

Thanks for asking me to check the CI. Buildkite #18006 exposed a Pyrefly error in the new regression test: Rlocation(...) has type str | None, so passing it directly to pathlib.Path is rejected. I added an explicit non-None assertion in b88ba42. Python syntax and the patch policy pass; the patch remains 3 files / 101 lines with no binary changes. Buildkite #18007, metadata checks, and Read the Docs are now queued or pending; I will share the result when they finish.

[sean0407]

Update: Buildkite #18007 passed in 11m 33s, Read the Docs succeeded, and both GitHub Actions checks passed. All four reported checks are green on b88ba42. The PR remains open and still awaits maintainer review.

[rickeylev]

Please change the PR description to abide by project guidelines.

[rickeylev]

Also: don't post a comment for every CI run. When you're confident CI is passing and code is fit for review, response PTAL ("Please take another look") or otherwise indicate you believe the code is ready for review again.

[sean0407]

PTAL — I updated the PR description to follow the project template and summarized the completed reproduction and checks.

[sean0407]

PTAL — I added the requested comment above the symlink check, explaining that a cached venv can leave this parent as a dangling link to an old EXTRACT_ROOT. This is comment-only; the implementation and regression test are unchanged. The prior Buildkite, Read the Docs, and GitHub Actions checks passed on b88ba42; GitHub has started checks for ad0a8b9.

[rickeylev]

I wonder if there's a similar issue in the script bootstrap? I don't recall the exact logic there.

Overall, it'd be nice if we could more automatically invalidate things somehow instead of having to manually check paths and recreate them. The main idea that comes to mind is doing something similar to what zipapp does: it hashes the zipfile and puts that hash as part of the extracted path. Maybe if we hashed the inputs to the create_venv() function and added that as a path prefix? Something to address another time, i think.