refactor(main): improve config file handling

Simplify config file watcher by debouncing events instead of throttling.

Make sure to flush config changes on application exit.

Signed-off-by: Kristóf Marussy <kristof@marussy.com>

4 files changed, 77 insertions, 68 deletions

diff --git a/packages/main/src/infrastructure/config/ConfigRepository.ts b/packages/main/src/infrastructure/config/ConfigRepository.ts
index 7d5e12b..182f446 100644
--- a/packages/main/src/infrastructure/config/ConfigRepository.ts
+++ b/packages/main/src/infrastructure/config/ConfigRepository.ts
@@ -29,5 +29,5 @@ export default interface ConfigPersistence {
 
   writeConfig(contents: string): Promise<void>;
 
-  watchConfig(callback: () => Promise<void>, throttleMs: number): Disposer;
+  watchConfig(callback: () => Promise<void>): Disposer;
 }
diff --git a/packages/main/src/infrastructure/config/impl/ConfigFile.ts b/packages/main/src/infrastructure/config/impl/ConfigFile.ts
index 4f0d4f0..8f0cc3f 100644
--- a/packages/main/src/infrastructure/config/impl/ConfigFile.ts
+++ b/packages/main/src/infrastructure/config/impl/ConfigFile.ts
@@ -22,7 +22,7 @@ import { watch } from 'node:fs';
 import { readFile, stat, writeFile } from 'node:fs/promises';
 import path from 'node:path';
 
-import { throttle } from 'lodash-es';
+import { debounce } from 'lodash-es';
 
 import type Disposer from '../../../utils/Disposer.js';
 import getLogger from '../../../utils/getLogger.js';
@@ -33,6 +33,7 @@ import type { ReadConfigResult } from '../ConfigRepository.js';
 const log = getLogger('ConfigFile');
 
 export const CONFIG_FILE_NAME = 'settings.json';
