fix(core): add atomicWriteFileSync to prevent truncated files on kill (#1943)
Replaces direct fs.writeFileSync calls for STATE.md, ROADMAP.md, and config.json with write-to-temp-then-rename so a process killed mid-write cannot leave an unparseable truncated file. Falls back to direct write if rename fails (e.g. cross-device). Adds regression tests for the helper. Closes #1915 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -4,7 +4,7 @@
|
|||||||
|
|
||||||
const fs = require('fs');
|
const fs = require('fs');
|
||||||
const path = require('path');
|
const path = require('path');
|
||||||
const { output, error, planningRoot, CONFIG_DEFAULTS } = require('./core.cjs');
|
const { output, error, planningRoot, CONFIG_DEFAULTS, atomicWriteFileSync } = require('./core.cjs');
|
||||||
const {
|
const {
|
||||||
VALID_PROFILES,
|
VALID_PROFILES,
|
||||||
getAgentToModelMapForProfile,
|
getAgentToModelMapForProfile,
|
||||||
@@ -227,7 +227,7 @@ function cmdConfigNewProject(cwd, choicesJson, raw) {
|
|||||||
const config = buildNewProjectConfig(userChoices);
|
const config = buildNewProjectConfig(userChoices);
|
||||||
|
|
||||||
try {
|
try {
|
||||||
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
||||||
output({ created: true, path: '.planning/config.json' }, raw, 'created');
|
output({ created: true, path: '.planning/config.json' }, raw, 'created');
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
error('Failed to write config.json: ' + err.message);
|
error('Failed to write config.json: ' + err.message);
|
||||||
@@ -261,7 +261,7 @@ function ensureConfigFile(cwd) {
|
|||||||
const config = buildNewProjectConfig({});
|
const config = buildNewProjectConfig({});
|
||||||
|
|
||||||
try {
|
try {
|
||||||
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
||||||
return { created: true, path: '.planning/config.json' };
|
return { created: true, path: '.planning/config.json' };
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
error('Failed to create config.json: ' + err.message);
|
error('Failed to create config.json: ' + err.message);
|
||||||
@@ -318,7 +318,7 @@ function setConfigValue(cwd, keyPath, parsedValue) {
|
|||||||
|
|
||||||
// Write back
|
// Write back
|
||||||
try {
|
try {
|
||||||
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8');
|
||||||
return { updated: true, key: keyPath, value: parsedValue, previousValue };
|
return { updated: true, key: keyPath, value: parsedValue, previousValue };
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
error('Failed to write config.json: ' + err.message);
|
error('Failed to write config.json: ' + err.message);
|
||||||
|
|||||||
@@ -1491,6 +1491,38 @@ function readSubdirectories(dirPath, sort = false) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ─── Atomic file writes ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Write a file atomically using write-to-temp-then-rename.
|
||||||
|
*
|
||||||
|
* On POSIX systems, `fs.renameSync` is atomic when the source and destination
|
||||||
|
* are on the same filesystem. This prevents a process killed mid-write from
|
||||||
|
* leaving a truncated file that is unparseable on next read.
|
||||||
|
*
|
||||||
|
* The temp file is placed alongside the target so it is guaranteed to be on
|
||||||
|
* the same filesystem (required for rename atomicity). The PID is embedded in
|
||||||
|
* the temp file name so concurrent writers use distinct paths.
|
||||||
|
*
|
||||||
|
* If `renameSync` fails (e.g. cross-device move), the function falls back to a
|
||||||
|
* direct `writeFileSync` so callers always get a best-effort write.
|
||||||
|
*
|
||||||
|
* @param {string} filePath Absolute path to write.
|
||||||
|
* @param {string|Buffer} content File content.
|
||||||
|
* @param {string} [encoding='utf-8'] Encoding passed to writeFileSync.
|
||||||
|
*/
|
||||||
|
function atomicWriteFileSync(filePath, content, encoding = 'utf-8') {
|
||||||
|
const tmpPath = filePath + '.tmp.' + process.pid;
|
||||||
|
try {
|
||||||
|
fs.writeFileSync(tmpPath, content, encoding);
|
||||||
|
fs.renameSync(tmpPath, filePath);
|
||||||
|
} catch (renameErr) {
|
||||||
|
// Clean up the temp file if rename failed, then fall back to direct write.
|
||||||
|
try { fs.unlinkSync(tmpPath); } catch { /* already gone or never created */ }
|
||||||
|
fs.writeFileSync(filePath, content, encoding);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
module.exports = {
|
module.exports = {
|
||||||
output,
|
output,
|
||||||
error,
|
error,
|
||||||
@@ -1536,4 +1568,5 @@ module.exports = {
|
|||||||
readSubdirectories,
|
readSubdirectories,
|
||||||
getAgentsDir,
|
getAgentsDir,
|
||||||
checkAgentsInstalled,
|
checkAgentsInstalled,
|
||||||
|
atomicWriteFileSync,
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -4,7 +4,7 @@
|
|||||||
|
|
||||||
const fs = require('fs');
|
const fs = require('fs');
|
||||||
const path = require('path');
|
const path = require('path');
|
||||||
const { escapeRegex, normalizePhaseName, planningPaths, withPlanningLock, output, error, findPhaseInternal, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, phaseTokenMatches } = require('./core.cjs');
|
const { escapeRegex, normalizePhaseName, planningPaths, withPlanningLock, output, error, findPhaseInternal, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, phaseTokenMatches, atomicWriteFileSync } = require('./core.cjs');
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Search for a phase header (and its section) within the given content string.
|
* Search for a phase header (and its section) within the given content string.
|
||||||
@@ -341,7 +341,7 @@ function cmdRoadmapUpdatePlanProgress(cwd, phaseNum, raw) {
|
|||||||
roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
|
roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
|
||||||
}
|
}
|
||||||
|
|
||||||
fs.writeFileSync(roadmapPath, roadmapContent, 'utf-8');
|
atomicWriteFileSync(roadmapPath, roadmapContent, 'utf-8');
|
||||||
});
|
});
|
||||||
output({
|
output({
|
||||||
updated: true,
|
updated: true,
|
||||||
|
|||||||
@@ -4,7 +4,7 @@
|
|||||||
|
|
||||||
const fs = require('fs');
|
const fs = require('fs');
|
||||||
const path = require('path');
|
const path = require('path');
|
||||||
const { escapeRegex, loadConfig, getMilestoneInfo, getMilestonePhaseFilter, normalizeMd, planningDir, planningPaths, output, error } = require('./core.cjs');
|
const { escapeRegex, loadConfig, getMilestoneInfo, getMilestonePhaseFilter, normalizeMd, planningDir, planningPaths, output, error, atomicWriteFileSync } = require('./core.cjs');
|
||||||
const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cjs');
|
const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cjs');
|
||||||
|
|
||||||
/** Shorthand — every state command needs this path */
|
/** Shorthand — every state command needs this path */
|
||||||
@@ -845,7 +845,7 @@ function writeStateMd(statePath, content, cwd) {
|
|||||||
const synced = syncStateFrontmatter(content, cwd);
|
const synced = syncStateFrontmatter(content, cwd);
|
||||||
const lockPath = acquireStateLock(statePath);
|
const lockPath = acquireStateLock(statePath);
|
||||||
try {
|
try {
|
||||||
fs.writeFileSync(statePath, normalizeMd(synced), 'utf-8');
|
atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8');
|
||||||
} finally {
|
} finally {
|
||||||
releaseStateLock(lockPath);
|
releaseStateLock(lockPath);
|
||||||
}
|
}
|
||||||
@@ -863,7 +863,7 @@ function readModifyWriteStateMd(statePath, transformFn, cwd) {
|
|||||||
const content = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : '';
|
const content = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : '';
|
||||||
const modified = transformFn(content);
|
const modified = transformFn(content);
|
||||||
const synced = syncStateFrontmatter(modified, cwd);
|
const synced = syncStateFrontmatter(modified, cwd);
|
||||||
fs.writeFileSync(statePath, normalizeMd(synced), 'utf-8');
|
atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8');
|
||||||
} finally {
|
} finally {
|
||||||
releaseStateLock(lockPath);
|
releaseStateLock(lockPath);
|
||||||
}
|
}
|
||||||
|
|||||||
78
tests/atomic-write.test.cjs
Normal file
78
tests/atomic-write.test.cjs
Normal file
@@ -0,0 +1,78 @@
|
|||||||
|
/**
|
||||||
|
* Tests for atomicWriteFileSync helper (issue #1915)
|
||||||
|
*/
|
||||||
|
|
||||||
|
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||||
|
const assert = require('node:assert/strict');
|
||||||
|
const fs = require('fs');
|
||||||
|
const path = require('path');
|
||||||
|
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||||
|
|
||||||
|
const CORE_PATH = path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'core.cjs');
|
||||||
|
|
||||||
|
describe('atomicWriteFileSync', () => {
|
||||||
|
let tmpDir;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
tmpDir = createTempDir();
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
cleanup(tmpDir);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('is exported from core.cjs', () => {
|
||||||
|
const core = require(CORE_PATH);
|
||||||
|
assert.strictEqual(typeof core.atomicWriteFileSync, 'function', 'atomicWriteFileSync must be exported');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('writes correct content to the target file', () => {
|
||||||
|
const { atomicWriteFileSync } = require(CORE_PATH);
|
||||||
|
const filePath = path.join(tmpDir, 'test.md');
|
||||||
|
const content = '# Hello\nworld\n';
|
||||||
|
|
||||||
|
atomicWriteFileSync(filePath, content, 'utf-8');
|
||||||
|
|
||||||
|
const written = fs.readFileSync(filePath, 'utf-8');
|
||||||
|
assert.strictEqual(written, content, 'written content must match');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('does not leave .tmp.* files after successful write', () => {
|
||||||
|
const { atomicWriteFileSync } = require(CORE_PATH);
|
||||||
|
const filePath = path.join(tmpDir, 'STATE.md');
|
||||||
|
|
||||||
|
atomicWriteFileSync(filePath, '# State\n', 'utf-8');
|
||||||
|
|
||||||
|
const entries = fs.readdirSync(tmpDir);
|
||||||
|
const tmpFiles = entries.filter(e => e.includes('.tmp.'));
|
||||||
|
assert.deepStrictEqual(tmpFiles, [], 'no .tmp.* files should remain after write');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('overwrites an existing file with new content', () => {
|
||||||
|
const { atomicWriteFileSync } = require(CORE_PATH);
|
||||||
|
const filePath = path.join(tmpDir, 'config.json');
|
||||||
|
|
||||||
|
atomicWriteFileSync(filePath, '{"first":true}', 'utf-8');
|
||||||
|
atomicWriteFileSync(filePath, '{"second":true}', 'utf-8');
|
||||||
|
|
||||||
|
const written = fs.readFileSync(filePath, 'utf-8');
|
||||||
|
assert.strictEqual(written, '{"second":true}', 'second write must replace first');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('cleans up stale tmp file if present before write', () => {
|
||||||
|
const { atomicWriteFileSync } = require(CORE_PATH);
|
||||||
|
const filePath = path.join(tmpDir, 'ROADMAP.md');
|
||||||
|
// Place a stale tmp file matching the pattern used by atomicWriteFileSync
|
||||||
|
const staleTmp = filePath + '.tmp.' + process.pid;
|
||||||
|
fs.writeFileSync(staleTmp, 'stale content', 'utf-8');
|
||||||
|
|
||||||
|
atomicWriteFileSync(filePath, '# Roadmap\n', 'utf-8');
|
||||||
|
|
||||||
|
const entries = fs.readdirSync(tmpDir);
|
||||||
|
const tmpFiles = entries.filter(e => e.includes('.tmp.'));
|
||||||
|
assert.deepStrictEqual(tmpFiles, [], 'stale .tmp.* file must be gone after write');
|
||||||
|
|
||||||
|
const written = fs.readFileSync(filePath, 'utf-8');
|
||||||
|
assert.strictEqual(written, '# Roadmap\n', 'target file must have correct content');
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user