Improve managed file management in watch

@olegych reported in https://github.com/sbt/sbt/issues/4722 that
sometimes even when a build was triggered during watch that no
recompilation would occur. The cause of this was that we never
invalidated the file stamp cache for managed sources or output files.
The optimization of persisting the source file stamps between task
evaluations in a continuous build only really makes sense for unmanaged
sources. We make the implicit assumption that unmanaged sources are
infrequently updated and generally one at a time. That assumption does
not hold for unmanaged source or output files.

To fix this, I split the fileStampCache into two caches: one for
unmanaged sources and one for everything else. We only persist the
unmanagedFileStampCache during continuous builds. The
managedFileStampCache gets invalidated every time.

I added a scripted test that simulates changing a generated source file.
Prior to this change, the test would fail because the file stamp was not
invalidated for the new source file content.

Fixes #4722

main/src/main/scala/sbt/Defaults.scala

@@ -148,7 +148,6 @@ object Defaults extends BuildCommon {
   private[sbt] lazy val globalCore: Seq[Setting[_]] = globalDefaults(
     defaultTestTasks(test) ++ defaultTestTasks(testOnly) ++ defaultTestTasks(testQuick) ++ Seq(
       excludeFilter :== HiddenFileFilter,
-      pathToFileStamp :== sbt.nio.FileStamp.hash,
       fileInputs :== Nil,
       inputFileStamper :== sbt.nio.FileStamper.Hash,
       outputFileStamper :== sbt.nio.FileStamper.LastModified,
@@ -157,11 +156,9 @@ object Defaults extends BuildCommon {
       watchPersistFileStamps :== true,
       watchTriggers :== Nil,
       clean := { () },
-      sbt.nio.Keys.fileStampCache := {
+      unmanagedFileStampCache :=
-        state.value
+        state.value.get(persistentFileStampCache).getOrElse(new sbt.nio.FileStamp.Cache),
-          .get(sbt.nio.Keys.persistentFileStampCache)
+      managedFileStampCache := new sbt.nio.FileStamp.Cache,
-          .getOrElse(new sbt.nio.FileStamp.Cache)
-      },
     ) ++ globalIvyCore ++ globalJvmCore
   ) ++ globalSbtCore
 
@@ -420,9 +417,12 @@ object Defaults extends BuildCommon {
     unmanagedSources := (unmanagedSources / inputFileStamps).value.map(_._1.toFile),
     managedSourceDirectories := Seq(sourceManaged.value),
     managedSources := {
-      val stamper = sbt.nio.Keys.pathToFileStamp.value
+      val stamper = inputFileStamper.value
+      val cache = managedFileStampCache.value
       val res = generate(sourceGenerators).value
-      res.foreach(f => stamper(f.toPath))
+      res.foreach { f =>
+        cache.putIfAbsent(f.toPath, stamper)
+      }
       res
     },
     sourceGenerators :== Nil,
@@ -1647,7 +1647,7 @@ object Defaults extends BuildCommon {
       val contents = AnalysisContents.create(analysisResult.analysis(), analysisResult.setup())
       store.set(contents)
     }
-    val map = sbt.nio.Keys.fileStampCache.value
+    val map = managedFileStampCache.value
     val analysis = analysisResult.analysis
     import scala.collection.JavaConverters._
     analysis.readStamps.getAllProductStamps.asScala.foreach {

main/src/main/scala/sbt/internal/ExternalHooks.scala

@@ -28,21 +28,23 @@ import scala.collection.mutable
 private[sbt] object ExternalHooks {
   private val javaHome = Option(System.getProperty("java.home")).map(Paths.get(_))
   def default: Def.Initialize[sbt.Task[ExternalHooks]] = Def.task {
-    val cache = fileStampCache.value
+    val unmanagedCache = unmanagedFileStampCache.value
+    val managedCache = managedFileStampCache.value
     val cp = dependencyClasspath.value.map(_.data)
     cp.foreach { file =>
       val path = file.toPath
-      cache.getOrElseUpdate(path, FileStamper.LastModified)
+      managedCache.getOrElseUpdate(path, FileStamper.LastModified)
     }
     val classGlob = classDirectory.value.toGlob / RecursiveGlob / "*.class"
     fileTreeView.value.list(classGlob).foreach {
-      case (path, _) => cache.update(path, FileStamper.LastModified)
+      case (path, _) => managedCache.update(path, FileStamper.LastModified)
     }
-    apply((compileOptions in compile).value, cache)
+    apply((compileOptions in compile).value, unmanagedCache, managedCache)
   }
   private def apply(
       options: CompileOptions,
-      fileStampCache: FileStamp.Cache
+      unmanagedCache: FileStamp.Cache,
+      managedCache: FileStamp.Cache
   ): DefaultExternalHooks = {
     val lookup = new ExternalLookup {
       override def changedSources(previousAnalysis: CompileAnalysis): Option[Changes[File]] = Some {
@@ -57,7 +59,10 @@ private[sbt] object ExternalHooks {
             previousAnalysis.readStamps().getAllSourceStamps.asScala
           prevSources.foreach {
             case (file: File, s: Stamp) =>
-              fileStampCache.getOrElseUpdate(file.toPath, FileStamper.Hash) match {
+              val path = file.toPath
+              unmanagedCache
+                .get(path)
+                .orElse(managedCache.getOrElseUpdate(file.toPath, FileStamper.Hash)) match {
                 case None => getRemoved.add(file)
                 case Some(stamp) =>
                   if (equiv(stamp.stamp, s)) getUnmodified.add(file) else getChanged.add(file)
@@ -79,7 +84,7 @@ private[sbt] object ExternalHooks {
       override def changedBinaries(previousAnalysis: CompileAnalysis): Option[Set[File]] = {
         Some(previousAnalysis.readStamps.getAllBinaryStamps.asScala.flatMap {
           case (file, stamp) =>
-            fileStampCache.get(file.toPath) match {
+            managedCache.get(file.toPath) match {
               case Some(cachedStamp) if equiv(cachedStamp.stamp, stamp) => None
               case _ =>
                 javaHome match {
@@ -94,7 +99,7 @@ private[sbt] object ExternalHooks {
       override def removedProducts(previousAnalysis: CompileAnalysis): Option[Set[File]] = {
         Some(previousAnalysis.readStamps.getAllProductStamps.asScala.flatMap {
           case (file, stamp) =>
-            fileStampCache.get(file.toPath) match {
+            managedCache.get(file.toPath) match {
               case Some(s) if equiv(s.stamp, stamp) => None
               case _                                => Some(file)
             }

main/src/main/scala/sbt/nio/FileStamp.scala

@@ -9,6 +9,7 @@ package sbt.nio
 
 import java.io.{ File, IOException }
 import java.nio.file.{ Path, Paths }
+import java.util.concurrent.ConcurrentHashMap
 
 import sbt.internal.inc.{ EmptyStamp, Stamper, LastModified => IncLastModified }
 import sbt.io.IO
@@ -217,7 +218,7 @@ private[sbt] object FileStamp {
   }
 
   private[sbt] class Cache {
-    private[this] val underlying = new java.util.HashMap[Path, Either[FileStamp, FileStamp]]
+    private[this] val underlying = new ConcurrentHashMap[Path, Either[FileStamp, FileStamp]]
 
     /**
      * Invalidate the cache entry, but don't re-stamp the file until it's actually used
@@ -251,6 +252,14 @@ private[sbt] object FileStamp {
         case null => None
         case e    => e.value
       }
+
+    def putIfAbsent(key: Path, stamper: FileStamper): Unit = {
+      underlying.get(key) match {
+        case null => updateImpl(key, stamper)
+        case _    =>
+      }
+      ()
+    }
     def update(key: Path, stamper: FileStamper): (Option[FileStamp], Option[FileStamp]) = {
       underlying.get(key) match {
         case null => (None, updateImpl(key, stamper))

main/src/main/scala/sbt/nio/Keys.scala

@@ -142,11 +142,11 @@ object Keys {
   private[sbt] val allInputPathsAndAttributes =
     taskKey[Seq[(Path, FileAttributes)]]("Get all of the file inputs for a task")
       .withRank(Invisible)
-  private[sbt] val fileStampCache = taskKey[FileStamp.Cache](
+  private[sbt] val unmanagedFileStampCache = taskKey[FileStamp.Cache](
-    "Map of file stamps that may be cleared between task evaluation runs."
+    "Map of managed file stamps that may be cleared between task evaluation runs."
   ).withRank(Invisible)
-  private[sbt] val pathToFileStamp = taskKey[Path => Option[FileStamp]](
+  private[sbt] val managedFileStampCache = taskKey[FileStamp.Cache](
-    "A function that computes a file stamp for a path. It may have the side effect of updating a cache."
+    "Map of managed file stamps that may be cleared between task evaluation runs."
   ).withRank(Invisible)
   private[sbt] val classpathFiles =
     taskKey[Seq[Path]]("The classpath for a task.").withRank(Invisible)

main/src/main/scala/sbt/nio/Settings.scala

@@ -144,8 +144,7 @@ private[sbt] object Settings {
         (transitiveClasspathDependency in scopedKey.scope := { () }) :: Nil
       case changedOutputFiles.key =>
         changedFilesImpl(scopedKey, changedOutputFiles, outputFileStamps)
-      case pathToFileStamp.key => stamper(scopedKey) :: Nil
+      case _ => Nil
-      case _                   => Nil
     }
 
   /**
@@ -318,10 +317,12 @@ private[sbt] object Settings {
    */
   private[sbt] def fileStamps(scopedKey: Def.ScopedKey[_]): Def.Setting[_] =
     addTaskDefinition(Keys.inputFileStamps in scopedKey.scope := {
-      val stamper = (Keys.pathToFileStamp in scopedKey.scope).value
+      val cache = (unmanagedFileStampCache in scopedKey.scope).value
+      val stamper = (Keys.inputFileStamper in scopedKey.scope).value
       (Keys.allInputPathsAndAttributes in scopedKey.scope).value.flatMap {
-        case (p, a) if a.isRegularFile && !Files.isHidden(p) => stamper(p).map(p -> _)
+        case (p, a) if a.isRegularFile && !Files.isHidden(p) =>
-        case _                                               => None
+          cache.getOrElseUpdate(p, stamper).map(p -> _)
+        case _ => None
       }
     })
   private[this] def outputsAndStamps[T: JsonFormat: ToSeqPath](
@@ -348,17 +349,4 @@ private[sbt] object Settings {
       (allOutputFiles in scope).value.flatMap(p => stamper(p).map(p -> _))
     })
 
-  /**
-   * Returns a function from `Path` to [[FileStamp]] that can be used by tasks to retrieve
-   * the stamp for a file. It has the side effect of stamping the file if it has not already
-   * been stamped during the task evaluation.
-   *
-   * @return a task definition for a function from `Path` to [[FileStamp]].
-   */
-  private[this] def stamper(scopedKey: Def.ScopedKey[_]): Def.Setting[_] =
-    addTaskDefinition((Keys.pathToFileStamp in scopedKey.scope) := {
-      val attributeMap = Keys.fileStampCache.value
-      val stamper = (Keys.inputFileStamper in scopedKey.scope).value
-      path: Path => attributeMap.getOrElseUpdate(path, stamper)
-    })
 }

sbt/src/sbt-test/watch/managed/build.sbt

@@ -0,0 +1,23 @@
+import java.nio.file.Files
+
+import sbt.nio.Watch
+
+import scala.concurrent.duration._
+
+Compile / sourceGenerators += Def.task {
+  baseDirectory.value / "sources" / "Write.scala" :: Nil
+}.taskValue
+
+val runTest = taskKey[Unit]("run the test")
+runTest := Def.taskDyn {
+  val args = s" ${baseDirectory.value}"
+  (Runtime / run).toTask(args)
+}.value
+
+runTest / watchTriggers += baseDirectory.value.toGlob / "*.txt"
+watchAntiEntropy := 0.milliseconds
+watchOnFileInputEvent := { (count, e) =>
+  if (new String(Files.readAllBytes(e.path)) == "ok") Watch.CancelWatch
+  else if (count < 2) Watch.Trigger
+  else new Watch.HandleError(new IllegalStateException(s"Wrong event triggered the build: $e"))
+}

sbt/src/sbt-test/watch/managed/changes/Write.scala

@@ -0,0 +1,7 @@
+import java.nio.file._
+
+object Write {
+  def main(args: Array[String]): Unit = {
+    Files.write(Paths.get(args(0)).resolve("output.txt"), "ok".getBytes)
+  }
+}

sbt/src/sbt-test/watch/managed/sources/Write.scala

@@ -0,0 +1,12 @@
+import java.nio.file._
+object Write {
+  def main(args: Array[String]): Unit = {
+    val dir = Paths.get(args(0))
+    Files.write(
+      dir.resolve("sources").resolve("Write.scala"),
+      Files.readAllBytes(dir.resolve("changes").resolve("Write.scala")),
+    )
+    val output = dir.resolve("output.txt")
+    Files.write(output, "failure".getBytes)
+  }
+}

sbt/src/sbt-test/watch/managed/test

@@ -0,0 +1 @@
+> ~runTest

scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala

@@ -232,6 +232,7 @@ final class ScriptedTests(
       case "source-dependencies/linearization"           => LauncherBased // sbt/Package$
       case "source-dependencies/named"                   => LauncherBased // sbt/Package$
       case "source-dependencies/specialized"             => LauncherBased // sbt/Package$
+      case "watch/managed"                               => LauncherBased // sbt/Package$
       case "tests/test-cross" =>
         LauncherBased // the sbt metabuild classpath leaks into the test interface classloader in older versions of sbt
       case _ => RunFromSourceBased