diff --git a/.changeset/vivid-pumas-roam.md b/.changeset/vivid-pumas-roam.md new file mode 100644 index 000000000..2c3ca5a80 --- /dev/null +++ b/.changeset/vivid-pumas-roam.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4313 +--- +**Update-check cache is now published atomically** — the statusline update segment no longer intermittently goes blank when several runtimes (Claude Code, Codex, Cursor, ...) share one machine. (#4091) diff --git a/hooks/gsd-check-update-worker.js b/hooks/gsd-check-update-worker.js index d6c1871a6..5ae7f4287 100644 --- a/hooks/gsd-check-update-worker.js +++ b/hooks/gsd-check-update-worker.js @@ -156,5 +156,22 @@ const result = { }; if (cacheFile) { - try { fs.writeFileSync(cacheFile, JSON.stringify(result)); } catch (e) {} -} + // #4091: the cache file is shared per-PACKAGE across every runtime's worker + // (#607/#1421), so concurrent statusline/banner readers parse it while this + // worker writes it. A direct writeFileSync truncates before writing — a + // reader landing mid-write sees a torn/empty record (its JSON.parse catch + // swallows it, so the symptom is an intermittently blank update segment). + // Publish atomically instead: stage under a unique same-directory temp + // (same filesystem, so rename(2) is atomic — readers see the old or the new + // record, never a partial one), then renameSync into place. Failure policy + // is unchanged (#3582 degrade): any error is swallowed and the temp, if + // left behind, is best-effort removed. + const tmp = cacheFile + '.tmp-' + process.pid; + try { + fs.writeFileSync(tmp, JSON.stringify(result)); + fs.renameSync(tmp, cacheFile); + } catch (e) { + try { + fs.rmSync(tmp, { force: true }); + } catch (e2) {} + }} diff --git a/tests/gsd-check-update-worker-atomic-cache.test.cjs b/tests/gsd-check-update-worker-atomic-cache.test.cjs new file mode 100644 index 000000000..b129ab141 --- /dev/null +++ b/tests/gsd-check-update-worker-atomic-cache.test.cjs @@ -0,0 +1,118 @@ +/** + * Tests for the atomic cache publish in gsd-check-update-worker.js (#4091). + * + * Background (issue #4091): + * The worker used to write its result cache in place + * (`fs.writeFileSync(cacheFile, ...)`), which truncates before writing. The + * cache file is deliberately per-PACKAGE (#607/#1421), so on a machine with + * several runtimes (Claude Code + Codex + Cursor, ...) every runtime's + * SessionStart worker writes the SAME file while every statusline/banner + * refresh reads it. A reader landing mid-write saw an empty or truncated + * record; the readers' JSON.parse catch swallowed it, so the symptom was an + * intermittently blank update segment. + * + * The fix publishes via write-temp-then-rename — rename(2) over a file in the + * same directory is atomic, so readers see either the old or the new record, + * never a torn one. + * + * Source-grep policy: the torn write is a concurrency defect that cannot be + * observed on POSIX CI without a genuine multi-process race (forbidden by + * repo test rules), so the contract is asserted structurally on the write + * shape — same policy as the #3103 platform-gate tests in + * gsd-check-update-worker-platform-gate.test.cjs (structural-regression-guard). + */ + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const WORKER_PATH = path.join(__dirname, '..', 'hooks', 'gsd-check-update-worker.js'); + +// allow-test-rule: structural-regression-guard (#4091) +// Feeds the real worker source (readFileSync) into the structural assertions +// below. The behavior it guards — rename-atomic publish of a file shared +// across runtime processes — needs a multi-process race to observe, which +// repo test rules forbid, so a structural assertion on the write shape is +// the minimum-cost contract (same policy as the #3103 platform gate). +function codeOnly(file) { + return fs.readFileSync(file, 'utf8') + // eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own bounded hooks source, not adversarial input + .replace(/\/\*[\s\S]*?\*\//g, '') + // eslint-disable-next-line local/no-unbounded-quantifier -- parses this repo's own hooks source, not adversarial input + .replace(/(^|[^:])\/\/[^\r\n]*/g, '$1'); +} + +describe('gsd-check-update-worker.js: atomic cache publish (#4091)', () => { + test('worker must not write the shared cache file in place', () => { + const src = codeOnly(WORKER_PATH); + // The only legitimate direct write target is a temp stage; the cache file + // itself must never be the writeFileSync destination. + assert.ok( + !/writeFileSync\(\s*cacheFile\b/.test(src), + 'writeFileSync(cacheFile, ...) truncates the shared per-package cache in place; ' + + 'concurrent readers see a torn/empty record (#4091). Stage a temp file and rename it.', + ); + }); + + test('worker publishes the cache via rename-atomic replace (torn-write guard #4091)', () => { + const src = codeOnly(WORKER_PATH); + // Stage: write a unique temp derived from cacheFile (same directory => + // same filesystem => POSIX rename(2) atomicity). + assert.match( + src, + /const\s+\w+\s*=\s*cacheFile\s*\+\s*['"][^'"]*tmp[^'"]*['"]/, + 'cache publish must stage a temp path derived from cacheFile (same-directory rename is atomic)', + ); + // Publish: renameSync(tmp, cacheFile). + assert.match( + src, + /renameSync\(\s*\w+\s*,\s*cacheFile\s*\)/, + 'cache publish must rename the staged temp into place, never write the cache file directly', + ); + }); + + test('temp stage path is same-directory by construction', () => { + const src = codeOnly(WORKER_PATH); + // A cross-filesystem temp (os.tmpdir()) would make the rename non-atomic + // (copy+unlink fallback on some platforms) — rejected in #4091 diagnosis. + assert.ok( + !/os\.tmpdir\(\)/.test(src), + 'temp stage must be derived from cacheFile (same directory), not os.tmpdir()', + ); + assert.match( + src, + /cacheFile\s*\+\s*['"][^'"]*['"]\s*\+\s*process\.pid/, + 'temp stage name must be unique per process (process.pid) so concurrent workers never share a stage path', + ); + }); + + // Behavioral end-to-end: the worker (run as a real child process, same seam + // as the #3582 cold-tree test) must leave a parseable cache and no temp + // residue. Guards the happy path of the rename publish. + test('worker run leaves a valid cache and no temp residue', (t) => { + const cacheDir = createTempDir('gsd-worker-atomic-'); + t.after(() => cleanup(cacheDir)); + const cacheFile = path.join(cacheDir, 'cache.json'); + // Pre-existing record from "another runtime" — the rename must replace it, + // not truncate it (readers between runs always see a complete record). + fs.writeFileSync(cacheFile, JSON.stringify({ update_available: false, installed: '0.0.1', latest: 'unknown', checked: 1, package_name: null }), 'utf8'); + + const env = { + ...process.env, + GSD_CACHE_FILE: cacheFile, + GSD_PROJECT_VERSION_FILE: path.join(cacheDir, 'no-such-project', 'VERSION'), + GSD_GLOBAL_VERSION_FILE: path.join(cacheDir, 'no-such-global', 'VERSION'), + }; + const r = runHookSeam(WORKER_PATH, [], { env, timeoutMs: 15000 }); + assert.equal(r.exitCode, 0, `worker must exit 0; stderr: ${r.stderr}`); + const cache = JSON.parse(fs.readFileSync(cacheFile, 'utf8')); + assert.equal(cache.installed, '0.0.0', 'worker record replaced the pre-existing one'); + const residue = fs.readdirSync(cacheDir).filter((f) => f.startsWith('cache.json.tmp') || f.includes('.tmp-')); + assert.deepEqual(residue, [], 'no temp stage files may remain after a successful publish'); + }); +});