Reduce locking and improve concurrency on cache hits

build.gradle

@@ -94,6 +94,7 @@ dependencies {
     implementation 'net.fabricmc:fabric-loom-native:0.2.1'
     implementation 'net.neoforged:srgutils:1.0.10'
     implementation 'net.neoforged.installertools:problems-api:3.0.3'
+    implementation 'net.mezzdev:readwritefilelock:0.2.1'
     annotationProcessor 'info.picocli:picocli-codegen:4.7.6'
 
     testImplementation platform('org.junit:junit-bom:5.13.2')

src/main/java/net/neoforged/neoform/runtime/cache/CacheKeyBuilder.java

@@ -31,7 +31,7 @@ public void addPaths(String component, Collection<Path> resultPath) {
             try {
                 return new CacheKey.AnnotatedValue(fileHashService.getHashValue(path), path.toString());
             } catch (IOException e) {
-                throw new UncheckedIOException(e);
+                throw new UncheckedIOException(createHashFailureExceptionMessage(component, path), e);
             }
         }).toList());
     }
@@ -41,7 +41,7 @@ public void addPath(String component, Path path) {
         try {
             hashValue = fileHashService.getHashValue(path);
         } catch (IOException e) {
-            throw new UncheckedIOException(e);
+            throw new UncheckedIOException(createHashFailureExceptionMessage(component, path), e);
         }
 
         add(component, hashValue, prettifyPath(path));
@@ -126,4 +126,10 @@ public CacheKey build() {
     public FileHashService getFileHashService() {
         return fileHashService;
     }
+
+    private static String createHashFailureExceptionMessage(String component, Path path) {
+        return "Failed to hash path " + path + " for cache key component '" + component
+                + "'. The file may have been deleted while this NeoFormRuntime run was still using it, "
+                + "for example by an older NeoFormRuntime version or by manual deletion.";
+    }
 }

src/main/java/net/neoforged/neoform/runtime/cache/CacheManager.java

@@ -1,5 +1,6 @@
 package net.neoforged.neoform.runtime.cache;
 
+import net.mezzdev.readwritefilelock.ReadWriteFileLock;
 import net.neoforged.neoform.runtime.graph.ExecutionNode;
 import net.neoforged.neoform.runtime.utils.AnsiColor;
 import net.neoforged.neoform.runtime.utils.FileUtil;
@@ -8,6 +9,7 @@
 import net.neoforged.neoform.runtime.utils.StringUtil;
 import org.jetbrains.annotations.Nullable;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
@@ -60,6 +62,13 @@ public class CacheManager implements AutoCloseable {
     private final Path assetsDir;
     private final Path workspacesDir;
 
+    /**
+     * Coordinates cleanup with processes that are using intermediate cache files.
+     * Readers hold the lock while returned cache paths may still be copied. Cleanup
+     * holds the write lock before deleting cache files.
+     */
+    private final ReadWriteFileLock cacheUseLock;
+
     /**
      * Maximum age of cache entries in the intermediate work cache in hours.
      */
@@ -80,6 +89,7 @@ public CacheManager(Path homeDir, @Nullable Path assetsDir, Path workspacesDir)
         this.intermediateResultsDir = homeDir.resolve("intermediate_results");
         this.assetsDir = Objects.requireNonNullElse(assetsDir, homeDir.resolve("assets"));
         this.workspacesDir = workspacesDir;
+        this.cacheUseLock = ReadWriteFileLock.forFile(homeDir.resolve("nfrt_cache_use.lock"));
     }
 
     public void performMaintenance() throws IOException {
@@ -109,11 +119,18 @@ public void performMaintenance() throws IOException {
                     return;
                 }
 
-                LOG.println("Performing periodic cache maintenance on " + homeDir);
+                try (var useLock = cacheUseLock.tryLockForWrite()) {
+                    if (useLock == null) {
+                        LOG.println("Cache is currently in use. Skipping periodic cache maintenance.");
+                        return;
+                    }
+
+                    LOG.println("Performing periodic cache maintenance on " + homeDir);
 
-                cleanUpIntermediateResults();
+                    cleanUpIntermediateResultsLocked();
 
-                Files.setLastModifiedTime(cacheLock, FileTime.from(Instant.now()));
+                    Files.setLastModifiedTime(cacheLock, FileTime.from(Instant.now()));
+                }
 
                 return;
             }
@@ -126,6 +143,20 @@ public void cleanUpAll() throws IOException {
         cleanUpIntermediateResults();
     }
 
+    /**
+     * Prevents cleanup from deleting intermediate-cache files while they're being used.
+     * <p>
+     * The node lock still protects each cache key. This lock covers the longer lifetime
+     * where returned cache paths may be read after the node lock has been released.
+     */
+    public Closeable lockCacheForUse() throws IOException {
+        if (disabled) {
+            return () -> {
+            };
+        }
+        return cacheUseLock.lockForRead();
+    }
+
     /**
      * Cleans the cache of intermediate results based on two goals:
      * <ol>
@@ -135,6 +166,12 @@ public void cleanUpAll() throws IOException {
      * </ul>
      */
     public void cleanUpIntermediateResults() throws IOException {
+        try (var lock = cacheUseLock.lockForWrite()) {
+            cleanUpIntermediateResultsLocked();
+        }
+    }
+
+    private void cleanUpIntermediateResultsLocked() throws IOException {
         if (!Files.exists(intermediateResultsDir)) {
             return;
         }
@@ -248,13 +285,20 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
     }
 
     public boolean restoreOutputsFromCache(ExecutionNode node, CacheKey cacheKey, Map<String, Path> outputValues) throws IOException {
+        return restoreOutputsFromCache(node, cacheKey, outputValues, analyzeMisses);
+    }
+
+    public boolean restoreOutputsFromCacheWithoutMissAnalysis(ExecutionNode node, CacheKey cacheKey, Map<String, Path> outputValues) throws IOException {
+        return restoreOutputsFromCache(node, cacheKey, outputValues, false);
+    }
+
+    private boolean restoreOutputsFromCache(ExecutionNode node, CacheKey cacheKey, Map<String, Path> outputValues, boolean analyzeMisses) throws IOException {
         if (disabled) {
             return false;
         }
 
         var intermediateCacheDir = getIntermediateResultsDir();
         var cacheMarkerFile = getCacheMarkerFile(cacheKey);
-        Files.createDirectories(intermediateCacheDir);
         if (Files.isRegularFile(cacheMarkerFile)) {
             // Try to rebuild output values from cache
             boolean complete = true;
@@ -287,6 +331,7 @@ public void saveOutputs(ExecutionNode node, CacheKey cacheKey, HashMap<String, P
         }
 
         var intermediateCacheDir = getIntermediateResultsDir();
+        Files.createDirectories(intermediateCacheDir);
         var finalOutputValues = new HashMap<String, Path>(outputValues.size());
         for (var entry : outputValues.entrySet()) {
             var filename = cacheKey + "_" + entry.getKey() + node.getRequiredOutput(entry.getKey()).type().getExtension();

src/main/java/net/neoforged/neoform/runtime/cli/RunNeoFormCommand.java

@@ -23,8 +23,6 @@
 import net.neoforged.neoform.runtime.graph.transforms.ModifyAction;
 import net.neoforged.neoform.runtime.graph.transforms.ReplaceNodeInput;
 import net.neoforged.neoform.runtime.graph.transforms.ReplaceNodeOutput;
-import net.neoforged.neoform.runtime.utils.FileUtil;
-import net.neoforged.neoform.runtime.utils.HashingUtil;
 import net.neoforged.neoform.runtime.utils.Logger;
 import net.neoforged.neoform.runtime.utils.MavenCoordinate;
 import net.neoforged.neoform.runtime.utils.ToolCoordinate;
@@ -35,11 +33,8 @@
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.nio.charset.StandardCharsets;
-import java.nio.file.Files;
-import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.Base64;
 import java.util.Collections;
@@ -462,25 +457,7 @@ private void execute(NeoFormEngine engine) throws InterruptedException, IOExcept
             System.exit(1);
         }
 
-        var results = engine.createResults(neededResults.keySet().toArray(new String[0]));
-
-        for (var entry : neededResults.entrySet()) {
-            var result = results.get(entry.getKey());
-            if (result == null) {
-                throw new IllegalStateException("Result " + entry.getKey() + " was requested but not produced");
-            }
-            var resultFileHash = HashingUtil.hashFile(result, "SHA-1");
-            try {
-                if (HashingUtil.hashFile(entry.getValue(), "SHA-1").equals(resultFileHash)) {
-                    continue; // Nothing to do the file already matches
-                }
-            } catch (NoSuchFileException ignored) {
-            }
-
-            var tmpFile = Paths.get(entry.getValue() + ".tmp");
-            Files.copy(result, tmpFile, StandardCopyOption.REPLACE_EXISTING);
-            FileUtil.atomicMove(tmpFile, entry.getValue());
-        }
+        engine.writeResults(neededResults);
     }
 
     private static ApplySourceTransformAction getOrAddTransformSourcesAction(NeoFormEngine engine) {

src/main/java/net/neoforged/neoform/runtime/engine/NeoFormEngine.java

@@ -36,6 +36,8 @@
 import net.neoforged.neoform.runtime.graph.transforms.GraphTransform;
 import net.neoforged.neoform.runtime.graph.transforms.ReplaceNodeOutput;
 import net.neoforged.neoform.runtime.utils.AnsiColor;
+import net.neoforged.neoform.runtime.utils.FileUtil;
+import net.neoforged.neoform.runtime.utils.HashingUtil;
 import net.neoforged.neoform.runtime.utils.JavaInstallationInformation;
 import net.neoforged.neoform.runtime.utils.Logger;
 import net.neoforged.neoform.runtime.utils.MavenCoordinate;
@@ -45,9 +47,12 @@
 import org.jetbrains.annotations.Nullable;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.PrintWriter;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -568,7 +573,7 @@ private synchronized CompletableFuture<Void> getWaitCondition(ExecutionNode node
         return future;
     }
 
-    public void runNode(ExecutionNode node) throws InterruptedException {
+    private void runNode(ExecutionNode node) throws InterruptedException {
         // Wait for pre-requisites
         Set<ExecutionNode> dependencies = Collections.newSetFromMap(new IdentityHashMap<>());
         for (var input : node.inputs().values()) {
@@ -590,9 +595,37 @@ public void runNode(ExecutionNode node) throws InterruptedException {
             LOG.println(AnsiColor.MUTED + StringUtil.indent(cacheKey.describe(), 2) + AnsiColor.RESET);
         }
 
+        try {
+            // Check for a complete cache hit before taking the node lock. This
+            // lets parallel Gradle/NFRT processes reuse a published cache entry
+            // without waiting behind another process that is producing or
+            // checking the same cache key.
+            //
+            // writeResults holds the cache-use lock while nodes run and
+            // requested results are copied. With cleanup excluded, the restored
+            // cache paths remain valid for the rest of that operation. Writers
+            // publish entries by moving every output into the cache before
+            // writing the marker file, and restore only succeeds when that
+            // marker and every declared output file are present.
+            //
+            // Skip miss analysis here; the locked fallback below performs the
+            // normal restore check and miss analysis.
+            var preLockOutputValues = new HashMap<String, Path>();
+            if (cacheManager.restoreOutputsFromCacheWithoutMissAnalysis(node, cacheKey, preLockOutputValues)) {
+                node.complete(preLockOutputValues, true);
+                return;
+            }
+        } catch (Throwable t) {
+            node.fail();
+            throw new NodeExecutionException(node, t);
+        }
+
         try (var lock = lockManager.lock(cacheKey.toString())) {
             var outputValues = new HashMap<String, Path>();
 
+            // Keep the locked restore as a race-safe fallback to avoid doing
+            // the work twice. Another process may have populated the cache
+            // while this invocation was waiting for the node lock.
             if (cacheManager.restoreOutputsFromCache(node, cacheKey, outputValues)) {
                 node.complete(outputValues, true);
                 return;
@@ -618,7 +651,7 @@ public ArtifactManager getArtifactManager() {
         return artifactManager;
     }
 
-    public Map<String, Path> createResults(String... ids) throws InterruptedException {
+    private Map<String, Path> createResults(Iterable<String> ids) throws InterruptedException {
         // Determine the nodes we need to run
         Set<ExecutionNode> nodes = Collections.newSetFromMap(new IdentityHashMap<>());
         for (String id : ids) {
@@ -636,11 +669,74 @@ public Map<String, Path> createResults(String... ids) throws InterruptedExceptio
         for (String id : ids) {
             var nodeOutput = graph.getResult(id);
             results.put(id, nodeOutput.getResultPath());
-            // TODO: move to actual result cache
         }
         return results;
     }
 
+    /**
+     * Creates requested results and copies them to their destination paths before
+     * returning.
+     * <p>
+     * Node outputs may point into the intermediate cache. The cache-use lock is
+     * acquired before nodes are scheduled and remains held until every requested
+     * result has been copied.
+     */
+    public void writeResults(Map<String, Path> destinations) throws InterruptedException, IOException {
+        // Restored outputs are paths in the intermediate cache, not copies.
+        // Hold the cache-use lock before any nodes run so cleanup cannot delete
+        // those paths before later nodes or result copies read them.
+        try (var lock = cacheManager.lockCacheForUse()) {
+            var results = createResults(destinations.keySet());
+            for (var entry : destinations.entrySet()) {
+                var result = results.get(entry.getKey());
+                if (result == null) {
+                    throw new IllegalStateException("Internal error: createResults did not return an output path for "
+                                                    + "requested result '" + entry.getKey() + "'.");
+                }
+                writeResult(entry.getKey(), result, entry.getValue());
+            }
+        }
+    }
+
+    private static void writeResult(String resultId, Path result, Path destination) throws IOException {
+        var resultFileHash = hashResult(resultId, result);
+        try {
+            if (HashingUtil.hashFile(destination, "SHA-1").equals(resultFileHash)) {
+                return;
+            }
+        } catch (NoSuchFileException ignored) {
+        }
+
+        var tmpFile = Path.of(destination + ".tmp");
+        try (var input = openResult(resultId, result)) {
+            Files.copy(input, tmpFile, StandardCopyOption.REPLACE_EXISTING);
+        }
+        FileUtil.atomicMove(tmpFile, destination);
+    }
+
+    private static String hashResult(String resultId, Path result) throws IOException {
+        try {
+            return HashingUtil.hashFile(result, "SHA-1");
+        } catch (NoSuchFileException e) {
+            throw new IOException(createMissingResultExceptionMessage(resultId, result), e);
+        }
+    }
+
+    private static InputStream openResult(String resultId, Path result) throws IOException {
+        try {
+            return Files.newInputStream(result);
+        } catch (NoSuchFileException e) {
+            throw new IOException(createMissingResultExceptionMessage(resultId, result), e);
+        }
+    }
+
+    private static String createMissingResultExceptionMessage(String resultId, Path result) {
+        return "Result '" + resultId + "' could not be written because its output path is "
+            + "missing: " + result + ". If this path is in the intermediate cache, it may "
+            + "have been deleted by an older NeoFormRuntime version or manual deletion "
+            + "while this process was still using it.";
+    }
+
     public void dumpGraph(PrintWriter printWriter) {
         graph.dump(printWriter);
     }