+export const DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS = 10;
 
 export default class ConfigFile implements ConfigRepository {
   private readonly configFilePath: string;
@@ -44,6 +45,7 @@ export default class ConfigFile implements ConfigRepository {
   constructor(
     private readonly userDataDir: string,
     private readonly configFileName = CONFIG_FILE_NAME,
+    private readonly debounceTime = DEFAULT_CONFIG_CHANGE_DEBOUNCE_MS,
   ) {
     this.configFilePath = path.join(userDataDir, configFileName);
   }
@@ -59,7 +61,7 @@ export default class ConfigFile implements ConfigRepository {
       }
       throw error;
     }
-    log.info('Read config file', this.configFilePath);
+    log.debug('Read config file', this.configFilePath);
     return {
       found: true,
       contents,
@@ -82,10 +84,10 @@ export default class ConfigFile implements ConfigRepository {
     log.debug('Wrote config file', this.configFilePath);
   }
 
-  watchConfig(callback: () => Promise<void>, throttleMs: number): Disposer {
+  watchConfig(callback: () => Promise<void>): Disposer {
     log.debug('Installing watcher for', this.userDataDir);
 
-    const configChanged = throttle(async () => {
+    const configChanged = debounce(async () => {
       let mtime: Date;
       try {
         const stats = await stat(this.configFilePath);
@@ -116,9 +118,13 @@ export default class ConfigFile implements ConfigRepository {
           'which is newer than last written',
           this.timeLastWritten,
         );
-        await callback();
+        try {
+          await callback();
+        } catch (error) {
+          log.error('Callback error while listening for config changes', error);
+        }
       }
-    }, throttleMs);
+    }, this.debounceTime);
 
     const watcher = watch(
       this.userDataDir,
@@ -127,8 +133,12 @@ export default class ConfigFile implements ConfigRepository {
         recursive: false,
       },
       (_eventType, filename) => {
+        // We handle both `rename` and `change` events for maximum portability.
+        // This may result in multiple calls to `configChanged` for a single config change,
+        // so we debounce it with a short (imperceptible) delay.
         if (filename === this.configFileName || filename === null) {
           configChanged()?.catch((err) => {
+            // This should never happen, because `configChanged` handles all exceptions.
             log.error(
               'Unhandled error while listening for config changes',
               err,
@@ -141,6 +151,7 @@ export default class ConfigFile implements ConfigRepository {
     return () => {
       log.trace('Removing watcher for', this.configFilePath);
       watcher.close();
+      configChanged.cancel();
     };
   }
 }
diff --git a/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts b/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts
index b61e85a..dd4aaaa 100644
--- a/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts
+++ b/packages/main/src/infrastructure/config/impl/__tests__/ConfigFile.integ.test.ts
@@ -26,6 +26,7 @@ import {
   rename,
   rm,
   stat,
+  utimes,
   writeFile,
 } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
@@ -37,7 +38,7 @@ import { mocked } from 'jest-mock';
 import Disposer from '../../../../utils/Disposer.js';
 import ConfigFile, { CONFIG_FILE_NAME } from '../ConfigFile.js';
 
-const THROTTLE_MS = 1000;
+const CONFIG_CHANGE_DEBOUNCE_MS = 10;
 
 let filesystemDelay = 100;
 let realSetTimeout: typeof setTimeout;
@@ -45,35 +46,29 @@ let userDataDir: string | undefined;
 let configFilePath: string;
 let repository: ConfigFile;
 
+async function realDelay(ms: number): Promise<void> {
+  return new Promise<void>((resolve) => {
+    realSetTimeout(() => resolve(), ms);
+  });
+}
+
 /**
  * Wait for a short (real-time) delay to let node be notified of file changes.
  *
- * Will also run all pending microtasks to completion.
- *
  * Use the `SOPHIE_INTEG_TEST_DELAY` environmental variable to customize the delay
  * (e.g., quicker test execution on faster computers or longer wait time in CI).
  * The default delay is 100 ms, but as little as 10 ms might be enough, depending on your system.
  *
- * @returns A promise that resolves in a short while.
+ * @param ms The time that should elapse after noticing the filesystem change.
+ * @returns A promise that resolves in a `filesystemDelay` ms.
  */
-function catchUpWithFilesystem(): Promise<void> {
-  return new Promise((resolve, reject) => {
-    realSetTimeout(() => {
-      try {
-        jest.runAllTicks();
-      } catch (error) {
-        reject(error);
-        return;
-      }
-      resolve();
-    }, filesystemDelay);
-  });
-}
-
-async function catchUpWithFilesystemAndTimers(): Promise<void> {
-  await catchUpWithFilesystem();
-  jest.runAllTimers();
-  await catchUpWithFilesystem();
+async function catchUpWithFilesystem(
+  ms = CONFIG_CHANGE_DEBOUNCE_MS,
+): Promise<void> {
+  await realDelay(filesystemDelay);
+  jest.advanceTimersByTime(ms);
+  // Some extra real time is needed after advancing the timer to make sure the callback runs.
+  await realDelay(Math.max(filesystemDelay / 10, 1));
 }
 
 beforeAll(() => {
@@ -91,7 +86,11 @@ beforeEach(async () => {
   try {
     userDataDir = await mkdtemp(path.join(tmpdir(), 'sophie-configFile-'));
     configFilePath = path.join(userDataDir, CONFIG_FILE_NAME);
-    repository = new ConfigFile(userDataDir);
+    repository = new ConfigFile(
+      userDataDir,
+      CONFIG_FILE_NAME,
+      CONFIG_CHANGE_DEBOUNCE_MS,
+    );
   } catch (error) {
     userDataDir = undefined;
     throw error;
@@ -180,7 +179,7 @@ describe('watchConfig', () => {
 
   describe('when the config file does not exist', () => {
     beforeEach(() => {
-      watcher = repository.watchConfig(callback, THROTTLE_MS);
+      watcher = repository.watchConfig(callback);
     });
 
     test('notifies when the config file is created externally', async () => {
@@ -191,7 +190,7 @@ describe('watchConfig', () => {
 
     test('does not notify when the config file is created by the repository', async () => {
       await repository.writeConfig('Hello World!');
-      await catchUpWithFilesystemAndTimers();
+      await catchUpWithFilesystem();
       expect(callback).not.toHaveBeenCalled();
     });
   });
@@ -199,7 +198,7 @@ describe('watchConfig', () => {
   describe('when the config file already exists', () => {
     beforeEach(async () => {
       await writeFile(configFilePath, 'Hello World!', 'utf8');
-      watcher = repository.watchConfig(callback, THROTTLE_MS);
+      watcher = repository.watchConfig(callback);
     });
 
     test('notifies when the config file is updated externally', async () => {
@@ -210,35 +209,23 @@ describe('watchConfig', () => {
 
     test('does not notify when the config file is created by the repository', async () => {
       await repository.writeConfig('Hi Mars!');
-      await catchUpWithFilesystemAndTimers();
+      await catchUpWithFilesystem();
       expect(callback).not.toHaveBeenCalled();
     });
 
-    test('throttles notifications of external changes to the config file', async () => {
+    test('debounces changes to the config file', async () => {
       await writeFile(configFilePath, 'Hi Mars!', 'utf8');
-      await catchUpWithFilesystem();
-      expect(callback).toHaveBeenCalledTimes(1);
-      mocked(callback).mockClear();
-
-      jest.advanceTimersByTime(100);
-      await writeFile(configFilePath, 'Howdy Venus!', 'utf8');
-      await catchUpWithFilesystem();
-
-      jest.advanceTimersByTime(100);
-      await catchUpWithFilesystem();
+      await catchUpWithFilesystem(5);
       expect(callback).not.toHaveBeenCalled();
 
-      jest.advanceTimersByTime(THROTTLE_MS);
+      await writeFile(configFilePath, 'Hi Mars!', 'utf8');
       await catchUpWithFilesystem();
       expect(callback).toHaveBeenCalledTimes(1);
     });
 
     test('handles the config file being deleted', async () => {
-      await writeFile(configFilePath, 'Hi Mars!', 'utf8');
-      // Clear the mock if we inadverently set off `callback`.
-      mocked(callback).mockClear();
       await rm(configFilePath);
-      await catchUpWithFilesystemAndTimers();
+      await catchUpWithFilesystem();
       expect(callback).not.toHaveBeenCalled();
 
       await writeFile(configFilePath, 'Hello World!', 'utf8');
@@ -249,15 +236,12 @@ describe('watchConfig', () => {
     test('handles the config file being renamed', async () => {
       const renamedPath = `${configFilePath}.renamed`;
 
-      await writeFile(configFilePath, 'Hi Mars!', 'utf8');
-      // Clear the mock if we inadverently set off `callback`.
-      mocked(callback).mockClear();
       await rename(configFilePath, renamedPath);
-      await catchUpWithFilesystemAndTimers();
+      await catchUpWithFilesystem();
       expect(callback).not.toHaveBeenCalled();
 
       await writeFile(renamedPath, 'Hello World!', 'utf8');
-      await catchUpWithFilesystemAndTimers();
+      await catchUpWithFilesystem();
       expect(callback).not.toHaveBeenCalled();
 
       await writeFile(configFilePath, 'Hello World!', 'utf8');
@@ -266,21 +250,27 @@ describe('watchConfig', () => {
     });
 
     test('handles other filesystem errors', async () => {
-      await writeFile(configFilePath, 'Hi Mars!', 'utf8');
-      // Clear the mock if we inadverently set off `callback`.
-      mocked(callback).mockClear();
       const { mode } = await stat(userDataDir!);
+      await writeFile(configFilePath, 'Hi Mars!', 'utf8');
       // Remove permission to force a filesystem error.
       // eslint-disable-next-line no-bitwise -- Compute reduced permissions.
       await chmod(userDataDir!, mode & 0o666);
       try {
-        await catchUpWithFilesystemAndTimers();
+        await catchUpWithFilesystem();
         expect(callback).not.toHaveBeenCalled();
       } finally {
         await chmod(userDataDir!, mode);
       }
     });
 
+    test('does not notify when the modification date is prior to the last write', async () => {
+      await repository.writeConfig('Hello World!');
+      const date = new Date(Date.now() - 3_600_000);
+      await utimes(configFilePath, date, date);
+      await catchUpWithFilesystem();
+      expect(callback).not.toHaveBeenCalled();
+    });
+
     test('handles callback errors', async () => {
       mocked(callback).mockRejectedValue(new Error('Test error'));
       await writeFile(configFilePath, 'Hi Mars!', 'utf8');
diff --git a/packages/main/src/reactions/synchronizeConfig.ts b/packages/main/src/reactions/synchronizeConfig.ts
index f3c8b4a..6044ee7 100644
--- a/packages/main/src/reactions/synchronizeConfig.ts
+++ b/packages/main/src/reactions/synchronizeConfig.ts
@@ -51,11 +51,16 @@ export default async function synchronizeConfig(
     const result = await repository.readConfig();
     if (result.found) {
       const { contents } = result;
+      if (contents === lastConfigOnDisk) {
+        // No need to re-apply config if we have already applied it.
+        return true;
+      }
       try {
         // This cast is unsound if the config file is invalid,
         // but we'll throw an error in the end anyways.
         const data = JSON.parse(contents) as Config;
         sharedStore.loadConfig(data);
+        log.info('Loaded config');
       } catch (error) {
         log.error('Failed to apply config snapshot', contents, error);
         return true;
@@ -75,16 +80,18 @@ export default async function synchronizeConfig(
     log.info('Created config file');
   }
 
+  const debouncedSerializeConfig = debounce(() => {
+    const serializedConfig = serializeConfig(sharedStore.config);
+    if (serializedConfig !== lastConfigOnDisk) {
+      writeConfig(serializedConfig).catch((error) => {
+        log.error('Failed to write config on config change', error);
+      });
+    }
+  }, debounceTime);
+
   const disposeReaction = reaction(
     () => sharedStore.config,
-    debounce(() => {
-      const serializedConfig = serializeConfig(sharedStore.config);
-      if (serializedConfig !== lastConfigOnDisk) {
-        writeConfig(serializedConfig).catch((error) => {
-          log.error('Failed to write config on config change', error);
-        });
-      }
-    }, debounceTime),
+    debouncedSerializeConfig,
   );
 
   const disposeWatcher = repository.watchConfig(async () => {
@@ -93,10 +100,11 @@ export default async function synchronizeConfig(
     } catch (error) {
       log.error('Failed to read config', error);
     }
-  }, debounceTime);
+  });
 
   return () => {
     disposeWatcher();
     disposeReaction();
+    debouncedSerializeConfig.flush();
   };
 }