From d8debaa449c40bf2522433f3841dde6403c9da77 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 19 Aug 2026 11:26:54 -0400 Subject: [PATCH] fix(tui): stabilize plugin reload generations (#43447) --- packages/tui/src/plugin/context.tsx | 132 ++++++++++--------- packages/tui/src/plugin/discovery.ts | 16 +-- packages/tui/test/plugin-hot-reload.test.tsx | 16 +-- 3 files changed, 83 insertions(+), 81 deletions(-) diff --git a/packages/tui/src/plugin/context.tsx b/packages/tui/src/plugin/context.tsx index f1ef9a388a..b5cff6f32a 100644 --- a/packages/tui/src/plugin/context.tsx +++ b/packages/tui/src/plugin/context.tsx @@ -14,9 +14,10 @@ import { type ParentProps, } from "solid-js" import path from "path" -import { stat } from "fs/promises" +import { readFile, stat } from "fs/promises" import { fileURLToPath, pathToFileURL } from "url" import type { Page } from "@opencode-ai/plugin/tui/context" +import { Hash } from "@opencode-ai/util/hash" import { resolveSlots, type Claim } from "./structure" import { createStore, produce, reconcile as reconcileStore, unwrap } from "solid-js/store" import { isDeepEqual } from "remeda" @@ -78,6 +79,7 @@ type Registration = { type Desired = Pick & { enabled: boolean } const PluginContext = createContext() +let sourceVersion = Date.now() export function combineMarkdownRenderers( sources: ReadonlyArray>>, @@ -107,6 +109,18 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d states: [] as ReadonlyArray, registrations: {} as Record, }) + // One save can emit several watch events. Remember setup failures so those + // events do not repeatedly tear down and restore the last good generation. + const setupFailures = new Map() + const sourceVersions = new Map() + const sourceGeneration = async (entrypoint: string) => { + const digest = Hash.sha256(await readFile(new URL(entrypoint))) + const previous = sourceVersions.get(entrypoint) + if (previous?.digest === digest) return previous.generation + const generation = ++sourceVersion + sourceVersions.set(entrypoint, { digest, generation }) + return generation + } const markdown = createMemo(() => combineMarkdownRenderers( Object.values(store.registrations).flatMap((registration) => @@ -114,15 +128,18 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d ), ), ) + const clearContributions = (id: string) => { + setStore("registrations", id, "routes", reconcileStore({})) + setStore("registrations", id, "slots", reconcileStore({})) + setStore("registrations", id, "markdown", reconcileStore({})) + } const activate = async (id: string) => { const item = store.registrations[id] if (!item) return false await deactivate(id) batch(() => { - setStore("registrations", id, "routes", reconcileStore({})) - setStore("registrations", id, "slots", reconcileStore({})) - setStore("registrations", id, "markdown", reconcileStore({})) + clearContributions(id) setStore("registrations", id, "cleanups", []) }) const owned: Dispose[] = [] @@ -150,12 +167,17 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d }, }) const cleanup = await setup(item.plugin, context, owned).catch((error) => { - setStore("registrations", id, "routes", reconcileStore({})) - setStore("registrations", id, "slots", reconcileStore({})) - setStore("registrations", id, "markdown", reconcileStore({})) + clearContributions(id) + if (item.target) + setupFailures.set(item.target, { + version: item.version, + options: snapshotOptions(item.options), + error: errorMessage(error), + }) throw error }) if (cleanup) owned.push(async () => cleanup()) + if (item.target && sameGeneration(setupFailures.get(item.target), item)) setupFailures.delete(item.target) batch(() => { setStore("registrations", id, "cleanups", owned) setStore("registrations", id, "active", true) @@ -179,9 +201,7 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d await disposeAll(cleanups).finally(() => batch(() => { if (store.registrations[id]) { - setStore("registrations", id, "routes", reconcileStore({})) - setStore("registrations", id, "slots", reconcileStore({})) - setStore("registrations", id, "markdown", reconcileStore({})) + clearContributions(id) } setStore("states", (items) => items.map((state) => @@ -239,9 +259,6 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d // Package resolution failures would otherwise retry a full npm install on // every watch event; remember them until the configuration changes. const npmFailures = new Map() - // A source that imports but fails setup must not tear down and restore its - // last-good generation again for every event in the same filesystem burst. - const setupFailures = new Map() const reconcile = async () => { await Promise.all(props.directories.map(watcher.wait)) const entries = [ @@ -278,10 +295,12 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d const memo = local ? undefined : npmFailures.get(target) const resolved = memo ? { status: "failed" as const, error: memo } - : await resolvePlugin(target, local, options, previous, props.packages, source.install).catch((error) => ({ - status: "failed" as const, - error: errorMessage(error), - })) + : await resolvePlugin(target, local, options, previous, props.packages, source.install, sourceGeneration).catch( + (error) => ({ + status: "failed" as const, + error: errorMessage(error), + }), + ) if (resolved.status === "unsupported") { if (source.server) continue failures.push({ target, status: "unsupported" }) @@ -295,37 +314,18 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d status: "failed", error: previous?.active ? `${resolved.error} (previous version still active)` : resolved.error, }) - if (previous) - desired.set(previous.plugin.id, { - plugin: previous.plugin, - source: previous.source, - target, - version: previous.version, - options: previous.options, - enabled: previous.active, - }) + if (previous) desired.set(previous.plugin.id, toDesired(previous)) continue } const setupFailure = setupFailures.get(target) - if ( - previous && - setupFailure?.version === resolved.version && - sameOptions(setupFailure.options, options) - ) { + if (setupFailure && sameGeneration(setupFailure, { version: resolved.version, options }) && previous) { failures.push({ target, id: previous.plugin.id, status: "failed", error: previous.active ? `${setupFailure.error} (previous version still active)` : setupFailure.error, }) - desired.set(previous.plugin.id, { - plugin: previous.plugin, - source: previous.source, - target, - version: previous.version, - options: previous.options, - enabled: previous.active, - }) + desired.set(previous.plugin.id, toDesired(previous)) continue } setupFailures.delete(target) @@ -361,11 +361,7 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d // enabled derives from config directives alone, so config wins over // manual dialog toggles on every reconcile — the same semantics // config saves had before hot reload existed, just more frequent. - return ( - registration.version !== item.version || - !sameOptions(registration.options, item.options) || - registration.active !== item.enabled - ) + return !sameGeneration(registration, item) || registration.active !== item.enabled }) // Swap: cleanup failures surface as a toast, never propagate, so one @@ -374,22 +370,11 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d for (const id of changed) { const item = desired.get(id)! const registration = store.registrations[id] - const replaced = - !registration || registration.version !== item.version || !sameOptions(registration.options, item.options) + const replaced = !registration || !sameGeneration(registration, item) // Snapshot the running version before it is overwritten: an import // failure keeps last-good in the resolve phase, and a setup failure // must not cost the previous version either. - const fallback: Desired | undefined = - replaced && registration - ? { - plugin: registration.plugin, - source: registration.source, - target: registration.target, - version: registration.version, - options: registration.options, - enabled: registration.active, - } - : undefined + const fallback = replaced && registration ? toDesired(registration) : undefined if (replaced) { if (registration) await deactivateNoisily(id) // In-place replacement keeps the registration's key position, which @@ -403,8 +388,6 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d const error = await activate(id).then(() => undefined, errorMessage) if (!error) continue errors.set(id, error) - if (item.target) - setupFailures.set(item.target, { version: item.version, options: item.options, error }) if (!fallback) continue setStore("registrations", id, toRegistration(fallback)) if (!fallback.enabled) continue @@ -592,6 +575,7 @@ async function resolvePlugin( previous: Registration | undefined, packages: PackageResolver, install: boolean, + sourceGeneration: (entrypoint: string) => Promise, ) { // Package entrypoints never change within a session, so a loaded previous // version needs no re-resolution (which could otherwise hit npm). @@ -599,9 +583,9 @@ async function resolvePlugin( return { status: "unchanged" as const, plugin: previous.plugin, version: previous.version } const entrypoint = local ? await resolveLocal(local) : await packages.resolve(spec, install) if (!entrypoint) return { status: "unsupported" as const } - // The cache-busted specifier doubles as the version: unique per entrypoint - // and mtime, so equal versions mean an identical module. - const version = local ? freshSpecifier(entrypoint, (await stat(new URL(entrypoint))).mtimeMs) : entrypoint + // Content remains stable across the several mtimes one save may expose to + // filesystem watchers, while the generation keeps reverted modules fresh. + const version = local ? freshSpecifier(entrypoint, await sourceGeneration(entrypoint)) : entrypoint if (previous && previous.version === version && sameOptions(previous.options, options)) return { status: "unchanged" as const, plugin: previous.plugin, version } const mod: { readonly default?: unknown } = await import(version) @@ -615,7 +599,7 @@ function toRegistration(item: Desired): Registration { source: item.source, target: item.target, version: item.version, - options: item.options, + options: snapshotOptions(item.options), active: false, routes: {}, slots: {}, @@ -624,10 +608,32 @@ function toRegistration(item: Desired): Registration { } } +function toDesired(item: Registration): Desired { + return { + plugin: item.plugin, + source: item.source, + target: item.target, + version: item.version, + options: item.options, + enabled: item.active, + } +} + function sameOptions(a: Registration["options"], b: Registration["options"]) { return isDeepEqual(a ?? null, b ?? null) } +function sameGeneration( + a: Pick | undefined, + b: Pick, +) { + return a?.version === b.version && sameOptions(a.options, b.options) +} + +function snapshotOptions(options: Registration["options"]) { + return options ? structuredClone(unwrap(options)) : undefined +} + async function resolveLocal(url: URL) { const info = await stat(url) if (info.isFile()) return url.href diff --git a/packages/tui/src/plugin/discovery.ts b/packages/tui/src/plugin/discovery.ts index 45f5497074..d9d57fd6d1 100644 --- a/packages/tui/src/plugin/discovery.ts +++ b/packages/tui/src/plugin/discovery.ts @@ -45,15 +45,13 @@ export function localSource(spec: string, directory: string) { return undefined } -// Key local plugin imports by mtime so edited sources re-import fresh instead -// of hitting the ESM cache. Bun ignores query params when caching file:// URL -// imports, so bust with a plain path there; Node keys its cache on the full -// URL. Mirrors the core plugin supervisor's loader. -// The mtime is truncated to whole milliseconds: a fractional mtimeMs puts a -// dot in the query, and Bun's compiled binaries then skip runtime plugin -// hooks for the import, breaking JSX/solid rewriting for external plugins. -export function freshSpecifier(entrypoint: string, mtime: number) { - const version = Math.trunc(mtime) +// Key local plugin imports by a numeric source version so edited sources +// re-import fresh instead of hitting the ESM cache. Bun ignores query params +// when caching file:// URL imports, so bust with a plain path there; Node keys +// its cache on the full URL. Fractional versions break Bun's runtime JSX/solid +// plugin hooks, so always truncate them. +export function freshSpecifier(entrypoint: string, sourceVersion: number) { + const version = Math.trunc(sourceVersion) if (typeof Bun !== "undefined") return `${fileURLToPath(entrypoint).replaceAll("\\", "/")}?mtime=${version}` return `${entrypoint}?mtime=${version}` } diff --git a/packages/tui/test/plugin-hot-reload.test.tsx b/packages/tui/test/plugin-hot-reload.test.tsx index 01e3e8c40f..196fdadf82 100644 --- a/packages/tui/test/plugin-hot-reload.test.tsx +++ b/packages/tui/test/plugin-hot-reload.test.tsx @@ -285,26 +285,24 @@ test("a save whose setup throws restores the previous version", async () => { // The module imports fine but its setup throws — unlike an import failure, // the swap has already torn down a1, so keep-last-good means restoring it. - await writeFile( - source, - ` + const broken = ` export default { id: "test.a", setup: async () => { throw new Error("setup boom") }, } -`, - ) +` + await writeFile(source, broken) expect(await until(read, (value) => value === "a1:setup\na1:cleanup\na1:setup\n")).toBe( "a1:setup\na1:cleanup\na1:setup\n", ) - // A later reconcile must not retry the same broken generation. + // Duplicate notifications for unchanged contents must not retry the broken + // generation and cycle the restored plugin again. + await writeFile(source, broken) await writeFile(sourceB, lifecycleSource(markerB, "test.b", "b2")) - expect(await until(readB, (value) => value?.includes("b2:setup") ?? false)).toBe( - "b1:setup\nb1:cleanup\nb2:setup\n", - ) + expect(await until(readB, (value) => value?.includes("b2:setup") ?? false)).toBe("b1:setup\nb1:cleanup\nb2:setup\n") expect(await read()).toBe("a1:setup\na1:cleanup\na1:setup\n") // Fixing the file swaps out the restored version normally.