From 8e1e520fe5ea442b119c87706f3ad2dc557bf883 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 4 Sep 2026 18:14:31 +0000 Subject: [PATCH] fix(setup): lock reclaim hands a fresh lock back; a live holder past the bound is stale; /proc walk strips through the last paren - Reclaim renamed the lock by path after judging it stale, so a second setup that had already reclaimed and re-created it lost its fresh lock and two installers ran. After the rename the moved directory's pid is re-read: a new live holder, or a fresh lock whose pid is not written yet, is moved straight back. - A pid file whose process is alive but whose lock is older than the install bound is stale too (the holder is past its own deadline, or the pid was recycled to an unrelated long-lived process); it was locked forever. - The /proc fallback stripped the comm field to the FIRST ") ", so a comm containing ") " hid a child from the kill. proc(5) says the last paren. Co-Authored-By: Claude Fable 5.1 --- setup | 59 ++++++++++++++++------- test/setup-playwright-best-effort.test.ts | 15 +++++- 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/setup b/setup index 4d8e0448b..0be360073 100755 --- a/setup +++ b/setup @@ -593,9 +593,9 @@ _kill_tree() { done elif [ -d /proc ]; then # debian-slim and git-bash ship no pgrep: walk /proc for children. The - # comm field "(name)" may contain spaces, so strip through the closing - # paren before reading the ppid (second field after it). - for child in $(awk -v p="$pid" '{ s=$0; sub(/^[^)]*\) /, "", s); split(s, f, " "); if (f[2]==p) print $1 }' /proc/[0-9]*/stat 2>/dev/null); do + # comm field "(name)" may contain spaces and parens, so strip through the + # LAST closing paren (proc(5)) before reading the ppid (second field after it). + for child in $(awk -v p="$pid" '{ s=$0; sub(/^.*\) /, "", s); split(s, f, " "); if (f[2]==p) print $1 }' /proc/[0-9]*/stat 2>/dev/null); do _kill_tree "$child" done fi @@ -999,23 +999,48 @@ elif ! ensure_playwright_browser; then # Stale-lock self-heal: a SIGKILL'd prior setup leaves the lock dir behind # forever (mkdir mutexes have no owner). If the recorded holder PID is dead, # reclaim instead of telling the user to rmdir by hand. - if [ -d "$_PW_LOCK" ] && [ -f "$_PW_LOCK/pid" ]; then - _PW_HOLDER=$(cat "$_PW_LOCK/pid" 2>/dev/null || true) - # A pid must be a positive integer: "", "-1" (kill -0 -1 signals every - # process and "succeeds") or "0" (the process group) are stale, not live. - case "$_PW_HOLDER" in ''|*[!0-9]*) _PW_HOLDER=0 ;; esac - if [ "$_PW_HOLDER" -eq 0 ] || ! kill -0 "$_PW_HOLDER" 2>/dev/null; then - echo " reclaiming stale Chromium-install lock (holder pid ${_PW_HOLDER:-?} is gone)" >&2 + if [ -d "$_PW_LOCK" ]; then + _PW_STALE=0; _PW_HOLDER=0 + _PW_LOCK_OLD="" + [ -n "$(find "$_PW_LOCK" -maxdepth 0 -mmin +$(( _PW_INSTALL_TIMEOUT / 60 + 1 )) 2>/dev/null)" ] && _PW_LOCK_OLD=1 + if [ ! -f "$_PW_LOCK/pid" ]; then + # No holder recorded (killed between mkdir and echo, or mid-write by a + # live setup): nothing to probe, so only age can prove abandonment. + [ -n "$_PW_LOCK_OLD" ] && _PW_STALE=1 + else + _PW_HOLDER=$(cat "$_PW_LOCK/pid" 2>/dev/null || true) + # A pid must be a positive integer: "", "-1" (kill -0 -1 signals every + # process and "succeeds") or "0" (the process group) are stale, not live. + case "$_PW_HOLDER" in ''|*[!0-9]*) _PW_HOLDER=0 ;; esac + if [ "$_PW_HOLDER" -eq 0 ] || ! kill -0 "$_PW_HOLDER" 2>/dev/null; then + _PW_STALE=1 + elif [ -n "$_PW_LOCK_OLD" ]; then + # The holder is alive but the lock is older than the install bound: the + # holder is past its own deadline, or its pid was recycled to an + # unrelated long-lived process. Either way nobody is installing. + _PW_STALE=1 + fi + fi + if [ "$_PW_STALE" -eq 1 ]; then + echo " reclaiming stale Chromium-install lock (holder pid ${_PW_HOLDER:-?} is gone or past the install bound)" >&2 # Rename first: two setups judging the same lock stale race on rm -rf + # mkdir, and the loser would delete the winner's fresh lock. mv of a - # directory is atomic, so exactly one of them reclaims. - if mv "$_PW_LOCK" "$_PW_LOCK.stale.$$" 2>/dev/null; then rm -rf "$_PW_LOCK.stale.$$" 2>/dev/null || true; fi + # directory is atomic, so exactly one of them reclaims — and if the dir + # we moved already belongs to a NEW live holder (it re-created the lock + # between our judgment and our mv), hand it straight back. + if mv "$_PW_LOCK" "$_PW_LOCK.stale.$$" 2>/dev/null; then + _PW_MOVED_PID=$(cat "$_PW_LOCK.stale.$$/pid" 2>/dev/null || true) + case "$_PW_MOVED_PID" in ''|*[!0-9]*) _PW_MOVED_PID=0 ;; esac + if { [ "$_PW_MOVED_PID" -gt 0 ] && [ "$_PW_MOVED_PID" != "$_PW_HOLDER" ] && kill -0 "$_PW_MOVED_PID" 2>/dev/null; } \ + || { [ ! -f "$_PW_LOCK.stale.$$/pid" ] && [ -z "$_PW_LOCK_OLD" ]; }; then + # A new live holder, or a fresh lock whose holder has not written its + # pid yet (mkdir done, echo pending): not ours to reclaim. + mv "$_PW_LOCK.stale.$$" "$_PW_LOCK" 2>/dev/null || true + else + rm -rf "$_PW_LOCK.stale.$$" 2>/dev/null || true + fi + fi fi - elif [ -d "$_PW_LOCK" ] && [ -n "$(find "$_PW_LOCK" -maxdepth 0 -mmin +$(( _PW_INSTALL_TIMEOUT / 60 + 1 )) 2>/dev/null)" ]; then - # A lock with no pid file (killed between mkdir and echo) has no holder to - # probe; once it is older than the install bound nobody is using it. - echo " reclaiming abandoned Chromium-install lock (no holder recorded, older than the install bound)" >&2 - if mv "$_PW_LOCK" "$_PW_LOCK.stale.$$" 2>/dev/null; then rm -rf "$_PW_LOCK.stale.$$" 2>/dev/null || true; fi fi if mkdir "$_PW_LOCK" 2>/dev/null; then echo "$$" > "$_PW_LOCK/pid" 2>/dev/null || true diff --git a/test/setup-playwright-best-effort.test.ts b/test/setup-playwright-best-effort.test.ts index 895198b63..03fe52afc 100644 --- a/test/setup-playwright-best-effort.test.ts +++ b/test/setup-playwright-best-effort.test.ts @@ -115,6 +115,7 @@ function runBlock(opts: { env?: Record; preLockPid?: string; // pre-create the install lock held by this pid preLockNoPidAgeMin?: number; // pre-create a lock dir with NO pid file, this many minutes old + preLockAgeMin?: number; // with preLockPid: age the lock dir this many minutes markKill?: boolean; // record _kill_tree invocations to $MARK isWindows?: '0' | '1'; prelude?: string; // extra shell lines (node/npm stubs) injected before the block @@ -130,6 +131,10 @@ function runBlock(opts: { const lock = path.join(tmp, 'gstack-playwright-install.lock'); fs.mkdirSync(lock); fs.writeFileSync(path.join(lock, 'pid'), opts.preLockPid); + if (opts.preLockAgeMin !== undefined) { + const t = new Date(Date.now() - opts.preLockAgeMin * 60_000); + fs.utimesSync(lock, t, t); + } } if (opts.preLockNoPidAgeMin !== undefined) { const lock = path.join(tmp, 'gstack-playwright-install.lock'); @@ -237,6 +242,14 @@ describe('setup: Chromium bootstrap block executes best-effort', () => { } }, 15_000); + test('a lock held by a LIVE pid but older than the install bound is reclaimed (holder past its deadline, or a recycled pid)', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: String(process.pid), preLockAgeMin: 30, env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: '60' } }); + expect(r.status).toBe(0); + expect(fs.readFileSync(path.join(r.tmp, 'mark'), 'utf-8')).toContain('bunx-called'); + expect(r.stdout).not.toContain('chromium-install-locked'); + expect(r.stderr).toContain('past the install bound'); + }, 15_000); + test('a lock dir with NO pid file is reclaimed once older than the install bound, and honored while fresh', () => { const old = runBlock({ probe: 'fail', bunx: 'exit 0', preLockNoPidAgeMin: 30, env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: '60' } }); expect(old.status).toBe(0); @@ -604,7 +617,7 @@ describe.skipIf(process.platform === 'win32')('setup: .gstack-owned ownership ma fs.writeFileSync(path.join(r.skills, 'my-own', 'SKILL.md'), '---\nname: my-own\n---\n'); const flip = spawnSync('bash', ['-c', [ 'set -e', 'IS_WINDOWS=1', - extractFn('_gstack_link_target_abs'), extractFn('_gstack_target_is_ours'), extractFn('_gstack_dir_only_links'), extractFn('_cleanup_linked_dir'), extractFn('_gstack_generated_header'), extractFn('_cleanup_weak_dir'), + extractFn('_gstack_link_target_abs'), extractFn('_gstack_target_is_ours'), extractFn('_gstack_dir_only_links'), extractFn('_cleanup_linked_dir'), extractFn('_gstack_generated_header'), extractFn('_cleanup_weak_dir'), extractFn('_backup_skill_md'), '_BACKED_UP_SKILL_MDS=()', '_SKILL_BACKUP_ROOT="$HOME/.gstack/backups/skills/test"', extractFn('cleanup_old_claude_symlinks'), `cleanup_old_claude_symlinks "${r.payload}" "${r.skills}"`, ].join('\n')], { encoding: 'utf-8', timeout: 10_000 });