Skip to content

Commit 05d679e

Browse files
kriszypclaude
andcommitted
fix: ownershipOf's stat catch conflated absent with inconclusive
Flagged independently by both gemini-code-assist and the CI claude reviewer bot on the pushed PR: the bare `catch { return 'absent' }` in ownershipOf() treated every statSync failure — not just ENOENT — as "nothing is on disk." A permission or I/O error on a path a live foreign worker actually owns would misclassify as 'absent' rather than 'foreign', and markUdsBindFailed()'s guard only blocks on 'foreign' — so an inconclusive stat error could make it delete a live worker's yaml metadata, reintroducing the bug class this PR exists to fix. cleanupUdsFiles() is unaffected (it treats 'absent' and 'foreign' identically), but the distinction matters wherever 'absent' alone is trusted. Now checks error.code === 'ENOENT' specifically; any other error returns 'foreign' (the conservative default) instead. Added a test forcing a non-ENOENT stat failure deterministically via ENOTDIR (treating a plain file as a path segment) rather than EACCES, since permission checks aren't reliably enforced when tests run as root (common in CI containers). Co-Authored-By: Claude Opus <noreply@anthropic.com>
1 parent beead41 commit 05d679e

2 files changed

Lines changed: 25 additions & 4 deletions

File tree

server/http.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,11 @@ export function recordUdsBindSuccess(socketPath: string) {
110110
/**
111111
* 'owned' — the (dev, inode) this worker recorded at bind time still matches what's on disk.
112112
* 'foreign' — a socket exists at the path, but with a different identity (or none was ever
113-
* recorded): something else — almost always a replacement worker — currently owns it.
114-
* 'absent' — nothing is on disk at the path at all: there is no live owner to protect.
113+
* recorded): something else — almost always a replacement worker — currently owns it. Also the
114+
* safe default when statSync fails for any reason other than ENOENT (e.g. EACCES): we can't
115+
* confirm the path is genuinely empty, so treat it as though something we can't verify owns it.
116+
* 'absent' — statSync confirmed ENOENT: nothing is on disk at the path. There is no live owner to
117+
* protect.
115118
*/
116119
function ownershipOf(entry: {
117120
socketPath: string;
@@ -121,8 +124,8 @@ function ownershipOf(entry: {
121124
try {
122125
const stat = statSync(entry.socketPath, { bigint: true });
123126
current = { dev: stat.dev, ino: stat.ino };
124-
} catch {
125-
return 'absent';
127+
} catch (error) {
128+
return (error as NodeJS.ErrnoException).code === 'ENOENT' ? 'absent' : 'foreign';
126129
}
127130
return entry.identity !== undefined && current.dev === entry.identity.dev && current.ino === entry.identity.ino
128131
? 'owned'

unitTests/server/udsMirror.test.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,24 @@ describe('UDS mirror (writeUdsMetadata, cleanup helpers)', () => {
625625
assert.ok(fs.existsSync(yamlPath), "a live socket this worker never bound means its yaml isn't ours to delete");
626626
});
627627

628+
it('leaves the yaml in place when stat fails for a reason other than ENOENT', () => {
629+
// A stat failure that isn't "the file doesn't exist" (permissions, I/O, etc.) means we
630+
// can't confirm the path is genuinely empty — treat it like 'foreign', not 'absent', so a
631+
// transient stat error can't be misread as license to delete a live owner's yaml. Forced
632+
// deterministically via ENOTDIR (treating a plain file as a path segment) rather than
633+
// EACCES, which isn't reliably enforced when tests run as root (common in CI containers).
634+
const blockingFile = path.join(TEST_SOCKETS_DIR, 'not-a-directory');
635+
const sockPath = path.join(blockingFile, 'nested.sock');
636+
const yamlPath = path.join(TEST_SOCKETS_DIR, 'enotdir-failed.yaml');
637+
fs.writeFileSync(blockingFile, '');
638+
fs.writeFileSync(yamlPath, 'stale: metadata\n');
639+
registerUdsCleanupPaths(sockPath, yamlPath);
640+
641+
markUdsBindFailed(sockPath);
642+
643+
assert.ok(fs.existsSync(yamlPath), 'an inconclusive stat error must not be treated as a safe-to-retract absence');
644+
});
645+
628646
it('leaves the yaml in place when a replacement worker now owns the socket at the same path', () => {
629647
const sockPath = path.join(TEST_SOCKETS_DIR, 'replaced-failed.sock');
630648
const yamlPath = path.join(TEST_SOCKETS_DIR, 'replaced-failed.yaml');

0 commit comments

Comments
 (0)