Raise InternalBufferOverflowException for FSEvents overflow on macOS - #130637
Raise InternalBufferOverflowException for FSEvents overflow on macOS#130637svick wants to merge 3 commits into
Conversation
On macOS, an FSEvents buffer/rescan overflow raised a plain IOException with the
unformatted SR.FSW_BufferOverflow string (so the message literally contained "{0}").
Windows and Linux both use the shared CreateBufferOverflowException helper, which
produces an InternalBufferOverflowException with a properly formatted message. Use
that helper on macOS too so the overflow exception type and message are consistent
across platforms, while preserving the FSEvents flags in HResult for diagnostics as
the previous IOException did.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: aeef6f66-c627-4a58-a4cb-6d14cf4d2649
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
This PR updates the macOS FileSystemWatcher FSEvents overflow/rescan error path to use the shared buffer-overflow exception creation helper, aligning macOS with the exception type/message pattern used by other platforms.
Changes:
- Replace the macOS FSEvents rescan/overflow
IOExceptionwithInternalBufferOverflowExceptioncreated viaCreateBufferOverflowException(...). - Preserve the FSEvents flag value for diagnostics (currently stored on the exception).
| InternalBufferOverflowException exception = CreateBufferOverflowException(_fullDirectory); | ||
| exception.HResult = (int)eventFlags[i]; | ||
| watcher.OnError(new ErrorEventArgs(exception)); |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
adamsitnik
left a comment
There was a problem hiding this comment.
@svick would it be possible to provide test coverage to it?
| { | ||
| watcher.OnError(new ErrorEventArgs(new IOException(SR.FSW_BufferOverflow, (int)eventFlags[i]))); | ||
| InternalBufferOverflowException exception = CreateBufferOverflowException(_fullDirectory); | ||
| exception.HResult = (int)eventFlags[i]; |
There was a problem hiding this comment.
We don't do that on for other OSes?
| exception.HResult = (int)eventFlags[i]; |
There was a problem hiding this comment.
I thought there is a chance somebody would want to differentiate between the various cases, since the old code did keep them.
But I'm fine with dropping them.
| if (ShouldRescanOccur(eventFlags[i])) | ||
| { | ||
| watcher.OnError(new ErrorEventArgs(new IOException(SR.FSW_BufferOverflow, (int)eventFlags[i]))); | ||
| watcher.OnError(new ErrorEventArgs(CreateBufferOverflowException(_fullDirectory))); |
| // First, we should check if this event should kick off a re-scan since we can't really rely on anything after this point if that is true | ||
| if (ShouldRescanOccur(eventFlags[i])) | ||
| { | ||
| watcher.OnError(new ErrorEventArgs(new IOException(SR.FSW_BufferOverflow, (int)eventFlags[i]))); | ||
| watcher.OnError(new ErrorEventArgs(CreateBufferOverflowException(_fullDirectory))); | ||
| break; |
|
@adamsitnik I will look into adding tests for this, but I don't have a mac, so it might take some time. |
Add an OSX-only OuterLoop test that mounts a disk image inside the watched directory. FSEvents reports a Mount flag for that path, which ShouldRescanOccur treats like a buffer overflow and surfaces through the Error event. This is a deterministic way to reach the rescan-error path (unlike an actual FSEvents buffer overflow, which can't be triggered reliably), and it asserts the exception is InternalBufferOverflowException with a formatted message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aeef6f66-c627-4a58-a4cb-6d14cf4d2649
|
I have added the test as OuterLoop and it succeeded when I run |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "32eec10b5bed92aaa97258ba3de1544c398f0a84",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "8a5301832157c4740b8c85ee25086762b29af531",
"last_reviewed_commit": "32eec10b5bed92aaa97258ba3de1544c398f0a84",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "8a5301832157c4740b8c85ee25086762b29af531",
"last_recorded_worker_run_id": "29686409653",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "32eec10b5bed92aaa97258ba3de1544c398f0a84",
"review_id": 4730733959
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: On macOS, when FSEvents signals a rescan/overflow condition, FileSystemWatcher raised a plain IOException instead of the InternalBufferOverflowException that Windows and Linux raise, and it passed the format string SR.FSW_BufferOverflow (Too many changes at once in directory:{0}.) directly to IOException(string, int) without SR.Format, so the surfaced message literally contained the {0} placeholder. Consumers keying off is InternalBufferOverflowException (e.g. the PhysicalFilesWatcher change in #130627) did not get the correct signal on macOS. The motivation is well-grounded and cross-platform consistency is a clear correctness win.
Approach: The one-line source change replaces the ad-hoc new IOException(SR.FSW_BufferOverflow, (int)eventFlags[i]) with the shared CreateBufferOverflowException(_fullDirectory) helper already used by the Windows and Linux implementations and by the base FileSystemWatcher.NotifyInternalBufferOverflowEvent. This fixes both the exception type and the message formatting in a single, minimal edit and centralizes behavior on the existing helper. A new OSX-only [OuterLoop] test deterministically reaches the rescan code path by mounting an HFS+ disk image inside the watched tree (the kFSEventStreamEventFlagMount flag is one of the conditions in ShouldRescanOccur), then asserts the reported exception is InternalBufferOverflowException and that its message does not contain {0}. This is a sound, minimal, and idiomatic approach.
Summary: This is a correct and well-scoped fix. The source change is safe and aligns macOS with the other platforms; the previously stored FSEvents flags (formerly in HResult) are intentionally dropped in favor of consistency, which is an acceptable tradeoff given the helper is the established pattern. The new test is appropriately gated ([PlatformSpecific(TestPlatforms.OSX)] + [OuterLoop]), cleans up the mount in a finally, and uses supported Process.RunAndCaptureTextAsync/ProcessTextOutput APIs that are present at the merge base. Because it creates/mounts a disk image and is OuterLoop, it will not run in normal PR CI, so its value is primarily as a regression check in outerloop runs; that is a reasonable choice for exercising this code path. No blocking issues found. LGTM.
Detailed Findings
No actionable findings. The change is minimal, matches existing conventions (file-scoped behavior, shared helper usage), and the test is correctly attributed and cleans up its resources.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 66.6 AIC · ⌖ 10.6 AIC · ⊞ 10K
…tems (#130627) Fixes #121475. ## Problem When a Generic Host app lives on a file system that can't be watched — e.g. a WSL path (`\\wsl.localhost\...`) accessed via `dotnet` from Windows, or other mounted/network drives — it crashes with a `StackOverflowException` after a while. On such a file system, `FileSystemWatcher` keeps failing to start and raises the same `Error` every time it's enabled. In `PhysicalFilesWatcher.OnError` this cancels the change tokens, which fires the `ChangeToken.OnChange` registration set up by `FileConfigurationProvider` (and similar consumers). That registration's producer re-creates a token via `CreateFileChangeToken`, which re-enables the watcher, which raises the same `Error` again — an unbounded cancel -> re-register -> re-enable -> error cycle that recurses until the stack overflows. ## Fix `PhysicalFilesWatcher.OnError` now distinguishes persistent failures from recoverable ones: - The **first** occurrence of an error is reported as before. - An **identical recurrence** — same exception type and OS error code (`Win32Exception.NativeErrorCode`, otherwise `HResult`) — with **no change delivered in between** is **suppressed** (the tokens aren't cancelled), which breaks the loop. - `InternalBufferOverflowException` (the watcher is alive but dropped events, so consumers must rescan) and `DirectoryNotFoundException` (the watched directory was deleted/moved) are **always** reported and reset the detection. - Any **delivered change** resets the remembered error, so a transient error that later recovers isn't mistaken for a persistent one. This is detection-only: on a genuinely unwatchable file system the app no longer crashes, but file-change-based reload is simply inactive there (matching the documented behavior that `FileSystemWatcher` "is ineffective in some scenarios such as mounted drives", whose remedy is `DOTNET_USE_POLLING_FILE_WATCHER`). Automatically falling back to polling is probably undesirable. ## Relationship to #130492 #130492 (switching `FileConfigurationProvider` to the async `ChangeToken.OnChange` overload) already works around the reported crash: awaiting `Task.Delay(ReloadDelay)` unwinds the stack between iterations, so the recursion becomes a (slow) async loop instead of growing the stack. That mitigates the `StackOverflowException` for the default `ReloadDelay` (250 ms), but it doesn't address the underlying churn (the watcher is still re-enabled and re-errors on a loop), and it's defeated by `ReloadDelay == 0` (`Task.Delay(0)` completes synchronously, restoring the synchronous recursion). This PR fixes the root cause in `PhysicalFilesWatcher`, independent of the consumer's overload or reload delay. ## Known limitation macOS raises buffer overflow as a plain `IOException(SR.FSW_BufferOverflow)` (with `HResult` set to the FSEvents flags), not `InternalBufferOverflowException` like Windows/Linux. So the "always report" carve-out doesn't recognize it, and a sustained macOS overflow with no delivered change in between could be suppressed. A clean follow-up is to make the macOS `FileSystemWatcher` raise `InternalBufferOverflowException` for consistency: #130637. ## Tests Added tests in `PhysicalFilesWatcherTests` that drive `OnError` via `MockFileSystemWatcher.CallOnError`: - an identical error recurrence is suppressed (covering both `Win32Exception`/`NativeErrorCode` and `IOException`/`HResult`); - a different error code is still reported; - `InternalBufferOverflowException` and `DirectoryNotFoundException` are always reported; - a delivered change resets detection so the same error is reported again; - an `Error` with no exception is always reported. > [!NOTE] > This pull request was created by GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
On macOS, when FSEvents signals a rescan/overflow (
kFSEventStreamEventFlagMustScanSubDirs/UserDropped/KernelDropped),FileSystemWatcherraised the wrong exception:Two issues:
InternalBufferOverflowExceptionfor a buffer overflow (both via the sharedCreateBufferOverflowExceptionhelper), but macOS raised a plainIOException. Consumers that checkis InternalBufferOverflowExceptionto recognize "events were dropped, rescan needed" don't get that signal on macOS.SR.FSW_BufferOverflowis a format string (Too many changes at once in directory:{0}.), but it was passed to theIOException(string, int)constructor withoutSR.Format, so the message literally contained the{0}placeholder instead of the directory path.Note that #130627 is changing
PhysicalFilesWatcher(inMicrosoft.Extensions.FileProviders.Physical) to look forInternalBufferOverflowException. Without this fix, macOS will behave a bit worse with PFW.Fix
Use the same shared
CreateBufferOverflowException(_fullDirectory)helper that Windows and Linux use, so the exception type and message are consistent across platforms. The FSEvents flags that were previously stored inHResultare preserved for diagnostics.Note
This pull request was created by GitHub Copilot.