From 036f59266bca54831fb565f98024bb7f97889490 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Fri, 2 Oct 2026 17:58:52 +0000 Subject: [PATCH] :bug: Discard cancelled plugin loads after unloading Cause: - Disabling MCP during script loading could leave a late plugin instance attached to a stopped controller, blocking connection after re-enabling. Fix: - Track pending loads and discard instances cancelled by unload. - Prevent duplicate global loads and preserve newer replacement instances. - Clear pending state after failures so a subsequent load can retry. Validation: - Pass 84 runtime tests, the runtime build, lint and formatting checks. - Reproduce the race with a delayed script in the browser, then verify re-enabling restores the session ID, themed indicator and MCP execution. AI-assisted-by: gpt-6 Signed-off-by: Dominik Jain --- plugins/CHANGELOG.md | 2 +- .../src/lib/load-plugin-real-path.spec.ts | 73 ++++++++++++++++++- .../plugins-runtime/src/lib/load-plugin.ts | 16 +++- 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 6dabccbb2f..94cd3f268c 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -2,7 +2,7 @@ ### 🚀 Features -- **plugins-runtime, plugin-types**: Added global plugins (`scope: "global"`) that remain active across dashboard and workspace navigation until logout, with `penpotMgmt` for project and file discovery, file navigation and workspace events. +- **plugins-runtime, plugin-types**: Added global plugins (`scope: "global"`) that remain active across dashboard and workspace navigation until closed or logged out, with `penpotMgmt` for project and file discovery, file navigation and workspace events. - **plugin-types:** Added `paddingType` (`'simple' | 'multiple'`) to flex and grid layouts and `marginType` (`'simple' | 'multiple'`) to layout children, exposing whether the four padding/margin sides are mirrored or honoured independently. - **plugin-types**: Added `waitForLayoutUpdate` to wait until pending layout updates have finished. It rejects when the optional timeout elapses, defaulting to 30 seconds so a wait never hangs. - **plugin-types**: Added `waitForLayoutUpdate` to the `Shape` interface to wait until the pending layout updates of a shape and its children have finished diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts index 4151e09fea..2200f434b1 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts @@ -1,6 +1,11 @@ import { describe, it, vi, expect, beforeAll } from 'vitest'; import 'ses'; -import { loadPlugin, setContextBuilder, getPlugins } from './load-plugin'; +import { + loadPlugin, + setContextBuilder, + getPlugins, + ɵunloadPlugin, +} from './load-plugin'; import type { Context } from '@penpot/plugin-types'; import type { Manifest } from './models/manifest.model.js'; @@ -69,7 +74,7 @@ function makeHostFixture() { // public penpot API. Plugin code must never see it (see B-2 below). __internalSecret: 'host-internal', } as unknown as Context; - return { context, listenerTypes, createRectangle, selection }; + return { context, listeners, listenerTypes, createRectangle, selection }; } function lastCompartmentGlobalThis(): Record { @@ -171,6 +176,70 @@ describe('loadPlugin real initialization path (regression for #11001)', () => { expect(fixture.createRectangle).toHaveBeenCalledTimes(1); expect(lastCompartmentGlobalThis()['__selectionFrozen']).toBe(true); }); + it.each(['before', 'after'])( + 'discards a cancelled global load that completes %s its replacement', + async (order) => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('plugin.js', []), + host: 'https://plugins.test/', + scope: 'global' as const, + }; + ɵunloadPlugin(manifest.pluginId); + let finishFetch!: (response: object) => void; + const response = new Promise((resolve) => { + finishFetch = resolve; + }); + const fetch = vi.fn().mockReturnValue(response); + vi.stubGlobal('fetch', fetch); + try { + const cancelled = loadPlugin(manifest); + await loadPlugin(manifest); + expect(fetch).toHaveBeenCalledOnce(); + ɵunloadPlugin(manifest.pluginId); + const replacement = () => + loadPlugin({ + ...manifest, + host: '', + code: 'globalThis.marker = "replacement";', + }); + if (order === 'after') await replacement(); + finishFetch({ + ok: true, + text: async () => 'globalThis.marker = "cancelled";', + }); + await cancelled; + if (order === 'before') { + expect(getPlugins()).toHaveLength(0); + expect(fixture.listeners.size).toBe(0); + await replacement(); + } + expect(getPlugins()).toHaveLength(1); + expect(lastCompartmentGlobalThis()['marker']).toBe('replacement'); + expect(fixture.listeners.size).toBe(3); + } finally { + ɵunloadPlugin(manifest.pluginId); + vi.unstubAllGlobals(); + } + }, + ); + + it('allows retrying a global plugin after a failed load', async () => { + const fixture = makeHostFixture(); + setContextBuilder(() => fixture.context); + const manifest = { + ...makeManifest('throw new Error("load failed");', []), + scope: 'global' as const, + }; + await expect(loadPlugin(manifest)).rejects.toThrow('load failed'); + expect(getPlugins()).toHaveLength(0); + await loadPlugin({ ...manifest, code: 'globalThis.marker = "retry";' }); + expect(lastCompartmentGlobalThis()['marker']).toBe('retry'); + ɵunloadPlugin(manifest.pluginId); + expect(fixture.listeners.size).toBe(0); + }); + it('keeps a global sandbox through navigation and other plugin loads, then closes it on logout', async () => { const fixture = makeHostFixture(); let workspace = { diff --git a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts index be332d9058..bad0d4fd63 100644 --- a/plugins/libs/plugins-runtime/src/lib/load-plugin.ts +++ b/plugins/libs/plugins-runtime/src/lib/load-plugin.ts @@ -5,6 +5,7 @@ import { Manifest } from './models/manifest.model.js'; import { createPlugin } from './create-plugin.js'; let plugins: Awaited>[] = []; +const pendingPlugins = new Map(); export type ContextBuilder = (id: string) => Context; @@ -49,6 +50,7 @@ export const loadPlugin = async function ( closeCallback?: () => void, apiExtensions?: object, ) { + const loadId = Symbol(); try { const context = contextBuilder && contextBuilder(manifest.pluginId); @@ -58,6 +60,7 @@ export const loadPlugin = async function ( if (manifest.scope === 'global') { if ( + pendingPlugins.has(manifest.pluginId) || plugins.some( (plugin) => plugin.manifest?.pluginId === manifest.pluginId, ) @@ -83,7 +86,9 @@ export const loadPlugin = async function ( // `createSandbox`'s proxy handler applies `ses.safeReturn` to values // crossing into the sandbox. Compartment isolation and intrinsics // hardening are performed by createSandbox, not here. - let plugin: Awaited> | undefined = undefined; + pendingPlugins.set(manifest.pluginId, loadId); + let plugin: Awaited> | undefined = + undefined; plugin = await createPlugin( context, manifest, @@ -96,10 +101,18 @@ export const loadPlugin = async function ( }, apiExtensions, ); + if (pendingPlugins.get(manifest.pluginId) !== loadId) { + plugin.plugin.close(); + return; + } plugins.push(plugin); } catch (error) { if (manifest.scope !== 'global') closeAllPlugins(); throw error; + } finally { + if (pendingPlugins.get(manifest.pluginId) === loadId) { + pendingPlugins.delete(manifest.pluginId); + } } }; @@ -117,6 +130,7 @@ export const ɵloadPluginByUrl = async function (manifestUrl: string) { }; export const ɵunloadPlugin = function (id: Manifest['pluginId']) { + pendingPlugins.delete(id); const plugin = plugins.find((plugin) => plugin.manifest.pluginId === id); if (plugin) {