From f76538b2b2d0f155964aca82478018a58c5c89a7 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 15:31:50 +0200 Subject: [PATCH] fix(plugins): uninstall a plugin and record its install in one transaction Uninstall deleted the plugin row and then up to fourteen tables of rows about it one by one. A failure halfway left grants, scheduled tasks or per-user settings behind for a later plugin that reuses the id to inherit. The rows now go in one transaction, and the data directory is removed after the commit instead of before the rows that describe it. A registry install records the discovered row, its provenance, the pinned author key and the cleared update block together, so a failure never leaves a registered plugin without its pinned key. Starting an OAuth connect drops the stale state and stores the fresh one together, and a disconnect drops tokens and state together; PluginOAuthService takes the UnitOfWork for that. The scheduler's per-task claims stay separate on purpose and say so for the transaction gate. --- server/scripts/size-baseline.json | 2 +- .../plugins/oauth/plugin-oauth.service.ts | 22 ++++++--- .../nest/plugins/plugin-runtime.service.ts | 45 ++++++++----------- .../nest/plugins/registry/registry.service.ts | 30 +++++++------ server/tests/helpers/plugin-host.ts | 2 +- .../plugin-runtime.task2-conversion.test.ts | 29 ++++++------ .../unit/plugins/plugin-oauth.service.test.ts | 12 ++++- 7 files changed, 80 insertions(+), 62 deletions(-) diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index f84666d63..1d8837261 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -15,7 +15,7 @@ "src/nest/journey/journey-domain.service.ts": 1804, "src/nest/maps/maps.service.ts": 3252, "src/nest/places/places.service.ts": 2022, - "src/nest/plugins/plugin-runtime.service.ts": 1472, + "src/nest/plugins/plugin-runtime.service.ts": 1463, "src/nest/plugins/runtime/plugin-sdk.ts": 1094, "src/nest/reservations/reservations.service.ts": 1248, "src/nest/trips/trips.service.ts": 1014, diff --git a/server/src/nest/plugins/oauth/plugin-oauth.service.ts b/server/src/nest/plugins/oauth/plugin-oauth.service.ts index b6fc7a0c3..ab1fa0465 100644 --- a/server/src/nest/plugins/oauth/plugin-oauth.service.ts +++ b/server/src/nest/plugins/oauth/plugin-oauth.service.ts @@ -14,6 +14,7 @@ import { PluginOauthState } from '../../../db/entities/PluginOauthState.entity'; import type { PluginOauthStateRepository } from '../../../db/repositories/PluginOauthState.repository'; import { PluginSettingsFields } from '../../../db/entities/PluginSettingsFields.entity'; import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository'; +import { UnitOfWork } from '../../database/unit-of-work'; /** * Host-brokered outbound OAuth (#plugins). A plugin becomes an OAuth *client* of a @@ -79,6 +80,7 @@ export class PluginOAuthService { @InjectRepository(PluginOauthTokens) private readonly tokensRepo: PluginOauthTokensRepository, @InjectRepository(PluginOauthState) private readonly stateRepo: PluginOauthStateRepository, @InjectRepository(PluginSettingsFields) private readonly settingsFieldsRepo: PluginSettingsFieldsRepository, + private readonly uow: UnitOfWork, ) {} /** The plugin's decrypted OAuth provider config from its INSTANCE settings, or null @@ -127,9 +129,11 @@ export class PluginOAuthService { const challenge = b64url(crypto.createHash('sha256').update(verifier).digest()); const state = b64url(crypto.randomBytes(24)); - // Drop this user's stale states for the plugin, then store the fresh one. - await this.stateRepo.deleteForUser(pluginId, userId); // PO3 - await this.stateRepo.insertState(state, pluginId, userId, verifier, nowMs); // PO4 + // Drop this user's stale states for the plugin and store the fresh one, together. + await this.uow.transactional(async () => { + await this.stateRepo.deleteForUser(pluginId, userId); // PO3 + await this.stateRepo.insertState(state, pluginId, userId, verifier, nowMs); // PO4 + }); authorize.searchParams.set('response_type', 'code'); authorize.searchParams.set('client_id', cfg.clientId); @@ -141,7 +145,11 @@ export class PluginOAuthService { return authorize.toString(); } - /** Complete the callback: verify state, exchange the code, store the tokens. */ + /** + * Complete the callback: verify state, exchange the code, store the tokens. + * @txIndependent the state is consumed before the token exchange (network I/O), + * and the tokens are stored after it. + */ async completeCallback(pluginId: string, userId: number, code: string, state: string, nowMs: number): Promise { // PO5+PO6+PO7, atomically — the state is consumed (deleted) by this ONE call // regardless of outcome, same as the legacy SELECT-then-unconditional-DELETE @@ -191,8 +199,10 @@ export class PluginOAuthService { } async disconnect(pluginId: string, userId: number): Promise { - await this.tokensRepo.deleteForUser(pluginId, userId); // PO9 - await this.stateRepo.deleteForUser(pluginId, userId); // PO10 + await this.uow.transactional(async () => { + await this.tokensRepo.deleteForUser(pluginId, userId); // PO9 + await this.stateRepo.deleteForUser(pluginId, userId); // PO10 + }); } // --- internals --- diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index a8b4fc696..8e5567fdf 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -243,18 +243,9 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // registry reads a recipient's own settings on every dispatch, so an absent one // would be a TypeError at send time rather than a missing-provider error at boot. private readonly userSettings: PluginUserSettingsService, - // Plan 3j Task 2 — every table this file's own statements (PR1-PR53) touch, now - // injected as repositories instead of raw prepared-statement calls on `this.db`. All 14 are - // REQUIRED (not `@Optional()`): virtually every method on this class reaches - // `plugins` or one of its satellite tables, so a hand-built test instance needs - // real ones regardless (`sharedTestOrm(...).repo(Entity)`, the same shape - // `PluginGuards`' `UsersRepository` injection (Task 1) already established for - // this plan). `PluginOauthTokens`/`PluginOauthState`/`PluginMetaMigrations`/ - // `PluginCapabilityAudit` are used ONLY by this file's own uninstall cascade and - // GDPR export — Task 3/Task 4 add their own, disjoint methods to the same four - // repositories for their own files. `Settings`/`NotificationChannelPreferences` - // are cross-domain (3f) — additive methods only (`deleteByKeyPrefix`/ - // `deleteAllForChannel`), never their own per-user CRUD surface. + // Every table this class touches, all required: a hand-built test instance needs + // real ones (`sharedTestOrm(...).repo(Entity)`). `Settings` and + // `NotificationChannelPreferences` are cross-domain and used for deletes only. @InjectRepository(Plugins) private readonly plugins: PluginsRepository, @InjectRepository(PluginErrorLog) private readonly pluginErrorLog: PluginErrorLogRepository, @InjectRepository(PluginScheduledTasks) private readonly pluginScheduledTasks: PluginScheduledTasksRepository, @@ -453,7 +444,8 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes /** Fire every scheduled task that is due on an ACTIVE plugin; re-arm recurring * ones, delete one-shots. The row is re-armed/deleted BEFORE the fire so a crash * mid-callback can't double-fire; an inactive plugin's tasks are left untouched so - * they run on the next sweep after it reactivates. Never throws. */ + * they run on the next sweep after it reactivates. Never throws. + * @txIndependent each task's claim is its own atomic statement, followed by its delivery. */ private async fireDueScheduled(): Promise { if (!pluginsEnabled()) return; try { @@ -465,10 +457,8 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes const due = await this.pluginScheduledTasks.findDueForPlugins(now, active); for (const t of due) { if (!this.supervisor.isActive(t.plugin_id)) continue; // leave for a later sweep - // Plan 3j Task 2 deviation (named, task-2-report.md — same spirit as R-uninstall's - // egress-host transaction): the claim is now atomic (`due_at <= now` guarded in - // the repository) — a second, overlapping tick's own claim on the SAME row loses - // and skips delivery entirely, rather than double-firing (RACE-SCHED-001). + // The claim is atomic (`due_at <= now` guarded in the repository): an overlapping + // tick's claim on the same row loses and skips delivery (RACE-SCHED-001). const claimed = t.every_ms ? await this.pluginScheduledTasks.rearm(t.id, now + t.every_ms, now) : await this.pluginScheduledTasks.deleteById(t.id, now); @@ -1088,16 +1078,10 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes } } - /** Stop the plugin, remove its code, and optionally delete all its data. */ /** - * R-uninstall: the 9-table cascade converts with the SAME statement order and the SAME - * (non-)transactional scope the legacy code had — NOT wrapped in `uow.transactional`. - * Nine independent best-effort deletes, several inside their own try/catch tolerating an - * absent table on a slimmed test schema (`plugin_actions`, `plugin_egress_hosts`) — - * `plugins`/`plugin_settings_fields`/`plugin_scheduled_tasks`/`plugin_error_log`/ - * `plugin_entity_metadata`/`plugin_user_config`/`plugin_oauth_tokens`/`plugin_oauth_state`/ - * `plugin_meta_migrations`/`plugin_capability_audit`/`plugin_user_erasure_queue` do NOT - * tolerate an absent table, unchanged from the legacy statement order and shape. + * Stop the plugin, remove its code, and optionally delete all its data. The row + * deletes keep the legacy order; `plugin_actions` and `plugin_egress_hosts` still + * tolerate an absent table (a slimmed test schema), the others do not. */ async uninstall(id: string, deleteData: boolean): Promise { await this.supervisor.disable(id); @@ -1106,6 +1090,14 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // Code always goes; the DB metadata + fields go so it disappears from the UI. // Link-safe: a dev-linked plugin only drops the symlink, never the author's source. removePluginCodeEntry(pluginCodeDir(id)); + // Every row about the plugin goes in one transaction: a failure halfway used to + // leave a plugin without its row but with its settings, grants or tasks, which a + // later plugin reusing the id would inherit. The data dir goes after the commit. + await this.uow.transactional(() => this.dropPluginRows(id, deleteData)); + if (deleteData) removePluginData(id); + } + + private async dropPluginRows(id: string, deleteData: boolean): Promise { await this.plugins.deleteById(id); await this.pluginSettingsFields.deleteAllForPlugin(id); try { await this.pluginActions.deleteAllForPlugin(id); } catch { /* table absent */ } @@ -1122,7 +1114,6 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // user's opt-outs and the admin's enablement. await this.retireNotificationChannel(id); if (deleteData) { - removePluginData(id); await this.pluginErrorLog.deleteAllForPlugin(id); await this.settingsRepo.deleteByKeyPrefix(`plugin:${id}:`); await this.pluginEntityMetadata.deleteAllForPlugin(id); diff --git a/server/src/nest/plugins/registry/registry.service.ts b/server/src/nest/plugins/registry/registry.service.ts index 26f6fa3c5..dce7e292f 100644 --- a/server/src/nest/plugins/registry/registry.service.ts +++ b/server/src/nest/plugins/registry/registry.service.ts @@ -512,19 +512,23 @@ export class PluginRegistryService { fs.rmSync(dest, { recursive: true, force: true }); fs.renameSync(pluginRoot, dest); - // 7. register INACTIVE (record provenance) - await discoverPlugins(this.discoveryRepos); - await this.plugins.setInstallProvenance(id, entry.repo, ver.commitSha, ver.sha256, entry.reviewedAt ?? null); - // Pin the author key on first successful install of a signed plugin (TOFU) — - // and, after a re-trust, re-pin to the new key the admin blessed. Only ever set - // to a key the artifact just verified under; NEVER cleared to NULL, because a - // NULL pin re-opens the "was never signed" path that accepts an unsigned update. - if (entry.authorPublicKey) { - await this.plugins.setAuthorPubkey(id, entry.authorPublicKey); - } - // The plugin is now on new code that passed every check — whatever refusal was - // recorded before no longer describes reality. - await clearUpdateBlock(this.plugins, id); + // 7. register INACTIVE (record provenance), the row and its provenance in one + // write, so a failure never leaves a registered plugin without its pinned key. + const repos = this.discoveryRepos; + await repos.uow.transactional(async () => { + await discoverPlugins(repos); + await this.plugins.setInstallProvenance(id, entry.repo, ver.commitSha, ver.sha256, entry.reviewedAt ?? null); + // Pin the author key on first successful install of a signed plugin (TOFU) — + // and, after a re-trust, re-pin to the new key the admin blessed. Only ever set + // to a key the artifact just verified under; NEVER cleared to NULL, because a + // NULL pin re-opens the "was never signed" path that accepts an unsigned update. + if (entry.authorPublicKey) { + await this.plugins.setAuthorPubkey(id, entry.authorPublicKey); + } + // The plugin is now on new code that passed every check — whatever refusal was + // recorded before no longer describes reality. + await clearUpdateBlock(this.plugins, id); + }); return { id, version: ver.version, trekRangeBypassed }; } finally { fs.rmSync(staging, { recursive: true, force: true }); diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index a95bf2c8b..b4e1573e0 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -284,7 +284,7 @@ export async function createPluginRpcHostParts( // moved off DatabaseService onto AddonsRepository. const llmConfig = new LlmConfigResolver(new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db)), (await sharedTestOrm(db)).repo(Addons), addons); const pluginOrm = await sharedTestOrm(db); - const oauth = new PluginOAuthService(pluginOrm.repo(Plugins), pluginOrm.repo(PluginOauthTokens), pluginOrm.repo(PluginOauthState), pluginOrm.repo(PluginSettingsFields)); + const oauth = new PluginOAuthService(pluginOrm.repo(Plugins), pluginOrm.repo(PluginOauthTokens), pluginOrm.repo(PluginOauthState), pluginOrm.repo(PluginSettingsFields), await createTestUnitOfWork(db)); const accommodations = new AccommodationsService( permissions, realtime, assignments, await createTestUnitOfWork(db), await createTestTripsRepo(db), diff --git a/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts b/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts index d86487d84..74fea082a 100644 --- a/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts +++ b/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts @@ -2,8 +2,8 @@ * Plan 3j Task 2 — mutation/parity proofs the brief names explicitly, on top of the * pre-existing (and still-green) `plugin-runtime.test.ts`/`plugins-service.test.ts` * suites: R-install-gates' three named accept+refuse pairs (PR17/PR26/PR28), the - * 9-table uninstall cascade's preserved NON-transactional partial-failure shape - * (R-uninstall), the egress-host DELETE+loop-INSERT's new transaction rollback proof + * uninstall cascade's all-or-nothing rollback (R-uninstall, one transaction since + * the transaction sweep), the egress-host DELETE+loop-INSERT's transaction rollback proof * (R-uninstall's ONE named exception), PS7/PS9/PS11's triple-duplicate SELECT * collapsing onto one `PluginUserConfigRepository.findConfig` method, and a two-tick * race for the scheduler sweep + the GDPR erasure drain. @@ -255,8 +255,8 @@ describe('Plan 3j Task 2 — R-install-gates named accept+refuse pairs (PR17/PR2 }); }); -describe('Plan 3j Task 2 — R-uninstall: the 9-table cascade stays non-transactional (concurrency-pin)', () => { - it('CASCADE-PARTIAL-001: a failure partway through the cascade leaves EARLIER deletes committed and LATER ones never attempted — the CURRENT legacy shape, not hardened to all-or-nothing', async () => { +describe('R-uninstall: the cascade is one transaction', () => { + it('CASCADE-PARTIAL-001: a failure partway through the cascade rolls every earlier delete back, and the data directory stays', async () => { const rt = await buildRuntime(); const id = 'cascade-partial'; seedPlugin(id); @@ -272,18 +272,21 @@ describe('Plan 3j Task 2 — R-uninstall: the 9-table cascade stays non-transact const errorLogRepo = (rt as any).pluginErrorLog; const spy = vi.spyOn(errorLogRepo, 'deleteAllForPlugin').mockRejectedValueOnce(new Error('simulated mid-cascade crash')); + const dataDir = path.join(dataRoot, id); + fs.mkdirSync(dataDir, { recursive: true }); + await expect(rt.uninstall(id, true)).rejects.toThrow(/simulated mid-cascade crash/); spy.mockRestore(); - // EVERYTHING before the simulated crash point is already gone — no transaction - // rolled it back, exactly matching the legacy sequential-statement shape. - expect(testDb.prepare("SELECT COUNT(*) c FROM plugins WHERE id=?").get(id)).toMatchObject({ c: 0 }); - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_settings_fields WHERE plugin_id=?").get(id)).toMatchObject({ c: 0 }); - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_scheduled_tasks WHERE plugin_id=?").get(id)).toMatchObject({ c: 0 }); - // The row the mock made deleteAllForPlugin THROW for is untouched (never committed)... - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_error_log WHERE plugin_id=?").get(id)).toMatchObject({ c: 1 }); - // ...and everything AFTER it in cascade order never even ran. - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_entity_metadata WHERE plugin_id=?").get(id)).toMatchObject({ c: 1 }); + // Everything before the simulated crash point was rolled back with it: a half + // uninstall used to leave settings, tasks and metadata for a later plugin that + // reuses the id to inherit. + for (const table of ['plugin_settings_fields', 'plugin_scheduled_tasks', 'plugin_error_log', 'plugin_entity_metadata']) { + expect(testDb.prepare(`SELECT COUNT(*) c FROM ${table} WHERE plugin_id=?`).get(id)).toMatchObject({ c: 1 }); + } + expect(testDb.prepare("SELECT COUNT(*) c FROM plugins WHERE id=?").get(id)).toMatchObject({ c: 1 }); + // The data directory goes only after the rows commit. + expect(fs.existsSync(dataDir)).toBe(true); }); }); diff --git a/server/tests/unit/plugins/plugin-oauth.service.test.ts b/server/tests/unit/plugins/plugin-oauth.service.test.ts index 832fce8b0..db807e306 100644 --- a/server/tests/unit/plugins/plugin-oauth.service.test.ts +++ b/server/tests/unit/plugins/plugin-oauth.service.test.ts @@ -35,6 +35,7 @@ import Database from 'better-sqlite3'; import { createSnapshotTestDb } from '../../helpers/db-mock'; import { PluginOAuthService } from '../../../src/nest/plugins/oauth/plugin-oauth.service'; import { sharedTestOrm } from '../../helpers/test-uow'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import { Plugins } from '../../../src/db/entities/Plugins.entity'; import { PluginOauthTokens } from '../../../src/db/entities/PluginOauthTokens.entity'; import { PluginOauthState } from '../../../src/db/entities/PluginOauthState.entity'; @@ -64,7 +65,7 @@ const NOW = 1_700_000_000_000; */ async function makeOauthService(): Promise { const orm = await sharedTestOrm(getDb.current as Database.Database); - return new PluginOAuthService(orm.repo(Plugins), orm.repo(PluginOauthTokens), orm.repo(PluginOauthState), orm.repo(PluginSettingsFields)); + return new PluginOAuthService(orm.repo(Plugins), orm.repo(PluginOauthTokens), orm.repo(PluginOauthState), orm.repo(PluginSettingsFields), new UnitOfWork(orm.em)); } describe('PluginOAuthService', () => { @@ -113,6 +114,15 @@ describe('PluginOAuthService', () => { expect((rows.prepare('SELECT COUNT(*) c FROM plugin_oauth_state WHERE user_id = 42').get() as { c: number }).c).toBe(1); }); + it('startConnect drops the old state and stores the new one together: a failed store keeps the old one', async () => { + const first = new URL(await svc.startConnect('p', 42, NOW)).searchParams.get('state')!; + const orm = await sharedTestOrm(getDb.current as Database.Database); + vi.spyOn(orm.repo(PluginOauthState), 'insertState').mockRejectedValueOnce(new Error('disk full')); + await expect(svc.startConnect('p', 42, NOW)).rejects.toThrow('disk full'); + const rows = getDb.current as unknown as InstanceType; + expect(rows.prepare('SELECT state FROM plugin_oauth_state WHERE user_id = 42').all()).toEqual([{ state: first }]); + }); + it('rejects a non-https / loopback / metadata / internal authorize endpoint', async () => { getDb.current = freshDb({ ...CFG, oauth_authorize_url: 'http://provider.example/authorize' }); await expect((await makeOauthService()).startConnect('p', 42, NOW)).rejects.toThrow(/https/);