Flaky test on Windows: EmbeddedOpenDJTest#testOpenDJ — EmbeddedOpenDJ.close() silently leaves its temporary directory behind

[vharseko]

EmbeddedOpenDJTest#testOpenDJ failed on windows-latest / Java 26 while every other job of the same run — including windows-latest / Java 11 — was green:

[ERROR] EmbeddedOpenDJTest.testOpenDJ:91 expected [false] but found [true]
java.lang.AssertionError: expected [false] but found [true]
	at org.testng.Assert.assertFalse(Assert.java:75)
	at org.openidentityplatform.opendj.embedded.EmbeddedOpenDJTest.testOpenDJ(EmbeddedOpenDJTest.java:91)

Run: https://github.com/OpenIdentityPlatform/OpenDJ/actions/runs/30554186279/job/90910350398 (observed on PR #793, whose diff does not touch opendj-embedded; the same job passed on the previous commit of that PR and on PRs #790, #791, #795).

What actually fails

Everything the test exercises works — the server starts, the LDIF import, the export, the bind and the search all succeed, and the shutdown completes cleanly:

15:05:16.685 [main] INFO  o.o.s.backends.pluggable.BackendImpl - The backend userRoot is now taken offline
15:05:16.692 [main] INFO  o.opends.server.core.DirectoryServer - The Directory Server is now stopped

The only failing step is the assertion added in #763, which checks that the per-instance temporary directory is gone after close():

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-embedded/src/test/java/org/openidentityplatform/opendj/embedded/EmbeddedOpenDJTest.java#L86-L92

//stop OpenDJ
embeddedOpenDJ.close();
assertFalse(embeddedOpenDJ.isRunning());

//the per-instance temporary directory is deleted on close
assertFalse(serverRoot.exists());
assertFalse(serverRoot.getParentFile().exists());

Cause

EmbeddedOpenDJ.close() deletes the instance directory with a single best-effort call, immediately after server.stop() returns:

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-embedded/src/main/java/org/openidentityplatform/opendj/embedded/EmbeddedOpenDJ.java#L166-L178

@Override
public void close()  {
    if (server.isRunning()) {
        ...
        server.stop(...);
    }
    // close() is also registered as a shutdown hook, so deletion must stay idempotent
    FileUtils.deleteQuietly(instanceDirectory);
}

On Windows a file cannot be deleted while any handle to it is still open, and server.stop() returning does not guarantee that every handle — backend files, the JSON audit event handler, server.out, the extracted opendj.zip — has already been released; some are closed by background threads shortly afterwards. When that race is lost, deleteQuietly fails, swallows the exception, and the directory survives. POSIX platforms are unaffected because unlinking an open file is allowed there, which is why only the Windows jobs can ever see this.

So the assertion is not wrong — it reports a real, Windows-only defect that is normally invisible:

• every embedded instance can leak its whole temporary directory (an extracted server root, backend data included) into %TEMP% on Windows;
• close() gives the caller no indication that the cleanup failed, since deleteQuietly discards the error.

Suggested fix

In EmbeddedOpenDJ.close(), retry the deletion for a short bounded period instead of attempting it once, so that handles released just after shutdown are still picked up; log a warning if the directory is still there when the retries are exhausted, and register the leftovers for deletion on JVM exit as a last resort. Keeping the operation idempotent matters — close() is also registered as a shutdown hook.

With that in place the existing assertions in EmbeddedOpenDJTest become deterministic on Windows and can stay as they are.

[vharseko]

Additional analysis

Digging into the code confirms the diagnosis and makes it stronger: "server.stop() returning does not guarantee that every handle has been released" is not an occasional accident in this setup — in embedded mode it is the documented-by-construction behaviour.

Why stop() never waits for anything in embedded mode

EmbeddedDirectoryServer.createEnvironmentConfig() forces every server thread to be a daemon thread:

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-server-legacy/src/main/java/org/forgerock/opendj/server/embedded/EmbeddedDirectoryServer.java#L545

env.setForceDaemonThreads(true);

which DirectoryThread applies to every thread it creates:

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-server-legacy/src/main/java/org/opends/server/api/DirectoryThread.java#L274-L276

And ServerShutdownMonitor — the component DirectoryServer.shutDown() relies on to make sure "everything goes smoothly" — only ever waits for non-daemon threads:

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-server-legacy/src/main/java/org/opends/server/core/ServerShutdownMonitor.java#L79

if (t.isAlive() && !t.isDaemon() && t.getId() != currentThreadID)
{
  threadList.add(t);
}

So in embedded mode threadList is always empty, run() returns on its first check, and shutdownMonitor.waitForMonitor() at the end of DirectoryServer.shutDown() returns immediately.

