diff --git a/build.gradle b/build.gradle index 4c7f9cb..d9e8e6e 100644 --- a/build.gradle +++ b/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.3.1' annotationProcessor 'info.picocli:picocli-codegen:4.7.6' testImplementation platform('org.junit:junit-bom:5.13.2') diff --git a/src/main/java/net/neoforged/neoform/runtime/cache/CacheKeyBuilder.java b/src/main/java/net/neoforged/neoform/runtime/cache/CacheKeyBuilder.java index d9ef4ba..fce83ed 100644 --- a/src/main/java/net/neoforged/neoform/runtime/cache/CacheKeyBuilder.java +++ b/src/main/java/net/neoforged/neoform/runtime/cache/CacheKeyBuilder.java @@ -31,7 +31,7 @@ public void addPaths(String component, Collection 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."; + } } diff --git a/src/main/java/net/neoforged/neoform/runtime/cache/CacheManager.java b/src/main/java/net/neoforged/neoform/runtime/cache/CacheManager.java index 06c5c58..8bb1f30 100644 --- a/src/main/java/net/neoforged/neoform/runtime/cache/CacheManager.java +++ b/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. + *

+ * 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: *

    @@ -135,6 +166,12 @@ public void cleanUpAll() throws IOException { * */ public void cleanUpIntermediateResults() throws IOException { + try (var lock = cacheUseLock.lockForWrite()) { + cleanUpIntermediateResultsLocked(); + } + } + + private void cleanUpIntermediateResultsLocked() throws IOException { if (!Files.exists(intermediateResultsDir)) { return; } diff --git a/src/main/java/net/neoforged/neoform/runtime/cli/RunNeoFormCommand.java b/src/main/java/net/neoforged/neoform/runtime/cli/RunNeoFormCommand.java index 6f39777..fffe3a9 100644 --- a/src/main/java/net/neoforged/neoform/runtime/cli/RunNeoFormCommand.java +++ b/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) { diff --git a/src/main/java/net/neoforged/neoform/runtime/engine/NeoFormEngine.java b/src/main/java/net/neoforged/neoform/runtime/engine/NeoFormEngine.java index fc6b934..74b9171 100644 --- a/src/main/java/net/neoforged/neoform/runtime/engine/NeoFormEngine.java +++ b/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 getWaitCondition(ExecutionNode node return future; } - public void runNode(ExecutionNode node) throws InterruptedException { + private void runNode(ExecutionNode node) throws InterruptedException { // Wait for pre-requisites Set dependencies = Collections.newSetFromMap(new IdentityHashMap<>()); for (var input : node.inputs().values()) { @@ -618,7 +623,7 @@ public ArtifactManager getArtifactManager() { return artifactManager; } - public Map createResults(String... ids) throws InterruptedException { + private Map createResults(Iterable ids) throws InterruptedException { // Determine the nodes we need to run Set nodes = Collections.newSetFromMap(new IdentityHashMap<>()); for (String id : ids) { @@ -636,11 +641,74 @@ public Map 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. + *

    + * 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 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); } diff --git a/src/test/java/net/neoforged/neoform/runtime/engine/NeoFormEngineTest.java b/src/test/java/net/neoforged/neoform/runtime/engine/NeoFormEngineTest.java new file mode 100644 index 0000000..7da0d1d --- /dev/null +++ b/src/test/java/net/neoforged/neoform/runtime/engine/NeoFormEngineTest.java @@ -0,0 +1,150 @@ +package net.neoforged.neoform.runtime.engine; + +import net.neoforged.neoform.runtime.artifacts.ArtifactManager; +import net.neoforged.neoform.runtime.cache.CacheKey; +import net.neoforged.neoform.runtime.cache.CacheManager; +import net.neoforged.neoform.runtime.cli.FileHashService; +import net.neoforged.neoform.runtime.cli.LockManager; +import net.neoforged.neoform.runtime.graph.ExecutionNode; +import net.neoforged.neoform.runtime.graph.ExecutionNodeAction; +import net.neoforged.neoform.runtime.graph.NodeOutputType; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.HashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +class NeoFormEngineTest { + private static final String NODE_ID = "cacheUseNode"; + + @TempDir + Path tempDir; + + @Test + void engineKeepsCacheEntryAndBlocksMaintenance() throws Exception { + // Set up temp-backed managers so this test covers the on-disk cache entry + // and the file locks used between NFRT processes. + var homeDir = tempDir.resolve("cache"); + var cacheManager = new MaintenanceDuringSaveCacheManager( + homeDir, + tempDir.resolve("assets"), + tempDir.resolve("workspaces") + ); + var lockManager = new LockManager(tempDir.resolve("locks")); + + Path cachedOutput; + Path buildOutput; + try (var engine = newEngine(cacheManager, lockManager)) { + var node = newNode(engine); + engine.getGraph().setResult("testResult", node.getRequiredOutput("output")); + buildOutput = tempDir.resolve("project-build").resolve("output.txt"); + Files.createDirectories(buildOutput.getParent()); + + // Write results through the engine. + // The special test cache manager will attempt maintenance after the cache entry is + // saved but before this call copies the cache path into the build directory. + engine.writeResults(Map.of("testResult", buildOutput)); + cachedOutput = node.getRequiredOutput("output").getResultPath(); + + // Verify maintenance tried to run during the engine operation and + // did not delete the cache-owned path before writeResults copied it. + assertThat(cacheManager.maintenanceAttempted()).isTrue(); + assertThat(cacheManager.cacheOutputSurvivedMaintenance()).isTrue(); + assertThat(cachedOutput).hasContent("cached output"); + assertThat(buildOutput).hasContent("cached output"); + + // Run maintenance again after writeResults releases the use lock, + // but before the engine closes. + cacheManager.performMaintenance(); + + // Verify the cache entry is allowed to disappear after writeResults + // is done, while the copied project output remains intact. + assertThat(cachedOutput).doesNotExist(); + assertThat(buildOutput).hasContent("cached output"); + } + } + + private static NeoFormEngine newEngine(CacheManager cacheManager, LockManager lockManager) { + return new NeoFormEngine( + mock(ArtifactManager.class), + new FileHashService(), + cacheManager, + lockManager + ); + } + + private static ExecutionNode newNode(NeoFormEngine engine) { + var builder = engine.getGraph().nodeBuilder(NODE_ID); + builder.output("output", NodeOutputType.TXT, "Test output"); + builder.action(new CacheableTestAction()); + return builder.build(); + } + + private static void makePeriodicMaintenanceDue(Path homeDir) throws IOException { + var maintenanceState = homeDir.resolve("nfrt_cache_cleanup.state"); + if (Files.notExists(maintenanceState)) { + Files.createFile(maintenanceState); + } + Files.setLastModifiedTime( + maintenanceState, + FileTime.from(Instant.now().minus(2, ChronoUnit.DAYS)) + ); + } + + private static final class CacheableTestAction implements ExecutionNodeAction { + @Override + public void run(ProcessingEnvironment environment) throws IOException { + Files.writeString(environment.getOutputPath("output"), "cached output"); + } + } + + private static final class MaintenanceDuringSaveCacheManager extends CacheManager { + private final Path homeDir; + private boolean maintenanceAttempted; + private boolean cacheOutputSurvivedMaintenance; + + private MaintenanceDuringSaveCacheManager(Path homeDir, + Path assetsDir, + Path workspacesDir) throws IOException { + super(homeDir, assetsDir, workspacesDir); + this.homeDir = homeDir; + } + + @Override + public void saveOutputs(ExecutionNode node, + CacheKey cacheKey, + HashMap outputValues) throws IOException { + super.saveOutputs(node, cacheKey, outputValues); + var cachedOutput = outputValues.get("output"); + + // Simulate a race condition: Make the just-published entry eligible for cleanup, + // then run maintenance before writeResults can copy the returned cache path. + var marker = homeDir.resolve("intermediate_results").resolve(cacheKey + ".txt"); + Files.setLastModifiedTime( + marker, + FileTime.from(Instant.now().minus(32, ChronoUnit.DAYS)) + ); + makePeriodicMaintenanceDue(homeDir); + maintenanceAttempted = true; + performMaintenance(); + cacheOutputSurvivedMaintenance = Files.isRegularFile(cachedOutput); + } + + private boolean maintenanceAttempted() { + return maintenanceAttempted; + } + + private boolean cacheOutputSurvivedMaintenance() { + return cacheOutputSurvivedMaintenance; + } + } +}