Skip to content

ci: bound the windows step correctly, and make kotlin cancellation actually cancel - #641

Merged
oyvindberg merged 1 commit into
masterfrom
fix-win-hang-properly
Aug 2, 2026
Merged

ci: bound the windows step correctly, and make kotlin cancellation actually cancel#641
oyvindberg merged 1 commit into
masterfrom
fix-win-hang-properly

Conversation

@oyvindberg

Copy link
Copy Markdown
Owner

The bound added in #639 did not work. Run 30752489661 hung for the same 55 minutes, with the same missing bsp-diagnostics-windows-latest artifact, despite run-bounded.sh being in effect. It bounded the wrong thing.

Reproduced, not guessed

A fixture that spawns a daemon which escapes a process-group kill and then hangs, both scripts given a 5–8s bound:

script result
old (child inherits stdout) still blocked after 20s
new (child writes to a file) finished in 12s — with the survivor still alive

So killing the process was never the missing piece.

What was actually wrong

GitHub waits for the step's output pipes to close, not for the shell to exit. bleep spawns a BSP daemon, which spawns forked test JVMs — all inheriting stdout. Kill the child and the pipe is still held by a grandchild. That is how a 20-minute bound produced a 45-minute step and a runner destroyed with the telemetry steps still pending.

Two changes, and the second is the one that matters

  1. Kill the process group (set -m so the child leads its own), plus taskkill //F //T on Windows, where processes are not in POSIX process groups.
  2. Redirect the child to a file. Descendants inherit the file, not the pipe. Only tail holds the pipe, and tail is ours to kill — so the step ends even when something survives, which is exactly the case that hung.

On timeout the last 200 lines print in a collapsed group, since the point of all this is to still have the evidence.

Verified: exit 0 on success, 7 propagated from the child, 124 on expiry, grandchildren reaped, streaming intact during a normal run, and the pipe closed with a deliberate survivor left running.

Also: the second flaky cancellation test

Kotlin: fiber cancellation interrupts compilation failed the same run with TimeoutException: 30 seconds. (The MachineResourcesTest fix from #639 held — that one passed.)

These compile through IO.interruptible, whose cancellation interrupts the thread and then waits for the block to return. kotlinc does not promise to notice an interrupt, so fiber.cancel can legitimately take as long as the entire compile — and each of these tests already accepts that outcome explicitly ("completed before cancellation took effect"). The bound only rules out waiting forever.

30s did not clear a full compile of a deliberately huge generated source on a contended runner. The suite runs in 3.8s healthy, so this was an 8× outlier — the shape of "kotlinc never reached an interruptible point", not of a slightly tight bound.

Named CancellationHangGuard, 120s, applied to all four sites: they share the pattern, and the other three would fail the next time a runner is busy. Same reasoning as the wall-clock bounds loosened in #623.

Note

This makes the hang fail fast and preserve diagnostics. It still does not explain why Windows hangs in the first place — that needs the bsp-diagnostics-windows-latest artifact this change finally makes survivable.

🤖 Generated with Claude Code

The bound added in #639 did not work: run 30752489661 hung the same 55 minutes
with the same missing bsp-diagnostics-windows-latest artifact, despite
run-bounded.sh being in effect. The reason is that it bounded the wrong thing.

Reproduced locally, with a fixture that spawns a daemon which escapes a process
group kill and then hangs, both scripts given a 5-8s bound:

  old (child inherits stdout):  still blocked after 20s
  new (child writes to a file): finished in 12s, WITH the survivor still alive

So killing the process was never the missing piece. GitHub waits for the step's
output PIPES to close, not for the shell to exit — and `bleep` spawns a BSP
daemon which spawns forked test JVMs, all inheriting stdout. Kill the child and
the pipe is still held by a grandchild, so a 20-minute bound produced a
45-minute step and the runner was destroyed with the telemetry steps pending.

Two changes, and the second is the one that matters:

  1. Kill the process GROUP (`set -m` so the child leads its own), plus
     `taskkill //F //T` on Windows, where processes are not in POSIX groups.
  2. Redirect the child to a FILE. Descendants then inherit the file, not the
     pipe. Only `tail` holds the pipe, and `tail` is ours to kill — so the step
     ends even when something survives, which is precisely the case that hung.

On timeout the last 200 lines are printed in a collapsed group, since the whole
point is to still have the evidence.

Verified: exit 0 on success, 7 propagated from the child, 124 on expiry,
grandchildren reaped, streaming intact during a normal run, and the pipe closed
with a deliberate survivor left running.

Also fixes the second flaky cancellation test, `Kotlin: fiber cancellation
interrupts compilation`, which failed the same run with
`TimeoutException: 30 seconds`. These compile through `IO.interruptible`, whose
cancellation interrupts the thread and then WAITS for the block to return —
and kotlinc does not promise to notice. So `fiber.cancel` can take as long as
the whole compile, which each of these tests already accepts explicitly
("completed before cancellation took effect"). The bound only rules out waiting
forever, and 30s did not clear a full compile of a deliberately huge generated
source on a contended runner: the suite is 3.8s healthy, so this was an 8x
outlier, the shape of "never reached an interruptible point" rather than of a
slightly tight bound. Named `CancellationHangGuard`, 120s, applied to all four
sites — they share the pattern, and the other three would fail next time a
runner is busy.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@oyvindberg
oyvindberg merged commit b34e5f5 into master Aug 2, 2026
9 checks passed
@oyvindberg
oyvindberg deleted the fix-win-hang-properly branch August 2, 2026 17:16
@oyvindberg oyvindberg changed the title ci: the windows hang was stdout inheritance, not a surviving process ci: bound the windows step correctly, and make kotlin cancellation actually cancel Aug 2, 2026
@oyvindberg

Copy link
Copy Markdown
Owner Author

Update: the cancellation-test fix in this PR was wrong, and hid a real bug

The first version bumped these tests to a 120s timeout. That was papering over — cancelling a compile should cost about nothing, and a bound tolerating two minutes asserts the opposite.

Setting the bound to what it should be (each test's own sleep + a 200ms cancellation budget) isolated it immediately:

compiler time to cancel
Scala (fiber) 162ms
Scala (mid-compile, ProgressCallback) 4ms
Java fast
Kotlin as long as the entire compile

Root cause

kotlinc observes cancellation only when it calls back into our message collector or polls the Services status. 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 finished by itself. compileWithReflection accidentally did better, because it parks the caller in an interruptible join.

That also explains the timing. 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 — so this is a regression that shipped in M11 and only showed up as a flaky test.

Fix

runCancellably gives the compile its own thread and has the caller wait on a latch either side can trip (cancellation.onCancel for one, 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 can't hold the JVM open.

Note on reproducibility

This 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, so it's the point of the change rather than incidental.

Verification

  • CancellationTest 7/7, three consecutive runs
  • the suite got faster (2314ms → 1578ms) because the abandoned compile no longer runs to completion
  • full bleep-bsp-tests minus linker suites: 618 passed, 0 failed

@oyvindberg

Copy link
Copy Markdown
Owner Author

Correction: the comment above describes work that is not in this PR — #641 was already merged when I wrote it. The Kotlin cancellation fix it describes is in #642.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant