Skip to content

FileWatchingService should not throw for missing file #126264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 7, 2025
Merged

FileWatchingService should not throw for missing file #126264

merged 5 commits into from
Apr 7, 2025

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Apr 4, 2025

Missing file is a valid state for FileWatchingService so that the exception should be suppressed.
@ywangd
FileWatchingService shoudld not throw for missing file
bad2d7e 
Missing file is a valid state for FileWatchingService so that the exception should be suppressed.
@ywangd ywangd added the :Core/Infra/Settings Settings infrastructure and APIs label Apr 4, 2025
@ywangd ywangd requested a review from jfreden April 4, 2025 06:55
@ywangd ywangd requested a review from a team as a code owner April 4, 2025 06:55
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Apr 4, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)
@elasticsearchmachine
Copy link
Collaborator

Hi @ywangd, I've created a changelog YAML for you.
ywangd
ywangd commented Apr 4, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/common/file/AbstractFileWatchingService.java
try {
attr = filesReadAttributes(path, BasicFileAttributes.class);
BasicFileAttributes attr = filesReadAttributes(path, BasicFileAttributes.class);
return new FileUpdateState(attr.lastModifiedTime().toMillis(), path.toRealPath().toString(), attr.fileKey());
Copy link
Member Author

@ywangd ywangd Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.toRealPath() can also throw NoSuchFileException and lead to failure like this one. So moving it inside the try-catch.
@jfreden
Copy link
Contributor

jfreden commented Apr 4, 2025

I agree with this change but looks like there are some unanswered questions:

  • What should happen when a file is removed, so when NoSuchFileException is thrown? This could happen when project files are removed. There is @FixForMultiProject on this, should there be a fileDeleted(Path path) that can be used to handle it in the extending class? My guess is that we shouldn't do anything until a project is deleted using the "deleted": true flag, so maybe this @FixForMultiProject is a noop.
  • For the test failure linked I don't see a stacktrace including the changed code?
@ywangd
Copy link
Member Author

ywangd commented Apr 4, 2025

Err I just realised that I linked the wrong build scan. The correct one is this (also fixed in the above comment).

This failure technically has nothing to do with MP. It failed on MP test because it happens to be the test that modify settings.json. The stacktrace is

[2025-04-03T17:44:44,244][INFO ][o.e.t.c.l.AbstractLocalClusterFactory][testBasicIndexOperationsWithOneProject] Updating config file 'operator/settings.json' [2025-04-03T17:44:44,243][ERROR][o.e.c.f.AbstractFileWatchingService] [index-KiHYNIT] shutting down watcher thread with exception java.nio.file.NoSuchFileException: /dev/shm/bk/bk-agent-prod-gcp-1743715723686534253/elastic/elasticsearch-serverless-validate-submodule/qa/multi-project-smoke-test/build/testrun/javaRestTest/temp/junit4634280600826086281/operator/settings.json	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)	at java.base/sun.nio.fs.UnixPath.toRealPath(UnixPath.java:856)	at org.elasticsearch.entitlement@9.1.0-SNAPSHOT/org.elasticsearch.entitlement.runtime.policy.PolicyManager.checkFileRead(PolicyManager.java:383)	at org.elasticsearch.entitlement@9.1.0-SNAPSHOT/org.elasticsearch.entitlement.runtime.api.ElasticsearchEntitlementChecker.checkPathToRealPath(ElasticsearchEntitlementChecker.java:2755)	at java.base/sun.nio.fs.UnixPath.toRealPath(UnixPath.java)	at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.common.file.AbstractFileWatchingService.readFileUpdateState(AbstractFileWatchingService.java:127)	at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.common.file.AbstractFileWatchingService.fileChanged(AbstractFileWatchingService.java:134)	at org.elasticsearch.server@9.1.0-SNAPSHOT/org.elasticsearch.common.file.AbstractFileWatchingService.watcherThread(AbstractFileWatchingService.java:247)	at java.base/java.lang.Thread.run(Thread.java:1447)

So the file watching service threw NoSuchFileException when the settings.json file (as a mutableResource) is updated by AbstractLocalClusterFactory. Java's Files#copy implementation deletes the existing file before writing the new file. Therefore there is a brief moment when file does not exist. This is the failure we are seeing. I hope this makes sense.
jfreden
jfreden approved these changes Apr 4, 2025
View reviewed changes
Copy link
Contributor

@jfreden jfreden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying! Yes, we should handle that case. LGTM!
@ywangd
Copy link
Member Author

ywangd commented Apr 4, 2025

@elasticmachine update branch
@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 4, 2025
@ywangd
Copy link
Member Author

ywangd commented Apr 7, 2025

@elasticmachine update branch
@ywangd ywangd changed the title FileWatchingService shoudld not throw for missing file FileWatchingService should not throw for missing file Apr 7, 2025
@ywangd
Copy link
Member Author

ywangd commented Apr 7, 2025

@elasticmachine update branch
@elasticsearchmachine elasticsearchmachine merged commit 997a7b8 into elastic:main Apr 7, 2025
17 checks passed
@ywangd ywangd deleted the do-not-throw-on-missing-file branch April 7, 2025 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Settings Settings infrastructure and APIs >enhancement Team:Core/Infra Meta label for core/infra team v9.1.0

4 participants

@ywangd @elasticsearchmachine @jfreden @elasticmachine