[2.x] fix: Don't cache environmental (position-less) compile failures (#9464)

Failure caching assumes a CompileFailed is a function of
the sources, which is true for source errors. But zinc also surfaces I/O write
failures ("error writing X.class") as compiler problems, so an environmental
failure (a concurrent target/ deletion, a permission blip) was cached under the
same mechanism and replayed from the global action cache on every later build,
even after the cause was gone. When the poisoned task is the metabuild compile
this is self-sustaining and unrecoverable from inside sbt: project loading
fails, so no task -- including clean -- can run, and only deleting the global
cache by hand recovers. The replayed diagnostics also name files/permissions
that no longer exist.

Refs #9455

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

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

sbt-app/src/sbt-test/cache/compile-io-failure/Main.scala

@@ -0,0 +1,2 @@
+object Main:
+  def main(args: Array[String]): Unit = ()

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")
+}

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

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 =

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(

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)