Skip to content

fix(lock): reap tickets close the stale-takeover race; one shared lock in _common.sh - #379

Draft
MendixMau wants to merge 6 commits into
masterfrom
fix/lock-reap-tickets
Draft

MendixMau wants to merge 6 commits into
masterfrom
fix/lock-reap-tickets

Conversation

@MendixMau

@MendixMau MendixMau commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Direct lane

What changed and why (one paragraph):
Before this change, a stale lock was taken over under one <lock>.takeover file. That file went stale by pid or by age, and it was moved aside and put back with ln. If a third contender created a new takeover file in that gap, the ln failed and two contenders were inside the takeover at once. One contender's compare-then-rm could then delete the other's fresh lock (r-eng-backlog item 1). An earlier fix that re-checked our own line just before the rm was rejected, because it only narrowed the window. This PR replaces the takeover file with reap tickets:

  • To reap a dead holder's line X, a contender creates <lock>.reap.<cksum X>.<N>. The ticket is written to <lock>.tmp.<pid> and then lned into place, so it is exclusive and never empty.
  • Ticket N+1 is allowed only when ticket N's pid is dead, never by age.
  • Only the holder of the highest ticket re-reads the lock and runs rm, and only if the lock still holds exactly X. It then creates its own lock exclusively.
  • A new holder deletes the tickets of every other key.
  • Lock lines gain a nonce= field, so a line can never come back. This is why the compare-then-rm cannot hit a fresh lock: while the lock holds X, only one live process can be past the ticket check for X.

The code now lives once, in project-bin/_common.sh, as mxtk_lock_take / mxtk_lock_release / mxtk_lock_live (+ mxtk_lock_fields, mxtk_lock_blocked_text). It is pure bash 3.2 with POSIX tools and no flock. It replaces exec.sh's exec_lock_* and test-scratch.sh's take_over/holder_live. The exec.sh diff is only the call sites plus the older-_common.sh guard (the app-restart.sh pattern), so it should not collide much with #355, the exec-on-copy PR or the baseline-lock PR.

Review fixes (second commit, 81981f2):

  1. An empty stale line is not unique. The old code, and the no-hard-link fallback, create the lock empty and write it afterwards. A fresh empty lock could therefore replace the stale empty one while a reaper sat between compare and rm, and the reaper would delete it. Now, before the rm, an empty X must still be more than a minute old (find -mmin +1). The new case in test-lock-reap.sh swaps in a fresh empty lock at the compare anchor. It fails on a677671 and passes now.
  2. Naming the blocker. When the take gives up (rc 2) because a live reaper's ticket, or a live old-style .takeover, never finished, MXTK_LOCK_BLOCKER names the pid and file. That covers a SIGSTOPped reaper, or a reused pid where ps -o args= cannot tell processes apart. mxtk_lock_blocked_text then says to resume the process (kill -CONT <pid> / fg) or end it (kill <pid>). exec.sh and test-scratch.sh print this on rc 2.
  3. mxtk_pid_args not used. Reusing mxtk_pid_args in the liveness check was left out on purpose. On Windows it runs PowerShell, about a second per call, inside a retry loop. On Linux it gives the same answer as ps -o args= for this match. So it does not come out clean.

Mixed versions:

  • The lock path, the line format (the nonce is a trailing field) and the liveness rule are unchanged, so old and new code each respect the other's live holders.
  • The new code also waits while an old-style .takeover is live, and clears a stale one once it holds the lock.
  • Neither version can deadlock the other: test-lock-reap.sh runs 9 old-block and 9 new contenders against one dead holder, and all 18 acquire.
  • An old and a new reaper of the same line are still not serialised against each other. That was already the case on master, and it lasts only while both versions are installed.

Test anchors: MXTK_LOCK_TEST_PAUSE names a command that runs as <cmd> <anchor> <pid> at four points: ticket-taken, compare, rm and created. When it is unset it does nothing. MXTK_LOCK_RETRY_SLEEP sets the pause between attempts (default 1 s).

Regression harness: tests/wave2/bench-lock-race.sh.

  • Each round starts 10 contenders together on a lock whose holder is dead.
  • Contenders pause 0–39 ms at every anchor, are SIGKILLed at ticket-taken/compare/rm (25% each), and are also SIGKILLed at random 0–50 ms offsets (10%).
  • Every holder checks two things: that no other live holder has a mark down, and that the lock file still names it.
  • After each round, a probe with no hooks must acquire the lock within 15 s.
  • --old runs master's block, the verbatim copy in fixtures/mxtest-guard/exec-lock-block.sh, identical to d16f578. Anchors are sed-injected at the same points, plus judged and aside (the old move-aside step).

