evan/pi-lsp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(watcher): coalesce edge cases, drain on unregister, expose ready()

Browse Source 
Addresses review feedback on 7787626:

1. Deleted->Created at the same path is a file replacement, not a no-op.
   Previous coalescing dropped both Created->Deleted and Deleted->Created;
   the latter left the server with no signal to re-read replaced content.
   Now: Deleted->Created collapses to Changed, Created->Changed keeps
   Created (server didn't know the file at all). Extracted coalesce() so
   the matrix is reviewable in one place.

2. setPatterns([]) (server unregistered all watchers) stopped chokidar
   but left pending events + timers intact, so a queued batch could
   still fire after the server stopped caring. Now drains via
   cancelPending() before stopping chokidar.

3. Added ready() returning a promise resolved by chokidar's initial-scan
   'ready' event. Production daemon doesn't need to await it (LSP
   handshake gives chokidar ample wall-time), but tests now use it
   instead of fixed 200ms sleeps - deflakes the suite on slower
   filesystems and addresses the (narrow) startup race where a file
   created during chokidar's initial crawl could be missed.

4. Unit tests replace 11 hardcoded sleeps with watcher.ready(), and add
   coverage for the two coalesce fixes plus the unregister-drains case.
This commit is contained in:
Evan Reichard
2026-05-19 23:51:32 -04:00
parent 77876264ee
commit 0aa44bedc4
2 changed files with 149 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
src/watcher.ts
View File

