Skip to content

windows: stop leaking directory handles in test teardown, and decode server logs leniently - #626

Merged
oyvindberg merged 2 commits into
masterfrom
windows-file-handling
Jul 29, 2026
Merged

windows: stop leaking directory handles in test teardown, and decode server logs leniently#626
oyvindberg merged 2 commits into
masterfrom
windows-file-handling

Conversation

@oyvindberg

Copy link
Copy Markdown
Owner

Two Windows-only CI failures, both caused by code that is merely untidy on POSIX. Independent of #624 — no workflow changes, no overlap.

1. DirectoryNotEmptyException out of test teardown

29 test files each carried their own copy of:

Files.list(path).toScala(List).foreach(deleteRecursively)

Files.list returns a stream backed by an open directory handle. Materialising it with .toScala(List) never closes that handle, so it lives until GC gets around to it. On POSIX nothing notices. On Windows a directory entry with a live handle isn't removed when you delete it, only marked delete pending — so the children look deleted, and removing the parent then fails with DirectoryNotEmptyException.

That's how it surfaced on windows-latest (run 30456495265): PlatformCancellationTest passed its assertion

+ Pre-cancelled Kotlin/JS compilation correctly returned ThreadOutcome.Cancelled

— and then threw out of the finally block:

java.nio.file.DirectoryNotEmptyException: C:\...\Temp\kotlin-js-cancel-10020005492201092876
  at bleep.analysis.PlatformCancellationTest.deleteRecursively(PlatformCancellationTest.scala:29)

830 passing tests, one failed run. Which of the 29 gets hit depends on GC timing — that's why Windows read as flaky rather than broken.

Replaced all 29 with one top-level deleteRecursively in bleep.analysis, built on Files.walkFileTree: the JDK's own recursive-delete recipe, which closes each directory stream before postVisitDirectory deletes the directory. Top-level, so no test needs an import. Seven files were left with an unused StreamConverters import that -Werror rightly rejected; those are gone too.

2. Build failed: Input length = 1

BspRifle.readOutput / readOutputTail and one diagnostic path in BspServerOperations read the server's captured stdout/stderr with Files.readString / Files.readAllLines. Both decode as strict UTF-8 and throw MalformedInputException on the first byte that doesn't decode.

That log isn't ours — it's whatever the server and every subprocess it spawned wrote, and on Windows that routinely includes console-codepage bytes. One stray byte in a diagnostic log was enough to fail the build that was reading it, with a message that names neither the file nor the cause.

containsOomMarker already got this right, with a comment saying exactly why:

lossy decode: the log interleaves subprocess output that need not be valid UTF-8, and a diagnosis helper must not throw over it

So this is finishing a job already started: that lossy read is promoted to BspServerOperations.readLogFile and the other three call sites now use it.

Verification

macOS: full bleep-bsp-tests green (648 passed, 0 failed, 73 suites), whole build compiles, bleep fmt clean.

The Windows behaviour itself can only be confirmed by CI — I can't reproduce either failure locally. #2 in particular is a strong inference from the error string plus the pre-existing comment on containsOomMarker, not something I observed directly; the failing stack never reaches the log because the top-level handler prints only the message.

Not fixed here

The ubuntu-22.04 / ubuntu-22.04-arm failures are unrelated: the compile-server self-deadlock from #622, where a test fork is charged CPU twice and every permit ends up held by a task waiting for a permit that can't exist. Symptom is 26-29 minutes of total silence before the job ceiling kills it. #623 fixes that (e3ad7bf0) and is the only PR currently passing both Linux jobs.

🤖 Generated with Claude Code

…server logs leniently

Two unrelated Windows-only failures in CI, both from code that is merely untidy
on POSIX.

1. `DirectoryNotEmptyException` out of test teardown.

29 test files each carried their own copy of:

    Files.list(path).toScala(List).foreach(deleteRecursively)

`Files.list` returns a stream backed by an open directory handle. Materialising
it with `.toScala(List)` never closes that handle, so it survives until GC. On
POSIX nothing notices. On Windows a directory entry with a live handle is not
removed when you delete it, only marked "delete pending" -- so the children look
deleted, and removing the parent then fails with `DirectoryNotEmptyException`.

