Analyzed and fixed fs.watch error crash

Summary

Closed, Completed

Edit
Added in-progress label to GitHub issue.
Edit
Understand
Reviewed code and verified root cause independently.
Understand
Build
Created small test to reproduce the bug.
Build
Edit
Drafted and committed fix for the issue.
Edit
Verify
Pushed changes and confirmed issue closure.
Verify

Analyze GitHub issue(s): https://github.com/badlogic/pi-mono/issues/2791

For each issue:

Add the inprogress label to the issue via GitHub CLI before analysis starts. If adding the label fails, report that explicitly and continue.
Read the issue in full, including all comments and linked issues/PRs.
Do not trust analysis written in the issue. Independently verify behavior and derive your own analysis from the code and execution path.
For bugs:
- Ignore any root cause analysis in the issue (likely wrong)
- Read all related code files in full (no truncation)
- Trace the code path and identify the actual root cause
- Propose a fix
For feature requests:
- Do not trust implementation proposals in the issue without verification
- Read all related code files in full (no truncation)
- Propose the most concise implementation approach
- List affected files and changes needed

Do NOT implement unless explicitly asked. Analyze and propose only.

Label added. Now let me read the actual source file to verify the issue independently.

Readtheme.ts

Now I've read the full file. Let me verify the issue independently.

Looking at startThemeWatcher() (around line 420-470):

