From 2944d2797f221c3a5dbacfefde14ebb90bfda972 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 1 Nov 2026 00:12:26 +0100 Subject: [PATCH] fix: keep every key when several of a plugin's settings are saved at once --- .../plugin-config-concurrent.test.ts | 44 +++++++ lib/admin/plugin-config.ts | 115 ++++++++++-------- 2 files changed, 111 insertions(+), 48 deletions(-) create mode 100644 lib/admin/__tests__/plugin-config-concurrent.test.ts diff --git a/lib/admin/__tests__/plugin-config-concurrent.test.ts b/lib/admin/__tests__/plugin-config-concurrent.test.ts new file mode 100644 index 000000000..9503e5d38 --- /dev/null +++ b/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 }); + }); +}); diff --git a/lib/admin/plugin-config.ts b/lib/admin/plugin-config.ts index 3af07d254..42cd6e4b9 100644 --- a/lib/admin/plugin-config.ts +++ b/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 { - 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/.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> { + +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 { + const g = globalThis as typeof globalThis & { [LAST_GOOD_KEY]?: Map }; + return (g[LAST_GOOD_KEY] ??= new Map()); +} + +function parseConfig(pluginId: string, data: Buffer | null): Record { + 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> { + 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) => void): Promise { + 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 { 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 { 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 { */ export async function deletePluginConfigKey(pluginId: string, key: string): Promise { 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]; + }); }