From b1464d9b85f706fbd5b4d91ee1e12a9fccc534bd Mon Sep 17 00:00:00 2001 From: Hermes Date: Sun, 23 Aug 2026 02:31:16 -0700 Subject: [PATCH] refactor(persistence): migrate caddy-upstream-watcher state to canonical atomic-write util (DC-105) [glm-grade=A] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _saveState drops its private fixed-name .tmp + writeFileSync (no fsync) copy and delegates to src/utils/atomic-write.js atomicWriteJSON — same wx/fsync/rename/dir-fsync discipline as the six previously migrated stores (DC-099..DC-104). A crash can no longer tear caddy-upstreams.json (mute list + probe state): power loss through the old path could leave an empty/short state file and silently drop every mute; concurrent saves (60s probe loop vs setMuted) collided on the shared tmp name. Test mock extended with the fd-level fs API (openSync/writeSync/ fsyncSync/closeSync/unlinkSync + closedTmp stash) so the canonical path is exercised under the existing file-wide fs mock; fsState renamed mockFsState (jest.mock out-of-scope-variable hoist rule). New DC-105 pin: wx+fsync+rename required, no fixed .tmp, zero leftover tmp files, destination JSON complete with mute preserved. Judge: GLM-5.3 cold read, round-1 A/ship (deleg_5f57fcce). URN urn:ump:iuhgj3ajr5llshjasttsvelnptv6iqaek6xji5l3klcl72nseqdq Full suite: 121 suites / 2779 tests green. --- .../__tests__/caddy-upstream-watcher.test.js | 124 ++++++++++++++---- .../src/monitoring/caddy-upstream-watcher.js | 12 +- 2 files changed, 104 insertions(+), 32 deletions(-) diff --git a/dashcaddy-api/__tests__/caddy-upstream-watcher.test.js b/dashcaddy-api/__tests__/caddy-upstream-watcher.test.js index f0df46e..87db288 100644 --- a/dashcaddy-api/__tests__/caddy-upstream-watcher.test.js +++ b/dashcaddy-api/__tests__/caddy-upstream-watcher.test.js @@ -10,38 +10,67 @@ const path = require('path'); const Module = require('module'); // Mock fs with controllable behavior. -const fsState = { +const mockFsState = { files: {}, // path -> string content exists: {}, // path -> bool - writeLog: [], // writes + writeLog: [], // writeFileSync calls + fdMap: new Map(), // open fd -> { p, content } (DC-105 atomic-write path) + closedTmp: new Map(), // closed-but-not-yet-renamed tmp path -> content + nextFd: 0, }; jest.mock('fs', () => { const real = jest.requireActual('fs'); return { ...real, - existsSync: jest.fn((p) => fsState.exists[p] !== undefined ? fsState.exists[p] : (fsState.files[p] !== undefined)), + existsSync: jest.fn((p) => mockFsState.exists[p] !== undefined ? mockFsState.exists[p] : (mockFsState.files[p] !== undefined)), readFileSync: jest.fn((p) => { - if (fsState.files[p] === undefined) { + if (mockFsState.files[p] === undefined) { const e = new Error(`ENOENT: ${p}`); e.code = 'ENOENT'; throw e; } - return fsState.files[p]; + return mockFsState.files[p]; }), - readdirSync: jest.fn((p) => Object.keys(fsState.files).filter(f => f.startsWith(p + '/')).map(f => f.substring(p.length + 1))), + readdirSync: jest.fn((p) => Object.keys(mockFsState.files).filter(f => f.startsWith(p + '/')).map(f => f.substring(p.length + 1))), writeFileSync: jest.fn((p, content) => { - fsState.writeLog.push({ p, content }); - fsState.files[p] = content; - fsState.exists[p] = true; + mockFsState.writeLog.push({ p, content }); + mockFsState.files[p] = content; + mockFsState.exists[p] = true; }), mkdirSync: jest.fn(), + // DC-105 canonical atomic-write path (atomic-write.js): openSync('wx') → + // writeSync → fsyncSync → closeSync → renameSync → dir fsync. Content + // accumulates per-fd, is stashed on close, and lands in files[] on rename. + openSync: jest.fn((p) => { + mockFsState.nextFd += 1; + mockFsState.fdMap.set(mockFsState.nextFd, { p, content: '' }); + return mockFsState.nextFd; + }), + writeSync: jest.fn((fd, content) => { + const rec = mockFsState.fdMap.get(fd); + if (!rec) throw new Error(`EBADF: fd ${fd}`); + rec.content += content; + }), + fsyncSync: jest.fn(), + closeSync: jest.fn((fd) => { + const rec = mockFsState.fdMap.get(fd); + if (rec) { + mockFsState.closedTmp.set(rec.p, rec.content); + mockFsState.fdMap.delete(fd); + } + }), renameSync: jest.fn((src, dst) => { - fsState.files[dst] = fsState.files[src]; - fsState.exists[dst] = true; - delete fsState.files[src]; - delete fsState.exists[src]; - }) + const content = mockFsState.closedTmp.has(src) + ? mockFsState.closedTmp.get(src) + : mockFsState.files[src]; + mockFsState.files[dst] = content; + mockFsState.exists[dst] = true; + mockFsState.closedTmp.delete(src); + delete mockFsState.files[src]; + delete mockFsState.exists[src]; + }), + unlinkSync: jest.fn() }; }); @@ -104,9 +133,12 @@ jest.mock('https', () => ({ // Reset fs mock state between tests. beforeEach(() => { - fsState.files = {}; - fsState.exists = {}; - fsState.writeLog = []; + mockFsState.files = {}; + mockFsState.exists = {}; + mockFsState.writeLog = []; + mockFsState.fdMap = new Map(); + mockFsState.closedTmp = new Map(); + mockFsState.nextFd = 0; probeQueue.length = 0; jest.clearAllMocks(); jest.resetModules(); @@ -118,8 +150,8 @@ describe('CaddyUpstreamWatcher', () => { function seedSites(files) { for (const [name, content] of Object.entries(files)) { - fsState.files[SITES + '/' + name] = content; - fsState.exists[SITES + '/' + name] = true; + mockFsState.files[SITES + '/' + name] = content; + mockFsState.exists[SITES + '/' + name] = true; } } @@ -183,8 +215,8 @@ describe('CaddyUpstreamWatcher', () => { const { w } = loadWatcher(); await w.scanSites(); expect(w.upstreams.size).toBe(1); - fsState.files = {}; // wipe - fsState.exists = {}; + mockFsState.files = {}; // wipe + mockFsState.exists = {}; await w.scanSites(); expect(w.upstreams.size).toBe(0); }); @@ -361,22 +393,56 @@ describe('CaddyUpstreamWatcher', () => { const { w } = loadWatcher(); await w.scanSites(); w.setMuted('1.1.1.1:80', true); - // _saveState writes to STATE + '.tmp' then renames. Look for the .tmp - // write since that's the actual writeFileSync call (rename is silent). - const writes = fsState.writeLog.filter(w => w.p === STATE + '.tmp' || w.p === STATE); - expect(writes.length).toBeGreaterThan(0); - const last = writes[writes.length - 1]; - const data = JSON.parse(last.content); + // DC-105: _saveState delegates to atomicWriteJSON — content lands via + // openSync('wx')+writeSync+rename, not writeFileSync to a fixed .tmp. + // The renamed destination must carry the muted host. + expect(mockFsState.exists[STATE]).toBe(true); + const data = JSON.parse(mockFsState.files[STATE]); expect(data.muted).toContain('1.1.1.1:80'); + // And the legacy fixed-name tmp path must NOT have been used. + expect(mockFsState.writeLog.filter(w => w.p === STATE + '.tmp').length).toBe(0); + }); + + // ---- DC-105: state file goes through the canonical atomic-write util ------ + + test('DC-105: _saveState uses atomicWriteJSON (wx tmp + fsync + rename, no fixed .tmp)', async () => { + seedSites({ 'a.sami': 'a.sami { reverse_proxy 1.1.1.1:80 }\n' }); + const { w } = loadWatcher(); + await w.scanSites(); + w.setMuted('1.1.1.1:80', true); + + const fs = require('fs'); + // The canonical writer must have been used: open with 'wx' (exclusive + // create), fsync before close, then rename onto the destination. + expect(fs.openSync).toHaveBeenCalled(); + expect(fs.fsyncSync).toHaveBeenCalled(); + expect(fs.closeSync).toHaveBeenCalled(); + const renames = fs.renameSync.mock.calls.filter(c => c[1] === STATE); + expect(renames.length).toBeGreaterThan(0); + // Tmp names are hidden dotfiles in the same dir with pid+counter — the + // old fixed `STATE + '.tmp'` collision window between concurrent saves + // (probe loop vs setMuted) is gone. + for (const [src] of renames) { + expect(src).toMatch(/[\\/].caddy-upstreams-test[.]json[.]tmp-/); + expect(src).not.toBe(STATE + '.tmp'); + } + // No leftover tmp files after a successful save. + const leftovers = Object.keys(mockFsState.files) + .filter(p => p.includes('.tmp-')); + expect(leftovers).toEqual([]); + // Destination holds complete, parseable JSON with the mute. + const data = JSON.parse(mockFsState.files[STATE]); + expect(data.muted).toContain('1.1.1.1:80'); + expect(data.upstreams['1.1.1.1:80'].site).toBe('a.sami'); }); test('reload from state file restores muted list', async () => { // Pre-seed a state file with a muted host - fsState.files[STATE] = JSON.stringify({ + mockFsState.files[STATE] = JSON.stringify({ muted: ['99.99.99.99:80'], upstreams: { '99.99.99.99:80': { ip: '99.99.99.99', port: '80', site: 'old.sami', siteFile: 'old.sami' } } }); - fsState.exists[STATE] = true; + mockFsState.exists[STATE] = true; // And the matching site file seedSites({ 'old.sami': 'old.sami { reverse_proxy 99.99.99.99:80 }\n' }); process.env.CADDY_UPSTREAMS_STATE_FILE = STATE; diff --git a/dashcaddy-api/src/monitoring/caddy-upstream-watcher.js b/dashcaddy-api/src/monitoring/caddy-upstream-watcher.js index 7917cdb..58c782c 100644 --- a/dashcaddy-api/src/monitoring/caddy-upstream-watcher.js +++ b/dashcaddy-api/src/monitoring/caddy-upstream-watcher.js @@ -16,6 +16,8 @@ * * State persisted to /caddy-upstreams.json. Mute list is part of * the same file so atomic-write semantics keep state + mutes consistent. + * Writes go through the canonical atomic-write util (DC-099/DC-105): + * fsync'd same-dir tmp + rename — a crash can never tear the mute list. * * The probe DOES NOT use Caddy's health_uri (that's Caddy's own probe and * the source of the spam). The probe also stamps `X-DashCaddy-HealthCheck: 1` @@ -31,6 +33,7 @@ const https = require('https'); const http = require('http'); const EventEmitter = require('events'); const platformPaths = require('../../platform-paths'); +const { atomicWriteJSON } = require('../utils/atomic-write'); /** Default probe interval: 60s. Independent of Caddy's 10s internal probe. */ const PROBE_INTERVAL_MS = parseInt(process.env.CADDY_UPSTREAM_PROBE_INTERVAL_MS || '60000', 10); @@ -532,9 +535,12 @@ class CaddyUpstreamWatcher extends EventEmitter { verifiedViaBridge: !!v.verifiedViaBridge }; } - const tmp = STATE_FILE + '.tmp'; - fs.writeFileSync(tmp, JSON.stringify({ muted: Array.from(this.muted), upstreams }, null, 2)); - fs.renameSync(tmp, STATE_FILE); + // Canonical atomic-write (DC-099, migrated DC-105): fsync'd same-dir + // tmp + rename via the shared util. A crash mid-write can no longer + // tear caddy-upstreams.json (muted list + probe state) — the old + // writeFileSync-to-fixed-.tmp had no fsync, so a power loss could + // leave an empty/short state file and silently drop every mute. + atomicWriteJSON(STATE_FILE, { muted: Array.from(this.muted), upstreams }); } catch (e) { this.log.warn?.('caddy-upstream-watcher', `state save failed: ${e.message}`); }