Skip to content

Desktop plugin loader: hung imports time out, timers die on disable, duplicate ids error, broken reloads retire stale code - #118902

Merged
teknium1 merged 1 commit into
mainfrom
fix/desktop-runtime-loader-hardening
Sep 22, 2026
Merged

teknium1 merged 1 commit into
mainfrom
fix/desktop-runtime-loader-hardening

Conversation

@teknium1

Copy link
Copy Markdown
Collaborator

Desktop runtime plugins can no longer freeze the plugin scan, leak timers past disable, silently steal each other's id, or keep stale code live after a broken save.

Four findings from a static + live-probe audit of apps/desktop/src/contrib/runtime-loader.ts (the <hermes home>/desktop-plugins/*/plugin.js door). No issues exist for these — they came from a code audit, not a report. Root cause for all four: the loader's error isolation ("a broken plugin can never take the app down / clean reload") held only for the happy path of register().

Changes

Finding Before After
Hung import A plugin with a top-level await that never settles hangs import(); scanDiskPlugins awaits it in a sequential loop under a scanning re-entrancy guard, so every later folder never loads and every future poll/watch tick returns early — dead loader until restart. import() races a 10 s deadline (IMPORT_TIMEOUT_MS). The plugin gets status:'error' (import timed out after 10s — module evaluation never settled) on its own row; the scan continues with the rest.
Timer / DOM leak on disable Only registrations through ctx were disposed; bare setInterval / addEventListener survived disable and every hot-reload (a runaway poller keeps running after the user turns the plugin off). PluginContext gains setTimeout / setInterval / addEventListener (contrib/plugin.ts::createPluginLifetime), tracked with the plugin and torn down on unload/reload/disable. Fired one-shots drop out of the set on their own; the host disposer is registered lazily on first use so plugins that never use them add nothing. SDK doc states bare globals are NOT tracked.
Duplicate id last-wins Two folders exporting the same id both load; the second's activate disposes the first's registrations and overwrites its row, and each file's hot-reload flips ownership — silently, in unsorted readDir order. First loaded owns the id; a later file with a different path errors on its own folder row: duplicate id "x", already loaded from <path>. Folder listing is sorted by name so ownership is deterministic across launches.
Stale incarnation after a broken reload loadDiskPlugin kept entry.id and left the old module's contributions + activate handle live when a save no longer loaded (syntax error, now also timeout/duplicate). The Plugins tab showed the broken file as "loaded" beside a folder-named error row, and toggling re-ran stale code. When the file no longer yields the previous id, the previous incarnation is unloaded and its row/handle dropped — the error row is the only row left.

Docs: website/docs/developer-guide/desktop-plugin-sdk.md — PluginContext block gains onDispose + the three scoped helpers; three new Pitfalls bullets (bare globals untracked, 10 s evaluation deadline, one id one file).

Validation

One invariant test per fix in runtime-loader.test.ts (loader hardening describe), A/B'd against origin/main sources with a cp swap:

Test base fix
hung import times out as its own error; later folder still loads ✗ (test itself hangs → 15 s timeout, i.e. the freeze) ✓
ctx.setInterval / ctx.addEventListener die on unload ✗ ✓
duplicate id: first (sorted) wins, later errors on own row ✗ ✓
broken save retires the previous incarnation ✗ ✓
  • npm run check:lint (typecheck + eslint): 0 errors.
  • npx vitest run src/contrib/ src/sdk/: 124/124. Full desktop vitest run recorded below.

Not changed (out of scope, deliberately): scanDiskPlugins stays sequential — a hung plugin now costs at most 10 s per scan pass rather than a dead loader; a duplicate row stays error until its own next reload after the owner is removed (the next hot-edit or manual "Reload desktop plugins" clears it).

Infographic

Desktop plugin loader: four holes closed

…ers, duplicate ids and broken reloads

Four error-isolation holes in the disk plugin door
(apps/desktop/src/contrib/runtime-loader.ts), found by a static+live audit of
the loader; none had an issue filed.

- A plugin whose module evaluation never settles (top-level `await` on a dead
  host) hung `import()` forever and, through the scan's sequential loop and
  its re-entrancy guard, froze every later plugin and all future scans until
  restart. `import()` now races a 10 s deadline; the plugin errors on its own
  row ("import timed out") and the scan continues.
- Timers and DOM listeners a plugin took out with bare globals survived
  disable and every hot-reload. `ctx.setTimeout` / `ctx.setInterval` /
  `ctx.addEventListener` are tracked with the plugin and torn down on
  unload; the SDK doc says bare globals are not.
- Two folders exporting one plugin id silently last-wins: the second
  disposed the first's registrations and each hot-reload flipped ownership.
  The first (folder-name sorted, so deterministic) owns the id; the later
  file errors on its own row ("duplicate id, already loaded from <path>").
- A save that no longer loads (syntax error, timeout, duplicate) left the old
  incarnation's contributions and activate handle live beside the error
  row, so the Plugins tab showed a broken file as "loaded" and could
  re-enable stale code. The previous incarnation is unloaded and dropped.

Tests: one invariant per fix in runtime-loader.test.ts, all red on base
(the hang case red by timing out).
@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

૮ >ﻌ< ა ci review

ran on 546d791 — fix(desktop): runtime plugin loader survives hung imports, l

debug info

CI timings

CI timings · View report · View job

Wall time 6m27s vs 5m44s (+12.5%). 6 job(s) slower, 8 faster,

  • Docs Site / docs-site-checks: -45.0s
  • OS-specific tests / Windows-only tests: +29.0s
  • Python tests / Run tests: +17.0s
  • Python tests / e2e: -9.0s
  • Python lints / Windows footguns (blocking): +9.0s

@teknium1 teknium1 added the ci-reviewed applied to manually approve dangerous changes label Sep 22, 2026
@teknium1
teknium1 merged commit e1a6679 into main Sep 22, 2026
33 checks passed
@teknium1
teknium1 deleted the fix/desktop-runtime-loader-hardening branch September 22, 2026 07:46
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) comp/plugins Plugin system and bundled plugins labels Sep 22, 2026
OutThisLife pushed a commit that referenced this pull request Sep 24, 2026
… on a local backend

The install dialog gated the package path on the agent install succeeding in
the same click (`agentInstalled && desktopHalfFromPackage`). A package already
on disk answers "Plugin '<name>' already exists. Use force reinstall" without
Force, so the retry fell through to installDesktopPlugin and cloned
desktop-plugins/<git-name>/ beside the package copy the app had already
materialised as desktop-plugins/<manifest-name>/. Two folders, one plugin id:
the on-disk duplicate from #100412 (since e1a6679 an error row instead of
a competing live instance, but still created by the install flow).

The package path now applies whenever the repo is a unified package, the
backend is local and the Agent box is ticked, whatever the agent install
returned. reconcileDesktopPlugins() is idempotent, so a retry touches
nothing; the desktop success toast is raised only when the agent half landed
or a copy was actually materialised, so a refused install shows the agent
error alone. A failed fresh install no longer leaves a standalone desktop
clone behind for the next successful install to duplicate.

Remote backends keep the separate clone (their plugins/ folder is not
readable from this machine); the Desktop-UI-only tick is unchanged.

Fixes #100412 (install-time half; runtime half landed in #118902).

Tests: two invariants in plugin-install-modal.test.tsx, the first red on
main (installDesktopPlugin called on the refused retry), the second guarding
the remote-backend clone.

(cherry picked from commit 9fbd3a9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-reviewed applied to manually approve dangerous changes comp/desktop Electron desktop app (apps/desktop/*) comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants