Skip to content

fix(skill-registry): contain asynchronous watcher errors - #1817

Merged
decode2 merged 2 commits into
mainfrom
fix/962-skill-watcher-crash
Oct 6, 2026
Merged

decode2 merged 2 commits into
mainfrom
fix/962-skill-watcher-crash

Conversation

@decode2

@decode2 decode2 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Linked issue

Closes #962

PR type

  • Bug fix (type:bug)

Summary

  • Handle asynchronous errors from recursive skill watchers instead of allowing an unhandled error event to terminate Pi.
  • Close and unregister only the failed watcher; warn with the existing /skill-registry:refresh manual continuation. Other skill roots remain watched.
  • Add child-process regressions for ENOENT from Node's real Linux recursive rescan boundary and asynchronous EMFILE, including refresh, shutdown, and restart.

The earlier attempt #963 was closed without merging. This change retains the minimal listener fix and adds reproducible runtime regression evidence. No dependencies, global exception handlers, retry state, or configuration flags are added.

Changes

File Change
extensions/skill-registry.ts Contain asynchronous watcher errors, release the failed handle, and warn.
tests/skill-registry-watcher.test.ts Exercise the registered lifecycle in isolated child processes with real filesystem watchers.

Test plan

  • RED on unchanged main: both new child-process tests terminate with unhandled error events; Linux ENOENT stack matches the reported recursive_watch boundary.
  • GREEN: node --experimental-strip-types --test tests/skill-registry-watcher.test.ts tests/skill-registry.test.ts — 35 passed, no failures or skips, on Node 24.18.0 and 24.21.0.
  • pnpm test — 4,953 passed, 44 skipped, no failures; provider contract and runtime harness passed.
  • pnpm run typecheck — no diagnostic regressions.
  • pnpm run check:runtime-modules and node scripts/verify-package-files.mjs passed.
  • pnpm run test:packed-package passed.
  • Native reliability review approved and acknowledged; separate read-only verification found no blockers.
  • git diff --check clean.

Linux ENOENT is fault-injected at Node's actual readdirSync boundary after watcher startup. Node 24.21.0 now swallows that rescan error itself; after verifying the native rescan was reached, the test additionally emits the public ENOENT event only when Node ignored it. Against unchanged main both cases still fail on Node 24.21.0, so the listener regression remains enforced. On Darwin/Windows the test injects the public watcher error event; those platforms were not executed locally. Pending debounce callbacks after shutdown are pre-existing and outside this fix.

Contributor checklist

  • Linked approved issue and exactly one matching type label.
  • Conventional commit; no attribution trailers.
  • No shell script changes; shellcheck is not applicable.
  • Existing skill indexing remains covered by the focal suite; no skill content changes.
  • Scope and degraded manual-refresh behavior documented above; no new configuration.

Rollback

Revert this single commit (the listener and its dedicated regression file). No unrelated changes are included. Authored scope: 124 added lines, including a seven-line runtime fix.

Summary by CodeRabbit

  • Bug Fixes
    • Skill registry watchers now handle asynchronous errors by stopping the failed watcher and notifying you that manual refresh is available. Other watchers can continue updating the registry.
  • Tests
    • Added coverage for watcher failures, manual refresh, continued updates from other watchers, and restarting watchers in a later session.

CI follow-up evidence

The initial CI run used Node 24.21.0 and exposed the newer ENOENT suppression. The test was corrected without changing runtime logic. The full suite on Node 24.21.0 passed on repeat (4,953 passed, 44 skipped); its first local run hit an unrelated orchestrator-consultation-sdk Git-probe timing assertion, which passed standalone and on the full repeat. Both candidate reviews and independent read-only checks passed.

@decode2 decode2 added the type:bug Bug fix label Oct 6, 2026
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e0dbacd2-92b7-4f0b-880e-24b0064516fa
📥 Commits

Reviewing files that changed from the base of the PR and between 1ed283d and 8a14b63.

📒 Files selected for processing (1)
  • tests/skill-registry-watcher.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The skill registry now handles asynchronous watcher errors by closing and removing the failed watcher and warning that manual refresh is available. Subprocess tests cover ENOENT and EMFILE errors, continued updates, manual refresh, shutdown, and watcher restart.

Changes

Skill registry watcher error handling

Layer / File(s) Summary
Handle watcher errors and validate recovery
extensions/skill-registry.ts, tests/skill-registry-watcher.test.ts
Each watcher now handles errors by closing and removing itself from the active watcher set, then warning that automatic updates stopped and manual refresh is available. Subprocess tests cover ENOENT and EMFILE failures, continued updates from the other root, manual refresh, shutdown, and starting both watchers in a later session.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 8a14b

Watcher errors no longer terminate Pi. The affected root may stop updating automatically until a later session, but users are warned and can refresh manually; no actionable merge-blocking risk is established.

Architecture Summary

Architecture risk: 🔵 Low · up to 8a14b

The change affects 2 systems.

Changed systems: extensions, tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — extensions (service) was modified; 1 changed file maps to changed impact.
  • observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in extensions/skill-registry.ts: Each watcher now has an error handler that closes it, removes it from activeWatchers, and notifies that automatic updates stopped and manual refresh is available.
  • observed — Modified behavior in tests/skill-registry-watcher.test.ts: Adds imports and resolves the skill-registry extension URL for the subprocess test.
  • observed — Modified behavior in tests/skill-registry-watcher.test.ts: Adds subprocess coverage for ENOENT and EMFILE watcher failures. ENOENT is injected at the recursive readdir boundary except on Darwin and Windows, with a direct watcher error event as a fallback if Node does not report it; Darwin and Windows use the event directly, as does EMFILE on all platforms. The test checks that the failed watcher stops while the other root continues updating the registry, manual refresh reflects a user-skill change, and shutdown clears watchers before a later session starts both again.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed [#962] requires asynchronous recursive-watch errors not to terminate Pi and requires skill watching to remain best-effort. extensions/skill-registry.ts now handles each watcher's error event, clos…
Out of Scope Changes check ✅ Passed The runtime change and new child-process regression tests are limited to asynchronous skill-watcher error handling and directly support [#962]. No unrelated changes appear in the whole-PR diff.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: containing asynchronous errors from skill-registry watchers.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@decode2
decode2 merged commit c806cf3 into main Oct 6, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(skill-registry): recursive skill watcher crashes Pi with uncaughtException ENOENT when a watched skill subdirectory is removed

1 participant