From feaf37a79d3625108ef8dd1a56a8777f039528cb Mon Sep 17 00:00:00 2001 From: Tim Pearson Date: Tue, 21 Jul 2026 22:56:27 -0400 Subject: [PATCH] fix(backend): make JSON-backed persistence atomic and race-free The JSON-backed services (tsigKey, apiToken, user, backup, webhookConfig, ssoConfig) persisted with a read-modify-write plus a direct fs.writeFile. Two failure modes: - Torn writes: fs.writeFile truncates and streams in place, so a crash mid-write leaves a truncated/corrupt file that fails to parse on restart. - Lost updates: overlapping writers to the same file (e.g. apiTokenService's fire-and-forget lastUsedAt touch racing a create/revoke) can interleave and clobber each other. Add backend/src/utils/atomicJson.ts: - writeFileAtomic/writeJsonAtomic write to a sibling temp file, fsync, then fs.rename over the target (atomic on the same filesystem), so a reader only ever sees the old or new complete file. - withFileLock serializes all operations for a given absolute path through a per-path promise chain; a rejecting task never wedges the queue and the map entry is dropped when the chain drains. - removeFileLocked deletes under the same lock (backup eviction), flushFileLock drains in-flight work (graceful shutdown / test teardown). Route every service write through the helper. On-disk shape is unchanged (same JSON.stringify(value, null, 2) output); only how bytes hit disk changes. Read paths are untouched. Add focused unit tests for the helper (temp+rename, target intact on failed write, no lost update under 50 concurrent writes, ordered non-overlapping execution, queue survives a rejection) and drain the fire-and-forget write in the apiTokenService test teardown. --- .../__tests__/apiTokenService.test.ts | 4 + backend/src/services/apiTokenService.ts | 6 +- backend/src/services/backupService.ts | 17 ++- backend/src/services/ssoConfigService.ts | 5 +- backend/src/services/tsigKeyService.ts | 5 +- backend/src/services/userService.ts | 6 +- backend/src/services/webhookConfigService.ts | 4 +- .../src/utils/__tests__/atomicJson.test.ts | 122 ++++++++++++++++++ backend/src/utils/atomicJson.ts | 117 +++++++++++++++++ 9 files changed, 275 insertions(+), 11 deletions(-) create mode 100644 backend/src/utils/__tests__/atomicJson.test.ts create mode 100644 backend/src/utils/atomicJson.ts diff --git a/backend/src/services/__tests__/apiTokenService.test.ts b/backend/src/services/__tests__/apiTokenService.test.ts index f740198..e6e79c1 100644 --- a/backend/src/services/__tests__/apiTokenService.test.ts +++ b/backend/src/services/__tests__/apiTokenService.test.ts @@ -8,6 +8,7 @@ import os from 'os'; import path from 'path'; import crypto from 'crypto'; import { ApiTokenService } from '../apiTokenService'; +import { flushFileLock } from '../../utils/atomicJson'; const sha256hex = (raw: string): string => crypto.createHash('sha256').update(raw).digest('hex'); @@ -26,6 +27,9 @@ describe('ApiTokenService', () => { afterEach(async () => { jest.restoreAllMocks(); + // verifyToken persists its lastUsedAt touch fire-and-forget; drain any + // in-flight atomic write so its temp file cannot race the directory removal. + await flushFileLock(filePath); await fs.rm(dir, { recursive: true, force: true }); }); diff --git a/backend/src/services/apiTokenService.ts b/backend/src/services/apiTokenService.ts index 62f8d7b..bb380ab 100644 --- a/backend/src/services/apiTokenService.ts +++ b/backend/src/services/apiTokenService.ts @@ -12,6 +12,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import crypto from 'crypto'; +import { writeJsonAtomic } from '../utils/atomicJson'; const TOKENS_FILE = path.join(process.cwd(), 'data', 'api-tokens.json'); @@ -107,7 +108,10 @@ export class ApiTokenService { */ private async save(): Promise { const arr = Array.from(this.tokens.values()); - await fs.writeFile(this.filePath, JSON.stringify(arr, null, 2), 'utf-8'); + // Atomic + per-path serialized write. This matters most for the throttled, + // fire-and-forget lastUsedAt touch in verifyToken(), which can otherwise + // race a create/revoke and lose an update. + await writeJsonAtomic(this.filePath, arr); } /** diff --git a/backend/src/services/backupService.ts b/backend/src/services/backupService.ts index 993361b..41d972b 100644 --- a/backend/src/services/backupService.ts +++ b/backend/src/services/backupService.ts @@ -4,6 +4,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import { BackupConfig, resolveBackupConfig } from '../config/backups'; +import { writeJsonAtomic, removeFileLocked } from '../utils/atomicJson'; const MAX_BACKUPS_PER_ZONE = 50; @@ -276,14 +277,20 @@ export class BackupService { } } - /** Write changed zone files; remove any that were emptied by eviction. */ + /** + * Write changed zone files; remove any that were emptied by eviction. Each + * write is atomic (temp file + rename) and each file's write/remove is + * serialized against other writers of that same path, so a crash cannot + * truncate a zone file and a concurrent createBackup for another zone cannot + * clobber this one. Distinct files still persist concurrently. + */ private async persistFiles(files: Map, changed: Set): Promise { for (const file of changed) { const arr = files.get(file) ?? []; if (arr.length === 0) { - await fs.rm(file, { force: true }); + await removeFileLocked(file); } else { - await fs.writeFile(file, JSON.stringify(arr, null, 2), 'utf-8'); + await writeJsonAtomic(file, arr); } } } @@ -313,8 +320,8 @@ export class BackupService { // Remove the backup backups = backups.filter(b => b.id !== backupId); - // Save updated list - await fs.writeFile(filePath, JSON.stringify(backups, null, 2), 'utf-8'); + // Save updated list atomically and serialized against other writers. + await writeJsonAtomic(filePath, backups); console.log(`Backup deleted: ${backupId} from zone ${zone} by user ${userId}`); } catch (error) { diff --git a/backend/src/services/ssoConfigService.ts b/backend/src/services/ssoConfigService.ts index 3acfd9a..5ba1b8b 100644 --- a/backend/src/services/ssoConfigService.ts +++ b/backend/src/services/ssoConfigService.ts @@ -3,6 +3,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import crypto from 'crypto'; import { SSOConfig, SSOConfigResponse, SSOProvider } from '../types/sso'; +import { writeJsonAtomic } from '../utils/atomicJson'; const SSO_CONFIG_FILE = path.join(process.cwd(), 'data', 'sso-config.json'); const ENCRYPTION_KEY = process.env.ENCRYPTION_KEY || 'default-encryption-key-change-in-production'; @@ -99,8 +100,8 @@ class SSOConfigService { configToStore.clientSecret = this.encrypt(configToStore.clientSecret); } - // Save to file - await fs.writeFile(SSO_CONFIG_FILE, JSON.stringify(configToStore, null, 2), 'utf-8'); + // Save to file atomically and serialized against other writers. + await writeJsonAtomic(SSO_CONFIG_FILE, configToStore); console.log(`SSO config updated: ${this.config.enabled ? 'enabled' : 'disabled'} (${this.config.provider})`); } diff --git a/backend/src/services/tsigKeyService.ts b/backend/src/services/tsigKeyService.ts index 6eb8f76..a2f3876 100644 --- a/backend/src/services/tsigKeyService.ts +++ b/backend/src/services/tsigKeyService.ts @@ -5,6 +5,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import crypto from 'crypto'; import { resolveTsigEncryptionKey } from '../config/secrets'; +import { writeJsonAtomic } from '../utils/atomicJson'; const KEYS_FILE = path.join(process.cwd(), 'data', 'tsig-keys.json'); @@ -122,7 +123,9 @@ class TSIGKeyService { */ private async saveKeys(): Promise { const keysArray = Array.from(this.keys.values()); - await fs.writeFile(KEYS_FILE, JSON.stringify(keysArray, null, 2), 'utf-8'); + // Atomic + per-path serialized write so a crash cannot truncate the file and + // concurrent callers cannot clobber each other. + await writeJsonAtomic(KEYS_FILE, keysArray); } /** diff --git a/backend/src/services/userService.ts b/backend/src/services/userService.ts index 03bd999..a8fb4a0 100644 --- a/backend/src/services/userService.ts +++ b/backend/src/services/userService.ts @@ -3,6 +3,7 @@ import bcrypt from 'bcrypt'; import { promises as fs } from 'fs'; import path from 'path'; import { User, UserCreateData, UserResponse, UserRole } from '../types/auth'; +import { writeJsonAtomic } from '../utils/atomicJson'; const SALT_ROUNDS = 12; const USERS_FILE = path.join(process.cwd(), 'data', 'users.json'); @@ -127,7 +128,10 @@ class UserService { private async saveUsers(): Promise { try { const usersArray = Array.from(this.users.values()); - await fs.writeFile(USERS_FILE, JSON.stringify(usersArray, null, 2), 'utf-8'); + // Atomic + per-path serialized write so a crash cannot truncate the file + // and concurrent saves (e.g. a login touching lastLogin while an admin + // edits a user) cannot clobber each other. + await writeJsonAtomic(USERS_FILE, usersArray); } catch (error) { console.error('Failed to save users:', error); throw error; diff --git a/backend/src/services/webhookConfigService.ts b/backend/src/services/webhookConfigService.ts index baa15a0..92a7028 100644 --- a/backend/src/services/webhookConfigService.ts +++ b/backend/src/services/webhookConfigService.ts @@ -3,6 +3,7 @@ import { promises as fs } from 'fs'; import path from 'path'; +import { writeJsonAtomic } from '../utils/atomicJson'; const WEBHOOK_CONFIG_FILE = path.join(process.cwd(), 'data', 'webhook-configs.json'); @@ -59,7 +60,8 @@ class WebhookConfigService { */ private async saveConfigs(): Promise { const configsArray = Array.from(this.configs.values()); - await fs.writeFile(WEBHOOK_CONFIG_FILE, JSON.stringify(configsArray, null, 2), 'utf-8'); + // Atomic + per-path serialized write (see utils/atomicJson). + await writeJsonAtomic(WEBHOOK_CONFIG_FILE, configsArray); } /** diff --git a/backend/src/utils/__tests__/atomicJson.test.ts b/backend/src/utils/__tests__/atomicJson.test.ts new file mode 100644 index 0000000..3210c77 --- /dev/null +++ b/backend/src/utils/__tests__/atomicJson.test.ts @@ -0,0 +1,122 @@ +// backend/src/utils/__tests__/atomicJson.test.ts +// Verifies the two guarantees of the shared JSON persistence helper: +// 1. Crash safety: a write failure never leaves the target truncated/corrupt — +// the temp file is renamed into place, so the target is only ever complete +// JSON (or its previous complete contents). +// 2. Serialization: many concurrent writes to the same path apply one at a time +// in call order, so the last write wins and no update is lost or interleaved. + +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { writeJsonAtomic, writeFileAtomic, removeFileLocked, withFileLock } from '../atomicJson'; + +describe('atomicJson', () => { + let dir: string; + let target: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'snap-atomic-')); + target = path.join(dir, 'data.json'); + }); + + afterEach(async () => { + jest.restoreAllMocks(); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('writes via a temp file and renames over the target', async () => { + const renameSpy = jest.spyOn(fs, 'rename'); + + await writeJsonAtomic(target, { hello: 'world' }); + + // The final write must be a rename onto the target, not an in-place write. + expect(renameSpy).toHaveBeenCalledTimes(1); + const [from, to] = renameSpy.mock.calls[0]; + expect(to).toBe(target); + expect(from).not.toBe(target); + + const parsed = JSON.parse(await fs.readFile(target, 'utf-8')); + expect(parsed).toEqual({ hello: 'world' }); + }); + + it('leaves an existing target intact and drops the temp file when the write fails', async () => { + // Seed a valid target so we can prove it survives a failed overwrite. + await writeJsonAtomic(target, { good: true }); + + // Simulate a crash mid-rename (data already streamed to temp, rename fails). + jest.spyOn(fs, 'rename').mockRejectedValueOnce(new Error('simulated crash')); + + await expect(writeJsonAtomic(target, { good: false })).rejects.toThrow('simulated crash'); + + // Target still parses and still holds the previous complete contents — + // never a truncated/partial file. + const parsed = JSON.parse(await fs.readFile(target, 'utf-8')); + expect(parsed).toEqual({ good: true }); + + // No stray temp files left behind in the directory. + const leftovers = (await fs.readdir(dir)).filter(n => n.endsWith('.tmp')); + expect(leftovers).toEqual([]); + }); + + it('creates a fresh target atomically even when none exists', async () => { + await writeFileAtomic(target, 'plain-contents'); + expect(await fs.readFile(target, 'utf-8')).toBe('plain-contents'); + }); + + it('serializes many concurrent writes to the same path with no lost update', async () => { + // Fire 50 overlapping writes at the same file. Without serialization their + // temp-file writes and renames would interleave; with it, the last enqueued + // write wins and the file is always complete JSON. + const count = 50; + const writes = Array.from({ length: count }, (_, i) => writeJsonAtomic(target, { seq: i })); + await Promise.all(writes); + + const parsed = JSON.parse(await fs.readFile(target, 'utf-8')); + // The final enqueued write (highest seq) is the durable one. + expect(parsed).toEqual({ seq: count - 1 }); + }); + + it('runs same-path tasks in enqueue order and never overlaps them', async () => { + const events: string[] = []; + const gated = (id: number, delayMs: number) => + withFileLock(target, async () => { + events.push(`start-${id}`); + await new Promise(r => setTimeout(r, delayMs)); + events.push(`end-${id}`); + }); + + // First task is slowest; if tasks overlapped, its end would come after a + // later task's start. Serialization forces strict start/end pairing in order. + await Promise.all([gated(0, 30), gated(1, 5), gated(2, 5)]); + + expect(events).toEqual([ + 'start-0', 'end-0', + 'start-1', 'end-1', + 'start-2', 'end-2', + ]); + }); + + it('does not wedge the queue when one task rejects', async () => { + const results: string[] = []; + const ok1 = withFileLock(target, async () => { results.push('a'); }); + const boom = withFileLock(target, async () => { throw new Error('boom'); }); + const ok2 = withFileLock(target, async () => { results.push('c'); }); + + await ok1; + await expect(boom).rejects.toThrow('boom'); + await ok2; // later caller still runs despite the middle rejection + + expect(results).toEqual(['a', 'c']); + }); + + it('removes a file under the same lock, serialized against writes', async () => { + await writeJsonAtomic(target, { x: 1 }); + // Enqueue a write then a remove; the remove must win as the last operation. + const w = writeJsonAtomic(target, { x: 2 }); + const r = removeFileLocked(target); + await Promise.all([w, r]); + + await expect(fs.readFile(target, 'utf-8')).rejects.toMatchObject({ code: 'ENOENT' }); + }); +}); diff --git a/backend/src/utils/atomicJson.ts b/backend/src/utils/atomicJson.ts new file mode 100644 index 0000000..f6eb0b1 --- /dev/null +++ b/backend/src/utils/atomicJson.ts @@ -0,0 +1,117 @@ +// backend/src/utils/atomicJson.ts +// Crash-safe, race-free JSON persistence shared by the JSON-backed services. +// +// Two hazards this addresses: +// 1. Torn writes. A plain fs.writeFile truncates the target and streams bytes +// in place, so a crash mid-write leaves a truncated/corrupt file that fails +// to parse on the next start. We instead write a sibling temp file and +// fs.rename() it over the target: rename is atomic on the same filesystem, +// so a reader ever only observes the old complete file or the new complete +// file, never a partial one. +// 2. Lost updates. The services do read-modify-write against in-memory state +// and then persist. Two overlapping writers (e.g. a fire-and-forget +// "lastUsedAt" touch racing a create) can interleave their rename calls and +// clobber each other. We serialize all writes to a given path through a +// per-path promise chain so they apply one at a time in call order. + +import { promises as fs } from 'fs'; +import path from 'path'; +import crypto from 'crypto'; + +// One in-flight promise chain per absolute file path. Each new operation is +// appended to the tail so callers targeting the same file run sequentially; +// operations on different files stay fully concurrent. +const locks = new Map>(); + +/** + * Run `task` with exclusive access to `filePath` relative to every other caller + * that goes through this helper. Tasks for the same resolved path execute in the + * order they were enqueued; a task that rejects does not break the chain for + * later callers. The map entry is dropped once the chain drains so it cannot + * grow without bound. + */ +export function withFileLock(filePath: string, task: () => Promise): Promise { + const key = path.resolve(filePath); + const prior = locks.get(key) ?? Promise.resolve(); + + // Chain after any in-flight work regardless of whether it settled ok or not. + const result = prior.then(task, task); + + // The stored tail must always resolve so one caller's failure never wedges the + // queue for the next caller. + const tail = result.then( + () => undefined, + () => undefined, + ); + locks.set(key, tail); + + // Best-effort cleanup: only clear the entry if nothing newer replaced it. + void tail.then(() => { + if (locks.get(key) === tail) locks.delete(key); + }); + + return result; +} + +/** + * Resolve once all currently-enqueued work for `filePath` has drained. Useful + * for graceful shutdown and for tests that trigger fire-and-forget writes (e.g. + * apiTokenService's throttled lastUsedAt touch) and must wait for them to hit + * disk before tearing the directory down. + */ +export function flushFileLock(filePath: string): Promise { + const key = path.resolve(filePath); + const prior = locks.get(key); + return prior ? prior.then(() => undefined, () => undefined) : Promise.resolve(); +} + +/** + * Atomically replace `filePath` with `contents`: write to a uniquely-named temp + * file in the same directory, fsync it so the bytes are durable, then rename + * over the target. The temp file is cleaned up on failure. Callers should + * normally use writeJsonAtomic; this is exposed for non-JSON payloads. + */ +export async function writeFileAtomic(filePath: string, contents: string): Promise { + const dir = path.dirname(filePath); + const tmp = path.join( + dir, + `.${path.basename(filePath)}.${process.pid}.${crypto.randomBytes(6).toString('hex')}.tmp`, + ); + + try { + // Open, write and fsync via a handle so the temp file is fully on disk + // before it becomes visible under the target name. + const handle = await fs.open(tmp, 'w'); + try { + await handle.writeFile(contents, 'utf-8'); + await handle.sync(); + } finally { + await handle.close(); + } + + await fs.rename(tmp, filePath); + } catch (error) { + // Never leave a stray temp file behind on a failed write. + await fs.rm(tmp, { force: true }).catch(() => undefined); + throw error; + } +} + +/** + * Serialize `value` as pretty-printed JSON (matching the services' existing + * on-disk shape) and write it atomically, serialized against other writers of + * the same path. + */ +export function writeJsonAtomic(filePath: string, value: unknown): Promise { + const contents = JSON.stringify(value, null, 2); + return withFileLock(filePath, () => writeFileAtomic(filePath, contents)); +} + +/** + * Remove `filePath` (no error if absent), serialized against writers of the same + * path so a delete and a concurrent write cannot interleave. Used by + * backupService when eviction empties a zone file. + */ +export function removeFileLocked(filePath: string): Promise { + return withFileLock(filePath, () => fs.rm(filePath, { force: true })); +}