fix python venv stub races when the same command runs in parallel

.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:

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: <root>/bin/<cmd> is the stub, <root>/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 })
+  }
+})

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,94 @@
 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 cfg_values(text):
+    return dict(l.split(' = ', 1) for l in text.splitlines() if ' = ' in l)
 
-          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 same_path(a, b):
+    return os.path.realpath(a) == os.path.realpath(b)
 
-# 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)
+# 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', '*'))
+
+
+# compare resolved paths so v3 vs v3.12.1 spellings of one interpreter don't churn
+def venv_is_current():
+    try:
+        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 stale_shebang(filepath):
+        return
+    with open(filepath, 'rb+') as f:
+        f.readline()
+        rest_of_file = f.read()
+        f.seek(0)
+        f.write(new_first_line + rest_of_file)
+        f.truncate()
+
+
+def rewrite_venv():
+    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 bin_files():
+        rewrite_shebang(filepath)
+
+    os.symlink(python_path, python_symlink)
+
 
-os.symlink(python_path, python_symlink)
+if not venv_is_current():
+    # 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)