fix(tui): stabilize plugin reload generations (#43447)

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<Registration, "plugin" | "source" | "target" | "version" | "options"> & { enabled: boolean }
 
 const PluginContext = createContext<Value>()
+let sourceVersion = Date.now()
 
 export function combineMarkdownRenderers(
   sources: ReadonlyArray<Readonly<Record<string, MarkdownCodeBlockRenderer>>>,
@@ -107,6 +109,18 @@ export function PluginProvider(props: ParentProps<{ packages: PackageResolver; d
     states: [] as ReadonlyArray<State>,
     registrations: {} as Record<string, Registration>,
   })
+  // 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<string, { version: string; options: Registration["options"]; error: string }>()
+  const sourceVersions = new Map<string, { digest: string; generation: number }>()
+  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<string, string>()
-  // 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<string, { version: string; options: Desired["options"]; error: string }>()
   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<number>,
 ) {
   // 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<Registration, "version" | "options"> | undefined,
+  b: Pick<Registration, "version" | "options">,
+) {
+  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

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}`
 }

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.