From 7bef38f3d78c85c66a734194951e48b92eebdb26 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 00:40:15 +0000 Subject: [PATCH] fix(lease): a dead lock that cannot be renamed aside no longer spins lock() break_lock ignored drop()'s failure and returned 0, so lock() continued past the tries limit forever when a dead holder's .lock could not be renamed aside (a Windows file in use). It now removes the lock in place under .lock.break, and returns 1 when that also fails so the try is counted. touch no longer calls a dead holder 'live' in its exit-3 message. Adds the /ship coverage-audit tests for the lock's failure paths. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-lease | 20 ++++--- test/gstack-lease.test.ts | 108 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/bin/gstack-lease b/bin/gstack-lease index d7efe9e40..2c65185c4 100755 --- a/bin/gstack-lease +++ b/bin/gstack-lease @@ -167,10 +167,13 @@ holder_gone() { case "$(state "${1%% *}" "${1#* }" 0)" in dead|stale) return 0 ; # 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. +# .lock.break whose holder is gone is cleared. A lock that cannot be renamed +# aside is removed in place (other breakers wait on .lock.break meanwhile). +# Returns 1 when another breaker holds .lock.break or the lock could not be +# removed, so lock() counts the try instead of spinning. 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 ! take "$LOCK.break"; then _b=$(owner_of "$LOCK.break") @@ -181,13 +184,14 @@ break_lock() { fi return 1 fi + _kept=0 if [ -n "$1" ]; then - [ "$(owner_of "$LOCK")" = "$1" ] && drop "$LOCK" + [ "$(owner_of "$LOCK")" = "$1" ] && { drop "$LOCK" || rm -rf "$LOCK" || _kept=1; } else - orphaned "$LOCK" && drop "$LOCK" + orphaned "$LOCK" && { drop "$LOCK" || rm -rf "$LOCK" || _kept=1; } fi drop "$LOCK.break" - return 0 + return $_kept } 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 @@ -208,7 +212,7 @@ case "$VERB" in [ -n "$PID" ] || PID=$(gstack_harness_pid "${FROM:-$PPID}") mkdir -p "$DIR" || exit 2 lock || { - if [ -n "${LOCK_OWNER:-}" ]; then echo "gstack-lease: the lease directory is locked by live process ${LOCK_OWNER%% *}; lease $PID not renewed" >&2 + if [ -n "${LOCK_OWNER:-}" ] && ! holder_gone "$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 diff --git a/test/gstack-lease.test.ts b/test/gstack-lease.test.ts index b372d5bbe..35494b445 100644 --- a/test/gstack-lease.test.ts +++ b/test/gstack-lease.test.ts @@ -343,6 +343,114 @@ describe('gstack-lease', () => { expect(fs.readdirSync(leases)).toEqual([]); }); + // Value: protects=a .lock.break held by a live breaker (owner, or claim on an ownerless dir) is never cleared; an ownerless one with no claimant is; + // fails_when=break_lock's liveness check on .lock.break is dropped, or orphaned() ignores a live claim; + // why_new=the existing .lock.break test plants only a dead breaker's owner, never a live breaker or an ownerless one; + // seam=none; no_claim=two live breakers racing for real (sleeping children stand in as the live holders) + test('a .lock.break whose breaker lives (owner or claim) is never cleared; an ownerless one with no live claimant is', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const breaker = sleeper(); + const lockPath = path.join(leases, '.lock'); + const breakPath = path.join(leases, '.lock.break'); + plantLock(lockPath, `${deadPid()} `); + plantLock(breakPath, `${breaker} `); + const owned = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(owned.status, owned.stdout + owned.stderr).toBe(3); + expect(ownerOf(breakPath)).toBe(String(breaker)); + expect(fs.existsSync(lockPath)).toBe(true); + expect(fs.existsSync(path.join(leases, String(holder)))).toBe(false); + + fs.rmSync(breakPath, { recursive: true }); + fs.mkdirSync(breakPath); + fs.writeFileSync(path.join(leases, `.lock.break.claim.${breaker}`), `${breaker} \n`); + const claimed = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '3' }); + expect(claimed.status, claimed.stdout + claimed.stderr).toBe(3); + expect(fs.readdirSync(breakPath)).toEqual([]); + expect(fs.existsSync(lockPath)).toBe(true); + expect(fs.existsSync(path.join(leases, String(holder)))).toBe(false); + + fs.rmSync(path.join(leases, `.lock.break.claim.${breaker}`)); + const orphaned = run(['touch', '--root', dir, '--pid', String(holder)], { GSTACK_LEASE_LOCK_TRIES: '5' }); + expect(orphaned.status, orphaned.stdout + orphaned.stderr).toBe(0); + expect(fs.readdirSync(leases)).toEqual([String(holder)]); + }); + + // Value: protects=a lock dir whose owner rename failed is removed, never left ownerless; touch exits 3 saying it cannot take the lock; + // fails_when=take() stops removing the lock dir after a failed owner rename; + // why_new=the ownerless-lock test plants the dir by hand; none fails take() between its mkdir and its owner rename; + // seam=none; no_claim=real mv failure modes (a BASH_ENV shim fails the claim-to-owner rename) + test('a lock whose owner cannot be published is removed, not left ownerless, and touch fails at once', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const shim = bashEnv(dir, 'mv() { case "$*" in *".lock.claim."*) return 1 ;; esac; command mv "$@"; }\n'); + const r = run(['touch', '--root', dir, '--pid', String(holder)], { BASH_ENV: shim }); + expect(r.status, r.stdout + r.stderr).toBe(3); + expect(r.stderr).toContain('cannot take the lease directory lock'); + expect(r.stderr).not.toContain('locked by live process'); + expect(fs.readdirSync(leases)).toEqual([]); + }); + + // Value: protects=a lock that vanishes between a failed take and its check is retaken at once: touch writes its lease, no sleep, no lock left; + // fails_when=the vanished-lock retry limit drops to 1, so one release race exits 3; + // why_new=the fails-at-once test pins only the upper bound (every take fails); no test fails a single take; + // seam=none; no_claim=a real release race (a BASH_ENV shim fails the first mkdir of the lock) + test('a lock released between a failed take and its check is retaken without waiting', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const waits = path.join(dir, 'waits.log'); + const once = path.join(dir, 'failed-once'); + const shim = bashEnv(dir, `mkdir() { case "$*" in *"/.lock") if [ ! -e "${once}" ]; then : > "${once}"; return 1; fi ;; esac; command mkdir "$@"; }\nsleep() { echo "$*" >> "${waits}"; command sleep "$@"; }\n`); + const r = run(['touch', '--root', dir, '--pid', String(holder)], { BASH_ENV: shim }); + expect(r.status, r.stdout + r.stderr).toBe(0); + expect(fs.existsSync(once), 'the shim never failed a take').toBe(true); + expect(fs.existsSync(waits), 'lock() slept between tries').toBe(false); + expect(fs.readdirSync(leases)).toEqual([String(holder)]); + }); + + // Value: protects=unlock removes the caller's own lock even when renaming it aside fails: touch exits 0, lease written, no .lock left behind; + // fails_when=the rm -rf fallback after drop in unlock() is removed; + // why_new=no test fails the rename-aside step of drop(), so unlock's fallback never runs; + // seam=none; no_claim=real mv failure modes such as a Windows file in use (a BASH_ENV shim fails the .lock.gone rename) + test('unlock still removes its own lock when renaming it aside fails', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const shim = bashEnv(dir, 'mv() { case "$*" in *".lock.gone."*) return 1 ;; esac; command mv "$@"; }\n'); + const r = run(['touch', '--root', dir, '--pid', String(holder)], { BASH_ENV: shim }); + expect(r.status, r.stdout + r.stderr).toBe(0); + expect(fs.readdirSync(leases)).toEqual([String(holder)]); + }); + + // Value: protects=a dead holder's lock that cannot be renamed aside is removed in place, and one that cannot be removed at all ends touch + // with exit 3 within the tries limit instead of spinning, without calling the dead holder live; + // fails_when=break_lock reports success after a failed drop and lock() loops without sleeping or counting tries; + // why_new=the dead-holder tests always let the rename-aside succeed; the tries limit is only reached through the live-holder path; + // seam=none; no_claim=real mv/rm failure modes such as a Windows file in use (a BASH_ENV shim fails the .lock.gone rename and the rm) + test('a dead holder\'s lock that cannot be renamed aside ends touch with a verdict instead of spinning', () => { + const { dir, leases } = checkout(); + const holder = sleeper(); + const dead = deadPid(); + const touch = (shim: string) => spawnSync('bash', [LEASE, 'touch', '--root', dir, '--pid', String(holder)], { + encoding: 'utf8', env: { ...baseEnv, BASH_ENV: shim, GSTACK_LEASE_LOCK_TRIES: '3' }, timeout: 8_000, + }); + const noRename = 'mv() { case "$*" in *".lock.gone."*) return 1 ;; esac; command mv "$@"; }\n'; + plantLock(path.join(leases, '.lock'), `${dead} `); + const removed = touch(bashEnv(dir, noRename)); + expect(removed.error?.message ?? '', 'touch did not end within 8 s').not.toContain('ETIMEDOUT'); + expect(removed.status, removed.stdout + removed.stderr).toBe(0); + expect(fs.readdirSync(leases)).toEqual([String(holder)]); + + fs.rmSync(path.join(leases, String(holder))); + plantLock(path.join(leases, '.lock'), `${dead} `); + const stuck = touch(bashEnv(dir, noRename + 'rm() { case "$*" in *"/.lock") return 1 ;; esac; command rm "$@"; }\n')); + expect(stuck.error?.message ?? '', 'touch did not end within 8 s').not.toContain('ETIMEDOUT'); + expect(stuck.status, stuck.stdout + stuck.stderr).toBe(3); + expect(stuck.stderr).toContain('cannot take the lease directory lock'); + expect(stuck.stderr).not.toContain('locked by live process'); + expect(ownerOf(path.join(leases, '.lock'))).toBe(String(dead)); + expect(fs.existsSync(path.join(leases, String(holder)))).toBe(false); + }, 20_000); + test('a lease without a recorded identity (older gstack) counts while fresh and is stale past the 120-minute window', () => { const { dir, leases } = checkout(); const holder = sleeper();