diff --git a/frontend/sync-client/src/persistence/settings.test.ts b/frontend/sync-client/src/persistence/settings.test.ts index feb81715..b17df261 100644 --- a/frontend/sync-client/src/persistence/settings.test.ts +++ b/frontend/sync-client/src/persistence/settings.test.ts @@ -37,3 +37,23 @@ describe("Settings logging", () => { assert.equal(saved, 9); }); }); + +it("owns settings arrays instead of sharing them with callers and listeners", async () => { + const original = ["original"]; + const settings = new Settings( + new Logger(), + { ignorePatterns: original }, + async () => {} + ); + original.push("external mutation"); + assert.deepEqual(settings.getSettings().ignorePatterns, ["original"]); + const update = ["updated"]; + await settings.setSettings({ ignorePatterns: update }); + update.push("external mutation"); + assert.deepEqual(settings.getSettings().ignorePatterns, ["updated"]); + settings.onSettingsChanged.add((_, old) => { + old.ignorePatterns.push("listener mutation"); + }); + await settings.setSettings({ maxFileSizeMB: 12 }); + assert.deepEqual(settings.getSettings().ignorePatterns, ["updated"]); +}); diff --git a/frontend/sync-client/src/persistence/settings.ts b/frontend/sync-client/src/persistence/settings.ts index 3e046b03..9cc3bbe3 100644 --- a/frontend/sync-client/src/persistence/settings.ts +++ b/frontend/sync-client/src/persistence/settings.ts @@ -52,7 +52,7 @@ export class Settings { ) { this.settings = { ...DEFAULT_SETTINGS, - ...(initialState ?? {}) + ...structuredClone(initialState ?? {}) }; this.logger.debug( @@ -76,20 +76,27 @@ export class Settings { }); } - public async setSettings(value: Partial): Promise { - await this.lock.withLock(async () => { + public async setSettings( + value: Partial, + notify = true + ): Promise { + const update = structuredClone(value); + + const change = await this.lock.withLock(async () => { this.logger.debug( - `Updating settings with: ${JSON.stringify(redactSettings(value))}` + `Updating settings with: ${JSON.stringify(redactSettings(update))}` ); - const oldSettings = this.settings; - const next = { ...this.settings, ...value }; + + const oldSettings = this.getSettings(); + const next = { ...this.settings, ...update }; await this.saveData(next); this.settings = next; - await this.onSettingsChanged.triggerAsync( - this.getSettings(), - oldSettings - ); + return [this.getSettings(), oldSettings] as const; }); + + // Listeners can themselves update settings. Never await them while + // holding a lock needed by the operation they invoke. + if (notify) await this.onSettingsChanged.triggerAsync(...change); } }