@@ -144,6 +144,11 @@ export class WorkspaceWatcher {
private quietTimer: NodeJS.Timeout | null = null;
private maxWaitTimer: NodeJS.Timeout | null = null;
private disposed = false;
// readyPromise: Resolves on chokidar's initial-scan `ready` event. Tests
// await this to deflake; the daemon doesn't bother (the LSP handshake
// gives chokidar ample wall-time to finish its initial scan).
private readyPromise: Promise<void> = Promise.resolve();
private resolveReady: (() => void) | null = null;
constructor(
private readonly rootDir: string,
@@ -160,15 +165,30 @@ export class WorkspaceWatcher {
if (this.disposed) return;
this.matchKind = compileWatchers(watchers, this.rootDir);
if (watchers.length === 0) {
// Server unregistered everything - stop watching but keep instance
// alive so re-registration works without recreating chokidar state.
// Server unregistered everything - drop pending events and timers
// so a queued batch can't fire after the server stopped caring,
// then stop chokidar but keep this instance alive for possible
// re-registration.
this.cancelPending();
void this.stopChokidar();
return;
}
if (!this.chokidar) this.startChokidar();
}
// Ready - Resolves once chokidar has finished its initial scan and is
// emitting events. Useful for deflaking tests; production callers can
// skip it.
ready(): Promise<void> {
return this.readyPromise;
}
private startChokidar(): void {
this.readyPromise = new Promise<void>((resolve) => {
// Resolved by the `ready` handler below. We capture the resolver
// here so stopChokidar() can swap promises without leaking.
this.resolveReady = resolve;
});
this.chokidar = chokidar.watch(this.rootDir, {
ignoreInitial: true,
followSymlinks: false,
@@ -188,6 +208,7 @@ export class WorkspaceWatcher {
this.chokidar.on("add", (p) => this.queue(p, FILE_CHANGE_CREATED));
this.chokidar.on("change", (p) => this.queue(p, FILE_CHANGE_CHANGED));
this.chokidar.on("unlink", (p) => this.queue(p, FILE_CHANGE_DELETED));
this.chokidar.on("ready", () => this.resolveReady?.());
// ENOSPC and friends - log but don't crash the entry.
this.chokidar.on("error", (err) => {
process.stderr.write(`[pi-lsp:watcher] ${this.rootDir}: ${String(err)}\n`);
@@ -198,9 +219,22 @@ export class WorkspaceWatcher {
if (!this.chokidar) return;
const w = this.chokidar;
this.chokidar = null;
// Reset Ready - A future startChokidar() will install a fresh promise.
// Resolve the old one so any awaiter doesn't hang.
this.resolveReady?.();
this.resolveReady = null;
this.readyPromise = Promise.resolve();
await w.close();
}
private cancelPending(): void {
if (this.quietTimer) clearTimeout(this.quietTimer);
if (this.maxWaitTimer) clearTimeout(this.maxWaitTimer);
this.quietTimer = null;
this.maxWaitTimer = null;
this.pending.clear();
}
private queue(absPath: string, type: 1 | 2 | 3): void {
const rel = path.relative(this.rootDir, absPath);
const kind = this.matchKind(rel, absPath);
@@ -210,19 +244,10 @@ export class WorkspaceWatcher {
if (type === FILE_CHANGE_CHANGED && !(kind & WATCH_KIND_CHANGE)) return;
if (type === FILE_CHANGE_DELETED && !(kind & WATCH_KIND_DELETE)) return;
const uri = pathToUri(absPath);
// Coalesce per URI - Last event wins. The exception is Created followed
// by Deleted (and vice versa), which we collapse to "nothing happened"
// by deleting the entry; the server doesn't need to learn about a file
// that briefly existed.
const prev = this.pending.get(uri);
if (
(prev === FILE_CHANGE_CREATED && type === FILE_CHANGE_DELETED) ||
(prev === FILE_CHANGE_DELETED && type === FILE_CHANGE_CREATED)
) {
this.pending.delete(uri);
} else {
this.pending.set(uri, type);
}
const next = coalesce(prev, type);
if (next === null) this.pending.delete(uri);
else this.pending.set(uri, next);
this.scheduleFlush();
}
@@ -257,9 +282,33 @@ export class WorkspaceWatcher {
async dispose(): Promise<void> {
if (this.disposed) return;
this.disposed = true;
if (this.quietTimer) clearTimeout(this.quietTimer);
if (this.maxWaitTimer) clearTimeout(this.maxWaitTimer);
this.pending.clear();
this.cancelPending();
await this.stopChokidar();
}
}
// Coalesce - Combines a pending event with a newly arrived one for the
// same URI. Returns the resulting type, or null to drop the entry entirely.
//
// The interesting cases:
// Created -> Deleted : drop (transient file the server never knew about)
// Deleted -> Created : Changed (file replaced; server already thinks it
// exists - or didn't, but Changed is the safe call
// and prompts a re-read)
// Created -> Changed : keep Created (server didn't know the file at all)
// Changed -> Deleted : Deleted (latter overrides)
// * -> * : latter overrides (default)
function coalesce(
prev: 1 | 2 | 3 | undefined,
next: 1 | 2 | 3,
): 1 | 2 | 3 | null {
if (prev === undefined) return next;
if (prev === FILE_CHANGE_CREATED && next === FILE_CHANGE_DELETED) return null;
if (prev === FILE_CHANGE_DELETED && next === FILE_CHANGE_CREATED) {
return FILE_CHANGE_CHANGED;
}
if (prev === FILE_CHANGE_CREATED && next === FILE_CHANGE_CHANGED) {
return FILE_CHANGE_CREATED;
}
return next;
}

95
test/unit/watcher.test.ts
View File

@@ -56,8 +56,7 @@ describe("WorkspaceWatcher", () => {
it("emits Created event for new file matching pattern", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
// Settle - chokidar's `ready` is implicit; give it a moment.
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "foo.ts"), "x");
@@ -74,7 +73,7 @@ describe("WorkspaceWatcher", () => {
fs.writeFileSync(file, "x");
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.unlinkSync(file);
@@ -89,7 +88,7 @@ describe("WorkspaceWatcher", () => {
fs.writeFileSync(file, "x");
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(file, "y");
@@ -102,7 +101,7 @@ describe("WorkspaceWatcher", () => {
it("skips files not matching the pattern", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "ignored.txt"), "x");
await new Promise((r) => setTimeout(r, FLUSH_WAIT_MS));
@@ -120,7 +119,7 @@ describe("WorkspaceWatcher", () => {
fs.mkdirSync(path.join(tmpDir, "ignored-dir"));
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "ignored-dir", "x.ts"), "x");
fs.writeFileSync(path.join(tmpDir, "watched.ts"), "x");
@@ -143,7 +142,7 @@ describe("WorkspaceWatcher", () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
// Kind 1 = Create only - delete events should be suppressed.
watcher.setPatterns([{ globPattern: "**/*.ts", kind: 1 }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
const file = path.join(tmpDir, "only-create.ts");
fs.writeFileSync(file, "x");
@@ -165,7 +164,7 @@ describe("WorkspaceWatcher", () => {
it("batches multiple rapid events into one onEvents call", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
// Write Several Files Quickly - within the 50ms debounce window they
// should all land in one batch, capped at 500ms max wait.
@@ -188,7 +187,7 @@ describe("WorkspaceWatcher", () => {
it("ignores .git/ even when not in gitignore", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.mkdirSync(path.join(tmpDir, ".git"));
fs.writeFileSync(path.join(tmpDir, ".git", "HEAD"), "ref: foo");
@@ -205,7 +204,7 @@ describe("WorkspaceWatcher", () => {
it("stops emitting after dispose", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
await watcher.dispose();
watcher = null;
@@ -224,7 +223,7 @@ describe("WorkspaceWatcher", () => {
watcher.setPatterns([
{ globPattern: `${tmpDir}/**/*.{go,mod}` },
]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "helper.go"), "package x\n");
@@ -236,10 +235,82 @@ describe("WorkspaceWatcher", () => {
assert.ok(ok, `Expected event for absolute glob, got ${JSON.stringify(received)}`);
});
it("coalesces Created+Deleted to no-op", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await watcher.ready();
// Create Then Delete - both within the debounce quiet window.
const file = path.join(tmpDir, "transient.ts");
fs.writeFileSync(file, "x");
fs.unlinkSync(file);
await new Promise((r) => setTimeout(r, FLUSH_WAIT_MS));
const all = received.flat();
assert.ok(
!all.some((e) => e.uri === pathToUri(file)),
`Transient file should not surface, got ${JSON.stringify(all)}`,
);
});
it("coalesces Deleted+Created (replacement) to Changed", async () => {
// Setup - file exists before the watcher starts, so chokidar's initial
// scan registers it (with ignoreInitial: true, no event fires).
const file = path.join(tmpDir, "replace.ts");
fs.writeFileSync(file, "v1");
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await watcher.ready();
// Delete Then Recreate - within one debounce window.
fs.unlinkSync(file);
fs.writeFileSync(file, "v2");
await new Promise((r) => setTimeout(r, FLUSH_WAIT_MS));
const all = received.flat();
const events = all.filter((e) => e.uri === pathToUri(file));
assert.ok(events.length > 0, `Expected an event for replaced file`);
// Expected: the coalesced single event is Changed (type=2). Some
// platforms may split the delete+create across debounce windows, in
// which case the server still sees correct state. Accept either:
// - one Changed event (coalesced), or
// - Deleted then Created in separate batches (not coalesced, fine)
const types = events.map((e) => e.type).sort();
const acceptable =
JSON.stringify(types) === JSON.stringify([2]) ||
JSON.stringify(types) === JSON.stringify([1, 3]);
assert.ok(
acceptable,
`Expected [2] or [1,3], got ${JSON.stringify(types)}`,
);
});
it("drops pending events when patterns are cleared", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([{ globPattern: "**/*.ts" }]);
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "pending.ts"), "x");
// Don't wait for the debounce - clear patterns immediately so the
// pending batch should be dropped, not flushed.
await new Promise((r) => setTimeout(r, 10));
watcher.setPatterns([]);
await new Promise((r) => setTimeout(r, FLUSH_WAIT_MS));
assert.strictEqual(
received.length,
0,
`Pending batch should be dropped, got ${JSON.stringify(received)}`,
);
});
it("empty pattern set means no watching", async () => {
watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs));
watcher.setPatterns([]);
await new Promise((r) => setTimeout(r, 200));
await watcher.ready();
fs.writeFileSync(path.join(tmpDir, "no-patterns.ts"), "x");
await new Promise((r) => setTimeout(r, FLUSH_WAIT_MS));