From 0e37b51f04f12b6d9e2d4f46e21be16ce76f58eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Raddum=20Berg?= Date: Sun, 2 Aug 2026 20:15:10 +0200 Subject: [PATCH 1/2] kotlin: cancelling a compile now returns at once, instead of waiting for kotlinc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 120s I put on these tests in the previous commit was wrong, and hid a real bug. Cancelling a compile should cost about nothing; a bound that tolerates two minutes asserts the opposite. Setting the bound to what it should be — the test's own sleep plus a 200ms cancellation budget — isolated it immediately. Scala returns in 162ms and its mid-compile ProgressCallback variant in 4ms; Java likewise. Only Kotlin failed, and it failed by taking as long as the whole compile. The cause: kotlinc observes cancellation only 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. `compileIncremental` then invokes the compiler ON THE CALLING THREAD, so there was nothing to interrupt into: `IO.interruptible` cancellation could not return until the compile had finished by itself. `compileWithReflection` accidentally did better, because it parks the caller in an interruptible `join`. That also explains why this surfaced now rather than earlier. Before #625 the incremental runner never resolved, so every Kotlin compile took the reflection path. Making incremental compilation actually engage moved these compiles onto the path with no way out. `runCancellably` gives the compile its own thread and has the caller wait on a latch either side can trip — `cancellation.onCancel` for one side, completion for the other. 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, and the alternative is making the user wait for output nobody wants. Daemon thread, so an abandoned one cannot hold the JVM open. Both paths go through it, since the fallback deserves the same guarantee. Note it cannot reproduce on a fast machine: the compile finishes inside any generous bound, which is exactly how a 30s timeout passed for so long while being wrong. The tight bound is what makes the behaviour observable at all, so it is the point of the change rather than incidental to it. Per-site budgets rather than one flat number, because the tests do not all wait the same time before cancelling — one deliberately sleeps 500ms to get well into a compile, and a flat total would assert something weaker for it than for the others. Verified: CancellationTest 7/7, three consecutive runs, and the suite got FASTER (2314ms -> 1578ms) because the abandoned compile no longer runs to completion. Full bleep-bsp-tests minus the linker suites: 618 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- .../bleep/analysis/CancellationTest.scala | 28 +++---- .../bleep/analysis/KotlinSourceCompiler.scala | 81 +++++++++++++++---- 2 files changed, 79 insertions(+), 30 deletions(-) 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..4cc90b852 100644 --- a/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala +++ b/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala @@ -184,6 +184,54 @@ 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]() + val worker = new Thread( + () => + try holder.set(work) + finally 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 + 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 + override def compile( input: CompilationInput, listener: DiagnosticListener, @@ -218,22 +266,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 { From f85aefe84328cf799bf61c72c3ab8a19b87aec5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Raddum=20Berg?= Date: Sun, 2 Aug 2026 20:23:18 +0200 Subject: [PATCH 2/2] kotlin: track the compiles a cancel walks away from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Abandoning a thread is not the same as it stopping, and the previous commit did not say which happens. It is cooperative: kotlinc calls `checkCanceled` at phase boundaries and our `CompilationCanceledStatus` proxy throws there, so most abandoned compiles stop shortly after the cancel. But that is cooperation, not a guarantee — one inside a stretch that polls neither the status nor the message collector runs to the end, still writing class files into an output directory after we have reported CompilationCancelled. That hazard is real and this repo has already been bitten by it: #626 fixed a teardown that raced "a kotlinc still emitting into it", which is the same thing one level up. So the threads are tracked, exactly as ZincBridge.abandonedEcjThreads tracks the ECJ equivalent — identity set, self-removing on completion, snapshot for diagnostics. A non-empty snapshot during a build means something is writing into a directory nobody is waiting for, which is worth being able to see rather than inferring from corrupted output. Not solved here, and worth stating: nothing stops a NEW compile of the same project from starting while an abandoned one still writes. ProjectLock serializes compiles, but the abandoned thread does not hold it. The alternatives are to keep holding the lock until the runaway finishes (correct, but reintroduces the wait for the next compile rather than for the cancel) or to compile into a scratch directory and publish atomically. Both are larger than this fix, and the existing ECJ and native-compiler paths make the same trade today. CancellationTest 7/7. Co-Authored-By: Claude Opus 5 (1M context) --- .../bleep/analysis/KotlinSourceCompiler.scala | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala b/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala index 4cc90b852..953def163 100644 --- a/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala +++ b/bleep-bsp/src/scala/bleep/analysis/KotlinSourceCompiler.scala @@ -198,10 +198,13 @@ object KotlinSourceCompiler extends Compiler { 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]() - val worker = new Thread( + lazy val worker: Thread = new Thread( () => try holder.set(work) - finally done.countDown(), + finally { + abandonedKotlinCompiles.remove(worker): Unit + done.countDown() + }, name ) worker.setContextClassLoader(loader) @@ -223,6 +226,7 @@ object KotlinSourceCompiler extends Compiler { 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) @@ -232,6 +236,27 @@ object KotlinSourceCompiler extends Compiler { 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,