Skip to content

fix: Handle symbolic linking race condition #3796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
scasagrande wants to merge 1 commit into
base: release/1.9
Choose a base branch
from
+4 −4
Open

fix: Handle symbolic linking race condition #3796

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions python/private/stage1_bootstrap_template.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ if [[ "$IS_ZIPFILE" == "1" ]]; then
fi
# The bin/ directory may not exist if it is empty.
mkdir -p "$(dirname $python_exe)"
ln -s "$symlink_to" "$python_exe"
ln -sfn "$symlink_to" "$python_exe"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using ln -sfn is not atomic and can introduce a different race condition.\n\nUnder the hood, ln -sf performs an unlink() followed by a symlink(). If multiple processes run this bootstrap script concurrently, one process might delete the symlink created by another process, leading to a transient window where the symlink does not exist. Any concurrent process attempting to execute or read the file during this window will fail with a No such file or directory error. Additionally, the -n flag is not standard POSIX and may behave differently or be unsupported on some platforms.\n\nA safer, fully atomic approach that avoids deleting existing symlinks is to attempt the creation with ln -s and fallback to verifying the symlink's existence if it fails due to already existing:\n\nbash\nln -s \"$symlink_to\" \"$python_exe\" 2>/dev/null || [[ -L \"$python_exe\" ]]\n\n\nSince symlink() is an atomic system call, this guarantees that the symlink is either created successfully or already exists, without ever unlinking it.

Suggested change
ln -sfn "$symlink_to" "$python_exe"
ln -s "$symlink_to" "$python_exe" 2>/dev/null || [[ -L "$python_exe" ]]

Copy link
Copy Markdown
Contributor Author

@scasagrande scasagrande May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta love seeing \n\n in a comment

elif [[ "$RECREATE_VENV_AT_RUNTIME" == "1" ]]; then
if [[ -n "$RULES_PYTHON_EXTRACT_ROOT" ]]; then
use_exec=1
Expand Down Expand Up @@ -226,16 +226,16 @@ EOF
fi

mkdir -p "$venv/bin"
ln -s "$python_exe_actual" "$python_exe"
ln -sfn "$python_exe_actual" "$python_exe"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

As mentioned above, ln -sfn is not atomic and can cause transient No such file or directory errors for concurrent processes due to the unlink and symlink sequence.\n\nUsing ln -s ... 2>/dev/null || [[ -L ... ]] provides an atomic alternative that avoids deleting the symlink if it already exists.

Suggested change
ln -sfn "$python_exe_actual" "$python_exe"
ln -s "$python_exe_actual" "$python_exe" 2>/dev/null || [[ -L "$python_exe" ]]


if [[ ! -e "$venv_site_packages" ]]; then
mkdir -p $(dirname $venv_site_packages)
ln -s "$runfiles_venv_site_packages" "$venv_site_packages"
ln -sfn "$runfiles_venv_site_packages" "$venv_site_packages"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using ln -sfn here can also cause race conditions under concurrent execution. We should use the atomic ln -s ... || [[ -L ... ]] pattern instead.

Suggested change
ln -sfn "$runfiles_venv_site_packages" "$venv_site_packages"
ln -s "$runfiles_venv_site_packages" "$venv_site_packages" 2>/dev/null || [[ -L "$venv_site_packages" ]]

fi
fi

if [[ ! -e "$venv/pyvenv.cfg" ]]; then
ln -s "$runfiles_venv/pyvenv.cfg" "$venv/pyvenv.cfg"
ln -sfn "$runfiles_venv/pyvenv.cfg" "$venv/pyvenv.cfg"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using ln -sfn here can also cause race conditions under concurrent execution. We should use the atomic ln -s ... || [[ -L ... ]] pattern instead.

Suggested change
ln -sfn "$runfiles_venv/pyvenv.cfg" "$venv/pyvenv.cfg"
ln -s "$runfiles_venv/pyvenv.cfg" "$venv/pyvenv.cfg" 2>/dev/null || [[ -L "$venv/pyvenv.cfg" ]]

Copy link
Copy Markdown
Collaborator

@aignas aignas May 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scasagrande, what about using the suggested approach? Because this just fails back if we cannot create the symlink. We also ensure that the existing file is a symlink with the AI proposed approach.

Could you please explain the usage of -fsn?

  • -s - symbolic link
  • -f - force replacement
  • -n - do not replace existing files, overriden by -f.

Copy link
Copy Markdown
Contributor Author

@scasagrande scasagrande May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take a closer look because I would want to better understand it.

But at the same time if no one else has had this problem, and I no longer have a problem, is there actually a problem that we're solving?

Either way if you prefer it I'll test it

Copy link
Copy Markdown
Contributor Author

@scasagrande scasagrande May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-n - do not replace existing files, overriden by -f.

no, -n is "if target is a symlink to a directory, do not follow it" to prevent your new symlink from being created within target dir, which is important when using -f to replace a symlink pointing to a directory

fi
else
use_exec=1
Expand Down