plugins: a reinstalled plugin was still being read from the deleted one

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 <noreply@anthropic.com>
This commit is contained in:
2026-08-20 21:42:55 +00:00
co-authored by Claude Opus 5
parent 16dd67a3ca
commit da30d1a69e
5 changed files with 143 additions and 3 deletions
+4 -1
View File
@@ -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<Discover
let manifest: PluginManifest | null = null;
try {
const module = (await import(manifestPath)) as { manifest?: PluginManifest };
// `importFresh`, not `import` — a reinstalled plugin writes a new manifest at the old path, and a
// plain import would hand back the deleted plugin's. See import-fresh.ts for what that cost.
const module = await importFresh<{ manifest?: PluginManifest }>(manifestPath);
manifest = module.manifest ?? null;
} catch (err) {
throw new Error(`${appName}: manifest.ts failed to load — ${err instanceof Error ? err.message : String(err)}`);
+74
View File
@@ -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<M>(path)).manifest.tag).toBe('OLD');
replace(path, `export const manifest = { userProvisioning: true, tag: 'NEW' };\n`, 10);
const fresh = await importFresh<M>(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();
});
});
+57
View File
@@ -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<T>(path: string): Promise<T> {
let stamp: string;
try {
stamp = String(statSync(path).mtimeMs);
} catch {
return (await import(path)) as T;
}
return (await import(`${path}?m=${stamp}`)) as T;
}
+4 -1
View File
@@ -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<PluginsSnapshot> {
export async function loadPluginRouter(plugin: DiscoveredPlugin): Promise<MountedPlugin | null> {
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)`);
+4 -1
View File
@@ -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<string, unknown>;
// 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<Record<string, unknown>>(plugin.schema);
const prefix = `${plugin.appName.replace(/-/g, '_')}_`;
const tables: string[] = [];