From 68e29a8b6e9ea17eb6a18cd1507ba1394d7bf56a Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Fri, 3 Jul 2026 14:45:30 +0200 Subject: [PATCH] fix(plugins): don't treat transient absence from /api/plugins as uninstall (#741) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(plugins): don't treat transient absence from /api/plugins as uninstall The backend clears its plugin registry at the start of load_plugins() and repopulates it incrementally while HTTP stays up, so every backend restart (desktop: Audio Quality soundfont switch, LAN toggle, update restart) serves a window of partial — even empty — /api/plugins responses. loadPlugins() treated absence from the current response as an uninstall, with three destructive consequences for still-loaded plugins: 1. Their settings-panel and screen DOM were wiped while their _loadedPluginScripts entry survived, so the NEXT refetch failed the DOM-existence check and re-evaluated the plugin's screen.js mid-session. For the desktop audio_engine plugin that re-ran init() against the surviving native audio chain and exactly duplicated every VST/NAM/IR stage (the alpha testers' "chain duplicates after leaving the Audio menu" / blown-out gain reports). 2. _reconcilePluginStyles dropped their stylesheet, leaving them visible but unstyled until they reappeared. 3. The stale-contribution sweep unmounted their UI contributions and unregistered their capability participant with no re-registration path (plugin scripts don't re-run thanks to the loadedScripts guard). Absence is now a non-signal everywhere in loadPlugins: the DOM wipe and style reconcile are scoped to plugins the response actually names, and the absence sweep is removed. Present plugins still fully re-sync via _registerLegacyPluginUiContributions each round; failed plugins are present in the response and still cleaned up; nav is rebuilt from the response so genuinely uninstalled plugins drop out of it, and their (un-unloadable) already-evaluated scripts keep their DOM until reload. Co-Authored-By: Claude Fable 5 * test: update idempotence contract to the absence-is-not-uninstall invariant The removed-plugin sweep contract pinned the old behavior this branch deletes; pin the new invariant instead (no absence sweep + respondedIds scoping on the DOM/style reconcilers). Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- static/app.js | 62 +++++++----- tests/js/plugin_hydration_wipe.test.js | 115 +++++++++++++++++++++++ tests/js/plugin_style_injection.test.js | 13 ++- tests/test_plugin_runtime_idempotence.py | 24 +++-- 4 files changed, 182 insertions(+), 32 deletions(-) create mode 100644 tests/js/plugin_hydration_wipe.test.js diff --git a/static/app.js b/static/app.js index 7bdcf01..50a6181 100644 --- a/static/app.js +++ b/static/app.js @@ -10977,20 +10977,19 @@ async function loadPlugins() { const nameDelta = String(a.name || a.id || '').localeCompare(String(b.name || b.id || '')); return nameDelta || String(a.id || '').localeCompare(String(b.id || '')); }); - const livePluginIds = new Set(plugins.map((plugin) => plugin.id)); - for (const [pluginId, contributions] of _pluginUiContributions) { - if (livePluginIds.has(pluginId)) continue; - const stalePlugin = { id: pluginId }; - for (const contribution of contributions) { - await _commandUiDomain(contribution.domain, 'unmount', stalePlugin, contribution); - } - try { - window.feedBack?.capabilities?.unregisterParticipant?.(pluginId); - } catch (e) { - console.warn(`capability participant unregister failed for ${pluginId}:`, e); - } - _pluginUiContributions.delete(pluginId); - } + // NOTE deliberately NO stale-contribution sweep for plugins absent + // from this response. Absent ≠ uninstalled: the backend clears its + // plugin registry at the start of load_plugins() and repopulates it + // incrementally while HTTP stays up, so every backend restart serves a + // window of partial (even empty) responses. The old sweep unmounted UI + // contributions and unregistered capability participants on mere + // absence, permanently breaking still-loaded plugins — their scripts + // don't re-run (loadedScripts guard below), so nothing ever + // re-registered. A genuine mid-session uninstall now leaves the + // (already-evaluated, un-unloadable) script's contributions in place + // until reload; its nav entry still disappears because nav is rebuilt + // from the response each round. Same invariant as the settings/screen + // DOM wipe and _reconcilePluginStyles below. console.log('[feedBack] loadPlugins: got', plugins.length, 'plugins'); try { @@ -11132,17 +11131,23 @@ async function loadPlugins() { loadedStyles.set(plugin.id, wantedVersion); }; const _reconcilePluginStyles = (currentPlugins) => { - // Drop stylesheets for plugins that vanished from /api/plugins or are - // no longer ready+styled this round. _injectPluginStyles below only - // visits plugins still returned by the API, so an uninstalled or - // newly-not-ready plugin would otherwise keep its applying. + // Drop stylesheets for plugins the response KNOWS about but that + // are no longer ready+styled this round. _injectPluginStyles below + // only visits plugins still returned by the API, so a newly-not- + // ready or unstyled plugin would otherwise keep its + // applying. Plugins merely ABSENT from the response keep their + // stylesheet — a transient partial response during a backend + // restart is not an uninstall (same invariant as the screen/ + // settings wipe below), and stripping the would leave a + // still-loaded plugin visible but unstyled. + const responded = new Set(currentPlugins.map((p) => p.id)); const styled = new Set( currentPlugins .filter((p) => (p.status || 'ready') === 'ready' && p.has_styles && p.styles) .map((p) => p.id), ); for (const id of Array.from(loadedStyles.keys())) { - if (!styled.has(id)) { + if (responded.has(id) && !styled.has(id)) { _removePluginStyleTags(id); loadedStyles.delete(id); } @@ -11155,6 +11160,18 @@ async function loadPlugins() { if (pid) existingSettingsByPluginId.set(pid, child); } } + // Plugins named in THIS response. A plugin can be transiently absent + // from /api/plugins — the backend clears its registry at the start of + // load_plugins() and repopulates it incrementally while HTTP stays up, + // so every backend restart serves a window of partial (even empty) + // responses. The wipe loops below must never treat that absence as an + // uninstall: stripping a still-loaded plugin's DOM while keeping its + // loadedScripts entry made the NEXT refetch fail the DOM check and + // re-evaluate its screen.js mid-session — which duplicated the desktop + // audio_engine's native signal chain (its init re-ran against the + // surviving engine chain). Absent plugins keep their DOM and script; + // they're re-reconciled when they reappear in a later response. + const respondedIds = new Set(plugins.map((p) => p.id)); const alreadyHydrated = new Set(); for (const p of plugins) { if (!p.has_script) continue; @@ -11182,7 +11199,10 @@ async function loadPlugins() { for (const container of _pluginSettingsContainers()) { [...container.children].forEach((el) => { const pid = el.dataset ? el.dataset.pluginId : null; - if (!pid || !alreadyHydrated.has(pid)) el.remove(); + // Remove junk (no plugin id) and plugins the response KNOWS + // about but that failed hydration; leave plugins absent from + // the response untouched (see respondedIds above). + if (!pid || (respondedIds.has(pid) && !alreadyHydrated.has(pid))) el.remove(); }); } document.querySelectorAll('.screen[id^="plugin-"]').forEach((el) => { @@ -11191,7 +11211,7 @@ async function loadPlugins() { // change shipped — both forms strip a single leading "plugin-". const pid = (el.dataset && el.dataset.pluginId) || el.id.replace(/^plugin-/, ''); - if (!alreadyHydrated.has(pid)) el.remove(); + if (!pid || (respondedIds.has(pid) && !alreadyHydrated.has(pid))) el.remove(); }); // Plugin settings area hosts both "Plugin Updates" and per-plugin diff --git a/tests/js/plugin_hydration_wipe.test.js b/tests/js/plugin_hydration_wipe.test.js new file mode 100644 index 0000000..3d824cc --- /dev/null +++ b/tests/js/plugin_hydration_wipe.test.js @@ -0,0 +1,115 @@ +// Verify loadPlugins' plugin-DOM wipe loops in static/app.js: a plugin that is +// merely ABSENT from the current /api/plugins response (transient partial +// response while the backend's plugin registry is repopulating after a +// restart) must keep its settings panel and screen DOM. Wiping it while its +// _loadedPluginScripts entry survives made the next refetch fail the +// DOM-existence check and re-evaluate the plugin's screen.js mid-session — +// which duplicated the desktop audio_engine's native signal chain. Plugins +// the response knows about but that failed hydration are still wiped, as is +// junk DOM carrying no plugin id. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +const APP_JS = path.join(__dirname, '..', '..', 'static', 'app.js'); + +// Slice the wipe block out of loadPlugins by its stable landmarks: from the +// nav reset that opens it to the comment introducing the next section. +function extractWipeBlock(src) { + const start = src.indexOf("navContainer.innerHTML = '';"); + assert.ok(start !== -1, 'wipe block start (nav reset) not found'); + const end = src.indexOf('// Plugin settings area hosts', start); + assert.ok(end !== -1, 'wipe block end marker not found'); + return src.slice(start, end); +} + +function makeEl(pluginId, id) { + return { + dataset: pluginId != null ? { pluginId } : {}, + id: id || (pluginId != null ? `plugin-${pluginId}` : ''), + removed: false, + remove() { + this.removed = true; + const idx = this._parent ? this._parent.indexOf(this) : -1; + if (idx >= 0) this._parent.splice(idx, 1); + }, + }; +} + +function runWipe({ respondedIds, alreadyHydrated, settingsChildren, screens }) { + const src = fs.readFileSync(APP_JS, 'utf8'); + const block = extractWipeBlock(src); + settingsChildren.forEach((el) => { el._parent = settingsChildren; }); + const container = { children: settingsChildren }; + const sandbox = { + navContainer: { innerHTML: 'seed' }, + mobileNavContainer: { innerHTML: 'seed' }, + _pluginSettingsContainers: () => [container], + respondedIds, + alreadyHydrated, + document: { + querySelectorAll: (sel) => { + assert.equal(sel, '.screen[id^="plugin-"]'); + return screens.slice(); + }, + }, + }; + vm.runInNewContext(block, sandbox, { filename: 'wipe-block.js' }); + return sandbox; +} + +test('plugin absent from the response keeps its settings + screen DOM', () => { + const settings = makeEl('audio_engine'); + const screen = makeEl('audio_engine'); + runWipe({ + respondedIds: new Set(), // partial response: plugin missing + alreadyHydrated: new Set(), // scan loop never saw it either + settingsChildren: [settings], + screens: [screen], + }); + assert.equal(settings.removed, false, 'settings panel must survive a partial response'); + assert.equal(screen.removed, false, 'screen must survive a partial response'); +}); + +test('plugin present in the response but not hydrated is wiped', () => { + const settings = makeEl('stale_plugin'); + const screen = makeEl('stale_plugin'); + runWipe({ + respondedIds: new Set(['stale_plugin']), + alreadyHydrated: new Set(), + settingsChildren: [settings], + screens: [screen], + }); + assert.equal(settings.removed, true); + assert.equal(screen.removed, true); +}); + +test('hydrated plugin present in the response is preserved', () => { + const settings = makeEl('audio_engine'); + const screen = makeEl('audio_engine'); + runWipe({ + respondedIds: new Set(['audio_engine']), + alreadyHydrated: new Set(['audio_engine']), + settingsChildren: [settings], + screens: [screen], + }); + assert.equal(settings.removed, false); + assert.equal(screen.removed, false); +}); + +test('junk DOM without a plugin id is still removed', () => { + const junkSettings = makeEl(null); + // Screen whose id strips to '' (no dataset.pluginId, bare "plugin-" id). + const junkScreen = makeEl(null, 'plugin-'); + runWipe({ + respondedIds: new Set(['whatever']), + alreadyHydrated: new Set(), + settingsChildren: [junkSettings], + screens: [junkScreen], + }); + assert.equal(junkSettings.removed, true); + assert.equal(junkScreen.removed, true); +}); diff --git a/tests/js/plugin_style_injection.test.js b/tests/js/plugin_style_injection.test.js index d6dd9bb..7bf5ee6 100644 --- a/tests/js/plugin_style_injection.test.js +++ b/tests/js/plugin_style_injection.test.js @@ -204,15 +204,20 @@ test('does not collide tags across two different plugins', () => { assert.deepEqual(headLinks.map((l) => l.dataset.pluginId).sort(), ['a', 'b']); }); -test('reconcile removes the of a plugin that vanished from /api/plugins', () => { +test('reconcile keeps the of a plugin absent from a partial response', () => { const { inject, reconcile, headLinks } = setupSandbox(); inject(plug({ id: 'a' })); inject(plug({ id: 'b' })); assert.equal(headLinks.length, 2); - // `a` is no longer returned (uninstalled) — its stylesheet must be dropped. + // `a` is missing from this response. That happens transiently during a + // backend restart (the plugin registry repopulates while HTTP stays up), + // so absence is NOT an uninstall signal — the still-loaded plugin must + // keep its stylesheet or it renders visible-but-unstyled until it + // reappears. Explicit removal still happens via the not-ready/unstyled + // paths (tests below). reconcile([plug({ id: 'b' })]); - assert.equal(headLinks.length, 1); - assert.equal(headLinks[0].dataset.pluginId, 'b'); + assert.equal(headLinks.length, 2); + assert.deepEqual(headLinks.map((l) => l.dataset.pluginId).sort(), ['a', 'b']); }); test('reconcile removes the of a plugin that is no longer ready', () => { diff --git a/tests/test_plugin_runtime_idempotence.py b/tests/test_plugin_runtime_idempotence.py index e5b31f0..92a44e0 100644 --- a/tests/test_plugin_runtime_idempotence.py +++ b/tests/test_plugin_runtime_idempotence.py @@ -38,15 +38,25 @@ def test_plugin_loader_unmounts_previous_ui_contributions_before_reregistering() assert "await _commandUiDomain(contribution.domain, 'mount', plugin, contribution)" in source -def test_plugin_loader_unmounts_contributions_for_removed_plugins(): +def test_plugin_loader_does_not_treat_response_absence_as_uninstall(): + # A plugin transiently absent from /api/plugins (the backend clears its + # registry at the start of load_plugins() and repopulates incrementally + # while HTTP stays up, so restarts serve partial responses) must NOT be + # torn down: the old absence sweep unmounted UI contributions and + # unregistered the capability participant with no re-registration path + # (plugin scripts don't re-run), and the DOM/style wipes forced a + # mid-session screen.js re-evaluation that duplicated the desktop + # audio_engine's native signal chain. source = (ROOT / "static" / "app.js").read_text(encoding="utf-8") - assert "const livePluginIds = new Set(plugins.map((plugin) => plugin.id))" in source - assert "for (const [pluginId, contributions] of _pluginUiContributions)" in source - assert "const stalePlugin = { id: pluginId }" in source - assert "await _commandUiDomain(contribution.domain, 'unmount', stalePlugin, contribution)" in source - assert "window.feedBack?.capabilities?.unregisterParticipant?.(pluginId)" in source - assert "_pluginUiContributions.delete(pluginId)" in source + # The absence-triggered sweep is gone (rationale comment in its place)... + assert "const livePluginIds" not in source + assert "const stalePlugin = { id: pluginId }" not in source + assert "deliberately NO stale-contribution sweep" in source + # ...and the DOM/style reconcilers only act on plugins the response names. + assert "const respondedIds = new Set(plugins.map((p) => p.id))" in source + assert "respondedIds.has(pid) && !alreadyHydrated.has(pid)" in source + assert "responded.has(id) && !styled.has(id)" in source