fix(state): acquireStateLock retries on transient Docker/NFS errno codes (#3777)
* fix(state): acquireStateLock retries on transient Docker/NFS errno codes Expands retry allowlist to ENOENT/EINVAL/EIO/ESTALE/EAGAIN/EINTR in addition to existing EPERM/EBUSY. Truly fatal codes (EMFILE/ENOSPC/EROFS/EACCES) still throw. Resolves the locking-bugs regression seen across all 8 open consolidation PRs (#3738, #3741, #3752, #3754, #3756, #3759, #3760, #3769). Closes #3776 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): update pr number to 3777 in retry-allowlist fragment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/3776-fix-acquirestatelock-retry-allowlist.md
Normal file
5
.changeset/3776-fix-acquirestatelock-retry-allowlist.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3777
|
||||
---
|
||||
acquireStateLock and withPlanningLock now retry on transient Docker overlay-fs (ENOENT/EINVAL/EIO) and NFS (ESTALE) errno codes in addition to the existing EPERM/EBUSY; truly fatal codes (EMFILE/ENOSPC/EROFS/EACCES) still throw immediately. Closes #3776.
|
||||
@@ -40,6 +40,23 @@ process.on('exit', () => {
|
||||
}
|
||||
});
|
||||
|
||||
// Transient errno codes that indicate a temporary filesystem condition under
|
||||
// concurrent O_EXCL races — Docker overlay-fs (ENOENT/EINVAL/EIO), NFS
|
||||
// (ESTALE), and OS-level interrupt/retry signals (EAGAIN/EINTR). These are
|
||||
// recoverable; withPlanningLock retries instead of propagating them.
|
||||
// Truly fatal codes (EMFILE, ENOSPC, EROFS, EACCES) are NOT in this set and
|
||||
// will still throw immediately.
|
||||
const PLANNING_LOCK_RETRY_ERRNOS = new Set([
|
||||
'EPERM', // Windows / macOS AV scanner holds the file open during delete
|
||||
'EBUSY', // Windows: file in use by another process
|
||||
'EAGAIN', // POSIX: resource temporarily unavailable
|
||||
'EINTR', // POSIX: syscall interrupted by signal
|
||||
'EINVAL', // Docker overlay-fs: transient during concurrent O_EXCL creation
|
||||
'EIO', // Docker overlay-fs / NFS: transient I/O error
|
||||
'ENOENT', // Docker overlay-fs: parent dir transiently missing during race
|
||||
'ESTALE', // NFS: stale file handle (self-resolves on retry)
|
||||
]);
|
||||
|
||||
function planningDir(cwd, ws, project) {
|
||||
if (project === undefined) project = process.env.GSD_PROJECT || null;
|
||||
if (ws === undefined) ws = process.env.GSD_WORKSTREAM || null;
|
||||
@@ -259,10 +276,10 @@ function withPlanningLock(cwd, fn) {
|
||||
try {
|
||||
return runWithHeldLock();
|
||||
} catch (err) {
|
||||
// EPERM / EBUSY occur transiently on some OS + AV scanner combinations when
|
||||
// the lock file is briefly held open by the deleting process. Treat as EEXIST
|
||||
// (file contention) — wait and retry rather than propagating.
|
||||
if (err.code === 'EPERM' || err.code === 'EBUSY') {
|
||||
// Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners)
|
||||
// are recoverable — wait and retry rather than propagating.
|
||||
// See PLANNING_LOCK_RETRY_ERRNOS for the full list and rationale.
|
||||
if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) {
|
||||
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 100);
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -913,6 +913,23 @@ function syncStateFrontmatter(content, cwd) {
|
||||
return `---\n${yamlStr}\n---\n\n${body}`;
|
||||
}
|
||||
|
||||
// Transient errno codes that indicate a temporary filesystem condition under
|
||||
// concurrent O_EXCL races — Docker overlay-fs (ENOENT/EINVAL/EIO), NFS
|
||||
// (ESTALE), and OS-level interrupt/retry signals (EAGAIN/EINTR). These are
|
||||
// recoverable; acquireStateLock retries instead of propagating them.
|
||||
// Truly fatal codes (EMFILE, ENOSPC, EROFS, EACCES) are NOT in this set and
|
||||
// will still throw immediately.
|
||||
const ACQUIRE_LOCK_RETRY_ERRNOS = new Set([
|
||||
'EPERM', // Windows / macOS AV scanner holds the file open during delete
|
||||
'EBUSY', // Windows: file in use by another process
|
||||
'EAGAIN', // POSIX: resource temporarily unavailable
|
||||
'EINTR', // POSIX: syscall interrupted by signal
|
||||
'EINVAL', // Docker overlay-fs: transient during concurrent O_EXCL creation
|
||||
'EIO', // Docker overlay-fs / NFS: transient I/O error
|
||||
'ENOENT', // Docker overlay-fs: parent dir transiently missing during race
|
||||
'ESTALE', // NFS: stale file handle (self-resolves on retry)
|
||||
]);
|
||||
|
||||
/**
|
||||
* Acquire a lockfile for STATE.md operations.
|
||||
* Returns the lock path for later release.
|
||||
@@ -934,10 +951,10 @@ function acquireStateLock(statePath) {
|
||||
_heldStateLocks.add(lockPath);
|
||||
return lockPath;
|
||||
} catch (err) {
|
||||
// EPERM / EBUSY occur transiently on some OS + AV scanner combinations when
|
||||
// the lock file is briefly held open by the process that is deleting it.
|
||||
// These are recoverable — retry the acquisition loop.
|
||||
if (err.code === 'EPERM' || err.code === 'EBUSY') { continue; }
|
||||
// Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners)
|
||||
// are recoverable — retry the acquisition loop rather than propagating.
|
||||
// See ACQUIRE_LOCK_RETRY_ERRNOS for the full list and rationale.
|
||||
if (ACQUIRE_LOCK_RETRY_ERRNOS.has(err.code)) { continue; }
|
||||
if (err.code !== 'EEXIST') throw err; // propagate — silent bypass causes lost updates
|
||||
// Only unlink a lock we did not place when it has crossed the staleness
|
||||
// threshold (crashed holder). Nuking a fresh lock held by a slow-but-live
|
||||
|
||||
@@ -9,10 +9,16 @@
|
||||
* Regression tests for #3772 — acquireStateLock silently returns false-success
|
||||
* on non-EEXIST openSync errors (EMFILE / EINTR / ENOSPC under load).
|
||||
*
|
||||
* Extended in #3776 to cover Docker overlay-fs and NFS transient errno codes.
|
||||
*
|
||||
* Contract under test:
|
||||
* C1. Non-EEXIST error from fs.openSync → must throw, not return lockPath
|
||||
* C2. Success path (openSync succeeds) → must return lockPath
|
||||
* C3. EEXIST error → retry / wait semantics unchanged (not impacted by this fix)
|
||||
* C4. RETRY_ERRNOS set: EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE must be in the retry allowlist
|
||||
* C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry allowlist
|
||||
* C6. Unknown errno codes must NOT be in the retry allowlist (conservative default)
|
||||
* C7. EPERM/EBUSY must remain in the retry allowlist (from #3773)
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
@@ -124,3 +130,142 @@ describe('acquireStateLock: EEXIST retry semantics unchanged', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// C4. RETRY_ERRNOS set: new Docker/NFS transient codes must be present (#3776)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
/** Extract the ACQUIRE_LOCK_RETRY_ERRNOS Set literal from the source file. */
|
||||
function extractRetryErrnosSource(src) {
|
||||
const constStart = src.indexOf('const ACQUIRE_LOCK_RETRY_ERRNOS');
|
||||
assert.ok(constStart !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS constant must exist in state.cjs');
|
||||
// Extract to the end of the Set(...) constructor — find the closing ]);
|
||||
const setEnd = src.indexOf(']);', constStart);
|
||||
assert.ok(setEnd !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS Set must have closing ]);');
|
||||
return src.slice(constStart, setEnd + 2);
|
||||
}
|
||||
|
||||
describe('acquireStateLock: RETRY_ERRNOS Set contains expected transient codes (#3776)', () => {
|
||||
test('C4a: EAGAIN is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EAGAIN'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EAGAIN (resource temporarily unavailable)');
|
||||
});
|
||||
|
||||
test('C4b: EINTR is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EINTR'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINTR (syscall interrupted)');
|
||||
});
|
||||
|
||||
test('C4c: EINVAL is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EINVAL'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINVAL (Docker overlay-fs transient)');
|
||||
});
|
||||
|
||||
test('C4d: EIO is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EIO'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EIO (Docker overlay-fs / NFS transient)');
|
||||
});
|
||||
|
||||
test('C4e: ENOENT is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'ENOENT'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ENOENT (Docker overlay-fs parent dir transient)');
|
||||
});
|
||||
|
||||
test('C4f: ESTALE is in ACQUIRE_LOCK_RETRY_ERRNOS', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'ESTALE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ESTALE (NFS stale file handle)');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry set
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock: fatal errno codes NOT in RETRY_ERRNOS (#3776)', () => {
|
||||
test('C5a: EMFILE is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (fd limit exhausted — fatal)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(!setBlock.includes("'EMFILE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EMFILE (fatal: fd limit)');
|
||||
});
|
||||
|
||||
test('C5b: ENOSPC is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (disk full — fatal)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(!setBlock.includes("'ENOSPC'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include ENOSPC (fatal: disk full)');
|
||||
});
|
||||
|
||||
test('C5c: EROFS is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (read-only fs — fatal)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(!setBlock.includes("'EROFS'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EROFS (fatal: read-only fs)');
|
||||
});
|
||||
|
||||
test('C5d: EACCES is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (permission denied — fatal)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(!setBlock.includes("'EACCES'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EACCES (fatal: no permission)');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// C6. Unknown errno codes are not in the retry set (conservative default)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock: unknown errno codes not retried (conservative default, #3776)', () => {
|
||||
test('C6: ACQUIRE_LOCK_RETRY_ERRNOS does not include ESOMETHING (unknown code)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(
|
||||
!setBlock.includes("'ESOMETHING'"),
|
||||
'ACQUIRE_LOCK_RETRY_ERRNOS must not include unknown errno ESOMETHING (conservative: surface unknowns)'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// C7. EPERM/EBUSY remain in the retry set (regression guard from #3773)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock: EPERM/EBUSY still in RETRY_ERRNOS (regression guard, #3773)', () => {
|
||||
test('C7a: EPERM is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows / macOS AV scanner)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EPERM'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EPERM (#3773 regression guard)');
|
||||
});
|
||||
|
||||
test('C7b: EBUSY is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows file in use)', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const setBlock = extractRetryErrnosSource(src);
|
||||
assert.ok(setBlock.includes("'EBUSY'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EBUSY (#3773 regression guard)');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// C8. acquireStateLock uses Set-based retry check (not inline literal)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock: uses Set-based retry check (grep-able, not inline literal)', () => {
|
||||
test('C8: retry check uses ACQUIRE_LOCK_RETRY_ERRNOS.has() not hardcoded comparisons', () => {
|
||||
const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8');
|
||||
const fnSrc = extractAcquireStateLockSource(src);
|
||||
|
||||
// Must use the named Set for the check inside the function
|
||||
assert.ok(
|
||||
fnSrc.includes('ACQUIRE_LOCK_RETRY_ERRNOS.has('),
|
||||
'acquireStateLock catch block must use ACQUIRE_LOCK_RETRY_ERRNOS.has() for retry decision'
|
||||
);
|
||||
|
||||
// Must NOT have the old inline EPERM/EBUSY literal check
|
||||
const oldPattern = /err\.code\s*===\s*['"]EPERM['"]\s*\|\|\s*err\.code\s*===\s*['"]EBUSY['"]/;
|
||||
assert.ok(
|
||||
!oldPattern.test(fnSrc),
|
||||
'acquireStateLock must not use old inline EPERM||EBUSY check (should use ACQUIRE_LOCK_RETRY_ERRNOS.has())'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user