Measured, Linux, 200 rounds × 10 contenders, same settings (first commit):

subject overlapping holders locks taken from a live holder starved probe failures SIGKILLs acquisitions
master's block (--old) 13 events in 6 rounds 12 0 0 664 1538
reap tickets 0 0 0 0 547 1653

The reap-ticket run left 0 files beside the lock at the end.

After the review fixes, 60 rounds × 10 contenders:

SIGKILL anchors overlaps stolen starved probe failures SIGKILLs acquisitions files left
default 0 0 0 0 164 496 0
all four (including created) 0 0 0 0 328 333 —

Fixtures:

  • tests/wave2/test-lock-reap.sh (45/45) covers each rule separately: the line format and nonce; release removes only our own line; a live holder refuses; every old and new line format is reaped; a live ticket dated 2000 still blocks (no age rule) and names its pid, file and remedy; dead tickets 1 and 2 lead to ticket 3; the line changing at the compare anchor; a higher ticket appearing stops the rm; a reaper SIGKILLed at rm does not block the next; GC; an empty lock; an empty lock swapped in mid-reap; legacy .takeover interop; mixed-version no-deadlock; and a 15-round harness run.
  • test-shared-lock-takeover.sh and test-test-scratch.sh now source _common.sh instead of awk-lifting the lock code.

Field runs

