Don't follow symlinks when creating a DirectoryWatcher. #2167

bwilkerson · 2025-09-18T21:33:55Z

No description provided.

gemini-code-assist

Code Review

This pull request introduces a breaking change to stop DirectoryWatcher from following symbolic links when scanning directories. This is achieved by setting followLinks: false in all Directory.list calls across the different platform implementations. The change is accompanied by a major version bump to 2.0.0, an updated changelog, and a new test to verify the behavior. The changes look correct and well-implemented. I have one critical comment regarding the new test code which appears to have a compilation error.

gemini-code-assist · 2025-09-18T21:35:17Z

pkgs/watcher/test/directory_watcher/shared.dart

+    test('are not watched when not enabled', () async {
+      writeFile('dir/sub/a.txt', contents: 'a');
+      writeFile('sibling/sub/b.txt', contents: '1');
+      writeSymlink('dir/linked', target: 'sibling');
+      await startWatcher(path: 'dir', followLinks: false);


The call to startWatcher on line 349 includes a followLinks parameter, but the startWatcher function (defined in test/utils.dart) does not declare this parameter, which will cause a compilation error. Since not following symlinks is now the default and only behavior, this parameter is unnecessary.

Additionally, the test name on line 345, "are not watched when not enabled", could be updated to more accurately reflect the new default behavior. A name like "does not follow directory symlinks" would be clearer.

I've provided a suggestion that addresses both points.

Suggested change

test('are not watched when not enabled', () async {

writeFile('dir/sub/a.txt', contents: 'a');

writeFile('sibling/sub/b.txt', contents: '1');

writeSymlink('dir/linked', target: 'sibling');

await startWatcher(path: 'dir', followLinks: false);

test('does not follow directory symlinks', () async {

writeFile('dir/sub/a.txt', contents: 'a');

writeFile('sibling/sub/b.txt', contents: '1');

writeSymlink('dir/linked', target: 'sibling');

await startWatcher(path: 'dir');

github-actions · 2025-09-18T21:37:10Z

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Coverage

⚠️

File	Coverage
pkgs/watcher/lib/src/directory_watcher/linux.dart	💔 0 % ⬇️ 100 %
pkgs/watcher/lib/src/directory_watcher/mac_os.dart	💔 0 % ⬇️ NaN %
pkgs/watcher/lib/src/directory_watcher/polling.dart	💔 0 % ⬇️ 100 %
pkgs/watcher/lib/src/directory_watcher/windows.dart	💔 0 % ⬇️ NaN %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

This check can be disabled by tagging the PR with skip-license-check.

bwilkerson · 2025-09-18T21:43:21Z

There appears to be a problem with bumping the version number, and I don't know how to fix it. Any advice?

munificent

This looks great, thank you!

munificent · 2025-09-18T23:09:16Z

pkgs/watcher/test/directory_watcher/shared.dart

+      writeFile('sibling/sub/b.txt', contents: '2');
+      // Ensure that any events for the first modification arrive before the
+      // events for the second modification.
+      await pumpEventQueue();


Maybe add a comment that there is expected to be no modification event for dir/linked/b.txt? Does the test API have a way to test that explicitly?

Comment added.

I don't know whether the test actually confirms that. It kind of looks like it's just expecting that there's a modify event for a.txt somewhere in the stream, but I can't find any way to ensure that there's only one event, or even that there's no event of a given kind.

munificent · 2025-09-18T23:10:35Z

pkgs/watcher/test/directory_watcher/shared.dart

+      await pumpEventQueue();
+      writeFile('dir/sub/a.txt', contents: 'a');
+      await expectModifyEvent('dir/sub/a.txt');
+    });


It would also be interesting to test whether the process of creating or deleting a symlink inside a watched directory fires a notification. I would expect it to, and it might be worth pinning that down. But it's not essential for this PR.

I think that should be tested and described with this release, since if there are any inconsistencies we might need another breaking release to fix them.

One possible behaviour is that within a watched directory, changes to symlinks to files/directories are reported exactly as changes to files would be.

Another possible behaviour is that they are always ignored.

I think either one is fine, as long as it is consistent across all three platforms + documented.

I will add a test.

davidmorgan

Thanks! This should be a nice improvement to the package.

Made some suggestions.

davidmorgan · 2025-09-19T09:26:58Z

pkgs/watcher/test/utils.dart

+void writeSymlink(String path, {required String target}) {
+  var fullPath = p.join(d.sandbox, path);
+
+  // Create any needed subdirectories.


I think this can be just

createSync(target, recursive: true)

I think you're right. I didn't do that because I was copying the pattern in writeFile, trying to keep a consistent style.

davidmorgan · 2025-09-19T09:29:01Z

pkgs/watcher/test/utils.dart

  Directory(p.join(d.sandbox, path)).deleteSync(recursive: true);
 }
+
+/// Schedules writing a symlink in the sandbox at [path] that links to [target].


I think "schedules" refers to tracking of modification times which is not done here.

Possibly it should be if we also test behaviour of symlinking files.

If not then "Schedules writing" can be replaced with just "Writes".

davidmorgan · 2025-09-19T09:31:36Z

pkgs/watcher/CHANGELOG.md

@@ -1,3 +1,12 @@
+## 2.0.0
+
+- Changes the behavior of the `DirectoryWatcher` so that it no longer follows


It would make sense to also mention the bug fix aspect of the change

Bug fix: fix DirectoryWatcher bad performance when there are cyclic symlinks in the directory being watched.

davidmorgan · 2025-09-19T09:33:19Z

pkgs/watcher/CHANGELOG.md

+- Changes the behavior of the `DirectoryWatcher` so that it no longer follows
+  symlinks. On Linux, but not on MacOS or Windows, it used to watch directories
+  to which there was a symlink within the directory being watched. It no longer
+  does that. Code that depends on that behavior will need to be updated so that


I would prefer people let us know if that is what they wanted, since we already have most of the implementation and could add it back.

So maybe

If you were relying on that behavior, please file an issue.

?

davidmorgan · 2025-09-19T09:40:20Z

pkgs/watcher/test/directory_watcher/shared.dart

+      await pumpEventQueue();
+      writeFile('dir/sub/a.txt', contents: 'a');
+      await expectModifyEvent('dir/sub/a.txt');
+    });


I think that should be tested and described with this release, since if there are any inconsistencies we might need another breaking release to fix them.

One possible behaviour is that within a watched directory, changes to symlinks to files/directories are reported exactly as changes to files would be.

Another possible behaviour is that they are always ignored.

I think either one is fine, as long as it is consistent across all three platforms + documented.

davidmorgan · 2025-09-19T09:41:45Z

pkgs/watcher/test/directory_watcher/shared.dart

    });
  });
