Don't keep the journal of a PersistentMap open

This ensures that the journal file is not kept open after a build, which has been observed to cause `RewindingTest` to fail on Windows, which disallows deleting open files (in this case during test case cleanup).

Since the journal is written out at most every 3 seconds for all current usages of the `PersistentMap` class, the overhead of the additional `open` is negligible.

Also include small tweaks to `RewindingTest` so that it can be enabled on Windows. In particular, the order of `SymlinkAction` and `SourceManifestAction` being rewound doesn't seem to be fixed and can differ on Windows.

Closes #28108.

PiperOrigin-RevId: 855242645
Change-Id: Ic7434139b290c6b0e2061977f747e890c3c5ece6
(cherry picked from commit 2be693e)

Author: fmeum

src/main/java/com/google/devtools/build/lib/util/PersistentMap.java

@@ -83,7 +83,6 @@ public abstract class PersistentMap<K, V> extends ForwardingConcurrentMap<K, V>
   private final Path journalFile;
 
   private final LinkedBlockingQueue<K> journal;
-  private DataOutputStream journalOut;
 
   /**
    * 'dirty' is true when the in-memory representation of the map is more recent than the on-disk
@@ -202,22 +201,24 @@ public V remove(Object object) {
    */
   private synchronized void writeJournal() {
     try {
-      if (journalOut == null) {
-        if (journalFile.exists()) {
-          // The journal file was left around after the last save() because
-          // keepJournal() was true. Append to it.
-          journalOut =
-              new DataOutputStream(new BufferedOutputStream(journalFile.getOutputStream(true)));
-        } else {
-          // Create new journal.
-          journalOut = createMapFile(journalFile);
-        }
+      DataOutputStream journalOut;
+      if (journalFile.exists()) {
+        // The journal file was left around after the last save() because
+        // keepJournal() was true. Append to it.
+        journalOut =
+            new DataOutputStream(new BufferedOutputStream(journalFile.getOutputStream(true)));
+      } else {
+        // Create new journal.
+        journalOut = createMapFile(journalFile);
+      }
+      try {
+        // Journal may have duplicates, we can ignore them.
+        LinkedHashSet<K> items = Sets.newLinkedHashSetWithExpectedSize(journal.size());
+        journal.drainTo(items);
+        writeEntries(journalOut, items, delegate());
+      } finally {
+        journalOut.close();
       }
-      // Journal may have duplicates, we can ignore them.
-      LinkedHashSet<K> items = Sets.newLinkedHashSetWithExpectedSize(journal.size());
-      journal.drainTo(items);
-      writeEntries(journalOut, items, delegate());
-      journalOut.flush();
     } catch (IOException e) {
       this.deferredIOFailure = e.getMessage() + " during journal append";
     }
@@ -303,8 +304,6 @@ private synchronized long save(boolean fullSave) throws IOException {
     if (dirty) {
       if (!fullSave && keepJournal()) {
         forceFlush();
-        journalOut.close();
-        journalOut = null;
         return journalSize() + cacheSize();
       } else {
         dirty = false;
@@ -343,12 +342,8 @@ protected boolean keepJournal() {
     return false;
   }
 
-  private synchronized void clearJournal() throws IOException {
+  private synchronized void clearJournal() {
     journal.clear();
-    if (journalOut != null) {
-      journalOut.close();
-      journalOut = null;
-    }
   }
 
   private synchronized void loadEntries(Path mapFile, boolean failFast) throws IOException {

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/BUILD

@@ -75,7 +75,6 @@ java_test(
     srcs = ["RewindingTest.java"],
     jvm_flags = ["-Djava.lang.Thread.allowVirtualThreads=true"],
     shard_count = 12,
-    tags = ["no_windows"],  # BuildIntegrationTestCase isn't fully compatible with Windows.
     deps = [
         ":rewinding_tests_helper",
         "//src/main/java/com/google/devtools/build/lib:runtime",

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java

@@ -1835,21 +1835,24 @@ final void runGeneratedRunfilesRewound(ImmutableList<String> lostRunfiles, Spawn
       HashSet<String> expectedRewoundGenrules =
           new HashSet<>(ImmutableList.of("//middle:gen1", "//middle:gen2"));
       int i = 0;
+      boolean sourceManifestActionSeen = false;
       while (i < 5) {
         assertThat(rewoundKeys.get(i)).isInstanceOf(ActionLookupData.class);
         ActionLookupData actionKey = (ActionLookupData) rewoundKeys.get(i);
         String actionLabel = actionKey.getLabel().getCanonicalForm();
         i++;
         if (actionLabel.equals("//middle:tool")) {
           switch (actionKey.getActionIndex()) {
-            case 0: // SymlinkAction
-              break;
-            case 1: // SourceManifestAction
-              assertActionKey(rewoundKeys.get(i), "//middle:tool", 2);
-              i++;
-              break;
-            default:
-              fail(String.format("Unexpected action index. actionKey: %s, i: %s", actionKey, i));
+            // SymlinkAction
+            case 0 -> {}
+            case 1 -> sourceManifestActionSeen = true;
+            // SymlinkTreeAction
+            case 2 -> assertThat(sourceManifestActionSeen).isTrue();
+            default ->
+                fail(
+                    String.format(
+                        "Unexpected action index. actionKey: %s, rewoundKeys: %s",
+                        actionKey, rewoundKeys));
           }
         } else {
           assertThat(expectedRewoundGenrules.remove(actionLabel)).isTrue();
@@ -2012,33 +2015,31 @@ final void runDupeDirectAndRunfilesDependencyRewound(
 
     if (buildRunfileManifests()) {
       assertThat(rewoundKeys).hasSize(6);
-      int i = 0;
-      while (i < 4) {
+      boolean sourceManifestActionSeen = false;
+      for (int i = 0; i < 4; i++) {
         assertThat(rewoundKeys.get(i)).isInstanceOf(ActionLookupData.class);
         ActionLookupData actionKey = (ActionLookupData) rewoundKeys.get(i);
         String actionLabel = actionKey.getLabel().getCanonicalForm();
-        i++;
         if (actionLabel.equals("//test:tool")) {
           switch (actionKey.getActionIndex()) {
-            case 0: // SymlinkAction
-              break;
-            case 1: // SourceManifestAction
-              assertActionKey(rewoundKeys.get(i), "//test:tool", /* index= */ 2);
-              i++;
-              break;
-            default:
-              fail(
-                  String.format(
-                      "Unexpected action index. actionKey: %s, rewoundKeys: %s",
-                      actionKey, rewoundKeys));
+            // SymlinkAction
+            case 0 -> {}
+            case 1 -> sourceManifestActionSeen = true;
+            // SymlinkTreeAction
+            case 2 -> assertThat(sourceManifestActionSeen).isTrue();
+            default ->
+                fail(
+                    String.format(
+                        "Unexpected action index. actionKey: %s, rewoundKeys: %s",
+                        actionKey, rewoundKeys));
           }
         } else {
           assertThat(actionLabel).isEqualTo("//test:rule1");
         }
       }
 
-      assertActionKey(rewoundKeys.get(i++), "//test:tool", /* index= */ 3);
-      assertArtifactKey(rewoundKeys.get(i), "_middlemen/test_Stool-runfiles");
+      assertActionKey(rewoundKeys.get(4), "//test:tool", /* index= */ 3);
+      assertArtifactKey(rewoundKeys.get(5), "_middlemen/test_Stool-runfiles");
     } else {
       assertThat(rewoundKeys).hasSize(4);
       int i = 0;
@@ -2168,21 +2169,24 @@ def _tree_impl(ctx):
     if (buildRunfileManifests()) {
       assertThat(rewoundKeys).hasSize(7);
       int i = 0;
+      boolean sourceManifestActionSeen = false;
       while (i < 5) {
         assertThat(rewoundKeys.get(i)).isInstanceOf(ActionLookupData.class);
         ActionLookupData actionKey = (ActionLookupData) rewoundKeys.get(i);
         String actionLabel = actionKey.getLabel().getCanonicalForm();
         i++;
         if (actionLabel.equals("//middle:tool")) {
           switch (actionKey.getActionIndex()) {
-            case 0: // SymlinkAction
-              break;
-            case 1: // SourceManifestAction
-              assertActionKey(rewoundKeys.get(i), "//middle:tool", 2);
-              i++;
-              break;
-            default:
-              fail(String.format("Unexpected action index. actionKey: %s", actionKey));
+            // SymlinkAction
+            case 0 -> {}
+            case 1 -> sourceManifestActionSeen = true;
+            // SymlinkTreeAction
+            case 2 -> assertThat(sourceManifestActionSeen).isTrue();
+            default ->
+                fail(
+                    String.format(
+                        "Unexpected action index. actionKey: %s, rewoundKeys: %s",
+                        actionKey, rewoundKeys));
           }
         } else {
           assertThat(actionLabel).isEqualTo("//middle:gen_tree");