fix: keep every key when several of a plugin's settings are saved at once

lib/admin/__tests__/plugin-config-concurrent.test.ts

@@ -0,0 +1,44 @@
+// @vitest-environment node
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { mkdtemp, readFile, rm } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
+
+// A plugin's admin config was read, changed and written back with nothing
+// in between to stop a second save, through one shared ".tmp" file: two
+// keys saved at once (the admin form saves each field) lost one of them or
+// answered 500.
+
+let dir: string;
+
+beforeEach(async () => {
+  dir = await mkdtemp(path.join(tmpdir(), 'bw-plugin-config-'));
+  vi.stubEnv('ADMIN_CONFIG_DIR', dir);
+  vi.resetModules();
+});
+
+afterEach(async () => {
+  vi.unstubAllEnvs();
+  await rm(dir, { recursive: true, force: true });
+});
+
+describe('plugin config saved at the same time', () => {
+  it('keeps every key', async () => {
+    const config = await import('../plugin-config');
+    const keys = Array.from({ length: 8 }, (_, i) => `key${i}`);
+    const results = await Promise.allSettled(keys.map((key, i) => config.setPluginConfig('cloud-sync', key, i)));
+
+    expect(results.filter((r) => r.status === 'rejected')).toEqual([]);
+    const expected = Object.fromEntries(keys.map((key, i) => [key, i]));
+    expect(await config.getPluginConfig('cloud-sync')).toEqual(expected);
+    expect(JSON.parse(await readFile(path.join(dir, 'plugin-config', 'cloud-sync.json'), 'utf-8'))).toEqual(expected);
+  });
+
+  it('keeps the other keys when one is deleted while another is set', async () => {
+    const config = await import('../plugin-config');
+    await config.setPluginConfig('cloud-sync', 'a', 1);
+    await config.setPluginConfig('cloud-sync', 'b', 2);
+    await Promise.all([config.deletePluginConfigKey('cloud-sync', 'a'), config.setPluginConfig('cloud-sync', 'c', 3)]);
+    expect(await config.getPluginConfig('cloud-sync')).toEqual({ b: 2, c: 3 });
+  });
+});

lib/admin/plugin-config.ts

@@ -1,32 +1,30 @@
-import { readFile, writeFile, mkdir, rename, unlink } from 'node:fs/promises';
-import { existsSync } from 'node:fs';
-import path from 'node:path';
 import { logger } from '@/lib/logger';
-import { getConfigDir, assertWritable } from './paths';
-
-function getPluginConfigDir(): string {
-  return path.join(getConfigDir(), 'plugin-config');
-}
-
-function configPath(pluginId: string): string {
-  return path.join(getPluginConfigDir(), `${pluginId}.json`);
-}
-
-async function ensureDir(dir: string): Promise<void> {
-  if (!existsSync(dir)) {
-    await mkdir(dir, { recursive: true });
-  }
-}
+import { isSharedMode } from '@/lib/shared-store';
+import { assertWritable } from './paths';
+import { isStoreUnavailable } from './admin-store';
+import { pluginConfigDocs } from './extension-store';
 
 /**
- * Get all config for a plugin.
+ * Admin config of each plugin (C/plugin-config/<id>.json, or the shared
+ * store in shared mode, #1066). Changes go through update(), so two admins
+ * setting different keys at once both keep theirs, on one instance or
+ * several.
  */
