From 4b3fc911a70f92529221415364c3438db6c94b4d Mon Sep 17 00:00:00 2001 From: Stas Shevchenko Date: Thu, 23 Jul 2026 01:28:59 +0200 Subject: [PATCH] [2.x] fix: Fixes partial cache restoration (#9488) A genuinely broken restore now degrades to the onsite task instead of a silent cache hit with incomplete outputs. Related to #9349. Co-authored-by: sshevchenko --- .../src/main/scala/sbt/util/ActionCache.scala | 4 +- .../scala/sbt/util/ActionCacheStore.scala | 19 ++-- .../test/scala/sbt/util/ActionCacheTest.scala | 104 +++++++++++++++++- 3 files changed, 116 insertions(+), 11 deletions(-) diff --git a/util-cache/src/main/scala/sbt/util/ActionCache.scala b/util-cache/src/main/scala/sbt/util/ActionCache.scala index 79f9c3cd2..143de2904 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCache.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCache.scala @@ -8,7 +8,7 @@ package sbt.util -import java.io.{ File, IOException, PrintWriter } +import java.io.{ File, PrintWriter } import java.nio.charset.StandardCharsets import java.nio.file.{ AtomicMoveNotSupportedException, @@ -147,7 +147,7 @@ object ActionCache: result case Left(e) => throw e catch - case e: IOException => + case NonFatal(e) => logger.debug(s"Skipping cache storage due to error: ${e.getMessage}") cacheEventLog.append(ActionCacheEvent.Error) result diff --git a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala index 4eaf41961..79b1e3f62 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala @@ -284,14 +284,17 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) override def syncBlobs(refs: Seq[HashedVirtualFileRef], outputDirectory: Path): Seq[Path] = refs.flatMap: r => - try - val casFile = toCasFile(Digest(r)) - if isCompleteBlob(casFile, Digest(r)) then - // println(s"syncBlobs: $casFile exists for $r") - Some(syncFile(r, casFile, outputDirectory)) - else None - // Digest(r) can throw NoSuchFileException - catch case _: NoSuchFileException => None + // Only the blob-availability lookup may swallow NoSuchFileException (Digest(r) can throw it): + // an absent blob is a cache miss for that entry. A write failure from syncFile, however, must + // propagate so the caller can degrade to the onsite task (sbt/sbt#8890) instead of silently + // leaving the output tree incomplete (sbt/sbt#9349). + val casFileOpt = + try + val digest = Digest(r) + val casFile = toCasFile(digest) + if isCompleteBlob(casFile, digest) then Some(casFile) else None + catch case _: NoSuchFileException => None + casFileOpt.map(syncFile(r, _, outputDirectory)) def syncFile(ref: HashedVirtualFileRef, casFile: Path, outputDirectory: Path): Path = val d = Digest(ref) diff --git a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala index 03c858829..eccc0e487 100644 --- a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala +++ b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala @@ -444,6 +444,90 @@ object ActionCacheTest extends BasicTestSuite: assert(Files.size(zipPath) > 0L) finally pool.shutdown() + // See https://github.com/sbt/sbt/issues/9349. With a warm cache, deleting the output tree + // out-of-band must not break the next build: restore recreates the missing parent directories. + test("Restore recreates a deleted output directory (direct blob)"): + IO.withTemporaryDirectory: cacheDir => + IO.withTemporaryDirectory: outDir => + val conv = fileConverter + val cache = DiskActionCacheStore(cacheDir.toPath, conv) + val nested = outDir.toPath.resolve("classes/pkg/A.txt") + val blob = StringVirtualFile1(nested.toString, "compiled") + val refs = cache.putBlobs(Seq(blob)) + cache.syncBlobs(refs, outDir.toPath) + assert(Files.exists(nested), "first sync should create the file") + IO.delete(outDir.toPath.resolve("classes").toFile()) + assert(!Files.exists(nested.getParent)) + cache.syncBlobs(refs, outDir.toPath) + assert(Files.exists(nested), "restore must recreate the deleted parent + file") + + test("Restore recreates a deleted output directory on a cache hit (dirzip)"): + import sjsonnew.BasicJsonProtocol.* + IO.withTemporaryDirectory: cacheDir => + IO.withTemporaryDirectory: outDir => + val conv = binaryConverter + val cache = DiskActionCacheStore(cacheDir.toPath, conv) + val classesDir = outDir.toPath.resolve("classes") + val classFile = classesDir.resolve("pkg/A.class") + var called = 0 + val action: Unit => InternalActionResult[Int] = { _ => + called += 1 + Files.createDirectories(classFile.getParent) + Files.writeString(classFile, "compiled") + val dirzip = + ActionCache.packageDirectory( + VirtualFileRef.of(classesDir.toString), + conv, + outDir.toPath + ) + InternalActionResult(1, Seq(dirzip)) + } + val config = getCacheConfig(cache, outDir, converter = conv) + val v1 = ActionCache.cache((), Digest.zero, Digest.zero, tags, config)(action) + assert(v1 == 1) + assert(called == 1) + assert(Files.exists(classFile)) + IO.delete(outDir.toPath.toFile()) + assert(!Files.exists(classesDir)) + val v2 = ActionCache.cache((), Digest.zero, Digest.zero, tags, config)(action) + assert(v2 == 1) + assert( + Files.exists(classFile), + s"class file must be restored after output dir deletion (called=$called)" + ) + + // A genuinely broken restore (syncFile write fails) must degrade to the onsite task rather than + // being silently swallowed by syncBlobs, which would report a cache hit with incomplete outputs. + test("Restore degrades to onsite recompute when materialising throws NoSuchFileException"): + testBrokenRestoreDegrades(r => new NoSuchFileException(r.toString)) + + test("Restore degrades to onsite recompute when materialising throws a non-IOException"): + testBrokenRestoreDegrades(_ => new RuntimeException("disk exploded")) + + def testBrokenRestoreDegrades(mkError: HashedVirtualFileRef => Throwable): Unit = + import sjsonnew.BasicJsonProtocol.* + IO.withTemporaryDirectory: cacheDir => + IO.withTemporaryDirectory: outDir => + val broken = new DiskActionCacheStore(cacheDir.toPath, fileConverter): + override def syncFile( + ref: HashedVirtualFileRef, + casFile: Path, + outputDirectory: Path, + ): Path = throw mkError(ref) + var called = 0 + val action: ((Int, Int)) => InternalActionResult[Int] = { (a, b) => + called += 1 + val out = StringVirtualFile1(s"$outDir/a.txt", (a + b).toString) + InternalActionResult(a + b, Seq(out)) + } + val config = getCacheConfig(broken, outDir) + val v1 = ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(v1 == 2) + assert(called == 1) + val v2 = ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(v2 == 2) + assert(called == 2, s"broken restore must degrade to onsite recompute (called=$called)") + test("Changing cacheVersion invalidates the cache"): withDiskCache(testCacheVersionInvalidation) @@ -492,6 +576,7 @@ object ActionCacheTest extends BasicTestSuite: cache: ActionCacheStore, outputDir: File, cacheVersion: Long = 0L, + converter: FileConverter = fileConverter, ): BuildWideCacheConfiguration = val logger = new Logger: override def trace(t: => Throwable): Unit = () @@ -500,7 +585,7 @@ object ActionCacheTest extends BasicTestSuite: BuildWideCacheConfiguration( cache, outputDir.toPath(), - fileConverter, + converter, logger, CacheEventLog(), CacheImplicits.defaultLocalDigestCacheByteSize, @@ -512,4 +597,21 @@ object ActionCacheTest extends BasicTestSuite: override def toVirtualFile(path: Path): VirtualFile = val content = if Files.isRegularFile(path) then new String(Files.readAllBytes(path)) else "" StringVirtualFile1(path.toString, content) + + // A converter whose VirtualFiles read raw bytes off disk, so binary blobs (e.g. dirzips) survive + // the put/sync round-trip instead of being mangled by a String round-trip. + def binaryConverter = new FileConverter: + override def toPath(ref: VirtualFileRef): Path = Paths.get(ref.id) + override def toVirtualFile(path: Path): VirtualFile = DiskVirtualFile(path.toString) + + final class DiskVirtualFile(path: String) + extends xsbti.BasicVirtualFileRef(path) + with VirtualFile: + private def bytes: Array[Byte] = + if Files.isRegularFile(Paths.get(path)) then Files.readAllBytes(Paths.get(path)) + else Array.emptyByteArray + override def contentHash: Long = HashUtil.xxhash64(bytes) + override def sizeBytes: Long = bytes.length.toLong + override def contentHashStr: String = Digest.sha256Hash(bytes).contentHashStr + override def input: InputStream = new java.io.ByteArrayInputStream(bytes) end ActionCacheTest