diff --git a/AGENTS.md b/AGENTS.md index 56b82c2..47d777d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,6 +63,8 @@ Non-obvious bits: - **Ignore layering.** Always-ignore baseline (`.git/`, `.DS_Store`) + root `.gitignore` parsed via the `ignore` package + a small fallback for non-git workspaces. Nested gitignores aren't supported yet. +- **Startup readiness.** The daemon waits for chokidar's initial scan, capped + at 5s, so first requests don't hang indefinitely on huge workspaces. - **Debounce.** 50ms quiet period, capped at 500ms max wait so sustained event streams (branch switches) still flush in bounded time. - **Watcher and mtime-sync coexist.** When the agent edits a file we'll diff --git a/src/client.ts b/src/client.ts index 0bbf91f..dc18a34 100644 --- a/src/client.ts +++ b/src/client.ts @@ -43,10 +43,6 @@ export class LspClient { // version numbers in didOpen/didChange. We track them so the daemon // can resync files via notifyChange after on-disk edits. private versions = new Map(); - // File Watcher Registrations - Servers announce which globs they care - // about via client/registerCapability("workspace/didChangeWatchedFiles"). - // Stored by registration id so unregister can remove them; listeners - // are notified so the daemon can (re)build a chokidar watcher. private fileWatchers = new Map(); private watchersListeners = new Set<() => void>(); @@ -173,8 +169,6 @@ export class LspClient { workspace: { workspaceFolders: true, configuration: true, - // Dynamic Registration - Required for servers like gopls to send - // client/registerCapability("workspace/didChangeWatchedFiles"). didChangeWatchedFiles: { dynamicRegistration: true }, }, }, @@ -258,21 +252,14 @@ export class LspClient { return this.conn.sendRequest(method, params) as Promise; } - // Send Raw LSP Notification - Used by the daemon's workspace watcher to - // push workspace/didChangeWatchedFiles batches without going through the - // typed command surface. sendNotification(method: string, params: unknown): void { this.conn.sendNotification(method, params); } - // Get File Watchers - Flat list of all watcher patterns the server has - // registered across all registration ids. getFileWatchers(): FileSystemWatcher[] { return Array.from(this.fileWatchers.values()).flat(); } - // On Watchers Changed - Subscribe to register/unregister events for - // workspace/didChangeWatchedFiles. Returns an unsubscribe fn. onWatchersChanged(listener: () => void): () => void { this.watchersListeners.add(listener); return () => { diff --git a/src/daemon.ts b/src/daemon.ts index 0167043..bfe2deb 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -18,8 +18,8 @@ import { type LaunchContext, } from "./daemonProtocol.ts"; -// Default Idle TTL - 5 minutes. Per-server overrides via ServerConfig.idleTtlMs. const DEFAULT_IDLE_TTL_MS = 5 * 60 * 1000; +const WATCHER_READY_TIMEOUT_MS = 5000; // Client Entry - One LspClient per (server.id, rootDir), plus the bookkeeping // needed to keep files in sync and evict on idleness. @@ -38,9 +38,6 @@ interface ClientEntry { idleTimer: NodeJS.Timeout | null; ttlMs: number; lastUsed: number; - // watcher: Lazy - created on first registerCapability for watched files, - // disposed on eviction. Null if the server never registered any watchers - // (or if PI_LSP_DISABLE_WATCHERS is set). watcher: WorkspaceWatcher | null; unsubscribeWatchers: (() => void) | null; } @@ -109,10 +106,7 @@ async function getOrCreateEntry( return entry; } -// Attach Watcher - Subscribes to the client's watcher-registration events. -// The first non-empty registration lazily creates the WorkspaceWatcher; -// subsequent register/unregister calls update its pattern set in place. -// Honors PI_LSP_DISABLE_WATCHERS for emergency rollback. +// Attach Watcher - Registration can happen during initialize, before the daemon subscribes. async function attachWatcher(entry: ClientEntry): Promise { if (process.env.PI_LSP_DISABLE_WATCHERS) return; const sync = async () => { @@ -128,17 +122,34 @@ async function attachWatcher(entry: ClientEntry): Promise { log(`watcher patterns`, entry.server.id, JSON.stringify(patterns)); } entry.watcher.setPatterns(patterns); - if (patterns.length > 0) await entry.watcher.ready(); + if (patterns.length > 0) await waitForWatcherReady(entry); }; entry.unsubscribeWatchers = entry.client.onWatchersChanged(() => void sync()); - // Initial Sync - Server may have already sent registerCapability during - // initialize before we subscribed. Wait for chokidar's initial scan so - // externally-created files are not swallowed as ignoreInitial events. await sync(); } -// Forward Events - Sends a batched workspace/didChangeWatchedFiles to the -// server. We catch errors so a downed transport doesn't crash the daemon. +async function waitForWatcherReady(entry: ClientEntry): Promise { + if (!entry.watcher) return; + let timeout: NodeJS.Timeout | null = null; + let timedOut = false; + try { + await Promise.race([ + entry.watcher.ready(), + new Promise((resolve) => { + timeout = setTimeout(() => { + timedOut = true; + resolve(); + }, WATCHER_READY_TIMEOUT_MS); + }), + ]); + } finally { + if (timeout) clearTimeout(timeout); + } + if (timedOut) { + log(`watcher ready timeout`, entry.server.id, entry.rootDir); + } +} + function forwardEvents(entry: ClientEntry, events: FileEvent[]): void { try { if (process.env.LSP_DEBUG) { diff --git a/src/watcher.ts b/src/watcher.ts index e467f01..692b358 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -1,17 +1,3 @@ -// Workspace Watcher - Per-entry filesystem watcher that translates chokidar -// events into LSP `workspace/didChangeWatchedFiles` notifications. One -// instance lives on each daemon ClientEntry; it's torn down on eviction. -// -// Design: -// - Patterns come from the server via `client/registerCapability`. If the -// server registers nothing, we don't watch anything (no speculative -// watching - wastes inotify slots and risks event storms). -// - We honor the repo's `.gitignore` (root level only for v1) plus a tiny -// always-ignore baseline so the watcher tracks the same set the LSP -// server would naturally care about. -// - Events are debounced (50ms quiet period) but force-flushed every 500ms -// so sustained event streams (e.g. `git checkout` of a big branch) -// can't stall the batch indefinitely. import * as fs from "node:fs"; import * as path from "node:path"; import chokidar, { type FSWatcher } from "chokidar"; @@ -20,26 +6,17 @@ import picomatch from "picomatch"; import type { FileSystemWatcher } from "vscode-languageserver-protocol"; import { pathToUri } from "./root.ts"; -// LSP Constants - Inlined to avoid pulling the enum into a hot path. See -// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#fileChangeType const FILE_CHANGE_CREATED = 1; const FILE_CHANGE_CHANGED = 2; const FILE_CHANGE_DELETED = 3; -// WatchKind - Bitmask the server uses to opt out of specific event kinds. -// Default (when `kind` is omitted) is 7 = Create | Change | Delete. const WATCH_KIND_CREATE = 1; const WATCH_KIND_CHANGE = 2; const WATCH_KIND_DELETE = 4; -// Debounce Settings - Quiet period before flushing, plus a max wait so a -// continuous stream of events still gets delivered in bounded time. const DEBOUNCE_QUIET_MS = 50; const DEBOUNCE_MAX_WAIT_MS = 500; -// Always-Ignore Baseline - Things that are never interesting to an LSP -// server regardless of gitignore contents. `.git/` is the obvious one -// (gitignore won't list itself); the rest are OS/editor noise. export const BASELINE_IGNORES = [ "**/.git/**", "**/.DS_Store", @@ -47,8 +24,6 @@ export const BASELINE_IGNORES = [ "**/.svn/**", ]; -// Fallback Ignores - Used only when no `.gitignore` exists at rootDir. -// Conservative list to keep non-git workspaces from blowing up. const NO_GITIGNORE_FALLBACK = ["**/node_modules/**", "**/.git/**"]; export interface FileEvent { @@ -56,10 +31,6 @@ export interface FileEvent { type: 1 | 2 | 3; } -// Build Ignore Matcher - Returns a function that takes a path relative to -// rootDir and returns true if it should be ignored. Reads root .gitignore -// once at construction; nested gitignores are intentionally not supported -// in v1 (covers ~95% of repos; add nesting if it becomes a real problem). function buildIgnoreMatcher(rootDir: string): (relPath: string) => boolean { const gitignorePath = path.join(rootDir, ".gitignore"); let ig: Ignore | null = null; @@ -67,35 +38,24 @@ function buildIgnoreMatcher(rootDir: string): (relPath: string) => boolean { try { ig = ignore().add(fs.readFileSync(gitignorePath, "utf8")); } catch { - // Treat parse failure as no gitignore. The fallback will catch the - // worst offenders (node_modules) so we don't watch everything. ig = null; } } - // Baseline always applies; gitignore (or fallback) layers on top. + const baselineMatcher = picomatch(BASELINE_IGNORES); if (ig) { return (relPath) => { if (baselineMatcher(relPath)) return true; - // `ignore` requires posix-style paths and bare relative paths. const posixRel = relPath.split(path.sep).join("/"); if (!posixRel || posixRel === ".") return false; - return ig!.ignores(posixRel); + return ig.ignores(posixRel); }; } + const fallbackMatcher = picomatch(NO_GITIGNORE_FALLBACK); return (relPath) => baselineMatcher(relPath) || fallbackMatcher(relPath); } -// Compile Watcher Patterns - Turns the server-supplied FileSystemWatcher[] -// into a single matcher that returns the matched `kind` bitmask (or 0 if -// no pattern matched). Returning the kind lets us filter individual event -// types (Created/Changed/Deleted) per the spec. -// -// Servers send patterns in mixed forms: bare relative globs (`**/*.ts`), -// absolute path globs (gopls sends `/abs/root/**/*.go`), or RelativePattern -// objects. We try matching the candidate path in both relative and absolute -// form so we accept any of those without server-specific special casing. function compileWatchers( watchers: FileSystemWatcher[], rootDir: string, @@ -109,6 +69,7 @@ function compileWatchers( kind: w.kind ?? (WATCH_KIND_CREATE | WATCH_KIND_CHANGE | WATCH_KIND_DELETE), }; }); + return (relPath, absPath) => { const posixRel = relPath.split(path.sep).join("/"); const posixAbs = absPath.split(path.sep).join("/"); @@ -120,10 +81,7 @@ function compileWatchers( }; } -// Resolve Relative Pattern - LSP 3.17 added RelativePattern with a baseUri -// that may be a string or WorkspaceFolder. We collapse to a glob relative -// to rootDir; out-of-root patterns fall back to the raw pattern (won't -// match anything inside rootDir, which is the safe behavior). +// Relative Pattern - Servers may send baseUri as a string or WorkspaceFolder. function resolveRelativePattern( rp: { baseUri: string | { uri: string }; pattern: string }, rootDir: string, @@ -144,9 +102,6 @@ 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; @@ -157,18 +112,10 @@ export class WorkspaceWatcher { this.isIgnored = buildIgnoreMatcher(rootDir); } - // Set Patterns - Called whenever the server's registered watchers change. - // First non-empty call lazily starts chokidar; subsequent calls update - // the matcher in place (chokidar already watches everything under - // rootDir minus ignores, so we just recompile filtering). setPatterns(watchers: FileSystemWatcher[]): void { if (this.disposed) return; this.matchKind = compileWatchers(watchers, this.rootDir); if (watchers.length === 0) { - // 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; @@ -176,40 +123,28 @@ export class WorkspaceWatcher { 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, - // Ignore Function - Called by chokidar for each path; returning true - // prevents both watching and event emission. Cheaper than emitting - // and filtering afterward. ignored: (absPath: string) => { if (absPath === this.rootDir) return false; const rel = path.relative(this.rootDir, absPath); return this.isIgnored(rel); }, - // Atomic Writes - Many editors write-via-rename; awaitWriteFinish - // coalesces those into a single change event. Conservative thresholds - // keep latency low while still suppressing partial-write noise. awaitWriteFinish: { stabilityThreshold: 50, pollInterval: 25 }, }); 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`); }); @@ -219,8 +154,6 @@ 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(); @@ -239,10 +172,10 @@ export class WorkspaceWatcher { const rel = path.relative(this.rootDir, absPath); const kind = this.matchKind(rel, absPath); if (kind === 0) return; - // Per-Spec Filter - Skip events the server explicitly opted out of. if (type === FILE_CHANGE_CREATED && !(kind & WATCH_KIND_CREATE)) return; if (type === FILE_CHANGE_CHANGED && !(kind & WATCH_KIND_CHANGE)) return; if (type === FILE_CHANGE_DELETED && !(kind & WATCH_KIND_DELETE)) return; + const uri = pathToUri(absPath); const prev = this.pending.get(uri); const next = coalesce(prev, type); @@ -265,6 +198,7 @@ export class WorkspaceWatcher { this.quietTimer = null; this.maxWaitTimer = null; if (this.pending.size === 0) return; + const events: FileEvent[] = Array.from(this.pending, ([uri, type]) => ({ uri, type, @@ -287,17 +221,6 @@ export class WorkspaceWatcher { } } -// 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, diff --git a/test/integration/watcher-gopls.test.ts b/test/integration/watcher-gopls.test.ts index c3cb812..249641d 100644 --- a/test/integration/watcher-gopls.test.ts +++ b/test/integration/watcher-gopls.test.ts @@ -1,8 +1,3 @@ -// Watcher Integration Test — proves the FS-watcher → LSP wiring works -// end-to-end against gopls: a stale "undefined symbol" diagnostic clears -// after the missing file is created externally (no LSP query touches it). -// -// This is the canonical staleness scenario from `_scratch/plan-fs-watching.md`. import { describe, it, before, after } from "node:test"; import * as assert from "node:assert/strict"; import * as fs from "node:fs"; @@ -17,10 +12,6 @@ import { const skip = requireServer("gopls"); -// Poll Until - Runs `fn` repeatedly until `predicate(result)` is true or -// the timeout elapses. We need this because gopls reanalysis after a -// workspace/didChangeWatchedFiles is asynchronous; diagnostics arrive on -// a `textDocument/publishDiagnostics` push that we then re-fetch. async function pollUntil( fn: () => Promise, predicate: (v: T) => boolean, @@ -53,12 +44,9 @@ describe("watcher: gopls picks up externally-created files", { skip: skip ?? und await stopTestDaemon(env); tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-lsp-gopls-watch-")); - // Minimal Go Module - go.mod is gopls's primary root marker. fs.writeFileSync(path.join(tmpDir, "go.mod"), "module example.com/wtest\n\ngo 1.21\n"); mainFile = path.join(tmpDir, "main.go"); helperFile = path.join(tmpDir, "helper.go"); - // main.go references Helper() which doesn't exist yet \u2014 should produce - // an "undefined: Helper" diagnostic. fs.writeFileSync( mainFile, "package main\n\nfunc main() {\n\tHelper()\n}\n", @@ -72,9 +60,6 @@ describe("watcher: gopls picks up externally-created files", { skip: skip ?? und }); it("initially reports undefined symbol", async () => { - // Poll - gopls's workspace load on a fresh tmp dir takes a beat; the - // first diagnostics call can return "No active builds contain ..." - // before the real analysis lands. Wait up to 15s for it to settle. const result = await pollUntil( async () => (await runCliJson( @@ -103,16 +88,11 @@ describe("watcher: gopls picks up externally-created files", { skip: skip ?? und }); it("clears the diagnostic after helper.go is created externally", async () => { - // Create The Missing File Without Touching It Via LSP - This is the - // whole point: only chokidar + workspace/didChangeWatchedFiles can tell - // gopls about helper.go's existence. fs.writeFileSync( helperFile, "package main\n\nfunc Helper() {}\n", ); - // Poll - Allow up to 15s for the watcher to fire, gopls to reanalyze, - // and the diagnostic to clear. Re-query the same file (main.go) only. const result = await pollUntil( async () => (await runCliJson( diff --git a/test/unit/watcher.test.ts b/test/unit/watcher.test.ts index 7a34c98..228af73 100644 --- a/test/unit/watcher.test.ts +++ b/test/unit/watcher.test.ts @@ -1,6 +1,3 @@ -// Watcher Unit Tests — exercises WorkspaceWatcher against a temp dir with -// real chokidar. We use real FS because mocking it would test the mock, -// not the actual behavior under inotify. import { describe, it, before, after, beforeEach } from "node:test"; import * as assert from "node:assert/strict"; import * as fs from "node:fs"; @@ -9,9 +6,6 @@ import * as path from "node:path"; import { WorkspaceWatcher, type FileEvent } from "../../src/watcher.ts"; import { pathToUri } from "../../src/root.ts"; -// Wait For - Polls a predicate up to timeoutMs. Returns true if it became -// true, false if it timed out. Used to wait on async chokidar events -// without arbitrary sleeps. async function waitFor( predicate: () => boolean, timeoutMs = 2000, @@ -24,8 +18,6 @@ async function waitFor( return predicate(); } -// Wait Quiet - Waits for the debounce window to elapse so any pending -// events flush. Slightly longer than DEBOUNCE_MAX_WAIT_MS. const FLUSH_WAIT_MS = 700; describe("WorkspaceWatcher", () => { @@ -47,7 +39,6 @@ describe("WorkspaceWatcher", () => { watcher = null; } received = []; - // Reset Dir - Wipe contents between tests so file lists are predictable. for (const entry of fs.readdirSync(tmpDir)) { fs.rmSync(path.join(tmpDir, entry), { recursive: true, force: true }); } @@ -140,7 +131,6 @@ describe("WorkspaceWatcher", () => { it("respects WatchKind to filter event types", async () => { watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs)); - // Kind 1 = Create only - delete events should be suppressed. watcher.setPatterns([{ globPattern: "**/*.ts", kind: 1 }]); await watcher.ready(); @@ -166,8 +156,6 @@ describe("WorkspaceWatcher", () => { watcher.setPatterns([{ globPattern: "**/*.ts" }]); await watcher.ready(); - // Write Several Files Quickly - within the 50ms debounce window they - // should all land in one batch, capped at 500ms max wait. for (let i = 0; i < 5; i++) { fs.writeFileSync(path.join(tmpDir, `f${i}.ts`), "x"); } @@ -176,8 +164,6 @@ describe("WorkspaceWatcher", () => { const all = received.flat(); assert.strictEqual(all.length, 5, `Expected 5 events, got ${all.length}`); - // The batches should be << 5 (debounce coalesces). Allow up to 2 in - // case the loop straddled a flush boundary. assert.ok( received.length <= 2, `Expected <=2 batches, got ${received.length}: ${JSON.stringify(received)}`, @@ -217,8 +203,6 @@ describe("WorkspaceWatcher", () => { }); it("matches absolute-path glob patterns (gopls-style)", async () => { - // Regression - gopls registers e.g. `/tmp/.../**/*.{go,mod}` rather - // than a bare relative `**/*.go`. The matcher must accept both. watcher = new WorkspaceWatcher(tmpDir, (evs) => received.push(evs)); watcher.setPatterns([ { globPattern: `${tmpDir}/**/*.{go,mod}` }, @@ -240,7 +224,6 @@ describe("WorkspaceWatcher", () => { 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); @@ -255,15 +238,12 @@ describe("WorkspaceWatcher", () => { }); 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"); @@ -272,11 +252,6 @@ describe("WorkspaceWatcher", () => { 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]) || @@ -293,8 +268,6 @@ describe("WorkspaceWatcher", () => { 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([]);