tests/run.sh refuses to start when it cannot record its lock's owner, and its cleanup removes only a lock that is still its own (#72 items 1-2)

tests/run.sh

@@ -40,7 +40,10 @@
 #
 # `flock` IS LINUX-ONLY. macOS ships none, so the fallback is `mkdir`, whose
 # atomicity POSIX guarantees; the holder's pid is left inside it so a lock a
-# killed run left behind is taken over rather than waited on for ever.
+# killed run left behind is taken over rather than waited on for ever. That
+# pid is the holder's whole claim (#72), so a run that cannot leave it there
+# does not start, and a run's exit removes the lock only while its pid is
+# still the one inside.
 #
 # NOTHING NEW LANDS BESIDE THE CODE (opensoft/openRepoTools#162, causes 3 and
 # 4). A suite run used to leave `tests/__pycache__`, `.pytest_cache` and a
@@ -145,9 +148,10 @@
 
 # THE RUN ROOT AND ITS CLEANUP. `cleanup` is the EXIT trap: it stops whatever
 # is left of the suite's PROCESS GROUP, then removes the run root and, on the
-# `mkdir` path, the lock it holds. The INT and TERM handlers stop the suite and
-# EXIT, so the EXIT trap runs - a handler that only cleaned up and returned
-# would release the lock while the suite carried on.
+# `mkdir` path, the lock it holds - while it still holds it (`release_lock`,
+# below). The INT and TERM handlers stop the suite and EXIT, so the EXIT trap
+# runs - a handler that only cleaned up and returned would release the lock
+# while the suite carried on.
 #
 # THE GROUP, NOT THE PID: pytest is waiting on a shell suite, a `git`, a
 # fixture's `sleep` when the signal comes, and a TERM to pytest alone leaves
@@ -236,10 +240,48 @@
   [ -z "$SS_PIDS" ] || kill -KILL $SS_PIDS 2>/dev/null || :
   return 0
 }
