[brettheap]

Part of #72: items 1 and 2, tests/run.sh's mkdir lock (the macOS path; Linux takes flock). Both were still as filed on today's main, aa14de3.

What was wrong

The mkdir lock names its holder by the pid inside it, and two things let that name lie.

Item 1: the lock was entered even when the holder could not record its pid. At aa14de3, tests/run.sh:390 read:

printf '%s\n' "$$" > "$LOCKDIR/pid" 2>/dev/null || :

A run whose pid did not get in (a full disk, a quota, a directory it cannot write) went on into the suite. Every waiter reads a lock with no readable pid as stale, claims it after three polls and runs beside it: two suites under one lock.

Item 2: cleanup was not ownership-safe. At aa14de3, cleanup (the EXIT trap) ran [ -n "$LOCKDIR_HELD" ] && rm -rf -- "$LOCKDIR_HELD" (tests/run.sh:242), which removes the PATH. A waiter that reads a live holder as gone can claim the directory and make it again with its own pid inside. Two ways that happens: a pid kill -0 cannot signal (another user's process), or a pid not yet written. The old holder's exit then deleted the new holder's lock, and that holder's suite ran on with no lock at all.

Evidence: the two new cases below fail on aa14de3's tests/run.sh, on exactly these two assertions.

What changed

• Item 1. After writing its pid, the holder reads it back the way a waiter reads it, with the same cat. If the pid is not there, the run releases the directory it made and exits 1 before the suite starts, naming the path. The shell's own reason is no longer hidden behind 2>/dev/null, so the refusal reads, for example:

  .../tests/run.sh: line 441: .../openrepotools-pytest.lock.d/pid: Is a directory
  run.sh: cannot record this run's pid in .../openrepotools-pytest.lock.d/pid, so a waiter would take the lock over while the suite ran — released, and the suite not started
  

• Item 2. cleanup calls a new release_lock, which removes the lock only while it still carries this run's pid:

  • A lock that carries another pid is not touched, not even renamed. A rename would leave the name free for an instant, and a waiter that polled in that instant would start beside its holder.
  • A lock that carries this run's pid is renamed to a name only this run uses ($LOCK.released.$$) and read again there, and only that private copy is removed. Once it is renamed, nothing another run does can change what is removed.
  • A copy that turns out not to be this run's (claimed and re-made in the instant between the read and the rename) is put straight back.
  • A leftover under the private name can only belong to a killed run whose pid this one now has. It is removed first, so the rename cannot land inside it.

  Two instants stay open: the one between the read and the rename, and the one before a copy is put back. bash 3.2 has no rename that refuses an existing target (mv onto a directory moves INTO it), so this is as close as it gets. The comment above release_lock says so.

• Unchanged: the waiters' staleness rule. "A LOCK WITH NO PID IN IT IS STALE, NOT ETERNAL" and the three-poll hold still stand; only the holder side changed. The flock path is untouched.

• Docs: AGENTS.md and docs/README-lanes.md say only that the wrapper "takes" the lock, never how it starts up or cleans up, so neither needed a change. tests/run.sh's header and its cleanup comment now state both rules.

Functions and regions edited

• tests/run.sh: header comment, the "flock IS LINUX-ONLY" paragraph (two sentences added)
• tests/run.sh: the "THE RUN ROOT AND ITS CLEANUP" comment (names release_lock)
• tests/run.sh: release_lock (new, just above cleanup)
• tests/run.sh: cleanup (its lock line)
• tests/run.sh: top level, the holder's pid write right after the while ! mkdir "$LOCKDIR" loop (the loop itself is unchanged)
• tests/test_run_wrapper.py: module docstring; import shlex; FAKE_SUITE (new FAKE_SUITE_TAKEOVER cue); FAKE_MKDIR (new)
• tests/test_run_wrapper.py: test_a_killed_run_removes_its_run_root_and_stops_the_suite (one assertion added in its mkdir-lock branch: no private copy left)
• tests/test_run_wrapper.py: test_a_run_that_cannot_record_its_pid_in_the_mkdir_lock_does_not_start (new)
• tests/test_run_wrapper.py: test_a_holder_whose_mkdir_lock_was_taken_over_leaves_the_new_lock (new)

Tests

Each case runs the wrapper on its mkdir path, with a private lock under the case's own temp TMPDIR and a stub suite. The workstation's real lock is never touched.

• Item 1: test_a_run_that_cannot_record_its_pid_in_the_mkdir_lock_does_not_start. A fake mkdir calls the real one, then puts a directory where the pid file goes, so the write fails with Is a directory. That is portable, and it fails for root too, where a directory root cannot write would not stop it. The case asserts:
  • the suite did not start;
  • the exit code is 1;
  • the message names <lock>/pid;
  • no lock directory, private copy or run root is left behind.
• Item 2: test_a_holder_whose_mkdir_lock_was_taken_over_leaves_the_new_lock. While the wrapper holds the lock, the stub suite claims the directory the way a waiter does (mv, then rm) and re-makes it with another live pid inside. The case asserts that the wrapper exits 0 and that the new holder's lock is still there with its pid, and nothing else.

Fail before, pass after. Run inside the worktree, on these cases only (no full suite):

env -u LANES_EDIT -u LANES_WORKSPACE_ROOT -u LANES_REPO -u LANES_LANE TMPDIR=<scratch>/tmp PYTHONPYCACHEPREFIX=<scratch>/pycache \
  python3 -m pytest tests/test_run_wrapper.py -k 'cannot_record_its_pid or was_taken_over or (killed_run and mkdir-lock)' -p no:cacheprovider -q

• With aa14de3's tests/run.sh (new tests only): 2 failed, 2 passed.
  • item 1 fails on AssertionError: the suite started with no pid in its lock;
  • item 2 fails on AssertionError: the old holder's exit removed the new holder's lock;
  • the two mkdir-lock killed-run cases pass; their added assertion is a guard, not a regression check.
• With this PR's tests/run.sh: 4 passed.
• The whole of tests/test_run_wrapper.py plus the hygiene pin test_repo_hygiene.py::test_the_suite_wrapper_takes_one_lock_and_names_it_where_agents_read_it: 15 passed in 16.94s.

release_lock alone. A scratch harness took the function out of tests/run.sh verbatim and drove each branch in a temp directory:

Lock state	Result
Carries this pid	removed, no copy left
Carries another pid	untouched; same inode, so never renamed
Changed between the read and the rename (a stub mv)	put back with the new pid
Stale private copy present	cleared, nothing nested
Item 1: no pid file	released
Item 1: another pid appeared	left alone
Lock already gone	no-op

bash -n tests/run.sh and python3 -m py_compile tests/test_run_wrapper.py are clean. No bash 4 constructs are used.

What remains of #72 after this PR and #218

• Item 8 waits for lane 4's skills/handoff/SKILL.md PR (Handoff skill: resolve lanes-edit.sh on PATH first, correct the comments that still promised write (c) without a workstation, and refuse the object-log and register-line writes for a lane that has never had a session recorded #207).
• Items 4 and 9 were kept as R-A11-11 by Brett Heap's ruling of 2026-10-11.
• Items 3, 7 and 10 are fixed or superseded, per lane 5's triage at f87fd5a.
• Items 5 and 6 are in A one-line record can't take a newline: append-line and lane-handoff's why refuse it before writing, and the deferred lane-start command keeps --agent (#85, #72 items 5, 6) #218.

Part of #72

🤖 Generated with Claude Code

Summary by Sourcery

Make the mkdir-based test lock refuse unsafe startup and preserve locks that have been taken over by another run.

Bug Fixes:

• Prevent the macOS mkdir-based test lock from starting a suite when its owner PID cannot be recorded.
• Make lock cleanup ownership-safe so an exiting run cannot remove a lock that has been taken over by another run.

Enhancements:

• Document the mkdir lock's PID ownership requirements and cleanup behavior.

Tests:

• Add coverage for failed PID recording, lock takeover during cleanup, and removal of private lock-release copies.

[sourcery-ai]

Reviewer's Guide

The PR hardens the macOS mkdir lock by refusing to enter the suite unless its PID is readable and by making EXIT cleanup ownership-aware, with race-focused tests and documentation while leaving the Linux flock path unchanged.

File-Level Changes

Change	Details	Files
Make the mkdir-based lock acquisition fail closed when the holder cannot persist its PID.	Removed the ignored PID-write failure. Read the PID back using the waiter-compatible mechanism and refuse to start if it is missing or incorrect. Release the newly created lock and report the lock PID path before exiting. Added a regression test ensuring the suite and run root are not created.	tests/run.sh tests/test_run_wrapper.py
Make mkdir-lock cleanup conditional on continued ownership to prevent deleting a replacement holder's lock.	Added release_lock to verify the current PID before cleanup. Rename a still-owned lock to a per-run private path, revalidate ownership, and remove only that private copy. Restore the lock if ownership changed during the handoff and clear stale private copies first. Added takeover and killed-run assertions covering lock preservation and cleanup.	tests/run.sh tests/test_run_wrapper.py
Document and test the ownership guarantees and failure behavior of the macOS mkdir locking path.	Updated wrapper comments and test documentation to describe PID recording and ownership-safe release. Added fake mkdir and suite takeover fixtures to exercise deterministic race scenarios. Kept the Linux flock path and waiter staleness policy unchanged.	tests/run.sh tests/test_run_wrapper.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sourcery-ai]

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 1 day and 23 hours by commenting @sourcery-ai review. Upgrade to get a review now.