From 40ce87bb2ca9fa21f8a823076faf28721cad222d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:00:28 -0700 Subject: [PATCH] fix(daemon): warn on non-atomic pid-publish degrade, correct reap comment, run claim-reap contract on Windows CI --- .github/workflows/pr.yml | 1 + config/scripts/pr-code-change-scope.mjs | 1 + .../daemon/daemon-pid-publish-atomicity.test.ts | 12 ++++++++++++ src/main/daemon/daemon-pid-publish-claim-reap.ts | 6 ++++-- src/main/daemon/daemon-spawner.ts | 14 +++++++++++++- 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 8cfafaa2734..7f7eb37d233 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -707,6 +707,7 @@ jobs: src/main/runtime/worktree-scan-admin-fingerprint-gate.test.ts src/shared/secure-file-fsync-flags.test.ts src/main/daemon/daemon-pid-publish-disk-contract.test.ts + src/main/daemon/daemon-pid-publish-claim-reap.test.ts src/main/ipc/pty-codex-account-attribution.test.ts src/main/ipc/pty-spawn-env-codex-resume-provenance.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index d470639c558..b7f0794125f 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -196,6 +196,7 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/main/runtime/worktree-scan-admin-fingerprint-gate.test.ts', 'src/shared/secure-file-fsync-flags.test.ts', 'src/main/daemon/daemon-pid-publish-disk-contract.test.ts', + 'src/main/daemon/daemon-pid-publish-claim-reap.test.ts', 'src/main/ipc/pty-codex-account-attribution.test.ts', 'src/main/ipc/pty-spawn-env-codex-resume-provenance.test.ts' ] diff --git a/src/main/daemon/daemon-pid-publish-atomicity.test.ts b/src/main/daemon/daemon-pid-publish-atomicity.test.ts index adaa2f96d6a..207a4d0e8f7 100644 --- a/src/main/daemon/daemon-pid-publish-atomicity.test.ts +++ b/src/main/daemon/daemon-pid-publish-atomicity.test.ts @@ -55,15 +55,19 @@ function listPublishScratch(): string[] { return readdirSync(dir).filter((name) => name !== 'daemon-v1.pid') } +let warnSpy: ReturnType + beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'daemon-pid-atomic-')) pidPath = join(dir, 'daemon-v1.pid') fsFaults.tornWriteBytes = null fsFaults.linkError = null fsFaults.events = [] + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) }) afterEach(() => { + warnSpy.mockRestore() rmSync(dir, { recursive: true, force: true }) }) @@ -100,6 +104,9 @@ describe('publishDaemonPidFile atomicity', () => { expect(parseDaemonPidFile(readFileSync(pidPath, 'utf8'))).toMatchObject(RECORD) expect(listPublishScratch()).toEqual([]) + // The atomic path is the normal one; warning on it would train operators to ignore + // the degrade warning below. + expect(warnSpy).not.toHaveBeenCalled() }) it('flushes the record to disk before it becomes visible at the canonical path', () => { @@ -132,6 +139,11 @@ describe('publishDaemonPidFile atomicity', () => { expect(parseDaemonPidFile(readFileSync(pidPath, 'utf8'))).toMatchObject(RECORD) expect(listPublishScratch()).toEqual([]) + // The degrade must be operator-visible: on a volume without hard links every publish + // silently loses torn-write protection otherwise. + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('ENOTSUP')) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('non-atomic')) }) it('keeps exclusive-create semantics on the degraded path too', () => { diff --git a/src/main/daemon/daemon-pid-publish-claim-reap.ts b/src/main/daemon/daemon-pid-publish-claim-reap.ts index 3f34d75d7b9..58cfdb75cdb 100644 --- a/src/main/daemon/daemon-pid-publish-claim-reap.ts +++ b/src/main/daemon/daemon-pid-publish-claim-reap.ts @@ -13,8 +13,10 @@ const PUBLISH_CLAIM_PATTERN = /^daemon-v\d+\.pid\.publish-(\d+)-[0-9a-f-]+$/ /** * Why proven death and not age: a claim is only scratch — its content was never canonical — - * but deleting a LIVE publisher's claim between its write and its link would fail that - * publish. Only ESRCH proves the owner is gone; EPERM means it exists under another user, + * but deleting a LIVE publisher's claim between its write and its link knocks that publish + * off the atomic path: the link fails non-EEXIST and degrades to the exclusive direct + * write, which succeeds but reopens the torn-write window this discipline exists to close. + * Only ESRCH proves the owner is gone; EPERM means it exists under another user, * and pid recycling merely preserves junk a little longer, which is the safe direction. */ function claimOwnerIsProvenDead(pid: number): boolean { diff --git a/src/main/daemon/daemon-spawner.ts b/src/main/daemon/daemon-spawner.ts index 17d6ea6b093..644122a8902 100644 --- a/src/main/daemon/daemon-spawner.ts +++ b/src/main/daemon/daemon-spawner.ts @@ -160,6 +160,12 @@ function isExistingFileError(error: unknown): boolean { return typeof error === 'object' && error !== null && 'code' in error && error.code === 'EEXIST' } +function describeErrnoCode(error: unknown): string { + return typeof error === 'object' && error !== null && 'code' in error + ? String(error.code) + : 'no errno' +} + /** * Scratch for the atomic publish below. Distinct from the swap/hold claims: those can hold * the ONLY copy of a live record and must be restored, while publish scratch holds a record @@ -190,7 +196,13 @@ export function publishDaemonPidFile(pidPath: string, pidFile: DaemonPidFile): v } // Why: hard links can be unsupported on exotic volumes. The exclusive direct write // is exactly the pre-atomic behavior, so those filesystems keep a working daemon - // instead of trading a torn-write window for no pid record at all. + // instead of trading a torn-write window for no pid record at all. Warn because the + // degrade is otherwise invisible: an operator on such a volume (network mount, + // container overlay) should be able to tell the atomic path is not in use. + console.warn( + `[daemon] pid-record hard-link publish failed (${describeErrnoCode(error)}); ` + + 'degrading to the non-atomic exclusive write — torn-write protection is off for this record' + ) writeFileSync(pidPath, serializeDaemonPidFile(pidFile), { mode: 0o600, flag: 'wx' }) } } finally {