🐛 Close plugins whose load is cancelled before they run

Unloading or replacing a plugin while its code was still being
fetched could leave a late instance running, or block reopening a
plugin that closed during startup.

Each load now carries an AbortController. Unloading, a newer load
of the same plugin or logout aborts it, and the plugin manager
closes itself on abort, also while it fetches its code.
createPlugin returns nothing for a plugin closed before its
sandbox exists, and failed loads clear their pending state so the
plugin can be loaded again.

plugins/CHANGELOG.md

@@ -9,6 +9,7 @@
 
 ### 🩹 Fixes
 
+- **plugin-runtime:** Cancel plugin loads before they execute after unloading or logout, and allow reopening plugins that close during startup.
 - **plugins-runtime**: An interaction obtained from `Shape.interactions` now keeps addressing that interaction instead of the position it held when the array was read. Removing every interaction of a shape from a single read removes all of them rather than leaving some behind, and writing through a held interaction after an earlier one is removed no longer lands on a different interaction.
 - **plugins-runtime**: `Shape.removeInteraction()` now rejects an interaction belonging to a different shape with a validation error, instead of removing whichever interaction sat at the same position on the target shape.
 - **plugins-runtime**: Writing `trigger`, `delay` or `action` on an interaction the shape no longer has now raises a validation error. The write used to be sent to the workspace with no position to apply it at, where it failed out of the plugin's reach: nothing was written and nothing was reported.

plugins/libs/plugins-runtime/src/lib/create-plugin.spec.ts

