Merge pull request #2054 from Tibsfox/fix/cache-state-frontmatter-1967
perf(state): cache buildStateFrontmatter disk scan per process
This commit is contained in:
Tom Boucher
@@ -7,6 +7,11 @@ const path = require('path');
|
||||
const { escapeRegex, loadConfig, getMilestoneInfo, getMilestonePhaseFilter, normalizeMd, planningDir, planningPaths, output, error, atomicWriteFileSync } = require('./core.cjs');
|
||||
const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cjs');
|
||||
|
||||
// Cache disk scan results from buildStateFrontmatter per cwd per process (#1967).
|
||||
// Avoids re-reading N+1 directories on every state write when the phase structure
|
||||
// hasn't changed within the same gsd-tools invocation.
|
||||
const _diskScanCache = new Map();
|
||||
|
||||
/** Shorthand — every state command needs this path */
|
||||
function getStatePath(cwd) {
|
||||
return planningPaths(cwd).state;
|
||||
@@ -737,28 +742,40 @@ function buildStateFrontmatter(bodyContent, cwd) {
|
||||
try {
|
||||
const phasesDir = planningPaths(cwd).phases;
|
||||
if (fs.existsSync(phasesDir)) {
|
||||
const isDirInMilestone = getMilestonePhaseFilter(cwd);
|
||||
const phaseDirs = fs.readdirSync(phasesDir, { withFileTypes: true })
|
||||
.filter(e => e.isDirectory()).map(e => e.name)
|
||||
.filter(isDirInMilestone);
|
||||
let diskTotalPlans = 0;
|
||||
let diskTotalSummaries = 0;
|
||||
let diskCompletedPhases = 0;
|
||||
// Use cached disk scan when available — avoids N+1 readdirSync calls
|
||||
// on repeated buildStateFrontmatter invocations within the same process (#1967)
|
||||
let cached = _diskScanCache.get(cwd);
|
||||
if (!cached) {
|
||||
const isDirInMilestone = getMilestonePhaseFilter(cwd);
|
||||
const phaseDirs = fs.readdirSync(phasesDir, { withFileTypes: true })
|
||||
.filter(e => e.isDirectory()).map(e => e.name)
|
||||
.filter(isDirInMilestone);
|
||||
let diskTotalPlans = 0;
|
||||
let diskTotalSummaries = 0;
|
||||
let diskCompletedPhases = 0;
|
||||
|
||||
for (const dir of phaseDirs) {
|
||||
const files = fs.readdirSync(path.join(phasesDir, dir));
|
||||
const plans = files.filter(f => f.match(/-PLAN\.md$/i)).length;
|
||||
const summaries = files.filter(f => f.match(/-SUMMARY\.md$/i)).length;
|
||||
diskTotalPlans += plans;
|
||||
diskTotalSummaries += summaries;
|
||||
if (plans > 0 && summaries >= plans) diskCompletedPhases++;
|
||||
for (const dir of phaseDirs) {
|
||||
const files = fs.readdirSync(path.join(phasesDir, dir));
|
||||
const plans = files.filter(f => f.match(/-PLAN\.md$/i)).length;
|
||||
const summaries = files.filter(f => f.match(/-SUMMARY\.md$/i)).length;
|
||||
diskTotalPlans += plans;
|
||||
diskTotalSummaries += summaries;
|
||||
if (plans > 0 && summaries >= plans) diskCompletedPhases++;
|
||||
}
|
||||
cached = {
|
||||
totalPhases: isDirInMilestone.phaseCount > 0
|
||||
? Math.max(phaseDirs.length, isDirInMilestone.phaseCount)
|
||||
: phaseDirs.length,
|
||||
completedPhases: diskCompletedPhases,
|
||||
totalPlans: diskTotalPlans,
|
||||
completedPlans: diskTotalSummaries,
|
||||
};
|
||||
_diskScanCache.set(cwd, cached);
|
||||
}
|
||||
totalPhases = isDirInMilestone.phaseCount > 0
|
||||
? Math.max(phaseDirs.length, isDirInMilestone.phaseCount)
|
||||
: phaseDirs.length;
|
||||
completedPhases = diskCompletedPhases;
|
||||
totalPlans = diskTotalPlans;
|
||||
completedPlans = diskTotalSummaries;
|
||||
totalPhases = cached.totalPhases;
|
||||
completedPhases = cached.completedPhases;
|
||||
totalPlans = cached.totalPlans;
|
||||
completedPlans = cached.completedPlans;
|
||||
}
|
||||
} catch { /* intentionally empty */ }
|
||||
}
|
||||
@@ -904,6 +921,10 @@ function releaseStateLock(lockPath) {
|
||||
* each other's changes (race condition with read-modify-write cycle).
|
||||
*/
|
||||
function writeStateMd(statePath, content, cwd) {
|
||||
// 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);
|
||||
try {
|
||||
|
||||
100
tests/bug-1967-cache-invalidation.test.cjs
Normal file
100
tests/bug-1967-cache-invalidation.test.cjs
Normal file
@@ -0,0 +1,100 @@
|
||||
/**
|
||||
* Regression tests for #1967 cache invalidation.
|
||||
*
|
||||
* The disk scan cache in buildStateFrontmatter must be invalidated on
|
||||
* writeStateMd to prevent stale reads if multiple state-mutating
|
||||
* operations occur within the same Node process. This matters for:
|
||||
* - SDK callers that require() gsd-tools.cjs as a module
|
||||
* - Future dispatcher extensions that handle compound operations
|
||||
* - Tests that import state.cjs directly
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
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 state = require('../get-shit-done/bin/lib/state.cjs');
|
||||
|
||||
describe('buildStateFrontmatter cache invalidation (#1967)', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let phasesDir;
|
||||
let statePath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1967-cache-'));
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
phasesDir = path.join(planningDir, 'phases');
|
||||
fs.mkdirSync(phasesDir, { recursive: true });
|
||||
|
||||
// Create a minimal config and STATE.md
|
||||
fs.writeFileSync(
|
||||
path.join(planningDir, 'config.json'),
|
||||
JSON.stringify({ project_code: 'TEST' })
|
||||
);
|
||||
|
||||
statePath = path.join(planningDir, 'STATE.md');
|
||||
fs.writeFileSync(statePath, [
|
||||
'# State',
|
||||
'',
|
||||
'**Current Phase:** 1',
|
||||
'**Status:** executing',
|
||||
'**Total Phases:** 2',
|
||||
'',
|
||||
].join('\n'));
|
||||
|
||||
// Start with one phase directory containing one PLAN
|
||||
const phase1 = path.join(phasesDir, '01-foo');
|
||||
fs.mkdirSync(phase1);
|
||||
fs.writeFileSync(path.join(phase1, '01-1-PLAN.md'), '---\nphase: 1\nplan: 1\n---\n# Plan\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test('writeStateMd invalidates cache so subsequent reads see new disk state', () => {
|
||||
// First write — populates cache via buildStateFrontmatter
|
||||
const content1 = fs.readFileSync(statePath, 'utf-8');
|
||||
state.writeStateMd(statePath, content1, tmpDir);
|
||||
|
||||
// Create a NEW phase directory AFTER the first write
|
||||
// Without cache invalidation, the second write would still see only 1 phase
|
||||
const phase2 = path.join(phasesDir, '02-bar');
|
||||
fs.mkdirSync(phase2);
|
||||
fs.writeFileSync(path.join(phase2, '02-1-PLAN.md'), '---\nphase: 2\nplan: 1\n---\n# Plan\n');
|
||||
fs.writeFileSync(path.join(phase2, '02-1-SUMMARY.md'), '---\nstatus: complete\n---\n# Summary\n');
|
||||
|
||||
// Second write in the SAME process — must see the new phase
|
||||
const content2 = fs.readFileSync(statePath, 'utf-8');
|
||||
state.writeStateMd(statePath, content2, tmpDir);
|
||||
|
||||
// Read back and parse frontmatter to verify it reflects 2 phases, not 1
|
||||
const result = fs.readFileSync(statePath, 'utf-8');
|
||||
const fmMatch = result.match(/^---\n([\s\S]*?)\n---/);
|
||||
assert.ok(fmMatch, 'STATE.md should have frontmatter after writeStateMd');
|
||||
|
||||
const fm = fmMatch[1];
|
||||
// Should show 2 total phases (the new disk state), not 1 (stale cache)
|
||||
const totalPhasesMatch = fm.match(/total_phases:\s*(\d+)/);
|
||||
assert.ok(totalPhasesMatch, 'frontmatter should contain total_phases');
|
||||
assert.strictEqual(
|
||||
parseInt(totalPhasesMatch[1], 10),
|
||||
2,
|
||||
'total_phases should reflect new disk state (2), not stale cache (1)'
|
||||
);
|
||||
|
||||
// Should show 1 completed phase (phase 2 has SUMMARY)
|
||||
const completedMatch = fm.match(/completed_phases:\s*(\d+)/);
|
||||
assert.ok(completedMatch, 'frontmatter should contain completed_phases');
|
||||
assert.strictEqual(
|
||||
parseInt(completedMatch[1], 10),
|
||||
1,
|
||||
'completed_phases should reflect new disk state (1 complete), not stale cache (0)'
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user