Skip to content

fix(listener): make file watcher setup disposal-safe - #3593

Open
GautamSharma99 wants to merge 1 commit into
letta-ai:mainfrom
GautamSharma99:fix/watch-file-disposal
Open

fix(listener): make file watcher setup disposal-safe#3593
GautamSharma99 wants to merge 1 commit into
letta-ai:mainfrom
GautamSharma99:fix/watch-file-disposal

Conversation

@GautamSharma99

Copy link
Copy Markdown

Summary

Fixes #3583.

A listener session could be disposed while detached watch_file setup was awaiting its dynamic filesystem imports. Because disposal only closed watchers already present in the active map, the setup could resume afterward, create a new fs.watch() handle, and retain the dead socket/session indefinitely.

This change moves watcher ownership into a dedicated lifecycle object with explicit active, pending, and disposed states.

What changed

  • Added a per-session disposed latch that is set before resource cleanup begins.
  • Track pending watcher setup synchronously, before detached work starts.
  • Check disposal and cancellation immediately after dependency loading, after watch() creates a resource, and after the error listener is registered.
  • Immediately close a watcher if disposal or unwatch races with synchronous resource creation.
  • Make watcher closure idempotent so disposal, unwatch, setup rollback, filesystem errors, and deletion cleanup cannot close the same handle more than once.
  • Guard watcher callbacks, debounce timers, asynchronous stat() completion, and file_changed delivery against disposed or replaced watchers.
  • Scope debounce entries to the watcher instance so late events from an old watcher cannot clear or replace a newer watcher timer for the same path.
  • Preserve explicit unwatch_file cancellation while setup is suspended.
  • Ref-count duplicate watch_file requests even when both arrive before asynchronous setup completes. This also prevents concurrent setup from creating and leaking two handles for the same path.
  • Keep createFileCommandSession() as the protocol owner while delegating watcher resource ownership to file-watch-session.ts through an injectable dependency loader.

Lifecycle guarantees

After dispose() begins:

  • a suspended setup cannot call watch();
  • a watcher created at the disposal boundary is closed immediately;
  • no new debounce timer is created;
  • a pending stat() result cannot emit file_changed;
  • repeated disposal or late watcher errors do not double-close handles.

The dependency loader is injectable only at the session boundary, which lets the race windows be tested deterministically without module-level mocks or real OS watchers.

Tests

Added deterministic coverage for:

  • disposal while dependency loading is suspended;
  • disposal synchronously at the watch() creation boundary;
  • suppression of file_changed when disposal races with stat() completion;
  • normal rounded modification-time delivery for an active watcher;
  • explicit unwatch during suspended setup;
  • duplicate request ref-counting during setup;
  • idempotent closure across repeated disposal and late error callbacks;
  • public createFileCommandSession() wiring from disposal to watcher close and socket-send suppression.

Validation completed:

  • bun test src/websocket/listener/file-watch-session.test.ts src/websocket/listener/file-commands.test.ts — 12 passed, 0 failed
  • bun run check — all 12 checks passed

Structural cleanup

Extracting watcher ownership reduced src/websocket/listener/file-commands.ts from 1,030 to 949 lines. The file is now below the repository limit, so its oversized source baseline entry is removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Detached watch_file setup can outlive session disposal and leak a watcher

2 participants