diff --git a/.changeset/1884-planning-lock-mkdir-failure.md b/.changeset/1884-planning-lock-mkdir-failure.md new file mode 100644 index 000000000..a290595a9 --- /dev/null +++ b/.changeset/1884-planning-lock-mkdir-failure.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 3472 +--- + +**`withPlanningLock` no longer reports a phantom "held by a live process" timeout when `.planning/` cannot be created** — a best-effort `try { platformEnsureDir(...) } catch { /* ok */ }` swallowed the real mkdir failure (EACCES/ENOSPC/EROFS), so the subsequent lock write failed with ENOENT (parent missing), and because ENOENT is in the lock's retry set (added for a Docker overlay-fs race) the loop spun the full 10 s budget before throwing a misattributed contention error that pointed operators at a nonexistent lock-holder. The mkdir failure now propagates immediately with its real filesystem errno and message, so an unwritable or full disk is reported as itself, not as concurrent-writer contention. The Docker overlay-fs ENOENT *lock-write* race (directory present) is still retried as before, and every code path where `.planning/` already exists or can be created is unchanged. Part of epic #1879 (distinguish "absent" from "corrupt/permission-denied" across engine read paths). (#1884) diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index ef3502883..a921c1e3d 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -206,8 +206,15 @@ function withPlanningLock(cwd: string, fn: () => T, clock?: Clock): T { const deadmanCeilingMs = 60000; const start = clock.now(); - // Ensure .planning/ exists - try { platformEnsureDir(planningDir(cwd)); } catch { /* ok */ } + // Ensure .planning/ exists. A genuine failure here (EACCES/ENOSPC/EROFS/EMFILE) + // MUST surface immediately: the prior `catch { /* ok */ }` swallowed it, the lock + // write below then failed with ENOENT (parent dir missing), and ENOENT is retryable + // (PLANNING_LOCK_RETRY_ERRNOS — added for a Docker overlay-fs race), so the loop + // spun the full 10s budget and reported a PHANTOM "held by a live process" + // contention pointing at a nonexistent holder (epic #1879 / F16, #1884). + // `mkdirSync(recursive:true)` does not throw on an existing dir, so the normal + // path (dir already present) is unaffected; only real creation failures propagate. + platformEnsureDir(planningDir(cwd)); function acquireLock(): void { // Atomic create — fails if file exists diff --git a/tests/planning-lock-mkdir-failure-1884.test.cjs b/tests/planning-lock-mkdir-failure-1884.test.cjs new file mode 100644 index 000000000..1edc64cf0 --- /dev/null +++ b/tests/planning-lock-mkdir-failure-1884.test.cjs @@ -0,0 +1,139 @@ +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { cleanup } = require('./helpers.cjs'); +const { makeFakeClock } = require('./helpers/clock.cjs'); + +// Built lib is the test target (same surface the rest of the suite imports). +const planningWorkspaceDirect = require('../gsd-core/bin/lib/planning-workspace.cjs'); +const { withPlanningLock } = planningWorkspaceDirect; + +// ───────────────────────────────────────────────────────────────────────────── +// Epic #1879 / F16 (#1884): withPlanningLock swallows platformEnsureDir failure. +// +// A mkdir failure (EACCES/ENOSPC/EROFS) creating `.planning/` was swallowed by a +// `catch { /* ok */ }`; the subsequent lock write then threw ENOENT (parent +// missing), and ENOENT is in PLANNING_LOCK_RETRY_ERRNOS (for the Docker +// overlay-fs race), so the loop spun the full 10s budget and reported a PHANTOM +// "held by a live process" contention pointing at a nonexistent holder. +// +// Fix: stop swallowing — surface the real FS errno immediately. These tests pin +// the new contract: a corrupt/permission mkdir is surfaced fast, not folded into +// a lock-contention retry. +// +// IO failure is forced via t.mock.method(fs, 'mkdirSync', ...), which auto-restores +// per-test (never chmod 0o000 — root bypasses mode bits, leaking coverage). The fake +// clock proves fail-fast with no wall-clock assertion. +// ───────────────────────────────────────────────────────────────────────────── + +const realMkdirSync = fs.mkdirSync; + +describe('withPlanningLock surfaces mkdir failure fast (epic #1879 / F16, #1884)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-planning-mkdir-fail-')); + // Deliberately do NOT pre-create tmpDir/.planning — the scenario under test + // is "the dir cannot be created." + }); + + afterEach(() => { + if (typeof planningWorkspaceDirect._resetLockProbes === 'function') { + planningWorkspaceDirect._resetLockProbes(); + } + if (typeof planningWorkspaceDirect._resetPlanningLockTestHooks === 'function') { + planningWorkspaceDirect._resetPlanningLockTestHooks(); + } + cleanup(tmpDir); + }); + + function failMkdirFor(t, targetDir, code) { + // Mock the shared fs.mkdirSync that platformEnsureDir calls dynamically. + // Re-throw the requested errno only for the planning dir; pass through everything + // else so unrelated test machinery keeps working. t.mock auto-restores after the test. + t.mock.method(fs, 'mkdirSync', (p, opts) => { + if (typeof p === 'string' && (p === targetDir || p.startsWith(targetDir + path.sep))) { + const err = new Error(code + ': permission denied, mkdir \'' + p + '\''); + err.code = code; + throw err; + } + return realMkdirSync.call(fs, p, opts); + }); + } + + test('EACCES creating .planning/ is surfaced immediately, not folded into a phantom lock timeout', (t) => { + const planningPath = path.join(tmpDir, '.planning'); + failMkdirFor(t, planningPath, 'EACCES'); + + const clock = makeFakeClock(0); + let ran = false; + assert.throws( + () => withPlanningLock(tmpDir, () => { ran = true; return 'should-not-run'; }, clock), + (err) => err && err.code === 'EACCES', + 'mkdir EACCES must surface as EACCES, not be swallowed into a phantom lock contention' + ); + assert.strictEqual(ran, false, 'critical section must not run when the planning dir cannot be created'); + assert.ok(clock.nowValue < 10000, + 'must fail fast — clock must NOT spin to the 10s lockTimeout budget (nowValue=' + clock.nowValue + 'ms)'); + }); + + test('ENOSPC creating .planning/ is surfaced immediately', (t) => { + failMkdirFor(t, path.join(tmpDir, '.planning'), 'ENOSPC'); + const clock = makeFakeClock(0); + assert.throws( + () => withPlanningLock(tmpDir, () => 'x', clock), + (err) => err && err.code === 'ENOSPC', + 'mkdir ENOSPC must surface as ENOSPC immediately' + ); + assert.ok(clock.nowValue < 10000, 'must fail fast on ENOSPC (nowValue=' + clock.nowValue + 'ms)'); + }); + + test('the thrown mkdir-failure error is NOT branded as a lock timeout (no phantom contention)', (t) => { + failMkdirFor(t, path.join(tmpDir, '.planning'), 'EACCES'); + const clock = makeFakeClock(0); + let captured; + try { + withPlanningLock(tmpDir, () => 'x', clock); + } catch (err) { + captured = err; + } + assert.ok(captured, 'withPlanningLock must throw on mkdir EACCES'); + assert.notStrictEqual(captured && captured.lockTimeout, true, + 'a mkdir failure must NOT carry lockTimeout=true (that brands a phantom contention)'); + }); + + test('normal path is unaffected: creatable .planning/ still runs the critical section', () => { + // No monkeypatch — .planning/ is creatable in the writable tmpDir. + const clock = makeFakeClock(0); + const result = withPlanningLock(tmpDir, () => 'ran-ok', clock); + assert.strictEqual(result, 'ran-ok', 'fn() must run and return when the planning dir is creatable'); + assert.ok(fs.existsSync(path.join(tmpDir, '.planning', '.lock')) === false, + 'lock is released after the critical section'); + }); + + test('a transient ENOENT on the lock WRITE (not mkdir) is still retried, not surfaced immediately', (t) => { + // Distinguishes this fix's target (mkdir failure, surfaced immediately) from the + // Docker overlay-fs race PLANNING_LOCK_RETRY_ERRNOS exists for (a transient ENOENT + // on the lock FILE write, with .planning/ already present) — that race must still + // retry and eventually succeed, not regress into fail-fast. + let writeAttempts = 0; + const realWriteFileSync = fs.writeFileSync; + t.mock.method(fs, 'writeFileSync', (p, data, opts) => { + if (typeof p === 'string' && p.endsWith(path.join('.planning', '.lock')) && writeAttempts === 0) { + writeAttempts++; + const err = new Error('ENOENT: transient overlay-fs race, open \'' + p + '\''); + err.code = 'ENOENT'; + throw err; + } + return realWriteFileSync.call(fs, p, data, opts); + }); + + const clock = makeFakeClock(0); + const result = withPlanningLock(tmpDir, () => 'survived-the-race', clock); + assert.strictEqual(result, 'survived-the-race', 'the critical section must still run after the transient race resolves'); + assert.strictEqual(writeAttempts, 1, 'the race must have been hit exactly once (sanity check on the mock)'); + assert.ok(clock.nowValue > 0, 'a genuine retry must have consumed at least one sleep cycle (nowValue=' + clock.nowValue + 'ms)'); + }); +});