Consequence: only the work that happens on the calling stack is guaranteed to be finished when stop() returns — shutdownLocalBackends(), removeAllLogPublishers(), LockFileManager.releaseLock(). Anything finished off by a background thread is still in flight. The JSON access log publisher is a concrete example: it is enabled by default in the shipped config.ldif

https://github.com/OpenIdentityPlatform/OpenDJ/blob/master/opendj-server-legacy/resource/config/config.ldif#L808-L819

and its JsonFileWriter closes the underlying FileChannel from its own audit-json-* scheduler thread.

That is the mechanism behind the Windows-only flake, and it also means no amount of tweaking the shutdown order in EmbeddedOpenDJ can fix it — the wait has to happen on the caller's side.

This is a regression introduced together with the assertion

Before #763 the instance directory was the fixed {java.io.tmpdir}/opendj and it was deleted at start-up, in the constructor:

File rootDirectory = new File(tempDirectory, "opendj");
if (rootDirectory.exists()) {
    FileUtils.deleteDirectory(rootDirectory);
}

Deleting at start-up always succeeds, because at that moment nothing holds a handle on the leftovers. #763 moved the deletion into close() — where it is inherently racy — and asserted on it in the same change. The per-instance temp directory itself is the right call and should stay; only the deletion needs to become tolerant.

deleteQuietly leaves a partially deleted tree

Worth spelling out, because it is worse than "the directory survives": FileUtils.deleteQuietly(dir) is cleanDirectory(dir) followed by dir.delete(), and cleanDirectory() throws on the first entry it cannot remove and stops walking. On a lost race the leftover in %TEMP% is therefore a half-destroyed server root, not an intact one — useless for post-mortem and still occupying disk.

Related defects found in the same area

While reviewing EmbeddedOpenDJ/EmbeddedOpenDJTest for this issue:

EmbeddedOpenDJ

• The catch block of the constructor uses the same single-shot FileUtils.deleteQuietly(instanceDirectory). That path is worse than close(): setup() may already have started and stopped a server, so on Windows a failed initialisation leaks the whole directory silently.
• Runtime.getRuntime().addShutdownHook(new Thread(this::close)) is never unregistered, so the hook keeps a strong reference to the instance (and to its EmbeddedDirectoryServer) for the lifetime of the JVM; hooks accumulate across create/close cycles.
• close() is neither idempotent-by-flag nor thread-safe — an explicit close() and the shutdown hook can enter it concurrently, and after close() the object still accepts run() on a deleted directory.
• opendj.zip is copied into the server root and never removed after extractArchiveForSetup(), so every instance keeps a redundant full copy of the distribution in %TEMP% — and one more file to delete on close.
• copyFilesFromJar(), run(), importData() and getData() close their streams / ManagementContext / internal Connection outside finally, so any failure leaks them. In importData() the throw new RuntimeException("no records were imported") happens before connection.close().
• isRunning() delegates to the static DirectoryServer.isRunning(), i.e. it reports a JVM-global state rather than this instance's.

EmbeddedOpenDJTest

• The test body has no try/finally, so a failure anywhere before close() leaves the server running for the rest of the JVM and the temp directory behind — on a static DirectoryServer that poisons every later test in the same fork.
• The search part asserts nothing: the result of reader.readEntry() is never checked and entry.getAllAttributes() is a no-op call; the ConnectionEntryReader is not closed.
• Host/port/password are hard-coded instead of taken from Config, so overriding org.openidentityplatform.opendj.embedded.port breaks the test.
• The LDIF fixture is round-tripped through the platform default charset (new String(bytes) then getBytes(UTF_8)), which corrupts non-ASCII on a Windows runner; bos.toString() has the same problem.
• The assertion that fails gives no diagnostics — it does not say which file survived, which is exactly what one needs from a Windows CI job.

Fix being implemented

1. A single retry-with-deadline deletion helper used both by close() and by the constructor's catch: retry for a bounded period, then logger.warn with the paths that survived, and register the leftovers for deletion on JVM exit as a last resort. Idempotent, so it stays safe as a shutdown hook.
2. Delete opendj.zip right after extraction; unregister the shutdown hook in close(); make close() idempotent.
3. try-with-resources / finally for the streams, ManagementContext and internal connections listed above.
4. EmbeddedOpenDJTest: wrap the body in try/finally, read the fixture without the charset round-trip, take host/port/password from Config, make the search assertion meaningful, report the surviving files in the assertion message, and add a case for a repeated close().

The existing assertions on the temporary directory stay as they are — with (1) in place they become deterministic on Windows.