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[] = [];