diff --git a/notes/2.0.0/cache-environmental-compile-failure.md b/notes/2.0.0/cache-environmental-compile-failure.md new file mode 100644 index 000000000..6260e5073 --- /dev/null +++ b/notes/2.0.0/cache-environmental-compile-failure.md @@ -0,0 +1,18 @@ +### Environmental compile failures are no longer cached + +sbt caches a `CompileFailed` so that an unchanged, still-broken compile does not +re-run from scratch (#7662). But zinc also reports I/O write failures ("error +writing X.class") as compiler problems, so a one-off environmental failure (a +concurrent `target/` deletion, a permission blip) was cached under the same +mechanism and replayed on every later build, even after the cause was gone. When +it hit the metabuild compile the build stayed wedged across restarts, recoverable +only by deleting the global cache by hand. Compile failures whose errors carry no +source position are now treated as environmental and left uncached, so the next +build retries for real. A position-less failure already sitting in a cache +written by an earlier sbt is likewise no longer replayed: the task re-runs and a +success overwrites the stale entry, so previously wedged builds recover on +upgrade without deleting the cache. + +This addresses [#9455][i9455]. + +[i9455]: https://github.com/sbt/sbt/issues/9455 diff --git a/project/Scripted.scala b/project/Scripted.scala index ccb61129a..462a53778 100644 --- a/project/Scripted.scala +++ b/project/Scripted.scala @@ -40,9 +40,11 @@ object Scripted { case ("classloader-cache", "jni") => true // no native lib is built for windows case ("classloader-cache", "spark") => true // the test spark server is unable to bind to a local socket on Visual Studio 2019 - case ("nio", "make-clone") => true // uses gcc which isn't set up on all systems - case ("watch", "symlinks") => true // symlinks don't work the same on windows - case _ => false + case ("nio", "make-clone") => true // uses gcc which isn't set up on all systems + case ("watch", "symlinks") => true // symlinks don't work the same on windows + case ("cache", "compile-io-failure") => + true // a read-only dir doesn't block writes on windows, so the I/O failure won't reproduce + case _ => false } ) else NothingFilter diff --git a/sbt-app/src/sbt-test/cache/compile-io-failure/Main.scala b/sbt-app/src/sbt-test/cache/compile-io-failure/Main.scala new file mode 100644 index 000000000..f2a3fb3f0 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-io-failure/Main.scala @@ -0,0 +1,2 @@ +object Main: + def main(args: Array[String]): Unit = () diff --git a/sbt-app/src/sbt-test/cache/compile-io-failure/build.sbt b/sbt-app/src/sbt-test/cache/compile-io-failure/build.sbt new file mode 100644 index 000000000..836e6a8d9 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-io-failure/build.sbt @@ -0,0 +1,15 @@ +Global / localCacheDirectory := baseDirectory.value / "diskcache" +scalaVersion := "3.8.4" + +val denyClassDir = taskKey[Unit]("Remove write permission from the compile output dir") +denyClassDir := { + val dir = (Compile / classDirectory).value + IO.createDirectory(dir) + assert(dir.setWritable(false, false), s"could not clear write permission on $dir") +} + +val allowClassDir = taskKey[Unit]("Restore write permission on the compile output dir") +allowClassDir := { + val dir = (Compile / classDirectory).value + assert(dir.setWritable(true, false), s"could not restore write permission on $dir") +} diff --git a/sbt-app/src/sbt-test/cache/compile-io-failure/test b/sbt-app/src/sbt-test/cache/compile-io-failure/test new file mode 100644 index 000000000..2d486eb2c --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-io-failure/test @@ -0,0 +1,8 @@ +# sbt/sbt#9455: an environmental I/O compile failure must not be cached and replayed forever. +# Make the compile output dir read-only so compile fails with an "error writing" I/O error. +> denyClassDir +-> compile +# Heal the environment. The sources are unchanged, so without the fix the cached environmental +# failure is replayed and this compile fails; with the fix the compile actually runs and succeeds. +> allowClassDir +> compile diff --git a/util-cache/src/main/scala/sbt/util/ActionCache.scala b/util-cache/src/main/scala/sbt/util/ActionCache.scala index 376ed87d9..79f9c3cd2 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCache.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCache.scala @@ -99,7 +99,10 @@ object ActionCache: try action(key): @unchecked catch case e: CompileFailed => - cacheFailure(e) + if CachedCompileFailure.hasSufficientInfo(e) then cacheFailure(e) + else + cacheEventLog.append(ActionCacheEvent.OnsiteTask) + throw e case e: Exception => cacheEventLog.append(ActionCacheEvent.Error) logExec( @@ -171,7 +174,7 @@ object ActionCache: ) ) value - case Left(Some(failure)) => + case Left(Some(failure)) if CachedCompileFailure.hasSufficientInfo(failure.toException) => config.cacheEventLog.append(ActionCacheEvent.Found("cached-failure")) // Replay problems to the logger so users see the cached errors/warnings failure.replay(config.logger) @@ -179,7 +182,7 @@ object ActionCache: SpawnExec(input = spawnInput, cacheHit = true, exitCode = 1) ) throw failure.toException - case Left(None) => organicTask + case Left(_) => organicTask end cache /** @@ -297,7 +300,7 @@ object ActionCache: ) config.store.get(getRequest) - private inline def mkInput[I: HashWriter]( + private[sbt] inline def mkInput[I: HashWriter]( key: I, codeContentHash: Digest, extraHash: Digest, @@ -331,7 +334,7 @@ object ActionCache: case e: NoSuchFileException => Option(e.getFile) case _ => findMissingFile(t.getCause) - private inline def mkValuePath(inputDigest: Digest): String = + private[sbt] inline def mkValuePath(inputDigest: Digest): String = s"$${OUT}/value/${inputDigest}.json" def manifestFromFile(manifest: Path): Manifest = diff --git a/util-cache/src/main/scala/sbt/util/CachedCompileFailure.scala b/util-cache/src/main/scala/sbt/util/CachedCompileFailure.scala index fc55bd681..83fcfa4e6 100644 --- a/util-cache/src/main/scala/sbt/util/CachedCompileFailure.scala +++ b/util-cache/src/main/scala/sbt/util/CachedCompileFailure.scala @@ -59,16 +59,16 @@ object CachedCompileFailure s"$file$line$pointer: ${problem.message}$lineContent$pointerLine" /** - * Check if the problems contain enough information to be useful when replayed. - * For Scala 2.13, the `rendered` field is empty, so we check if position info exists. + * Whether a compile failure carries enough source information to be safely cached and replayed. + * It is a function of the sources only when every error has a source position; a position-less + * error such as "error writing X.class" is an environmental I/O fault, and caching it replays a + * stale failure on every later build until the global cache is cleared by hand (sbt/sbt#9455). + * Such failures are treated as not cacheable. */ def hasSufficientInfo(e: CompileFailed): Boolean = import sbt.util.InterfaceUtil.toOption - e.problems() - .forall: problem => - // Either has rendered text (Scala 3) or has position info (Scala 2.13) - toOption(problem.rendered).isDefined || - (toOption(problem.position.sourcePath).isDefined && problem.message.nonEmpty) + val errors = e.problems().filter(_.severity == Severity.Error) + errors.nonEmpty && errors.forall(p => toOption(p.position.sourcePath).isDefined) def fromException(e: CompileFailed): CachedCompileFailure = CachedCompileFailure( diff --git a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala index 5978aeee5..03c858829 100644 --- a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala +++ b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala @@ -222,6 +222,108 @@ object ActionCacheTest extends BasicTestSuite: assert(caught2.problems()(0).message() == "Test error message") assert(caught2.getMessage() == "Compilation failed") + test("Disk cache does not cache an environmental (position-less) CompileFailed"): + withDiskCache(testEnvironmentalCompileFailureNotCached) + + def testEnvironmentalCompileFailureNotCached(cache: DiskActionCacheStore): Unit = + import sjsonnew.BasicJsonProtocol.* + var called = 0 + // An I/O write failure surfaced by zinc as a compiler problem: an error with no source position. + val ioProblem = new Problem: + override def category(): String = "Test" + override def severity(): Severity = Severity.Error + override def message(): String = "error writing Meta$.class: AccessDeniedException" + override def position(): Position = new Position: + override def line(): Optional[Integer] = Optional.empty() + override def lineContent(): String = "" + override def offset(): Optional[Integer] = Optional.empty() + override def pointer(): Optional[Integer] = Optional.empty() + override def pointerSpace(): Optional[String] = Optional.empty() + override def sourcePath(): Optional[String] = Optional.empty() + override def sourceFile(): Optional[java.io.File] = Optional.empty() + + val ioException = new CompileFailed: + override def arguments(): Array[String] = Array.empty + override def problems(): Array[Problem] = Array(ioProblem) + override def getMessage(): String = "Compilation failed" + + val action: ((Int, Int)) => InternalActionResult[Int] = { (_, _) => + called += 1 + throw ioException + } + IO.withTemporaryDirectory: tempDir => + val config = getCacheConfig(cache, tempDir) + + try + ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(false, "Expected CompileFailed to be thrown") + catch case _: CompileFailed => () + assert(called == 1) + + // The environment may have healed, so the second call must re-run the action rather than + // replay a stale cached failure. + try + ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(false, "Expected CompileFailed to be thrown") + catch case _: CompileFailed => () + assert( + called == 2, + s"environmental failure must not be cached; action should re-run (called=$called)" + ) + + test("A position-less failure cached by an older sbt is not replayed and is cured by a re-run"): + withDiskCache(testLegacyPoisonedFailureCured) + + def testLegacyPoisonedFailureCured(cache: DiskActionCacheStore): Unit = + import sjsonnew.BasicJsonProtocol.* + import sjsonnew.support.scalajson.unsafe.{ CompactPrinter, Converter } + var called = 0 + val ioProblem = new Problem: + override def category(): String = "Test" + override def severity(): Severity = Severity.Error + override def message(): String = "error writing Meta$.class: AccessDeniedException" + override def position(): Position = new Position: + override def line(): Optional[Integer] = Optional.empty() + override def lineContent(): String = "" + override def offset(): Optional[Integer] = Optional.empty() + override def pointer(): Optional[Integer] = Optional.empty() + override def pointerSpace(): Optional[String] = Optional.empty() + override def sourcePath(): Optional[String] = Optional.empty() + override def sourceFile(): Optional[java.io.File] = Optional.empty() + val ioException = new CompileFailed: + override def arguments(): Array[String] = Array.empty + override def problems(): Array[Problem] = Array(ioProblem) + override def getMessage(): String = "Compilation failed" + + val action: ((Int, Int)) => InternalActionResult[Int] = { (a, b) => + called += 1 + InternalActionResult(a + b, Nil) + } + IO.withTemporaryDirectory: tempDir => + val config = getCacheConfig(cache, tempDir) + + // Write the poisoned entry exactly the way sbt cached failures before the gate existed. + val digest = ActionCache.mkInput((1, 1), Digest.zero, Digest.zero, config.cacheVersion) + val json = Converter.toJsonUnsafe(CachedCompileFailure.fromException(ioException)) + val failureFile = StringVirtualFile1(ActionCache.mkValuePath(digest), CompactPrinter(json)) + cache.put( + UpdateActionResultRequest( + digest, + Vector(failureFile), + exitCode = ActionCache.failureExitCode + ) + ) + + // The poison must not replay: the action runs and succeeds. + val v1 = ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(v1 == 2) + assert(called == 1, s"poisoned failure must not replay; action should run (called=$called)") + + // The success overwrites the poison: the next call is a cache hit. + val v2 = ActionCache.cache((1, 1), Digest.zero, Digest.zero, tags, config)(action) + assert(v2 == 2) + assert(called == 1, s"expected a success cache hit after the cure (called=$called)") + test("Cache falls back to recompute when syncBlobs throws FileNotFoundException"): withDiskCache(testSyncBlobsThrowsFallback)