Use ctime in file digest cache key #18003

fmeum · 2023-04-06T06:46:22Z

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for --cache_computed_file_digests, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with mv, which preserves inodes.

Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to WindowsFileOperation. Adding a call to this function to stat uncovered previously silent bugs where Unix-style PathFragments were created on Windows:

Bzlmod's createLocalRepoSpec did not correctly extract the path from a registry's file:// URI on Windows.
--package_path isn't usable with absolute paths on Windows as it splits on :. Since the flag is deprecated, this commit fixes the tests rather than the implementation.

Fixes #14723

fmeum · 2023-04-06T07:51:30Z

@janakdr Could you review this fix?

test_cache_computed_file_digests_behavior is failing as it incurs two cache misses rather than getting the expected cache hit for stable-status.txt. This is surprising to me as BazelWorkspaceStatusAction only recreates stable-status.txt if the content changed, which it shouldn't in this test.

Edit: Is it possible that ctime is changed when the action inputs are made read-only? That could make the digest cache useless for output files with this change.

Edit: Can confirm that the failure goes away if I comment out https://cs.opensource.google/bazel/bazel/+/9800ffd1a471582fb42fd782e2e2eeb39dba6c9b:src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java;l=731, so this probably requires more extensive changes.

Edit: The Windows file system implementation does not support ctimes, which means that the issue isn't fixed by this PR on Windows. However, ctime could be obtained with native calls.

meteorcloudy · 2023-04-06T10:29:25Z

/cc @justinhorvitz can you take a look at this?

justinhorvitz · 2023-04-06T15:11:49Z

test_cache_computed_file_digests_behavior is failing as it incurs two cache misses rather than getting the expected cache hit for stable-status.txt. This is surprising to me as BazelWorkspaceStatusAction only recreates stable-status.txt if the content changed, which it shouldn't in this test.

Does the latest change in 1affa3e fix this?

fmeum · 2023-04-06T15:18:32Z

@justinhorvitz Yes, at least as far as the tests are concerned. I don't know whether the cache thrashing is only relevant for stable-status.txt because it's generating actions runs on every build or whether this would affect performance more broadly. If it could, then tree artifact files may still be affected.

Regarding ctime on Windows: I could add native functions that get it, but this requires an additional stat call per file. I have no intuition for performance on Windows, would that still be better than disabling the digest cache?

src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java

justinhorvitz · 2023-04-06T15:29:13Z

@justinhorvitz Yes, at least as far as the tests are concerned. I don't know whether the cache thrashing is only relevant for stable-status.txt because it's generating actions runs on every build or whether this would affect performance more broadly. If it could, then tree artifact files may still be affected.

Regarding ctime on Windows: I could add native functions that get it, but this requires an additional stat call per file. I have no intuition for performance on Windows, would that still be better than disabling the digest cache?

I don't have much personal stake in external bazel or Windows performance so let's ask someone else. @tjgq do you see any potential scenario where tree artifact outputs would have a changed ctime but the same mtime?

fmeum · 2023-04-06T15:46:36Z

I don't have much personal stake in external bazel or Windows performance so let's ask someone else. @tjgq do you see any potential scenario where tree artifact outputs would have a changed ctime but the same mtime?

Just want to note that these are separate issues:

ActionMetadataHandler may still thrash the digest cache for tree artifacts by unconditionally chmodding them on all OSes.
Windows doesn't provide an actual ctime, which means that this PR can't fix the actual issue.

justinhorvitz · 2023-04-06T15:51:40Z

I don't have much personal stake in external bazel or Windows performance so let's ask someone else. @tjgq do you see any potential scenario where tree artifact outputs would have a changed ctime but the same mtime?

Just want to note that these are separate issues:

ActionMetadataHandler may still thrash the digest cache for tree artifacts by unconditionally chmodding them on all OSes.

Windows doesn't provide an actual ctime, which means that this PR can't fix the actual issue.

Yup, understood.

justinhorvitz · 2023-04-06T16:13:19Z

src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs/InMemoryContentInfo.java

@@ -120,6 +120,22 @@ public long getNodeId() {
    return System.identityHashCode(this);
  }

