Derive worker files hash from a self-describing layout
`WorkerFilesHash#getCombinedHash()` hashed bytes and strings without also hashing their length. It also reencoded path strings from the internally used UTF-8-in-Latin-1 format to UTF-8 on macOS, which results in key mismatches for the worker across platforms and is unnecessarily slow.
Since this commit already changes the key, also use a slightly more efficient `Fingerprint` method to hash the args.
Closes #23701.
PiperOrigin-RevId: 689386153
Change-Id: I02d4a8d77ea37cdca633bd253eee58ecabdb7756
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index ff6012b..0da922f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -702,8 +702,9 @@
execRoot, Options.getDefaults(WorkerOptions.class), LocalEnvProvider.NOOP, null);
WorkerKey workerKey = workerParser.compute(spawn, context).getWorkerKey();
Fingerprint fingerprint = new Fingerprint();
+ // getWorkerFilesCombinedHash always uses SHA-256, so the hash is always 32 bytes.
fingerprint.addBytes(workerKey.getWorkerFilesCombinedHash().asBytes());
- fingerprint.addIterableStrings(workerKey.getArgs());
+ fingerprint.addStrings(workerKey.getArgs());
fingerprint.addStringMap(workerKey.getEnv());
return new ToolSignature(
fingerprint.hexDigestAndReset(), workerKey.getWorkerFilesWithDigests().keySet());
diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD
index 67a4914..395b895 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD
@@ -36,6 +36,7 @@
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/actions:runfiles_tree",
+ "//src/main/java/com/google/devtools/build/lib/unsafe:string",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//third_party:guava",
"//third_party:jsr305",
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
index f310370..2c0f1ba 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
@@ -26,9 +26,9 @@
import com.google.devtools.build.lib.actions.InputMetadataProvider;
import com.google.devtools.build.lib.actions.RunfilesTree;
import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.unsafe.StringUnsafe;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
-import java.nio.charset.Charset;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
@@ -47,7 +47,12 @@
Hasher hasher = Hashing.sha256().newHasher();
workerFilesMap.forEach(
(execPath, digest) -> {
- hasher.putString(execPath.getPathString(), Charset.defaultCharset());
+ String execPathString = execPath.getPathString();
+ hasher.putByte(StringUnsafe.getInstance().getCoder(execPathString));
+ hasher.putInt(execPathString.length());
+ hasher.putBytes(StringUnsafe.getInstance().getByteArray(execPathString));
+
+ hasher.putInt(digest.length);
hasher.putBytes(digest);
});
return hasher.hash();
@@ -71,7 +76,7 @@
/* keepEmptyTreeArtifacts= */ false,
/* keepMiddlemanArtifacts= */ true);
for (ActionInput tool : tools) {
- if ((tool instanceof Artifact) && ((Artifact) tool).isMiddlemanArtifact()) {
+ if (tool instanceof Artifact artifact && artifact.isMiddlemanArtifact()) {
RunfilesTree runfilesTree =
actionInputFileCache.getRunfilesMetadata(tool).getRunfilesTree();
PathFragment root = runfilesTree.getExecPath();
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index 61f08d3..7df124e 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -2351,7 +2351,7 @@
Platform.Property.newBuilder()
.setName("persistentWorkerKey")
.setValue(
- "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de"))
+ "628637504c26bb74fb6bb3f60fb7132b3aa574b866db4181770774882a8853e5"))
.build());
var merkleTree = remoteAction.getMerkleTree();
var outputDirectory =
@@ -2387,7 +2387,7 @@
Platform.Property.newBuilder()
.setName("persistentWorkerKey")
.setValue(
- "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de"))
+ "628637504c26bb74fb6bb3f60fb7132b3aa574b866db4181770774882a8853e5"))
.build());
// Check that if a tool input changes, the persistent worker key changes.
@@ -2399,7 +2399,7 @@
Platform.Property.newBuilder()
.setName("persistentWorkerKey")
.setValue(
- "997337de8dc20123cd7c8fcaed2c9c79cd8138831f9fbbf119f37d0859c9e83a"))
+ "98e07ff5afc8f4d127e93d326c87c132f89cfd009517422671e6abec2fe05e2b"))
.build());
}
