diff --git a/CHANGELOG.md b/CHANGELOG.md index ba97bc0e7..ec26c1988 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## [Unreleased] + +**Skills start at once on Windows again: the workflow lease lock no longer depends on symlinks (#3330).** `bin/gstack-lease` took its lock with `ln -s "" .lock`. In Git for Windows' default symlink mode (`MSYS` unset), `ln -s` cannot link to a target that does not exist, so the lock was never taken. Every skill start then waited out all 300 tries (0.1 s apart, plus process start costs) before `gstack-skill-start` printed anything, and still wrote no lease. One report measured 499 s per skill start. The error also blamed a "live process" with an empty pid. + +### Fixed +- **`bin/gstack-lease`**: the lock is now a `.lock` directory taken with `mkdir`, which is atomic on every host gstack runs on, with an `owner` file naming the holder's pid and start identity. A taker writes a claim (`.lock.claim.`) before its `mkdir` and renames it in as the owner after, so a lock left ownerless by a taker killed between the two steps is broken once every claimant is dead, never by age. A lock is removed by renaming it aside first, so nobody sees it half-removed. Stale-holder semantics are unchanged: a lock is broken only when its holder is dead or its pid now belongs to another process, breakers still serialize on `.lock.break` and reread before removing, and unlock removes only the caller's own lock. A symlink `.lock` left by an older gstack is still read and broken the same way, and it excludes the new lock at the same path, so mixed versions never both hold it. +- When the lock cannot be taken although nobody holds it, `touch` now fails after three quick retries with `gstack-lease: cannot take the lease directory lock ` instead of waiting out every try and naming an empty pid. `gstack-skill-start` stays fail-open, as before. +- `docs/capy-install.md` describes the directory lock. + +### Tests +- `test/gstack-lease-git-bash.test.ts` (new, in the curated windows-free-tests lane, launched through `lib/script-invocation.ts`, so Git Bash on `windows-latest`): `touch` with the default 300 tries writes its lease and leaves no lock; a host whose `ln -s` fails like Git Bash's still takes the lock (with a control that the shim really fails `ln -s`); a lock held by a live Git Bash process is kept and named, and a dead holder's lock is broken. The last two fail against the old bin. +- `test/gstack-lease.test.ts`: the dead-holder, live-holder and dead-breaker cases run against both the directory lock and the legacy symlink lock. The race cases inject at `mkdir` instead of `ln`. New cases: a lock held by a running gstack-lease excludes a second taker, which names it; 16 parallel touches all land and leave nothing behind; an ownerless lock is kept while a claimant lives and broken once none does; a lock that cannot be taken while nobody holds it fails in under 10 s. Seven of these fail against the old bin. + +### Gate integrity +- No threshold, budget, timeout, trial count, case kind or assertion was weakened. Three existing race tests moved their fault injection from `ln` to `mkdir`, because the lock no longer calls `ln`. Their assertions are unchanged. + ## [1.91.221.0] - 2026-10-25 **The session-start updater no longer skips because of its own session's workflow lease, and the autostash tests stop failing their first pass in Free Tests.** Since #3385, `gstack-session-update` skips the fast-forward while another session holds a workflow lease, and it means to ignore the session that started it (`--exclude-ancestry-of`). The check runs in the forked background updater, though, and passed the hook's own `$$`, which has exited by then, so the ancestry walk excluded nothing. A Claude Code process that had already run a skill (and so held a lease) and then started a session again in the same process (`/clear`, resume or compaction) logged `SKIP leased pid=` and never updated. `test/session-update-autostash.test.ts` hit the same gap plus a fixture leak: its first two fixtures spawned the updater with the whole parent environment, so it read the test preload's `GSTACK_LEASE_DIR`, which every test file in the same Bun process shares. A lease another file left there gave "Received: SKIP leased pid=3196/1m" where UPDATING or PULL_FAILED was expected, in 13 Free Tests runs since #3385 merged (each passed on the serial retry). diff --git a/bin/gstack-lease b/bin/gstack-lease index ded79313e..d7efe9e40 100755 --- a/bin/gstack-lease +++ b/bin/gstack-lease @@ -31,7 +31,7 @@ # runtime root's .source-path, else ../). A root that is not a git checkout # exits 2 and writes nothing. GSTACK_LEASE_DIR overrides the lease directory # (hermetic tests; a dev checkout's own tests must not lease it). Bash 3.2 -# plus git, ps, find, ln, readlink. +# plus git, ps, find, mkdir, mv, readlink. set -u BIN="$(cd "$(dirname "$0")" && pwd -P)" # shellcheck source=bin/gstack-install-verdict.sh @@ -99,43 +99,97 @@ state() { } # The lease directory's lock: touch's publish and prune, and reclaim's # reread-and-delete, run under it, so a reclaim never deletes a renewal and a -# lease never disappears while it is live. The lock is a symlink whose target -# names its holder (` `): ln -s creates it and its owner -# in one atomic step. It is broken only when that holder is dead or its pid -# now belongs to another process, never by age, and unlock removes only the -# caller's own lock. A live holder makes lock() give up after -# GSTACK_LEASE_LOCK_TRIES tries 0.1 s apart (default 300). +# lease never disappears while it is live. The lock is a directory, .lock, +# whose owner file names its holder (` `); mkdir is +# atomic everywhere gstack runs, Git Bash on Windows included, where ln -s to a +# target that does not exist always fails (#3330). A taker publishes its token +# as a claim (.lock.claim.) before its mkdir and renames the claim into +# the lock as its owner after, so a lock without an owner is either being +# taken by a live claimant or orphaned by a dead one. A lock is broken only +# when its holder (or, ownerless, every claimant) is dead or its pid now +# belongs to another process, never by age, and unlock removes only the +# caller's own lock. A symlink .lock left by an older gstack names its holder +# the same way and excludes a directory lock at the same path. A live holder +# makes lock() give up after GSTACK_LEASE_LOCK_TRIES tries 0.1 s apart +# (default 300). LOCK="$DIR/.lock" TOKEN="" +# take PATH — create the lock PATH owned by TOKEN; 1 when it exists. +take() { + printf '%s\n' "$TOKEN" > "$1.claim.$$" || return 1 + if mkdir "$1" 2>/dev/null; then + mv -f "$1.claim.$$" "$1/owner" && return 0 + rm -rf "$1" + fi + rm -f "$1.claim.$$" + return 1 +} +# owner_of PATH — the token the lock PATH names, empty when it names none. +owner_of() { if [ -L "$1" ]; then readlink "$1" 2>/dev/null; else head -1 "$1/owner" 2>/dev/null; fi; } +# orphaned PATH — PATH is a lock directory without an owner and no live +# claimant. Claims are read before the owner, so a taker that renames its +# claim in between is seen as the owner. An empty claim is a taker that has +# not reached its mkdir, so it never made this lock. +orphaned() { + [ -d "$1" ] && [ ! -L "$1" ] || return 1 + for _c in "$1".claim.*; do + _t=$(head -1 "$_c" 2>/dev/null) + [ -n "$_t" ] && ! holder_gone "$_t" && return 1 + done + [ ! -e "$1/owner" ] +} +# drop PATH — remove a lock in one step: a directory is renamed aside first, +# so nobody sees it ownerless on the way out. +drop() { + if [ -L "$1" ]; then rm -f "$1"; return; fi + mv "$1" "$1.gone.$$" 2>/dev/null && rm -rf "$1.gone.$$" +} lock() { [ -n "$TOKEN" ] || TOKEN="$$ $(identity $$)" - _tries=0 - until ln -s "$TOKEN" "$LOCK" 2>/dev/null; do - _tries=$((_tries + 1)) - LOCK_OWNER=$(readlink "$LOCK" 2>/dev/null) - [ -n "$LOCK_OWNER" ] && holder_gone "$LOCK_OWNER" && break_lock "$LOCK_OWNER" && continue + _tries=0 _gone=0 + until take "$LOCK"; do + LOCK_OWNER=$(owner_of "$LOCK") + if [ ! -e "$LOCK" ] && [ ! -L "$LOCK" ]; then + # Released since take, or take itself cannot work here: never wait out + # the tries for a lock nobody holds. + _gone=$((_gone + 1)) + [ "$_gone" -ge 3 ] && return 1 + continue + fi + _gone=0 _tries=$((_tries + 1)) + if [ -n "$LOCK_OWNER" ]; then holder_gone "$LOCK_OWNER" && break_lock "$LOCK_OWNER" && continue + elif orphaned "$LOCK"; then break_lock "" && continue + fi [ "$_tries" -ge "${GSTACK_LEASE_LOCK_TRIES:-300}" ] && return 1 sleep 0.1 done } holder_gone() { case "$(state "${1%% *}" "${1#* }" 0)" in dead|stale) return 0 ;; esac; return 1; } -# break_lock OWNER — remove the lock only if it still names OWNER. Breakers -# take a second symlink, .lock.break, and reread the lock under it, so a lock -# taken since the verdict is never removed; a .lock.break whose holder is gone -# is cleared. Returns 1 when another breaker holds .lock.break. Known limit: -# that clear is not atomic, so two gstack-lease processes killed in a row (one -# holding .lock, the next inside this function) can let a reclaim delete a -# renewal; one crash is always safe. +# break_lock OWNER — remove the lock only if it still names OWNER (empty: is +# still orphaned). Breakers take a second lock, .lock.break, and reread the +# lock under it, so a lock taken since the verdict is never removed; a +# .lock.break whose holder is gone is cleared. Returns 1 when another breaker +# holds .lock.break. Known limit: that clear is not atomic, so two +# gstack-lease processes killed in a row (one holding .lock, the next inside +# this function) can let a reclaim delete a renewal; one crash is always safe. break_lock() { - if ! ln -s "$TOKEN" "$LOCK.break" 2>/dev/null; then - _b=$(readlink "$LOCK.break" 2>/dev/null) - [ -n "$_b" ] && holder_gone "$_b" && [ "$(readlink "$LOCK.break" 2>/dev/null)" = "$_b" ] && rm -f "$LOCK.break" + if ! take "$LOCK.break"; then + _b=$(owner_of "$LOCK.break") + if [ -n "$_b" ]; then + holder_gone "$_b" && [ "$(owner_of "$LOCK.break")" = "$_b" ] && drop "$LOCK.break" + else + orphaned "$LOCK.break" && drop "$LOCK.break" + fi return 1 fi - [ "$(readlink "$LOCK" 2>/dev/null)" = "$1" ] && rm -f "$LOCK" - rm -f "$LOCK.break" + if [ -n "$1" ]; then + [ "$(owner_of "$LOCK")" = "$1" ] && drop "$LOCK" + else + orphaned "$LOCK" && drop "$LOCK" + fi + drop "$LOCK.break" return 0 } -unlock() { [ "$(readlink "$LOCK" 2>/dev/null)" = "$TOKEN" ] && rm -f "$LOCK"; return 0; } +unlock() { [ "$(owner_of "$LOCK")" = "$TOKEN" ] && { drop "$LOCK" || rm -rf "$LOCK"; }; return 0; } # reclaim FILE RECORD — delete a stale lease only if, under the lock, it still # holds the record judged stale; a renewal since then keeps it. reclaim() { @@ -153,7 +207,12 @@ case "$VERB" in touch) [ -n "$PID" ] || PID=$(gstack_harness_pid "${FROM:-$PPID}") mkdir -p "$DIR" || exit 2 - lock || { echo "gstack-lease: the lease directory is locked by live process ${LOCK_OWNER%% *}; lease $PID not renewed" >&2; exit 3; } + lock || { + if [ -n "${LOCK_OWNER:-}" ]; then echo "gstack-lease: the lease directory is locked by live process ${LOCK_OWNER%% *}; lease $PID not renewed" >&2 + else echo "gstack-lease: cannot take the lease directory lock $LOCK; lease $PID not renewed" >&2 + fi + exit 3 + } _tmp="$DIR/.$PID.$$" identity "$PID" > "$_tmp" && mv -f "$_tmp" "$DIR/$PID" || { rm -f "$_tmp"; unlock; exit 2; } find "$DIR" -mmin +120 -type f -exec rm -f {} + 2>/dev/null diff --git a/docs/capy-install.md b/docs/capy-install.md index c2d0ce97b..4ab9e65e3 100644 --- a/docs/capy-install.md +++ b/docs/capy-install.md @@ -120,11 +120,16 @@ lease …`). Stale needs proof: two well-formed identities from the same backend that differ (`ps` is read under `LC_ALL=C TZ=UTC0`). An identity that cannot be read, a malformed record or a backend change keeps the lease. `touch` (publish by rename, prune) and reclaim (reread, then delete only the record judged -stale) run under one lock in the lease directory, a symlink naming its holder's -pid and start identity, so a renewal is never deleted and a live lease never -disappears from a concurrent scan. The lock is broken only when its holder is -dead or its pid was reused, never by age, and breakers serialize on a second -symlink and reread the lock before removing it. One killed gstack-lease process +stale) run under one lock in the lease directory, a `.lock` directory whose +`owner` file names its holder's pid and start identity, so a renewal is never +deleted and a live lease never disappears from a concurrent scan. The lock is a +directory because `mkdir` is atomic on every host, while Git Bash on Windows +cannot `ln -s` to a target that does not exist (#3330). A taker publishes a +claim before its `mkdir` and renames it in as the owner after, so an ownerless +lock is broken only once every claimant is dead. The lock is broken only when +its holder is dead or its pid was reused, never by age, and breakers serialize +on a second lock and reread the lock before removing it. A symlink `.lock` left +by an older gstack is honored the same way. One killed gstack-lease process is always safe; two killed in a row, one holding the lock and the next while breaking it, can still let a reclaim delete a renewal. While a live process holds it, `touch` exits 3 without writing (`gstack-skill-start` reports that the diff --git a/test/gstack-lease-git-bash.test.ts b/test/gstack-lease-git-bash.test.ts new file mode 100644 index 000000000..c174c0e81 --- /dev/null +++ b/test/gstack-lease-git-bash.test.ts @@ -0,0 +1,86 @@ +/** + * #3330: gstack-lease took its lock with `ln -s .lock`. Git + * for Windows' default symlink mode (MSYS unset) cannot link to a target that + * does not exist, so the lock was never taken and every skill start waited out + * 300 tries (one report: 499 s) and still wrote no lease. The lock is now a + * directory (mkdir) with an owner file. test/gstack-lease.test.ts pins the full + * contract but is POSIX-bound (mode bits, sleep children), so this file is the + * one the curated windows-free-tests lane runs: it launches the bin through + * lib/script-invocation.ts, which is Git Bash on windows-latest. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { BIN_DIR, runBashSync, runScriptSync } from '../lib/script-invocation'; + +const LEASE = path.join(BIN_DIR, 'gstack-lease'); +const fwd = (p: string) => p.replace(/\\/g, '/'); +const dirs: string[] = []; +afterEach(() => { + for (const d of dirs.splice(0)) fs.rmSync(d, { recursive: true, force: true }); +}); + +// No inherited lease dir from the test preload, and Git Bash's default symlink mode (no winsymlinks). +const { GSTACK_LEASE_DIR: _preload, MSYS: _msys, CYGWIN: _cygwin, GSTACK_LEASE_LOCK_TRIES: _tries, ...baseEnv } = process.env as Record; + +function checkout() { + const dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-lease-gb-'))); + dirs.push(dir); + const r = spawnSync('git', ['init', '-q', dir], { encoding: 'utf8', timeout: 30_000 }); + if (r.status !== 0) throw new Error(r.stderr); + return { dir, leases: path.join(dir, '.git', 'gstack-leases') }; +} + +/** A BASH_ENV file in which `ln -s` behaves as Git Bash's default: linking to a missing target fails. */ +function gitBashLn(dir: string): string { + const file = path.join(dir, 'git-bash-ln.sh'); + fs.writeFileSync(file, 'ln() { if [ "$1" = -s ] && [ ! -e "$2" ]; then echo "ln: failed to create symbolic link \'$3\': No such file or directory" >&2; return 1; fi; command ln "$@"; }\n'); + return file; +} + +describe('gstack-lease under Git Bash semantics (#3330)', () => { + test('touch takes the lock and writes the lease at once with the default tries, leaving no lock behind', () => { + const { dir, leases } = checkout(); + const started = Date.now(); + const r = runScriptSync(LEASE, ['touch', '--root', fwd(dir), '--pid', '4242'], { encoding: 'utf8', env: baseEnv, timeout: 120_000 }); + expect(r.status, `${r.error ?? ''}${r.stdout}${r.stderr}`).toBe(0); + expect(Date.now() - started).toBeLessThan(25_000); + expect(fs.readdirSync(leases)).toEqual(['4242']); + }); + + test('a host whose ln -s cannot link to a missing target still takes the lock (negative control: the shim really fails ln -s)', () => { + const { dir, leases } = checkout(); + const shim = gitBashLn(dir); + const probe = runBashSync('ln -s "1 2" "$PROBE"', { encoding: 'utf8', env: { ...baseEnv, BASH_ENV: fwd(shim), PROBE: fwd(path.join(dir, 'probe')) }, timeout: 30_000 }); + expect(probe.status, `${probe.error ?? ''}${probe.stderr}`).not.toBe(0); + expect(probe.stderr).toContain('failed to create symbolic link'); + const r = runScriptSync(LEASE, ['touch', '--root', fwd(dir), '--pid', '4243'], { encoding: 'utf8', env: { ...baseEnv, BASH_ENV: fwd(shim), GSTACK_LEASE_LOCK_TRIES: '3' }, timeout: 120_000 }); + expect(r.status, `${r.error ?? ''}${r.stdout}${r.stderr}`).toBe(0); + expect(fs.readdirSync(leases)).toEqual(['4243']); + }); + + test('a lock held by a live Git Bash process is kept (touch exits 3 naming it); a dead holder\'s lock is broken', () => { + const { dir, leases } = checkout(); + const script = [ + 'set -u', + 'mkdir -p "$LEASES/.lock" && printf "%s \\n" "$$" > "$LEASES/.lock/owner"', + 'GSTACK_LEASE_LOCK_TRIES=2 bash "$LEASE" touch --root "$ROOT" --pid 4244; echo "held=$? holder=$$"', + '( exit 0 ) & dead=$!; wait "$dead"', + 'printf "%s \\n" "$dead" > "$LEASES/.lock/owner"', + 'GSTACK_LEASE_LOCK_TRIES=2 bash "$LEASE" touch --root "$ROOT" --pid 4245; echo "dead=$?"', + ].join('\n'); + const r = runBashSync(script, { + encoding: 'utf8', timeout: 120_000, + env: { ...baseEnv, LEASE: fwd(LEASE), ROOT: fwd(dir), LEASES: fwd(leases) }, + }); + const out = `${r.error ?? ''}${r.stdout}${r.stderr}`; + expect(r.status, out).toBe(0); + const holder = /held=(\d+) holder=(\d+)/.exec(r.stdout); + expect(holder?.[1], out).toBe('3'); + expect(r.stderr).toContain(`locked by live process ${holder?.[2]}`); + expect(r.stdout, out).toContain('dead=0'); + expect(fs.readdirSync(leases).sort()).toEqual(['4245']); + }); +}); diff --git a/test/gstack-lease.test.ts b/test/gstack-lease.test.ts index f2b6585d2..85a90a4f2 100644 --- a/test/gstack-lease.test.ts +++ b/test/gstack-lease.test.ts @@ -59,6 +59,19 @@ function otherStart(recorded: string): string { return recorded.startsWith('linux ') ? recorded.replace(/\d+$/, (n) => String(Number(n) + 1)) : 'ps Thu Jan 1 00:00:00 1970'; } +/** A directory lock as gstack-lease takes it: /owner holds the holder's token. */ +function plantLock(lockPath: string, token: string) { + fs.mkdirSync(lockPath, { recursive: true }); + fs.writeFileSync(path.join(lockPath, 'owner'), token + '\n'); +} +const ownerOf = (lockPath: string) => fs.readFileSync(path.join(lockPath, 'owner'), 'utf8').trim(); + +/** Each lock layout a holder can leave: the directory lock, and the symlink lock an older gstack took. */ +const LAYOUTS: Array<[string, (lockPath: string, token: string) => void]> = [ + ['directory lock', plantLock], + ['legacy symlink lock', (lockPath, token) => fs.symlinkSync(token, lockPath)], +]; + /** A BASH_ENV file whose functions shadow commands inside the lease script (fault injection). */ function bashEnv(dir: string, body: string): string { const file = path.join(dir, 'bash-env.sh'); @@ -160,7 +173,7 @@ describe('gstack-lease', () => { const renewed = path.join(dir, 'renewed'); fs.writeFileSync(renewed, fresh); // The holder renews just before reclaim takes the lock, as if touch ran after the stale verdict. - const racing = bashEnv(dir, `ln() { case "$*" in *"/.lock") command cp "${renewed}" "${file}" ;; esac; command ln "$@"; }\n`); + const racing = bashEnv(dir, `mkdir() { case "$*" in *"/.lock") command cp "${renewed}" "${file}" ;; esac; command mkdir "$@"; }\n`); const r = run(['live', '--root', dir], { BASH_ENV: racing }); expect(r.status, r.stdout + r.stderr).toBe(0); expect(r.stderr).not.toContain('reclaimed'); @@ -168,23 +181,24 @@ describe('gstack-lease', () => { expect(fs.readdirSync(leases).filter((n) => n.startsWith('.'))).toEqual([]); }); - test('a lock whose holder died, or whose pid now belongs to another process, is broken at once', () => { + test.each(LAYOUTS)('a %s whose holder died, or whose pid now belongs to another process, is broken at once', (_layout, plant) => { const { dir, leases } = checkout(); const holder = sleeper(); fs.mkdirSync(leases, { recursive: true }); - fs.symlinkSync(`${deadPid()} `, path.join(leases, '.lock')); + plant(path.join(leases, '.lock'), `${deadPid()} `); const r = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); expect(r.status, r.stdout + r.stderr).toBe(0); expect(fs.existsSync(path.join(leases, '.lock'))).toBe(false); const identity = fs.readFileSync(path.join(leases, String(holder)), 'utf8').trim(); expect(identity).toMatch(/^(linux|ps) /); - fs.symlinkSync(`${holder} ${otherStart(identity)}`, path.join(leases, '.lock')); + plant(path.join(leases, '.lock'), `${holder} ${otherStart(identity)}`); const again = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); expect(again.status, again.stdout + again.stderr).toBe(0); expect(fs.existsSync(path.join(leases, '.lock'))).toBe(false); + expect(fs.readdirSync(leases).filter((n) => n.startsWith('.'))).toEqual([]); }); - test('a live holder\'s lock is never broken, however old: touch exits 3 without writing and a reclaim keeps the lease', () => { + test.each(LAYOUTS)('a live holder\'s %s is never broken, however old: touch exits 3 without writing and a reclaim keeps the lease', (layout, plant) => { const { dir, leases } = checkout(); const owner = sleeper(); const holder = sleeper(); @@ -192,9 +206,10 @@ describe('gstack-lease', () => { expect(run(['touch', '--root', dir, '--pid', String(holder)]).status).toBe(0); const ownerToken = `${owner} ${fs.readFileSync(path.join(leases, String(owner)), 'utf8').trim()}`; const lockPath = path.join(leases, '.lock'); - fs.symlinkSync(ownerToken, lockPath); + plant(lockPath, ownerToken); const old = new Date(Date.now() - 5 * 60_000); fs.lutimesSync(lockPath, old, old); + if (layout === 'directory lock') fs.utimesSync(path.join(lockPath, 'owner'), old, old); const holderFile = path.join(leases, String(holder)); const stale = otherStart(fs.readFileSync(holderFile, 'utf8').trim()) + '\n'; fs.writeFileSync(holderFile, stale); @@ -207,7 +222,7 @@ describe('gstack-lease', () => { expect(l.stderr).not.toContain('reclaimed'); expect(l.stdout).toContain(`${holder} `); expect(fs.existsSync(holderFile)).toBe(true); - expect(fs.readlinkSync(lockPath)).toBe(ownerToken); + expect(layout === 'directory lock' ? ownerOf(lockPath) : fs.readlinkSync(lockPath)).toBe(ownerToken); }); test('two breakers of one dead lock: the second rereads under .lock.break and leaves the lock the first one took', () => { @@ -217,21 +232,21 @@ describe('gstack-lease', () => { expect(run(['touch', '--root', dir, '--pid', String(owner)]).status).toBe(0); const ownerToken = `${owner} ${fs.readFileSync(path.join(leases, String(owner)), 'utf8').trim()}`; const lockPath = path.join(leases, '.lock'); - fs.symlinkSync(`${deadPid()} `, lockPath); + plantLock(lockPath, `${deadPid()} `); // Between this breaker's dead verdict and its .lock.break, another breaker removes the dead lock and takes it. - const first = bashEnv(dir, `ln() { case "$*" in *"/.lock.break") command rm -f "${lockPath}"; command ln -s "${ownerToken}" "${lockPath}" ;; esac; command ln "$@"; }\n`); + const first = bashEnv(dir, `mkdir() { case "$*" in *"/.lock.break") command rm -rf "${lockPath}"; command mkdir "${lockPath}"; printf '%s\\n' "${ownerToken}" > "${lockPath}/owner" ;; esac; command mkdir "$@"; }\n`); const t = run(['touch', '--root', dir, '--pid', String(holder)], { BASH_ENV: first, GSTACK_LEASE_LOCK_TRIES: '3' }); expect(t.status, t.stdout + t.stderr).toBe(3); - expect(fs.readlinkSync(lockPath)).toBe(ownerToken); - expect(fs.existsSync(path.join(leases, '.lock.break'))).toBe(false); + expect(ownerOf(lockPath)).toBe(ownerToken); + expect(fs.readdirSync(leases).filter((n) => n.startsWith('.')).sort()).toEqual(['.lock']); }); - test('a .lock.break left by a dead breaker is cleared and the dead lock is still broken', () => { + test.each(LAYOUTS)('a .lock.break (%s) left by a dead breaker is cleared and the dead lock is still broken', (_layout, plant) => { const { dir, leases } = checkout(); const holder = sleeper(); fs.mkdirSync(leases, { recursive: true }); - fs.symlinkSync(`${deadPid()} `, path.join(leases, '.lock')); - fs.symlinkSync(`${deadPid()} `, path.join(leases, '.lock.break')); + plant(path.join(leases, '.lock'), `${deadPid()} `); + plant(path.join(leases, '.lock.break'), `${deadPid()} `); const r = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '5' }); expect(r.status, r.stdout + r.stderr).toBe(0); expect(fs.existsSync(path.join(leases, String(holder)))).toBe(true); @@ -249,10 +264,83 @@ describe('gstack-lease', () => { fs.writeFileSync(holderFile, otherStart(fs.readFileSync(holderFile, 'utf8').trim()) + '\n'); const lockPath = path.join(leases, '.lock'); // While reclaim deletes the stale lease under its lock, the lock changes hands. - const swap = bashEnv(dir, `rm() { if [ "$*" = "-f ${holderFile}" ]; then command rm -f "${lockPath}"; command ln -s "${ownerToken}" "${lockPath}"; fi; command rm "$@"; }\n`); + const swap = bashEnv(dir, `rm() { if [ "$*" = "-f ${holderFile}" ]; then command rm -rf "${lockPath}"; command mkdir "${lockPath}"; printf '%s\\n' "${ownerToken}" > "${lockPath}/owner"; fi; command rm "$@"; }\n`); const r = run(['live', '--root', dir, '--exclude-ancestry-of', String(owner)], { BASH_ENV: swap }); expect(r.stderr).toContain(`reclaimed stale lease ${holder}`); - expect(fs.readlinkSync(lockPath)).toBe(ownerToken); + expect(ownerOf(lockPath)).toBe(ownerToken); + }); + + test('a lock held by a running gstack-lease excludes a second taker, which names it; both leases land once it is released', async () => { + const { dir, leases } = checkout(); + const [first, second] = [sleeper(), sleeper()]; + // The first touch holds the lock across its prune (find runs under the lock). + const slow = bashEnv(dir, 'find() { sleep 3; command find "$@"; }\n'); + const holder = spawn('bash', [LEASE, 'touch', '--root', dir, '--pid', String(first)], { env: { ...baseEnv, BASH_ENV: slow }, stdio: 'ignore' }); + children.push(holder); + const exited = new Promise((resolve) => holder.on('exit', resolve)); + const owner = path.join(leases, '.lock', 'owner'); + for (let i = 0; i < 100 && !fs.existsSync(owner); i++) await Bun.sleep(20); + expect(fs.readFileSync(owner, 'utf8')).toMatch(new RegExp(`^${holder.pid} `)); + const blocked = run(['touch', '--root', dir, '--pid', String(second)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(blocked.status, blocked.stdout + blocked.stderr).toBe(3); + expect(blocked.stderr).toContain(`locked by live process ${holder.pid};`); + expect(await exited).toBe(0); + expect(run(['touch', '--root', dir, '--pid', String(second)]).status).toBe(0); + expect(fs.readdirSync(leases).sort()).toEqual([String(first), String(second)].sort()); + }); + + test('parallel touches all take the lock in turn and leave no lock, claim or temp file behind', async () => { + const { dir, leases } = checkout(); + const pids = Array.from({ length: 16 }, (_, i) => 4300 + i); + const codes = await Promise.all(pids.map((p) => new Promise((resolve) => { + const c = spawn('bash', [LEASE, 'touch', '--root', dir, '--pid', String(p)], { env: baseEnv, stdio: 'ignore' }); + children.push(c); + c.on('exit', resolve); + }))); + expect(codes).toEqual(pids.map(() => 0)); + expect(fs.readdirSync(leases).sort()).toEqual(pids.map(String).sort()); + }); + + // #3330: a taker killed between its mkdir and its owner rename leaves an ownerless lock. + test('an ownerless lock is broken once every claimant is gone, and kept while a live claimant may still own it', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const lockPath = path.join(leases, '.lock'); + fs.mkdirSync(lockPath, { recursive: true }); + fs.writeFileSync(path.join(leases, `.lock.claim.${holder}`), `${holder} \n`); + const kept = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(kept.status, kept.stdout + kept.stderr).toBe(3); + expect(kept.stderr).toContain('cannot take the lease directory lock'); + expect(fs.existsSync(path.join(leases, String(holder)))).toBe(false); + expect(fs.readdirSync(lockPath)).toEqual([]); + + const dead = deadPid(); + fs.renameSync(path.join(leases, `.lock.claim.${holder}`), path.join(leases, `.lock.claim.${dead}`)); + fs.writeFileSync(path.join(leases, `.lock.claim.${dead}`), `${dead} \n`); + fs.writeFileSync(path.join(leases, '.lock.claim.1'), ''); + const broken = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(broken.status, broken.stdout + broken.stderr).toBe(0); + expect(fs.existsSync(path.join(leases, String(holder)))).toBe(true); + expect(fs.existsSync(lockPath)).toBe(false); + + fs.rmSync(path.join(leases, `.lock.claim.${dead}`)); + fs.mkdirSync(lockPath); + const unclaimed = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(unclaimed.status, unclaimed.stdout + unclaimed.stderr).toBe(0); + expect(fs.existsSync(lockPath)).toBe(false); + }); + + test('a lock that cannot be taken while nobody holds it fails at once instead of waiting out the tries', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const broken = bashEnv(dir, 'mkdir() { case "$*" in *"/.lock") return 1 ;; esac; command mkdir "$@"; }\n'); + const started = Date.now(); + const r = run(['touch', '--root', dir, '--pid', String(holder)], { BASH_ENV: broken }); + expect(r.status, r.stdout + r.stderr).toBe(3); + expect(Date.now() - started).toBeLessThan(10_000); + expect(r.stderr).toContain('cannot take the lease directory lock'); + expect(r.stderr).not.toContain('locked by live process'); + expect(fs.readdirSync(leases)).toEqual([]); }); test('a lease without a recorded identity (older gstack) counts while fresh and is stale past the 120-minute window', () => { @@ -333,7 +421,7 @@ describe('gstack-lease', () => { fs.writeFileSync(path.join(state, 'config.yaml'), 'update_check: false\n'); const leaseDir = path.join(state, 'leases'); fs.mkdirSync(leaseDir); - fs.symlinkSync(`${sleeper()} `, path.join(leaseDir, '.lock')); + plantLock(path.join(leaseDir, '.lock'), `${sleeper()} `); const r = spawnSync(START, ['--skill', 'testskill', '--parent-pid', '424244'], { encoding: 'utf8', cwd: home, env: { PATH: process.env.PATH!, HOME: home, GSTACK_HOME: state, GSTACK_LEASE_DIR: leaseDir, GSTACK_LEASE_LOCK_TRIES: '2' }, timeout: 30_000, });