From f2657e1ba06ccb1830940ee8acca67408908d3d4 Mon Sep 17 00:00:00 2001 From: Derek Pearson <32114370+dpearson2699@users.noreply.github.com> Date: Thu, 19 Mar 2026 14:12:06 -0400 Subject: [PATCH] fix: release stranded bootstrap locks and handle re-entrant reacquire (#1352) Release session locks on bootstrap abort paths and reset same-process lock state before re-acquiring so stale proper-lockfile callbacks cannot poison a fresh auto-mode session. Adds regression coverage for bootstrap cleanup and re-entrant lock acquisition. --- src/resources/extensions/gsd/auto-start.ts | 32 +++++++--- src/resources/extensions/gsd/session-lock.ts | 11 ++++ .../gsd/tests/auto-lock-creation.test.ts | 37 +++++++++++ .../gsd/tests/session-lock-regression.test.ts | 64 +++++++++++++++++++ 4 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/resources/extensions/gsd/auto-start.ts b/src/resources/extensions/gsd/auto-start.ts index 067300534..078ed3f31 100644 --- a/src/resources/extensions/gsd/auto-start.ts +++ b/src/resources/extensions/gsd/auto-start.ts @@ -103,13 +103,20 @@ export async function bootstrapAutoSession( return false; } - // Ensure git repo exists - if (!nativeIsRepo(base)) { - const mainBranch = loadEffectiveGSDPreferences()?.preferences?.git?.main_branch || "main"; - nativeInit(base, mainBranch); + function releaseLockAndReturn(): false { + releaseSessionLock(base); + clearLock(base); + return false; } - // Ensure .gitignore has baseline patterns + try { + // Ensure git repo exists + if (!nativeIsRepo(base)) { + const mainBranch = loadEffectiveGSDPreferences()?.preferences?.git?.main_branch || "main"; + nativeInit(base, mainBranch); + } + + // Ensure .gitignore has baseline patterns const gitPrefs = loadEffectiveGSDPreferences()?.preferences?.git; const manageGitignore = gitPrefs?.manage_gitignore; ensureGitignore(base, { manageGitignore }); @@ -251,10 +258,10 @@ export async function bootstrapAutoSession( "Discussion completed but no milestone context was written. Run /gsd to try the discussion again, or /gsd auto after creating the milestone manually.", "warning", ); - return false; + return releaseLockAndReturn(); } } else { - return false; + return releaseLockAndReturn(); } } @@ -276,7 +283,7 @@ export async function bootstrapAutoSession( "Discussion completed but milestone context is still missing. Run /gsd to try again.", "warning", ); - return false; + return releaseLockAndReturn(); } } } @@ -286,7 +293,7 @@ export async function bootstrapAutoSession( if (!state.activeMilestone) { const { showSmartEntry } = await import("./guided-flow.js"); await showSmartEntry(ctx, pi, base, { step: requestedStepMode }); - return false; + return releaseLockAndReturn(); } // ── Initialize session state ── @@ -479,5 +486,10 @@ export async function bootstrapAutoSession( } } catch { /* non-fatal */ } - return true; + return true; + } catch (err) { + releaseSessionLock(base); + clearLock(base); + throw err; + } } diff --git a/src/resources/extensions/gsd/session-lock.ts b/src/resources/extensions/gsd/session-lock.ts index 0f0ca227b..749e85e37 100644 --- a/src/resources/extensions/gsd/session-lock.ts +++ b/src/resources/extensions/gsd/session-lock.ts @@ -149,6 +149,16 @@ function ensureExitHandler(gsdDir: string): void { export function acquireSessionLock(basePath: string): SessionLockResult { const lp = lockPath(basePath); + // Re-entrant acquire on the same path: release our current OS lock first so + // proper-lockfile clears its update timer before we acquire a fresh lock. + if (_releaseFunction && _lockedPath === basePath) { + try { _releaseFunction(); } catch { /* may already be released */ } + _releaseFunction = null; + _lockedPath = null; + _lockPid = 0; + _lockCompromised = false; + } + // Ensure the directory exists mkdirSync(dirname(lp), { recursive: true }); @@ -234,6 +244,7 @@ export function acquireSessionLock(basePath: string): SessionLockResult { _releaseFunction = release; _lockedPath = basePath; _lockPid = process.pid; + _lockCompromised = false; // Safety net — uses centralized handler to avoid double-registration ensureExitHandler(gsdDir); diff --git a/src/resources/extensions/gsd/tests/auto-lock-creation.test.ts b/src/resources/extensions/gsd/tests/auto-lock-creation.test.ts index 2694e8820..e18bc2b6b 100644 --- a/src/resources/extensions/gsd/tests/auto-lock-creation.test.ts +++ b/src/resources/extensions/gsd/tests/auto-lock-creation.test.ts @@ -1,10 +1,25 @@ import test from "node:test"; import assert from "node:assert/strict"; import { mkdirSync, mkdtempSync, writeFileSync, existsSync, readFileSync, rmSync } from "node:fs"; +import { createRequire } from "node:module"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { writeLock, readCrashLock, clearLock, isLockProcessAlive } from "../crash-recovery.ts"; +import { acquireSessionLock, releaseSessionLock } from "../session-lock.ts"; + +const require = createRequire(import.meta.url); + +function hasProperLockfile(): boolean { + try { + require("proper-lockfile"); + return true; + } catch { + return false; + } +} + +const properLockfileAvailable = hasProperLockfile(); // ─── writeLock creates auto.lock in .gsd/ ──────────────────────────────── @@ -95,6 +110,28 @@ test("clearLock is safe when no lock file exists", () => { rmSync(dir, { recursive: true, force: true }); }); +test("bootstrap cleanup releases session lock artifacts", () => { + const dir = mkdtempSync(join(tmpdir(), "gsd-lock-test-")); + mkdirSync(join(dir, ".gsd"), { recursive: true }); + + try { + const result = acquireSessionLock(dir); + assert.equal(result.acquired, true, "session lock should be acquired"); + assert.ok(existsSync(join(dir, ".gsd", "auto.lock")), "auto.lock should exist while lock is held"); + if (properLockfileAvailable) { + assert.ok(existsSync(join(dir, ".gsd.lock")), ".gsd.lock should exist while lock is held"); + } + + releaseSessionLock(dir); + clearLock(dir); + + assert.ok(!existsSync(join(dir, ".gsd", "auto.lock")), "auto.lock should be removed by bootstrap cleanup"); + assert.ok(!existsSync(join(dir, ".gsd.lock")), ".gsd.lock should be removed by bootstrap cleanup"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + // ─── isLockProcessAlive detects live vs dead PIDs ──────────────────────── test("isLockProcessAlive returns false for dead PID", () => { diff --git a/src/resources/extensions/gsd/tests/session-lock-regression.test.ts b/src/resources/extensions/gsd/tests/session-lock-regression.test.ts index 00c1ac031..f955ebf83 100644 --- a/src/resources/extensions/gsd/tests/session-lock-regression.test.ts +++ b/src/resources/extensions/gsd/tests/session-lock-regression.test.ts @@ -11,6 +11,7 @@ */ import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync, readFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -26,6 +27,18 @@ import { gsdRoot } from '../paths.ts'; import { createTestContext } from './test-helpers.ts'; const { assertEq, assertTrue, report } = createTestContext(); +const require = createRequire(import.meta.url); + +function hasProperLockfile(): boolean { + try { + require("proper-lockfile"); + return true; + } catch { + return false; + } +} + +const properLockfileAvailable = hasProperLockfile(); async function main(): Promise { @@ -207,6 +220,57 @@ async function main(): Promise { } } + // ─── 9. Re-entrant acquisition without explicit release ─────────────── + console.log('\n=== 9. re-entrant acquire without explicit release ==='); + { + const base = mkdtempSync(join(tmpdir(), 'gsd-session-lock-')); + mkdirSync(join(base, '.gsd'), { recursive: true }); + + try { + const r1 = acquireSessionLock(base); + assertTrue(r1.acquired, 'first acquisition succeeds'); + + const r2 = acquireSessionLock(base); + assertTrue(r2.acquired, 're-entrant acquisition succeeds'); + + const valid = validateSessionLock(base); + assertTrue(valid, 're-entrant acquisition does not corrupt validation state'); + + releaseSessionLock(base); + } finally { + rmSync(base, { recursive: true, force: true }); + } + } + + // ─── 10. Re-entrant acquisition refreshes lock artifacts ────────────── + console.log('\n=== 10. re-entrant acquire refreshes lock artifacts ==='); + { + const base = mkdtempSync(join(tmpdir(), 'gsd-session-lock-')); + mkdirSync(join(base, '.gsd'), { recursive: true }); + + try { + const r1 = acquireSessionLock(base); + assertTrue(r1.acquired, 'first acquisition succeeds'); + + const lockDir = gsdRoot(base) + '.lock'; + if (properLockfileAvailable) { + assertTrue(existsSync(lockDir), '.gsd.lock/ exists after first acquisition'); + } + + const r2 = acquireSessionLock(base); + assertTrue(r2.acquired, 'second acquisition succeeds'); + if (properLockfileAvailable) { + assertTrue(existsSync(lockDir), '.gsd.lock/ exists after re-entrant acquisition'); + } + assertTrue(validateSessionLock(base), 'lock remains valid after re-entrant acquisition'); + + releaseSessionLock(base); + assertTrue(!existsSync(lockDir), '.gsd.lock/ is removed after release'); + } finally { + rmSync(base, { recursive: true, force: true }); + } + } + report(); }