@@ -83,6 +83,7 @@ describe('createPlugin', () => {
       manifest,
       expect.any(Function),
       expect.any(Function),
+      undefined,
     );
     expect(createSandbox).toHaveBeenCalledWith(mockPluginManager, undefined);
     expect(mockSandbox.evaluate).toHaveBeenCalled();

plugins/libs/plugins-runtime/src/lib/create-plugin.ts

@@ -8,8 +8,13 @@ export async function createPlugin(
   manifest: Manifest,
   onCloseCallback: () => void,
   apiExtensions?: object,
+  signal?: AbortSignal,
 ) {
+  // Closing can happen while the manager is still fetching code.
+  let sandbox: ReturnType<typeof createSandbox> | undefined = undefined;
+
   const evaluateSandbox = async () => {
+    if (plugin.destroyed || !sandbox) return;
     try {
       sandbox.evaluate();
     } catch (error) {
@@ -23,15 +28,18 @@ export async function createPlugin(
     context,
     manifest,
     function onClose() {
-      sandbox.cleanGlobalThis();
+      sandbox?.cleanGlobalThis();
       onCloseCallback();
     },
     function onReloadModal() {
       evaluateSandbox();
     },
+    signal,
   );
 
-  const sandbox = createSandbox(plugin, apiExtensions);
+  if (plugin.destroyed) return;
+
+  sandbox = createSandbox(plugin, apiExtensions);
 
   await evaluateSandbox();
 

plugins/libs/plugins-runtime/src/lib/load-plugin-context.spec.ts

@@ -30,7 +30,7 @@ describe('loadPlugin host context boundary (regression for #11001)', () => {
         close: vi.fn(),
         sendMessage: vi.fn(),
       },
-    } as unknown as Awaited<ReturnType<typeof createPlugin>>);
+    } as unknown as NonNullable<Awaited<ReturnType<typeof createPlugin>>>);
   });
 
   it('does not freeze host-owned functions reachable through the context', async () => {
@@ -60,6 +60,7 @@ describe('loadPlugin host context boundary (regression for #11001)', () => {
       manifest,
       expect.any(Function),
       undefined,
+      expect.any(AbortSignal),
     );
 
     // Host-owned functions must remain extensible: page navigation and

plugins/libs/plugins-runtime/src/lib/load-plugin-real-path.spec.ts

@@ -52,15 +52,23 @@ function makeManifest(
 
 function makeHostFixture() {
   const listenerTypes: string[] = [];
-  const listeners = new Map<symbol, string>();
+  const listeners = new Map<
+    symbol,
+    { type: string; callback: (...args: unknown[]) => unknown }
+  >();
+  const shapes: object[] = [];
   // Inline code (empty host + non-URL code) resolves without network, so no
   // fetch mock is needed. UI/modal APIs are never touched by the probe code.
-  const createRectangle = vi.fn(() => ({ type: 'rectangle-marker' }));
+  const createRectangle = vi.fn(() => {
+    const shape = { type: 'rectangle-marker' };
+    shapes.push(shape);
+    return shape;
+  });
   const selection: object[] = [{ id: 'shape-1' }];
   const context = {
-    addListener: (type: string, _callback: (...args: unknown[]) => unknown) => {
+    addListener: (type: string, callback: (...args: unknown[]) => unknown) => {
       const id = Symbol(type);
-      listeners.set(id, type);
+      listeners.set(id, { type, callback });
       listenerTypes.push(type);
       return id;
     },
@@ -68,13 +76,21 @@ function makeHostFixture() {
       listeners.delete(id);
     },
     theme: 'dark',
+    management: { workspace: { status: 'ready' } },
     createRectangle,
     selection,
     // Host-only member: present on the raw context but NOT part of the
     // public penpot API. Plugin code must never see it (see B-2 below).
     __internalSecret: 'host-internal',
   } as unknown as Context;
-  return { context, listeners, listenerTypes, createRectangle, selection };
+  return {
+    context,
+    listeners,
+    listenerTypes,
+    createRectangle,
+    selection,
+    shapes,
+  };
 }
 
 function lastCompartmentGlobalThis(): Record<string, unknown> {
@@ -182,7 +198,7 @@ describe('loadPlugin real initialization path (regression for #11001)', () => {
       const fixture = makeHostFixture();
       setContextBuilder(() => fixture.context);
       const manifest = {
-        ...makeManifest('plugin.js', []),
+        ...makeManifest('plugin.js', ['content:read', 'content:write']),
         host: 'https://plugins.test/',
         scope: 'global' as const,
       };
@@ -207,9 +223,10 @@ describe('loadPlugin real initialization path (regression for #11001)', () => {
         if (order === 'after') await replacement();
         finishFetch({
           ok: true,
-          text: async () => 'globalThis.marker = "cancelled";',
+          text: async () => 'penpot.createRectangle();',
         });
         await cancelled;
+        expect(fixture.shapes).toEqual([]);
         if (order === 'before') {
           expect(getPlugins()).toHaveLength(0);
           expect(fixture.listeners.size).toBe(0);
@@ -225,6 +242,74 @@ describe('loadPlugin real initialization path (regression for #11001)', () => {
     },
   );
 
+  it('cancels a workspace load when another load replaces the same plugin', async () => {
+    const fixture = makeHostFixture();
+    setContextBuilder(() => fixture.context);
+    const manifest = {
+      ...makeManifest('plugin.js', ['content:read', 'content:write']),
+      host: 'https://plugins.test/',
+    };
+    let finishFetch!: (response: object) => void;
+    vi.stubGlobal(
+      'fetch',
+      () =>
+        new Promise((resolve) => {
+          finishFetch = resolve;
+        }),
+    );
+    try {
+      const cancelled = loadPlugin(manifest);
+      await loadPlugin({
+        ...manifest,
+        host: '',
+        code: 'globalThis.marker = "replacement";',
+      });
+      finishFetch({ ok: true, text: async () => 'penpot.createRectangle();' });
+      await cancelled;
+      expect(fixture.shapes).toEqual([]);
+      expect(getPlugins()).toHaveLength(1);
+      expect(lastCompartmentGlobalThis()['marker']).toBe('replacement');
+    } finally {
+      for (const plugin of [...getPlugins()]) plugin.plugin.close();
+      vi.unstubAllGlobals();
+    }
+  });
+
+  it('ignores a fetch failure after a load has been cancelled', async () => {
+    const fixture = makeHostFixture();
+    setContextBuilder(() => fixture.context);
+    const manifest = {
+      ...makeManifest('plugin.js', []),
+      host: 'https://plugins.test/',
+      scope: 'global' as const,
+    };
+    let failFetch!: (error: Error) => void;
+    vi.stubGlobal(
+      'fetch',
+      () =>
+        new Promise((_resolve, reject) => {
+          failFetch = reject;
+        }),
+    );
+    try {
+      const cancelled = loadPlugin(manifest);
+      ɵunloadPlugin(manifest.pluginId);
+      await loadPlugin({
+        ...manifest,
+        host: '',
+        code: 'globalThis.marker = "replacement";',
+      });
+      failFetch(new Error('Cancelled fetch failed'));
+      await cancelled;
+      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);
@@ -315,4 +400,94 @@ describe('loadPlugin real initialization path (regression for #11001)', () => {
     expect(globals['penpotMgmt']).toBeUndefined();
     expect(listeners.size).toBe(0);
   });
+
+  it('cancels a global load at logout before it can execute in another session', async () => {
+    const fixture = makeHostFixture();
+    setContextBuilder(() => fixture.context);
+    const manifest = {
+      ...makeManifest('plugin.js', ['content:read', 'content:write']),
+      pluginId: 'logout-plugin',
+      host: 'https://plugins.test/',
+      scope: 'global' as const,
+    };
+    let finishFetch!: (response: object) => void;
+    vi.stubGlobal(
+      'fetch',
+      () =>
+        new Promise((resolve) => {
+          finishFetch = resolve;
+        }),
+    );
+    try {
+      const cancelled = loadPlugin(manifest);
+      for (const listener of [...fixture.listeners.values()]) {
+        if (listener.type === 'logout') listener.callback();
+      }
+      await loadPlugin({
+        ...manifest,
+        host: '',
+        code: 'globalThis.marker = "new-session";',
+      });
+      finishFetch({ ok: true, text: async () => 'penpot.createRectangle();' });
+      await cancelled;
+      expect(fixture.shapes).toEqual([]);
+      expect(getPlugins()).toHaveLength(1);
+      expect(lastCompartmentGlobalThis()['marker']).toBe('new-session');
+      expect(fixture.listeners.size).toBe(3);
+    } finally {
+      ɵunloadPlugin(manifest.pluginId);
+      vi.unstubAllGlobals();
+    }
+  });
+
+  it('cleans up a failed fetch and allows loading the plugin again', async () => {
+    const fixture = makeHostFixture();
+    setContextBuilder(() => fixture.context);
+    const manifest = {
+      ...makeManifest('plugin.js', []),
+      pluginId: 'failed-fetch-plugin',
+      host: 'https://plugins.test/',
+      scope: 'global' as const,
+    };
+    vi.stubGlobal('fetch', () => Promise.reject(new Error('Fetch failed')));
+    try {
+      await expect(loadPlugin(manifest)).rejects.toThrow('Fetch failed');
+      expect(getPlugins()).toHaveLength(0);
+      expect(fixture.listeners.size).toBe(0);
+      await loadPlugin({
+        ...manifest,
+        host: '',
+        code: 'globalThis.marker = "retry";',
+      });
+      expect(lastCompartmentGlobalThis()['marker']).toBe('retry');
+    } finally {
+      ɵunloadPlugin(manifest.pluginId);
+      vi.unstubAllGlobals();
+    }
+  });
+
+  it('does not register a global plugin that closes during startup and permits reopening', async () => {
+    const fixture = makeHostFixture();
+    setContextBuilder(() => fixture.context);
+    const manifest = {
+      ...makeManifest('penpot.closePlugin();', [
+        'content:read',
+        'content:write',
+      ]),
+      pluginId: 'self-closing-plugin',
+      scope: 'global' as const,
+    };
+    try {
+      await loadPlugin(manifest);
+      expect(getPlugins()).toHaveLength(0);
+      expect(fixture.listeners.size).toBe(0);
+      await loadPlugin({ ...manifest, code: 'penpot.createRectangle();' });
+      expect(fixture.shapes).toEqual([{ type: 'rectangle-marker' }]);
+      expect(getPlugins()).toHaveLength(1);
+    } finally {
+      ɵunloadPlugin(manifest.pluginId);
+    }
+    expect(getPlugins()).toHaveLength(0);
+    expect(fixture.listeners.size).toBe(0);
+  });
 });

plugins/libs/plugins-runtime/src/lib/load-plugin.spec.ts

@@ -33,7 +33,7 @@ vi.mock('./ses.js', () => ({
 describe('plugin-loader', () => {
   let mockContext: Context;
   let manifest: Manifest;
-  let mockPluginApi: Awaited<ReturnType<typeof createPlugin>>;
+  let mockPluginApi: NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
   let mockClose: ReturnType<typeof vi.fn>;
 
   beforeEach(() => {
@@ -61,7 +61,7 @@ describe('plugin-loader', () => {
         close: mockClose,
         sendMessage: vi.fn(),
       },
-    } as unknown as Awaited<ReturnType<typeof createPlugin>>;
+    } as unknown as NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
 
     mockContext = {
       addListener: vi.fn(),
@@ -84,6 +84,7 @@ describe('plugin-loader', () => {
       manifest,
       expect.any(Function),
       undefined,
+      expect.any(AbortSignal),
     );
     expect(mockPluginApi.plugin.close).not.toHaveBeenCalled();
     expect(getPlugins()).toHaveLength(1);
@@ -129,7 +130,7 @@ describe('plugin-loader', () => {
       },
       iframeWindow: mockIframeWindow,
       manifest: { ...manifest, host: 'http://localhost:4202' },
-    } as unknown as Awaited<ReturnType<typeof createPlugin>>;
+    } as unknown as NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
 
     vi.mocked(createPlugin).mockResolvedValue(mockPluginWithIframe);
 
@@ -173,7 +174,7 @@ describe('plugin-loader', () => {
       },
       iframeWindow: mockIframeWindow1,
       manifest: { ...manifest, host: 'http://localhost:4202' },
-    } as unknown as Awaited<ReturnType<typeof createPlugin>>;
+    } as unknown as NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
 
     const mockPluginApi2 = {
       plugin: {
@@ -182,7 +183,7 @@ describe('plugin-loader', () => {
       },
       iframeWindow: mockIframeWindow2,
       manifest: { ...manifest, host: 'http://localhost:4203' },
-    } as unknown as Awaited<ReturnType<typeof createPlugin>>;
+    } as unknown as NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
 
     vi.mocked(createPlugin).mockResolvedValue(mockPluginApi1);
     await loadPlugin(manifest);
@@ -250,6 +251,7 @@ describe('plugin-loader', () => {
       manifest,
       expect.any(Function),
       undefined,
+      expect.any(AbortSignal),
     );
   });
 
@@ -265,6 +267,7 @@ describe('plugin-loader', () => {
       manifest,
       expect.any(Function),
       undefined,
+      expect.any(AbortSignal),
     );
   });
 });

plugins/libs/plugins-runtime/src/lib/load-plugin.ts

@@ -4,8 +4,10 @@ import { loadManifest } from './parse-manifest.js';
 import { Manifest } from './models/manifest.model.js';
 import { createPlugin } from './create-plugin.js';
 
-let plugins: Awaited<ReturnType<typeof createPlugin>>[] = [];
-const pendingPlugins = new Map<Manifest['pluginId'], symbol>();
+type Plugin = NonNullable<Awaited<ReturnType<typeof createPlugin>>>;
+
+let plugins: Plugin[] = [];
+const pendingPlugins = new Map<Manifest['pluginId'], AbortController>();
 
 export type ContextBuilder = (id: string) => Context;
 
@@ -50,7 +52,7 @@ export const loadPlugin = async function (
   closeCallback?: () => void,
   apiExtensions?: object,
 ) {
-  const loadId = Symbol();
+  const load = new AbortController();
   try {
     const context = contextBuilder && contextBuilder(manifest.pluginId);
 
@@ -86,13 +88,17 @@ 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.
-    pendingPlugins.set(manifest.pluginId, loadId);
-    let plugin: Awaited<ReturnType<typeof createPlugin>> | undefined =
-      undefined;
+    pendingPlugins.get(manifest.pluginId)?.abort();
+    pendingPlugins.set(manifest.pluginId, load);
+    let plugin: Plugin | undefined = undefined;
     plugin = await createPlugin(
       context,
       manifest,
       () => {
+        load.abort();
+        if (pendingPlugins.get(manifest.pluginId) === load) {
+          pendingPlugins.delete(manifest.pluginId);
+        }
         plugins = plugins.filter((api) => api !== plugin);
 
         if (closeCallback) {
@@ -100,9 +106,10 @@ export const loadPlugin = async function (
         }
       },
       apiExtensions,
+      load.signal,
     );
-    if (pendingPlugins.get(manifest.pluginId) !== loadId) {
-      plugin.plugin.close();
+    if (!plugin || load.signal.aborted) {
+      plugin?.plugin.close();
       return;
     }
     plugins.push(plugin);
@@ -110,7 +117,7 @@ export const loadPlugin = async function (
     if (manifest.scope !== 'global') closeAllPlugins();
     throw error;
   } finally {
-    if (pendingPlugins.get(manifest.pluginId) === loadId) {
+    if (pendingPlugins.get(manifest.pluginId) === load) {
       pendingPlugins.delete(manifest.pluginId);
     }
   }
@@ -130,7 +137,7 @@ export const ɵloadPluginByUrl = async function (manifestUrl: string) {
 };
 
 export const ɵunloadPlugin = function (id: Manifest['pluginId']) {
-  pendingPlugins.delete(id);
+  pendingPlugins.get(id)?.abort();
   const plugin = plugins.find((plugin) => plugin.manifest.pluginId === id);
 
   if (plugin) {

plugins/libs/plugins-runtime/src/lib/plugin-manager.ts

@@ -14,8 +14,9 @@ export async function createPluginManager(
   manifest: Manifest,
   onCloseCallback: () => void,
   onReloadModal: (code: string) => void,
+  signal?: AbortSignal,
 ) {
-  let code = await loadManifestCode(manifest);
+  let code = '';
 
   let loaded = false;
   let destroyed = false;
@@ -61,6 +62,7 @@ export async function createPluginManager(
   const closePlugin = () => {
     if (destroyed) return;
     destroyed = true;
+    signal?.removeEventListener('abort', closePlugin);
     removeAllEventListeners();
     destroyListener(listenerId);
     destroyListener(logoutId);
@@ -148,7 +150,24 @@ export async function createPluginManager(
     context.removeListener(listenerId);
   };
 
+  signal?.addEventListener('abort', closePlugin, { once: true });
+  try {
+    if (signal?.aborted) {
+      closePlugin();
+    } else {
+      code = await loadManifestCode(manifest);
+    }
+  } catch (error) {
+    if (!destroyed) {
+      closePlugin();
+      throw error;
+    }
+  }
+
   return {
+    get destroyed() {
+      return destroyed;
+    },
     close: closePlugin,
     destroyListener,
     openModal,