+# THE `mkdir` LOCK IS REMOVED ONLY WHILE IT IS STILL THIS RUN'S (#72 item 2).
+# A waiter that reads a live holder as gone - a pid `kill -0` cannot signal
+# (another user's), or one not yet written - claims the directory and makes
+# it again with its own pid inside, and an EXIT trap that removed the PATH
+# then deleted that new holder's lock, leaving its suite running under no lock
+# at all. So the lock is read before anything is done:
+#
+#   * A LOCK THAT CARRIES ANOTHER PID IS NOT TOUCHED - not even renamed, since
+#     a rename leaves the name free for an instant, and a waiter that polled in
+#     that instant would start beside its holder.
+#   * ONE THAT CARRIES THIS RUN'S IS RENAMED to a name only this run uses, read
+#     again THERE, and only that copy is removed. A read and then an `rm` of
+#     the path removes whatever is behind the path at the `rm`, which may have
+#     been claimed and made again since the read; once it is renamed, nothing
+#     another run does can change what is removed.
+#   * A COPY THAT IS NOT THIS RUN'S AFTER ALL - claimed and made again in the
+#     instant between the read and the rename - is put straight back.
+#
+# That instant, and the one before a copy is put back, are as close as bash 3.2
+# gets: it has no rename that refuses an existing target (`mv` onto a directory
+# moves INTO it), and the lock is a directory because `mkdir` is the atomic act
+# a shell has. A leftover under this run's private name can only be a killed
+# run's whose pid this one now has, and it goes first, so the rename cannot
+# land inside it.
+release_lock() {   # <lock directory> <the pid this run left in it, or '' for none>
+  rl_pid="$(cat "$1/pid" 2>/dev/null || printf '')"
+  [ "$rl_pid" = "$2" ] || return 0
+  rl_mine="$LOCK.released.$$"
+  rm -rf -- "$rl_mine"
+  mv -- "$1" "$rl_mine" 2>/dev/null || return 0
+  rl_pid="$(cat "$rl_mine/pid" 2>/dev/null || printf '')"
+  if [ "$rl_pid" = "$2" ]; then
+    rm -rf -- "$rl_mine"
+  else
+    mv -- "$rl_mine" "$1" 2>/dev/null || :
+  fi
+  return 0
+}
 cleanup() {
   stop_group
   [ -n "$RUN_ROOT" ] && rm -rf -- "$RUN_ROOT"
-  [ -n "$LOCKDIR_HELD" ] && rm -rf -- "$LOCKDIR_HELD"
+  [ -n "$LOCKDIR_HELD" ] && release_lock "$LOCKDIR_HELD" "$$"
   return 0
 }
 on_signal() {   # <exit code>
@@ -387,7 +429,22 @@
   printf '%s: waiting for the suite holding %s\n' "$prog" "$LOCKDIR" >&2
   sleep 20
 done
-printf '%s\n' "$$" > "$LOCKDIR/pid" 2>/dev/null || :
+# THE PID IS THE HOLDER'S WHOLE CLAIM, SO A RUN THAT CANNOT LEAVE IT DOES NOT
+# START (#72 item 1). The write was `|| :`, and a run whose pid did not get in -
+# a full disk, a quota, a directory it cannot write - went on into the suite
+# holding a lock every waiter reads as stale, claims after three polls and runs
+# beside. So the pid is read back the way a waiter reads it, with the same
+# `cat`, and where it is not there this run releases the directory it made and
+# exits before anything starts. Nothing hides the write's own error: the
+# shell's line saying why, where it gives one, comes just before the refusal.
+# A holder that is only SLOW to write is the waiters' rule above, unchanged.
+printf '%s\n' "$$" > "$LOCKDIR/pid"
+if [ "$(cat "$LOCKDIR/pid" 2>/dev/null || printf '')" != "$$" ]; then
+  release_lock "$LOCKDIR" ""
+  printf '%s: cannot record this run'"'"'s pid in %s, so a waiter would take the lock over while the suite ran — released, and the suite not started\n' \
+    "$prog" "$LOCKDIR/pid" >&2
+  exit 1
+fi
 # THE SIGNAL HANDLERS EXIT, and that is the whole point of writing them out
 # separately: a handler that only cleans up and RETURNS releases the lock while
 # this suite carries on running, which is the collision the lock exists for.

tests/test_run_wrapper.py

@@ -16,6 +16,10 @@
   * the repository's virtual environment is used when it has pytest, with the
     command line still reading `python3 -m pytest`.
 
+And what #72 asked of the `mkdir` lock itself: a run that cannot record its
+pid in the lock does not start, and a run's exit removes the lock only while
+that pid is still the one inside.
+
 THE SUITE ITSELF IS A STUB. A fake `python3` records how it was called and
 sleeps or exits on cue, and a fake `pgrep` reports no other run - the real one
 would see THIS suite's own `python3 -m pytest` and wait for ever.
@@ -24,6 +28,7 @@
 from __future__ import annotations
 
 import os
+import shlex
 import shutil
 import signal
 import subprocess
@@ -71,6 +76,16 @@
   printf '%s\n' "$!" > "$FAKE_SUITE_LOG.child"
   exit 0
 fi
+if [ -n "${FAKE_SUITE_TAKEOVER:-}" ]; then
+  # THE LOCK IS TAKEN OVER WHILE THIS RUN HOLDS IT (#72 item 2): claimed as a
+  # waiter that read this holder as gone claims it, and made again with that
+  # waiter's pid inside.
+  mv "$FAKE_SUITE_TAKEOVER" "$FAKE_SUITE_TAKEOVER.claimed" &&
+    rm -rf "$FAKE_SUITE_TAKEOVER.claimed" &&
+    mkdir "$FAKE_SUITE_TAKEOVER" &&
+    printf '%s\\n' "$FAKE_SUITE_OTHER" > "$FAKE_SUITE_TAKEOVER/pid" || exit 99
+  exit 0
+fi
 if [ -n "${FAKE_SUITE_SLEEP:-}" ]; then
   # A CHILD OF ITS OWN, as pytest waits on a shell suite or a `git`: the
   # wrapper must stop it too, not only the process it started.
@@ -85,6 +100,18 @@
 
 FAKE_PGREP = "#!/bin/sh\nexit 1\n"
 
+# THE REAL `mkdir`, and then - for the workstation's lock directory alone - a
+# DIRECTORY where the holder's pid file goes, so the wrapper's write of its pid
+# fails (#72 item 1) the same way on every platform and for every user, root
+# included, where a directory it cannot write would not stop root.
+FAKE_MKDIR = """#!/bin/sh
+REAL_MKDIR "$@" || exit
+case "$1" in
+  *.lock.d) REAL_MKDIR "$1/pid" || exit 99 ;;
+esac
+exit 0
+"""
+
 
 class Box:
     def __init__(self, root: Path, flock: bool = True):
@@ -221,6 +248,49 @@ def test_a_killed_run_removes_its_run_root_and_stops_the_suite(tmp_path, sig, rc
             raise AssertionError(f"{what} outlived its wrapper")
     if not flock:
         assert not (box.lockdir / "openrepotools-pytest.lock.d").exists(), "the lock was kept"
+        # NOR A COPY OF IT (#72 item 2): the lock is released by renaming it
+        # to a name only this run uses, and that copy goes too.
+        assert os.listdir(box.lockdir) == [], os.listdir(box.lockdir)
+
+
+def test_a_run_that_cannot_record_its_pid_in_the_mkdir_lock_does_not_start(tmp_path):
+    """#72 item 1: the holder's pid write was `|| :`, so a run that could not
+    write it entered the suite anyway - holding a lock every waiter reads as
+    stale, claims after three polls and runs beside. A pid that is not there
+    to be read is a refusal before the suite: the run releases the directory
+    it made and exits 1, naming the path."""
+    box = Box(tmp_path, flock=False)
+    fake = box.fakebin / "mkdir"
+    real = os.readlink(fake)
+    fake.unlink()
+    fake.write_text(FAKE_MKDIR.replace("REAL_MKDIR", shlex.quote(real)))
+    fake.chmod(0o755)
+    lock = box.lockdir / "openrepotools-pytest.lock.d"
+    proc = box.popen()
+    err = box.finish(proc)
+    assert not box.log.exists(), "the suite started with no pid in its lock"
+    assert proc.returncode == 1, err
+    assert f"cannot record this run's pid in {lock}/pid" in err, err
+    assert not lock.exists(), "a run that did not start kept the lock"
+    assert os.listdir(box.lockdir) == [], os.listdir(box.lockdir)
+    assert box.runs() == [], "a run that did not start made a run root"
+
+
+def test_a_holder_whose_mkdir_lock_was_taken_over_leaves_the_new_lock(tmp_path):
+    """#72 item 2: a waiter that reads a live holder as gone claims the lock
+    and makes it again with its own pid inside, and the old holder's EXIT
+    trap then removed the path - the NEW holder's lock. The trap removes the
+    lock only while it still carries this run's pid."""
+    box = Box(tmp_path, flock=False)
+    lock = box.lockdir / "openrepotools-pytest.lock.d"
+    other = str(os.getpid())                     # a live run that is not this one
+    proc = box.popen(FAKE_SUITE_TAKEOVER=str(lock), FAKE_SUITE_OTHER=other)
+    err = box.finish(proc)
+    assert proc.returncode == 0, err
+    assert lock.is_dir(), "the old holder's exit removed the new holder's lock"
+    assert (lock / "pid").read_text() == f"{other}\n"
+    assert os.listdir(box.lockdir) == [lock.name], os.listdir(box.lockdir)
+    assert box.runs() == [], "the run root outlived the run"
 
 
 @pytest.mark.skipif(not hasattr(os, "openpty") or sys.platform.startswith("win"),