Repository navigation
fix(indexing): replace chokidar file watcher with @parcel/watcher #12784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
61ec24d
07b082e
0a12bda
3b4cc06
f15f88b
1d267c6
9d83843
c4834a1
d9c94b7
1da932b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { | |
| BATCH_SEGMENT_THRESHOLD, | ||
| MAX_BATCH_RETRIES, | ||
| INITIAL_RETRY_DELAY_MS, | ||
| WATCHER_READY_TIMEOUT_MS, | ||
| } from "../constants" | ||
| import { scannerExtensions } from "../shared/supported-extensions" | ||
| import { | ||
|
|
@@ -38,6 +39,53 @@ import { isBinary } from "../shared/is-binary" | |
|
|
||
| const log = Log.create({ service: "file-watcher" }) | ||
|
|
||
| /** | ||
| * Minimal watcher surface needed to await readiness. Declaring our own narrow | ||
| * interface (rather than the full chokidar type) keeps {@link waitForWatcherReady} | ||
| * unit-testable without spinning up a real filesystem watcher. | ||
| */ | ||
| export interface WatcherReadySource { | ||
| once(event: "ready", listener: () => void): unknown | ||
| once(event: "error", listener: (error: unknown) => void): unknown | ||
| } | ||
|
|
||
| /** | ||
| * Waits for a chokidar watcher to finish its initial scan and emit "ready". | ||
| * | ||
| * Rejects if the watcher emits "error", or if neither "ready" nor "error" | ||
| * occurs within `timeoutMs`. The timeout is what prevents a silent, indefinite | ||
| * hang on very large workspaces (or when chokidar's native backend is | ||
| * unavailable and it falls back to a slow per-directory walk). The caller owns | ||
| * the watcher lifecycle and is responsible for closing it on failure. | ||
| */ | ||
| export function waitForWatcherReady( | ||
| watcher: WatcherReadySource, | ||
| workspacePath: string, | ||
| timeoutMs: number = WATCHER_READY_TIMEOUT_MS, | ||
| ): Promise<void> { | ||
| return new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| reject( | ||
| new Error( | ||
| `File watcher did not become ready within ${Math.round(timeoutMs / 1000)}s for "${workspacePath}". ` + | ||
| `The workspace may be too large or contain problematic symlinks — scope indexing to a single ` + | ||
| `repository, or add .gitignore/.kilocodeignore patterns to exclude large directories.`, | ||
| ), | ||
| ) | ||
| }, timeoutMs) | ||
| // Do not let the safety timeout keep the event loop alive on its own. | ||
| timer.unref() | ||
| watcher.once("ready", () => { | ||
| clearTimeout(timer) | ||
| resolve() | ||
| }) | ||
| watcher.once("error", (error: unknown) => { | ||
| clearTimeout(timer) | ||
| reject(error instanceof Error ? error : new Error(String(error))) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Implementation of the file watcher interface. | ||
| * | ||
|
|
@@ -137,7 +185,7 @@ export class FileWatcher implements IFileWatcher { | |
|
|
||
| log.info("initializing file watcher", { workspacePath: this.workspacePath }) | ||
|
|
||
| this.watcher = chokidarWatch(this.workspacePath, { | ||
| const watcher = chokidarWatch(this.workspacePath, { | ||
| ignored: (filePath: string) => { | ||
| const relativeFilePath = generateRelativeIgnorePath(filePath, this.workspacePath) | ||
| if (!relativeFilePath) return false | ||
|
|
@@ -146,16 +194,32 @@ export class FileWatcher implements IFileWatcher { | |
| }, | ||
| persistent: true, | ||
| ignoreInitial: true, | ||
| // Match the scanner's glob (which does not traverse symlinks) and git's | ||
| // own semantics. Following symlinked directories can cause chokidar to | ||
| // re-walk/cycle through link targets, which on symlink-heavy trees can | ||
| // stall the initial scan so "ready" never fires. | ||
| followSymlinks: false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: The comment justifies this by matching the scanner, and for symlinked directories that is accurate ( Probably an acceptable trade for the hang this fixes, but it's a behavior change beyond "avoid symlink cycles" and worth calling out in the changeset/PR notes (or narrowing, e.g. keep following links but bound the walk). Reply with |
||
| }) | ||
| this.watcher = watcher | ||
|
|
||
| this.watcher.on("add", (filePath) => this.handleFileEvent(filePath, "create")) | ||
| this.watcher.on("change", (filePath) => this.handleFileEvent(filePath, "change")) | ||
| this.watcher.on("unlink", (filePath) => this.handleFileEvent(filePath, "delete")) | ||
| this.ready = new Promise((resolve, reject) => { | ||
| this.watcher?.once("ready", resolve) | ||
| this.watcher?.once("error", reject) | ||
| }) | ||
| await this.ready | ||
| watcher.on("add", (filePath) => this.handleFileEvent(filePath, "create")) | ||
| watcher.on("change", (filePath) => this.handleFileEvent(filePath, "change")) | ||
| watcher.on("unlink", (filePath) => this.handleFileEvent(filePath, "delete")) | ||
|
|
||
| const ready = waitForWatcherReady(watcher, this.workspacePath) | ||
| this.ready = ready | ||
| try { | ||
| await ready | ||
| } catch (error) { | ||
| // The watcher never became ready (or emitted an error). Surface the | ||
| // failure instead of hanging the indexing pipeline forever, and reset | ||
| // state so a later initialize() can retry cleanly rather than re-awaiting | ||
| // this rejected promise. | ||
| if (this.ready === ready) this.ready = undefined | ||
| if (this.watcher === watcher) this.watcher = undefined | ||
| void watcher.close() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Fire-and-forget Two concerns on this line, both specific to the failure path this PR adds:
Something like Reply with |
||
| throw error | ||
| } | ||
| log.info("file watcher ready", { workspacePath: this.workspacePath }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,11 @@ import type { | |
| PointStruct, | ||
| VectorStoreSearchResult, | ||
| } from "../../../../src/indexing/interfaces" | ||
| import { FileWatcher } from "../../../../src/indexing/processors/file-watcher" | ||
| import { | ||
| FileWatcher, | ||
| waitForWatcherReady, | ||
| type WatcherReadySource, | ||
| } from "../../../../src/indexing/processors/file-watcher" | ||
| import { CodeParser } from "../../../../src/indexing/processors/parser" | ||
| import { loadIgnore } from "../../../../src/indexing/shared/load-ignore" | ||
| import { WorktreeOverlay } from "../../../../src/indexing/worktree-overlay" | ||
|
|
@@ -384,3 +388,43 @@ describe("FileWatcher", () => { | |
| } | ||
| }) | ||
| }) | ||
|
|
||
| describe("waitForWatcherReady", () => { | ||
| type FakeWatcher = WatcherReadySource & { emit(event: string, arg?: unknown): void } | ||
|
|
||
| function createReadyWatcher(): FakeWatcher { | ||
| const handlers = new Map<string, (arg?: unknown) => void>() | ||
| return { | ||
| once(event: string, listener: (arg?: unknown) => void) { | ||
| handlers.set(event, listener) | ||
| }, | ||
| emit(event: string, arg?: unknown) { | ||
| handlers.get(event)?.(arg) | ||
| }, | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: The riskiest new logic — The three new tests cover A test against a real temp directory with a very small timeout — or injecting the readiness wait — would cover the retry path without a real hang. Minor note on the fake: Reply with |
||
|
|
||
| test("resolves once the watcher emits ready", async () => { | ||
| const watcher = createReadyWatcher() | ||
| const ready = waitForWatcherReady(watcher, "/workspace", 1000) | ||
| watcher.emit("ready") | ||
| await expect(ready).resolves.toBeUndefined() | ||
| }) | ||
|
|
||
| test("rejects with an actionable error when ready never fires within the timeout", async () => { | ||
| // A watcher that never emits "ready" simulates a very large tree or a | ||
| // missing native backend; this must fail fast instead of hanging forever. | ||
| const watcher = createReadyWatcher() | ||
| const ready = waitForWatcherReady(watcher, "/huge/workspace", 20) | ||
| await expect(ready).rejects.toThrow(/did not become ready/) | ||
| await expect(ready).rejects.toThrow(/scope indexing to a single/) | ||
| await expect(ready).rejects.toThrow(/\/huge\/workspace/) | ||
| }) | ||
|
|
||
| test("rejects when the watcher emits an error", async () => { | ||
| const watcher = createReadyWatcher() | ||
| const ready = waitForWatcherReady(watcher, "/workspace", 1000) | ||
| watcher.emit("error", new Error("watch boom")) | ||
| await expect(ready).rejects.toThrow("watch boom") | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: A hard 60s deadline turns a hang into "no indexing at all"
Since
orchestrator.runIndexing()awaits_startWatcher()before the vector store init and the scan, a rejection here aborts the whole pipeline and lands inErrorstate — the user gets zero indexed files instead of an infinite spinner. That is strictly better for diagnosability, but as the PR notes, the watcher is only needed for incremental updates, so a genuinely large monorepo whose initial chokidar walk takes 70s now goes from "eventually works" to "never works".Given the fix is already touching this path, consider degrading instead of failing: log/telemetry the readiness timeout, skip the watcher (no incremental updates), and let the scan proceed. If the hard failure is intentional for now, a follow-up issue for the degraded mode would be worth linking, and making the bound configurable (as suggested in the PR description) reduces the risk of false failures on slow network filesystems.
Reply with
@kilocode-bot fix itto have Kilo Code address this issue.