From da30d1a69ea36c6170f799d289bef7d8548a9685 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Thu, 20 Aug 2026 21:42:55 +0000 Subject: [PATCH] plugins: a reinstalled plugin was still being read from the deleted one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete jellyfin, install it again at a newer commit, connect a server — and no accounts were created, no mapping rows written, and no error anywhere. Every step reported success. The new checkout writes manifest.ts at exactly the path the old one occupied, and `await import(path)` returns the module it already has: ESM caches by resolved specifier, and the specifier did not change. So the platform kept obeying the manifest of a plugin that was no longer on disk. That manifest predates `userProvisioning`, so the plugin was judged not entitled to the account list, the sidecar was sent nothing, and provisioning skipped with "the platform has not sent the account list yet" — a report that goes into the response body nobody reads. It began working four minutes later for a reason that had nothing to do with the plugin: officer was restarted for an unrelated deploy, which emptied the module registry. `importFresh` keys the import on the file's mtime, so a file that changed is evaluated again and one that did not is still a cache hit — `discoverPlugins` runs on every sidecar registration, every mount and every plugins page load, so a Date.now() key would leak a module instance each time. Used for the manifest, the api router and the db schema. For manifests it is complete: every manifest imports one thing, `import type { PluginManifest }`, which is erased before it runs, so they are leaf modules. For the other two it narrows the window rather than closing it — only the named file is re-read, and its own static imports come back from the cache. A restart is still the only thing that guarantees a plugin's whole module graph is the one on disk, and that is written down in the file rather than left to be rediscovered. The test pins the failure and not the mechanism: one case proves a plain import keeps serving a manifest that is no longer on disk, the next proves importFresh sees the replacement, and a third proves an unchanged file is evaluated once. mtimes are set explicitly, because two writes inside one millisecond would otherwise decide the result. Co-Authored-By: Claude Opus 5 --- src/servers/plugins/discover.ts | 5 +- src/servers/plugins/import-fresh.test.ts | 74 ++++++++++++++++++++++++ src/servers/plugins/import-fresh.ts | 57 ++++++++++++++++++ src/servers/plugins/mount.ts | 5 +- src/servers/plugins/tables.ts | 5 +- 5 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 src/servers/plugins/import-fresh.test.ts create mode 100644 src/servers/plugins/import-fresh.ts diff --git a/src/servers/plugins/discover.ts b/src/servers/plugins/discover.ts index 2ca3ad92..72cf1011 100644 --- a/src/servers/plugins/discover.ts +++ b/src/servers/plugins/discover.ts @@ -2,6 +2,7 @@ import { existsSync, readdirSync, statSync } from 'node:fs'; import { join } from 'node:path'; import { PLATFORM_DIR } from '../data-path'; import { manifestProblems, type DiscoveredPlugin, type PluginManifest } from './manifest'; +import { importFresh } from './import-fresh'; // Finding plugins on disk. // @@ -85,7 +86,9 @@ export async function loadPlugin(dir: string, appName: string): Promise(manifestPath); manifest = module.manifest ?? null; } catch (err) { throw new Error(`${appName}: manifest.ts failed to load — ${err instanceof Error ? err.message : String(err)}`); diff --git a/src/servers/plugins/import-fresh.test.ts b/src/servers/plugins/import-fresh.test.ts new file mode 100644 index 00000000..4e96a544 --- /dev/null +++ b/src/servers/plugins/import-fresh.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, test, afterAll } from 'bun:test'; +import { mkdtempSync, rmSync, writeFileSync, utimesSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { importFresh } from './import-fresh'; + +// The failure this pins is not "an import cache exists" — it is that a plugin can be REPLACED on disk +// while the process keeps answering from the version that is gone. Deleting jellyfin and installing a +// newer commit left the platform reading the deleted manifest, so a field only the new one declared was +// invisible and provisioning silently did nothing. +// +// Each case uses its own file. Two imports of one path through different doors would interfere: whichever +// ran first would be the one that cached, so the test would be asserting its own ordering. + +const dir = mkdtempSync(join(tmpdir(), 'import-fresh-')); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +/** + * Replace a file's contents AND move its mtime on explicitly. + * + * Two writes can land in the same millisecond, which would leave the mtime unchanged and make + * `importFresh` correctly return the cached copy — a green-or-red test decided by how fast the disk is. + */ +const replace = (path: string, body: string, secondsLater: number) => { + writeFileSync(path, body); + const when = new Date(Date.now() + secondsLater * 1000); + utimesSync(path, when, when); +}; + +const seed = (file: string, body: string) => { + const path = join(dir, file); + writeFileSync(path, body); + return path; +}; + +describe('importFresh re-reads a file that changed', () => { + test('a plain import keeps serving a manifest that is no longer on disk — the bug', async () => { + const path = seed('plain.ts', `export const manifest = { userProvisioning: false, tag: 'OLD' };\n`); + + const before = (await import(path)) as { manifest: { tag: string } }; + expect(before.manifest.tag).toBe('OLD'); + + replace(path, `export const manifest = { userProvisioning: true, tag: 'NEW' };\n`, 10); + + const after = (await import(path)) as { manifest: { tag: string } }; + expect(after.manifest.tag).toBe('OLD'); // <- the deleted plugin's manifest, still being obeyed + }); + + test('importFresh sees the replacement', async () => { + const path = seed('fresh.ts', `export const manifest = { userProvisioning: false, tag: 'OLD' };\n`); + type M = { manifest: { userProvisioning: boolean; tag: string } }; + + expect((await importFresh(path)).manifest.tag).toBe('OLD'); + + replace(path, `export const manifest = { userProvisioning: true, tag: 'NEW' };\n`, 10); + + const fresh = await importFresh(path); + expect(fresh.manifest.tag).toBe('NEW'); + expect(fresh.manifest.userProvisioning).toBe(true); // the field whose absence lost a test cycle + }); + + test('an unchanged file is not re-evaluated — the mtime key has to be stable', async () => { + // Stability is what stops this leaking a module per call: `discoverPlugins` runs on every sidecar + // registration, every mount, and every load of the plugins page. + const path = seed('stable.ts', `export const value = { n: Math.random() };\n`); + const a = await importFresh<{ value: { n: number } }>(path); + const b = await importFresh<{ value: { n: number } }>(path); + expect(b.value).toBe(a.value); // same object identity — evaluated once + }); + + test('a missing file still fails as an import error, not as a stat error', async () => { + await expect(importFresh(join(dir, 'does-not-exist.ts'))).rejects.toThrow(); + }); +}); diff --git a/src/servers/plugins/import-fresh.ts b/src/servers/plugins/import-fresh.ts new file mode 100644 index 00000000..519ea251 --- /dev/null +++ b/src/servers/plugins/import-fresh.ts @@ -0,0 +1,57 @@ +import { statSync } from 'node:fs'; + +// Importing a plugin's own files, and re-reading them when they change on disk. +// +// ── The bug this exists for ── +// +// A plugin is deleted and installed again. The new checkout writes `manifest.ts` at exactly the path the +// old one occupied — and `await import(path)` returns the module it already has, because ESM caches by +// resolved specifier and the specifier did not change. The running server therefore keeps obeying the +// manifest of a plugin that is no longer on disk, until somebody restarts it. +// +// Found on 2026-08-20, and it had already cost a full test cycle. Jellyfin was deleted and reinstalled at +// a commit that declares `userProvisioning: true`; the platform, still holding the previous manifest from +// a commit that predates the field, decided the plugin was not entitled to the account list and sent +// nothing. The sidecar then had nothing to provision with, so connecting a server created no accounts and +// reported no error — every step it did run had succeeded. It began working four minutes later for a +// reason unrelated to the plugin: the server was restarted for an unrelated deploy, which emptied the +// module registry. +// +// That is the shape worth naming. Nothing failed. The install said installed, the sidecar said registered, +// the connection said connected, and the answer was silently computed from a deleted file. +// +// ── Keyed on mtime, not on a counter ── +// +// The query string makes a NEW specifier, so the module is evaluated again. It has to be derived from the +// file rather than from the clock: `?v=${Date.now()}` would re-evaluate on every discovery call — and +// `discoverPlugins` runs on registration, on mount, on every plugins page load — leaking a module instance +// each time. An mtime is stable while the file is, so the common case stays a cache hit and only a file +// that actually changed pays for a re-read. +// +// ── What this does NOT fix ── +// +// Only the file named here is re-read. Its own static imports resolve to the same specifiers they did +// before and come back from the cache, so a plugin whose entry file is unchanged but whose helper module +// changed is still stale in this process. +// +// For manifests that limitation is theoretical: every manifest on this machine imports exactly one thing, +// `import type { PluginManifest }`, which is erased before it runs. They are leaf modules, so re-reading +// one is complete. For a plugin's api/router.ts and db/schema.ts it is a real limit, and the honest +// summary is that this narrows the window rather than closing it — a restart is still the only thing that +// guarantees a plugin's whole module graph is the one on disk. + +/** + * `await import(path)`, except that a file rewritten since the last import is read again. + * + * Falls back to a plain import when the file cannot be stat'd — the import is about to produce a much + * better error than anything this could say about it. + */ +export async function importFresh(path: string): Promise { + let stamp: string; + try { + stamp = String(statSync(path).mtimeMs); + } catch { + return (await import(path)) as T; + } + return (await import(`${path}?m=${stamp}`)) as T; +} diff --git a/src/servers/plugins/mount.ts b/src/servers/plugins/mount.ts index 8f96e67a..619d82b6 100644 --- a/src/servers/plugins/mount.ts +++ b/src/servers/plugins/mount.ts @@ -16,6 +16,7 @@ import { mountPrefix, type DiscoveredPlugin } from './manifest'; import { generatePluginsModule, rebuildFrontend } from './generate'; import { iconUrl } from '../app-store/assets'; import { publishAssets } from '../app-store/assets'; +import { importFresh } from './import-fresh'; // Turning what is on disk plus what is in the database into a mounted application. // @@ -71,7 +72,9 @@ export async function snapshotPlugins(): Promise { export async function loadPluginRouter(plugin: DiscoveredPlugin): Promise { if (!plugin.api) return null; - const module = (await import(plugin.api)) as { router?: unknown; publicRouter?: unknown }; + // Re-read when the file has changed, so remounting after an update serves the new routes rather than + // the ones this process happened to import first. Only this file — see import-fresh.ts. + const module = await importFresh<{ router?: unknown; publicRouter?: unknown }>(plugin.api); const router = module.router; if (!router || typeof (router as { fetch?: unknown }).fetch !== 'function') { throw new Error(`${plugin.appName}: api/router.ts must export \`router\` (a Hono router)`); diff --git a/src/servers/plugins/tables.ts b/src/servers/plugins/tables.ts index 9dfe265c..bae9dba7 100644 --- a/src/servers/plugins/tables.ts +++ b/src/servers/plugins/tables.ts @@ -2,6 +2,7 @@ import { getTableName, is, sql } from 'drizzle-orm'; import { PgTable } from 'drizzle-orm/pg-core'; import { db } from 'officerdb/db'; import type { DiscoveredPlugin } from './manifest'; +import { importFresh } from './import-fresh'; // Which tables a plugin owns, and whether they are actually there. // @@ -23,7 +24,9 @@ import type { DiscoveredPlugin } from './manifest'; export async function declaredTables(plugin: DiscoveredPlugin): Promise<{ tables: string[]; refused: string[] }> { if (!plugin.schema) return { tables: [], refused: [] }; - const module = (await import(plugin.schema)) as Record; + // Deleting a plugin reads this to know which tables it owns. A stale copy would name the wrong ones, + // which for a delete-with-data means dropping the wrong tables or missing some. + const module = await importFresh>(plugin.schema); const prefix = `${plugin.appName.replace(/-/g, '_')}_`; const tables: string[] = [];