From 1853156da3639a3f765da2a17942ffc39cca3cfb Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 06:17:23 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=204=20=E2=80=94=20plugin=20?= =?UTF-8?q?discovery's=20delete+reinsert=20pairs=20are=20atomic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install/discovery.ts#upsert refreshed a plugin's actions and settings-field descriptors as two independent delete-then-reinsert pairs (DI5-DI8) with no transaction spanning either pair, let alone both: a crash between a delete and its own reinsert left that plugin with an EMPTY actions or settings-field set until the next discovery run, instead of its previous still-valid rows. Wraps both pairs in one uow.transactional, so a discovery refresh for a plugin's descriptors is now all-or-nothing. Deliberate behaviour change, named for the user per the task brief. DiscoveryRepos grows a UnitOfWork; PluginRegistryService (the other production caller, registry.service.ts) picks it up the same @Optional() way PluginRuntimeService already does, with the same guard-throw. Updates the two hand-constructed test call sites that actually exercise discovery (discovery.test.ts, registry.test.ts) to pass a real UnitOfWork. --- server/src/nest/plugins/install/discovery.ts | 76 ++++++++++++------- .../nest/plugins/registry/registry.service.ts | 12 ++- .../integration/plugins/registry.test.ts | 3 +- server/tests/unit/plugins/discovery.test.ts | 3 +- 4 files changed, 64 insertions(+), 30 deletions(-) diff --git a/server/src/nest/plugins/install/discovery.ts b/server/src/nest/plugins/install/discovery.ts index babefcc45..7caee966b 100644 --- a/server/src/nest/plugins/install/discovery.ts +++ b/server/src/nest/plugins/install/discovery.ts @@ -8,15 +8,24 @@ import type { PluginsRepository } from '../../../db/repositories/Plugins.reposit import type { PluginActionsRepository } from '../../../db/repositories/PluginActions.repository'; import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository'; import type { PluginErrorLogRepository } from '../../../db/repositories/PluginErrorLog.repository'; +import type { UnitOfWork } from '../../database/unit-of-work'; /** `discoverPlugins`/`upsert`'s repository set — "swap the parameter type" (plan3j-inputs.md * §0): the raw `BetterSqlite3.Database` parameter becomes this bundle of the four - * repositories the scan actually writes through. */ + * repositories the scan actually writes through, plus the `UnitOfWork` `upsert` + * uses to make its own delete-then-reinsert pairs (DI5–DI8, Plan 4 Task 8a) + * atomic. Optional, not required: both call sites' own `uow` is itself + * `@Optional()` (Nest always injects it in the real app; only a hand-built + * partial-DI-graph test instance omits it — the same "boot must never block + * app init" defensiveness this file's own callers already apply elsewhere) — + * `upsert` runs the pairs sequentially, un-transacted, exactly as before, + * when it is absent, rather than refusing to discover at all. */ export interface DiscoveryRepos { plugins: PluginsRepository; actions: PluginActionsRepository; settingsFields: PluginSettingsFieldsRepository; errorLog: PluginErrorLogRepository; + uow?: UnitOfWork; } /** @@ -97,30 +106,45 @@ async function upsert(repos: DiscoveryRepos, m: PluginManifest): Promise { await repos.plugins.insertManifest({ id: m.id, ...manifestRow }); } - // Refresh the settings-page action descriptors from the manifest. - await repos.actions.deleteAllForPlugin(m.id); - await repos.actions.insertActions( - m.id, - m.actions.map((a, i) => ({ action_key: a.key, label: a.label, hint: a.hint ?? null, danger: a.danger ? 1 : 0, scope: a.scope, sort_order: i })), - ); + // Refresh the settings-page action and settings-field descriptors from the + // manifest. DI5–DI8 (Plan 4 Task 8a — a deliberate behaviour change, named + // for the user): each pair used to run its own delete then its own + // re-insert with no transaction spanning them, so a crash between the two + // (of either pair) left that plugin with an EMPTY actions or settings-field + // set until the next discovery run, rather than its previous (still valid) + // rows. Wrapped in one `uow.transactional` (when a `uow` is available — + // see `DiscoveryRepos.uow`'s own docstring) so a discovery refresh for a + // single plugin's descriptors is now all-or-nothing: either both pairs land + // or neither does, and the plugin keeps its prior rows on any failure. + const refreshDescriptors = async (): Promise => { + await repos.actions.deleteAllForPlugin(m.id); + await repos.actions.insertActions( + m.id, + m.actions.map((a, i) => ({ action_key: a.key, label: a.label, hint: a.hint ?? null, danger: a.danger ? 1 : 0, scope: a.scope, sort_order: i })), + ); - // Refresh the settings-field descriptors from the manifest. - await repos.settingsFields.deleteAllForPlugin(m.id); - await repos.settingsFields.insertFields( - m.id, - m.settings.map((f, i) => ({ - field_key: f.key, - label: f.label ?? f.key, - input_type: f.input_type ?? 'text', - placeholder: f.placeholder ?? null, - hint: f.hint ?? null, - required: f.required ? 1 : 0, - secret: f.secret ? 1 : 0, - scope: f.scope ?? 'instance', - options: f.options ? JSON.stringify(f.options) : null, - oauth_config: f.oauth ? JSON.stringify(f.oauth) : null, - default_value: f.default === undefined ? null : JSON.stringify(f.default), - sort_order: i, - })), - ); + await repos.settingsFields.deleteAllForPlugin(m.id); + await repos.settingsFields.insertFields( + m.id, + m.settings.map((f, i) => ({ + field_key: f.key, + label: f.label ?? f.key, + input_type: f.input_type ?? 'text', + placeholder: f.placeholder ?? null, + hint: f.hint ?? null, + required: f.required ? 1 : 0, + secret: f.secret ? 1 : 0, + scope: f.scope ?? 'instance', + options: f.options ? JSON.stringify(f.options) : null, + oauth_config: f.oauth ? JSON.stringify(f.oauth) : null, + default_value: f.default === undefined ? null : JSON.stringify(f.default), + sort_order: i, + })), + ); + }; + if (repos.uow) { + await repos.uow.transactional(refreshDescriptors); + } else { + await refreshDescriptors(); + } } diff --git a/server/src/nest/plugins/registry/registry.service.ts b/server/src/nest/plugins/registry/registry.service.ts index 060cc1f38..6f941e102 100644 --- a/server/src/nest/plugins/registry/registry.service.ts +++ b/server/src/nest/plugins/registry/registry.service.ts @@ -10,7 +10,7 @@ import { safeDownload, sha256Matches } from '../install/safe-fetch'; import { verifyAuthorSignature, SignatureError } from '../install/verify-signature'; import { pluginCodeDir, pluginsCodeRoot, pluginsDataRoot } from '../paths'; import { clearUpdateBlock, isSignatureCode, setUpdateBlock, RETRUSTABLE_CODE } from '../signature-status'; -import { Injectable } from '@nestjs/common'; +import { Injectable, Optional } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import { Plugins } from '../../../db/entities/Plugins.entity'; import type { PluginsRepository } from '../../../db/repositories/Plugins.repository'; @@ -20,6 +20,7 @@ import { PluginSettingsFields } from '../../../db/entities/PluginSettingsFields. import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository'; import { PluginErrorLog } from '../../../db/entities/PluginErrorLog.entity'; import type { PluginErrorLogRepository } from '../../../db/repositories/PluginErrorLog.repository'; +import { UnitOfWork } from '../../database/unit-of-work'; import fs from 'node:fs'; import path from 'node:path'; @@ -182,10 +183,17 @@ export class PluginRegistryService { @InjectRepository(PluginActions) private readonly pluginActions: PluginActionsRepository, @InjectRepository(PluginSettingsFields) private readonly pluginSettingsFields: PluginSettingsFieldsRepository, @InjectRepository(PluginErrorLog) private readonly pluginErrorLog: PluginErrorLogRepository, + // Plan 4 Task 8a: DiscoveryRepos now needs a `UnitOfWork` so `discoverPlugins`'s + // own upsert can wrap its delete-then-reinsert pairs atomically (DI5-DI8). + // `@Optional()` matches `PluginRuntimeService`'s own precedent for the same + // reason: a hand-built test instance that never exercises discovery need not + // construct one — `discoveryRepos` below throws if one is actually needed. + @Optional() private readonly uow?: UnitOfWork, ) {} private get discoveryRepos(): DiscoveryRepos { - return { plugins: this.plugins, actions: this.pluginActions, settingsFields: this.pluginSettingsFields, errorLog: this.pluginErrorLog }; + if (!this.uow) throw new Error('UnitOfWork not provided — tests that exercise plugin discovery must pass one'); + return { plugins: this.plugins, actions: this.pluginActions, settingsFields: this.pluginSettingsFields, errorLog: this.pluginErrorLog, uow: this.uow }; } /** diff --git a/server/tests/integration/plugins/registry.test.ts b/server/tests/integration/plugins/registry.test.ts index 01801d423..e3e69a6b6 100644 --- a/server/tests/integration/plugins/registry.test.ts +++ b/server/tests/integration/plugins/registry.test.ts @@ -30,6 +30,7 @@ import { Plugins } from '../../../src/db/entities/Plugins.entity'; import { PluginActions } from '../../../src/db/entities/PluginActions.entity'; import { PluginSettingsFields } from '../../../src/db/entities/PluginSettingsFields.entity'; import { PluginErrorLog } from '../../../src/db/entities/PluginErrorLog.entity'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import { PluginRegistryService, RegistryError, __clearRegistryCacheForTests } from '../../../src/nest/plugins/registry/registry.service'; import type { ManifestPreview } from '../../../src/nest/plugins/registry/registry.service'; @@ -100,7 +101,7 @@ beforeEach(() => { t.clear(); __clearRegistryCacheForTests(); vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => REGISTRY }) as unknown as Response)); - svc = new PluginRegistryService(t.repo(Plugins), t.repo(PluginActions), t.repo(PluginSettingsFields), t.repo(PluginErrorLog)); + svc = new PluginRegistryService(t.repo(Plugins), t.repo(PluginActions), t.repo(PluginSettingsFields), t.repo(PluginErrorLog), new UnitOfWork(t.em)); }); afterEach(() => { vi.unstubAllGlobals(); diff --git a/server/tests/unit/plugins/discovery.test.ts b/server/tests/unit/plugins/discovery.test.ts index 9a9660dc1..ba5ea585b 100644 --- a/server/tests/unit/plugins/discovery.test.ts +++ b/server/tests/unit/plugins/discovery.test.ts @@ -20,6 +20,7 @@ import { PluginActions } from '../../../src/db/entities/PluginActions.entity'; import { PluginSettingsFields } from '../../../src/db/entities/PluginSettingsFields.entity'; import { PluginErrorLog } from '../../../src/db/entities/PluginErrorLog.entity'; import { discoverPlugins, type DiscoveryRepos } from '../../../src/nest/plugins/install/discovery'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -36,7 +37,7 @@ function writePlugin(id: string, manifest: Record, extra?: () = beforeAll(async () => { t = await createTestOrm(testDb); - repos = { plugins: t.repo(Plugins), actions: t.repo(PluginActions), settingsFields: t.repo(PluginSettingsFields), errorLog: t.repo(PluginErrorLog) }; + repos = { plugins: t.repo(Plugins), actions: t.repo(PluginActions), settingsFields: t.repo(PluginSettingsFields), errorLog: t.repo(PluginErrorLog), uow: new UnitOfWork(t.em) }; }); beforeEach(() => { resetTestDb(testDb);