kotlin: cancelling a compile now returns at once, instead of waiting for kotlinc

bleep-bsp-tests/src/scala/bleep/analysis/CancellationTest.scala

@@ -18,20 +18,18 @@ import scala.concurrent.duration.*
   */
 class CancellationTest extends AnyFunSuite with Matchers {
 
-  /** Hang guard for the fiber-cancellation tests, not a measurement of how fast cancellation is.
+  /** Cancelling a compile must return immediately. This is the assertion, not a hang guard.
     *
-    * These compile through `IO.interruptible`, whose cancellation interrupts the worker thread and then WAITS for the block to return. Neither scalac, kotlinc
-    * nor javac promises to notice an interrupt promptly, so `fiber.cancel` can legitimately take as long as the whole compile — and each of these tests already
-    * accepts that outcome explicitly ("compilation completed before cancellation took effect"). The only thing the bound rules out is waiting forever.
+    * Each site spends it ON TOP of that test's own `IO.sleep`, because the tests do not all wait the same amount before cancelling — one deliberately sleeps
+    * 500ms to get well into a compile. A flat total would assert "the sleep plus the cancel", which is a different and weaker claim for every test that sleeps
+    * longer.
     *
-    * So it has to clear a FULL uncancelled compile of a deliberately huge generated source on a contended runner, and 30s did not: the whole suite runs in 3.8s
-    * healthy, and CI still saw one of these blow through 30s while the other suites had the machine busy. That is a 8x outlier against healthy, which is the
-    * shape of "kotlinc never reached an interruptible point", not of "the bound is slightly tight".
-    *
-    * Applied to all four sites rather than the one that failed, on the same reasoning as the wall-clock bounds loosened in #623: they share the pattern and the
-    * flaw, and the other three would fail the next time a runner is busy.
+    * Deliberately tight. The previous 30s was loose enough to hide the real behaviour: it passed not because cancellation worked but because a fast machine
+    * finished the whole compile inside the bound, and CI failed it only once the machine was contended enough for a full compile to exceed 30s. Raising it to
+    * 120s would have buried that for good, which is the wrong direction — a build tool that takes a minute to honour Ctrl-C is broken whether or not a test
+    * says so.
     */
-  private val CancellationHangGuard = 120.seconds
+  private val CancelBudget = 200.millis
 
   def createTempDir(prefix: String): Path =
     Files.createTempDirectory(prefix)
@@ -185,7 +183,7 @@ class CancellationTest extends AnyFunSuite with Matchers {
         _ <- IO(cancellation.cancel()) // Signal cancellation to the compiler
         _ <- fiber.cancel // Cancel the fiber
         outcome <- fiber.join
-      } yield outcome).timeout(CancellationHangGuard)
+      } yield outcome).timeout(100.millis + CancelBudget)
 
       val startTime = System.currentTimeMillis()
       val outcome = program.unsafeRunSync()
@@ -236,7 +234,7 @@ class CancellationTest extends AnyFunSuite with Matchers {
         _ <- IO(cancellation.cancel())
         _ <- fiber.cancel
         outcome <- fiber.join
-      } yield outcome).timeout(CancellationHangGuard)
+      } yield outcome).timeout(500.millis + CancelBudget)
 
       val startTime = System.currentTimeMillis()
       val outcome = program.unsafeRunSync()
@@ -311,7 +309,7 @@ class CancellationTest extends AnyFunSuite with Matchers {
         _ <- IO(cancellation.cancel())
         _ <- fiber.cancel
         outcome <- fiber.join
-      } yield outcome).timeout(CancellationHangGuard)
+      } yield outcome).timeout(100.millis + CancelBudget)
 
       val startTime = System.currentTimeMillis()
       val outcome = program.unsafeRunSync()
@@ -386,7 +384,7 @@ class CancellationTest extends AnyFunSuite with Matchers {
         _ <- IO(cancellation.cancel())
         _ <- fiber.cancel
         outcome <- fiber.join
-      } yield outcome).timeout(CancellationHangGuard)
+      } yield outcome).timeout(50.millis + CancelBudget)
 
       val startTime = System.currentTimeMillis()
       val outcome = program.unsafeRunSync()

bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala

