Fall back to polling-based file watching on inotify/FD exhaustion - #821
Merged
Conversation
Contributor
|
Reviewed; no blockers found. |
kriszyp
added a commit
that referenced
this pull request
May 27, 2026
Per Claude review on #821: only OptionsWatcher had _simulateWatcherErrorForTests / _usingPollingForTests hooks and a polling-fallback test block, leaving EntryHandler's and RootConfigWatcher's recovery paths uncovered. EntryHandler in particular has a structurally different async flow (void this.#watch() with an internal #closed check, vs the explicit .close().catch().finally() chain in the other two watchers), so the close-during-recovery guard wasn't exercised at all. Adds the same hooks + a 4-case polling-fallback describe block (basic fallback, non-exhaustion passthrough, repeated exhaustion errors, close- during-recovery) to both watchers, mirroring the OptionsWatcher coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
May 27, 2026
Per Claude review on #821: `void this.#watch()` had no rejection handler. If `await this.#watcher?.close()` inside #watch() rejects (plausible under the same FD/inotify pressure that triggered the fallback), Node treats it as an unhandled rejection. OptionsWatcher and RootConfigWatcher already had `.catch(() => {})` on their equivalent close chains — bring EntryHandler to parity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
May 27, 2026 13:02
When the host hits its inotify watch limit (ENOSPC) or open-file-descriptor limit (EMFILE), chokidar's native watchers fire an error and stop emitting change events. Harper's three file watchers (EntryHandler, OptionsWatcher, RootConfigWatcher) now detect those exhaustion errors and transparently re-open the affected watcher with `usePolling: true` — polling-based watching doesn't consume per-watch inotify handles or file descriptors, so the system continues to function in a degraded but functional state until the operator raises the OS limit. A one-time process-wide warning is logged on first fallback, noting both the sysctl knobs and the CPU trade-off of polling. Subsequent exhaustion errors in the same watcher are silently swallowed to avoid a flurry of duplicate emissions during recovery. The reopen is guarded against a close() race so a watcher that's been torn down mid-fallback doesn't get a replacement installed behind the consumer's back. Closes harper#488. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Claude review on #821: only OptionsWatcher had _simulateWatcherErrorForTests / _usingPollingForTests hooks and a polling-fallback test block, leaving EntryHandler's and RootConfigWatcher's recovery paths uncovered. EntryHandler in particular has a structurally different async flow (void this.#watch() with an internal #closed check, vs the explicit .close().catch().finally() chain in the other two watchers), so the close-during-recovery guard wasn't exercised at all. Adds the same hooks + a 4-case polling-fallback describe block (basic fallback, non-exhaustion passthrough, repeated exhaustion errors, close- during-recovery) to both watchers, mirroring the OptionsWatcher coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Claude review on #821: `void this.#watch()` had no rejection handler. If `await this.#watcher?.close()` inside #watch() rejects (plausible under the same FD/inotify pressure that triggered the fallback), Node treats it as an unhandled rejection. OptionsWatcher and RootConfigWatcher already had `.catch(() => {})` on their equivalent close chains — bring EntryHandler to parity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/watcher-enospc-polling-fallback
branch
from
June 1, 2026 12:23
0dfd05d to
e58ad79
Compare
The previous assertion (no 'add' event within 500ms) couldn't distinguish a working close-during-fallback guard from a regression that installed a polling watcher: DIRECTORY_POLLING_FALLBACK_OPTIONS.interval is 3000ms, so the 500ms window would pass either way. Mirror the OptionsWatcher / RootConfigWatcher pattern: expose _openCountForTests on EntryHandler and assert it stays at 1 across the close()-during-recovery race. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pbrumblay
pushed a commit
to pbrumblay/harper
that referenced
this pull request
Jun 26, 2026
Component deploys (extractApplication + npm install) write into a directory that every Scope's EntryHandler is watching. Without coordination, each intermediate file change fires scope.requestRestart() and drives a watcher teardown/recreate cycle through componentLoader — briefly doubling inotify occupancy and amplifying the exhaustion risk that motivated harper#488. Add a cross-thread deploy lifecycle: - `components/deployLifecycle.ts` — module emitter with ref-counted in-flight state, plus `broadcastDeployStart`/`broadcastDeployEnd` helpers that propagate via `manageThreads.broadcastWithAcknowledgement` (start) and `manageThreads.broadcast` (end). Receiver installed at module load so worker threads react to events originating on main. - `components/Application.ts` — `prepareApplication` wraps extract + install in `broadcastDeployStart` / `finally broadcastDeployEnd`. Best-effort: a broadcast failure doesn't block the deploy. - `components/Scope.ts` — each Scope subscribes to the deploy emitter for its own component name. On deploy:start, pauses all EntryHandlers and sets an in-flight flag that suppresses requestRestart(). On deploy:end, resumes the EntryHandlers (which re-scan and fire add events that collapse into a single coalesced restart via the existing debounce). Exposes `deploy:start` / `deploy:end` events on the scope so plugins can observe. - `components/EntryHandler.ts` — adds `pause()` / `resume()`. pause() closes the chokidar watcher (releasing inotify) but preserves the EntryHandler EventEmitter and its listener attachments, so plugins registered via `scope.handleEntry(handler)` survive the pause. resume() awaits the pause-initiated close before installing a new watcher to avoid overlapping teardown/setup under inotify pressure. Refs harper#488 (third of three independent mitigations, after HarperFast#809 and HarperFast#821). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes harper#488.
When chokidar's native watchers hit
ENOSPC(inotify limit) orEMFILE(file descriptor limit), they emit an error and stop firing change events. Harper's three watcher classes —EntryHandler(per-component file tree),OptionsWatcher(per-component config.yaml),RootConfigWatcher(root harperdb-config.yaml) — now detect those exhaustion errors and transparently re-open the affected watcher withusePolling: true. Polling doesn't consume per-watch inotify handles or file descriptors, so Harper continues to function (with somewhat higher CPU and longer change-detection latency) until the operator raises the OS limit.Why
The original symptom in #488 was an
ENOSPCthrown from anOptionsWatchertrying to add a single config-file watch — the system-wide watch pool was already exhausted by the time it tried. Increasingfs.inotify.max_user_instancesresolves the immediate failure, but Harper was effectively dead at that point: subsequent watcher activity was unrecoverable without restart. This PR makes that scenario self-healing.This is one of three independent mitigations for #488 (alongside PR #809 which tightens the ignore list, and a forthcoming PR that suppresses auto-reload during deploy).
Where to look
utility/watcherFallback.ts— shared module:isWatcherExhaustionError,POLLING_FALLBACK_OPTIONS(1s/2s for single-file watchers),DIRECTORY_POLLING_FALLBACK_OPTIONS(3s/5s for tree watchers, to bound CPU),warnWatcherFallback(process-wide one-time warning that includes the OS sysctl knobs and notes the CPU trade-off).#openWatcher()(or reuses existing#watch()forEntryHandler)#usingPollingand#closedflags to make the close→reopen sequence race-safeCross-model review notes
#usingPolling, subsequent exhaustion errors during the same recovery cycle were still being emitted to consumers. Now swallowed.close()is called while the failed-watcherclose()is still pending, thefinallycallback was reopening a watcher after teardown. Now guarded by#closed.setTimeoutto assert "no event fired" — replaced with a deterministic_openCountForTestsgetter check (test now 102ms).void .close().finally(...)could surface unhandled promise rejections — added.catch(() => {})to swallow teardown rejections (these aren't actionable on an already-failed watcher).Potential concerns to focus on
_simulateWatcherErrorForTests/_usingPollingForTests(and_openCountForTestsonOptionsWatcher). They're underscore-prefixed and*ForTests-suffixed. Open to renaming if there's an existing Harper convention.fs.statcalls themselves can't acquire an FD, we'd silently miss changes until pressure subsides. The alternative (re-emitting after first warn) seems noisier than helpful given the operator already has the actionable info from the first warning.