typescript

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        if (currentThemeName !== watchedThemeName) {            return;        }        if (!filename) {            scheduleReload();            return;        }        const changedFile = String(filename);        if (changedFile !== watchedFileName) {            return;        }        scheduleReload();    });} catch (_error) {    // Ignore errors starting watcher}

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        if (currentThemeName !== watchedThemeName) {            return;        }        if (!filename) {            scheduleReload();            return;        }        const changedFile = String(filename);        if (changedFile !== watchedFileName) {            return;        }        scheduleReload();    });} catch (_error) {    // Ignore errors starting watcher}

The problem is clear: fs.watch() returns an FSWatcher that's an EventEmitter, but the try/catch only handles synchronous errors during the initial call. If the watcher later encounters an error—like the directory being deleted or an OS handle becoming invalid—it emits an error event. Without a handler for that event, Node.js crashes the process with an unhandled exception.

The fix is straightforward: add an error event listener...

Now I should scan the rest of the codebase to see if there are other fs.watch calls that might have the same vulnerability.

Let me check if there are other fs.watch usages with the same pattern in the codebase.

The example file has the same pattern (no .on('error') handler), but it's just an example, not production code.

Analysis

Issue: #2791 — fs.watch error event crashes all pi sessions simultaneously

Type: Bug

Independently verified: Yes. The issue is real and the root cause analysis in the issue is correct.

Root Cause

In packages/coding-agent/src/modes/interactive/theme/theme.ts, the startThemeWatcher() function (lines ~420-470) creates an fs.watch() handle on the custom themes directory. The code wraps the call in try/catch, but this only catches synchronous errors from fs.watch() itself.

fs.watch() returns an FSWatcher (an EventEmitter). Runtime errors (directory renamed/deleted, OS handle invalidation, handle limit exhaustion) are emitted asynchronously as error events. Without an .on('error', ...) handler, Node.js treats these as uncaught exceptions and terminates the process.

The specific code:

typescript

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        // ... callback ...    });    // No .on('error', ...) handler} catch (_error) {    // Only catches synchronous errors}

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        // ... callback ...    });    // No .on('error', ...) handler} catch (_error) {    // Only catches synchronous errors}

Proposed Fix

Add an error event handler to themeWatcher immediately after creation, inside the try block. On error, close the watcher and set it to undefined (theme hot-reload becomes unavailable, but pi stays alive):

typescript

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        // ... existing callback ...    });    themeWatcher.on("error", () => {        try { themeWatcher?.close(); } catch { /* ignore */ }        themeWatcher = undefined;    });} catch (_error) {    // Ignore errors starting watcher}

try {    themeWatcher = fs.watch(customThemesDir, (_eventType, filename) => {        // ... existing callback ...    });    themeWatcher.on("error", () => {        try { themeWatcher?.close(); } catch { /* ignore */ }        themeWatcher = undefined;    });} catch (_error) {    // Ignore errors starting watcher}

Affected Files

packages/coding-agent/src/modes/interactive/theme/theme.ts — single change in startThemeWatcher()

Impact

Without fix: any async watcher error crashes all concurrent pi sessions
With fix: watcher silently stops; theme hot-reload is lost until the next initTheme()/setTheme() call, but all sessions remain alive

can you reproduce this in a small test?

The user wants me to reproduce the bug in a small test. Let me think about how to do this.

The core issue is that fs.watch() returns an FSWatcher EventEmitter, and if it emits an error event without a handler, Node.js crashes the process.

I can write a test that:

Calls startThemeWatcher() (or the relevant code path)
Triggers an error event on the watcher
Verifies the process would crash (or with the fix, wouldn't crash)

Actually, the simplest approach would be to directly test the behavior: create an fs.watch on a directory, then programmatically emit an error on it, and see what happens.

But I need to consider the test infrastructure for this codebase. The themeWatcher variable is module-private, so I can't directly access it from a test. I could either write a standalone reproduction script that demonstrates the bug, or create a test that exercises the theme initialization code path with a custom theme and then triggers a directory error. Let me explore the second approach by setting up a test scenario that exercises the relevant code paths. Looking at the test structure in AGENTS.md, I need to put regression tests under packages/coding-agent/test/suite/regressions/ with the naming convention <issue-number>-<short-slug>.test.ts. The challenge is that themeWatcher isn't exported, so I'll need to either mock getCustomThemesDir and trigger the directory deletion to test the error handling, or find another way to access the watcher instance for verification.

The simplest approach is to spy on fs.watch to capture the FSWatcher instance, then manually emit an error event on it to verify the process handles it gracefully without crashing.

Let me look at the test infrastructure to understand how to write this properly.

Readtheme-export.test.ts

Good, now I understand the test patterns. Let me also check how getCustomThemesDir works to know how to set up the environment.

Now let me look at a dark.json theme to use as a template for the test:

Now let me write the regression test:

Can't spy on ESM modules. Let me take a different approach. Instead of spying on fs.watch, I can:

Set up the theme with the watcher enabled
Then directly remove the watched directory to trigger an error event on the watcher
Or, I can use a different approach: intercept via process 'uncaughtException' handling

Actually, the simplest approach is to not try to capture the watcher at all. Instead:

Set up the custom theme and enable the watcher
Delete the watched directory (which should trigger an error on the FSWatcher on most platforms)
Listen for uncaughtException events on the process to detect if the error crashes it

A subprocess approach would be more reliable—spawn a child process that sets up the theme, creates the watcher, deletes the directory, and then check its exit code to determine if the bug is present.

Actually, I'm overcomplicating this. The real issue is whether the themeWatcher FSWatcher has an error handler attached, so I can verify that directly by checking the listeners on the process or by examining the watcher's error handlers after setup. I can access the active handles in the process using process._getActiveHandles() to locate the FSWatcher, then emit an error directly on it to test if the error handling works. But actually, the child process approach is more reliable for testing whether the process crashes or survives an error.

Can't spy on ESM namespace. Let me use a child process approach instead, which is actually more faithful to the bug (it tests whether the process actually crashes).

Exit code 3 means ERROR_THROWN - the error was thrown when fsWatcher.emit("error", ...) was called. This confirms the bug: there's no error handler, so emit("error") throws.

Actually wait, the emit("error") in EventEmitter: when there are no error listeners, EventEmitter.emit("error") throws the error. The try/catch in the script caught it. So exit code 3 and "ERROR_THROWN" means: confirmed, no error handler is registered.

The test is working correctly - it's demonstrating the bug.

Now I want to make the test output clearer and verify that stderr also shows the expected error message. I'll refine the test slightly so it's ready to pass once the fix is applied, and I should capture stderr to confirm the error details are being logged correctly.

The test confirms the bug: exit code 3 means emit("error") threw because there's no error handler. The test is working correctly: it fails with the current code (bug present) and will pass once the fix is applied.

Let me clean up the test slightly to make the failure output clearer:

Test works correctly. The failure message is clear:

This confirms the bug: the FSWatcher from fs.watch() has zero error listeners, so any async error event crashes the process.

Summary:

Test file: packages/coding-agent/test/suite/regressions/2791-fswatch-error-crash.test.ts
How it works: Spawns a child process that sets up the theme watcher, finds the FSWatcher handle, and emits a synthetic error event on it. Without an error handler, EventEmitter.emit('error') throws/crashes. With a handler, the child exits cleanly.
Current result: Fails (exit 1), confirming the bug
After fix: Will pass (exit 0)

The fix remains the same one-liner addition in startThemeWatcher() in theme.ts:

typescript

themeWatcher.on("error", () => {    try { themeWatcher?.close(); } catch { /* ignore */ }    themeWatcher = undefined;});

themeWatcher.on("error", () => {    try { themeWatcher?.close(); } catch { /* ignore */ }    themeWatcher = undefined;});

ok fix it

Readtheme.ts20 lines

Test passes. Now run the full check:

All checks pass. Now update the changelog:

ReadCHANGELOG.md30 lines

Run the test one more time to confirm everything is solid:

All done. Here's a summary of the changes:

Files modified:

packages/coding-agent/src/modes/interactive/theme/theme.ts — Added .on("error", ...) handler to the FSWatcher in startThemeWatcher(). On error, the watcher is closed and set to undefined. Theme hot-reload stops, but pi stays alive.
packages/coding-agent/test/suite/regressions/2791-fswatch-error-crash.test.ts — New regression test. Spawns a child process that sets up the theme watcher, finds the FSWatcher handle, and emits a synthetic error event. Verifies the process survives instead of crashing.
packages/coding-agent/CHANGELOG.md — Added entry under [Unreleased] > Fixed.

Wrap it.

Additional instructions:

Determine context from the conversation history first.

Rules for context detection:

If the conversation already mentions a GitHub issue or PR, use that existing context.
If the work came from /is or /pr, assume the issue or PR context is already known from the conversation and from the analysis work already done.
If there is no GitHub issue or PR in the conversation history, treat this as non-GitHub work.

Unless I explicitly override something in this request, do the following in order:

Add or update the relevant package changelog entry under ## [Unreleased] using the repo changelog rules.
If this task is tied to a GitHub issue or PR and a final issue or PR comment has not already been posted in this session, draft it in my tone, preview it, and post exactly one final comment.
Commit only files you changed in this session.
If this task is tied to exactly one GitHub issue, include closes #<issue> in the commit message. If it is tied to multiple issues, stop and ask which one to use. If it is not tied to any issue, do not include closes # or fixes # in the commit message.
Check the current git branch. If it is not main, stop and ask what to do. Do not push from another branch unless I explicitly say so.
Push the current branch.

Constraints:

Never stage unrelated files.
Never use git add . or git add -A.
Run required checks before committing if code changed.
Do not open a PR unless I explicitly ask.
If this is not GitHub issue or PR work, do not post a GitHub comment.
If a final issue or PR comment was already posted in this session, do not post another one unless I explicitly ask.

Let me review what happened in this session:

Analyzed GitHub issue #2791
Added inprogress label
Wrote a regression test
Fixed the bug in theme.ts
Updated CHANGELOG.md

The changelog is already updated. I need to:

✅ Changelog already updated
Post a final comment on the issue
Commit the files I changed
Include closes #2791 in the commit
Check branch and push

Let me check the branch first, then draft the comment.

On main. My files are: theme.ts, CHANGELOG.md, and the new test file. The other changes are from other work.

Now draft the issue comment:

>cat > /tmp/2791-comment.md << 'EOF'
Fixed. Added `.on("error", ...)` to the `FSWatcher` in `startThemeWatcher()`. On error, the watcher closes itself and sets the handle to `undefined`. Theme hot-reload stops but pi stays alive.

Regression test in `test/suite/regressions/2791-fswatch-error-crash.test.ts` spawns a child process, finds the `FSWatcher` handle, and emits a synthetic error event to verify the process survives.
EOF
cat /tmp/2791-comment.md

Looks good. Posting and committing:

Only my 3 files staged.

Pushed. Issue #2791 will be auto-closed by the commit.