That is how it surfaced on windows-latest: `PlatformCancellationTest` passed its
assertion and then threw out of the `finally` block, turning 830 green tests into
a failed run. Which of the 29 gets hit depends on GC timing, which is why Windows
read as flaky rather than broken.

Replaced all 29 with one top-level `deleteRecursively` in `bleep.analysis`, built
on `Files.walkFileTree` -- the JDK's own recursive-delete recipe, which closes
each directory stream before `postVisitDirectory` deletes the directory. Being
top-level, no test needs an import. Seven files kept a now-unused
`StreamConverters` import, which `-Werror` rightly rejected; those are gone too.

2. `Build failed: Input length = 1`.

`BspRifle.readOutput`/`readOutputTail` and one diagnostic path in
`BspServerOperations` read the server's captured stdout/stderr with
`Files.readString`/`Files.readAllLines`. Both decode as strict UTF-8 and throw
`MalformedInputException` on the first byte that does not decode. That log is not
ours -- it is whatever the server and every subprocess it spawned wrote, and on
Windows that routinely includes console-codepage bytes. One stray byte in a
diagnostic log was enough to fail the build that was reading it.

`containsOomMarker` already got this right, with a comment explaining exactly
why. Promoted that lossy read to `BspServerOperations.readLogFile` and pointed
the other three call sites at it.

Verified on macOS: full `bleep-bsp-tests` green (648 passed, 0 failed), whole
build compiles. The Windows behaviour itself can only be confirmed by CI.
@oyvindberg
oyvindberg force-pushed the windows-file-handling branch from 17e3cf0 to fc35a05 Compare July 29, 2026 17:20
@oyvindberg
oyvindberg marked this pull request as draft July 29, 2026 17:24
The handle-leak fix in the parent commit was real but was not what made these
tests fail. `PlatformCancellationTest` failed again after it, this time on
ubuntu, with `DirectoryNotEmptyException` raised from inside the new
`walkFileTree` -- so the cause is not Windows delete-pending semantics and not an
unclosed stream.

`Outcome.runInFreshThread` implements cancellation as `cancel()` +
`Thread.interrupt()` and returns `ThreadOutcome.Cancelled` without waiting for
the worker to stop, parking it in `runawayThreads` on the grounds that "most
native compilers ignore interrupt and keep running". A cancellation test that
asserts `Cancelled` and then deletes its output directory is therefore racing a
kotlinc still emitting into it: the walk empties the directory, the writer
refills it, and unlinking the directory fails.

Retry the walk so the delete converges once the runaway writer is done, and use
deleteIfExists so an entry that vanishes mid-walk is not an error either. Bounded
at ten attempts -- if the tree will not settle the exception is rethrown, rather
than leaving cleanup silently half-done.

Ran PlatformCancellationTest three times locally, green each time.
@oyvindberg
oyvindberg marked this pull request as ready for review July 29, 2026 22:47
@oyvindberg
oyvindberg merged commit 91d8db3 into master Jul 29, 2026
17 of 18 checks passed
@oyvindberg
oyvindberg deleted the windows-file-handling branch July 29, 2026 22:47
oyvindberg pushed a commit that referenced this pull request Jul 29, 2026
Experiment against `Build failed: Input length = 1` in "Compile and publish
locally". That is a MalformedInputException from a strict UTF-8 decode, and the
bleep running there is the released M9 from the setup action -- not this
checkout, whose dev script does not exist until two steps later. M9 read the
server's `output` log with Files.readString for readiness and error messages;
master removed that read in #617 and made the survivors lossy in #626, but
neither reaches a binary that is already on disk.

So change what goes into the log instead. With stdout redirected to a file a
JDK 19+ JVM takes stdout.encoding from native.encoding, which on Windows is the
ANSI codepage -- mappable non-ASCII becomes a single high byte (invalid UTF-8)
and unmappable characters become '?'. This job's log shows exactly that mangling:
4 mangled lines here against 0 in the cmd-shell runs.