@@ -184,6 +184,79 @@ object KotlinSourceCompiler extends Compiler {
   // Compilation
   // ==========================================================================
 
+  /** Run a compile so that cancelling it returns immediately, whatever kotlinc happens to be doing.
+    *
+    * kotlinc only observes cancellation when it calls back into our message collector or polls the `Services` status, and between those points it is
+    * unreachable: `Thread.interrupt` sets a flag nothing checks. The incremental path made that visible, because it invokes the compiler on the CALLING thread
+    * — so cancellation could not return until the entire compile had finished. Measured against the other two compilers on the same tests, Scala returns in
+    * 4-162ms and Java likewise, while Kotlin took as long as the compile, which on a contended CI runner meant blowing a 30s bound.
+    *
+    * So the compile gets its own thread and the caller waits on a latch either side can trip. A cancelled compile ABANDONS that thread rather than joining it —
+    * the same trade `Outcome.runInFreshThread` documents for native compilers that ignore interrupts. The work is wasted either way; the only question is
+    * whether the user waits for output nobody wants. The thread is a daemon so an abandoned one cannot hold the JVM open.
+    */
+  private def runCancellably(name: String, loader: ClassLoader, cancellation: CancellationToken)(work: => CompilationResult): CompilationResult = {
+    val done = new java.util.concurrent.CountDownLatch(1)
+    val holder = new java.util.concurrent.atomic.AtomicReference[CompilationResult]()
+    lazy val worker: Thread = new Thread(
+      () =>
+        try holder.set(work)
+        finally {
+          abandonedKotlinCompiles.remove(worker): Unit
+          done.countDown()
+        },
+      name
+    )
+    worker.setContextClassLoader(loader)
+    worker.setDaemon(true)
+    worker.start()
+
+    // Fires immediately if the token is already cancelled, so a cancel that lands before this returns is not lost.
+    cancellation.onCancel(() => done.countDown())
+
+    val finished =
+      try done.await(CompileTimeoutMinutes, java.util.concurrent.TimeUnit.MINUTES)
+      catch {
+        case _: InterruptedException =>
+          // The caller was cancelled through `IO.interruptible`. Re-assert the flag so anything above still sees it.
+          Thread.currentThread().interrupt()
+          worker.interrupt()
+          return CompilationCancelled
+      }
+
+    if cancellation.isCancelled then {
+      worker.interrupt() // best effort — kotlinc may well ignore it, which is why we do not wait
+      if worker.isAlive then abandonedKotlinCompiles.add(worker): Unit
+      CompilationCancelled
+    } else if !finished then {
+      val err = CompilerError(None, 0, 0, s"Kotlin compilation timed out after $CompileTimeoutMinutes minutes", None, CompilerError.Severity.Error)
+      CompilationFailure(List(err))
+    } else holder.get()
+  }
+
+  private val CompileTimeoutMinutes = 5L
+
+  /** Kotlin compiles that a cancel walked away from and which had not stopped yet.
+    *
+    * They do not necessarily run to completion: kotlinc calls `checkCanceled` at phase boundaries and our proxy throws there, so most stop shortly after the
+    * cancel. But that is cooperation, not a guarantee — one inside a long stretch that polls neither the status nor the message collector runs to the end,
+    * still emitting class files into the output directory after we have reported `CompilationCancelled`.
+    *
+    * Identity-set, and a thread that finishes removes itself, so the snapshot is a live count rather than a tally. Same bookkeeping as
+    * [[ZincBridge.abandonedEcjThreads]], which exists for the same reason and is the precedent for accepting the trade at all.
+    */
+  private[analysis] val abandonedKotlinCompiles: java.util.Set[Thread] =
+    java.util.Collections.newSetFromMap(new ConcurrentHashMap[Thread, java.lang.Boolean]())
+
+  /** Snapshot of Kotlin compiles still running after being cancelled. For diagnostics — a non-empty list during a build means something is writing into an
+    * output directory nobody is waiting for.
+    */
+  def abandonedKotlinCompilesSnapshot: List[(String, Long)] = {
+    val out = List.newBuilder[(String, Long)]
+    abandonedKotlinCompiles.forEach(t => out += ((t.getName, t.threadId())))
+    out.result()
+  }
+
   override def compile(
       input: CompilationInput,
       listener: DiagnosticListener,
@@ -218,22 +291,25 @@ object KotlinSourceCompiler extends Compiler {
       // Get or create cached compiler setup
       val setup = getOrCreateSetup(config.version)
 
-      // Try incremental compilation first, fall back to full compilation
-      val result = if setup.incrementalRunnerClass != null then {
-        debug(s"Compiling ${sourcePaths.size} Kotlin files (incremental)")
-        val incrementalResult = compileIncremental(setup, config, sourcePaths, input, listener, cancellation)
-        incrementalResult match {
-          case CompilationFailure(errs) if errs.exists(e => e.message.contains("cache\" is null") || e.message.contains("cache is null")) =>
-            // Kotlin IC cache is corrupted — invalidate and retry
-            debug("Kotlin IC cache corrupted (cache is null), invalidating and retrying")
-            val cacheDir = input.outputDir.resolve(".kotlin-ic")
-            invalidateCache(cacheDir)
-            compileIncremental(setup, config, sourcePaths, input, listener, cancellation)
-          case other => other
+      // Both paths run through `runCancellably`, so a cancel returns at once instead of waiting for kotlinc. The
+      // incremental path needs it most — it invokes the compiler inline, on this very thread.
+      val result = runCancellably("kotlin-compile", setup.loader, cancellation) {
+        if setup.incrementalRunnerClass != null then {
+          debug(s"Compiling ${sourcePaths.size} Kotlin files (incremental)")
+          val incrementalResult = compileIncremental(setup, config, sourcePaths, input, listener, cancellation)
+          incrementalResult match {
+            case CompilationFailure(errs) if errs.exists(e => e.message.contains("cache\" is null") || e.message.contains("cache is null")) =>
+              // Kotlin IC cache is corrupted — invalidate and retry
+              debug("Kotlin IC cache corrupted (cache is null), invalidating and retrying")
+              val cacheDir = input.outputDir.resolve(".kotlin-ic")
+              invalidateCache(cacheDir)
+              compileIncremental(setup, config, sourcePaths, input, listener, cancellation)
+            case other => other
+          }
+        } else {
+          debug(s"Compiling ${sourcePaths.size} Kotlin files (full)")
+          compileWithReflection(setup, config, sourcePaths, input, listener, cancellation)
         }
-      } else {
-        debug(s"Compiling ${sourcePaths.size} Kotlin files (full)")
-        compileWithReflection(setup, config, sourcePaths, input, listener, cancellation)
       }
 
       result match {