fix(runtime): prevent file watcher SIGSEGV from crashing orca serve#8370
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces worker-thread runtime file watching with supervised Parcel watcher child processes. It adds watcher IPC contracts, event batching with bounded metadata lookup and backpressure, cancellation and timeout handling, crash recovery, crash fuses, canary directories, and shared or quarantined supervisor pools. Runtime file-watch integration now supports shared root subscriptions, terminal errors, abort signals, recovery, and cleanup. RPC and renderer clients handle explicit error and end events. Tests and reliability evidence cover lifecycle, isolation, cancellation, recovery, and fault injection. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/ipc/filesystem-watcher.ts (2)
561-567: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
unsubscribe()doesn't abort the in-flight install like the other two cancellation paths.
closeLocalWatcherForWorktreePath(line 618) andcloseAllWatchers(line 949) both callinFlight.abortController.abort()right after settingcancelled = true. This function setsinFlight.cancelled = inFlight.listeners.size === 0(line 566) but never aborts the controller, so when the last listener disconnects normally, the pending native/forked watcher subscription keeps running to completion instead of being cancelled early — the exact wasted work this abort mechanism was introduced to avoid.♻️ Proposed fix
function unsubscribe(worktreePath: string, senderId: number): void { const rootKey = normalizeRootPath(worktreePath) const inFlight = inFlightLocalInstalls.get(rootKey) if (inFlight) { inFlight.listeners.delete(senderId) inFlight.cancelled = inFlight.listeners.size === 0 + if (inFlight.cancelled) { + inFlight.abortController.abort() + } }
521-527: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the abort signal into the WSL watcher path
createWslWatcherdoesn’t accept anAbortSignal, so cancelling a local install still lets the WSL snapshot subprocess keep starting until it settles or times out. ThreadcancelToken.abortController.signalthrough that path too.src/main/runtime/orca-runtime-files.ts (1)
922-955: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread the abort signal through the SSH watch path.
watchFileExploreralready receivessignalfrom the RPC layer, but the SSH branch drops it (provider.watch()/registerSshFilesystemWatch()), so an aborted SSH-backed watch still waits for the remotefs.watchrequest to finish before tearing down. The Windows path is intentionally the synchronousfs.watchbranch, so this only needs to be addressed for SSH.
🧹 Nitpick comments (5)
src/main/ipc/parcel-watcher-process-entry.test.ts (1)
276-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate union member in
callbacktype.The callback type union lists the identical
((err, events) => void)member twice (also repeated in the tests at ~331-333 and ~380-382). Collapse to a single member.♻️ Proposed cleanup
let callback: | ((err: Error | null, events: { type: string; path: string }[]) => void) - | ((err: Error | null, events: { type: string; path: string }[]) => void) | undefinedsrc/main/ipc/parcel-watcher-supervisor-subscribe.ts (1)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
VITESTbranch.Branching production subscription behavior on
process.env.VITESTis non-obvious; add a one-line comment explaining why tests use the in-process fallback (avoids forking a real watcher child).💬 Suggested comment
+ // Why: under Vitest we cannot fork a real watcher child, so exercise the + // subscription path in-process instead. if (process.env.VITEST) { return subscribeWithInProcessWatcher(dir, callback, opts, hooks) }As per coding guidelines: "When code is driven by a design document or non-obvious constraint, add a brief one- or two-line comment explaining why it behaves that way."
Source: Coding guidelines
src/main/ipc/runtime-watcher-process-pool.ts (2)
113-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
forgetRoothas no direct test coverage.
resetForTestand the constructor/assignment paths are exercised by the test file, butforgetRoot(which clearsisolatedRoots/failedQuarantineRootsfault history) isn't covered by any test. Given it mutates fault-tracking invariants thatassignmentForRootdepends on (thefailedQuarantineRootsfuse check on line 134), a regression here could silently let fused roots retry forever or vice versa.
113-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly teardown path for all pooled supervisors is named for tests only.
resetForTest()(public) is the sole mechanism that disposes every slot inallSlots. If any production code path (e.g. app shutdown, hot-reinitialization of the pool infile-watcher-host.ts) relies on this method to release pooled watcher child processes, the "-ForTest" naming is misleading and risks being missed/removed by a future refactor that assumes it's test-only. If it truly is test-only, consider whether production callers have a separate, real disposal path for this pool; if not, consider exposing a properly nameddisposeAll()/shutdown()alongside (or instead of)resetForTest().♻️ Possible approach
- resetForTest(): void { - for (const slot of this.allSlots) { - this.disposeSlot(slot) - } - this.activeSlots.clear() - this.allSlots.clear() - this.assignments.clear() - this.isolatedRoots.clear() - this.failedQuarantineRoots.clear() - } + /** Disposes every pooled supervisor and clears fault state. Safe to call from production shutdown paths. */ + disposeAll(): void { + for (const slot of this.allSlots) { + this.disposeSlot(slot) + } + this.activeSlots.clear() + this.allSlots.clear() + this.assignments.clear() + this.isolatedRoots.clear() + this.failedQuarantineRoots.clear() + } + + /** `@deprecated` use disposeAll(); kept for existing test call sites. */ + resetForTest(): void { + this.disposeAll() + }src/main/ipc/runtime-watcher-process-pool.test.ts (1)
40-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated pool-construction boilerplate across most tests.
The
new RuntimeWatcherProcessPool({ maxSharedSupervisors: 1, createSupervisor: () => { const supervisor = new FakeSupervisor(); supervisors.push(supervisor); return supervisor } })block is duplicated nearly verbatim inbeforeEach(lines 40-50) and 5 more tests (lines 75-82, 108-115, 146-156, 166-173, 195-202). Extracting a smallcreatePool(overrides)helper would reduce duplication and make future changes to the fake-supervisor wiring less error-prone.♻️ Suggested helper
function createPool(overrides: RuntimeWatcherProcessPoolOptions = {}): RuntimeWatcherProcessPool { return new RuntimeWatcherProcessPool({ maxSharedSupervisors: 4, createSupervisor: () => { const supervisor = new FakeSupervisor() supervisors.push(supervisor) return supervisor }, ...overrides }) }Then each test can call
pool = createPool({ maxSharedSupervisors: 1 }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43246895-3aea-43b7-839a-ac0fd7e53fa5
📒 Files selected for processing (34)
config/reliability-gates.jsoncconfig/scripts/runtime-file-watcher-fault-harness.mjselectron.vite.config.tssrc/main/ipc/filesystem-watcher-local-unsubscribe.test.tssrc/main/ipc/filesystem-watcher.tssrc/main/ipc/parcel-watcher-canary-directory.tssrc/main/ipc/parcel-watcher-child-launch.tssrc/main/ipc/parcel-watcher-crash-fuse.tssrc/main/ipc/parcel-watcher-event-delivery.test.tssrc/main/ipc/parcel-watcher-event-delivery.tssrc/main/ipc/parcel-watcher-host-subscriptions.tssrc/main/ipc/parcel-watcher-in-process-fallback.tssrc/main/ipc/parcel-watcher-pending-subscribe.tssrc/main/ipc/parcel-watcher-process-entry.test.tssrc/main/ipc/parcel-watcher-process-entry.tssrc/main/ipc/parcel-watcher-process-failure.tssrc/main/ipc/parcel-watcher-process-protocol.tssrc/main/ipc/parcel-watcher-process-subscription.tssrc/main/ipc/parcel-watcher-process-supervisor.tssrc/main/ipc/parcel-watcher-process.test.tssrc/main/ipc/parcel-watcher-process.tssrc/main/ipc/parcel-watcher-supervisor-subscribe.tssrc/main/ipc/runtime-watcher-process-pool.test.tssrc/main/ipc/runtime-watcher-process-pool.tssrc/main/runtime/file-watcher-host.test.tssrc/main/runtime/file-watcher-host.tssrc/main/runtime/file-watcher-worker.tssrc/main/runtime/orca-runtime-files-watch.test.tssrc/main/runtime/orca-runtime-files.test.tssrc/main/runtime/orca-runtime-files.tssrc/main/runtime/rpc/methods/files.test.tssrc/main/runtime/rpc/methods/files.tssrc/renderer/src/runtime/runtime-file-client.test.tssrc/renderer/src/runtime/runtime-file-client.ts
💤 Files with no reviewable changes (2)
- src/main/runtime/file-watcher-worker.ts
- electron.vite.config.ts
Replace the worker-thread runtime file watcher with a forked, crash-isolated @parcel/watcher child process pool so a native FSEvents fault can no longer take down the main/serve process, and add bounded event batching, delivery backpressure, and quarantine-based recovery for faulty watch roots.
- Fault harness could throw before mkdtemp/realpath completed, skipping cleanup; now tracks each temp path independently and races an async watcher-callback error so it can't escape the try/finally unhandled. - In-process fallback swallowed unsubscribe failures via a bare rejection handler that could still throw; use .catch() instead. - Watcher process entry's cancel-subscribe handler now reuses the async unsubscribe path when a crawl already finished, releasing the native handle instead of leaking it (blocks worktree unlock on Windows). - Runtime watcher process pool exposed no real dispose(); shutdown now kills pooled children so they don't outlive the main process.
4ea16a2 to
9198b01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9cffd8e8-3f32-43f5-9a4a-46c664c5ee94
📒 Files selected for processing (32)
config/reliability-gates.jsoncconfig/scripts/runtime-file-watcher-fault-harness.mjselectron.vite.config.tssrc/main/ipc/filesystem-watcher-local-unsubscribe.test.tssrc/main/ipc/filesystem-watcher.tssrc/main/ipc/parcel-watcher-canary-directory.tssrc/main/ipc/parcel-watcher-child-launch.tssrc/main/ipc/parcel-watcher-crash-fuse.tssrc/main/ipc/parcel-watcher-event-delivery.test.tssrc/main/ipc/parcel-watcher-event-delivery.tssrc/main/ipc/parcel-watcher-host-subscriptions.tssrc/main/ipc/parcel-watcher-in-process-fallback.tssrc/main/ipc/parcel-watcher-pending-subscribe.tssrc/main/ipc/parcel-watcher-process-entry.test.tssrc/main/ipc/parcel-watcher-process-entry.tssrc/main/ipc/parcel-watcher-process-failure.tssrc/main/ipc/parcel-watcher-process-protocol.tssrc/main/ipc/parcel-watcher-process-subscription.tssrc/main/ipc/parcel-watcher-process-supervisor.tssrc/main/ipc/parcel-watcher-process.test.tssrc/main/ipc/parcel-watcher-process.tssrc/main/ipc/parcel-watcher-supervisor-subscribe.tssrc/main/ipc/runtime-watcher-process-pool.test.tssrc/main/ipc/runtime-watcher-process-pool.tssrc/main/runtime/file-watcher-host.test.tssrc/main/runtime/file-watcher-host.tssrc/main/runtime/file-watcher-worker.tssrc/main/runtime/orca-runtime-files-watch.test.tssrc/main/runtime/orca-runtime-files.test.tssrc/main/runtime/orca-runtime-files.tssrc/main/runtime/rpc/methods/files.test.tssrc/main/runtime/rpc/methods/files.ts
💤 Files with no reviewable changes (2)
- src/main/runtime/file-watcher-worker.ts
- electron.vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- src/main/ipc/parcel-watcher-process-subscription.ts
- src/main/ipc/parcel-watcher-process-failure.ts
- src/main/ipc/parcel-watcher-event-delivery.test.ts
- src/main/ipc/parcel-watcher-canary-directory.ts
- src/main/ipc/parcel-watcher-process-protocol.ts
- src/main/runtime/rpc/methods/files.ts
- src/main/ipc/parcel-watcher-crash-fuse.ts
- src/main/runtime/orca-runtime-files.ts
- src/main/runtime/orca-runtime-files.test.ts
- config/scripts/runtime-file-watcher-fault-harness.mjs
- src/main/runtime/file-watcher-host.test.ts
- src/main/ipc/parcel-watcher-process.ts
- src/main/ipc/runtime-watcher-process-pool.test.ts
- src/main/ipc/parcel-watcher-pending-subscribe.ts
- src/main/ipc/parcel-watcher-event-delivery.ts
- src/main/ipc/parcel-watcher-process-entry.test.ts
- src/main/ipc/parcel-watcher-in-process-fallback.ts
- src/main/ipc/parcel-watcher-process.test.ts
- src/main/runtime/rpc/methods/files.test.ts
- src/main/ipc/parcel-watcher-process-entry.ts
- src/main/ipc/filesystem-watcher-local-unsubscribe.test.ts
- config/reliability-gates.jsonc
Remove the unnecessary array snapshot in dispose(): disposeSlot mutates allSlots by deleting the slot being visited, and deleting the in-progress element during Set iteration is well-defined, so the spread copy was dead weight left over from prior debugging.
- Local/WSL watcher installs and SSH fs.watch setup now honor the in-flight AbortSignal, so the last unwatch cancels a slow native subscribe or remote setup instead of waiting for it to finish. - Thread signal through IFilesystemProvider.watch and SSH-backed file explorer watches for the same early-cancel behavior.
- Add a bounded deadline for post-crash resubscription crawls so one stuck root quarantines instead of pinning its whole shard forever. - Report FSEvents overflow as recoverable so delivery continues after a dropped-events error instead of surfacing as terminal. - Make WSL watcher abort errors real DOMException instances so AbortSignal-based cancellation checks recognize them. - Rework SSH watch registration so ownership of the shared setup request (not just the first caller) decides teardown, preventing one caller's abort from cancelling another's shared watch and guaranteeing exactly one fs.unwatch per registration. - Reformat reliability-gates.jsonc arrays and refresh WSL/SSH coverage entries and evidence runs to match the above.
- The reliability gate and release workflows (mac, Linux) previously only exercised the crash-isolation harness under vanilla Node, which doesn't catch runtime differences in the actual Electron binary that ships to users. - Adds an `ELECTRON_RUN_AS_NODE=1 pnpm exec electron ...` run of the same harness alongside the existing Node run, so #8212's SIGSEGV-survival contract is proven against both runtimes before packaging.
…ve-exits-with-sigsegv-after-fsevents-eve
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/parcel-watcher-host-subscriptions.ts (1)
77-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSnapshot records before invoking terminal hooks.
reportWatcherTerminalErrorruns hooks synchronously. A supervisor-level hook can retire/dispose the supervisor and clearrecordswhile this loop is active, so later subscriptions may never receive the terminal error before the map is cleared. Iterate over a snapshot.Proposed fix
- for (const record of records.values()) { + for (const record of [...records.values()]) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6a0837fe-2e8d-4fe1-a3ab-b04c2110ff7e
📒 Files selected for processing (17)
.github/workflows/release-cut.yml.github/workflows/release-mac-build.ymlconfig/reliability-gates.jsoncconfig/scripts/runtime-file-watcher-fault-harness.mjssrc/main/ipc/filesystem-watcher-wsl.tssrc/main/ipc/parcel-watcher-host-subscriptions.tssrc/main/ipc/parcel-watcher-pending-subscribe.tssrc/main/ipc/parcel-watcher-process-entry.test.tssrc/main/ipc/parcel-watcher-process-subscription.tssrc/main/ipc/parcel-watcher-process-supervisor.tssrc/main/ipc/parcel-watcher-process.test.tssrc/main/ipc/runtime-watcher-process-pool.test.tssrc/main/ipc/runtime-watcher-process-pool.tssrc/main/providers/ssh-filesystem-provider-watch.tssrc/main/providers/ssh-filesystem-provider.test.tssrc/main/providers/ssh-filesystem-provider.tssrc/main/runtime/file-watcher-host.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/ipc/parcel-watcher-process-subscription.ts
- config/scripts/runtime-file-watcher-fault-harness.mjs
- src/main/ipc/parcel-watcher-pending-subscribe.ts
- src/main/ipc/parcel-watcher-process-entry.test.ts
- src/main/ipc/runtime-watcher-process-pool.ts
- src/main/ipc/parcel-watcher-process.test.ts
- src/main/runtime/file-watcher-host.ts
- src/main/ipc/runtime-watcher-process-pool.test.ts
Adds a contract test asserting release-cut.yml and release-mac-build.yml run the runtime-file-watcher-fault-harness after building and before publishing artifacts, so a regression in watcher process fault recovery fails release packaging instead of shipping silently.
Snapshot the records map before iterating, since onTerminalError hooks can dispose the supervisor and clear `records` mid-loop, causing a crash. Also update the matching test to assert against the shared buildParcelWatcherIgnoreOptions helper instead of a loose arrayContaining match.
Snapshot watcher records with Array.from instead of spread, since spread syntax over an iterator that's mutated mid-loop by onTerminalError hooks can produce inconsistent results.
Summary
@parcel/watcherchild processes so a native FSEvents fault cannot terminate Electron main or headlessorca serve.Root cause
The runtime watcher previously ran
@parcel/watcherin a worker thread. Worker threads isolate JavaScript execution but still share the host process, so a nativewatcher.nodeSIGSEGV or fail-fast during FSEvents delivery or teardown still kills the entireorca serveprocess and every connected agent session.A forked child provides the required fault boundary: process death releases native watcher state without running the crash-prone teardown inside the host, while the supervisor can replace the child and resubscribe live roots.
Recovery and resource bounds
Compatibility
fs.watchoverflow-refresh path.ELECTRON_RUN_AS_NODEresolution.Validation
Recorded on macOS:
pnpm exec vitest run --config config/vitest.config.ts \ src/main/ipc/parcel-watcher-process.test.ts \ src/main/ipc/runtime-watcher-process-pool.test.ts \ src/main/ipc/parcel-watcher-process-entry.test.ts \ src/main/ipc/parcel-watcher-event-delivery.test.ts \ src/main/ipc/filesystem-watcher-local-unsubscribe.test.ts \ src/main/runtime/file-watcher-host.test.ts \ src/main/runtime/rpc/methods/files.test.ts \ src/renderer/src/runtime/runtime-file-client.test.tsKnown gaps
orca servesession orapp.asarpath.Fixes #8212
Related: #5308, #7547