feat(watch): replace 250 ms watch polling with evented filesystem observation#531
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Nice |
20f5fc7 to
fd00dd6
Compare
|
Frozen watch benchmark campaign:
The candidate merge-base equals the frozen base. Any revision change invalidates this campaign ID. |
10f6017 to
e19943f
Compare
Greptile SummaryThis PR replaces the
Confidence Score: 4/5The core controller and observer are well-tested and structurally sound; the main concern is a coverage-override in watchPlan that silently slows jj/sl VCS change detection when an agent-context file is present. The controller state machine is thoroughly exercised by deterministic unit tests, real-filesystem integration tests, and end-to-end PTY scenarios. The one functional gap is in watchPlan.ts: when a jj or sl adapter reports poll-only coverage but the user also supplies --agent-context, the unconditional coverage = hybrid override causes the WatchController to use the 10-second healthy safety interval for VCS change detection instead of the 2-second degraded poll interval. src/core/watchPlan.ts — the coverage override at line 141 deserves a second look before merge. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant FS as Filesystem
participant Obs as WatchObserver
participant Ctrl as WatchController
participant Sig as getSignature
participant UI as App refresh
Note over Obs,Ctrl: Startup
Obs->>Ctrl: onReady
Ctrl->>Sig: beginCheck
Sig-->>Ctrl: signature matches, idle
Note over FS,Ctrl: Event-driven path
FS->>Obs: raw fs event
Obs->>Obs: ignored-root filter
Obs->>Ctrl: onEvent
Ctrl->>Ctrl: debounce 200ms quiet / 1s max
Ctrl->>Sig: beginCheck
alt signature changed
Sig-->>Ctrl: new signature
Ctrl->>UI: refresh
UI-->>Ctrl: done
Ctrl->>Ctrl: update appliedSignature, idle
else unchanged
Sig-->>Ctrl: same signature, idle
end
Note over Ctrl: Safety poll path
Ctrl->>Ctrl: safety timer 10s healthy / 2s degraded
Ctrl->>Sig: beginCheck
Note over Ctrl: Degradation
Obs->>Ctrl: onError ENOSPC or EMFILE
Ctrl->>Obs: close
Ctrl->>Ctrl: degraded true, 2s safety poll
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant FS as Filesystem
participant Obs as WatchObserver
participant Ctrl as WatchController
participant Sig as getSignature
participant UI as App refresh
Note over Obs,Ctrl: Startup
Obs->>Ctrl: onReady
Ctrl->>Sig: beginCheck
Sig-->>Ctrl: signature matches, idle
Note over FS,Ctrl: Event-driven path
FS->>Obs: raw fs event
Obs->>Obs: ignored-root filter
Obs->>Ctrl: onEvent
Ctrl->>Ctrl: debounce 200ms quiet / 1s max
Ctrl->>Sig: beginCheck
alt signature changed
Sig-->>Ctrl: new signature
Ctrl->>UI: refresh
UI-->>Ctrl: done
Ctrl->>Ctrl: update appliedSignature, idle
else unchanged
Sig-->>Ctrl: same signature, idle
end
Note over Ctrl: Safety poll path
Ctrl->>Ctrl: safety timer 10s healthy / 2s degraded
Ctrl->>Sig: beginCheck
Note over Ctrl: Degradation
Obs->>Ctrl: onError ENOSPC or EMFILE
Ctrl->>Obs: close
Ctrl->>Ctrl: degraded true, 2s safety poll
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/core/watchPlan.ts:133-142
**Sidecar overrides poll-only coverage, doubling VCS-change latency for jj/sl**
When a VCS adapter (jj or sl) returns `coverage: "poll-only"` but the user supplies an `--agent-context` file, the unconditional `coverage = "hybrid"` on line 141 causes `useWatchedInput` to pass `pollOnly: false` to `createWatchController`. The controller then uses `healthyCheckMs` (10 s) for its safety interval instead of `degradedCheckMs` (2 s). For a jj/sl user with an agent-context file, a committed or amended revision goes undetected for up to 10 s — five times longer than the "2-second polling fallback" the PR description promises for those adapters.
The fix is to preserve the adapter's coverage when overriding it toward hybrid, or to keep `pollOnly: true` so the controller continues using the 2 s interval even while the sidecar event source is active.
### Issue 2 of 2
src/core/watchObserver.ts:52-62
**Ancestor-walk in `createIgnoredRootMatcher` is O(depth) per event on the hot path**
`createIgnoredRootMatcher` returns a closure that climbs the directory tree on every raw filesystem event (one `dirname` per path segment). For a typical monorepo with 15 levels of nesting and `node_modules` ignored, every event under `src/` results in ~15 string comparisons and allocations. On Linux/portable backends the ignored matcher is called by Chokidar's own traversal as well, so events within deep trees are charged twice. Depth is bounded in practice, but the allocation-per-level cost is worth noting if high-frequency saves in deeply nested directories prove noisy. A path-prefix check (`current.startsWith(root + sep)`) would avoid the inner loop entirely for roots that share a common prefix with the event path.
Reviews (1): Last reviewed commit: "docs(watch): preserve preliminary benchm..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| if (input.options.agentContext) { | ||
| fileTargets.push({ path: input.options.agentContext, source: "sidecar" }); | ||
| coverage = "hybrid"; | ||
| } | ||
|
|
||
| return { | ||
| coverage, | ||
| targets: [...adapterTargets, ...groupFileTargets(fileTargets, context)], | ||
| }; |
There was a problem hiding this comment.
When a VCS adapter (jj or sl) returns coverage: "poll-only" but the user supplies an --agent-context file, the unconditional coverage = "hybrid" on line 141 causes useWatchedInput to pass pollOnly: false to createWatchController. The controller then uses healthyCheckMs (10 s) for its safety interval instead of degradedCheckMs (2 s). For a jj/sl user with an agent-context file, a committed or amended revision goes undetected for up to 10 s — five times longer than the "2-second polling fallback" the PR description promises for those adapters.
The fix is to preserve the adapter's coverage when overriding it toward hybrid, or to keep pollOnly: true so the controller continues using the 2 s interval even while the sidecar event source is active.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/watchPlan.ts
Line: 133-142
Comment:
**Sidecar overrides poll-only coverage, doubling VCS-change latency for jj/sl**
When a VCS adapter (jj or sl) returns `coverage: "poll-only"` but the user supplies an `--agent-context` file, the unconditional `coverage = "hybrid"` on line 141 causes `useWatchedInput` to pass `pollOnly: false` to `createWatchController`. The controller then uses `healthyCheckMs` (10 s) for its safety interval instead of `degradedCheckMs` (2 s). For a jj/sl user with an agent-context file, a committed or amended revision goes undetected for up to 10 s — five times longer than the "2-second polling fallback" the PR description promises for those adapters.
The fix is to preserve the adapter's coverage when overriding it toward hybrid, or to keep `pollOnly: true` so the controller continues using the 2 s interval even while the sidecar event source is active.
How can I resolve this? If you propose a fix, please make it concise.| const absolute = resolve(path); | ||
| return process.platform === "win32" ? absolute.toLowerCase() : absolute; | ||
| } | ||
|
|
||
| /** Build an ancestor-set matcher whose cost depends on path depth, not ignored-root count. */ | ||
| function createIgnoredRootMatcher(ignoredRoots: string[]) { | ||
| const roots = new Set(ignoredRoots.map(normalizedPath)); | ||
| return (path: string) => { | ||
| let current = normalizedPath(path); | ||
| for (;;) { | ||
| if (roots.has(current)) return true; |
There was a problem hiding this comment.
createIgnoredRootMatcher is O(depth) per event on the hot path
createIgnoredRootMatcher returns a closure that climbs the directory tree on every raw filesystem event (one dirname per path segment). For a typical monorepo with 15 levels of nesting and node_modules ignored, every event under src/ results in ~15 string comparisons and allocations. On Linux/portable backends the ignored matcher is called by Chokidar's own traversal as well, so events within deep trees are charged twice. Depth is bounded in practice, but the allocation-per-level cost is worth noting if high-frequency saves in deeply nested directories prove noisy. A path-prefix check (current.startsWith(root + sep)) would avoid the inner loop entirely for roots that share a common prefix with the event path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/watchObserver.ts
Line: 52-62
Comment:
**Ancestor-walk in `createIgnoredRootMatcher` is O(depth) per event on the hot path**
`createIgnoredRootMatcher` returns a closure that climbs the directory tree on every raw filesystem event (one `dirname` per path segment). For a typical monorepo with 15 levels of nesting and `node_modules` ignored, every event under `src/` results in ~15 string comparisons and allocations. On Linux/portable backends the ignored matcher is called by Chokidar's own traversal as well, so events within deep trees are charged twice. Depth is bounded in practice, but the allocation-per-level cost is worth noting if high-frequency saves in deeply nested directories prove noisy. A path-prefix check (`current.startsWith(root + sep)`) would avoid the inner loop entirely for roots that share a common prefix with the event path.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
I think a follow up would be to document the watch* files more (like at the top of the file). And maybe some guidance for how other VCSs can tap in. |
2 participants
Summary
Replace
--watch's 250 ms Git polling loop with filesystem event hints plus the existing authoritative Git signature. Watch remains continuous—passively viewed sessions stay fresh—while idle sessions stop spawning several Git commands four times per second.PR #520 identified the same important unattended-session cost and proposed pausing polling after UI inactivity. This PR takes a different approach that preserves watch semantics and keeps filesystem observation independent of keyboard and mouse activity.
Why a hybrid watcher
fs.watch, where the OS can observe a whole tree with a small constant number of registrations.This is a standard shape for development tooling: Turbopack similarly uses efficient recursive observation on macOS/Windows and selective non-recursive observation on Linux. Git remains Hunk's source of truth; events only tell Hunk when it may be worth recomputing the signature.
A 2-second readiness deadline and
ENOSPC/EMFILEhandling close partial observers and fall back to a safe 2-second poll. A healthy observer retains a 10-second safety check for missed events or unusual filesystems.macOS and Linux benchmark summary
The current benchmark report compares exact compiled base and candidate SHAs with Bun 1.3.14 across a little repo (1,260 subdirectories) and big repo (25,223 subdirectories).
All measured refresh trials were correct for tracked writes, atomic saves, and relevant untracked creation. Candidate p95 remained below 610 ms. The report contains the frozen provenance, raw-artifact checksums, full startup/idle/latency tables, and disclosed Linux recovery evidence.
Windows status
Windows unit and focused watch tests pass, and native/Chokidar observer probes both reach ready. We were not able to complete the full TUI benchmark matrix yet:
Windows performance is therefore not inferred from macOS, and no Windows startup, CPU, memory, or latency claim is made. Follow-up work will fix the Windows TUI harness and run the same two-fixture campaign for both the native production path and forced Chokidar comparison.
How it works
setIntervalpath is removed from the UI.Benchmark harness
The WIP harness lives on
elucid/watch-benchmark-harness. It freezes source/binary provenance, builds portable directory-heavy fixtures, runs 120-second idle cohorts and TUI latency trials, and retains raw JSON/log artifacts. The campaign is intentionally too heavy for normal CI; once the Windows blockers are resolved, the branch will be adjusted for manual/ad-hoc runs rather than every PR push.Scope and verification
chokidar@^4.0.3as a bundled runtime dependency and includes patch changesets.