Platform Result New code, 200 × 10 --old (master's block) test-lock-reap Source
Linux PASS overlaps 0, stolen 0, starved 0, probe failures 0 13 overlaps in 6 rounds, 12 stolen 45/45 this PR (tables above)
macOS 26.6 arm64, bash 3.2.57 PASS overlaps 0, stolen 0, starved 0, probe failures 0 21 overlaps in 9 rounds, 18 stolen 45/45 issue #390
Git Bash (Windows) owed — — — —

Field evidence: no consuming-project run; this is concurrency code, and the proof is the harness. The macOS run covers APFS hard links, ps -o args= on a macOS zombie and fractional sleep. Still owed before merge: one Git Bash run of bash tests/wave2/bench-lock-race.sh (and --old) to cover NTFS hard links and kill -0 alone deciding liveness. That is why this PR stays a draft.

Merge with master (7f4169d): origin/master (24fe336) is merged in with a merge commit. The end-of-file conflict in project-bin/_common.sh with #370's mxtest helpers was resolved by keeping both sections. After that, c932fa1 clears MXTK_LOCK_BLOCKER at the start of every take attempt (review nit), so it describes only the last attempt.

  • No client data anywhere in the diff
  • Size cap: 9 files, about 400 non-fixture lines
  • Test tier: T1, fixtures plus the race harness on Linux and macOS. Fixtures run on c932fa1 (after the merge): test-lock-reap 45/45, test-shared-lock-takeover 11/11, test-test-scratch 52/52, test-exec-lock-watch 84/84, test-exec-change-class 21/21, test-bug07-08 43/43, test-app-restart 69/69; bench 30 rounds × 10: 0 overlaps, 0 stolen, 0 starved, 0 probe failures; check-portability clean; check-scripts 114/114
  • Both platforms: macOS PASS (Field runs 2026-10-15: #379 PASS, #384 PASS (sync --upgrade-bin not scoped; copy 87-134 s), #374 PASS (.bak in mprcontents) #390); Git Bash run still owed
  • CHANGELOG.md line appended, credited
  • Baseline words: 73,522 before and after on current master (no skill text changed)

Impact

Row Effect Evidence
Process None new. When a take gives up on a live reaper ticket (rc 2), exec.sh and test-scratch.sh now name the blocking pid/file and the resume-or-kill remedy; the same condition blocked before. Draft until the macOS + Git Bash bench runs exist. PR text
Dependencies Shared files: project-bin/_common.sh (new mxtk_lock_*), project-bin/exec.sh (call sites), project-bin/test-scratch.sh. The end-of-file conflict with master's mxtest-identity section in project-bin/_common.sh is resolved by merge 7f4169d (both sections kept; fixtures green on c932fa1). Also conflicts with #384 in _common.sh; exec.sh merges clean with #374 and #375. Platform: bash 3.2 + POSIX; relies on ln hard links, ps -o args=, find -mmin, fractional sleep; macOS + Git Bash runs owed. Reads only its own lock and ticket files. git merge-tree vs 868b3cb (CONFLICT in _common.sh) and pairwise
Context cost Unchanged: baseline 73,522 → 73,522 words (budget 80,000); Stage 5 mdl 30,185 → 30,185, lead 25,820 → 25,820 bin/render-routing.sh --check on master 868b3cb vs this head (81981f2) merged onto 868b3cb via git merge-tree in a temporary worktree (conflict markers left in project-bin/_common.sh, which render-routing does not read)
Behaviour n/a (scripts only). diff touches no skill or agent stub
Runtime Lock bench, 200 rounds × 10 contenders: 0 overlapping holders on the new code vs 13 on master's block (Linux) and vs 21 (macOS, #390). Uncontended take cost: NOT RUN (not measured). Contended retry pause MXTK_LOCK_RETRY_SLEEP default 1 s. tests/wave2/bench-lock-race.sh results in the PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw


Generated by Claude Code

…k in _common.sh

A dead holder's line X is now reaped under tickets <lock>.reap.<cksum X>.<N>,
written aside and ln'ed (exclusive, never empty). Ticket N+1 is allowed only
when ticket N's pid is dead, never by age. Only the highest ticket's holder
may rm X, and only while the lock still holds exactly X. A new holder clears
other keys' tickets. Lock lines gain a nonce= field. exec.sh and
test-scratch.sh call mxtk_lock_take/mxtk_lock_release; both stop with the
sync remedy on an older _common.sh.

bench-lock-race.sh, 200 rounds x 10 contenders, Linux:
  old block (master d16f578): 13 overlaps in 6 rounds, 12 stolen locks
  reap tickets: 0 overlaps, 0 stolen, 0 starved, 0 probe failures

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
…me a blocking reaper

1. An empty X is not unique: the old code (and the no-hard-link fallback)
   creates its lock empty and writes after, so a fresh empty lock could
   replace the stale one while a reaper sat between compare and rm. Before
   the rm, an empty X must still be over a minute old (find -mmin +1).
   test-lock-reap.sh swaps one in at the compare anchor; the case fails on
   a677671 and passes here.
2. When mxtk_lock_take gives up because a live reaper's ticket (or a live
   old-style .takeover) never finished, MXTK_LOCK_BLOCKER names the pid and
   file, and mxtk_lock_blocked_text says to resume (kill -CONT) or end it.
   exec.sh and test-scratch.sh print it on rc 2.

Bench, 60 rounds x 10 (Linux): default anchors 0 overlaps / 0 stolen /
0 starved / 0 probe failures; all four anchors the same.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
_common.sh: end-of-file conflict with the mxtest helpers (#370), both sections kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
Review nit on #379: it described a blocker from an earlier attempt when the
last attempt ended for another reason (a fresh empty lock being written).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
MendixMau added a commit that referenced this pull request Oct 15, 2026
sync-project.sh --only-bin <name> installs or refreshes just the named project-bin scripts, without running a full sync. It fixes the #390 field finding that --upgrade-bin ran a whole sync.

Review: independent review MERGE after one fix round.
- --only-bin scope probed on both layouts: only bin/<name> changes. A dry run, a path-like or unknown name, a NOINSTALL name, a glob and an empty name all write nothing.
- The full sync is byte-identical to master over 18 runs.
- All five fix-round findings verified: the refresh hold when _common.sh is edited, manifest validation, set -f, the empty-name usage error and the dry-run backup note.
- test-sync-only-bin 40/0 and test-bug12-sync 61/0. Portability, pr-discipline, leak guard and render-routing are clean.
- Follow-up: #379 switches its two plain --upgrade-bin _common.sh remedies to --only-bin.
Brings in #396 (sync-project.sh --only-bin). No conflicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
exec.sh and test-scratch.sh told users to run sync-project.sh with
--upgrade-bin _common.sh alone, which runs a full sync. They now give
--only-bin _common.sh --upgrade-bin _common.sh, matching test-scratch.sh's
mxtest guard on master. app-restart.sh is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJgWP5vEoAsNsJYqCDMGNw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants