From 0aa44bedc42925300ce723a2c26d687c7fda8935 Mon Sep 17 00:00:00 2001 From: Evan Reichard Date: Tue, 19 May 2026 23:51:32 -0400 Subject: [PATCH] fix(watcher): coalesce edge cases, drain on unregister, expose ready() 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. --- src/watcher.ts | 83 +++++++++++++++++++++++++++------- test/unit/watcher.test.ts | 95 ++++++++++++++++++++++++++++++++++----- 2 files changed, 149 insertions(+), 29 deletions(-) diff --git a/src/watcher.ts b/src/watcher.ts index 75119c6..e467f01 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -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 = 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 { + return this.readyPromise; + } + private startChokidar(): void { + this.readyPromise = new Promise((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 { 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; +} diff --git a/test/unit/watcher.test.ts b/test/unit/watcher.test.ts index 4ec8a92..7a34c98 100644 --- a/test/unit/watcher.test.ts +++ b/test/unit/watcher.test.ts @@ -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));