luke
/
sbt
mirror of https://github.com/sbt/sbt.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[2.x] fix: Fixes three test-classloader / jar-handle leaks in the sbt server JVM (#9485) 

Three independent, pre-existing retention bugs kept the classloader of a
finished in-process test run -- and the open jar handles it holds -- alive
for the rest of the server session.

- JUnitXmlTestsListener: testSuite is an InheritableThreadLocal, so
  threads spawned during a run (e.g. async-framework pool workers) inherit
  a copy of the suite reference that remove() cannot reach.

- TestRecap.collect: the recap is stashed on State.attributes and outlives
  the command, so it must not retain live throwables.

- ClassLoaderCache: loaders evicted from delegate by clearExpiredLoaders
  are unreachable from the map and never enqueued on the ReferenceQueue, so
  neither clear()/close() nor the cleanup thread could ever close them.

SuiteResult now documents the retention hazard on throwables. Adds
deterministic, cross-platform tests for all three severed chains.
This commit is contained in:
Kevin Lee 2026-07-24 08:05:37 +10:00 committed by GitHub
parent 4721f7f496
commit 97a1dc360b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 270 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
main-actions/src/main/scala/sbt/internal/testing/TestRecap.scala
View File

@ -45,7 +45,12 @@ import sbt.util.Logger
*/
private[sbt] object TestRecap:
/** A single failed test task contributing to the recap. */
/**
* A single failed test task contributing to the recap.
*
* `testOutput` is sanitized by [[collect]]: its `SuiteResult.throwables` are dropped so the
* recap cannot pin the test class loader. See [[collect]] for why.
*/
final case class Failure(taskName: String, testOutput: Option[Tests.Output])
/**
@ -53,6 +58,13 @@ private[sbt] object TestRecap:
* aggregated run that produced at least one `TestsFailedException`.
* Monotonic-latest-failure semantics: never cleared on success, only
* overwritten by the next failure.
*
* Note for consumers: the `SuiteResult.throwables` reachable from these
* failures are always empty -- [[collect]] strips them so the recap cannot
* pin the test class loader. An empty `throwables` here therefore means
* "not retained", NOT "no exception was thrown". The real throwables live on
* the `TestsFailedException` in the `Incomplete` tree, which is where error
* reporting reads them from.
*/
val recapKey: AttributeKey[Vector[Failure]] = AttributeKey[Vector[Failure]](
"testRecap",
@ -69,17 +81,44 @@ private[sbt] object TestRecap:
* Identity-deduplicated via `Incomplete.allExceptions` (which uses an
* `IDSet[Throwable]` internally), so a single failing task shared across
* multiple Incomplete paths in a DAG is counted once.
*
* The retained `Tests.Output` is stripped of `SuiteResult.throwables`: the
* recap only renders names and counts, but a test-thrown Throwable's
* backtrace pins the `Class` objects of every frame, and a `Class` strongly
* references its defining class loader. Since this data is stashed on
* `State.attributes` where it outlives the command, retaining the throwables
* would keep the test class loader -- and its open jar handles -- alive for
* the rest of the session. On Windows those handles make the cached jars
* undeletable (e.g. by `clearCaches`).
*/
def collect(i: Incomplete): Vector[Failure] =
Incomplete
.allExceptions(i)
.iterator
.flatMap {
case e: TestsFailedException => Some(Failure(e.taskName, e.testOutput))
case _ => None
case e: TestsFailedException =>
Some(Failure(e.taskName, e.testOutput.map(dropThrowables)))
case _ => None
}
.toVector
private def dropThrowables(o: Tests.Output): Tests.Output =
o.copy(events = o.events.view.mapValues(dropThrowables).toMap)
private def dropThrowables(s: SuiteResult): SuiteResult =
if s.throwables.isEmpty then s
else
new SuiteResult(
s.result,
s.passedCount,
s.failureCount,
s.errorCount,
s.skippedCount,
s.ignoredCount,
s.canceledCount,
s.pendingCount,
)
/**
* The rendered recap as a sequence of `\n`-free lines. Failures are
* sorted by `taskName` (lexicographically; empty task names last) for

46
main-actions/src/test/scala/sbt/internal/testing/TestRecapTest.scala
View File

@ -50,6 +50,52 @@ object TestRecapTest extends verify.BasicTestSuite:
override def log(level: sbt.util.Level.Value, msg: => String): Unit =
lines += level.toString -> msg
test("collect strips SuiteResult.throwables so the recap cannot pin the test classloader") {
// A test-thrown Throwable's backtrace pins the Class objects of every frame, and a Class
// strongly references its defining classloader; the recap is stashed on State.attributes,
// so retaining them would keep the test classloader (and its jar handles) alive.
val thrown = new AssertionError("boom")
val withThrowables =
new SuiteResult(TestResult.Failed, 0, 1, 0, 0, 0, 0, 0, thrown :: Nil)
val i = incompleteOf(
new TestsFailedException(
"a / Test / test",
Some(output(TestResult.Failed, "AFailing" -> withThrowables))
)
)
val collected = TestRecap.collect(i)
val retained = collected.head.testOutput.get.events("AFailing")
assert(retained.throwables.isEmpty, "throwables should be dropped from the retained output")
// Everything the recap actually renders must survive.
assert(retained.result == TestResult.Failed)
assert(retained.failureCount == 1)
assert(collected.head.testOutput.get.overall == TestResult.Failed)
}
test("collect sanitizes a copy and leaves the source exception's throwables intact") {
// The stripping above must not mutate the TestsFailedException in the Incomplete tree:
// that exception is what error reporting and the ClassLoaderLayeringStrategy diagnostic
// in Defaults read, and both need the real throwables. Only the copy parked on
// State.attributes is sanitized.
val thrown = new AssertionError("boom")
val source = new TestsFailedException(
"a / Test / test",
Some(
output(
TestResult.Failed,
"AFailing" -> new SuiteResult(TestResult.Failed, 0, 1, 0, 0, 0, 0, 0, thrown :: Nil)
)
)
)
val collected = TestRecap.collect(incompleteOf(source))
assert(collected.head.testOutput.get.events("AFailing").throwables.isEmpty)
val original = source.testOutput.get.events("AFailing").throwables
assert(
original == (thrown :: Nil),
s"collect must not strip the source exception, but its throwables became: $original"
)
}
test("collect picks up TestsFailedException payloads from the Incomplete tree") {
val i = incompleteOf(
failure("a / Test / test", TestResult.Failed, "AFailing"),

33
main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala
View File

@ -77,12 +77,34 @@ private[sbt] class ClassLoaderCache(
new java.util.concurrent.ConcurrentHashMap[Key, Reference[ClassLoader]]()
private val referenceQueue = new ReferenceQueue[ClassLoader]
/*
* Loaders evicted from `delegate` by clearExpiredLoaders are no longer reachable from the
* map, so clear()/close() alone can never close them. Nor can the cleanup thread: once the
* entry is removed, the Reference object itself becomes unreachable, and an unreachable
* Reference is never enqueued on the ReferenceQueue. They would linger with open jar handles
* for the life of the JVM. On Windows those handles make the underlying jars undeletable
* (e.g. clearCaches cannot delete cas blobs the loaders still reference). Track evicted
* loaders weakly so clear() can close them deterministically; weak keys preserve the
* metaspace-pressure design above by adding no strong retention of their own.
*/
private val retired =
java.util.Collections.synchronizedMap(new java.util.WeakHashMap[ClassLoader, java.lang.Boolean])
private def clearExpiredLoaders(): Unit = lock.synchronized {
val clear = (k: Key, ref: Reference[ClassLoader]) => {
ref.get() match {
case w: WrappedLoader => w.invalidate()
case _ =>
}
ref match {
case ClassLoaderReference(_, underlying) =>
retired.put(underlying, java.lang.Boolean.TRUE)
case r =>
r.get() match {
case null =>
case loader => retired.put(loader, java.lang.Boolean.TRUE)
}
}
delegate.remove(k)
()
}
@ -109,6 +131,7 @@ private[sbt] class ClassLoaderCache(
referenceQueue.remove(1000) match {
case ClassLoaderReference(key, classLoader) =>
close(classLoader)
retired.remove(classLoader)
delegate.remove(key)
()
case _ =>
@ -148,6 +171,10 @@ private[sbt] class ClassLoaderCache(
* handle from being modified. On linux and mac, we probably leak some file descriptors but it's
* fairly uncommon for sbt to run out of file descriptors.
*
* Loaders evicted by clearExpiredLoaders (as opposed to by garbage collection) are a separate
* case: they are removed from `delegate`, so neither clear()/close() nor the reference-queue
* cleanup thread can reach them. Those are tracked weakly in `retired` and closed by
* clear()/close(), so their handles are released deterministically rather than never.
*/
private val metaspaceIsLimited =
ManagementFactory.getMemoryPoolMXBeans.asScala
@ -230,6 +257,12 @@ private[sbt] class ClassLoaderCache(
}
}
delegate.clear()
/* Also close loaders that were evicted from the delegate map but never closed (see
* `retired`); WeakHashMap iteration requires holding its monitor, so snapshot the keys
* first and close outside the lock. */
val evicted = retired.synchronized(new java.util.ArrayList(retired.keySet()).asScala.toList)
evicted.foreach(close)
retired.clear()
}
/**

27
main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala
View File

@ -51,4 +51,31 @@ object ClassLoaderCacheTest extends BasicTestSuite:
Predef.assert(cache.get(jarClassPath) == secondLoader)
Predef.assert(cache.get(jarClassPath) != initLoader)
test("Loaders evicted by a newer timestamp should be closed by clear()"):
IO.withTemporaryDirectory: dir =>
val entry = "leak-test-resource.txt"
val jar = dir.toPath.resolve("evicted.jar").toFile
Using.resource(new java.util.jar.JarOutputStream(new java.io.FileOutputStream(jar))): out =>
out.putNextEntry(new java.util.zip.ZipEntry(entry))
out.write("hello".getBytes("UTF-8"))
out.closeEntry()
withCache: cache =>
val classPath = jar :: Nil
val first = cache.get(classPath)
Predef.assert(first.getResource(entry) != null, "jar resource should load before eviction")
// Bumping the timestamp makes a new Key, and addLoader calls clearExpiredLoaders after
// inserting it, so `first`'s entry is evicted from the delegate map here. Once evicted
// it is unreachable from the map, so only the `retired` set can still close it.
IO.setModifiedTimeOrFalse(jar, System.currentTimeMillis + 5000L)
val second = cache.get(classPath)
Predef.assert(first != second, "a newer timestamp should produce a new loader")
cache.clear()
Predef.assert(
first.getResource(entry) == null,
"clear() should have closed the evicted loader, releasing its jar handle"
)
end ClassLoaderCacheTest

10
sbt-app/src/sbt-test/tests/multi-failure-recap/project/Checks.scala
View File

@ -39,6 +39,16 @@ object Checks {
val failedSuites = f.testOutput.get.events.values.count: s =>
s.result == sbt.protocol.testing.TestResult.Failed
assert(failedSuites >= 1, s"${f.taskName} has no failed suite: ${f.testOutput.get.events}")
// The retained output must not carry live test-thrown Throwables: their backtraces pin
// the test classloader (and its open jar handles) for as long as the recap sits on
// State.attributes, which is the whole session. The recap only renders names and counts.
f.testOutput.get.events.foreach { case (suiteName, s) =>
assert(
s.throwables.isEmpty,
s"${f.taskName} suite $suiteName retained ${s.throwables.size} throwable(s): " +
s.throwables.map(_.getClass.getName).mkString(", ")
)
}
}
val lines = TestRecap.render(recap)
assert(

55
testing/src/main/scala/sbt/JUnitXmlTestsListener.scala
View File

@ -15,6 +15,7 @@ import java.time.format.DateTimeFormatter
import java.time.temporal.ChronoUnit
import java.util.Hashtable
import java.util.concurrent.TimeUnit.NANOSECONDS
import java.util.concurrent.atomic.AtomicReference
import scala.collection.mutable.ListBuffer
import scala.util.Properties
@ -196,13 +197,31 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg
}
}
/**
* A mutable cell holding the suite that is currently running on a thread.
*
* The cell exists purely so the suite can be released deterministically. `testSuite` below
* is an [[InheritableThreadLocal]], so every thread created while a suite is running -- for
* example a pooled worker spawned by an async test framework -- receives a copy of the
* *reference* to this cell at construction time. `ThreadLocal.remove()` only clears the
* calling thread's entry, so those inherited copies would otherwise pin the `TestSuite`, its
* buffered events, and through them the test class loader (and its open jar handles) for the
* remaining life of the JVM. Clearing the cell severs the reference for the owning thread and
* every thread that inherited it at once.
*/
private final class SuiteRef(initial: Option[TestSuite]) {
private val ref = new AtomicReference(initial)
def current: Option[TestSuite] = ref.get()
def clear(): Unit = ref.set(None)
}
/** The currently running test suite */
private val testSuite = new InheritableThreadLocal[Option[TestSuite]] {
override def initialValue(): Option[TestSuite] = None
private val testSuite = new InheritableThreadLocal[SuiteRef] {
override def initialValue(): SuiteRef = new SuiteRef(None)
}
private def withTestSuite[T](f: TestSuite => T): T =
testSuite.get().map(f).getOrElse(sys.error("no test suite"))
testSuite.get().current.map(f).getOrElse(sys.error("no test suite"))
/** Creates the output Dir */
override def doInit(): Unit = {
@ -212,14 +231,29 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg
/**
* Starts a new, initially empty Suite with the given name.
*/
override def startGroup(name: String): Unit = testSuite.set(Some(new TestSuite(name)))
override def startGroup(name: String): Unit =
testSuite.set(new SuiteRef(Some(new TestSuite(name))))
/**
* Adds all details for the given even to the current suite.
*
* Events that arrive after the suite has been written are dropped. Test frameworks may call
* the event handler from threads they spawned during the run (see `TestFramework.TestRunner`),
* and such a thread can report after `writeSuite` has already emitted the XML. Before the
* suite was released those late events were appended to an already-written suite, so they
* were discarded in practice; dropping them here keeps that outcome without turning every
* late event into an error line via `TestFramework.safeForeach`.
*/
override def testEvent(event: TestEvent): Unit = for (e <- event.detail) {
withTestSuite(_.addEvent(e))
}
override def testEvent(event: TestEvent): Unit =
testSuite.get().current match {
case Some(suite) => for (e <- event.detail) suite.addEvent(e)
case None =>
if (logger != null) {
logger.debug(
s"ignoring ${event.detail.size} test event(s) reported after the suite was written"
)
} else ()
}
/**
* called for each class or equivalent grouping We map one group to one Testsuite, so for each
@ -283,6 +317,13 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg
}
val testSuiteResult = withTestSuite(_.stop())
XML.save(file, testSuiteResult, "UTF-8", xmlDecl = true, null)
/* Order matters: `clear()` releases the suite for this thread *and* for every thread that
* inherited the cell, which `remove()` cannot reach. `remove()` then drops this thread's
* own entry. Without the `clear()` the suite -- and through its buffered events the test
* class loader with its open jar handles -- would stay reachable from pooled worker threads
* for the life of the JVM.
*/
testSuite.get().clear()
testSuite.remove()
}

14
testing/src/main/scala/sbt/TestReportListener.scala
View File

@ -44,6 +44,20 @@ trait TestsListener extends TestReportListener {
/**
* Provides the overall `result` of a group of tests (a suite) and test counts for each result type.
*
* @param throwables
* The exceptions thrown by the tests in this suite, as live objects. They are needed as objects
* rather than as rendered text because the ClassLoaderLayeringStrategy diagnostic in `Defaults`
* inspects them with `isInstanceOf` and walks `getCause` to recognise `NoClassDefFoundError` and
* friends.
*
* RETENTION HAZARD: a `Throwable`'s backtrace references the `Class` objects of every frame, and
* a `Class` strongly references its defining class loader. Holding a `SuiteResult` with a
* non-empty `throwables` therefore keeps the test class loader -- and every jar handle it has
* open -- alive. That is fine for the duration of the test task, which is where these are
* consumed, but anything that outlives the task must drop them first. `TestRecap.collect` does
* exactly that before stashing a copy on `State.attributes`; see the note on `TestRecap.recapKey`.
* On Windows a leaked handle makes the underlying jar undeletable (e.g. by `clearCaches`).
*/
private[sbt] final class SuiteResult(
val result: TestResult,

50
testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala
View File

@ -88,4 +88,54 @@ object JUnitXmlTestsListenerSpec extends BasicTestSuite:
val xmlFile = new File(tempDir, "TEST-TestSuite.xml")
assert(xmlFile.exists(), "XML file should be created even when logger is null")
test("JUnitXmlTestsListener should release the suite from threads that inherited it"):
IO.withTemporaryDirectory: tempDir =>
val listener = new JUnitXmlTestsListener(tempDir, false, null)
listener.doInit()
def event(name: String) = new TEvent:
def fullyQualifiedName = s"InheritSuite.$name"
def duration() = 1L
def status = TStatus.Success
def fingerprint = null
def selector = new TestSelector(name)
def throwable = new OptionalThrowable()
listener.startGroup("InheritSuite")
// A thread created *while* the suite is set inherits the suite cell, standing in for a
// pooled worker spawned by an async test framework during the run.
val suiteWritten = new java.util.concurrent.CountDownLatch(1)
val childDone = new java.util.concurrent.CountDownLatch(1)
val endGroupOutcome = new AtomicReference[Option[Throwable]](None)
val testEventOutcome = new AtomicReference[Option[Throwable]](None)
val child = new Thread(() =>
suiteWritten.await()
// The inherited cell must no longer reach a TestSuite, so the strict path fails...
endGroupOutcome.set(
scala.util.Try(listener.endGroup("InheritSuite", TestResult.Passed)).failed.toOption
)
// ...while a late event is dropped rather than raised.
testEventOutcome.set(
scala.util.Try(listener.testEvent(sbt.TestEvent(Seq(event("late"))))).failed.toOption
)
childDone.countDown()
)
child.setDaemon(true)
child.start()
listener.testEvent(sbt.TestEvent(Seq(event("testMethod"))))
listener.endGroup("InheritSuite", TestResult.Passed)
suiteWritten.countDown()
assert(childDone.await(30, java.util.concurrent.TimeUnit.SECONDS), "child thread timed out")
assert(
endGroupOutcome.get().isDefined,
"a thread that inherited the suite could still reach it after writeSuite"
)
assert(
testEventOutcome.get().isEmpty,
s"a late test event should be dropped, but threw: ${testEventOutcome.get()}"
)
end JUnitXmlTestsListenerSpec