fix(filesystem): report creation of watched paths that did not exist yet - #17899
fix(filesystem): report creation of watched paths that did not exist yet#17899sdirix wants to merge 1 commit into
Conversation
`ParcelWatcher.start()` waits for the watched path to exist before subscribing, so the creation itself happens while nobody is subscribed and its event is lost. Report the creation synthetically once subscribed, like VS Code does when it resumes a suspended watch request. Fixes #17842
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
What this PR does
ParcelWatcher.start() polls (every 500ms) until the watched path exists before calling subscribe(), so the creation event itself is lost. The fix records that the path was missing on entry (createdWhileWaiting) and, once subscribed, feeds a synthetic { type: 'create', path: this.fsPath } through the normal handleWatcherEvents pipeline. A new spec (parcel-watcher-missing-path.spec.ts) covers the file case with a real parcel subscription.
The approach is small, sits in the right place, matches VS Code's behaviour for resumed watch requests, and reuses the existing event path (so ignore filtering, the ordering queue and client fan-out all apply). Nothing unrelated is in the diff.
Verification I ran
Built the repo and ran the parcel watcher specs: all 8 pass. Also verified the fix end to end in the browser example with a fresh ~/.theia: setting breadcrumbs.enabled to false in the Settings UI creates settings.json after startup, and API Samples: Get Backend Preference then reports The value is false (pre-fix it reported true).
Where to focus
- The synthetic event carries the raw
this.fsPathwhile every real event from the same watcher is realpath'd. I reproduced the divergence with a symlinked directory: ADDED and the following UPDATE arrive under two different URIs. This is the one point I'd want a maintainer to decide on. - Directory watches only report the directory itself; anything created inside during the poll window is still dropped (verified).
- The new spec skips the Windows 8.3 short-path normalisation the sibling spec does, which can break the second test on the windows-2022 CI job.
- No test asserts that an already-existing path does not get a synthetic ADDED.
| // synthetically, otherwise clients keep the state they observed while the path was | ||
| // still missing until the next change. VS Code behaves the same way when it resumes | ||
| // a watch request that was suspended because its path did not exist. | ||
| this.handleWatcherEvents([{ type: 'create', path: this.fsPath }]); |
There was a problem hiding this comment.
The synthetic event uses the raw this.fsPath, but every real event from this watcher comes back realpath'd, since createWatcher subscribes to fsp.realpath(this.fsPath) (master). I ran this against a symlinked dir: the ADDED arrives as file:///tmp/x/link/settings.json while the very next UPDATE arrives as file:///tmp/x/real/settings.json. Clients that key on the URI (BackendPreferenceStorage does e.resource.isEqual(uri)) then see two different resources, so this should probably reuse the resolved path.
| throw WatcherDisposal; | ||
| } | ||
| this.watcher = watcher; | ||
| if (createdWhileWaiting) { |
There was a problem hiding this comment.
For directory watches only the directory itself is reported. I watched a non-existing dir and then created it with a file and a subdir inside within the 500ms poll window: the only change delivered was ADDED for the dir, both children were lost. Tree clients recover because a root ADDED triggers a full refresh (master), but plugin watchers do not, so this limitation is worth spelling out in the comment.
| let changes: FileChange[]; | ||
|
|
||
| beforeEach(() => { | ||
| root = FileUri.create(fs.realpathSync(temp.mkdirSync('parcel-missing-path-root'))); |
There was a problem hiding this comment.
fs.realpathSync does not expand Windows 8.3 short names, which is exactly why the sibling spec adds the powershell step (master). The second test compares a real parcel event (long name) against file.toString() (potentially short name), so it can fail on the windows-2022 CI job. The other two parcel specs get away without it because they stub subscribe.
| fs.writeFileSync(FileUri.fsPath(file), '{ "breadcrumbs.enabled": false }'); | ||
|
|
||
| // The path is polled every 500ms, so allow a few intervals plus the subscribe. | ||
| await waitFor(() => changes.some(change => change.uri === file.toString() && change.type === FileChangeType.ADDED), 5000); |
There was a problem hiding this comment.
The negative case is missing: a path that already exists must not produce a synthetic ADDED. That is the regression createdWhileWaiting could introduce and it is one cheap assertion.
2 participants
What it does
ParcelWatcher.start()waits for the watched path to exist before subscribing, so the creation itself happens while nobody is subscribed and its event is lost. Report the creation synthetically once subscribed, like VS Code does when it resumes a suspended watch request.Fixes #17842
How to test
~/.theia/settings.jsonand startexamples/browser.breadcrumbs.enabledtofalsein the Settings UI. This creates~/.theia/settings.json.API Samples: Get Backend Preferencewith keybreadcrumbs.enabled.The backend now correctly reports
false. Before this change it reportedtrueFollow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers