From 2fe17cb87df4901aa4093ff89323500a84a072ac Mon Sep 17 00:00:00 2001 From: Dave Date: Sun, 21 Jun 2026 10:38:24 -0400 Subject: [PATCH] fix(core): writeStateMd must scan inside the lock (M8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: writeStateMd computed its frontmatter disk scan (syncStateFrontmatter — the READ half of a read-modify-write) BEFORE acquireStateLock, leaving a TOCTOU window. A concurrent writer that committed a new PLAN/SUMMARY between our scan and our lock made writeStateMd stamp stale progress counts (lost update — the #500/#905/#1230 family). The atomic sibling readModifyWriteStateMd already scans inside its lock. Fix: move _diskScanCache.delete + syncStateFrontmatter inside the acquireStateLock-held try, before platformWriteSync. Byte-for-behaviour identical for single-threaded callers — only the concurrent-writer window closes. Adds an afterAcquire test seam (mirrors the M1 _setLockProbes seam) to make the window deterministic; new test proves RED (stale count) before the reorder and GREEN after. Source of truth src/state.cts (ADR-457); bin/lib/state.cjs is generated. Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz --- src/state.cts | 47 ++++++- .../m8-writestatemd-scan-after-lock.test.cjs | 124 ++++++++++++++++++ 2 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 tests/m8-writestatemd-scan-after-lock.test.cjs diff --git a/src/state.cts b/src/state.cts index 45f40fba2..8e494b43f 100644 --- a/src/state.cts +++ b/src/state.cts @@ -177,6 +177,24 @@ function _realIsPidAlive(pid: number): boolean { const _stateLockProbes: { isPidAlive: (pid: number) => boolean } = { isPidAlive: _realIsPidAlive }; +// --------------------------------------------------------------------------- +// State-lock test hooks (test seam) — audit M8 +// +// M8 (scan-before-lock TOCTOU in writeStateMd) is a concurrency issue a single- +// threaded test cannot otherwise observe. The afterAcquire hook makes the +// failure window deterministic (mirrors the M1 _setLockProbes seam above): +// +// afterAcquire(lockPath) — fired inside writeStateMd immediately AFTER the lock +// is acquired. A test can mutate the disk here (simulate a concurrent writer +// landing in the scan→lock window) to prove the disk scan runs INSIDE the lock. +// +// All hooks default to no-ops; real callers are byte-for-behaviour unchanged. +// --------------------------------------------------------------------------- +interface StateLockTestHooks { + afterAcquire?: (lockPath: string) => void; +} +const _stateLockTestHooks: StateLockTestHooks = {}; + function _stateLockIsPidAlive(pid: number): boolean { return _stateLockProbes.isPidAlive(pid); } @@ -1749,13 +1767,24 @@ function withStateLock(statePath: string, fn: () => T): T { * Optional clock seam; defaults to realClock. Passed through to acquireStateLock. */ function writeStateMd(statePath: string, content: string, cwd?: string, clock?: StateLockClock): void { - // Invalidate disk scan cache before computing new frontmatter — the write - // may create new PLAN/SUMMARY files that buildStateFrontmatter must see. - // Safe for any calling pattern, not just short-lived CLI processes (#1967). - if (cwd) _diskScanCache.delete(cwd); - const synced = syncStateFrontmatter(content, cwd); const lockPath = acquireStateLock(statePath, clock); + // Test seam (audit M8): fire AFTER the lock is taken so a test can simulate a + // concurrent writer landing in the (now-closed) scan→lock window. + if (_stateLockTestHooks.afterAcquire) _stateLockTestHooks.afterAcquire(lockPath); try { + // Audit M8 (leaky-abstractions): the disk scan that counts PLAN/SUMMARY files + // to build the frontmatter is the READ half of this read-modify-write — it must + // run INSIDE the lock (mirroring readModifyWriteStateMd), not before it. Scanning + // before acquireStateLock left a TOCTOU window where a concurrent writer that + // committed a new PLAN/SUMMARY between our scan and our lock made writeStateMd + // stamp STALE progress counts (lost update — the #500/#905/#1230 family). The + // scan order is otherwise byte-for-behaviour identical for single-threaded + // callers — only the concurrent-writer window closes. + // + // Invalidate the disk scan cache first — the write may create new PLAN/SUMMARY + // files that buildStateFrontmatter must see (#1967). + if (cwd) _diskScanCache.delete(cwd); + const synced = syncStateFrontmatter(content, cwd); platformWriteSync(statePath, synced); } finally { releaseStateLock(lockPath); @@ -2959,4 +2988,12 @@ export = { _resetLockProbes(): void { _stateLockProbes.isPidAlive = _realIsPidAlive; }, + // Test seam (audit M8): inject the deterministic scan-in-lock hook (afterAcquire). + // See _stateLockTestHooks. + _setStateLockTestHooks(hooks: StateLockTestHooks): void { + if ('afterAcquire' in hooks) _stateLockTestHooks.afterAcquire = hooks.afterAcquire; + }, + _resetStateLockTestHooks(): void { + delete _stateLockTestHooks.afterAcquire; + }, }; diff --git a/tests/m8-writestatemd-scan-after-lock.test.cjs b/tests/m8-writestatemd-scan-after-lock.test.cjs new file mode 100644 index 000000000..a5c87cd09 --- /dev/null +++ b/tests/m8-writestatemd-scan-after-lock.test.cjs @@ -0,0 +1,124 @@ +'use strict'; +// allow-test-rule: architectural-invariant +// writeStateMd's "scan happens INSIDE the lock" property is a concurrency invariant. +// A single-threaded test cannot observe the difference between scan-before-lock and +// scan-after-lock unless something mutates the disk in the window between the two. +// The afterAcquire test hook (fired inside writeStateMd right after the lock is +// taken) is the deterministic seam that simulates a concurrent writer landing in +// exactly that window — the only level at which the TOCTOU is observable. + +/** + * M8 — writeStateMd scans the disk (syncStateFrontmatter / PLAN-SUMMARY count) + * BEFORE taking the lock, so a concurrent writer that commits a new PLAN/SUMMARY + * between our scan and our lock acquisition makes writeStateMd stamp STALE + * progress counts (a lost-update of the frontmatter progress block). + * readModifyWriteStateMd (the atomic variant) correctly scans INSIDE its lock — + * this non-atomic variant was the outlier. + * + * Deterministic repro (no wall-clock, no threads): the afterAcquire test hook + * fires inside writeStateMd immediately after the lock is acquired and adds a + * second PLAN file to the phase dir — simulating a concurrent writer who landed + * in the scan→lock window. The written frontmatter's progress.total_plans then + * reveals whether the scan ran before the hook (stale: 1) or after it (fresh: 2). + * + * RED (pre-fix): scan runs BEFORE acquire → before the hook → total_plans = 1. + * GREEN (post-fix): scan runs AFTER acquire → after the hook → total_plans = 2. + * + * Recurring closed family this guards: #500 / #905 / #1230 (STATE.md write + * corruption). #453 deleted the flaky race tests in favor of seams, so this exact + * path was under-tested — the hook restores deterministic coverage. + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +const stateMod = require('../gsd-core/bin/lib/state.cjs'); +const { writeStateMd } = stateMod; +const { cleanup } = require('./helpers.cjs'); + +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +const MINIMAL_STATE_MD = [ + '# Project State', + '', + '**Status:** Planning', + '**Current Phase:** 01', +].join('\n') + '\n'; + +/** Parse progress.total_plans out of the STATE.md frontmatter block. */ +function readTotalPlans(statePath) { + const written = fs.readFileSync(statePath, 'utf-8'); + const fmMatch = written.match(/^---\r?\n([\s\S]*?)\r?\n---/); + assert.ok(fmMatch, 'STATE.md must have a frontmatter block after writeStateMd'); + const m = fmMatch[1].match(/total_plans:\s*(\d+)/); + assert.ok(m, 'frontmatter must carry a progress.total_plans line'); + return parseInt(m[1], 10); +} + +// ───────────────────────────────────────────────────────────────────────────── +// M8 — afterAcquire hook proves the scan runs INSIDE the lock +// ───────────────────────────────────────────────────────────────────────────── + +describe('M8: writeStateMd scans disk AFTER acquiring the lock (scan-in-lock)', () => { + let tmpDir; + let statePath; + let phaseDir; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-m8-')); + const planningDir = path.join(tmpDir, '.planning'); + phaseDir = path.join(planningDir, 'phases', '01-init'); + fs.mkdirSync(phaseDir, { recursive: true }); + // Start with exactly ONE plan file on disk. + fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan 01\n'); + statePath = path.join(planningDir, 'STATE.md'); + fs.writeFileSync(statePath, MINIMAL_STATE_MD); + }); + + afterEach(() => { + stateMod._resetStateLockTestHooks(); + try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ } + cleanup(tmpDir); + }); + + test('a PLAN added in the post-acquire window is reflected in the written progress count', () => { + // The hook simulates a concurrent writer who commits a second PLAN file in the + // window between scan and lock. It MUST be observed only if the scan runs after + // the lock (and therefore after this hook fires). + let fired = 0; + stateMod._setStateLockTestHooks({ + afterAcquire() { + fired++; + fs.writeFileSync(path.join(phaseDir, '02-PLAN.md'), '# Plan 02\n'); + }, + }); + + writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir); + + assert.equal(fired, 1, 'afterAcquire hook must fire exactly once inside writeStateMd'); + + const totalPlans = readTotalPlans(statePath); + // RED pre-fix: scan ran before the hook → counts only 01-PLAN.md → 1. + // GREEN post-fix: scan ran after the hook → counts both PLANs → 2. + assert.equal( + totalPlans, 2, + 'writeStateMd must scan the disk INSIDE the lock (after the concurrent ' + + 'writer landed), stamping total_plans=2 — not the stale pre-lock count of 1' + ); + }); + + test('single-threaded callers (no hook) are byte-for-behaviour unchanged: count = 1', () => { + // Regression guard: with no concurrent writer (hook unset), the count must be + // exactly the on-disk truth — the fix must NOT change the uncontended result. + writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir); + assert.equal( + readTotalPlans(statePath), 1, + 'uncontended writeStateMd must stamp the real on-disk plan count (1)' + ); + }); +});