-export async function getPluginConfig(pluginId: string): Promise<Record<string, unknown>> {
+
+const fileOf = (pluginId: string) => `${pluginId}.json`;
+
+/** Shared mode: the last config read per plugin, served while the store is down. */
+const LAST_GOOD_KEY = Symbol.for('bulwark.admin.pluginConfig.lastGood');
+function lastGood(): Map<string, string> {
+  const g = globalThis as typeof globalThis & { [LAST_GOOD_KEY]?: Map<string, string> };
+  return (g[LAST_GOOD_KEY] ??= new Map());
+}
+
+function parseConfig(pluginId: string, data: Buffer | null): Record<string, unknown> {
+  if (!data) return {};
   try {
-    const raw = await readFile(configPath(pluginId), 'utf-8');
-    return JSON.parse(raw);
+    return JSON.parse(data.toString('utf-8'));
   } catch (error) {
-    if ((error as NodeJS.ErrnoException).code === 'ENOENT') return {};
     logger.warn(`Failed to read plugin config for ${pluginId}`, {
       error: error instanceof Error ? error.message : 'Unknown error',
     });
@@ -34,27 +32,59 @@ export async function getPluginConfig(pluginId: string): Promise<Record<string,
   }
 }
 
+/**
+ * Get all config for a plugin.
+ */
+export async function getPluginConfig(pluginId: string): Promise<Record<string, unknown>> {
+  let data: Buffer | null;
+  try {
+    data = (await pluginConfigDocs().get(fileOf(pluginId)))?.data ?? null;
+  } catch (error) {
+    if (isStoreUnavailable(error)) {
+      const last = lastGood().get(pluginId);
+      if (last !== undefined) return JSON.parse(last);
+      throw error;
+    }
+    logger.warn(`Failed to read plugin config for ${pluginId}`, {
+      error: error instanceof Error ? error.message : 'Unknown error',
+    });
+    return {};
+  }
+  const config = parseConfig(pluginId, data);
+  if (isSharedMode()) lastGood().set(pluginId, JSON.stringify(config));
+  return config;
+}
+
+async function changeConfig(pluginId: string, change: (config: Record<string, unknown>) => void): Promise<void> {
+  const result = await pluginConfigDocs().update(fileOf(pluginId), (current) => {
+    const config = parseConfig(pluginId, current);
+    change(config);
+    // The last key gone: the file goes too.
+    return Object.keys(config).length === 0 ? null : JSON.stringify(config, null, 2);
+  });
+  if (isSharedMode()) lastGood().set(pluginId, result.data ? result.data.toString('utf-8') : '{}');
+}
+
 /**
  * Set a single config key for a plugin.
  */
 export async function setPluginConfig(pluginId: string, key: string, value: unknown): Promise<void> {
   assertWritable('update plugin config');
-  const dir = getPluginConfigDir();
-  await ensureDir(dir);
-
-  const config = await getPluginConfig(pluginId);
-  config[key] = value;
-
-  const filePath = configPath(pluginId);
-  const tmpPath = filePath + '.tmp';
-  await writeFile(tmpPath, JSON.stringify(config, null, 2), 'utf-8');
-  await rename(tmpPath, filePath);
+  await changeConfig(pluginId, (config) => {
+    config[key] = value;
+  });
 }
 
 /** Remove all stored config, secrets included, when a plugin is deleted. */
 export async function deletePluginConfig(pluginId: string): Promise<void> {
   assertWritable('delete plugin config');
-  try { await unlink(configPath(pluginId)); } catch { /* none stored */ }
+  try {
+    await pluginConfigDocs().delete(fileOf(pluginId));
+  } catch (error) {
+    // None stored is fine; a store outage is not.
+    if (isStoreUnavailable(error)) throw error;
+  }
+  lastGood().delete(pluginId);
 }
 
 /**
@@ -62,18 +92,7 @@ export async function deletePluginConfig(pluginId: string): Promise<void> {
  */
 export async function deletePluginConfigKey(pluginId: string, key: string): Promise<void> {
   assertWritable('delete plugin config key');
-  const config = await getPluginConfig(pluginId);
-  delete config[key];
-
-  if (Object.keys(config).length === 0) {
-    try { await unlink(configPath(pluginId)); } catch { /* ok if missing */ }
-    return;
-  }
-
-  const dir = getPluginConfigDir();
-  await ensureDir(dir);
-  const filePath = configPath(pluginId);
-  const tmpPath = filePath + '.tmp';
-  await writeFile(tmpPath, JSON.stringify(config, null, 2), 'utf-8');
-  await rename(tmpPath, filePath);
+  await changeConfig(pluginId, (config) => {
+    delete config[key];
+  });
 }