From 4bb85f19bc74e8c7832059f2503ce0bcc30b9634 Mon Sep 17 00:00:00 2001 From: sudacode Date: Tue, 27 Oct 2026 00:33:29 -0700 Subject: [PATCH] fix(plugins): unload late-loading plugins before quit teardown - unloadAll waits for an in-progress load so plugins still activating get deactivated too - One 2 s budget covers the load wait and all deactivates - Quit cleanup awaits plugin unload before any other teardown - Force-quit timer now allows the plugin budget plus 2 s --- docs-site/plugins.md | 2 +- src/core/services/plugins/plugin-host.test.ts | 24 +++++++++ src/core/services/plugins/plugin-host.ts | 52 +++++++++++++------ src/main.ts | 8 ++- .../runtime/app-lifecycle-actions.test.ts | 20 ++++++- src/main/runtime/app-lifecycle-actions.ts | 10 ++-- 6 files changed, 92 insertions(+), 24 deletions(-) diff --git a/docs-site/plugins.md b/docs-site/plugins.md index 41af7875..880bd6fe 100644 --- a/docs-site/plugins.md +++ b/docs-site/plugins.md @@ -67,7 +67,7 @@ A plugin folder needs a `plugin.json`: { "name": "My Plugin", "version": "1.0.0", "main": "main.js" } ``` -`main` points to a CommonJS module (`.js`) or an ES module (`.mjs`) that exports `activate(ctx)`. It may return a cleanup function, or the module can export `deactivate()`. On quit, SubMiner waits up to 3 seconds for it, including any promise it returns. Everything a plugin registers through `ctx` (listeners, timers, actions, panels, running programs) is cleaned up automatically when SubMiner quits. +`main` points to a CommonJS module (`.js`) or an ES module (`.mjs`) that exports `activate(ctx)`. It may return a cleanup function, or the module can export `deactivate()`. On quit, SubMiner waits up to 2 seconds for it, including any promise it returns. Everything a plugin registers through `ctx` (listeners, timers, actions, panels, running programs) is cleaned up automatically when SubMiner quits. ```js function activate(ctx) { diff --git a/src/core/services/plugins/plugin-host.test.ts b/src/core/services/plugins/plugin-host.test.ts index 6413f145..d00f2230 100644 --- a/src/core/services/plugins/plugin-host.test.ts +++ b/src/core/services/plugins/plugin-host.test.ts @@ -279,6 +279,30 @@ test('events follow a replacement mpv client', async () => { } }); +test('unload waits for an in-progress load so late plugins are unloaded too', async () => { + const h = createHarness({ late: enabled() }); + const client = createFakeMpvClient(); + try { + writePlugin(h.root, 'late', { + 'plugin.json': JSON.stringify({ main: 'main.cjs' }), + 'main.cjs': `exports.activate = (ctx) => { + ctx.mpv.on('pause-change', () => ctx.notify('still listening')); + return () => ctx.notify('deactivated'); + };`, + }); + h.host.attachMpvClient(client); + + // The plugin is still reading its manifest when unload starts. + void h.host.loadAll(); + await h.host.unloadAll(); + client.emit('pause-change', { paused: true }); + + assert.deepEqual(h.notifications, ['deactivated']); + } finally { + h.cleanup(); + } +}); + test('runAction waits for a load that is still in progress', async () => { const h = createHarness({ slow: enabled() }); try { diff --git a/src/core/services/plugins/plugin-host.ts b/src/core/services/plugins/plugin-host.ts index 4b7b1714..5bbc13a3 100644 --- a/src/core/services/plugins/plugin-host.ts +++ b/src/core/services/plugins/plugin-host.ts @@ -25,8 +25,11 @@ import { const MIN_TIMER_INTERVAL_MS = 100; const DEFAULT_WINDOW_WIDTH = 960; const DEFAULT_WINDOW_HEIGHT = 720; -// Quit waits for unloadAll, so a deactivate that never settles must not hold the app open. -const DEFAULT_DEACTIVATE_TIMEOUT_MS = 3_000; +/** + * Quit waits for unloadAll, so a load or deactivate that never settles must not hold the app open. + * The forced-exit fallback in main adds its own budget on top of this. + */ +export const PLUGIN_DEACTIVATE_TIMEOUT_MS = 2_000; interface MpvClientEvents { on: (event: PluginMpvEventName, listener: (payload: unknown) => void) => void; @@ -54,7 +57,7 @@ export interface PluginHostDeps { /** Test seam; defaults to Node's `require`, which also loads synchronous ES modules. */ loadModule?: (entryPath: string) => PluginModule; exec?: typeof execPluginCommand; - /** How long unload waits for each plugin's deactivate before disposing it anyway. */ + /** How long unload waits, in total, for loading and deactivate before disposing anyway. */ deactivateTimeoutMs?: number; } @@ -66,6 +69,21 @@ interface LoadedPlugin { lifetime: AbortController; } +/** Resolves when `promise` settles, or rejects once `timeoutMs` passes first. */ +async function settleWithin(promise: Promise, timeoutMs: number): Promise { + let timer: ReturnType | undefined; + try { + await Promise.race([ + promise, + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(`timed out after ${timeoutMs} ms`)), timeoutMs); + }), + ]); + } finally { + clearTimeout(timer); + } +} + function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } @@ -352,27 +370,31 @@ export function createPluginHost(deps: PluginHostDeps) { }, async unloadAll(): Promise { + // One budget covers the load wait and every deactivate, so quit is bounded by it. + const timeoutMs = deps.deactivateTimeoutMs ?? PLUGIN_DEACTIVATE_TIMEOUT_MS; + const deadline = Date.now() + timeoutMs; + // Plugins still reading their files are not in `plugins` yet; let the load finish first. + if (loadPromise) { + try { + await settleWithin(loadPromise, timeoutMs); + } catch (error) { + deps + .createLogger('plugins') + .warn(`Unloading before load finished: ${errorMessage(error)}`); + } + } loadPromise = null; - const timeoutMs = deps.deactivateTimeoutMs ?? DEFAULT_DEACTIVATE_TIMEOUT_MS; await Promise.all( [...plugins.values()].map(async (plugin) => { - let timer: ReturnType | undefined; try { - await Promise.race([ + await settleWithin( Promise.resolve().then(() => plugin.deactivate?.()), - new Promise((_, reject) => { - timer = setTimeout( - () => reject(new Error(`timed out after ${timeoutMs} ms`)), - timeoutMs, - ); - }), - ]); + Math.max(0, deadline - Date.now()), + ); } catch (error) { deps .createLogger(`plugin:${plugin.id}`) .error(`deactivate failed: ${errorMessage(error)}`); - } finally { - clearTimeout(timer); } disposePlugin(plugin); }), diff --git a/src/main.ts b/src/main.ts index 86b87359..83088bbc 100644 --- a/src/main.ts +++ b/src/main.ts @@ -512,7 +512,10 @@ import { openSubsyncManualModal as openSubsyncManualModalRuntime } from './main/ import { openSessionHelpModal as openSessionHelpModalRuntime } from './main/runtime/session-help-open'; import { openChangelogModal as openChangelogModalRuntime } from './main/runtime/changelog-open'; import { createChangelogRuntime } from './main/runtime/changelog/changelog-runtime'; -import { createPluginHost } from './core/services/plugins/plugin-host'; +import { + createPluginHost, + PLUGIN_DEACTIVATE_TIMEOUT_MS, +} from './core/services/plugins/plugin-host'; import { buildPluginSubtitleContext } from './core/services/plugins/subtitle-context'; import type { PluginSubtitleLine } from './core/services/plugins/types'; import { @@ -1054,6 +1057,7 @@ function requestAppQuit(): void { logger.warn('Failed to stop stats server while quitting.', error); }); if (!forceQuitTimer) { + // Quit cleanup unloads plugins first, so allow their budget plus 2 s for the rest. forceQuitTimer = setTimeout(() => { logger.warn('App quit timed out; forcing process exit.'); void createForceQuitHandler({ @@ -1061,7 +1065,7 @@ function requestAppQuit(): void { logError: (error) => logger.error('Failed to finalize stats before forced exit.', error), exit: () => app.exit(0), })(); - }, 2000); + }, PLUGIN_DEACTIVATE_TIMEOUT_MS + 2000); } app.quit(); } diff --git a/src/main/runtime/app-lifecycle-actions.test.ts b/src/main/runtime/app-lifecycle-actions.test.ts index d7e6c1bb..815ab91f 100644 --- a/src/main/runtime/app-lifecycle-actions.test.ts +++ b/src/main/runtime/app-lifecycle-actions.test.ts @@ -100,7 +100,7 @@ test('on will quit cleanup handler flushes before teardown and awaits stats befo assert.ok(calls.indexOf('destroyImmersionTracker') < calls.indexOf('destroyAnkiIntegration')); }); -test('on will quit cleanup handler starts plugin unload first and waits for it to settle', async () => { +test('on will quit cleanup handler finishes plugin unload before any other teardown', async () => { const calls: string[] = []; let finishUnload!: () => void; const cleanup = createOnWillQuitCleanupHandler( @@ -117,12 +117,28 @@ test('on will quit cleanup handler starts plugin unload first and waits for it t settled = true; }); await new Promise((resolve) => setImmediate(resolve)); - assert.equal(calls[0], 'unloadPlugins'); + assert.deepEqual(calls, ['unloadPlugins']); assert.equal(settled, false); finishUnload(); await pending; assert.equal(settled, true); + assert.ok(calls.indexOf('destroyMpvSocket') > 0); +}); + +test('on will quit cleanup handler keeps tearing down when plugin unload fails', async () => { + const calls: string[] = []; + const cleanup = createOnWillQuitCleanupHandler( + makeCleanupDeps(calls, { + unloadPlugins: async () => { + throw new Error('unload failed'); + }, + }), + ); + + await assert.rejects(cleanup(), /unload failed/); + assert.ok(calls.includes('destroyMpvSocket')); + assert.ok(calls.includes('stopDiscordPresenceService')); }); test('on will quit cleanup handler cleans subtitle caches when stopping remote session fails', async () => { diff --git a/src/main/runtime/app-lifecycle-actions.ts b/src/main/runtime/app-lifecycle-actions.ts index a744bc24..a0e96b37 100644 --- a/src/main/runtime/app-lifecycle-actions.ts +++ b/src/main/runtime/app-lifecycle-actions.ts @@ -63,10 +63,13 @@ export function createOnWillQuitCleanupHandler(deps: { }) { return async (): Promise => { const cleanupErrors: unknown[] = []; - // Plugins deactivate first, while the app is still intact; the host bounds how long that takes. - const unloadPlugins = (async () => deps.unloadPlugins())().catch((error: unknown) => { + // Plugins deactivate before any teardown so they can still reach mpv and the overlay; + // the plugin host bounds how long that takes. + try { + await deps.unloadPlugins(); + } catch (error) { cleanupErrors.push(error); - }); + } deps.destroyTray(); deps.stopConfigHotReload(); deps.restorePreviousSecondarySubVisibility(); @@ -123,7 +126,6 @@ export function createOnWillQuitCleanupHandler(deps: { deps.cleanupRemoteMediaWindows(); deps.stopDiscordPresenceService(); await stopSyncAutoScheduler; - await unloadPlugins; if (cleanupErrors.length > 0) { throw cleanupErrors[0]; }