From effe14c9d91f3dee6af47255dddb724f614b587d Mon Sep 17 00:00:00 2001 From: Cristiano Nunes Date: Fri, 16 Oct 2026 19:09:52 -0300 Subject: [PATCH 1/2] fix python venv stub races when the same command runs in parallel Skip the virtualenv rewrite when it already points at the current interpreter, so concurrent pkgx runs do not clobber pyvenv.cfg, shebangs, or venv/bin/python. --- share/brewkit/python-venv-stub.py | 103 +++++++++++++++++++++++------- 1 file changed, 80 insertions(+), 23 deletions(-) diff --git a/share/brewkit/python-venv-stub.py b/share/brewkit/python-venv-stub.py index 812fb6aa..9b0b3ad1 100644 --- a/share/brewkit/python-venv-stub.py +++ b/share/brewkit/python-venv-stub.py @@ -1,8 +1,9 @@ #!/usr/bin/env python +import fcntl +import glob import os import sys -import glob # Determine directories and paths script_dir = os.path.dirname(os.path.realpath(__file__)) @@ -11,36 +12,92 @@ python_path = sys.executable python_home = os.path.dirname(python_path) -# Write pyvenv.cfg file +# Virtualenvs are not relocatable, so the stub still rewrites them when the +# interpreter moves. Concurrent runs of an already-correct venv must not write. pyvenv_cfg_path = os.path.join(virtual_env, 'pyvenv.cfg') -with open(pyvenv_cfg_path, 'w') as f: - f.write(f"home = {python_home}\ninclude-system-site-packages = false\nexecutable = {python_path}\n") +python_symlink = os.path.join(virtual_env, 'bin', 'python') +expected_cfg = ( + f"home = {python_home}\n" + "include-system-site-packages = false\n" + f"executable = {python_path}\n" +) +new_first_line = b"#!" + python_symlink.encode('utf-8') + b"\n" -new_first_line = b"#!" + os.path.join(virtual_env, 'bin', 'python').encode('utf-8') + b"\n" -# Go through files in the bin directory -for filepath in glob.glob(os.path.join(virtual_env, 'bin', '*')): - if os.path.isfile(filepath) and not os.path.islink(filepath): - with open(filepath, 'rb+') as f: - first_two_chars = f.read(2) +def venv_is_current(): + try: + with open(pyvenv_cfg_path, 'r') as f: + cfg = f.read() + except OSError: + return False + if cfg != expected_cfg: + return False + try: + return os.readlink(python_symlink) == python_path + except OSError: + return False - if first_two_chars == b'#!': - old_first_line = first_two_chars + f.readline() # Read the rest of the first line - if old_first_line.strip().endswith(b"python") and old_first_line != new_first_line: - rest_of_file = f.read() - f.seek(0) - f.write(new_first_line + rest_of_file) - f.truncate() +def replace_bytes(path, data, mode=None): + tmp = path + ".new" + try: + with open(tmp, 'wb') as f: + f.write(data) + if mode is not None: + os.chmod(tmp, mode) + os.replace(tmp, path) + finally: + if os.path.lexists(tmp): + os.remove(tmp) -# Create symlink to the specified Python version in the virtual environment -python_symlink = os.path.join(virtual_env, 'bin', 'python') -# Remove the symbolic link if it already exists -if os.path.islink(python_symlink) or os.path.exists(python_symlink): - os.remove(python_symlink) +def rewrite_shebang(filepath): + if not os.path.isfile(filepath) or os.path.islink(filepath): + return + with open(filepath, 'rb') as f: + first_two_chars = f.read(2) + if first_two_chars != b'#!': + return + old_first_line = first_two_chars + f.readline() + if not (old_first_line.strip().endswith(b"python") and old_first_line != new_first_line): + return + rest_of_file = f.read() + replace_bytes(filepath, new_first_line + rest_of_file, os.stat(filepath).st_mode) + + +def rewrite_venv(): + tmp_cfg = pyvenv_cfg_path + ".new" + try: + with open(tmp_cfg, 'w') as f: + f.write(expected_cfg) + os.replace(tmp_cfg, pyvenv_cfg_path) + finally: + if os.path.lexists(tmp_cfg): + os.remove(tmp_cfg) + + for filepath in glob.glob(os.path.join(virtual_env, 'bin', '*')): + rewrite_shebang(filepath) + + tmp_link = python_symlink + ".new" + try: + if os.path.lexists(tmp_link): + os.remove(tmp_link) + os.symlink(python_path, tmp_link) + os.replace(tmp_link, python_symlink) + finally: + if os.path.lexists(tmp_link): + os.remove(tmp_link) + -os.symlink(python_path, python_symlink) +if not venv_is_current(): + lock_path = os.path.join(virtual_env, '.pkgx-stub.lock') + with open(lock_path, 'a+') as lockf: + fcntl.flock(lockf, fcntl.LOCK_EX) + try: + if not venv_is_current(): + rewrite_venv() + finally: + fcntl.flock(lockf, fcntl.LOCK_UN) # Execute the corresponding script in the virtual environment arg0 = os.path.join(virtual_env, 'bin', arg0) From b32785eb3551e5471390148d234ba2e651da4065 Mon Sep 17 00:00:00 2001 From: Jacob Heider Date: Fri, 16 Oct 2026 18:29:55 -0400 Subject: [PATCH 2/2] hopefully, improvements --- .github/workflows/ci.cli.yml | 7 +- lib/porcelain/python-venv-stub.test.ts | 76 +++++++++++++++++ share/brewkit/python-venv-stub.py | 114 +++++++++++++------------ 3 files changed, 139 insertions(+), 58 deletions(-) create mode 100644 lib/porcelain/python-venv-stub.test.ts diff --git a/.github/workflows/ci.cli.yml b/.github/workflows/ci.cli.yml index 0c660dc8..6be58211 100644 --- a/.github/workflows/ci.cli.yml +++ b/.github/workflows/ci.cli.yml @@ -33,7 +33,10 @@ jobs: container: ${{ matrix.platform.img }} steps: # we need libc6-dev headers and gcc bits - - run: apt-get update && apt-get install -y build-essential + # bullseye is EOL: its security pool is gone from deb.debian.org + - run: | + sed -i 's/deb\.debian/archive\.debian/g' /etc/apt/sources.list + apt-get update && apt-get install -y build-essential if: matrix.platform.img != null - uses: actions/checkout@v4 - uses: pkgxdev/setup@v4 @@ -66,7 +69,7 @@ jobs: unset PKGX_PANTRY_DIR - uses: actions/checkout@v4 - uses: pkgxdev/dev@v1 - - run: deno test --allow-env --allow-net --ignore=.data + - run: deno test --allow-env --allow-net --allow-run --allow-read --allow-write --ignore=.data working-directory: lib unavailable: diff --git a/lib/porcelain/python-venv-stub.test.ts b/lib/porcelain/python-venv-stub.test.ts new file mode 100644 index 00000000..8bbe4ec1 --- /dev/null +++ b/lib/porcelain/python-venv-stub.test.ts @@ -0,0 +1,76 @@ +import { assert, assertEquals } from "deno/testing/asserts.ts" + +const stub = new URL("../../share/brewkit/python-venv-stub.py", import.meta.url).pathname + +// layout mirrors an installed pkg: /bin/ is the stub, /venv is the venv +async function fixture() { + const root = await Deno.realPath(await Deno.makeTempDir({ prefix: "venv-stub-" })) + await Deno.mkdir(`${root}/bin`) + await Deno.mkdir(`${root}/venv/bin`, { recursive: true }) + await Deno.copyFile(stub, `${root}/bin/tool`) + await Deno.chmod(`${root}/bin/tool`, 0o755) + // stale, as if the pkg were relocated or python moved + await Deno.writeTextFile(`${root}/venv/pyvenv.cfg`, "home = /nowhere\n") + await Deno.writeTextFile(`${root}/venv/bin/tool`, "#!/old/place/python\nimport sys\nprint('ok')\n") + await Deno.chmod(`${root}/venv/bin/tool`, 0o755) + await Deno.symlink("/old/place/python", `${root}/venv/bin/python`) + return root +} + +async function listing(root: string) { + const out: string[] = [] + for await (const e of Deno.readDir(`${root}/venv`)) out.push(e.name) + for await (const e of Deno.readDir(`${root}/venv/bin`)) out.push(`bin/${e.name}`) + return out.sort() +} + +Deno.test("parallel runs neither fail nor add files", async () => { + const root = await fixture() + try { + // the stub's own shebang is `env python`; run it with python3 explicitly + const run = () => new Deno.Command("python3", { args: [`${root}/bin/tool`], stdout: "piped", stderr: "piped" }).output() + const results = await Promise.all(Array.from({ length: 24 }, run)) + for (const r of results) { + assert(r.success, new TextDecoder().decode(r.stderr)) + assertEquals(new TextDecoder().decode(r.stdout), "ok\n") + } + // nothing may be added to the tree, ever + assertEquals(await listing(root), ["bin", "bin/python", "bin/tool", "pyvenv.cfg"]) + const shebang = (await Deno.readTextFile(`${root}/venv/bin/tool`)).split("\n")[0] + assertEquals(shebang, `#!${root}/venv/bin/python`) + } finally { + await Deno.remove(root, { recursive: true }) + } +}) + +Deno.test("current venv is not rewritten", async () => { + const root = await fixture() + try { + const run = () => new Deno.Command("python3", { args: [`${root}/bin/tool`] }).output() + await run() + const mtimes = async () => (await Promise.all( + ["pyvenv.cfg", "bin/tool"].map(f => Deno.stat(`${root}/venv/${f}`)))).map(x => x.mtime!.getTime()) + const before = await mtimes() + await new Promise(r => setTimeout(r, 50)) + await run() + assertEquals(await mtimes(), before) + } finally { + await Deno.remove(root, { recursive: true }) + } +}) + +// a relocated pkg keeps its interpreter and cfg but its shebangs point at the old venv path +Deno.test("stale shebang is fixed even when cfg and symlink are current", async () => { + const root = await fixture() + try { + const run = () => new Deno.Command("python3", { args: [`${root}/bin/tool`], stdout: "piped", stderr: "piped" }).output() + await run() + const tool = `${root}/venv/bin/tool` + await Deno.writeTextFile(tool, "#!/old/place/python\nprint('ok')\n") + const r = await run() + assert(r.success, new TextDecoder().decode(r.stderr)) + assertEquals((await Deno.readTextFile(tool)).split("\n")[0], `#!${root}/venv/bin/python`) + } finally { + await Deno.remove(root, { recursive: true }) + } +}) diff --git a/share/brewkit/python-venv-stub.py b/share/brewkit/python-venv-stub.py index 9b0b3ad1..8e9783e8 100644 --- a/share/brewkit/python-venv-stub.py +++ b/share/brewkit/python-venv-stub.py @@ -24,80 +24,82 @@ new_first_line = b"#!" + python_symlink.encode('utf-8') + b"\n" -def venv_is_current(): - try: - with open(pyvenv_cfg_path, 'r') as f: - cfg = f.read() - except OSError: - return False - if cfg != expected_cfg: - return False - try: - return os.readlink(python_symlink) == python_path - except OSError: +def cfg_values(text): + return dict(l.split(' = ', 1) for l in text.splitlines() if ' = ' in l) + + +def same_path(a, b): + return os.path.realpath(a) == os.path.realpath(b) + + +# a relocated pkg keeps its interpreter, but its shebangs name the old venv path +def stale_shebang(filepath): + if not os.path.isfile(filepath) or os.path.islink(filepath): return False + with open(filepath, 'rb') as f: + if f.read(2) != b'#!': + return False + first_line = b'#!' + f.readline() + return first_line.strip().endswith(b"python") and first_line != new_first_line + + +def bin_files(): + return glob.glob(os.path.join(virtual_env, 'bin', '*')) -def replace_bytes(path, data, mode=None): - tmp = path + ".new" +# compare resolved paths so v3 vs v3.12.1 spellings of one interpreter don't churn +def venv_is_current(): try: - with open(tmp, 'wb') as f: - f.write(data) - if mode is not None: - os.chmod(tmp, mode) - os.replace(tmp, path) - finally: - if os.path.lexists(tmp): - os.remove(tmp) + with open(pyvenv_cfg_path, 'r') as f: + cfg = cfg_values(f.read()) + return ( + same_path(cfg['home'], python_home) + and same_path(cfg['executable'], python_path) + and cfg.get('include-system-site-packages') == 'false' + and same_path(python_symlink, python_path) + and not any(stale_shebang(x) for x in bin_files()) + ) + except (OSError, KeyError): + return False +# Nothing is ever created beyond what a venv already holds, so everything is +# rewritten in place. That is safe because rewrites hold the lock and the +# symlink is removed first and recreated last: until it is back, every +# unlocked caller sees "not current" and waits on the lock. def rewrite_shebang(filepath): - if not os.path.isfile(filepath) or os.path.islink(filepath): + if not stale_shebang(filepath): return - with open(filepath, 'rb') as f: - first_two_chars = f.read(2) - if first_two_chars != b'#!': - return - old_first_line = first_two_chars + f.readline() - if not (old_first_line.strip().endswith(b"python") and old_first_line != new_first_line): - return + with open(filepath, 'rb+') as f: + f.readline() rest_of_file = f.read() - replace_bytes(filepath, new_first_line + rest_of_file, os.stat(filepath).st_mode) + f.seek(0) + f.write(new_first_line + rest_of_file) + f.truncate() def rewrite_venv(): - tmp_cfg = pyvenv_cfg_path + ".new" - try: - with open(tmp_cfg, 'w') as f: - f.write(expected_cfg) - os.replace(tmp_cfg, pyvenv_cfg_path) - finally: - if os.path.lexists(tmp_cfg): - os.remove(tmp_cfg) + if os.path.lexists(python_symlink): + os.remove(python_symlink) + + with open(pyvenv_cfg_path, 'w') as f: + f.write(expected_cfg) - for filepath in glob.glob(os.path.join(virtual_env, 'bin', '*')): + for filepath in bin_files(): rewrite_shebang(filepath) - tmp_link = python_symlink + ".new" - try: - if os.path.lexists(tmp_link): - os.remove(tmp_link) - os.symlink(python_path, tmp_link) - os.replace(tmp_link, python_symlink) - finally: - if os.path.lexists(tmp_link): - os.remove(tmp_link) + os.symlink(python_path, python_symlink) if not venv_is_current(): - lock_path = os.path.join(virtual_env, '.pkgx-stub.lock') - with open(lock_path, 'a+') as lockf: - fcntl.flock(lockf, fcntl.LOCK_EX) - try: - if not venv_is_current(): - rewrite_venv() - finally: - fcntl.flock(lockf, fcntl.LOCK_UN) + # lock the existing bin/ dir rather than a file: nothing is added to the tree + fd = os.open(os.path.join(virtual_env, 'bin'), os.O_RDONLY) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + if not venv_is_current(): + rewrite_venv() + finally: + os.close(fd) # Execute the corresponding script in the virtual environment arg0 = os.path.join(virtual_env, 'bin', arg0)