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 });