Fix flaky PhysicalFilesWatcher timeout when root is deleted and recreated#130967
Fix flaky PhysicalFilesWatcher timeout when root is deleted and recreated#130967svick wants to merge 2 commits into
Conversation
…ated TryEnableFileSystemWatcher decided whether to set up the root creation watcher solely based on FileSystemWatcher.EnableRaisingEvents. On Linux, when the watched root directory is deleted the runtime FileSystemWatcher tears down the inotify watch and queues an Error, but leaves EnableRaisingEvents == true. It is only reset once OnError runs TryDisableFileSystemWatcher. If a token was re-registered before that happened, TryEnableFileSystemWatcher saw EnableRaisingEvents == true and skipped setting up the PendingCreationWatcher, leaving a dead inotify watch bound to the deleted inode. Recreating the root never re-armed it, so the token never fired and the change token wait timed out (30s), causing the flaky failure in CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated. Handle the case where the watcher still reports enabled but the root no longer exists by tearing down the stale watch and falling back to the root creation watcher. Fixes dotnet#129691 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea806a13-89ae-46ef-a4d9-03a6db0a506e
|
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. |
|
Tagging subscribers to this area: @dotnet/area-extensions-filesystem |
There was a problem hiding this comment.
Pull request overview
This PR hardens PhysicalFilesWatcher against a race where the underlying FileSystemWatcher can remain logically “enabled” while its OS-level watch has been torn down after the watched root directory is deleted (notably on Linux/inotify), causing subsequent re-registrations to miss root recreation and never observe the target file creation.
Changes:
- In
TryEnableFileSystemWatcher, detect the “enabled but root missing” state and proactively tear down the stale watch by settingEnableRaisingEvents = false. - Fall back to the existing root reappearance mechanism (
PendingCreationWatcher) by settingneedsRootWatcher = trueand marking_rootWasUnavailable = trueso the re-enable path performs the “gap” scan when the root comes back.
|
/azp run runtime-coreclr libraries-jitstress-random |
|
No pipelines are associated with this pull request. |
|
/azp list |
|
/azp run runtime-coreclr libraries-jitstress-random |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr libraries-pgo, runtime-coreclr libraries-jitstress2-jitstressregs |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "0111d60aa5a5809d12e5ab11b73ddd26c3fa0547",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
"last_reviewed_commit": "0111d60aa5a5809d12e5ab11b73ddd26c3fa0547",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
"last_recorded_worker_run_id": "29755357810",
"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": "affce1c3815c07f222c815924b144659f261a3e9",
"review_id": 4734863581
},
{
"commit": "0111d60aa5a5809d12e5ab11b73ddd26c3fa0547",
"review_id": 4736507456
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: PhysicalFilesWatcherTests.CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated intermittently fails with a 30s TimeoutException (issue #129691), most often under jitstress/pgo legs on Linux. The change token never fires after the watched root is deleted and recreated.
Approach: The PR adds a single defensive branch in TryEnableFileSystemWatcher. Previously the decision to set up the PendingCreationWatcher (root-reappearance fallback) was gated solely on FileSystemWatcher.EnableRaisingEvents. On Linux, deleting the watched root tears down the inotify watch and queues an Error, but does not reset EnableRaisingEvents — only OnError→TryDisableFileSystemWatcher does. If a token is re-registered before OnError runs, the old code saw EnableRaisingEvents == true, skipped the fallback, and left a dead inotify watch bound to the deleted inode, so recreating the root never re-armed it. The new else if (!Directory.Exists(_root)) branch detects this stale state, tears down the watch (EnableRaisingEvents = false), sets needsRootWatcher/_rootWasUnavailable, and falls back to the root-creation watcher — closing the race deterministically.
Summary: The fix is small (14 additions, no deletions), well-scoped, and correct. It executes inside _fileWatcherLock, mirrors the semantics of the existing root-missing branch directly above it (setting the same needsRootWatcher/_rootWasUnavailable flags so EnsureRootCreationWatcher runs outside the lock), and preserves the justEnabledAfterRootCreated scan path for when the root reappears. The added comment clearly explains the platform-specific rationale. No correctness, concurrency, or behavioral regressions identified; the change only activates in a state that previously produced a hung watcher. I have no actionable findings.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 49.7 AIC · ⌖ 15.3 AIC · ⊞ 10K
|
LGTM. Is it possible to cover it by test? |
The existing CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated test only reproduces the underlying race probabilistically. Add a focused unit test that deterministically drives the fixed code path: it suspends the mock FileSystemWatcher via BeginInit so EnableRaisingEvents can be set without starting a real OS watch (avoiding the async Error that would otherwise make the test racy), enables it, deletes the root directory, then re-registers a token while the root is missing and asserts the stale watcher was torn down (EnableRaisingEvents == false). This test fails without the TryEnableFileSystemWatcher fix and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea806a13-89ae-46ef-a4d9-03a6db0a506e
|
@mrek-msft I have added a test. |
There was a problem hiding this comment.
Holistic Review
Motivation: The PR fixes a flaky 30s TimeoutException in CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated (issue #129691). On Linux, deleting a watched root tears down the inotify watch but leaves FileSystemWatcher.EnableRaisingEvents == true until OnError runs, so a token re-registered in that window left a dead watch and the change token never fired.
Approach: The core fix (unchanged since the initial review) adds an else if (!Directory.Exists(_root)) branch in TryEnableFileSystemWatcher that tears down the stale watch (EnableRaisingEvents = false), sets needsRootWatcher/_rootWasUnavailable, and falls back to the root-creation watcher. The incremental commit adds one deterministic regression test, CreateFileChangeToken_ReRegisterWhileRootMissing_TearsDownStaleWatcher, that reproduces the stale-watcher state without relying on OS timing: it uses MockFileSystemWatcher.BeginInit() to suspend the watcher (so enabling it stores EnableRaisingEvents without starting a real OS watch, avoiding the background Error race), deletes the root, re-registers a token, and asserts the watcher was torn down (EnableRaisingEvents == false).
Summary: The added test is well-scoped and correct. It targets exactly the state the fix addresses; without the else if branch the second registration would not reset EnableRaisingEvents, so the assertion fails, making this a genuine regression guard rather than a tautology. The BeginInit-based simulation is a reasonable, well-commented way to remove timing nondeterminism, and [SkipOnPlatform(Browser | iOS | tvOS)] correctly excludes unsupported platforms. No correctness, concurrency, or behavioral concerns in the incremental change. I have no actionable findings.
Assessment History:
- review 4734863581 reviewed commit
affce1c3815cand reached an LGTM verdict (no actionable findings). Current verdict is also LGTM. The assessment is unchanged: the incremental commit only adds a regression test and does not alter the fix's motivation, approach, or risk.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 51.3 AIC · ⌖ 10.5 AIC · ⊞ 10K
Fixes #129691.
Problem
PhysicalFilesWatcherTests.CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreatedintermittently fails with a 30sSystem.TimeoutException(the change token never fires), most frequently under jitstress/pgo legs on Linux.Root cause
TryEnableFileSystemWatcherdecided whether to set up thePendingCreationWatcher(the fallback that watches for the root directory to reappear) solely based onFileSystemWatcher.EnableRaisingEvents.On Linux, when the watched root directory is deleted, the runtime
FileSystemWatchertears down the inotify watch and queues anError, but does not resetEnableRaisingEvents— it keeps reportingtrue. The only thing that flips it tofalseisOnError→TryDisableFileSystemWatcher.The test deletes the root, then re-registers a token as soon as the initial token fires (from
OnError'sCancelAll). This is a race:TryEnableFileSystemWatcherruns afterOnError'sTryDisableFileSystemWatcher,EnableRaisingEventsisfalseand thePendingCreationWatcheris set up correctly.EnableRaisingEventsis stilltrue, so theif (!EnableRaisingEvents)block is skipped and noPendingCreationWatcheris created.TryDisableFileSystemWatcherthen sees the freshly re-added token and does not disable. The result is a dead inotify watch bound to the deleted inode: recreating the root never re-arms it, and the token never fires.Fix
In
TryEnableFileSystemWatcher, handle the case where the watcher still reports enabled but_rootno longer exists: tear down the stale watch (EnableRaisingEvents = false) and fall back to the root creation watcher. Since the test re-registers while the root is still deleted, this closes the race deterministically.Note
This pull request was created by GitHub Copilot.