+  group('symlinks', () {
+    test('are not watched when not enabled', () async {


Update the name? There is no longer a choice whether to enable watching symlinks.

davidmorgan · 2025-09-19T09:42:46Z

pkgs/watcher/CHANGELOG.md

@@ -1,3 +1,12 @@
+## 2.0.0


Please also describe how symlinks are handled in the DirectoryWatcher dartdoc.

bwilkerson · 2025-09-19T15:32:47Z

Thanks for the great feedback!

I'm not sure how useful it would be for me to address the comments before we address the bigger issue, which is that it doesn't appear to be possible to create a breaking change version of this package. See https://github.com/dart-lang/tools/actions/runs/17842069555/job/50733710413?pr=2167 for details. I don't know how to proceed, so advice is more than welcome.

davidmorgan · 2025-09-19T15:50:53Z

Thanks for the great feedback!

I'm not sure how useful it would be for me to address the comments before we address the bigger issue, which is that it doesn't appear to be possible to create a breaking change version of this package. See https://github.com/dart-lang/tools/actions/runs/17842069555/job/50733710413?pr=2167 for details. I don't know how to proceed, so advice is more than welcome.

Sorry, I missed that bit.

I think a dependency override forcing the watch version should work for now, then test will need a release allowing the new version, then the dependency override can be removed and the new watch can be published.

bwilkerson · 2025-09-19T16:16:02Z

I think a dependency override forcing the watch version ...

Where would this dependency override be added?

devoncarew · 2025-09-19T17:33:42Z

Re: the CI failures - dart pub get can't run since package:test can't solve with this version of watcher. You'll likely want to force resolution of a specific version of test. This may work:

Create a new pubspec_overrides.yaml adject to watcher's pubspec;

dependency_overrides:
  test: ^1.16.6

If that doesn't work, perhaps:

dependency_overrides:
  watcher: 
    path: .

bwilkerson · 2025-09-21T17:31:57Z

Create a new pubspec_overrides.yaml adject to watcher's pubspec;

I didn't know that there was support for such a file. How widely used / publicized is this file? I ask because neither the analyzer nor the server has any knowledge of this file either, so none of the support for the pubspec.yaml file is supported in this file and I'm wondering whether it ought to be.

Is it safe to publish a version of the package while this file exists? That is, will publish refuse to copy this file?

test: ^1.16.6

I'm not sure how that will help given that it exactly matches the dependency in the pubspec.yaml file. Unless the semantics of the pubspec_overrides.yaml file is significantly different than what I expect it to be?

davidmorgan · 2025-09-22T07:19:14Z

Not sure about pubspec_overrides.yaml, I think you can also just put it as a dependency override in pubspec.yaml.

You don't need to publish with a dependency override. Once the version is committed, we can hop over to test and check that it works correctly with the new version via a path/ref dep, if so, publish test with the wider constraint, and remove the dependency override.

devoncarew · 2025-09-22T16:46:40Z

You can publish with a pubspec_overrides.yaml, however I think the near term goals are more getting the CI green with this PR, then testing the changes in the sdk and google3 (and w/ test).

Once the version is committed, we can hop over to test and check that it works correctly with the new version via a path/ref dep, if so, publish test with the wider constraint, and remove the dependency override.

👍

bwilkerson · 2025-09-22T23:57:37Z

Unfortunately, none of those suggestions worked.

When I added

dependency_overrides:
  test: ^1.16.6

to a pubspec_overrides.yaml file it caused the same "version solving failed" message.

When I replaced the content of the pubspec_overrides.yaml with

dependency_overrides:
  watcher: 
    path: .

I got the message "A package may not list itself as a dependency."

The same thing happened when I added the dependency_overrides section to the end of the pubspec.yaml file.

Any other ideas of how to proceed?

bwilkerson · 2025-09-23T00:01:37Z

Would it work to revert the version to 1.1.3, land the rest of the changes, bump the version number to 2.0.0, publish the uncommitted version, update test, and then commit the version bump PR? It's kind of ugly, but it might work (and we have to have some way to make a breaking change).

bwilkerson · 2025-09-23T04:05:41Z

Or, can we just widen the constraints on the watcher dependency in the test package prior to landing this CL?

davidmorgan · 2025-09-23T06:55:24Z

Looks like the analyzer needs an override too:

dependency_overrides:
  analyzer: '8.2.0'
  test: '1.26.3'

bwilkerson · 2025-09-23T16:33:48Z

Looks like the analyzer needs an override too:

That appears to have solved the problem. Thanks!

davidmorgan · 2025-09-24T08:57:28Z

I don't see your changes, I think you need to push them?

bwilkerson · 2025-09-24T20:39:27Z

Yes, I failed to push them because of technical issues. I'll have to figure that out first.

It would also be interesting to test whether the process of creating or deleting a symlink inside a watched directory fires a notification. I would expect it to, and it might be worth pinning that down. But it's not essential for this PR.

I think that should be tested and described with this release, since if there are any inconsistencies we might need another breaking release to fix them.

Ok, I've looked into this, and it's a mess, so I'd like some input.

The watcher code assumes that every file system entity is either a Directory or a File (but only explicitly tests for Directory). That used to be a safe assumption because Directory.list was resolving symlinks, so it never saw a Link.

My assumption was that it would therefore treat symlinks as if they were just files (which they mostly are), but that turns out to not be the case. I think that's because the underlying OS's aren't treating them like files in all cases. I don't know anything about the native file watching APIs, so I asked Gemini and it summarized the behavior with the following table:

Operation	Linux (inotify)	macOS (FSEvents)	Windows (ReadDirectoryChangesW)
Create Symlink	IN_CREATE on the link	ItemCreated event on the link's path	FILE_ACTION_ADDED on the link
Delete Symlink	IN_DELETE on the link	ItemRemoved event on the link's path	FILE_ACTION_REMOVED on the link
Change Symlink's Target	IN_ATTRIB or DELETE/CREATE on the link	ItemModified or similar event on the link's path	FILE_ACTION_MODIFIED or REMOVED/ADDED on the link
Modify Symlink's Target	No event	No event	No event

In other words, it appears to be the case that the operating systems don't always do the same thing, possibly based on the version of the OS or possibly based on other factors (kind of file system, etc.).

I tried writing a test, and on macOS it appears that changing the symlink's target produces a 'remove' event, but no 'add' event.

Another possible behaviour is that they are always ignored.

If we're happy with that (and I think that will be fine for the analysis server), then I can probably test for Link instances and just ignore them everywhere. I do not, however, know how to test that no event was produced for a given change. The current tests appear to be completely focused on proving a positive and not in proving a negative.

If we think they need to be reported, then this might be more than I currently know how to do.

davidmorgan · 2025-09-25T08:52:24Z

If we're happy with that (and I think that will be fine for the analysis server), then I can probably test for Link instances and just ignore them everywhere. I do not, however, know how to test that no event was produced for a given change. The current tests appear to be completely focused on proving a positive and not in proving a negative.

It looks to me like the test expectations are by default applied in order. So if the test causes an event and expects it, it implicitly asserts that there were no unexpected events before that.

I think what we need is

add test coverage of things that we expect to change; we now know that this includes the watch output of changes to linked files and linked directories within normal directories, since these continue to be in scope even if we don't watch linked directories
use this test coverage to evaluate whether the PR can land without further implementation changes or if we need to design+implement the new behaviour

bwilkerson · 2025-09-25T14:25:40Z

add test coverage of things that we expect to change; we now know that this includes the watch output of changes to linked files and linked directories within normal directories, since these continue to be in scope even if we don't watch linked directories

I forgot to explicitly state this, but if we do nothing then the behavior for symlink files will be unchanged. That is, before, if there was a directory D1 that contained a symlink to a directory D2, and the symlink was changed to link to D3, then we'd still get an OS level event for the change and we'd sometimes convert that to a WatchEvent and sometimes not. If we were to land what I have today then that behavior would be unchanged.

So unless we explicitly block events for symlinks (that is, if we land what's there today) we wouldn't be changing that behavior at all.

davidmorgan · 2025-09-25T15:30:45Z

In any case the first step is to cover any undefined behavior that we know about with tests, so it becomes defined, then we can go from there to deciding what it should be for the release, and documenting and dealing with any changes.

bwilkerson · 2025-09-25T21:43:19Z

In any case the first step is to cover any undefined behavior that we know about with tests ...

Respectfully, I'd like to challenge that. If I were taking ownership of this package (which I am absolutely not doing), then yes, it would be appropriate to write tests for previously undefined and untested behavior. But it isn't my responsibility to clean up the package. My responsibility is to solve the known performance problem in the analysis server and then look for other performance issues that I can solve. I think it's fine if, as a result of that work, the package isn't in worse shape than when I started fixing the real issue. In other words, I don't think it's a problem for the package's behavior around symlinks to remain undefined and untested as long as it's unchanged (which it is).

On the contrary, I think that given the relative priorities of improving the performance of the server and improving the handling of symlinks in this package, it would be irresponsible for me to spend more time on this PR than is absolutely necessary.

davidmorgan · 2025-09-26T09:19:44Z

I think this code is very high risk: it's genuinely cross platform, and by the nature of the package bugs can cause worse user experiences without ever failing in a way that would lead to a bug report being filed.

Given this, and how critical it is to build_runner + analyzer, I don't think "undefined" / "untested" / "unowned" are an acceptable state. I think it needs to be owned and then we push ahead with this + other improvements.

I chatted with Johnni and he's up for the Model team owning the package, and I'll be happy to take on the work here.

I am assuming there will be no objection to someone taking ownership :) but I will kick off that discussion separately to confirm.

For this change, I agree the current state of the tests is insufficient, and can take over adding them so it's possible to make progress.

bwilkerson · 2025-09-26T14:30:21Z

I agree that it would be good for the watcher package to be owned, and I'd be delighted for the dart model team to own it.

I also have no objections to you taking over this PR (which is what I think you're suggesting). I'll send you a git diff of the changes, together with the changes that I haven't uploaded yet (and can't upload because something got messed up).

davidmorgan · 2025-09-26T14:36:05Z

Sounds good to me--thanks!

munificent · 2025-09-26T16:15:39Z

Thanks for putting all the work into it so far @bwilkerson, and thanks @davidmorgan for taking ownership.

Don't follow symlinks when creating a DirectoryWatcher.

0c472a6

bwilkerson requested a review from a team as a code owner September 18, 2025 21:33

github-actions bot added the package:watcher label Sep 18, 2025

gemini-code-assist bot reviewed Sep 18, 2025

View reviewed changes

Fixed test code

0c7bfb0

Merge branch 'main' into stop-following-symlinks

39c11fb

bwilkerson requested a review from munificent September 18, 2025 21:42

munificent approved these changes Sep 18, 2025

View reviewed changes

davidmorgan reviewed Sep 19, 2025

View reviewed changes

		@@ -1,3 +1,12 @@
		## 2.0.0

		- Changes the behavior of the `DirectoryWatcher` so that it no longer follows

Don't follow symlinks when creating a DirectoryWatcher. #2167

Are you sure you want to change the base?

Don't follow symlinks when creating a DirectoryWatcher. #2167

Uh oh!

Conversation

bwilkerson commented Sep 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 18, 2025

PR Health

Uh oh!

bwilkerson commented Sep 18, 2025

Uh oh!

munificent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmorgan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwilkerson commented Sep 19, 2025

Uh oh!

davidmorgan commented Sep 19, 2025

Uh oh!

bwilkerson commented Sep 19, 2025

Uh oh!

devoncarew commented Sep 19, 2025

Uh oh!

bwilkerson commented Sep 21, 2025

Uh oh!

davidmorgan commented Sep 22, 2025

Uh oh!

devoncarew commented Sep 22, 2025

Uh oh!

bwilkerson commented Sep 22, 2025

Uh oh!

bwilkerson commented Sep 23, 2025

Uh oh!

bwilkerson commented Sep 23, 2025

Uh oh!

davidmorgan commented Sep 23, 2025