From f826ef41025c829e6043cac412fec728328a7f32 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 22:12:46 +0000 Subject: [PATCH] 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) The `mkdir` lock (the macOS path; Linux has `flock`) names its holder by the pid inside it, and two things let that name lie. Item 1: the holder's pid write was `|| :`. A run whose pid did not get in went on into the suite holding a lock every waiter reads as stale, claims after three polls and runs beside. The pid is now read back the way a waiter reads it; where it is not there, the run releases the directory it made and exits 1 before the suite, naming the path. Item 2: the EXIT trap removed the lock's PATH. A waiter that read a live holder as gone could claim the directory and make it again with its own pid, and the old holder's exit then deleted the new holder's lock. `release_lock` now leaves a lock that carries another pid untouched (not even renamed), and removes this run's only after renaming it to a private name and reading the pid again there; a copy that turns out not to be ours is put back. The waiters' staleness rule is unchanged. Two cases in tests/test_run_wrapper.py fail before the change and pass after it, and the mkdir-lock killed-run case now also checks that no private copy is left behind. Part of #72. Co-Authored-By: Claude Opus 5.5 --- tests/run.sh | 69 ++++++++++++++++++++++++++++++++++---- tests/test_run_wrapper.py | 70 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 6 deletions(-) diff --git a/tests/run.sh b/tests/run.sh index 481e6995..316c37f5 100755 --- a/tests/run.sh +++ b/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 @@ wait_for_pytest() { # — block while any other suite is running # 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 @@ stop_strays() { [ -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() { # + 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() { # @@ -387,7 +429,22 @@ while ! mkdir "$LOCKDIR" 2>/dev/null; do 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. diff --git a/tests/test_run_wrapper.py b/tests/test_run_wrapper.py index a59b13af..6c96226d 100644 --- a/tests/test_run_wrapper.py +++ b/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"),