mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 06:28:59 +02:00
fix(setup): Chromium-install lock reclaim is atomic and pid-validated; abandoned locks expire; the tree kill walks /proc without pgrep
- A pid file holding "", "-1" or "0" counted as a live holder (kill -0 -1 signals every process and succeeds), locking Chromium out for good. A pid must be a positive integer; anything else is stale. - Two setups judging the same lock stale raced on rm -rf + mkdir and the loser deleted the winner's fresh lock. The stale dir is renamed first (atomic), so exactly one reclaims. - A lock dir with no pid file (killed between mkdir and echo) was never reclaimed; it now expires once older than the install bound. - _kill_tree needed pgrep; debian-slim and git-bash ship none, so the bound killed only the wrapper subshell and the installer kept running. Without pgrep the children are found by walking /proc/*/stat. - The timeout knob is normalized in one place with one comment; the trap's exit 130 is the only exit the block may contain. 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
73506b59c6
commit
f1a580e1f0
@@ -569,6 +569,13 @@ _kill_tree() {
|
||||
for child in $(pgrep -P "$pid" 2>/dev/null); do
|
||||
_kill_tree "$child"
|
||||
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
|
||||
_kill_tree "$child"
|
||||
done
|
||||
fi
|
||||
kill -9 "$pid" 2>/dev/null || true
|
||||
}
|
||||
@@ -935,8 +942,9 @@ fi
|
||||
# setup is installing Chromium right now, so this run registers skills and
|
||||
# re-probes next time. The download is bounded (default 600s, env
|
||||
# GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT) because Playwright's own retries cover a
|
||||
# flaky socket but not a wedged bunx; the killed tree is the whole process
|
||||
# group via _kill_tree. Reason codes: skipped, chromium-install,
|
||||
# flaky socket but not a wedged bunx; a wedged installer is killed with its
|
||||
# child tree (_kill_tree: pgrep-walked, /proc-walked where pgrep is missing).
|
||||
# Reason codes: skipped, chromium-install,
|
||||
# chromium-install-timeout, chromium-install-locked, windows-no-node,
|
||||
# windows-node-modules, post-install-launch.
|
||||
# test/setup-playwright-best-effort.test.ts pins this block.
|
||||
@@ -947,13 +955,12 @@ _pw_fail() {
|
||||
echo " Chromium bootstrap: $code — $*" >&2
|
||||
}
|
||||
_PW_INSTALL_TIMEOUT="${GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT:-600}"
|
||||
# Positive seconds only: 0 would kill the install on the first poll, so it
|
||||
# (and anything non-numeric) falls back to the default.
|
||||
# Normalize to a plain positive integer or fall back to the default: empty and
|
||||
# non-numeric are garbage; "0"/"000" would kill the install on the first poll;
|
||||
# "0600" is 600; anything past nine digits is not a deadline (a value bash
|
||||
# cannot compare would leave the install unbounded — the exact failure the
|
||||
# bound exists to prevent).
|
||||
case "$_PW_INSTALL_TIMEOUT" in ''|*[!0-9]*) _PW_INSTALL_TIMEOUT=600 ;; esac
|
||||
# Normalize to a plain positive integer: "000"/"0" mean the default (never
|
||||
# kill-on-first-poll), "0600" is 600, and anything past nine digits is garbage
|
||||
# rather than a deadline (a value bash cannot compare would leave the install
|
||||
# unbounded — the exact failure the bound exists to prevent).
|
||||
[ "${#_PW_INSTALL_TIMEOUT}" -le 9 ] || _PW_INSTALL_TIMEOUT=600
|
||||
_PW_INSTALL_TIMEOUT=$((10#$_PW_INSTALL_TIMEOUT))
|
||||
[ "$_PW_INSTALL_TIMEOUT" -gt 0 ] || _PW_INSTALL_TIMEOUT=600
|
||||
@@ -972,10 +979,21 @@ elif ! ensure_playwright_browser; then
|
||||
# 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)
|
||||
if [ -n "$_PW_HOLDER" ] && ! kill -0 "$_PW_HOLDER" 2>/dev/null; then
|
||||
echo " reclaiming stale Chromium-install lock (holder pid $_PW_HOLDER is gone)" >&2
|
||||
rm -rf "$_PW_LOCK" 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
|
||||
# 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
|
||||
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
|
||||
@@ -2857,7 +2875,7 @@ fi
|
||||
|
||||
# ─── Chromium bootstrap summary (best-effort browser, see # 2) ───────────────
|
||||
# Printed LAST so it is the thing the user sees, after every skill registered.
|
||||
_PW_BROWSER_SKILLS="/qa, /qa-only, /design-review, /browse, make-pdf, /pair-agent"
|
||||
_PW_BROWSER_SKILLS="/qa, /qa-only, /design-review, /browse, make-pdf, /pair-agent, and any other skill that drives the browser"
|
||||
if [ "${_PW_FAIL_REASON:-}" = "skipped" ]; then
|
||||
# An explicit opt-out is not a failure: say what is unavailable and stop.
|
||||
log ""
|
||||
|
||||
@@ -77,7 +77,6 @@ describe('setup: Chromium bootstrap static invariants', () => {
|
||||
|
||||
test('lock contention is a reason code, not a fatal', () => {
|
||||
expect(codeLines).toContain('_pw_fail chromium-install-locked');
|
||||
expect(codeLines).not.toMatch(/another gstack setup is already installing[\s\S]*exit 1/);
|
||||
expect(block).toContain('GSTACK_SKIP_PLAYWRIGHT');
|
||||
});
|
||||
|
||||
@@ -115,6 +114,7 @@ function runBlock(opts: {
|
||||
bunx: string; // body of the bunx stub
|
||||
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
|
||||
markKill?: boolean; // record _kill_tree invocations to $MARK
|
||||
isWindows?: '0' | '1';
|
||||
prelude?: string; // extra shell lines (node/npm stubs) injected before the block
|
||||
@@ -131,6 +131,12 @@ function runBlock(opts: {
|
||||
fs.mkdirSync(lock);
|
||||
fs.writeFileSync(path.join(lock, 'pid'), opts.preLockPid);
|
||||
}
|
||||
if (opts.preLockNoPidAgeMin !== undefined) {
|
||||
const lock = path.join(tmp, 'gstack-playwright-install.lock');
|
||||
fs.mkdirSync(lock);
|
||||
const t = new Date(Date.now() - opts.preLockNoPidAgeMin * 60_000);
|
||||
fs.utimesSync(lock, t, t);
|
||||
}
|
||||
const probeFn = opts.probe === 'fail-then-ok'
|
||||
? 'ensure_playwright_browser() { if [ -f "$MARK.probed" ]; then return 0; fi; : > "$MARK.probed"; return 1; }'
|
||||
: `ensure_playwright_browser() { ${opts.probe === 'ok' ? 'return 0' : 'return 1'}; }`;
|
||||
@@ -196,7 +202,7 @@ describe('setup: Chromium bootstrap block executes best-effort', () => {
|
||||
expect(r.stdout).toContain('REACHED_END=1');
|
||||
expect(r.elapsedMs).toBeLessThan(20_000);
|
||||
expect(fs.readFileSync(path.join(r.tmp, 'mark'), 'utf-8')).toContain('killed');
|
||||
});
|
||||
}, 15_000);
|
||||
|
||||
test('non-numeric timeout knob falls back to the default instead of erroring', () => {
|
||||
const r = runBlock({ probe: 'fail', bunx: 'exit 3', env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: 'soon' } });
|
||||
@@ -216,12 +222,64 @@ describe('setup: Chromium bootstrap block executes best-effort', () => {
|
||||
});
|
||||
|
||||
test('stale lock (dead pid) is reclaimed and the install proceeds', () => {
|
||||
const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: '999999' });
|
||||
const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: '4194305' /* above Linux's largest pid_max: never a live pid */ });
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.stderr).toContain('reclaiming stale Chromium-install lock');
|
||||
expect(fs.readFileSync(path.join(r.tmp, 'mark'), 'utf-8')).toContain('bunx-called');
|
||||
});
|
||||
|
||||
test('a lock whose pid file is garbage (-1, abc, 0) is stale, not "locked": kill -0 -1 would signal everything and "succeed"', () => {
|
||||
for (const pid of ['-1', 'abc', '0']) {
|
||||
const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: pid });
|
||||
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');
|
||||
}
|
||||
}, 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);
|
||||
expect(fs.readFileSync(path.join(old.tmp, 'mark'), 'utf-8')).toContain('bunx-called');
|
||||
expect(old.stdout).not.toContain('chromium-install-locked');
|
||||
const fresh = runBlock({ probe: 'fail', bunx: 'exit 0', preLockNoPidAgeMin: 0, env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: '600' } });
|
||||
expect(fresh.status).toBe(0);
|
||||
expect(fresh.stdout).toContain('REASON=chromium-install-locked\n');
|
||||
}, 15_000);
|
||||
|
||||
test('_kill_tree without pgrep on PATH still kills the grandchild (walks /proc)', () => {
|
||||
if (!fs.existsSync('/proc')) return;
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-killtree-'));
|
||||
try {
|
||||
const bin = path.join(tmp, 'bin');
|
||||
fs.mkdirSync(bin);
|
||||
for (const name of ['bash', 'sleep', 'awk']) {
|
||||
const real = (spawnSync('which', [name], { encoding: 'utf-8', timeout: 10_000 }).stdout ?? '').trim();
|
||||
if (real) fs.symlinkSync(real, path.join(bin, name));
|
||||
}
|
||||
const script = [
|
||||
`export PATH="${bin}"`,
|
||||
'hash -r',
|
||||
'command -v pgrep >/dev/null 2>&1 && { echo "PGREP_PRESENT"; exit 0; }',
|
||||
extractFn('_kill_tree'),
|
||||
// two commands so bash forks a real subshell instead of exec-ing sleep directly
|
||||
'( sleep 30; true ) & pid=$!',
|
||||
'sleep 0.3',
|
||||
// find the sleep grandchild via /proc, the same way the fallback does
|
||||
'child=$(awk -v p="$pid" \'{ s=$0; sub(/^[^)]*\\) /, "", s); split(s, f, " "); if (f[2]==p) { print $1; exit } }\' /proc/[0-9]*/stat 2>/dev/null)',
|
||||
'[ -n "$child" ] || { echo "NO_CHILD"; exit 0; }',
|
||||
'_kill_tree "$pid"',
|
||||
'sleep 0.3',
|
||||
'if kill -0 "$child" 2>/dev/null; then echo "CHILD_ALIVE"; else echo "CHILD_DEAD"; fi',
|
||||
].join('\n');
|
||||
const r = spawnSync('/bin/bash', ['-c', script], { encoding: 'utf-8', timeout: 20_000 });
|
||||
expect(r.stdout).toContain('CHILD_DEAD');
|
||||
expect(r.stdout).not.toContain('PGREP_PRESENT');
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
test('install succeeds but the post-install probe fails: reason post-install-launch with the userns hint', () => {
|
||||
const r = runBlock({ probe: 'fail', bunx: 'exit 0' });
|
||||
expect(r.status).toBe(0);
|
||||
@@ -333,6 +391,7 @@ function runEmojiStep(reason: string, fontOk: boolean): { stdout: string; stderr
|
||||
'echo "REACHED_END=1"',
|
||||
].join('\n');
|
||||
const r = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 10_000 });
|
||||
if (/command not found/.test(r.stderr ?? '')) throw new Error(`harness drift (missing extracted helper):\n${r.stderr}`);
|
||||
return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1 };
|
||||
}
|
||||
|
||||
@@ -378,6 +437,8 @@ function runSummary(reason: string, telemetry: 'ok' | 'fail' | 'missing'): { std
|
||||
'echo "REACHED_END=1"',
|
||||
].join('\n');
|
||||
const r = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 10_000 });
|
||||
if (/command not found/.test(r.stderr ?? '')) throw new Error(`harness drift (missing extracted helper):\n${r.stderr}`);
|
||||
if (/command not found/.test(r.stderr ?? '')) throw new Error(`harness drift (missing extracted helper):\n${r.stderr}`);
|
||||
const argv = fs.existsSync(argvFile) ? fs.readFileSync(argvFile, 'utf-8') : '';
|
||||
return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1, argv };
|
||||
} finally {
|
||||
@@ -395,8 +456,8 @@ describe('setup: Chromium bootstrap summary block executes', () => {
|
||||
}
|
||||
expect(timeout.stdout).toContain('GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT=1800');
|
||||
expect(timeout.stdout).not.toContain('GSTACK_CHROMIUM_NO_SANDBOX');
|
||||
// Reason code only, never a path or command line; a synthetic session id
|
||||
// keeps the one-shot event from sweeping other sessions' pending markers.
|
||||
// Reason code only, never a path or command line; --no-sweep keeps the
|
||||
// one-shot event from finalizing other sessions' pending markers.
|
||||
expect(timeout.argv.trim()).toBe('--event-type onboarding --skill _setup_playwright --outcome chromium-install-timeout --no-sweep');
|
||||
|
||||
const launch = runSummary('post-install-launch', 'ok');
|
||||
@@ -483,17 +544,25 @@ function runLinker(opts: {
|
||||
extractFn('_claude_entry_is_ours'),
|
||||
extractFn('_write_owned_marker'),
|
||||
extractFn('_gstack_generated_header'),
|
||||
extractFn('_claude_entry_owned_strongly'),
|
||||
extractFn('_backup_skill_md'),
|
||||
extractFn('_cleanup_weak_dir'),
|
||||
extractFn('_gstack_dir_only_links'),
|
||||
extractFn('_cleanup_linked_dir'),
|
||||
'_BACKED_UP_SKILL_MDS=()',
|
||||
'_SKILL_BACKUP_ROOT="$HOME/.gstack/backups/skills/test"',
|
||||
extractFn('link_claude_skill_dirs'),
|
||||
extractFn('cleanup_old_claude_symlinks'),
|
||||
`link_claude_skill_dirs "${payload}" "${skills}"`,
|
||||
'echo "FOREIGN=${_FOREIGN_SKIPPED_ENTRIES[*]:-}"',
|
||||
].join('\n');
|
||||
const r = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 10_000, env: { PATH: process.env.PATH ?? '', HOME: tmp } });
|
||||
if (/command not found/.test(r.stderr ?? '')) throw new Error(`harness drift (missing extracted helper):\n${r.stderr}`);
|
||||
return { status: r.status ?? -1, stdout: r.stdout ?? '', stderr: r.stderr ?? '', skills, payload, tmp };
|
||||
}
|
||||
|
||||
describe.skipIf(process.platform === 'win32')('setup: .gstack-owned ownership marker for Windows copy installs (#2119)', () => {
|
||||
test('IS_WINDOWS=1 writes the marker beside a COPIED SKILL.md; IS_WINDOWS=0 writes no marker beside a symlinked one', () => {
|
||||
test('the marker is written beside a COPIED SKILL.md (IS_WINDOWS=1) and beside a symlinked one (IS_WINDOWS=0): a directory we created is ours on every platform', () => {
|
||||
const win = runLinker({ isWindows: '1', payload: ['qa', 'ship'] });
|
||||
try {
|
||||
expect(win.status).toBe(0);
|
||||
@@ -511,7 +580,7 @@ describe.skipIf(process.platform === 'win32')('setup: .gstack-owned ownership ma
|
||||
try {
|
||||
expect(unix.status).toBe(0);
|
||||
expect(fs.lstatSync(path.join(unix.skills, 'qa', 'SKILL.md')).isSymbolicLink()).toBe(true);
|
||||
expect(fs.existsSync(path.join(unix.skills, 'qa', '.gstack-owned'))).toBe(false);
|
||||
expect(fs.existsSync(path.join(unix.skills, 'qa', '.gstack-owned'))).toBe(true);
|
||||
} finally {
|
||||
fs.rmSync(unix.tmp, { recursive: true, force: true });
|
||||
}
|
||||
@@ -535,7 +604,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_generated_header'),
|
||||
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('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