Via JAVA_TOOL_OPTIONS because the writer is the server JVM the client spawns, and
it inherits the variable.

Unverified -- it targets the mechanism, but the mechanism is inferred from the
error class plus the mangling, not from the log bytes themselves. #626 collects
Windows telemetry now, so a repeat gives us the actual log.
oyvindberg pushed a commit that referenced this pull request Jul 29, 2026
…tEmpty

#626 retried the delete only on DirectoryNotEmptyException, which is how the race
presents on Linux. Windows presents the same race as a blocked unlink instead, so
#625 went from failing PlatformCancellationTest to failing TimeoutAndResourceTest:

  java.nio.file.FileSystemException:
    C:\...\Temp\scalajs-mid-cancel...: The process cannot access the file
    at Files.deleteIfExists
    at TestFiles$package$$anon$1.postVisitDirectory(TestFiles.scala:55)

One cause, two symptoms: a cancelled-but-still-running toolchain either puts a
file back after the walk emptied the directory (ubuntu) or holds a handle that
blocks the unlink (windows). Both are FileSystemException, so retry on that
rather than on either leaf type. Budget raised to 20 × 100ms, enough for a killed
node or kotlinc to exit and drop its handles.

TimeoutAndResourceTest and PlatformCancellationTest both green locally.
oyvindberg added a commit that referenced this pull request Jul 29, 2026
…fy CI on one shell (#624)

* windows: resolve user dirs via SHGetKnownFolderPath, not powershell + Add-Type

Every `bleep` invocation on Windows shelled out to
`powershell.exe -NoProfile -EncodedCommand` and compiled C# with `Add-Type`
at runtime, just to learn where its own cache and config directories live.
`Main._main` does it before anything else and the BSP server does it from
five more places, none of them memoized. That is the startup cost, and every
one of those spawns is a hard failure point: PowerShell in Constrained
Language Mode, AppLocker/WDAC, powershell.exe off PATH, a non-writable TEMP,
or a PSModulePath inherited from a parent pwsh that breaks Add-Type.

The fix already shipped in the code we depend on; it was just unreachable.
coursier-cache 2.1.24 carries an FFM implementation under
META-INF/versions/23, but that jar has no MANIFEST.MF at all — so no
`Multi-Release: true`, so the JDK never loads it. And WindowsJni gates the
JNI path off for java >= 23 precisely because it expects the multi-release
class to take over. Both escape hatches closed, PowerShell every time.

coursier split coursier-paths out in 2.1.25 with `Multi-Release: true` and
the impl under META-INF/versions/22, so JDK 22+ gets WindowsForeign.
Verified rather than assumed:

  JDK 21 -> coursier.paths.shaded.dirs.impl.WindowsPowerShell
  JDK 25 -> coursier.paths.shaded.dirs.impl.WindowsForeign

`coursier.cache.shaded.dirs` no longer exists in 2.1.25, so UserPaths had to
move regardless of any of this. `fromAppDirs` is now a lazy val.

Native image needs `foreign` reachability metadata for the two downcalls or
it aborts at runtime with MissingForeignRegistrationError — the same failure
class as #601, which stayed invisible until a real Windows user hit it.
Registered SHGetKnownFolderPath's descriptor (CLSIDFromString's shape is
already covered by the native-terminal file; matching is by shape, not name),
and `selftest` now prints and validates the resolved directories so CI has
to link them. The BSP server JVM gets --enable-native-access=ALL-UNNAMED on
JDK 22+, which the native image already passed; without it the JVM warns on
every start and a future JDK will refuse the call outright.

The bump costs 73 deprecation sites across 12 files, all load-bearing under
-Werror: Dependency.version -> versionConstraint.asString, configuration ->
variantSelector, Project.version -> version0, Conflict.*Version ->
*VersionConstraint, Resolution.{projectCache,subset,dependencyArtifacts} ->
the 0 variants, Authentication.user -> userOpt. New internal/coursierDeps.scala
converts at the two boundaries where coursier generalised a type bleep cannot
express (Gradle Module variants) — throwing rather than defaulting, and note
coursier's own deprecated dependencyArtifacts silently collects those away.
bleep's CoursierResolver.Result deliberately stays the narrower shape, which
also keeps the on-disk resolution cache format byte-identical.

