fix: update RecoveryFileManager to allow distinct files for multiple invocations of equivalent info (#2207)

BenWhitehead · web-flow · commit 44e9dd558bb9 · 2023-09-25T19:04:52.000-04:00
When creating a new recovery file it is important that distinct recovery files be created always, even if the provided BlobInfo is equivalent to a previously created one.

Allocate a UUID, and hash it with goodFastHash(64) before encoding to base64 (url safe).
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/RecoveryFileManager.java
@@ -19,22 +19,29 @@
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.primitives.Ints;
+import com.google.common.hash.HashCode;
+import com.google.common.hash.HashFunction;
+import com.google.common.hash.Hasher;
+import com.google.common.hash.Hashing;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Base64;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.UUID;
 
 final class RecoveryFileManager {
 
   private final ImmutableList<RecoveryVolume> volumes;
   /** Keep track of active info and file */
   private final Map<BlobInfo, RecoveryFile> files;
 
+  private final HashFunction hashFunction;
+
   /**
    * Round-robin assign recovery files to the configured volumes. Use this index to keep track of
    * which volume to assign to next.
@@ -45,13 +52,18 @@ private RecoveryFileManager(List<RecoveryVolume> volumes) {
     this.volumes = ImmutableList.copyOf(volumes);
     this.files = Collections.synchronizedMap(new HashMap<>());
     this.nextVolumeIndex = 0;
+    this.hashFunction = Hashing.goodFastHash(64);
   }
 
+  @SuppressWarnings("UnstableApiUsage")
   public RecoveryFile newRecoveryFile(BlobInfo info) {
     int i = getNextVolumeIndex();
     RecoveryVolume v = volumes.get(i);
-    int hashCode = info.hashCode();
-    String fileName = Base64.getUrlEncoder().encodeToString(Ints.toByteArray(hashCode));
+    UUID uuid = UUID.randomUUID();
+    String string = uuid.toString();
+    Hasher hasher = hashFunction.newHasher();
+    HashCode hash = hasher.putString(string, StandardCharsets.UTF_8).hash();
+    String fileName = Base64.getUrlEncoder().encodeToString(hash.asBytes());
     Path path = v.basePath.resolve(fileName);
     RecoveryFile recoveryFile = new RecoveryFile(path, v.sink, () -> files.remove(info));
     files.put(info, recoveryFile);
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/RecoveryFileManagerTest.java
@@ -16,7 +16,10 @@
 
 package com.google.cloud.storage;
 
+import static com.google.cloud.storage.TestUtils.assertAll;
+import static com.google.cloud.storage.TestUtils.xxd;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
@@ -33,6 +36,7 @@
 import java.time.Duration;
 import java.time.Instant;
 import java.util.Objects;
+import java.util.Random;
 import java.util.stream.Stream;
 import org.junit.Rule;
 import org.junit.Test;
@@ -143,4 +147,40 @@ public void fileAssignmentIsRoundRobin() throws IOException {
       assertThat(parentDirs).isEqualTo(ImmutableSet.of(tempDir1, tempDir2, tempDir3));
     }
   }
+
+  @Test
+  public void multipleRecoveryFilesForEqualBlobInfoAreAbleToExistConcurrently() throws Exception {
+    Path tempDir = temporaryFolder.newFolder(testName.getMethodName()).toPath();
+    RecoveryFileManager rfm =
+        RecoveryFileManager.of(
+            ImmutableList.of(tempDir),
+            path -> ThroughputSink.logged(path.toAbsolutePath().toString(), clock));
+
+    BlobInfo info = BlobInfo.newBuilder("bucket", "object").build();
+    try (RecoveryFile rf1 = rfm.newRecoveryFile(info);
+        RecoveryFile rf2 = rfm.newRecoveryFile(info); ) {
+
+      Random rand = new Random(467123);
+      byte[] bytes1 = DataGenerator.rand(rand).genBytes(7);
+      byte[] bytes2 = DataGenerator.rand(rand).genBytes(41);
+      try (WritableByteChannel writer = rf1.writer()) {
+        writer.write(ByteBuffer.wrap(bytes1));
+      }
+      try (WritableByteChannel writer = rf2.writer()) {
+        writer.write(ByteBuffer.wrap(bytes2));
+      }
+
+      byte[] actual1 = ByteStreams.toByteArray(Files.newInputStream(rf1.getPath()));
+      byte[] actual2 = ByteStreams.toByteArray(Files.newInputStream(rf2.getPath()));
+
+      String expected1 = xxd(bytes1);
+      String expected2 = xxd(bytes2);
+
+      String xxd1 = xxd(actual1);
+      String xxd2 = xxd(actual2);
+      assertAll(
+          () -> assertWithMessage("rf1 should contain bytes1").that(xxd1).isEqualTo(expected1),
+          () -> assertWithMessage("rf2 should contain bytes2").that(xxd2).isEqualTo(expected2));
+    }
+  }
 }
