From 80a82c4cae129c5426f3ccd042d1be7da5471946 Mon Sep 17 00:00:00 2001 From: Hermes Date: Sat, 22 Aug 2026 23:28:36 -0700 Subject: [PATCH] refactor(persistence): canonical atomic file writer + notifications.json crash-safety (DC-099) [glm-grade=B] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - src/utils/atomic-write.js: single shared tmp+fsync+rename writer (exclusive-create 0600, unique tmp names, cleanup-on-failure, best-effort parent-dir fsync after rename for swap durability) - notification-manager: both write paths (load-time canonicalization write-back + saveConfig) converted from plain writeFileSync — a crash mid-write can no longer truncate notifications.json - DC-097/098 test seams migrated to the atomic path; new suite pins syscall discipline (order, wx flags, tmp naming, error cleanup, dir-fsync swallow) - 121 suites / 2768 tests green - Judge: GLM-5.3 cold read grade B/ship (deleg_23f7abad); polish items folded: dir-fsync added, header copy-count corrected. Remaining: migrate invite/user/share-store private _atomicWriteJSON copies as they are touched (queued). URN: pending (recorded post-commit) --- .../__tests__/atomic-write-dc099.test.js | 198 ++++++++++++++++++ ...otification-config-writeback-dc097.test.js | 35 ++-- .../__tests__/notification-manager.test.js | 17 +- .../src/managers/notification-manager.js | 9 +- dashcaddy-api/src/utils/atomic-write.js | 101 +++++++++ 5 files changed, 341 insertions(+), 19 deletions(-) create mode 100644 dashcaddy-api/__tests__/atomic-write-dc099.test.js create mode 100644 dashcaddy-api/src/utils/atomic-write.js diff --git a/dashcaddy-api/__tests__/atomic-write-dc099.test.js b/dashcaddy-api/__tests__/atomic-write-dc099.test.js new file mode 100644 index 0000000..3157c0b --- /dev/null +++ b/dashcaddy-api/__tests__/atomic-write-dc099.test.js @@ -0,0 +1,198 @@ +/** + * DC-099: canonical atomic file writer (src/utils/atomic-write.js). + * + * The notification config's two write paths (load-time canonicalization + * write-back and the UI saveConfig) used plain fs.writeFileSync — a crash or + * power loss mid-write could leave a truncated/empty notifications.json. The + * same risk exists in every store that grew its own private + * _atomicWriteJSON copy (invite-store, user-store, share-store, …). + * + * These tests pin the shared writer's contract: + * - durability: fsync before rename, exclusive create, 0600 default + * - atomicity: destination only ever replaced via rename + * - failure: destination untouched, temp cleaned up, error propagated + * - JSON helper: single serialization shape (2-space, no trailing newline — + * notification-manager._persistCanonicalForm depends on byte-for-byte + * idempotence) + */ + +'use strict'; + +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { atomicWriteFile, atomicWriteJSON, tmpPathFor } = require('../src/utils/atomic-write'); + +// Real-FS tests: the actual syscalls, in a private temp dir. +describe('DC-099 atomic-write (real fs)', () => { + let dir; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'dc099-atomic-')); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + test('writes contents and returns the final path', () => { + const target = path.join(dir, 'state.json'); + const ret = atomicWriteFile(target, '{"a":1}'); + expect(ret).toBe(target); + expect(fs.readFileSync(target, 'utf8')).toBe('{"a":1}'); + }); + + test('replaces an existing file completely (no torn writes possible)', () => { + const target = path.join(dir, 'state.json'); + atomicWriteFile(target, 'x'.repeat(1000)); + atomicWriteFile(target, 'y'.repeat(10)); + expect(fs.readFileSync(target, 'utf8')).toBe('y'.repeat(10)); + }); + + test('creates the file 0600 by default', () => { + const target = path.join(dir, 'secret.json'); + atomicWriteJSON(target, { ok: true }); + expect(fs.statSync(target).mode & 0o777).toBe(0o600); + }); + + test('honors an explicit mode override', () => { + const target = path.join(dir, 'public.json'); + atomicWriteFile(target, '{}', { mode: 0o644 }); + expect(fs.statSync(target).mode & 0o777).toBe(0o644); + }); + + test('leaves no temp files behind after success', () => { + const target = path.join(dir, 'state.json'); + atomicWriteFile(target, 'abc'); + expect(fs.readdirSync(dir).filter((f) => f.includes('.tmp-'))).toEqual([]); + }); + + test('two rapid writes both land (unique tmp names per write)', () => { + const target = path.join(dir, 'state.json'); + atomicWriteFile(target, 'first'); + atomicWriteFile(target, 'second'); + expect(fs.readFileSync(target, 'utf8')).toBe('second'); + }); + + test('atomicWriteJSON serializes 2-space, no trailing newline', () => { + const target = path.join(dir, 'conf.json'); + atomicWriteJSON(target, { a: { b: 1 } }); + const raw = fs.readFileSync(target, 'utf8'); + expect(raw).toBe('{\n "a": {\n "b": 1\n }\n}'); + }); + + test('write failure leaves the destination untouched and cleans the temp file', () => { + const target = path.join(dir, 'state.json'); + fs.writeFileSync(target, 'ORIGINAL'); + const origWrite = fs.writeSync; + fs.writeSync = () => { + throw Object.assign(new Error('ENOSPC: no space left on device'), { code: 'ENOSPC' }); + }; + try { + expect(() => atomicWriteFile(target, 'NEW-CONTENT')).toThrow(/ENOSPC/); + } finally { + fs.writeSync = origWrite; + } + expect(fs.readFileSync(target, 'utf8')).toBe('ORIGINAL'); + expect(fs.readdirSync(dir).filter((f) => f.includes('.tmp-'))).toEqual([]); + }); + + test('tmpPathFor: unique per call, hidden dotfile in the same directory', () => { + const a = tmpPathFor('/data/x.json'); + const b = tmpPathFor('/data/x.json'); + expect(a).not.toBe(b); + expect(path.dirname(a)).toBe('/data'); + expect(path.basename(a)).toMatch(/^\.x\.json\.tmp-/); + }); +}); + +// Mocked-FS tests: pin the syscall DISCIPLINE itself (order + flags), which +// the real-fs tests can't observe directly. +describe('DC-099 atomic-write syscall discipline (mocked fs)', () => { + const calls = []; + + beforeEach(() => { + calls.length = 0; + const rec = (name, impl) => + jest.spyOn(fs, name).mockImplementation((...args) => { + calls.push(name); + return impl(...args); + }); + rec('openSync', () => 3); + rec('writeSync', () => 8); + rec('fsyncSync', () => {}); + rec('closeSync', () => {}); + rec('renameSync', () => {}); + rec('unlinkSync', () => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test('order: open → write → fsync → close → rename, then dir fsync (open → fsync → close)', () => { + atomicWriteFile('/data/x.json', '{"a":1}'); + expect(calls).toEqual([ + 'openSync', 'writeSync', 'fsyncSync', 'closeSync', 'renameSync', + 'openSync', 'fsyncSync', 'closeSync', + ]); + }); + + test('dir fsync opens the PARENT directory (second openSync), not another tmp file', () => { + atomicWriteFile('/data/x.json', '{}'); + const dirOpen = fs.openSync.mock.calls[1]; + expect(dirOpen[0]).toBe('/data'); + expect(dirOpen[1]).toBe('r'); + }); + + test('dir fsync failure is swallowed (write still succeeds)', () => { + let n = 0; + fs.fsyncSync.mockImplementation(() => { + n += 1; + if (n === 2) throw new Error('EINVAL: invalid argument'); // 2nd fsync = dir + }); + expect(() => atomicWriteFile('/data/x.json', '{}')).not.toThrow(); + expect(fs.renameSync).toHaveBeenCalled(); + }); + + test('open uses exclusive-create with the 0600 default on the tmp path', () => { + atomicWriteFile('/data/x.json', '{}'); + const [tmpPath, flags, modeArg] = fs.openSync.mock.calls[0]; + expect(tmpPath).toMatch(/^\/data\/\.x\.json\.tmp-/); + expect(flags).toBe('wx'); + expect(modeArg).toBe(0o600); + }); + + test('write passes the payload with utf8 encoding', () => { + atomicWriteFile('/data/x.json', '{"a":1}'); + expect(fs.writeSync.mock.calls[0]).toEqual([3, '{"a":1}', null, 'utf8']); + }); + + test('rename swaps a same-dir temp onto the target', () => { + atomicWriteFile('/data/x.json', '{}'); + const [tmp, dest] = fs.renameSync.mock.calls[0]; + expect(tmp).toMatch(/\/data\/\.x\.json\.tmp-/); + expect(dest).toBe('/data/x.json'); + }); + + test('rename failure unlinks the temp and propagates the error', () => { + fs.renameSync.mockImplementation(() => { + calls.push('renameSync'); + throw new Error('EXDEV: cross-device link not permitted'); + }); + expect(() => atomicWriteFile('/data/x.json', '{}')).toThrow(/EXDEV/); + expect(calls).toEqual([ + 'openSync', 'writeSync', 'fsyncSync', 'closeSync', 'renameSync', 'unlinkSync', + ]); + }); + + test('open failure propagates without write/rename (nothing was created)', () => { + fs.openSync.mockImplementation(() => { + calls.push('openSync'); + throw new Error('EACCES: permission denied'); + }); + expect(() => atomicWriteFile('/data/x.json', '{}')).toThrow(/EACCES/); + // best-effort unlink of the never-created temp, then stop + expect(calls).toEqual(['openSync', 'unlinkSync']); + }); +}); diff --git a/dashcaddy-api/__tests__/notification-config-writeback-dc097.test.js b/dashcaddy-api/__tests__/notification-config-writeback-dc097.test.js index 70ca478..466addb 100644 --- a/dashcaddy-api/__tests__/notification-config-writeback-dc097.test.js +++ b/dashcaddy-api/__tests__/notification-config-writeback-dc097.test.js @@ -12,6 +12,13 @@ jest.mock('fs', () => ({ readFileSync: jest.fn().mockReturnValue('{}'), writeFileSync: jest.fn(), mkdirSync: jest.fn(), + // DC-099 atomic write path (open tmp → write → fsync → close → rename). + openSync: jest.fn().mockReturnValue(3), + writeSync: jest.fn(), + fsyncSync: jest.fn(), + closeSync: jest.fn(), + renameSync: jest.fn(), + unlinkSync: jest.fn(), })); jest.mock('nodemailer', () => ({ @@ -86,10 +93,10 @@ describe('DC-097 notification config canonicalization write-back', () => { expect(nm.config.events['container-down']).toBe(false); expect(nm.config.events['deploy-success']).toBe(false); - // On-disk write-back: exactly one write of the full canonical config. - expect(fs.writeFileSync).toHaveBeenCalledTimes(1); - const [pathArg, contentsArg] = fs.writeFileSync.mock.calls[0]; - expect(pathArg).toBe(NOTIF_FILE); + // On-disk write-back: exactly one atomic write (DC-099: write tmp → fsync → rename). + expect(fs.renameSync).toHaveBeenCalledTimes(1); + expect(fs.renameSync.mock.calls[0][1]).toBe(NOTIF_FILE); + const contentsArg = fs.writeSync.mock.calls[0][1]; const written = JSON.parse(contentsArg); expect(written.providers.email.username).toBe('legacy-user'); expect(written.providers.email.password).toBe('legacy-pass'); @@ -108,14 +115,15 @@ describe('DC-097 notification config canonicalization write-back', () => { events: { containerDown: true }, }); const first = loadWithFile(legacy, log); - expect(fs.writeFileSync).toHaveBeenCalledTimes(1); - const canonicalContents = fs.writeFileSync.mock.calls[0][1]; + expect(fs.renameSync).toHaveBeenCalledTimes(1); + const canonicalContents = fs.writeSync.mock.calls[0][1]; first.stopHealthDaemon && first.stopHealthDaemon(); - fs.writeFileSync.mockClear(); + fs.renameSync.mockClear(); + fs.writeSync.mockClear(); // Second load against the canonical bytes: no write. const second = loadWithFile(canonicalContents, log); - expect(fs.writeFileSync).not.toHaveBeenCalled(); + expect(fs.renameSync).not.toHaveBeenCalled(); expect(second.config.providers.email.username).toBe('u'); second.stopHealthDaemon && second.stopHealthDaemon(); }); @@ -125,11 +133,12 @@ describe('DC-097 notification config canonicalization write-back', () => { // Build it by round-tripping: write-back from a minimal legacy file // produces the canonical full shape; feed those exact bytes back. const nm = loadWithFile(ser({ enabled: true }), log); // 1 write (defaults fill-in) - const canonicalContents = fs.writeFileSync.mock.calls[0][1]; + const canonicalContents = fs.writeSync.mock.calls[0][1]; nm.stopHealthDaemon && nm.stopHealthDaemon(); - fs.writeFileSync.mockClear(); + fs.renameSync.mockClear(); + fs.writeSync.mockClear(); const again = loadWithFile(canonicalContents, log); - expect(fs.writeFileSync).not.toHaveBeenCalled(); + expect(fs.renameSync).not.toHaveBeenCalled(); again.stopHealthDaemon && again.stopHealthDaemon(); }); @@ -138,7 +147,7 @@ describe('DC-097 notification config canonicalization write-back', () => { providers: { email: { user: 'u2', pass: 'p2' } }, events: { workflowDone: true }, }); - fs.writeFileSync.mockImplementation(() => { throw new Error('EACCES: permission denied'); }); + fs.openSync.mockImplementation(() => { throw new Error('EACCES: permission denied'); }); let nm; expect(() => { nm = loadWithFile(legacy, log); }).not.toThrow(); expect(nm.config.providers.email.username).toBe('u2'); @@ -153,7 +162,7 @@ describe('DC-097 notification config canonicalization write-back', () => { fs.existsSync.mockReturnValue(false); const nm = new NotificationManager(makeCtx(log)); expect(fs.readFileSync).not.toHaveBeenCalled(); - expect(fs.writeFileSync).not.toHaveBeenCalled(); + expect(fs.renameSync).not.toHaveBeenCalled(); nm.stopHealthDaemon && nm.stopHealthDaemon(); }); }); diff --git a/dashcaddy-api/__tests__/notification-manager.test.js b/dashcaddy-api/__tests__/notification-manager.test.js index 4315a7c..c910ca4 100644 --- a/dashcaddy-api/__tests__/notification-manager.test.js +++ b/dashcaddy-api/__tests__/notification-manager.test.js @@ -10,6 +10,13 @@ jest.mock('fs', () => ({ readFileSync: jest.fn().mockReturnValue('{}'), writeFileSync: jest.fn(), mkdirSync: jest.fn(), + // DC-099 atomic write path (open tmp → write → fsync → close → rename). + openSync: jest.fn().mockReturnValue(3), + writeSync: jest.fn(), + fsyncSync: jest.fn(), + closeSync: jest.fn(), + renameSync: jest.fn(), + unlinkSync: jest.fn(), })); jest.mock('nodemailer', () => ({ @@ -63,10 +70,12 @@ describe('NotificationManager', () => { fs.existsSync.mockReturnValue(false); await nm.saveConfig(); expect(fs.mkdirSync).toHaveBeenCalled(); - expect(fs.writeFileSync).toHaveBeenCalled(); - const callArgs = fs.writeFileSync.mock.calls[0]; - expect(callArgs[0]).toBe(NOTIF_FILE); - expect(callArgs[1]).toContain('enabled'); + // DC-099: atomic write path — payload lands via writeSync, then tmp is renamed onto the target. + expect(fs.writeSync).toHaveBeenCalled(); + expect(fs.renameSync).toHaveBeenCalled(); + const writeArgs = fs.writeSync.mock.calls[0]; + expect(fs.renameSync.mock.calls[0][1]).toBe(NOTIF_FILE); + expect(writeArgs[1]).toContain('enabled'); }); test('loadConfig merges file content with defaults', () => { diff --git a/dashcaddy-api/src/managers/notification-manager.js b/dashcaddy-api/src/managers/notification-manager.js index fef300e..7047f5a 100644 --- a/dashcaddy-api/src/managers/notification-manager.js +++ b/dashcaddy-api/src/managers/notification-manager.js @@ -6,6 +6,7 @@ const EventEmitter = require('events'); const fs = require('fs'); const path = require('path'); const nodemailer = require('nodemailer'); +const { atomicWriteJSON } = require('../utils/atomic-write'); // Canonical event names are kebab-case ('container-down'). Emitters and the // settings UI historically send camelCase ('containerDown', 'deploymentSuccess') @@ -125,7 +126,9 @@ class NotificationManager extends EventEmitter { try { const canonical = JSON.stringify(this.config, null, 2); if (canonical !== rawFileContents) { - fs.writeFileSync(this.NOTIFICATIONS_FILE, canonical); + // DC-099: tmp+fsync+rename — a crash mid-write can no longer leave a + // truncated/empty notifications.json (plain writeFileSync could). + atomicWriteJSON(this.NOTIFICATIONS_FILE, this.config); this.log.info?.('notification', 'Notification config canonicalized on disk (legacy keys normalized)', {}); } } catch (writeError) { @@ -198,7 +201,9 @@ class NotificationManager extends EventEmitter { if (!fs.existsSync(dir)) { fs.mkdirSync(dir, { recursive: true }); } - fs.writeFileSync(this.NOTIFICATIONS_FILE, JSON.stringify(this.config, null, 2)); + // DC-099: atomic tmp+fsync+rename — the UI save path gets the same + // crash-safety as the load-path write-back (no torn notifications.json). + atomicWriteJSON(this.NOTIFICATIONS_FILE, this.config); return true; } catch (error) { this.log.error('notification', error, null, { note: 'Failed to save config' }); diff --git a/dashcaddy-api/src/utils/atomic-write.js b/dashcaddy-api/src/utils/atomic-write.js new file mode 100644 index 0000000..c08f3f8 --- /dev/null +++ b/dashcaddy-api/src/utils/atomic-write.js @@ -0,0 +1,101 @@ +/** + * Canonical atomic file writer (DC-099). + * + * Write discipline: same-directory temp file → write → fsync → close → rename. + * rename() is atomic on POSIX, so a reader (or a crash) can only ever see the + * complete old file or the complete new file — never a truncated mix. fsync + * before rename pins the bytes so a post-rename power loss doesn't leave an + * empty/short file behind (the failure mode plain writeFileSync has). + * + * This is the ONE shared implementation. It replaces the three private + * `_atomicWriteJSON` copies (invite-store, user-store, share-store) as they + * are touched, and mirrors the DC-098 redact tool's write path. Do not add a + * fourth copy — require this module instead. + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +// Monotonic counter guarantees unique tmp names even for back-to-back writes +// of the same target within one process tick. +let writeCounter = 0; + +function tmpPathFor(filePath) { + writeCounter += 1; + const base = path.basename(filePath); + const dir = path.dirname(filePath); + return path.join(dir, `.${base}.tmp-${process.pid}-${Date.now().toString(36)}-${writeCounter}`); +} + +/** + * Best-effort durability for the rename itself: fsync the parent directory so + * the swap survives a post-rename power loss. POSIX guarantees the rename is + * atomic *visibly*, but without a dir fsync a crash can leave the old entry — + * a stale-but-complete file, never a torn one, so failure here is not fatal. + */ +function fsyncDir(dirPath) { + let dfd = null; + try { + dfd = fs.openSync(dirPath, 'r'); + fs.fsyncSync(dfd); + } catch (_) { + // Some platforms/filesystems reject fsync on directory fds; the payload + // is already durable via the file-level fsync above. + } finally { + if (dfd !== null) { + try { fs.closeSync(dfd); } catch (_) { /* fd already closed */ } + } + } +} + +/** + * Atomically replace `filePath` with `contents`. + * + * @param {string} filePath - destination (parent dir must exist) + * @param {string} contents - full file contents + * @param {object} [opts] + * @param {number} [opts.mode=0o600] - mode for a newly created file + * @returns {string} the final path (filePath) + * @throws whatever fs throws (ENOSPC, EACCES, …); on failure the destination + * is untouched and the temp file is removed best-effort. + */ +function atomicWriteFile(filePath, contents, opts = {}) { + const mode = typeof opts.mode === 'number' ? opts.mode : 0o600; + const tmp = tmpPathFor(filePath); + let fd = null; + try { + // 'wx' — fail loudly if the tmp name somehow exists rather than clobber. + fd = fs.openSync(tmp, 'wx', mode); + fs.writeSync(fd, contents, null, 'utf8'); + fs.fsyncSync(fd); + fs.closeSync(fd); + fd = null; + fs.renameSync(tmp, filePath); + fsyncDir(path.dirname(filePath)); + return filePath; + } catch (err) { + if (fd !== null) { + try { fs.closeSync(fd); } catch (_) { /* fd already closed or broken */ } + } + try { fs.unlinkSync(tmp); } catch (_) { /* nothing to clean up */ } + throw err; + } +} + +/** + * Atomically write `data` as JSON. Single serializer for the whole codebase: + * 2-space indent, no trailing newline (matches the notification config's + * byte-for-byte idempotence check in _persistCanonicalForm). + * + * @param {string} filePath + * @param {*} data - JSON.stringify-able value + * @param {object} [opts] - passed through to atomicWriteFile + * @returns {string} the final path (filePath) + */ +function atomicWriteJSON(filePath, data, opts = {}) { + return atomicWriteFile(filePath, JSON.stringify(data, null, 2), opts); +} + +module.exports = { atomicWriteFile, atomicWriteJSON, tmpPathFor };