Snapshot churn is coursier 2.1.25 emitting resolution modules in a different
order: all 1094 files diff order-only, zero semantic change under an
order-insensitive comparison, and a second run produces no further drift.

906 tests pass, 0 failed, including the slow ITs.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* ci: one shell for every OS, and drop the setup-graalvm action

The native-image job carried a parallel universe of eight `shell: cmd` /
`shell: pwsh` Windows steps beside the unix ones. That existed because bleep
could not resolve its own directories without spawning PowerShell, so nesting
one inside a pwsh step was a known way to break it. The parent commit removes
that constraint, so every step is now `shell: bash` — GitHub's Windows runners
ship Git Bash, and it runs .bat/.cmd through the Windows loader. The only
per-OS difference left is the name of the two generated launcher scripts,
hence matrix.dev_script / matrix.ni_script.

bash also means GitHub runs each block with `set -eo pipefail`, so every
command's exit code is load-bearing. The old cmd blocks propagated only the
last command's status, which is how 42 Windows test failures stayed hidden
behind green runs (#607).

graalvm/setup-graalvm@v1 and the JAVA_HOME override in GenNativeImage are both
gone. That override dated to GraalVM 21 and said "the one we install ourselves
does not work"; neither reason survives on graalvm-community 25. Read out of
the actual Windows distribution bleep provisions:

  bin/native-image.cmd          present  <- what NativeImagePlugin resolves
  bin/java.exe                  present
  bin/gu                        ABSENT   <- so the gu-install fallback is dead

and native-image.cmd locates lib/svm/bin/native-image.exe purely relative to
itself, never reading JAVA_HOME. Since GraalVM for JDK 17/20 it also finds
Visual Studio through vswhere and sets up the MSVC environment itself, so no
developer command prompt is needed. The override was inert off Windows anyway
— it filtered on bin/java.exe existing — so this is a no-op for linux/macOS.

Two Windows-only steps remain and neither is about shells: nothing, now, plus
the mandatory-file-lock cleanup. That cleanup had been looking under
%USERPROFILE%\.bleep\cache for the BSP classpath cache, a directory bleep has
never used on Windows — UserPaths.cacheDir is %LOCALAPPDATA%\bleep\cache,
confirmed against a runner log. So it silently matched nothing on every run,
and the dev script could resolve bleep-bsp from a pre-publish classpath. Fixed,
and it now warns and lists the directory instead of no-op'ing if the glob ever
comes up empty again.

Costs `native-image-job-reports`, the action's PR size/metrics comment.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* ci: force UTF-8 stdout/stderr for the windows bootstrap step

Experiment against `Build failed: Input length = 1` in "Compile and publish
locally". That is a MalformedInputException from a strict UTF-8 decode, and the
bleep running there is the released M9 from the setup action -- not this
checkout, whose dev script does not exist until two steps later. M9 read the
server's `output` log with Files.readString for readiness and error messages;
master removed that read in #617 and made the survivors lossy in #626, but
neither reaches a binary that is already on disk.

So change what goes into the log instead. With stdout redirected to a file a
JDK 19+ JVM takes stdout.encoding from native.encoding, which on Windows is the
ANSI codepage -- mappable non-ASCII becomes a single high byte (invalid UTF-8)
and unmappable characters become '?'. This job's log shows exactly that mangling:
4 mangled lines here against 0 in the cmd-shell runs.

Via JAVA_TOOL_OPTIONS because the writer is the server JVM the client spawns, and
it inherits the variable.

Unverified -- it targets the mechanism, but the mechanism is inferred from the
error class plus the mangling, not from the log bytes themselves. #626 collects
Windows telemetry now, so a repeat gives us the actual log.

---------

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Co-authored-by: Øyvind Raddum Berg <[email protected]>
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