mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 06:28:59 +02:00
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 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Fable 5.1
Claude Fable 5.1
parent
a8bc93eb2c
commit
8e1e520fe5
@@ -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
|
||||
|
||||
@@ -115,6 +115,7 @@ function runBlock(opts: {
|
||||
env?: Record<string, string>;
|
||||
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 });
|
||||
|
||||
Reference in New Issue
Block a user