+  @Override
+  public synchronized int getPermissions() {


I don't see what this needs to be synchronized with. Also, please make it final.

Made it final and removed synchronized. I am not entirely sure about the intended behavior of getPermissions when the file permissions are updated concurrently, but I agree that synchronized doesn't help as the setters aren't synchronized.

justinhorvitz

This PR looks good to me, but I think we should get a second opinion specifically for the tree artifact question.

src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java

fmeum · 2023-04-12T06:08:46Z

@bazel-io flag

keertk · 2023-04-12T15:44:43Z

@bazel-io fork 6.2.0

fmeum · 2023-04-12T17:07:31Z

@keertk @meteorcloudy I think that we should probably fork this for 5.4.1 and 6.1.2 as well as it fixes a correctness issue.

keertk · 2023-04-12T17:17:29Z

@bazel-io fork 5.4.1

keertk · 2023-04-12T17:18:04Z

@bazel-io fork 6.1.2

meteorcloudy · 2023-04-13T09:12:12Z

@fmeum Some update: we are importing this CL, but one internal test was broken, looking into a fix.

justinhorvitz · 2023-04-13T20:20:51Z

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -727,7 +727,8 @@ private static FileArtifactValue fileArtifactValueFromStat(
  }

  private void setPathPermissionsIfFile(Path path) throws IOException {
-    if (path.isFile(Symlinks.NOFOLLOW)) {
+    FileStatus stat = path.stat(Symlinks.NOFOLLOW);


Should this use statIfFound? All of the other stats in this file use that to tolerate a nonexistent file.

True, it should to preserve the original semantics. I pushed a commit to do this. I am a bit surprised that no test failed due to this change - do all missing files at this point result in a missing outputs error down the line anyway?

justinhorvitz · 2023-04-13T20:21:16Z

@fmeum Some update: we are importing this CL, but one internal test was broken, looking into a fix.

The broken test was counting the number of stats, but wasn't doing so in a very robust way. I've updated it which should unblock the import. Meanwhile, while looking into this, I found I have one more question which I asked in ActionMetadataHandler.

ulrfa · 2023-04-14T10:28:27Z

I'm thinking about https://bazel.build/reference/command-line-reference#flag--experimental_use_hermetic_linux_sandbox that populates sandboxes by creating hard links.

Ctime is changed when additional hard links are created to existing inodes (link count changes).

How is this change affecting the file digest cache for that use case?

fmeum · 2023-04-14T10:33:42Z

I'm thinking about https://bazel.build/reference/command-line-reference#flag--experimental_use_hermetic_linux_sandbox that populates sandboxes by creating hard links.

Ctime is changed when additional hard links are created to existing inodes (link count changes).

How is this change affecting the file digest cache for that use case?

I assume it would make it so that digests are recomputed every time a hard link is created, which doesn't sound good. Could you maybe try this out with Bazel built from this PR?

I don't know enough about the hermetic sandbox to think of ways to improve digest caching for it. Since Bazel doesn't really care about changes to metadata, we would ideally like to have a "modification time, but not overrideable by users", for which ctime is really just a proxy, but I don't see a way to get this more restricted information efficiently.

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes. Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows: 1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows. 2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation. Fixes #14723 Closes #18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85 Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes. Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows: 1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows. 2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation. Fixes bazelbuild#14723 Closes bazelbuild#18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes. Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows: 1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows. 2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation. Fixes #14723 Closes #18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85

File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes. Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows: 1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows. 2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation. Fixes bazelbuild#14723 Closes bazelbuild#18003. PiperOrigin-RevId: 524297459 Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85

fmeum force-pushed the 14723-ctime-in-cache branch from 856188a to e6e6e50 Compare April 6, 2023 07:10

fmeum mentioned this pull request Apr 6, 2023

Action outputs with fixed timestamps cause problems #14723

Closed

fmeum force-pushed the 14723-ctime-in-cache branch 2 times, most recently from 2723c48 to 254d490 Compare April 6, 2023 07:49

fmeum force-pushed the 14723-ctime-in-cache branch 2 times, most recently from ad913d0 to 26c0be6 Compare April 6, 2023 09:25

meteorcloudy added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Apr 6, 2023

meteorcloudy requested a review from justinhorvitz April 6, 2023 10:22

fmeum force-pushed the 14723-ctime-in-cache branch from 94b6c3c to 0ba0fad Compare April 6, 2023 13:04

justinhorvitz requested changes Apr 6, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java Show resolved Hide resolved

fmeum force-pushed the 14723-ctime-in-cache branch from 1affa3e to a2c9a39 Compare April 6, 2023 16:08

justinhorvitz reviewed Apr 6, 2023

View reviewed changes

fmeum force-pushed the 14723-ctime-in-cache branch from a2c9a39 to 984683f Compare April 6, 2023 16:24

justinhorvitz requested a review from tjgq April 6, 2023 16:33

justinhorvitz approved these changes Apr 6, 2023

View reviewed changes

fmeum commented Apr 6, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java Show resolved Hide resolved

fmeum force-pushed the 14723-ctime-in-cache branch 7 times, most recently from beac457 to 8043408 Compare April 9, 2023 14:11

meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 11, 2023

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 12, 2023

bazel-io mentioned this pull request Apr 12, 2023

[6.2.0] Use ctime in file digest cache key #18055

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 12, 2023

bazel-io mentioned this pull request Apr 12, 2023

[5.4.1] Use ctime in file digest cache key #18059

Closed

bazel-io mentioned this pull request Apr 12, 2023

[6.1.2] Use ctime in file digest cache key #18060

Closed

justinhorvitz reviewed Apr 13, 2023

View reviewed changes

Use statIfFound

e19dae7

copybara-service bot closed this in 763f1d9 Apr 14, 2023

fmeum deleted the 14723-ctime-in-cache branch April 14, 2023 15:31

keertk removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 14, 2023

This was referenced Apr 14, 2023

[6.1.2] Use ctime in file digest cache key #18101

Merged

[5.4.1] Use ctime in file digest cache key #18102

Closed

keertk mentioned this pull request Apr 14, 2023

[6.2.0] Use ctime in file digest cache key #18105

Merged

fmeum mentioned this pull request Apr 17, 2023

Use ctime in file digest cache key #18115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ctime in file digest cache key #18003

Use ctime in file digest cache key #18003

fmeum commented Apr 6, 2023 •

edited

Loading

fmeum commented Apr 6, 2023 •

edited

Loading

meteorcloudy commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

fmeum commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

fmeum commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

justinhorvitz Apr 6, 2023

fmeum Apr 6, 2023

justinhorvitz left a comment

fmeum commented Apr 12, 2023

keertk commented Apr 12, 2023

fmeum commented Apr 12, 2023

keertk commented Apr 12, 2023

keertk commented Apr 12, 2023

meteorcloudy commented Apr 13, 2023 •

edited

Loading

justinhorvitz Apr 13, 2023

fmeum Apr 13, 2023

justinhorvitz commented Apr 13, 2023

ulrfa commented Apr 14, 2023

fmeum commented Apr 14, 2023

Use ctime in file digest cache key #18003

Use ctime in file digest cache key #18003

Conversation

fmeum commented Apr 6, 2023 • edited Loading

fmeum commented Apr 6, 2023 • edited Loading

meteorcloudy commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

fmeum commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

fmeum commented Apr 6, 2023

justinhorvitz commented Apr 6, 2023

justinhorvitz Apr 6, 2023

Choose a reason for hiding this comment

fmeum Apr 6, 2023

Choose a reason for hiding this comment

justinhorvitz left a comment

Choose a reason for hiding this comment

fmeum commented Apr 12, 2023

keertk commented Apr 12, 2023

fmeum commented Apr 12, 2023

keertk commented Apr 12, 2023

keertk commented Apr 12, 2023

meteorcloudy commented Apr 13, 2023 • edited Loading

justinhorvitz Apr 13, 2023

Choose a reason for hiding this comment

fmeum Apr 13, 2023

Choose a reason for hiding this comment

justinhorvitz commented Apr 13, 2023

ulrfa commented Apr 14, 2023

fmeum commented Apr 14, 2023

fmeum commented Apr 6, 2023 •

edited

Loading

fmeum commented Apr 6, 2023 •

edited

Loading

meteorcloudy commented Apr 13, 2023 •

edited

Loading