diff --git a/docs-site/plugins.md b/docs-site/plugins.md index f80e8bb5..41af7875 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()`. 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 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. ```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 a0ab1a6b..6413f145 100644 --- a/src/core/services/plugins/plugin-host.test.ts +++ b/src/core/services/plugins/plugin-host.test.ts @@ -42,7 +42,10 @@ function createFakeWindow() { return fake; } -function createHarness(config: Record) { +function createHarness( + config: Record, + overrides: Partial> = {}, +) { const root = mkdtempSync(path.join(os.tmpdir(), 'subminer-plugins-')); const errors: string[] = []; const notifications: string[] = []; @@ -82,6 +85,7 @@ function createHarness(config: Record) { return { window: window.asWindow(), created: true }; }, unstable: { marker: 'internals' }, + ...overrides, }; return { root: deps.pluginsDir, @@ -227,6 +231,30 @@ test('mpv listener errors are contained and unload detaches listeners and calls } }); +test('unload gives up on a deactivate that never settles and still disposes the plugin', async () => { + const h = createHarness({ stuck: enabled() }, { deactivateTimeoutMs: 20 }); + const client = createFakeMpvClient(); + try { + writePlugin(h.root, 'stuck', { + 'plugin.json': JSON.stringify({ main: 'main.cjs' }), + 'main.cjs': `exports.activate = (ctx) => { + ctx.mpv.on('pause-change', () => ctx.notify('still listening')); + return () => new Promise(() => {}); + };`, + }); + h.host.attachMpvClient(client); + await h.host.loadAll(); + + await h.host.unloadAll(); + client.emit('pause-change', { paused: true }); + + assert.deepEqual(h.notifications, []); + assert.deepEqual(h.errors, ['deactivate failed: timed out after 20 ms']); + } finally { + h.cleanup(); + } +}); + test('events follow a replacement mpv client', async () => { const h = createHarness({ watcher: enabled() }); const first = createFakeMpvClient(); diff --git a/src/core/services/plugins/plugin-host.ts b/src/core/services/plugins/plugin-host.ts index b325bda4..4b7b1714 100644 --- a/src/core/services/plugins/plugin-host.ts +++ b/src/core/services/plugins/plugin-host.ts @@ -25,6 +25,8 @@ 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; interface MpvClientEvents { on: (event: PluginMpvEventName, listener: (payload: unknown) => void) => void; @@ -52,6 +54,8 @@ 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. */ + deactivateTimeoutMs?: number; } interface LoadedPlugin { @@ -349,14 +353,26 @@ export function createPluginHost(deps: PluginHostDeps) { async unloadAll(): Promise { loadPromise = null; + const timeoutMs = deps.deactivateTimeoutMs ?? DEFAULT_DEACTIVATE_TIMEOUT_MS; await Promise.all( [...plugins.values()].map(async (plugin) => { + let timer: ReturnType | undefined; try { - await plugin.deactivate?.(); + await Promise.race([ + Promise.resolve().then(() => plugin.deactivate?.()), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error(`timed out after ${timeoutMs} ms`)), + timeoutMs, + ); + }), + ]); } 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 0f4606d3..86b87359 100644 --- a/src/main.ts +++ b/src/main.ts @@ -4403,6 +4403,7 @@ const { void appState.discordPresenceService?.stop(); appState.discordPresenceService = null; }, + unloadPlugins: () => pluginHost.unloadAll(), }, shouldRestoreWindowsOnActivateMainDeps: { isOverlayRuntimeInitialized: () => appState.overlayRuntimeInitialized, @@ -7067,4 +7068,3 @@ registerSubtitleGenerationIpc({ openModal: () => openSubtitleGenerationModal(createOverlayHostedModalOpenDeps()), }); app.on('before-quit', () => subtitleGenerationRuntime.cancel()); -app.on('before-quit', () => void pluginHost.unloadAll()); diff --git a/src/main/runtime/app-lifecycle-actions.test.ts b/src/main/runtime/app-lifecycle-actions.test.ts index 325fd60e..d7e6c1bb 100644 --- a/src/main/runtime/app-lifecycle-actions.test.ts +++ b/src/main/runtime/app-lifecycle-actions.test.ts @@ -55,6 +55,7 @@ function makeCleanupDeps(calls: string[], overrides: Partial = {}): cleanupRemoteMediaWindows: record('cleanupRemoteMediaWindows'), cleanupJellyfinSubtitleCache: record('cleanupJellyfinSubtitleCache'), stopDiscordPresenceService: record('stopDiscordPresenceService'), + unloadPlugins: record('unloadPlugins'), ...overrides, }; } @@ -99,6 +100,31 @@ 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 () => { + const calls: string[] = []; + let finishUnload!: () => void; + const cleanup = createOnWillQuitCleanupHandler( + makeCleanupDeps(calls, { + unloadPlugins: () => { + calls.push('unloadPlugins'); + return new Promise((resolve) => (finishUnload = resolve)); + }, + }), + ); + + let settled = false; + const pending = cleanup().then(() => { + settled = true; + }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(calls[0], 'unloadPlugins'); + assert.equal(settled, false); + + finishUnload(); + await pending; + assert.equal(settled, true); +}); + test('on will quit cleanup handler cleans subtitle caches when stopping remote session fails', async () => { const calls: string[] = []; const cleanup = createOnWillQuitCleanupHandler( diff --git a/src/main/runtime/app-lifecycle-actions.ts b/src/main/runtime/app-lifecycle-actions.ts index e754b280..a744bc24 100644 --- a/src/main/runtime/app-lifecycle-actions.ts +++ b/src/main/runtime/app-lifecycle-actions.ts @@ -59,8 +59,14 @@ export function createOnWillQuitCleanupHandler(deps: { cleanupRemoteMediaWindows: () => void; cleanupJellyfinSubtitleCache: () => void; stopDiscordPresenceService: () => void; + unloadPlugins: () => void | Promise; }) { 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) => { + cleanupErrors.push(error); + }); deps.destroyTray(); deps.stopConfigHotReload(); deps.restorePreviousSecondarySubVisibility(); @@ -68,7 +74,6 @@ export function createOnWillQuitCleanupHandler(deps: { deps.unregisterAllGlobalShortcuts(); deps.stopSubtitleWebsocket(); deps.stopTexthookerService(); - const cleanupErrors: unknown[] = []; const stopSyncAutoScheduler = Promise.resolve(deps.stopSyncAutoScheduler()).catch( (error: unknown) => { cleanupErrors.push(error); @@ -118,6 +123,7 @@ export function createOnWillQuitCleanupHandler(deps: { deps.cleanupRemoteMediaWindows(); deps.stopDiscordPresenceService(); await stopSyncAutoScheduler; + await unloadPlugins; if (cleanupErrors.length > 0) { throw cleanupErrors[0]; } diff --git a/src/main/runtime/app-lifecycle-main-cleanup.test.ts b/src/main/runtime/app-lifecycle-main-cleanup.test.ts index 9bc04239..a5633871 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.test.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.test.ts @@ -52,6 +52,7 @@ function buildCleanupDeps(overrides: Partial = {}) { cleanupRemoteMediaWindows: noop, cleanupJellyfinSubtitleCache: noop, stopDiscordPresenceService: noop, + unloadPlugins: noop, ...overrides, })(); } diff --git a/src/main/runtime/app-lifecycle-main-cleanup.ts b/src/main/runtime/app-lifecycle-main-cleanup.ts index 888c0a1f..7f5d877d 100644 --- a/src/main/runtime/app-lifecycle-main-cleanup.ts +++ b/src/main/runtime/app-lifecycle-main-cleanup.ts @@ -65,6 +65,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { cleanupRemoteMediaWindows: () => void; cleanupJellyfinSubtitleCache: () => void; stopDiscordPresenceService: () => void; + unloadPlugins: () => void | Promise; }) { return () => ({ destroyTray: () => deps.destroyTray(), @@ -154,5 +155,6 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: { cleanupRemoteMediaWindows: () => deps.cleanupRemoteMediaWindows(), cleanupJellyfinSubtitleCache: () => deps.cleanupJellyfinSubtitleCache(), stopDiscordPresenceService: () => deps.stopDiscordPresenceService(), + unloadPlugins: () => deps.unloadPlugins(), }); }