diff --git a/.changeset/happy-pandas-glide.md b/.changeset/happy-pandas-glide.md new file mode 100644 index 000000000..522e33ed1 --- /dev/null +++ b/.changeset/happy-pandas-glide.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3765 +--- +**`--cursor --local` install no longer self-deadlocks on `gsd-install-migration.lock` on Windows** — release closure now uses `unlinkSync` so NTFS EPERM surfaces rather than being silently swallowed; stale-lock reclamation detects same-process PID re-entry and dead PIDs, reclaiming immediately instead of spinning for 30 s; empty lock files left by mid-write failures are also cleaned up to prevent orphan stale locks. (#3670) diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index 18fe72be0..c1039034d 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -223,6 +223,40 @@ function sleepSync(ms) { Atomics.wait(new Int32Array(buffer), 0, 0, ms); } +/** + * Check whether a given PID is alive on the current host. + * Uses process.kill(pid, 0) which works on POSIX and Windows (Node's + * implementation maps it to OpenProcess + GetExitCodeProcess on win32). + * Returns true if alive or permission-denied (live but not ours), + * false if ESRCH (no such process). + */ +function isPidAlive(pid) { + if (typeof pid !== 'number' || !Number.isFinite(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; // alive (or permission denied — treat as live) + } catch (err) { + return err.code !== 'ESRCH'; + } +} + +/** + * Try to read and parse the lock file JSON. Returns null on any error + * (missing, invalid JSON, I/O failure). + */ +function readLockFile(lockPath) { + try { + const raw = fs.readFileSync(lockPath, 'utf8'); + const parsed = JSON.parse(raw); + if (parsed && typeof parsed === 'object' && typeof parsed.pid === 'number') { + return parsed; + } + return null; + } catch { + return null; + } +} + function acquireInstallMigrationLock(configDir, { timeoutMs = DEFAULT_LOCK_TIMEOUT_MS } = {}) { fs.mkdirSync(configDir, { recursive: true }); const lockPath = path.join(configDir, INSTALL_MIGRATION_LOCK_NAME); @@ -230,16 +264,28 @@ function acquireInstallMigrationLock(configDir, { timeoutMs = DEFAULT_LOCK_TIMEO while (true) { let fd = null; + let lockCreatedByUs = false; try { fd = fs.openSync(lockPath, 'wx'); - fs.writeFileSync(fd, JSON.stringify({ + // Close the open descriptor before writing so the file handle is + // released on Windows before the release closure unlinks it. + // Write payload via writeFileSync with the path (not the fd) so we + // don't hold an open fd across the lifetime of the lock. + fs.closeSync(fd); + fd = null; + lockCreatedByUs = true; // we own the file; clean it up on any subsequent error + fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, acquiredAt: new Date().toISOString(), }) + '\n'); + lockCreatedByUs = false; // release closure owns cleanup from here return () => { const failures = []; - try { fs.closeSync(fd); } catch (error) { failures.push(error); } - try { fs.rmSync(lockPath, { force: true }); } catch (error) { failures.push(error); } + // Use unlinkSync (not rmSync with { force: true }) so EPERM errors + // are NOT silently swallowed. On Windows, if the unlink fails + // transiently, the error surfaces via releaseError so the caller + // can observe and surface it rather than leaving a stale lock. + try { fs.unlinkSync(lockPath); } catch (error) { failures.push(error); } if (failures.length > 0) { const releaseError = new Error(`failed to release installer migration lock: ${lockPath}`); releaseError.failures = failures; @@ -249,11 +295,38 @@ function acquireInstallMigrationLock(configDir, { timeoutMs = DEFAULT_LOCK_TIMEO } catch (error) { if (fd !== null) { try { fs.closeSync(fd); } catch { /* best-effort */ } - try { fs.rmSync(lockPath, { force: true }); } catch { /* best-effort */ } + try { fs.unlinkSync(lockPath); } catch { /* best-effort */ } + fd = null; + } else if (lockCreatedByUs) { + // fd was closed but writeFileSync threw before we returned the release + // closure — the empty lock file is still on disk and must be removed + // so it does not orphan as an unreadable (empty/invalid JSON) stale lock. + try { fs.unlinkSync(lockPath); } catch { /* best-effort */ } } if (error && error.code === 'EEXIST') { + // Stale-lock reclamation: read the on-disk PID and check liveness. + // If the PID is dead (ESRCH) or is our own process (same-process + // re-entry caused by rmSync silently swallowing an unlink error on + // a previous call in the same invocation — the root cause of #3670), + // reclaim the lock by removing the stale file and retrying. + const lockData = readLockFile(lockPath); + if (lockData !== null) { + const holderPid = lockData.pid; + const isSameProcess = holderPid === process.pid; + const isDeadProcess = !isPidAlive(holderPid); + if (isSameProcess || isDeadProcess) { + // Reclaim: remove the stale lock and loop back to openSync. + // Only continue (retry) when unlink actually succeeds — a silent + // continue on reclaim failure recreates the original deadlock: + // the lock stays on disk and we spin indefinitely. + let reclaimed = false; + try { fs.unlinkSync(lockPath); reclaimed = true; } catch { /* unlink failed — fall through to timeout path */ } + if (reclaimed) continue; + } + } if (Date.now() - started >= timeoutMs) { - throw new Error(`installer migration lock is held: ${lockPath}`); + const holderInfo = lockData ? ` (held by pid ${lockData.pid} since ${lockData.acquiredAt})` : ''; + throw new Error(`installer migration lock is held: ${lockPath}${holderInfo}`); } sleepSync(Math.min(50, Math.max(1, timeoutMs - (Date.now() - started)))); continue; diff --git a/tests/bug-3670-cursor-local-install-migration-lock.test.cjs b/tests/bug-3670-cursor-local-install-migration-lock.test.cjs new file mode 100644 index 000000000..668b3b896 --- /dev/null +++ b/tests/bug-3670-cursor-local-install-migration-lock.test.cjs @@ -0,0 +1,323 @@ +/** + * Regression tests for issue #3670: --cursor --local install self-deadlocks + * on gsd-install-migration.lock. + * + * Root cause: On Windows, `fs.rmSync(lockPath, { force: true })` in the lock + * release closure silently swallows EPERM errors that NTFS returns when a + * recently-closed file descriptor's handle has not yet been fully released by + * the OS. The lock file is left on disk. The next `runInstallerMigrations` + * call in the same install() invocation hits EEXIST, spins for + * DEFAULT_LOCK_TIMEOUT_MS (30 s), then throws "installer migration lock is + * held". There is also no stale-PID reclamation: if the lock names the + * current process's PID, the helper should reclaim rather than spin. + * + * Windows wall-clock deadlock repro depends on Docker matrix Windows runners. + * These tests reproduce the failure modes via mock-injected fs faults on any + * platform (macOS/Linux/Windows). They fail deterministically WITHOUT the fix + * and pass WITH it. + * + * Test plan: + * T1 (same-process re-entry / stale-PID reclamation — primary regression) + * Pre-seed the lock file with {pid: process.pid, ...}. Verify that a + * runInstallerMigrations call reclaims the lock and succeeds rather than + * spinning 30 s and throwing. + * + * T2 (dead-PID reclamation — cross-invocation stale lock) + * Pre-seed the lock file with a PID known to be dead. Verify that acquire + * reclaims rather than throws. + * + * T3 (silent rmSync swallow / Windows EPERM simulation) + * Inject a fault that makes fs.rmSync throw EPERM for the lock file only + * (simulating Windows NTFS delete-pending). Verify that the lock file IS + * removed by an alternative path (or that the error propagates) — i.e. + * verify that the fix does not silently leave the lock on disk. + * + * T4 (counter-test: normal single acquire/release round-trip still works) + * No pre-seeded lock. One runInstallerMigrations call. Must succeed and + * leave no lock file behind. + * + * T5 (counter-test: genuinely-held live lock still surfaces an error) + * Pre-seed lock with a live PID (process.pid) AND simulate a lock that + * has been "truly acquired" (fd still open). With lockTimeoutMs: 0 and a + * truly un-reclaimable lock, must still throw with a useful message naming + * the holder PID. (This guards against over-reclamation.) + * + * @see https://github.com/gsd-build/get-shit-done/issues/3670 + */ + +'use strict'; + +const { test, mock } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { + INSTALL_MIGRATION_LOCK_NAME, + runInstallerMigrations, +} = require('../get-shit-done/bin/lib/installer-migrations.cjs'); + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function createTempDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3670-')); +} + +function cleanup(dir) { + fs.rmSync(dir, { recursive: true, force: true }); +} + +function lockPath(dir) { + return path.join(dir, INSTALL_MIGRATION_LOCK_NAME); +} + +function writeLockFile(dir, pid, acquiredAt) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + lockPath(dir), + JSON.stringify({ pid, acquiredAt: acquiredAt || new Date().toISOString() }) + '\n', + 'utf8' + ); +} + +/** + * Find a PID that is guaranteed to be dead on this host. + * We probe a set of high candidate PIDs (far outside the running set) and + * pick the first one for which process.kill(pid, 0) throws ESRCH. + * Falls back to 99999 if the probe loop exhausts (extremely unlikely). + */ +function findDeadPid() { + // Avoid process.pid ± small numbers — those could be live siblings. + for (let candidate = 600000; candidate < 700000; candidate += 1000) { + try { + process.kill(candidate, 0); + // Still alive (or permission denied but exists) — try next + } catch (err) { + if (err.code === 'ESRCH') return candidate; + } + } + return 99999; // fallback: extremely unlikely to be a live PID +} + +// --------------------------------------------------------------------------- +// T1: Same-process re-entry — stale lock with current process.pid reclaimed +// --------------------------------------------------------------------------- +test('T1: reclaims stale lock that names the current process PID (same-process re-entry)', (t) => { + const configDir = createTempDir(); + t.after(() => cleanup(configDir)); + + // Pre-seed lock file with the CURRENT process's PID — exactly what happens + // on Windows when rmSync swallows EPERM after the first runInstallerMigrations + // call releases (or fails to release) the lock. + writeLockFile(configDir, process.pid); + + // Without the fix: this would spin for lockTimeoutMs then throw. + // With the fix: detects own PID → reclaims → succeeds. + // lockTimeoutMs: 200 (fail fast so the test doesn't hang for 30 s without fix) + const result = runInstallerMigrations({ + configDir, + migrations: [], + lockTimeoutMs: 200, + }); + + assert.ok(result, 'runInstallerMigrations must return a result object'); + // Lock file must be removed after the call completes. + assert.equal( + fs.existsSync(lockPath(configDir)), + false, + 'lock file must not remain on disk after successful runInstallerMigrations' + ); +}); + +// --------------------------------------------------------------------------- +// T2: Dead-PID reclamation — cross-invocation stale lock +// --------------------------------------------------------------------------- +test('T2: reclaims stale lock whose PID is no longer alive', (t) => { + const configDir = createTempDir(); + t.after(() => cleanup(configDir)); + + const deadPid = findDeadPid(); + writeLockFile(configDir, deadPid); + + const result = runInstallerMigrations({ + configDir, + migrations: [], + lockTimeoutMs: 200, + }); + + assert.ok(result, 'runInstallerMigrations must return a result object'); + assert.equal( + fs.existsSync(lockPath(configDir)), + false, + 'lock file must not remain on disk after stale-PID reclamation' + ); +}); + +// --------------------------------------------------------------------------- +// T3: Windows EPERM simulation — unlinkSync failure surfaces (not silently swallowed) +// --------------------------------------------------------------------------- +test('T3: lock release does not silently leave lock file on disk when unlink fails (Windows EPERM simulation)', (t) => { + const configDir = createTempDir(); + const originalUnlinkSync = fs.unlinkSync; + + t.after(() => { + fs.unlinkSync = originalUnlinkSync; + cleanup(configDir); + }); + + // The fix uses fs.unlinkSync (not fs.rmSync with { force: true }) in the + // release closure. Inject EPERM on the lock file to simulate the Windows + // NTFS condition where the recently-closed handle has not been fully + // released by the OS. + // + // The fix's contract: EPERM must NOT be silently swallowed. + // Either (a) the error propagates as a releaseError, or (b) some alternative + // deletion path succeeds. Silent-swallow (no error + file still exists) is + // the failure condition we guard against. + let unlinkCallCount = 0; + fs.unlinkSync = function faultInjectUnlinkSync(targetPath) { + const isLock = path.basename(String(targetPath)) === INSTALL_MIGRATION_LOCK_NAME; + if (isLock) { + unlinkCallCount++; + // Simulate Windows EPERM (file handle not fully released by OS) + const err = Object.assign( + new Error('EPERM: operation not permitted, unlink ' + targetPath), + { code: 'EPERM' } + ); + throw err; + } + return originalUnlinkSync.call(fs, targetPath); + }; + + // With the fix: unlinkSync throws EPERM → releaseError is thrown by the + // release closure → runInstallerMigrations throws releaseError. + // With the buggy code (rmSync + force:true): EPERM was swallowed silently, + // no error thrown, lock file left on disk. + // + // Assert: if the call succeeds (no throw), the lock file must be gone. + // If the call throws, the error message must reference the lock. + let threw = false; + let thrownError = null; + try { + runInstallerMigrations({ + configDir, + migrations: [], + lockTimeoutMs: 500, + }); + } catch (err) { + threw = true; + thrownError = err; + } + + if (threw) { + // Acceptable: error surfaced. Verify it's lock-related (not a bug elsewhere). + assert.match( + thrownError.message, + /lock/i, + 'thrown error must reference the lock file' + ); + } else { + // If no error was thrown, the lock file must have been removed by some + // alternative path (not left silently on disk). + assert.equal( + fs.existsSync(lockPath(configDir)), + false, + 'if unlinkSync EPERM is encountered but no error thrown, lock file must still be removed' + ); + } + + // Sanity: the fault injection was actually triggered. + assert.ok(unlinkCallCount > 0, 'unlinkSync must have been called for the lock file at least once'); +}); + +// --------------------------------------------------------------------------- +// T4: Counter-test — normal single acquire/release round-trip still works +// --------------------------------------------------------------------------- +test('T4: normal (non-recursive) runInstallerMigrations acquires and releases lock correctly', (t) => { + const configDir = createTempDir(); + t.after(() => cleanup(configDir)); + + // No pre-seeded lock. Standard happy path. + const result = runInstallerMigrations({ + configDir, + migrations: [], + }); + + assert.ok(result, 'runInstallerMigrations must return a result'); + assert.equal( + fs.existsSync(lockPath(configDir)), + false, + 'lock file must be cleaned up after normal completion' + ); +}); + +// --------------------------------------------------------------------------- +// T5: Counter-test — unreclaimable live lock must surface a bounded error +// --------------------------------------------------------------------------- +// This test guards against over-reclamation: if the reclaim-unlink fails +// (e.g. Windows EPERM on a live open handle), the fix must NOT spin +// indefinitely — it must fall through to the timeout path and throw. +// +// Conditions forced by this test: +// 1. Lock file contains the CURRENT process.pid (triggers isSameProcess branch). +// 2. fs.unlinkSync is mocked to throw EPERM for the lock file (reclaim fails). +// 3. lockTimeoutMs: 200 — timeout must fire within a short wall-clock window. +// +// Expected outcome: throws with /installer migration lock is held/ within +// ~200ms. SUCCESS (no throw) is NOT acceptable here — that would mean the fix +// over-reclaimed a lock that it couldn't actually remove. +test('T5: unreclaimable same-PID lock throws bounded error (reclaim-unlink failure falls through to timeout)', (t) => { + const configDir = createTempDir(); + const originalUnlinkSync = fs.unlinkSync; + + t.after(() => { + mock.restoreAll(); + fs.unlinkSync = originalUnlinkSync; + cleanup(configDir); + }); + + // Pre-seed lock file with the CURRENT process's PID. + // This triggers the isSameProcess reclamation path inside acquireInstallerMigrationLock. + writeLockFile(configDir, process.pid); + + // Mock unlinkSync to throw EPERM for the lock file only. + // This simulates Windows NTFS refusing to delete a file with an open handle. + // With the fix: reclaim-unlink fails → reclaimed=false → falls through to + // the timeout check → throws "installer migration lock is held" after ≤200ms. + // Without the fix (original code): unlink throws but continue runs anyway → + // spins indefinitely, never reaches the timeout check → deadlock. + mock.method(fs, 'unlinkSync', function faultInjectUnlinkSync(targetPath) { + const isLock = path.basename(String(targetPath)) === INSTALL_MIGRATION_LOCK_NAME; + if (isLock) { + const err = Object.assign( + new Error('EPERM: operation not permitted, unlink ' + targetPath), + { code: 'EPERM' } + ); + throw err; + } + return originalUnlinkSync.call(fs, targetPath); + }); + + const startMs = Date.now(); + assert.throws( + () => runInstallerMigrations({ + configDir, + migrations: [], + lockTimeoutMs: 200, + }), + (err) => { + assert.match(err.message, /installer migration lock is held/, 'error must name the held lock'); + return true; + }, + 'must throw "installer migration lock is held" when reclaim-unlink fails — not spin indefinitely' + ); + const elapsedMs = Date.now() - startMs; + + // Must resolve within a generous but finite window (fix + timeout overhead). + // If it spins (deadlock regression), the process-level test timeout fires instead. + t.diagnostic(`T5 elapsed: ${elapsedMs}ms (expected ≤500ms for lockTimeoutMs:200 + overhead)`); + assert.ok(elapsedMs < 500, `T5 must resolve within 500ms; actual ${elapsedMs}ms — possible spin-loop regression`); +}); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 6d081261d..02e60d67f 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -681,17 +681,20 @@ test('refuses to run migrations while another installer owns the migration lock' test('reports lock release failures after migration work completes', (t) => { const configDir = createTempInstall(); - const originalRmSync = fs.rmSync; + const originalUnlinkSync = fs.unlinkSync; t.after(() => { - fs.rmSync = originalRmSync; + fs.unlinkSync = originalUnlinkSync; cleanup(configDir); }); - fs.rmSync = (targetPath, ...args) => { + // The release closure uses fs.unlinkSync (not fs.rmSync) so that EPERM is + // NOT silently swallowed on Windows (#3670). Mock unlinkSync to simulate + // a Windows NTFS EPERM condition when the lock file is removed. + fs.unlinkSync = (targetPath) => { if (path.basename(String(targetPath)) === 'gsd-install-migration.lock') { throw new Error('simulated lock unlink failure'); } - return originalRmSync.call(fs, targetPath, ...args); + return originalUnlinkSync.call(fs, targetPath); }; assert.throws(