fix(admin): hold the settings write lock for every writer

.changeset/settings-write-lock-1039.md

@@ -0,0 +1,5 @@
+---
+"nostream": patch
+---
+
+fix(admin): serialize every settings-file writer with the settings write lock

src/cli/commands/config.ts

@@ -11,6 +11,7 @@ import {
   toCategoryLabel,
   validatePathAgainstDefaults,
   validateSettings,
+  withSettingsWriteLock,
 } from '../../utils/settings-config'
 import {
   isSecretEnvKey,
@@ -128,25 +129,38 @@ export const runConfigSet = async (
     return 1
   }
 
-  const settings = loadUserSettings() as unknown as Record<string, unknown>
-  const next = setByPath(settings, path, parseTypedValue(rawValue, valueType))
+  // Read and write under the settings-file lock so a CLI update cannot clobber
+  // a moderation ban written by a running relay in between.
+  const outcome = await withSettingsWriteLock<
+    | { ok: true }
+    | { ok: false; validationIssues: Array<{ path: string; message: string }> }
+  >(() => {
+    const settings = loadUserSettings() as unknown as Record<string, unknown>
+    const next = setByPath(settings, path, parseTypedValue(rawValue, valueType))
+
+    if (options.validate !== false) {
+      const merged = loadMergedSettings() as unknown as Record<string, unknown>
+      const mergedNext = setByPath(merged, path, getByPath(next, path))
+      const validationIssues = validateSettings(mergedNext as any)
+
+      if (validationIssues.length > 0) {
+        return { ok: false as const, validationIssues }
+      }
+    }
 
-  if (options.validate !== false) {
-    const merged = loadMergedSettings() as unknown as Record<string, unknown>
-    const mergedNext = setByPath(merged, path, getByPath(next, path))
-    const validationIssues = validateSettings(mergedNext as any)
+    saveSettings(next as any)
 
-    if (validationIssues.length > 0) {
-      logError('Config update rejected by validation:')
-      for (const issue of validationIssues) {
-        logError(`- ${issue.path}: ${issue.message}`)
-      }
+    return { ok: true as const }
+  })
 
-      return 1
+  if ('validationIssues' in outcome) {
+    logError('Config update rejected by validation:')
+    for (const issue of outcome.validationIssues) {
+      logError(`- ${issue.path}: ${issue.message}`)
     }
-  }
 
-  saveSettings(next as any)
+    return 1
+  }
 
   logInfo(`Updated ${path}`)
 

src/controllers/admin/patch-notifications-controller.ts

@@ -19,6 +19,7 @@ import {
   loadUserSettings,
   saveSettings,
   validateSettings,
+  withSettingsWriteLock,
 } from '../../utils/settings-config'
 import { validateSchema } from '../../utils/validation'
 import { recordAdminAuditFromRequest } from '../../utils/record-admin-audit'
@@ -35,27 +36,44 @@ export class PatchAdminNotificationsController implements IController {
       return
     }
 
-    const merged = loadMergedSettings()
-    const current = getMergedAdminNotifications(merged)
-    const nextNotifications = mergeAdminNotificationsPatch(current, validation.value)
+    // Read and write under the same settings-file lock the moderation writer
+    // uses, so a notification save cannot clobber a ban written meanwhile.
+    const outcome = await withSettingsWriteLock<
+      | { ok: true; redacted: ReturnType<typeof getRedactedAdminNotifications> }
+      | { ok: false; issues: Array<{ path: string; message: string }> }
+    >(() => {
+      const merged = loadMergedSettings()
+      const current = getMergedAdminNotifications(merged)
+      const nextNotifications = mergeAdminNotificationsPatch(current, validation.value)
 
-    const userSettings = loadUserSettings() as Settings
-    const nextUserSettings = mergeDeepRight(userSettings, {
-      admin: {
-        ...userSettings.admin,
-        notifications: nextNotifications,
-      },
-    }) as Settings
+      const userSettings = loadUserSettings() as Settings
+      const nextUserSettings = mergeDeepRight(userSettings, {
+        admin: {
+          ...userSettings.admin,
+          notifications: nextNotifications,
+        },
+      }) as Settings
 
-    const mergedNext = mergeDeepRight(loadDefaults(), nextUserSettings) as Settings
+      const mergedNext = mergeDeepRight(loadDefaults(), nextUserSettings) as Settings
 
-    const issues = validateSettings(mergedNext)
-    if (issues.length > 0) {
-      response.status(400).setHeader('content-type', 'application/json').send({ error: 'Validation failed', issues })
+      const issues = validateSettings(mergedNext)
+      if (issues.length > 0) {
+        return { ok: false as const, issues }
+      }
+
+      saveSettings(nextUserSettings)
+
+      return { ok: true as const, redacted: getRedactedAdminNotifications() }
+    })
+
+    if ('issues' in outcome) {
+      response
+        .status(400)
+        .setHeader('content-type', 'application/json')
+        .send({ error: 'Validation failed', issues: outcome.issues })
       return
     }
 
-    saveSettings(nextUserSettings)
     appendSettingsAuditLog({
       action: 'settings.updated',
       changes: [{ path: 'admin.notifications', reload: 'hot-reload' }],
@@ -74,7 +92,7 @@ export class PatchAdminNotificationsController implements IController {
 
     response.status(200).setHeader('content-type', 'application/json').send({
       ok: true,
-      notifications: getRedactedAdminNotifications(),
+      notifications: outcome.redacted,
     })
   }
 }

src/controllers/admin/patch-settings-controller.ts

@@ -12,6 +12,7 @@ import {
   setByPath,
   validatePathAgainstDefaults,
   validateSettings,
+  withSettingsWriteLock,
 } from '../../utils/settings-config'
 import {
   isWriteProtectedSettingsPath,
@@ -50,27 +51,43 @@ export class PatchAdminSettingsController implements IController {
       return
     }
 
-    const userSettings = loadUserSettings() as unknown as Record<string, unknown>
-    const nextUserSettings = changes.reduce(
-      (settings, change) => setByPath(settings, change.path, change.value),
-      userSettings,
-    )
-    const merged = loadMergedSettings() as unknown as Record<string, unknown>
-    const mergedNext = changes.reduce(
-      (settings, change) => setByPath(settings, change.path, getByPath(nextUserSettings, change.path)),
-      merged,
-    )
-    const validationIssues = validateSettings(mergedNext as unknown as Settings)
-
-    if (validationIssues.length > 0) {
+    // Every writer of settings.yaml takes this lock. Reading inside it is what
+    // stops an ordinary settings save from writing a document read before a
+    // moderation ban was applied, which would silently undo the ban.
+    const outcome = await withSettingsWriteLock<
+      | { ok: true; nextUserSettings: Record<string, unknown> }
+      | { ok: false; issues: Array<{ path: string; message: string }> }
+    >(() => {
+      const userSettings = loadUserSettings() as unknown as Record<string, unknown>
+      const nextUserSettings = changes.reduce(
+        (settings, change) => setByPath(settings, change.path, change.value),
+        userSettings,
+      )
+      const merged = loadMergedSettings() as unknown as Record<string, unknown>
+      const mergedNext = changes.reduce(
+        (settings, change) => setByPath(settings, change.path, getByPath(nextUserSettings, change.path)),
+        merged,
+      )
+      const validationIssues = validateSettings(mergedNext as unknown as Settings)
+
+      if (validationIssues.length > 0) {
+        return { ok: false as const, issues: validationIssues }
+      }
+
+      saveSettings(nextUserSettings as unknown as Settings)
+
+      return { ok: true as const, nextUserSettings }
+    })
+
+    if ('issues' in outcome) {
       response
         .status(400)
         .setHeader('content-type', 'application/json')
-        .send({ error: 'Validation failed', issues: validationIssues })
+        .send({ error: 'Validation failed', issues: outcome.issues })
       return
     }
 
-    saveSettings(nextUserSettings as unknown as Settings)
+    const nextUserSettings = outcome.nextUserSettings
     const updatedChanges = changes.map(({ path }) => ({
       path,
       value: redactSettingsValue(path, getByPath(nextUserSettings, path)),

src/controllers/admin/post-settings-restore-controller.ts

@@ -15,7 +15,7 @@ export class PostAdminSettingsRestoreController implements IController {
     }
 
     try {
-      restoreSettingsBackup(validation.value.filename)
+      await restoreSettingsBackup(validation.value.filename)
       appendSettingsAuditLog({
         action: 'settings.restored',
         filename: validation.value.filename,

src/utils/settings-config.ts

@@ -585,24 +585,26 @@ export const listSettingsBackups = (): SettingsBackupInfo[] => {
     .sort((left, right) => right.createdAt.localeCompare(left.createdAt))
 }
 
-export const restoreSettingsBackup = (filename: string): Settings => {
+export const restoreSettingsBackup = async (filename: string): Promise<Settings> => {
   if (!isSafeBackupFilename(filename)) {
     throw new Error('Invalid backup filename')
   }
 
-  const backupPath = join(getSettingsBackupDir(), filename)
-  if (!fs.existsSync(backupPath)) {
-    throw new Error('Backup not found')
-  }
+  return withSettingsWriteLock(() => {
+    const backupPath = join(getSettingsBackupDir(), filename)
+    if (!fs.existsSync(backupPath)) {
+      throw new Error('Backup not found')
+    }
 
-  const restored = yaml.load(fs.readFileSync(backupPath, 'utf-8')) as Settings
-  const issues = validateSettings(mergeDeepRight(loadDefaults(), restored) as Settings)
-  if (issues.length > 0) {
-    throw new Error(`Backup failed validation: ${issues.map((issue) => issue.path).join(', ')}`)
-  }
+    const restored = yaml.load(fs.readFileSync(backupPath, 'utf-8')) as Settings
+    const issues = validateSettings(mergeDeepRight(loadDefaults(), restored) as Settings)
+    if (issues.length > 0) {
+      throw new Error(`Backup failed validation: ${issues.map((issue) => issue.path).join(', ')}`)
+    }
 
-  saveSettings(restored)
-  return restored
+    saveSettings(restored)
+    return restored
+  })
 }
 
 export const getTopLevelSettingCategories = (): string[] => {

test/unit/controllers/admin/patch-notifications-controller.spec.ts

@@ -47,6 +47,7 @@ describe('PatchAdminNotificationsController', () => {
       send: sandbox.stub().returnsThis(),
     }
 
+    sandbox.stub(settingsConfig, 'withSettingsWriteLock').callsFake(async (operation: () => unknown) => operation())
     sandbox.stub(settingsConfig, 'loadMergedSettings').returns(baseMergedSettings as any)
     sandbox.stub(settingsConfig, 'loadUserSettings').returns({ admin: { notifications: baseNotifications } } as any)
     sandbox.stub(settingsConfig, 'saveSettings')
@@ -130,6 +131,30 @@ describe('PatchAdminNotificationsController', () => {
     expect(response.send.firstCall.args[0].ok).to.equal(true)
   })
 
+  it('reads and saves inside the settings write lock', async () => {
+    const order: string[] = []
+    ;(settingsConfig.withSettingsWriteLock as Sinon.SinonStub).callsFake(async (operation: () => unknown) => {
+      order.push('lock:enter')
+      try {
+        return operation()
+      } finally {
+        order.push('lock:exit')
+      }
+    })
+    ;(settingsConfig.loadUserSettings as Sinon.SinonStub).callsFake(() => {
+      order.push('read')
+      return { admin: { notifications: baseNotifications } }
+    })
+    ;(settingsConfig.saveSettings as Sinon.SinonStub).callsFake(() => {
+      order.push('save')
+    })
+
+    const controller = new PatchAdminNotificationsController(outboxRepository as any)
+    await controller.handleRequest({ body: { enabled: true }, ip: '127.0.0.1' } as any, response as any)
+
+    expect(order).to.deep.equal(['lock:enter', 'read', 'save', 'lock:exit'])
+  })
+
   it('still returns 200 when outbox enqueue fails', async () => {
     outboxRepository.enqueue.rejects(new Error('db unavailable'))
     const controller = new PatchAdminNotificationsController(outboxRepository as any)