fix(file-service): improve watcher reliability and fix memory leaks #4694
Merged
Conversation
Member
Author
|
/next |
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769080100.0 |
Contributor
Walkthrough统一使用解析后的 realpath 作为 watcher 的内部键，添加 dispose 保护，改为异步文件检测并在 Linux 上用订阅式探测验证 ParcelWatcher 可用性，失败或超时则回退到 NSFW；同时扩展相关单元测试与日志、错误处理。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Watch 请求
participant FS as 文件系统
participant Parcel as ParcelWatcher
participant NSFW as NSFW Watcher
participant Logger as Logger
Client->>FS: resolveWatchPath(basePath)
FS-->>Client: realWatchPath
Client->>Parcel: subscribe(realWatchPath, probeCallback)
alt subscribe throws
Parcel-->>Client: throw
Client->>Logger: warn("subscribe failed, fallback to NSFW")
Client->>NSFW: start(realWatchPath)
NSFW-->>Client: started
else subscribe succeeds
Parcel-->>Client: subscription
Client->>FS: 创建临时 probe 文件 写入 "probe"
FS-->>Parcel: 触发文件系统事件
Parcel-->>Client: onEvent -> probe 成功
Client->>Parcel: unsubscribe
Client->>FS: 删除临时 probe 目录
alt probe verified
Client->>Parcel: start watching realWatchPath
Parcel-->>Client: started
else probe timeout/no event
Client->>Logger: warn("probe timeout, fallback to NSFW")
Client->>NSFW: start(realWatchPath)
NSFW-->>Client: started
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/file-service/src/node/hosted/recursive/file-service-watcher.ts`:
- Around line 139-151: The code sets watchPathMap.set(basePath, realWatchPath)
without clearing an existing mapping, so if basePath previously pointed to a
different real path the old watcher and resources leak; before assigning, check
if this.watchPathMap.has(basePath), get the oldReal =
this.watchPathMap.get(basePath), and if oldReal && oldReal !== realWatchPath
then find its handler in this.WATCHER_HANDLERS, call
handler?.disposable.dispose(), delete that entry from WATCHER_HANDLERS, call
this.cleanupWatchPathMap(oldReal), and remove the old mapping, then proceed to
set the new mapping; use the existing resolveWatchPath, WATCHER_HANDLERS,
watchPathMap, cleanupWatchPathMap symbols to locate and implement the cleanup
logic.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4694 +/- ##
==========================================
+ Coverage 52.77% 52.90% +0.13%
==========================================
Files 1686 1686
Lines 104718 104886 +168
Branches 22756 22787 +31
==========================================
+ Hits 55260 55487 +227
+ Misses 41089 41042 -47
+ Partials 8369 8357 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
/next |
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769081124.0 |
Member
Author
|
/next |
coffeedeveloper
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/file-service/src/node/hosted/un-recursive/file-service-watcher.ts`:
- Around line 44-53: The watcher is being added to watcherCollections
immediately after watch(basePath) but before fs.lstat(basePath) completes,
risking a leaked watcher if lstat throws; update doWatch to only call
this.watcherCollections.set(basePath, watcher) after all async initialization
(e.g., after awaiting fs.lstat and determining isDirectory) succeeds, or if you
prefer to keep the early set, ensure the catch path cleans up by removing the
entry from watcherCollections and closing the watcher (call watcher.close() or
equivalent) when an error occurs; reference doWatch, watch, watcherCollections
and fs.lstat when making the change.
♻️ Duplicate comments (1)
packages/file-service/src/node/hosted/recursive/file-service-watcher.ts (1)
139-146: basePath 重映射时的旧 watcher 清理已实现。当同一个
basePath的realWatchPath发生变化时（如符号链接目标变更），代码现在正确地先清理旧的 watcher 再创建新的映射。这解决了之前 review 中提出的资源泄漏问题。
🧹 Nitpick comments (1)
packages/file-service/src/node/hosted/recursive/file-service-watcher.ts (1)
576-616: 探测机制存在潜在的竞态条件。
ParcelWatcher.subscribe的 Promise 在eventPromise内部启动但未等待完成，而外部代码在固定 100ms 后（line 610）就写入测试文件。如果订阅建立耗时超过 100ms，可能导致事件丢失，使探测误判为失败。虽然这是"安全失败"（回退到 NSFW），但可能导致本可使用 parcel 的系统不必要地使用 NSFW。建议等待订阅确认建立后再触发文件事件：
建议修复
+ // 等待订阅建立 + let subscriptionReady = false; const eventPromise = new Promise<boolean>((resolve) => { const timeout = setTimeout(() => { resolve(false); }, PROBE_TIMEOUT_MS); ParcelWatcher.subscribe( tempDir!, (err, events) => { // ... callback logic }, { backend: RecursiveFileSystemWatcher.PARCEL_WATCHER_BACKEND, }, ) .then((sub) => { subscription = sub; + subscriptionReady = true; }) .catch((subError) => { // ... error handling }); }); - // 等待订阅建立后再触发文件变更 - await new Promise((resolve) => setTimeout(resolve, 100)); + // 等待订阅确认建立 + const maxWait = 1000; + const startTime = Date.now(); + while (!subscriptionReady && Date.now() - startTime < maxWait) { + await new Promise((resolve) => setTimeout(resolve, 50)); + } + if (!subscriptionReady) { + this.logger.warn('[Recursive] parcel/watcher subscription setup timeout during probe'); + return false; + }
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769136679.0 |
RecursiveFileSystemWatcher: - Improve parcel watcher availability detection on Linux by verifying actual file events instead of just snapshot writing - Add fallback to NSFW when parcel watcher fails on Linux - Ensure notifyWatcherFailed is called before fallback UnRecursiveFileSystemWatcher: - Fix memory leak: watchers now properly added to watcherCollections - Fix dispose() to clean up all watchers in collections - Fix unwatchFileChanges to handle realPath vs basePath mismatch - Fix empty disposables returned from start() method - Add disposed check in setTimeout callbacks to prevent use-after-dispose - Convert sync APIs to async to avoid blocking event loop Tests: - Add tests for parcel watcher event-based availability detection - Add tests for Linux fallback behavior - Add resource management tests for UnRecursiveFileSystemWatcher
coffeedeveloper
force-pushed
the
fix/watcher_bug
branch
from
7fc0e95 to
94bef07
Compare
Member
Author
|
/next |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/file-service/src/node/hosted/un-recursive/file-service-watcher.ts (2)
182-205:fs.realpath抛异常导致start方法失败。
pathExists与realpath之间存在竞态条件。路径在第 184 行的检查中存在，但在第 188 行调用realpath时可能已被删除或符号链接目标缺失，导致异常被抛出而未被捕获。建议增加异常处理，回退到basePath。注：同文件中
unwatchFileChanges方法（第 217-220 行）已实现类似的异常处理模式。🛠️ 建议修复
- const realPath = await fs.realpath(basePath); + let realPath = basePath; + try { + realPath = await fs.realpath(basePath); + } catch (error) { + this.logger.warn('[Un-Recursive] Failed to resolve realPath, fallback to basePath:', error); + } if (this.watcherCollections.has(realPath)) { return disposables; }
103-141: 异步 change 处理缺少 try/catch，可能产生未处理的 Promise rejection。
fs.pathExists/fs.lstat在文件快速删除或权限问题下可能抛错。位于setTimeout中的异步回调（两处）未捕获异常会导致 unhandled rejection。应包一层try/catch并使用this.logger.error()记录日志，与文件中其他 fs 操作的错误处理保持一致。🛠️ 建议修复
if (isDirectory) { setTimeout(async () => { // 检查是否已销毁，避免在销毁后执行 if (this.toDispose.disposed) { return; } + try { // 监听的目录如果是文件夹，那么只对其下面的文件改动做出响应 if (docChildren.has(changeFileName)) { if ((type === 'rename' || type === 'change') && changeFileName === filename) { const fileExists = await fs.pathExists(changePath); if (fileExists) { this.pushUpdated(changePath); } else { docChildren.delete(changeFileName); this.pushDeleted(changePath); } } } else if (await fs.pathExists(changePath)) { const changeStat = await fs.lstat(changePath); if (!changeStat.isDirectory()) { this.pushAdded(changePath); docChildren.add(changeFileName); } } + } catch (error) { + this.logger.error('[Un-Recursive] Error handling change event:', error); + } }, UnRecursiveFileSystemWatcher.FILE_DELETE_HANDLER_DELAY); } else { setTimeout(async () => { // 检查是否已销毁，避免在销毁后执行 if (this.toDispose.disposed) { return; } + try { if (changeFileName === signalDoc) { if (await fs.pathExists(basePath)) { this.pushUpdated(basePath); } else { this.pushDeleted(basePath); signalDoc = ''; } } + } catch (error) { + this.logger.error('[Un-Recursive] Error handling change event:', error); + } }, UnRecursiveFileSystemWatcher.FILE_DELETE_HANDLER_DELAY); }
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769137704.0 |
- Add isDisposed flag to RecursiveFileSystemWatcher to track disposal state - Prevent watchFileChanges and doWatchFileChange if watcher is disposed, throwing error - Log and cleanup watcher handlers if disposed during start - Enhance WatcherHostServiceImpl with watcher server version tracking for safe reinitialization - Defer excludes reset until pending watchers complete to avoid conflicts - Retry watch start if watcher server resets during the process - Remove stale watcher handlers and invalidate pending watchers with outdated versions - Add tests to verify disposal behavior and deferred excludes update handling
Member
Author
|
/next |
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769159114.0 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/file-service/__tests__/node/recursive-watcher-dispose.test.ts`:
- Line 59: The test uses root.path.toString() which returns only the filesystem
path; change the call to root.toString() so the watcher.watchFileChanges(...)
receives the full file URI string consistent with other tests and production
usage (update the invocation of watcher.watchFileChanges to pass root.toString()
instead of root.path.toString()); locate the call to watcher.watchFileChanges in
the test and replace the argument accordingly.
In `@packages/file-service/src/node/hosted/watcher.host.service.ts`:
- Around line 95-121: When force=true in initWatcherServer you currently dispose
watchers and call this.doWatch() to rewatch (using rewatchTargets) but don't
await those async rewatch calls; change initWatcherServer to gather the promises
returned by this.doWatch(...) when iterating rewatchTargets (the same loop that
uses rewatchTargets and references watcherCollection, pendingWatchers,
WATCHER_HANDLERS, watcherServerVersion) and await Promise.all on them before
returning so initWatcherServer only resolves after rewatch completes; ensure
initWatcherServer remains async, propagate or log errors from the awaited
promises, and keep existing cleanup (dispose/clear) logic intact.
🧹 Nitpick comments (4)
packages/file-service/src/node/hosted/recursive/file-service-watcher.ts (3)
211-218: 启动期间 dispose 检查的时机可能存在竞态。在
await this.start()返回后才检查isDisposed，但在start()内部（特别是 parcel subscribe 的重试循环中）可能已经花费了相当长的时间。如果在start()执行期间调用了dispose()，理想情况下应该能更早中断。不过当前实现在
start()返回后进行清理也是可接受的，因为最终资源会被正确释放。
590-678: 基于订阅的 parcel 可用性探测实现全面。新的探测逻辑通过实际创建订阅并触发文件事件来验证 parcel watcher 是否真正工作，比之前的 snapshot 探测更可靠。finally 块确保订阅和临时目录在所有路径上都被清理。
但有一个潜在问题：Line 641 的 100ms 延迟可能不足以确保订阅已建立，特别是在负载较重的系统上。
♻️ 建议改进
考虑使用更可靠的方式等待订阅建立，而不是固定延迟：
- // 等待订阅建立后再触发文件变更 - await new Promise((resolve) => setTimeout(resolve, 100)); + // 等待订阅建立后再触发文件变更 + // 给予更多时间确保订阅在高负载系统上也能建立 + await new Promise((resolve) => setTimeout(resolve, 200));或者可以在订阅成功回调中设置一个标志，然后轮询等待该标志。
607-637: 探测中的 Promise 竞态：订阅可能在eventPromise创建后才完成。
ParcelWatcher.subscribe()是异步的，在eventPromise的 Promise 构造函数中调用它，然后通过.then()设置subscription。如果订阅在超时后才完成或出错，subscription可能为undefined，导致 finally 中无法正确清理。当前实现在
.catch()中调用clearTimeout和resolve(false)是正确的，但建议重构以使逻辑更清晰。packages/file-service/__tests__/node/watcher-host-service.test.ts (1)
64-78: 建议添加afterEach清理以确保测试隔离。虽然
beforeEach中清理了 mocks 和状态，但考虑在每个测试后也清理 service 实例，以防止潜在的资源泄漏或状态污染。♻️ 可选改进
beforeEach(() => { jest.clearAllMocks(); getMockState().unrecursiveWatchDefers.length = 0; }); + afterEach(() => { + // 如果 service 有 dispose 方法，考虑调用以确保资源清理 + });
- Correct watcher server initialization by awaiting async calls in WatcherHostServiceImpl - Add error handling for rewatch operations with Promise.all in watcher server re-initialization - Update several calls to initWatcherServer to use await to ensure completion before proceeding - Fix test usage by changing root.path.toString() to root.toString() for FileUri consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/file-service/src/node/hosted/watcher.host.service.ts`:
- Line 80: The constructor currently calls void
this.initWatcherServer(this.defaultExcludes) which swallows the Promise and
leaves any rejection unhandled; change the call to attach proper error handling
(e.g. this.initWatcherServer(this.defaultExcludes).catch(err => {
this.logger?.error?.("initWatcherServer failed", err); /* optionally rethrow or
set an error state */ })) or await it inside an async init method called from
the constructor and handle errors there; reference the initWatcherServer method
and defaultExcludes property and ensure the error is logged with the thrown
error details rather than ignored.
Member
Author
|
/next |
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769161249.0 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/file-service/src/node/hosted/watcher.host.service.ts (1)
188-285: 添加 Promise.all 错误捕获与处理器清理。
unrecursiveWatcher.watchFileChanges()仅使用.then()，缺少.catch()。若 Promise.all 因此异常，finally虽会执行清理pendingWatchers，但WATCHER_HANDLERS中的占位符将被遗漏，导致该watcherId无法再次使用。应添加catch块以记录错误并主动删除占位符。🛠️ 建议修复
- try { - await Promise.all(startWatchers); - } finally { + try { + await Promise.all(startWatchers); + } catch (error) { + this.logger.error('watch error (unrecursive): ', uriString, error); + this.WATCHER_HANDLERS.delete(watcherId); + throw error; + } finally { this.pendingWatchers.delete(basePath); if (this.pendingWatchers.size === 0 && this.deferredExcludes) { const deferred = this.deferredExcludes; this.deferredExcludes = undefined;
🤖 Fix all issues with AI agents
In `@packages/file-service/src/node/hosted/watcher.host.service.ts`:
- Around line 84-86: The serialize logic in normalizeExcludes uses join('|')
which can collide if an exclude contains '|' — update normalizeExcludes (the
private method normalizeExcludes) to return a deterministic JSON string instead:
take (excludes ?? []), shallow-copy and sort it (slice().sort()) and then return
JSON.stringify(...) so the serialized key is unique and order-stable; replace
the current join('|') with JSON.stringify on the sorted array.
♻️ Duplicate comments (1)
packages/file-service/src/node/hosted/watcher.host.service.ts (1)
78-81: 构造函数仍可能触发未处理的 Promise 拒绝。
void丢弃initWatcherServer失败会导致未处理 rejection。建议显式记录错误。🛠️ 建议修复
- void this.initWatcherServer(this.defaultExcludes); + this.initWatcherServer(this.defaultExcludes).catch((err) => { + this.logger.error('init watcher server failed: ', err); + });#!/bin/bash # 验证是否已有全局未处理异常/拒绝处理器 rg -nP "unhandledRejection|uncaughtException" -S
Member
Author
|
/next |
Contributor
|
🎉 PR Next publish successful! 3.9.1-next-1769396616.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types
Background
[Recursive] No handler found for xxx, which correlates with missed file change events.Solution
Changelog
recursive/file-service-watcher.ts.
Summary by CodeRabbit
发布说明
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.