Harden JSON-backed persistence: atomic writes + per-path serialization - #62
Merged
Conversation
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.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every JSON-backed service persisted state with a read-modify-write followed by a direct
fs.writeFile, with no locking and no atomic write. Two concrete risks:fs.writeFiletruncates the target and streams bytes in place. A crash (or container kill) mid-write leaves a truncated file that failsJSON.parseon the next startup — losing users, TSIG keys, tokens, or backups.apiTokenService.verifyToken()fires a throttledlastUsedAt"touch" fire-and-forget, which can race a concurrentcreateToken/revokeTokenand drop that change.Affected services:
tsigKeyService,apiTokenService,userService,backupService, pluswebhookConfigServiceandssoConfigService(same pattern).Fix
New shared helper
backend/src/utils/atomicJson.ts:writeFileAtomic/writeJsonAtomic— write to a uniquely-named temp file in the same directory,fsync, thenfs.renameover the target. Rename is atomic on the same filesystem, so a reader only ever observes the old complete file or the new complete file, never a partial one. The temp file is cleaned up on failure.withFileLock— serializes all operations for a given absolute path through a per-path promise chain, so concurrent callers apply one at a time in call order. A rejecting task never wedges the queue for later callers, and the map entry is dropped once the chain drains (no unbounded growth).removeFileLocked— deletes under the same lock (used by backup eviction, so a delete cannot interleave with a write to the same zone file).flushFileLock— drains in-flight work for a path (graceful shutdown / test teardown).Each service's write path now goes through the helper.
backupServicestill writes/deletes multiple zone files per operation — each is now individually atomic and serialized, while distinct files stay concurrent.Behavior is unchanged: same
JSON.stringify(value, null, 2)on-disk shape; only how the bytes hit disk changed. Read paths and data shapes are untouched.Tests
backend/src/utils/__tests__/atomicJson.test.ts: asserts the final write is arenameonto the target (not in-place); a failed write leaves the existing target intact and complete JSON with no stray.tmpfiles; 50 concurrent writes to one path all serialize with the last write winning (no lost update); same-path tasks run strictly ordered and never overlap; a rejecting task does not wedge the queue;removeFileLockedserializes against writes.apiTokenServicenow drains the fire-and-forget write viaflushFileLockbefore removing its temp dir.Verification:
cd backend && npm run build— clean.cd backend && npx jest— 39 suites, 369 tests, all green.🤖 Generated with Claude Code