diff --git a/bleep-bsp-tests/src/scala/bleep/analysis/CancellationTest.scala b/bleep-bsp-tests/src/scala/bleep/analysis/CancellationTest.scala index be64f9607..4c287b0a3 100644 --- a/bleep-bsp-tests/src/scala/bleep/analysis/CancellationTest.scala +++ b/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() diff --git a/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala b/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala index a3850e326..953def163 100644 --- a/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala +++ b/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 {