Merge pull request #493 from HarperFast/fix/clone-config-atomic-write

fix: write Harper config files atomically

Author: kriszyp

config/configUtils.js

@@ -88,6 +88,14 @@ function getConfigPath(param) {
 	if (!rootPath) return value;
 	return path.resolve(rootPath, value);
 }
+
+// Write atomically via temp file + rename so readers don't observe a truncated/empty file
+function atomicWriteFile(filePath, content) {
+	const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`;
+	fs.writeFileSync(tempPath, content);
+	fs.renameSync(tempPath, filePath);
+}
+
 /**
  * Builds the Harper config file using user inputs and default values from defaultConfig.yaml
  * @param args - any args that the user provided.
@@ -164,7 +172,7 @@ function createConfigFile(args, skipFsValidation = false) {
 			true
 		);
 	}
-	fs.writeFileSync(configFilePath, String(configDoc));
+	atomicWriteFile(configFilePath, String(configDoc));
 	logger.trace(`Config file written to ${configFilePath}`);
 }
 
@@ -389,7 +397,7 @@ function checkForUpdatedConfig(configDoc, configFilePath) {
 				HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR
 			);
 		}
-		fs.writeFileSync(configFilePath, String(configDoc));
+		atomicWriteFile(configFilePath, String(configDoc));
 	}
 }
 
@@ -633,7 +641,7 @@ function updateConfigValue(
 			HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR
 		);
 	}
-	fs.writeFileSync(configFileLocation, String(configDoc));
+	atomicWriteFile(configFileLocation, String(configDoc));
 	if (update_config_obj) {
 		flatConfigObj = flattenConfig(configDoc.toJSON());
 	}
@@ -895,7 +903,7 @@ function applyRuntimeEnvVarConfig(configDoc, configFilePath, options = {}) {
 				HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR
 			);
 		}
-		fs.writeFileSync(configFilePath, String(configDoc));
+		atomicWriteFile(configFilePath, String(configDoc));
 		logger.debug('Config file updated with runtime env var values');
 	} catch (error) {
 		logger.error(`Failed to write config file after applying runtime env vars: ${error.message}`);
@@ -956,7 +964,7 @@ async function addConfig(topLevelElement, values) {
 			HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR
 		);
 	}
-	await fs.writeFile(getConfigFilePath(), String(configDoc));
+	atomicWriteFile(getConfigFilePath(), String(configDoc));
 }
 
 function deleteConfigFromFile(param) {
@@ -965,7 +973,7 @@ function deleteConfigFromFile(param) {
 	configDoc.deleteIn(param);
 	const hdbRoot = configDoc.getIn(['rootPath']);
 	const configFileLocation = path.join(hdbRoot, hdbTerms.HARPER_CONFIG_FILE);
-	fs.writeFileSync(configFileLocation, String(configDoc));
+	atomicWriteFile(configFileLocation, String(configDoc));
 }
 
 function getConfigObj() {

unitTests/config/configUtils-runtimeEnvVars.test.js

@@ -11,13 +11,15 @@ describe('configUtils - applyRuntimeEnvVarConfig', function () {
 	let mockConfigDoc;
 	let applyRuntimeEnvConfigStub;
 	let fsWriteFileSyncStub;
+	let fsRenameSyncStub;
 	let loggerStub;
 	let YAMLStub;
 
 	before(function () {
 		// Create stubs for dependencies
 		applyRuntimeEnvConfigStub = sinon.stub();
 		fsWriteFileSyncStub = sinon.stub();
+		fsRenameSyncStub = sinon.stub();
 		loggerStub = {
 			debug: sinon.stub(),
 			warn: sinon.stub(),
@@ -32,7 +34,7 @@ describe('configUtils - applyRuntimeEnvVarConfig', function () {
 
 		// Inject stubs
 		configUtils.__set__('logger', loggerStub);
-		configUtils.__set__('fs', { writeFileSync: fsWriteFileSyncStub });
+		configUtils.__set__('fs', { writeFileSync: fsWriteFileSyncStub, renameSync: fsRenameSyncStub });
 		configUtils.__set__('YAML', YAMLStub);
 
 		// Mock harperConfigEnvVars module
@@ -48,6 +50,7 @@ describe('configUtils - applyRuntimeEnvVarConfig', function () {
 		// Reset stubs
 		applyRuntimeEnvConfigStub.reset();
 		fsWriteFileSyncStub.reset();
+		fsRenameSyncStub.reset();
 		loggerStub.debug.reset();
 		loggerStub.warn.reset();
 		loggerStub.error.reset();
@@ -140,8 +143,13 @@ describe('configUtils - applyRuntimeEnvVarConfig', function () {
 
 		applyRuntimeEnvVarConfig(mockConfigDoc, '/test/config.yaml');
 
+		// Atomic write: writeFileSync writes to a temp path, then renameSync moves it over the target
 		assert.strictEqual(fsWriteFileSyncStub.called, true);
-		assert.strictEqual(fsWriteFileSyncStub.firstCall.args[0], '/test/config.yaml');
+		const writeTarget = fsWriteFileSyncStub.firstCall.args[0];
+		assert.ok(writeTarget.startsWith('/test/config.yaml.'), `expected temp path, got ${writeTarget}`);
+		assert.ok(writeTarget.endsWith('.tmp'), `expected .tmp suffix, got ${writeTarget}`);
+		assert.strictEqual(fsRenameSyncStub.calledOnce, true);
+		assert.deepStrictEqual(fsRenameSyncStub.firstCall.args, [writeTarget, '/test/config.yaml']);
 		assert.strictEqual(loggerStub.debug.called, true);
 		assert.match(loggerStub.debug.firstCall.args[0], /Config file updated/);
 

unitTests/config/configUtils.test.js

@@ -136,6 +136,67 @@ describe('Test configUtils module', () => {
 		});
 	});
 
+	describe('Test atomicWriteFile function', () => {
+		const atomicWriteFile = config_utils_rw.__get__('atomicWriteFile');
+		const ATOMIC_TEST_DIR = path.join(DIRNAME, 'yaml');
+		const ATOMIC_TEST_PATH = path.join(ATOMIC_TEST_DIR, 'atomic-write-test.yaml');
+
+		before(() => {
+			fs.ensureDirSync(ATOMIC_TEST_DIR);
+		});
+
+		afterEach(() => {
+			try {
+				fs.unlinkSync(ATOMIC_TEST_PATH);
+			} catch {}
+			// Clean up any stray temp files from a failed run
+			for (const entry of fs.readdirSync(ATOMIC_TEST_DIR)) {
+				if (entry.startsWith('atomic-write-test.yaml.') && entry.endsWith('.tmp')) {
+					fs.unlinkSync(path.join(ATOMIC_TEST_DIR, entry));
+				}
+			}
+		});
+
+		it('writes content to the target path', () => {
+			atomicWriteFile(ATOMIC_TEST_PATH, 'rootPath: /tmp/hdb');
+			expect(fs.readFileSync(ATOMIC_TEST_PATH, 'utf8')).to.equal('rootPath: /tmp/hdb');
+		});
+
+		it('overwrites an existing file', () => {
+			fs.writeFileSync(ATOMIC_TEST_PATH, 'rootPath: /old');
+			atomicWriteFile(ATOMIC_TEST_PATH, 'rootPath: /new');
+			expect(fs.readFileSync(ATOMIC_TEST_PATH, 'utf8')).to.equal('rootPath: /new');
+		});
+
+		it('writes via temp file then rename so target is never truncated mid-write', () => {
+			const writeStub = sandbox.stub(fs, 'writeFileSync');
+			const renameStub = sandbox.stub(fs, 'renameSync');
+			try {
+				atomicWriteFile(ATOMIC_TEST_PATH, 'content');
+				expect(writeStub.calledOnce).to.be.true;
+				expect(renameStub.calledOnce).to.be.true;
+				const tempPath = writeStub.firstCall.args[0];
+				expect(tempPath).to.not.equal(ATOMIC_TEST_PATH);
+				expect(tempPath.startsWith(`${ATOMIC_TEST_PATH}.`)).to.be.true;
+				expect(tempPath.endsWith('.tmp')).to.be.true;
+				expect(path.dirname(tempPath)).to.equal(path.dirname(ATOMIC_TEST_PATH));
+				expect(renameStub.firstCall.args).to.deep.equal([tempPath, ATOMIC_TEST_PATH]);
+				expect(writeStub.calledBefore(renameStub)).to.be.true;
+			} finally {
+				writeStub.restore();
+				renameStub.restore();
+			}
+		});
+
+		it('leaves no temp file behind after a successful write', () => {
+			atomicWriteFile(ATOMIC_TEST_PATH, 'content');
+			const stragglers = fs
+				.readdirSync(path.dirname(ATOMIC_TEST_PATH))
+				.filter((e) => e.startsWith('atomic-write-test.yaml.') && e.endsWith('.tmp'));
+			expect(stragglers).to.be.empty;
+		});
+	});
+
 	describe('Test getDefaultConfig function', () => {
 		const expected_flat_default_config_obj = {
 			analytics_aggregateperiod: 60,

unitTests/config/rootConfigWatcher.test.js

@@ -3,7 +3,7 @@ const { RootConfigWatcher } = require('#src/config/RootConfigWatcher');
 const { tmpdir } = require('node:os');
 const { once } = require('node:events');
 const { join } = require('node:path');
-const { writeFileSync, mkdtempSync, rmSync } = require('node:fs');
+const { writeFileSync, mkdtempSync, rmSync, renameSync } = require('node:fs');
 const { writeFile } = require('node:fs/promises');
 const { replace, fake, restore, spy } = require('sinon');
 const configUtils = require('#js/config/configUtils');
@@ -56,4 +56,23 @@ describe('RootConfigWatcher', () => {
 			'RootConfigWatcher should not have a config property after close() is called'
 		);
 	});
+
+	it('should detect changes written via temp-file + rename (atomic write)', async () => {
+		const initial = { foo: 'bar' };
+		writeFileSync(this.configFilePath, stringify(initial));
+		const configWatcher = new RootConfigWatcher();
+
+		const [readyValue] = await configWatcher.ready;
+		assert.deepEqual(readyValue, initial, 'watcher should pick up initial config');
+
+		const updated = { foo: 'baz' };
+		const tempPath = `${this.configFilePath}.${process.pid}.${Date.now()}.tmp`;
+		writeFileSync(tempPath, stringify(updated));
+		renameSync(tempPath, this.configFilePath);
+
+		const [changeValue] = await once(configWatcher, 'change');
+		assert.deepEqual(changeValue, updated, 'watcher should fire change after atomic rename');
+
+		configWatcher.close();
+	});
 });