fix(plugins): follow local package path changes in dev watcher (#15247)

## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - Local plugin development uses file changes to restart the plugin
worker.
> - A plugin can move to a different local package directory.
> - The watcher rejected every later registration for the same plugin
ID.
> - This pull request replaces the watcher when the resolved package
path changes.
> - The benefit is that current plugin files trigger reloads and
previous files stop doing so.

## Linked Issues or Issue Description

Refs: #14955. This change addresses its second observation: the watcher
keeps the previous local directory after its package path changes. The
issue also tracks stored configuration replay as a separate problem.

Related PRs: #11336 shares the lifecycle manager across routes and the
dispatcher. #14381 controls when production file watching is enabled.
#10100 orders configuration delivery. Those changes address other parts
of plugin reload behavior.

## What Changed

- Store the resolved package path with each watcher. Keep repeated
registrations of the same path as a no-op. Replace the watcher for a
different path.
- Cancel pending changes from the previous directory. Ignore late
change, error, and timer callbacks from a replaced watcher.
- Apply only the latest pending directory lookup. Manual registration,
unwatching, and shutdown invalidate earlier lookups.
- Stop a failed watcher without cancelling its pending replacement
lookup. Explicit unwatching, disable, unload, and shutdown still cancel
pending registrations.
- Stop the old watcher when the new directory is unavailable or the
plugin no longer has a local package path.
- Document the path-change behavior and add twelve regression cases.

## Verification

- `pnpm exec vitest run server/src/__tests__/plugin-dev-watcher.test.ts
server/src/__tests__/plugin-lifecycle-restart.test.ts`: 18 tests passed.
Regressions cover path changes, stale callbacks, lookup order, explicit
cancellation, and watcher errors during a pending replacement. The
current-watcher error case fails before the stop-versus-cancel fix; the
stale-watcher error control passes.
- A local smoke check used real Chokidar watchers and two real temporary
plugin directories. It confirmed that a pending old change is cancelled,
later old-directory changes are ignored, and a new-directory change
triggers one restart.
- `pnpm -r typecheck` passed on the path and lookup guards. The final
watcher-error fix passed `pnpm --filter @paperclipai/server exec tsc
--noEmit`.
- `pnpm build` passed on the initial path-change fix. The final source
passed `pnpm --filter @paperclipai/server build`.
- `git diff --check`: passed.
- Greptile reviewed `bef3182` at 5/5 with no new findings. The
watcher-error review thread is resolved. CI is still running.
- The full `pnpm test:run` suite was not repeated for this watcher-only
change. The affected suites ran locally. The broad test shards will run
in CI.

## Risks

- Low risk. The watcher follows the current registered path. An
unavailable replacement stops watching until another valid registration
arrives.
- Closing a filesystem watcher is asynchronous. Callback identity checks
prevent an old watcher from acting on its replacement.
- No database migration, API shape, or dependency change is required.

## Model Used

- OpenAI Codex based on GPT-6, with reasoning, tool use, and code
execution. The runtime does not expose the exact serving model ID or
context window.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
This commit is contained in:
Huanyi Xie authored and GitHub committed 2026-10-15 16:13:35 -07:00
1 parent 457d3209b1
commit c4a16cbe98
3 files changed
+255 -8

No files matched your search

+9
View File
@@ -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.
@@ -74,6 +74,12 @@ function installMockFsWatcher() {
return { fakeWatcher, handlers };
}
function deferred<T>() {
let resolve!: (value: T) => void;
const promise = new Promise<T>((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<string | null>();
const newLookup = deferred<string | null>();
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<string | null>();
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<string | null>();
const newLookup = deferred<string | null>();
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<string | null>();
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();
});
});
+33 -8
View File
@@ -161,8 +161,11 @@ export function createPluginDevWatcher(
resolvePluginPackagePath?: ResolvePluginPackagePath,
fsDeps?: PluginDevWatcherFsDeps,
): PluginDevWatcher {
const watchers = new Map<string, FSWatcher>();
const watchers = new Map<string, { watcher: FSWatcher; packagePath: string }>();
const debounceTimers = new Map<string, ReturnType<typeof setTimeout>>();
const pendingRegistrations = new Map<string, number>();
// 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);
}
}
}