From 4d42516ffedeb4ebcc24ee30004d4f8ec914790f Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Fri, 16 Oct 2026 11:01:25 +0100 Subject: [PATCH] Make changelog assembly recoverable after a partial write `--write` wrote CHANGELOG.md and then removed fragments one by one, so a failure after the write left surviving fragments that a rerun spliced in a second time. The write is now staged. The assembled file goes into changelog.d/.assembling/, the fragments are renamed in beside it, and renaming the assembled file over CHANGELOG.md is the single commit point. A run that fails rolls itself back when it can; otherwise the next `npm run changelog` reads the staging directory and either restores the fragments (assembled file still staged) or deletes the leftovers (batch landed). `--check` reports a staged batch instead of reporting past it. A failure-injection test makes every filesystem call in a `--write` fail in turn, both as a one-off error and as a crash with no in-process rollback. It asserts the retry produces exactly the uninterrupted CHANGELOG.md, with each entry once and an empty directory. The test fails against the previous write-then-delete and against delete-first. Co-Authored-By: Claude Opus 5.5 --- .gitignore | 3 + changelog.d/README.md | 7 ++ docs/MAINTAINER_WORKFLOW.md | 4 +- scripts/assemble-changelog.mjs | 98 ++++++++++++++-- src/tooling/changelogFragments.test.mjs | 141 ++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 14e8d320b..d2f0107c0 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,6 @@ qa-artifacts/ 3d-models/ pinokio/ENVIRONMENT pinokio/.installed + +# Transient: a release interrupted mid-assembly (npm run changelog recovers it) +changelog.d/.assembling/ diff --git a/changelog.d/README.md b/changelog.d/README.md index 26a7c2e07..df17e32f2 100644 --- a/changelog.d/README.md +++ b/changelog.d/README.md @@ -38,6 +38,13 @@ The assembler refuses a batch rather than guessing: a filename that is not `-.md`, or two fragments claiming one pull request number, stops the run before anything is written and names the files. +If `npm run changelog` fails part-way (a full disk, a killed process), run it +again. The batch is staged in `changelog.d/.assembling/` and only lands when the +assembled file replaces `CHANGELOG.md` in a single rename, so the rerun either +finishes that batch or puts its fragments back first. It never adds an entry +twice or drops one. `npm run changelog:check` names a staged batch, if one is +left over, instead of reporting past it. + A documentation claim that needs a regression test — the way `src/annotations/drawTool.test.mjs` asserts against `CHANGELOG.md` — should assert against `docs/CURRENT-STATE.md` or `README.md` instead. A pull request diff --git a/docs/MAINTAINER_WORKFLOW.md b/docs/MAINTAINER_WORKFLOW.md index 5c773f399..ec6cd026d 100644 --- a/docs/MAINTAINER_WORKFLOW.md +++ b/docs/MAINTAINER_WORKFLOW.md @@ -130,7 +130,9 @@ resolves the top of `CHANGELOG.md`. At release, `npm run changelog:check` previe the batch and its order; `npm run changelog` splices it into `CHANGELOG.md` and empties the directory. The assembler refuses a malformed filename or two fragments claiming one PR number before writing, and copies text through unchanged so the -contributor's wording and credit ship. +contributor's wording and credit ship. The write is staged in +`changelog.d/.assembling/`, so if it fails part-way, rerunning `npm run changelog` +finishes or rolls back the batch. It never duplicates or drops an entry. Return this concise record privately unless posting is authorized. Keep sensitive security evidence in the private report: diff --git a/scripts/assemble-changelog.mjs b/scripts/assemble-changelog.mjs index 8f3e854ef..430fca77a 100644 --- a/scripts/assemble-changelog.mjs +++ b/scripts/assemble-changelog.mjs @@ -1,4 +1,4 @@ -import { readdir, readFile, writeFile, rm } from 'node:fs/promises'; +import * as fs from 'node:fs/promises'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -14,8 +14,19 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; * `src/annotations/drawTool.test.mjs` reads `CHANGELOG.md` as prose and slices a * fixed window after a heading, so reflowing an entry here could move text * across that window. + * + * A write is staged so that a failure at any point leaves a rerun unable to + * duplicate or lose an entry. The assembled file is written into + * `changelog.d/.assembling/` first, the fragments are renamed in beside it, and + * renaming the assembled file over `CHANGELOG.md` is the single commit point. + * While the assembled file is still staged the batch has not landed, so + * recovery moves the fragments back; once it is gone the batch has landed, and + * the staged fragments are leftovers to delete. Either way they are out of the + * input directory, so they are never assembled twice. */ export const FRAGMENT_DIR = 'changelog.d'; +export const STAGING_DIR = '.assembling'; +const STAGED_CHANGELOG = 'CHANGELOG.md'; const FRAGMENT_NAME = /^(\d+)-[a-z0-9]+(?:-[a-z0-9]+)*\.md$/; const HEADING = '# Changelog\n'; @@ -56,36 +67,109 @@ export function assembleChangelog(changelog, fragments) { return `${HEADING}\n${batch}\n\n${entries}`; } +/** + * Finish or roll back a batch an earlier `--write` left in the staging + * directory. A no-op when there is none. + * + * @returns {Promise<'none'|'rolled-back'|'cleaned-up'>} + */ +export async function recoverInterruptedBatch(root, io = fs) { + const directory = path.join(root, FRAGMENT_DIR); + const staging = path.join(directory, STAGING_DIR); + let staged; + try { + staged = await io.readdir(staging); + } catch (error) { + if (error.code === 'ENOENT') return 'none'; + throw error; + } + if (!staged.includes(STAGED_CHANGELOG)) { + // The assembled file was renamed over CHANGELOG.md: the batch landed. + await io.rm(staging, { recursive: true, force: true }); + return 'cleaned-up'; + } + // The batch never reached CHANGELOG.md, so every fragment goes back. + const pending = new Set(await io.readdir(directory)); + const fragments = staged.filter((name) => name !== STAGED_CHANGELOG); + const clash = fragments.filter((name) => pending.has(name)); + if (clash.length) { + throw new Error( + `Cannot roll back ${FRAGMENT_DIR}/${STAGING_DIR}/: ${clash.join(', ')}` + + ` also exist in ${FRAGMENT_DIR}/ — resolve by hand`, + ); + } + for (const name of fragments) { + await io.rename(path.join(staging, name), path.join(directory, name)); + } + await io.rm(staging, { recursive: true, force: true }); + return 'rolled-back'; +} + /** * Report or apply the pending fragments in `/changelog.d`. * * The batch is validated in full before anything is written, so a malformed * filename or a duplicate pull request number leaves the directory untouched. + * `--write` first recovers any batch an interrupted run left staged. * * @param {string} root repository root * @param {'--check'|'--write'} mode `--check` reports and changes nothing + * @param {typeof fs} [io] filesystem calls, replaceable for failure injection * @returns {Promise<{ consumed: string[], changelog: string }>} */ -export async function assembleFragments(root, mode) { +export async function assembleFragments(root, mode, io = fs) { if (!['--check', '--write'].includes(mode)) { throw new Error( 'Usage: node scripts/assemble-changelog.mjs --check|--write', ); } const directory = path.join(root, FRAGMENT_DIR); + const staging = path.join(directory, STAGING_DIR); const changelogFile = path.join(root, 'CHANGELOG.md'); - const consumed = orderFragmentNames(await readdir(directory)); + if (mode === '--write') { + await recoverInterruptedBatch(root, io); + } + const names = await io.readdir(directory); + if (names.includes(STAGING_DIR)) { + throw new Error( + `${FRAGMENT_DIR}/${STAGING_DIR}/ holds a batch an interrupted release` + + ' left behind — `npm run changelog` finishes or rolls it back', + ); + } + const consumed = orderFragmentNames(names); const bodies = []; for (const name of consumed) { - bodies.push(await readFile(path.join(directory, name), 'utf8')); + bodies.push(await io.readFile(path.join(directory, name), 'utf8')); } const changelog = assembleChangelog( - await readFile(changelogFile, 'utf8'), + await io.readFile(changelogFile, 'utf8'), bodies, ); if (mode === '--write' && consumed.length) { - await writeFile(changelogFile, changelog, 'utf8'); - for (const name of consumed) await rm(path.join(directory, name)); + try { + await io.mkdir(staging); + await io.writeFile( + path.join(staging, STAGED_CHANGELOG), + changelog, + 'utf8', + ); + for (const name of consumed) { + await io.rename(path.join(directory, name), path.join(staging, name)); + } + await io.rename(path.join(staging, STAGED_CHANGELOG), changelogFile); + await io.rm(staging, { recursive: true, force: true }); + } catch (error) { + try { + await recoverInterruptedBatch(root, io); + } catch { + throw new Error( + `${error.message} — the batch is staged in ${FRAGMENT_DIR}/${STAGING_DIR}/,` + + ' and the next `npm run changelog` finishes or rolls it back', + { cause: error }, + ); + } + throw error; + } } return { consumed, changelog }; } diff --git a/src/tooling/changelogFragments.test.mjs b/src/tooling/changelogFragments.test.mjs index 880e3318d..792542f68 100644 --- a/src/tooling/changelogFragments.test.mjs +++ b/src/tooling/changelogFragments.test.mjs @@ -1,5 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; +import * as fs from 'node:fs/promises'; import { mkdtemp, mkdir, @@ -14,9 +15,11 @@ import { fileURLToPath } from 'node:url'; import { FRAGMENT_DIR, + STAGING_DIR, assembleChangelog, assembleFragments, orderFragmentNames, + recoverInterruptedBatch, } from '../../scripts/assemble-changelog.mjs'; const EXISTING = @@ -154,6 +157,144 @@ test('a mode that is neither check nor write is refused', async (t) => { await assert.rejects(assembleFragments(root, '--force'), /--check\|--write/); }); +// Failure injection. Every filesystem call the write path makes is a point +// where a release can die: a full disk, a permissions error, a killed process. + +const IO_CALLS = ['readdir', 'readFile', 'mkdir', 'writeFile', 'rename', 'rm']; +const BATCH = { + '20-second.md': '- Second (someone, #20).', + '100-first.md': '- First (someone, #100).', + '7-third.md': '- Third (someone, #7).', +}; + +/** + * The real filesystem, except that `call` throws the `nth` time it runs. With + * `crash`, every later call throws as well, the way a killed process makes no + * further calls: the in-process rollback cannot run either, so the next + * invocation is the only thing that can recover. + */ +function failing(call, nth, { crash = false } = {}) { + let seen = 0; + let dead = false; + const io = { ...fs }; + for (const name of IO_CALLS) { + io[name] = async (...args) => { + if (dead) throw new Error('process already gone'); + if (name === call && ++seen === nth) { + dead = crash; + throw new Error(`injected ${call} failure #${nth}`); + } + return fs[name](...args); + }; + } + return io; +} + +/** How many times one uninterrupted `--write` makes each filesystem call. */ +async function countCalls(t) { + const root = await fixture(t, BATCH); + const counts = Object.fromEntries(IO_CALLS.map((name) => [name, 0])); + const io = { ...fs }; + for (const name of IO_CALLS) { + io[name] = (...args) => { + counts[name] += 1; + return fs[name](...args); + }; + } + await assembleFragments(root, '--write', io); + return { + counts, + expected: await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + }; +} + +test('a write that fails at any call never lets a retry duplicate or lose an entry', async (t) => { + const { counts, expected } = await countCalls(t); + // The write path itself: one mkdir, one writeFile, a rename per fragment plus + // the commit rename, and the cleanup rm. Fewer would mean a step went missing. + assert.ok(counts.rename >= Object.keys(BATCH).length + 1); + for (const crash of [false, true]) { + for (const call of IO_CALLS) { + for (let nth = 1; nth <= counts[call]; nth += 1) { + const label = `${crash ? 'crash' : 'error'} at ${call} #${nth}`; + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing(call, nth, { crash })), + /injected|already gone/, + label, + ); + await assembleFragments(root, '--write'); + const changelog = await readFile( + path.join(root, 'CHANGELOG.md'), + 'utf8', + ); + assert.equal(changelog, expected, label); + for (const pr of [100, 20, 7, 1]) { + assert.equal( + changelog.split(`(someone, #${pr}).`).length - 1, + 1, + `${label}: #${pr} appears exactly once`, + ); + } + assert.deepEqual( + await readdir(path.join(root, FRAGMENT_DIR)), + ['README.md'], + label, + ); + } + } + } +}); + +test('an error before the commit rolls the batch back in the same run', async (t) => { + const root = await fixture(t, BATCH); + // The second rename has already moved one fragment into the staging directory. + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2)), + /injected rename failure #2/, + ); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); + assert.deepEqual( + (await readdir(path.join(root, FRAGMENT_DIR))).sort(), + [...Object.keys(BATCH), 'README.md'].sort(), + ); +}); + +test('check mode reports a batch an interrupted release left staged', async (t) => { + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2, { crash: true })), + ); + await assert.rejects( + assembleFragments(root, '--check'), + new RegExp(`${STAGING_DIR}/ holds a batch an interrupted release`), + ); + assert.equal(await recoverInterruptedBatch(root), 'rolled-back'); + assert.equal(await recoverInterruptedBatch(root), 'none'); +}); + +test('recovery refuses to roll a fragment back over a file of the same name', async (t) => { + const root = await fixture(t, BATCH); + await assert.rejects( + assembleFragments(root, '--write', failing('rename', 2, { crash: true })), + ); + await writeFile( + path.join(root, FRAGMENT_DIR, '100-first.md'), + '- Rewritten.', + ); + await assert.rejects( + assembleFragments(root, '--write'), + /Cannot roll back .*100-first\.md/, + ); + assert.equal( + await readFile(path.join(root, 'CHANGELOG.md'), 'utf8'), + EXISTING, + ); +}); + // The two below run against the repository itself, so a fragment that the // release could not assemble fails `npm test` in the pull request that adds it // rather than at release time, when its author has moved on.