diff --git a/doc/plugins/LOCAL_PLUGIN_DEVELOPMENT.md b/doc/plugins/LOCAL_PLUGIN_DEVELOPMENT.md index b3a489815da..3bb95399f77 100644 --- a/doc/plugins/LOCAL_PLUGIN_DEVELOPMENT.md +++ b/doc/plugins/LOCAL_PLUGIN_DEVELOPMENT.md @@ -181,6 +181,15 @@ You can, but you usually should not. The control-plane host is shared and may be Paperclip watches the on-disk plugin package after a local install. The watcher targets the runtime entrypoints declared in the package's `paperclipPlugin` field (`dist/manifest.js`, `dist/worker.js`, `dist/ui/`). +When the lifecycle registers a different local package path for the same plugin, +the watcher switches to that directory. Pending changes and late events from the +previous directory do not restart the worker. If the new directory is unavailable +or the plugin no longer has a local path, the old watcher stops. +Only the latest pending path lookup can register a watcher. Manual registration, +unwatching, and shutdown cancel earlier pending lookups. +A watcher error stops the failed watcher while allowing a pending replacement +path lookup to finish. + What that means in practice: - **Worker code:** save a `.ts` file → esbuild rewrites `dist/worker.js` → Paperclip debounces ~500ms and restarts the plugin worker. The next worker call uses the new code. There is no in-process hot module replacement for worker code; it is a worker restart. diff --git a/server/src/__tests__/plugin-dev-watcher.test.ts b/server/src/__tests__/plugin-dev-watcher.test.ts index c6ffa74257d..c066e8908d1 100644 --- a/server/src/__tests__/plugin-dev-watcher.test.ts +++ b/server/src/__tests__/plugin-dev-watcher.test.ts @@ -74,6 +74,12 @@ function installMockFsWatcher() { return { fakeWatcher, handlers }; } +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +} + describe("resolvePluginWatchTargets", () => { it("watches package metadata plus concrete declared runtime files", () => { const pluginDir = makeTempPluginDir(); @@ -145,4 +151,211 @@ describe("createPluginDevWatcher", () => { devWatcher.close(); }); + + it("replaces the watcher when a loaded plugin moves to another local path", async () => { + vi.useFakeTimers(); + const firstDir = makeTempPluginDir(); + const secondDir = makeTempPluginDir(); + writePluginPackage(firstDir); + writePluginPackage(secondDir); + const first = installMockFsWatcher(); + const lifecycle = createLifecycle(); + let packagePath = firstDir; + const devWatcher = createPluginDevWatcher(lifecycle as never, async () => packagePath); + devWatcher.watch("plugin-1", firstDir); + first.handlers.all?.("change", path.join(firstDir, "dist", "worker.js")); + + const second = installMockFsWatcher(); + packagePath = secondDir; + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + await Promise.resolve(); + + expect(first.fakeWatcher.close).toHaveBeenCalledTimes(1); + expect(chokidarMock.watch).toHaveBeenCalledTimes(2); + expect(chokidarMock.watch.mock.calls[1]?.[0]).toContain(path.join(secondDir, "dist", "worker.js")); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).not.toHaveBeenCalled(); + + // Closing a watcher is asynchronous. Late callbacks from the old watcher + // must neither restart the worker nor close its replacement. + first.handlers.all?.("change", path.join(firstDir, "dist", "worker.js")); + first.handlers.error?.(new Error("old directory removed")); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).not.toHaveBeenCalled(); + expect(second.fakeWatcher.close).not.toHaveBeenCalled(); + + second.handlers.all?.("change", path.join(secondDir, "dist", "worker.js")); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).toHaveBeenCalledTimes(1); + expect(lifecycle.restartWorker).toHaveBeenCalledWith("plugin-1"); + devWatcher.close(); + expect(second.fakeWatcher.close).toHaveBeenCalledTimes(1); + }); + + it("keeps one watcher for repeated registrations of the same resolved path", () => { + const pluginDir = makeTempPluginDir(); + writePluginPackage(pluginDir); + const { fakeWatcher } = installMockFsWatcher(); + const devWatcher = createPluginDevWatcher(createLifecycle() as never); + + devWatcher.watch("plugin-1", pluginDir); + devWatcher.watch("plugin-1", path.join(pluginDir, "dist", "..")); + + expect(chokidarMock.watch).toHaveBeenCalledTimes(1); + expect(fakeWatcher.close).not.toHaveBeenCalled(); + devWatcher.close(); + }); + + it("stops watching the old path when the replacement directory is unavailable", async () => { + vi.useFakeTimers(); + const pluginDir = makeTempPluginDir(); + writePluginPackage(pluginDir); + const { fakeWatcher, handlers } = installMockFsWatcher(); + const lifecycle = createLifecycle(); + const devWatcher = createPluginDevWatcher(lifecycle as never); + devWatcher.watch("plugin-1", pluginDir); + handlers.all?.("change", path.join(pluginDir, "dist", "worker.js")); + + devWatcher.watch("plugin-1", path.join(pluginDir, "missing")); + + expect(fakeWatcher.close).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).not.toHaveBeenCalled(); + expect(chokidarMock.watch).toHaveBeenCalledTimes(1); + devWatcher.close(); + }); + + it("stops watching a loaded plugin that no longer has a local package path", async () => { + vi.useFakeTimers(); + const pluginDir = makeTempPluginDir(); + writePluginPackage(pluginDir); + const { fakeWatcher, handlers } = installMockFsWatcher(); + const lifecycle = createLifecycle(); + const devWatcher = createPluginDevWatcher(lifecycle as never, async () => null); + devWatcher.watch("plugin-1", pluginDir); + handlers.all?.("change", path.join(pluginDir, "dist", "worker.js")); + + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + await Promise.resolve(); + + expect(fakeWatcher.close).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).not.toHaveBeenCalled(); + devWatcher.close(); + }); + + it.each(["new path", "no local path"])("ignores an older path lookup after a newer %s registration", async (replacement) => { + const firstDir = makeTempPluginDir(); + const secondDir = makeTempPluginDir(); + writePluginPackage(firstDir); + writePluginPackage(secondDir); + const first = installMockFsWatcher(); + const lifecycle = createLifecycle(); + const oldLookup = deferred(); + const newLookup = deferred(); + const resolver = vi.fn().mockReturnValueOnce(oldLookup.promise).mockReturnValueOnce(newLookup.promise); + const devWatcher = createPluginDevWatcher(lifecycle as never, resolver); + devWatcher.watch("plugin-1", firstDir); + + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + const second = installMockFsWatcher(); + newLookup.resolve(replacement === "new path" ? secondDir : null); + await Promise.resolve(); + expect(first.fakeWatcher.close).toHaveBeenCalledTimes(1); + const expectedWatchCount = replacement === "new path" ? 2 : 1; + expect(chokidarMock.watch).toHaveBeenCalledTimes(expectedWatchCount); + + oldLookup.resolve(firstDir); + await Promise.resolve(); + + expect(chokidarMock.watch).toHaveBeenCalledTimes(expectedWatchCount); + expect(second.fakeWatcher.close).not.toHaveBeenCalled(); + devWatcher.close(); + }); + + it.each(["watch", "unwatch", "close"])("invalidates pending path lookups on a manual %s", async (action) => { + const firstDir = makeTempPluginDir(); + const secondDir = makeTempPluginDir(); + writePluginPackage(firstDir); + writePluginPackage(secondDir); + installMockFsWatcher(); + const lifecycle = createLifecycle(); + const lookup = deferred(); + const devWatcher = createPluginDevWatcher(lifecycle as never, () => lookup.promise); + if (action !== "close") devWatcher.watch("plugin-1", firstDir); + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + + if (action === "watch") devWatcher.watch("plugin-1", firstDir); + if (action === "unwatch") devWatcher.unwatch("plugin-1"); + if (action === "close") devWatcher.close(); + lookup.resolve(secondDir); + await Promise.resolve(); + await Promise.resolve(); + + expect(chokidarMock.watch).toHaveBeenCalledTimes(action === "close" ? 0 : 1); + devWatcher.close(); + }); + + it("does not reuse a pending lookup identity after unwatching", async () => { + const pluginDir = makeTempPluginDir(); + writePluginPackage(pluginDir); + installMockFsWatcher(); + const lifecycle = createLifecycle(); + const oldLookup = deferred(); + const newLookup = deferred(); + const resolver = vi.fn().mockReturnValueOnce(oldLookup.promise).mockReturnValueOnce(newLookup.promise); + const devWatcher = createPluginDevWatcher(lifecycle as never, resolver); + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + devWatcher.unwatch("plugin-1"); + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + + oldLookup.resolve(pluginDir); + await Promise.resolve(); + expect(chokidarMock.watch).not.toHaveBeenCalled(); + newLookup.resolve(pluginDir); + await Promise.resolve(); + expect(chokidarMock.watch).toHaveBeenCalledTimes(1); + devWatcher.close(); + }); + + it.each(["current", "replaced"])("keeps a pending replacement lookup when the %s watcher reports an error", async (state) => { + vi.useFakeTimers(); + const firstDir = makeTempPluginDir(); + const secondDir = makeTempPluginDir(); + const replacementDir = makeTempPluginDir(); + [firstDir, secondDir, replacementDir].forEach(writePluginPackage); + const first = installMockFsWatcher(); + const lifecycle = createLifecycle(); + const lookup = deferred(); + const devWatcher = createPluginDevWatcher(lifecycle as never, () => lookup.promise); + devWatcher.watch("plugin-1", firstDir); + let active = first; + if (state === "replaced") { + active = installMockFsWatcher(); + devWatcher.watch("plugin-1", secondDir); + } + first.handlers.all?.("change", path.join(firstDir, "dist", "worker.js")); + lifecycle.emit("plugin.loaded", { pluginId: "plugin-1" }); + + first.handlers.error?.(new Error("previous directory removed")); + + expect(first.fakeWatcher.close).toHaveBeenCalledTimes(1); + if (state === "replaced") expect(active.fakeWatcher.close).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).not.toHaveBeenCalled(); + const replacement = installMockFsWatcher(); + lookup.resolve(replacementDir); + await Promise.resolve(); + expect(chokidarMock.watch).toHaveBeenCalledTimes(state === "current" ? 2 : 3); + expect(chokidarMock.watch.mock.lastCall?.[0]).toContain(path.join(replacementDir, "dist", "worker.js")); + + first.handlers.error?.(new Error("late old watcher error")); + expect(replacement.fakeWatcher.close).not.toHaveBeenCalled(); + replacement.handlers.all?.("change", path.join(replacementDir, "dist", "worker.js")); + await vi.advanceTimersByTimeAsync(500); + expect(lifecycle.restartWorker).toHaveBeenCalledTimes(1); + expect(lifecycle.restartWorker).toHaveBeenCalledWith("plugin-1"); + devWatcher.close(); + }); }); diff --git a/server/src/services/plugin-dev-watcher.ts b/server/src/services/plugin-dev-watcher.ts index ceba5d4e181..fbfa4a5425f 100644 --- a/server/src/services/plugin-dev-watcher.ts +++ b/server/src/services/plugin-dev-watcher.ts @@ -161,8 +161,11 @@ export function createPluginDevWatcher( resolvePluginPackagePath?: ResolvePluginPackagePath, fsDeps?: PluginDevWatcherFsDeps, ): PluginDevWatcher { - const watchers = new Map(); + const watchers = new Map(); const debounceTimers = new Map>(); + const pendingRegistrations = new Map(); + // Never reuse an identity after unwatching: an older lookup may still finish. + let registrationGeneration = 0; const fileExists = fsDeps?.existsSync ?? existsSync; log.info( { resolvesInstalledPlugins: Boolean(resolvePluginPackagePath) }, @@ -170,10 +173,13 @@ export function createPluginDevWatcher( ); function watchPlugin(pluginId: string, packagePath: string): void { - // Don't double-watch - if (watchers.has(pluginId)) return; - + pendingRegistrations.delete(pluginId); const absPath = path.resolve(packagePath); + // A reinstall can move a plugin to another directory. Keep an existing + // watcher only while it still targets the installed package path. + if (watchers.get(pluginId)?.packagePath === absPath) return; + unwatchPlugin(pluginId); + if (!fileExists(absPath)) { log.warn( { pluginId, packagePath: absPath }, @@ -208,6 +214,7 @@ export function createPluginDevWatcher( ); watcher.on("all", (_eventName, changedPath) => { + if (watchers.get(pluginId)?.watcher !== watcher) return; const relativePath = path.relative(absPath, changedPath); if (shouldIgnorePath(relativePath)) return; @@ -217,6 +224,7 @@ export function createPluginDevWatcher( debounceTimers.set( pluginId, setTimeout(() => { + if (watchers.get(pluginId)?.watcher !== watcher) return; debounceTimers.delete(pluginId); log.info( { pluginId, changedFile: relativePath || path.basename(changedPath) }, @@ -237,6 +245,7 @@ export function createPluginDevWatcher( }); watcher.on("error", (err) => { + if (watchers.get(pluginId)?.watcher !== watcher) return; log.warn( { pluginId, @@ -245,10 +254,11 @@ export function createPluginDevWatcher( }, "plugin-dev-watcher: watcher error, stopping watch for this plugin", ); - unwatchPlugin(pluginId); + // Stop this failed watcher without cancelling a pending replacement. + stopWatchingPlugin(pluginId); }); - watchers.set(pluginId, watcher); + watchers.set(pluginId, { watcher, packagePath: absPath }); log.info( { pluginId, @@ -272,10 +282,10 @@ export function createPluginDevWatcher( } } - function unwatchPlugin(pluginId: string): void { + function stopWatchingPlugin(pluginId: string): void { const pluginWatcher = watchers.get(pluginId); if (pluginWatcher) { - void pluginWatcher.close(); + void pluginWatcher.watcher.close(); watchers.delete(pluginId); } const timer = debounceTimers.get(pluginId); @@ -285,7 +295,13 @@ export function createPluginDevWatcher( } } + function unwatchPlugin(pluginId: string): void { + pendingRegistrations.delete(pluginId); + stopWatchingPlugin(pluginId); + } + function close(): void { + pendingRegistrations.clear(); lifecycle.off("plugin.loaded", handlePluginLoaded); lifecycle.off("plugin.enabled", handlePluginEnabled); lifecycle.off("plugin.disabled", handlePluginDisabled); @@ -305,9 +321,13 @@ export function createPluginDevWatcher( return; } + const generation = ++registrationGeneration; + pendingRegistrations.set(pluginId, generation); try { const packagePath = await resolvePluginPackagePath(pluginId); + if (pendingRegistrations.get(pluginId) !== generation) return; if (!packagePath) { + unwatchPlugin(pluginId); log.debug( { pluginId }, "plugin-dev-watcher: plugin is not a local-path install, skipping watch", @@ -316,6 +336,7 @@ export function createPluginDevWatcher( } watchPlugin(pluginId, packagePath); } catch (err) { + if (pendingRegistrations.get(pluginId) !== generation) return; log.warn( { pluginId, @@ -323,6 +344,10 @@ export function createPluginDevWatcher( }, "plugin-dev-watcher: failed to resolve plugin package path", ); + } finally { + if (pendingRegistrations.get(pluginId) === generation) { + pendingRegistrations.delete(pluginId); + } } }