Skip to content

refactor(filesystem): serve both kinds of watching through one interface - #17944

Draft
EhabY wants to merge 3 commits into
eclipse-theia:masterfrom
EhabY:refactor/unified-watcher-interface
Draft

refactor(filesystem): serve both kinds of watching through one interface#17944
EhabY wants to merge 3 commits into
eclipse-theia:masterfrom
EhabY:refactor/unified-watcher-interface

Conversation

@EhabY

@EhabY EhabY commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

What it does

Follow-up to #17875, restructuring what it added. Last of the stack (#17943, then #17875, then this); it shows their commits until they merge, so review only the last commit, with copy detection on so the file split does not read as an unrelated add and delete:

git diff -C --find-copies-harder HEAD~1...HEAD

The raw +1384/−980 overstates the diff: with copy detection it is ~1000 effective lines. parcel-watcher.ts pairs with the old parcel-filesystem-service.ts at ~307 shown lines, that file's remaining 592 deletions are the move's mirror image, and about half of the ~525 lines of new structure are logic extracted from the watcher rather than written new.

The service told the two watchers apart by hand: an instanceof to release a request, a client id on one path and a watcher id on the other, key prefixes to share one map, parallel create and key methods. Now both watchers implement FileSystemWatcher, a WatcherProvider per kind owns its key, factory and sharing map, and picking one by recursive is all the service knows about the difference.

The service moves out of parcel-filesystem-service.ts, which becomes a deprecated re-export so published names keep resolving until the next major. Platform and file system calls live in WatcherHost, request routing in WatchRequestRouter; neither touches the event machine, and tests drive the real watcher with the host substituted.

Defects found while auditing the feature against VS Code, fixed here:

  • a queued task that threw left the change queue rejected with no handler, ending the watcher process, and swallowed the task behind it
  • a change naming a file the initial snapshot had struck out reported an update and never recorded the name, so a later deletion announced a file that had never appeared
  • excludes applied to the path a client explicitly asked to watch, so watching a single file under a files.watcherExclude directory was silently deaf
  • the recovery diff ran against the adjusted snapshot rather than what was on disk
  • clearing a delete timer that had already fired cancelled nothing
  • the watched directory's own name was compared case-sensitively, and two separately resolved paths with ===, both wrong on case-insensitive file systems
  • a handle error during a start was declined by a boolean guard, leaving a dead handle for good; a start attempt is a cancellation token now
  • the dropped-FS-events log printed the fs.promises module instead of the watched path (pre-existing)

Honest cost: the provider abstraction is two implementations selected by a boolean. It pays by deleting the key prefixes, the instanceof and the parallel create and key pairs, but a third kind may never appear.

How to test

npx lerna run test --scope @theia/filesystem. The specs assert only on what clients were told, so they survive the restructuring; routing and sharing get their own seam via createProviders().

Follow-ups

Delete parcel-filesystem-service.ts and rename ParcelFileSystemWatcherServerOptions (only parcelOptions is parcel-specific) at the next major. Parcel failure handling: #17922.

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

[filesystem]: FileSystemWatcherServiceImpl moved to filesystem-watcher-service-impl.ts and delegates to providers, dropping its watchers/watcherHandles maps and createWatcher/getWatcherKey/registerWatcher/getLiveWatcher methods, along with the unreleased WatcherInstance and WatcherHandle types; ParcelWatcher moved to parcel-watcher/parcel-watcher.ts, deprecating initialClientId and renaming ParcelWatcherOptions to ParcelWatcherExcludes with a deprecated alias.

Attribution

Review checklist

Reminder for reviewers

@github-project-automation github-project-automation Bot moved this to Waiting on reviewers in PR Backlog Aug 23, 2026
@EhabY
EhabY force-pushed the refactor/unified-watcher-interface branch from e58bb54 to 4df2b40 Compare August 23, 2026 14:30
On macOS, creating a child also modifies its parent, so FSEvents may or
may not additionally report the watched root. The exact-set assertion
therefore failed intermittently: on one commit, macos-15 node-24.x
passed while macos-15 node-22.x failed.

Assert that no unexpected path arrived instead, excluding the root. The
waitForChange calls above already establish that each expected URI
arrived.
@EhabY
EhabY force-pushed the refactor/unified-watcher-interface branch from 4df2b40 to f3017e3 Compare August 23, 2026 14:56
EhabY added 2 commits August 23, 2026 18:35
The backend dropped the recursive flag, so every watch became a recursive
parcel subscription: a non-recursive watch rooted outside the workspace
crawled every sibling subtree, and watching a single file crawled its parent
directory.

Non-recursive requests now go to a NodeDirectoryWatcher that watches one
directory level with fs.watch, and requests resolving to the same directory
share one handle. Sharing matters beyond saving descriptors: on macOS libuv
keeps one FSEventStream per event loop and rebuilds it whenever any handle
starts or stops, losing the events of every other watcher while it does.

Platform handling follows VS Code where it is settled: NFC normalization and
case-insensitive matching on macOS, an exact-case readdir so a rename that
only changes case is not reported as an update, the deferred delete that
survives atomic saves, and refusing to watch a macOS network share. It
diverges where VS Code drops events: a Windows ReadDirectoryChangesW overflow
arrives as a null file name and is recovered by rescanning the directory.

This removes the mitigation from eclipse-theia#17633, restoring detection of a workspace
folder deleted while a plugin watches its parent.

Closes eclipse-theia#17700
The service told the two watchers apart by hand: an instanceof to release a
request, a client id on one path and a watcher id on the other, key prefixes
to keep both kinds in one map, and parallel create and key methods.

Both watchers implement FileSystemWatcher now, and a WatcherProvider per kind
owns its key, its factory and its sharing map. Picking one by the recursive
option is all the service still knows about the difference.

The service moves out of parcel-filesystem-service.ts, which defined it next
to only one of the two watchers it routes to. That file is a re-export now,
so the names it published, including the misspelled PacelWatcherHandle, keep
resolving.

Reviewing the result against VS Code, and auditing the async interleavings,
turned up defects worth listing:

- a queued task that threw left the change queue rejected with no handler,
  which ends the watcher process, and swallowed the task behind it
- a change event naming a file the initial snapshot had struck out reported
  an update and never recorded the name, so a later deletion of it announced
  a file that had never appeared
- excludes applied to the path a client explicitly asked to watch, and
  files.watcherExclude reaches every request, so watching a single file under
  an excluded directory was silently deaf
- the recovery diff ran against the adjusted snapshot rather than what was on
  disk, reporting a deletion for a file that was still there
- clearing a delete timer that had already fired cancelled nothing, so a
  settled deletion could still be followed by an addition
- the watched directory's own name was compared case-sensitively
- the dropped-FS-events log printed the fs.promises module instead of the
  watched path

A start attempt is a cancellation token rather than a boolean. A boolean
could say a start was in flight but not that it was stale, so a handle error
during one was declined and left the watcher holding a dead handle for good.

A request carries its excludes as written and where its path resolves to,
both readonly, rather than a compiled matcher and a name whose absence meant
something. Platform and file system calls live in WatcherHost, requests and
notification in WatchRequestRouter, and neither touches the event machine.
Tests drive the real watcher with the host substituted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Waiting on reviewers

Development

Successfully merging this pull request may close these issues.

1 participant