Fix CodeQL warning-severity alerts: missed wakeups, resource leaks, escaping threads

[vharseko]

Tier 2 of the warning-severity CodeQL triage (Tier 1 is #789). Of the 73 alerts across these four rules, 66 are fixed in code; the remaining 7 are dismissed in code scanning as won't fix / false positive, each with the per-alert reason repeated below.

java/notify-instead-of-notify-all — 25 of 25

Each site was checked individually: every waiter re-checks its condition in a loop, so notifyAll() is safe everywhere. The conversion fixes genuine missed wakeups where more than one thread can wait on the same monitor:

• NodeSearcherQueue (3 sites) — BrowserController creates the queue with two consumer threads, so waking only one could leave queued work unattended.
• ReplicationBroker.connectPhaseLock (2 sites) — any thread that called publish() can be waiting there.
• ReplicationBroker.monitorResponse — several monitor requests can be outstanding.

The remaining sites have a single waiter by construction (TaskThread, ChangeNumberIndexer, the FileChangelogDB purger, ServerStateFlush, …) and are converted for consistency. MessageHandler.shutdown() called notify() immediately before an existing notifyAll(); the redundant call is removed.

The six waitListen conversions in the LDAP / HTTP / reactive connection handlers belong to that second group rather than to the missed-wakeup list: LDAPConnectionHandler and HTTPConnectionHandler have exactly one waiter (the server startup thread), and LDAPConnectionHandler2 has none at all — its waitListen monitor was copied from its legacy sibling without the waiting half, which is tracked separately in #794.

Two related defects found while checking those sites are fixed here:

• InitializeTask synchronized on initState, a reassigned TaskState field. runTask() locks the monitor of TaskState.RUNNING and waits on it, while updateTaskCompletionState() reassigns the field first and then locks the monitor of COMPLETED_SUCCESSFULLY / STOPPED_BY_ERROR — so the notification could never reach the waiter and completion was detected only by the 1-second poll. Reassignment between the loop condition and initState.wait(1000) could also raise IllegalMonitorStateException, and the monitor was a JVM-wide enum constant. Replaced by a dedicated final Object lock plus a volatile state field.
• HTTPConnectionHandler.start() waited on waitListen with no condition variable and no loop, so a spurious wakeup let startup return before the listener was confirmed. It now uses the same while (!listenAttempted) guard as LDAPConnectionHandler.start(), and restores the interrupt flag like its sibling does.

java/input-resource-leak + java/output-resource-leak — 26 of 31

• Never closed at all: GenerateGlobalAcisTableMojo (2), quicksetup/Utils.supportsOption, ConfigureWindowsService (2), and the Server / ModifyAsync examples.
• Handed to a background reader thread and then dropped: ServerController.StopReader / StartReader (4) and OutputReader (2). The reader is now closed by the same thread that drains it.
• ZipExtractor validated the .zip extension after delegating to the constructor that opens the file, so the stream leaked whenever validation failed. The name is now checked before the stream is opened.
• Streams that leaked only if the wrapping constructor threw are given their own try-with-resources slot or closed explicitly: ConfigurationHandler, UpgradeUtils (2), OpenDJProvider, ProfileViewer, LDIFImportConfig (2, the GZIP wrap), ID2Entry, Base64, BuildInformation, LogFile.

ID2Entry deserves a note: reader.asInputStream() is an in-memory stream whose close() is a no-op, so no file descriptor is at stake. The gain is that InflaterInputStream.close() calls Inflater.end(), releasing native zlib memory eagerly instead of waiting for the finalizer.

Base64 is a partial fix by design: the alert covers a ternary that yields either a FileReader or an InputStreamReader(System.in). Only the file branch is closed — closing the console input would break the tool — and that is enough to clear the alert.

Five alerts are dismissed, because closing would be wrong or the resource is already released elsewhere:

Alert	Why it stays
InstallDS:455	wraps System.in — closing the console input would break the tool
DirectoryServer:4998	server.out is held for the lifetime of the process and installed via System.setOut
TempLogFile:84	released through writer.shutdown()
ReplicationDomain:1563, :2328	released through LDIFExportConfig.close() / LDIFImportConfig.close()

java/non-sync-override — 8 of 10

SearchOperationBasis.abort overrode a synchronized method and mutated cancelRequest without holding the lock — that one is a real defect. RecordingInputStream.mark/reset, MissingMandatoryPropertiesException.getCause and four Swing methods are made synchronized to match the contract they override.

LDAPConnectionHandler.start and HTTPConnectionHandler.start are left unsynchronized on purpose. The state they touch is guarded by the waitListen monitor and the already-started check belongs to super.start(), so synchronized would add no safety — while holding the thread's own monitor across waitListen.wait() would block Thread.join() on the handler. Trading real risk for no benefit, so these two are won't-fix.

java/thread-start-in-constructor — 7 of 7

Thread.start() is moved out of the constructor into an explicit start() method in OutputReader, NodeSearcherQueue, AsynchronousTextWriter, MultifileTextWriter, ReplicationServerDomain and DynamicGroupMemberList, so this is no longer published to the thread before construction completes. All 19 construction sites are updated.

MakeLDIFInputStream goes one step further: its constructor is private and the only way to obtain an instance is the static newStartedInputStream() factory, which constructs and then starts. Two-phase construction is easy to get wrong — new X(...) without .start() compiles fine and fails silently at runtime — and the factory makes that mistake unrepresentable. It also keeps java/input-resource-leak quiet: the stream escapes through a return, whereas new MakeLDIFInputStream(f).start() inside a this(...) delegation looked like a leaked resource to CodeQL.

Side benefit: the rotater thread of MultifileTextWriter now starts after the rotation and retention policies are registered, instead of possibly running before they were added.

Verification

• mvn -o compile across all six touched modules — passes.
• mvn -o test-compile for opendj-server-legacy — passes.
• A missed .start() in the last group would compile silently, so that was checked separately: every construction site calls .start() (or the factory), and each refactored class starts its thread in exactly one place.
• No unit-test run: opendj-server-legacy disables surefire and runs tests through failsafe against a real server, so the affected classes are not covered by the plain test route.

Note for reviewers

This branch is based on origin/master, so AsynchronousTextWriter.java is touched by both this PR and #789 (#789 changes the constructor, this PR removes writerThread.start() from it). The edits are on adjacent lines and should merge cleanly, but merging #789 first is the simpler order.

[maximthomas]

Solid triage — the concurrency reasoning is site-specific and the two start() won't-fixes are correct for non-obvious reasons. I verified the risky parts independently: all 19 .start() sites are present, none of the 7 refactored classes has a subclass that could bypass new X via super(...), all 14 affected monitors have waiters that re-check their condition (so notifyAll is safe everywhere), and #789 merges cleanly (git merge-tree → no conflict, both changes survive).

Three things to address, all in code this PR already touches.

Synchronization on a reassigned field in InitializeTask (medium)

opendj-server-legacy/src/main/java/org/opends/server/tasks/InitializeTask.java — initState is a reassigned TaskState enum field used as a monitor, so the notifyAll() this PR converts can never reach the waiter.

// :107  runTask() locks the monitor of TaskState.RUNNING
synchronized (initState) {
  while (initState == TaskState.RUNNING) {
    initState.wait(1000);           // :112

// :147-164  updateTaskCompletionState() reassigns FIRST, then locks a DIFFERENT monitor
initState = TaskState.STOPPED_BY_ERROR;
...
initState = TaskState.COMPLETED_SUCCESSFULLY;
...
synchronized (initState) {          // :162  -> monitor of COMPLETED_SUCCESSFULLY
  initState.notifyAll();            // :164  waiter is parked on RUNNING; never woken
}

Consequences:

• The notification is unreachable — completion is detected only by the 1-second poll.
• If the field is reassigned between the while check (:110) and initState.wait(1000) (:112), the wait targets a monitor the thread does not hold → IllegalMonitorStateException, unchecked, so it escapes runTask()'s InterruptedException/DirectoryException handlers.
• TaskState.RUNNING is a JVM-wide enum constant, so this locks a globally shared object.

Fix: a dedicated private final Object lock plus a volatile TaskState initState.

waitListen has no waiters in LDAPConnectionHandler2 (low-medium)

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java — 5 occurrences of waitListen: one declaration (:191), two synchronized, two notifyAll() (:705, :723). There is no wait() anywhere in the file and no start() override.

Two of the 25 conversions notify a monitor nobody waits on. The substantive point: unlike its LDAP sibling, the reactive handler never waits for its listener to come up, so server startup can return before the port is open. The monitor looks copy-pasted from LDAPConnectionHandler. Either wire up the wait or drop the vestigial monitor.

Unguarded wait() in HTTPConnectionHandler.start() (low)

opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java:577

synchronized (waitListen) {
  super.start();
  try {
    waitListen.wait();   // :583  no condition variable, no loop

Compare opendj-server-legacy/src/main/java/org/opends/server/protocols/ldap/LDAPConnectionHandler.java:826:

while (!listenAttempted) {
  waitListen.wait();
}

The lost-wakeup race is covered (the monitor is held across super.start()), so this is spurious-wakeup exposure only — start() can return before the listener is confirmed. Worth giving it the same guard as its sibling while you're here.

Nits

• Duplicate copyright line: opendj-server-legacy/src/main/java/org/opends/server/config/ConfigurationHandler.java:15-16 now carries two 3A Systems lines (2025 3A Systems,LLC + 2026 3A Systems, LLC.) — the only such file in the tree. Repo precedent uses ranges (SearchOperationBasis.java has 2024-2026, ReplicationDomain.java has 2025-2026), so collapse to Portions Copyright 2025-2026 3A Systems, LLC.
• ProfileViewer comment overstates the fix: opendj-server-legacy/src/main/java/org/opends/server/plugins/profiler/ProfileViewer.java:218-221 — the comment says the stream is closed "in case the reader could not be created around it", but new FileInputStream and ASN1.getReader are both outside the try, so that path is still uncovered. In practice nothing leaks either way: ASN1.getReader declares no checked exceptions, and ASN1InputStreamReader.close() already calls in.close() on the parent stream — so the added fileStream at :280 is redundant. Reword the comment or use try-with-resources.
• Fully-qualified call: opendj-server-legacy/src/main/java/org/opends/server/tools/upgrade/UpgradeUtils.java:317 uses org.forgerock.util.Utils.closeSilently(...) but there is no name clash in the file (only SchemaUtils is imported). Add the import, or use StaticUtils.close as the rest of the PR does.
• Orphaned comment: opendj-server-legacy/src/main/java/org/opends/server/extensions/DynamicGroupMemberList.java:129 — // Assigned at the end of this constructor and started separately by start(). sits ~170 lines from the assignment it describes. The field javadoc (:65) and start() javadoc (:312) already cover it; delete.
• Dead null check: opendj-server-legacy/src/main/java/org/opends/server/loggers/AsynchronousTextWriter.java:233 — removing this.writerThread = null; from the constructor leaves writerThread != null permanently true. Make the field (:45) final and drop the check. (Unstarted threads are otherwise safe here — isAlive() is false, so neither this nor MultifileTextWriter.processServerShutdown hangs if start() is skipped.)
• PR description accuracy: the waitListen conversions are listed under "fixes genuine missed wakeups where more than one thread can wait on the same monitor", but LDAP and HTTP have a single waiter and the reactive handler has none. The count is 6 sites, not 5. The other three claims do check out — NodeSearcherQueue (2 real consumer threads), connectPhaseLock (the wait at ReplicationBroker.java:2372 is inside publish(), called concurrently by many threads), and monitorResponse.
• ID2Entry rationale: reader.asInputStream() is an in-memory stream whose close() is a no-op, so this is not a file-descriptor fix — the real gain is InflaterInputStream.close() calling inf.end(), releasing native zlib memory eagerly instead of at GC. Worth stating, since it's a genuine win that the rule name obscures. close() cannot realistically throw here (CipherInputStream.close() swallows crypto exceptions), so masking a decoded entry isn't a practical concern.
• Two-phase construction is easy to forget: new X(...).start() compiles fine without .start(), and the failure is silent — a hung DynamicGroupMemberList/MakeLDIFInputStream, or a log that never rotates. All 19 current sites are correct, but a private constructor plus static X create(...) would make the mistake unrepresentable for future callers. Also, start() is not idempotent (second call throws IllegalThreadStateException) — worth a @throws note.

[vharseko]

Thanks for the depth here — the independent checks on the .start() sites and the monitor waiters are exactly what this PR needed, and all three findings hold up. Addressed in c365c99 (opendj-server-legacy main + test compile clean).

InitializeTask — fixed

Confirmed the diagnosis before changing anything: runTask() locks the monitor of TaskState.RUNNING at :107 and the waiter parks on it, while updateTaskCompletionState() reassigns the field at :147/:152 and only then locks :162 — a different enum constant's monitor. The notifyAll() was unreachable and completion was detected purely by the 1-second poll, exactly as you describe. The IllegalMonitorStateException window between the loop condition and initState.wait(1000) is real too, since the field is re-read at the wait.

Fixed as suggested: private final Object initStateLock for the monitor, private volatile TaskState initState for the state. That also closes the visibility hole — the field was published across threads without any barrier on the polling path.

LDAPConnectionHandler2 — filed as #794, not fixed here

You are right that nothing waits on that monitor: no wait() in the file, no start() override, and the field javadoc at :191 plus the comments at :700 and :719 all describe a waiting side that was never copied over from the legacy handler.

One thing worth adding to your write-up: this is not a dormant class. resource/config/config.ldif wires both the LDAP (:316) and LDAPS (:340) connection handlers to org.forgerock.opendj.reactive.LDAPConnectionHandler2; the legacy org.opends.server.protocols.ldap.LDAPConnectionHandler is not referenced by the default configuration at all. So the handler that skips the startup barrier is the one actually in use, and HTTPConnectionHandler waiting while LDAP does not is an accidental asymmetry rather than a design choice.

That makes it a behaviour change to server startup ordering, which does not belong in a static-analysis cleanup, so it is tracked separately as #794 with the full analysis and the suggested listenAttempted + start() override fix. The two notifyAll() conversions stay: notifying a monitor with no waiters is harmless today and correct once the waiting side exists.

HTTPConnectionHandler.start() — fixed

Now mirrors LDAPConnectionHandler: a listenAttempted flag guarded by waitListen, set under the monitor next to both existing notifyAll() calls (the disabled-connector path and the about-to-listen path), and while (!listenAttempted) waitListen.wait(); in start().

Slightly beyond what you asked: I also restored the interrupt flag in the catch, since the sibling does it and swallowing the interrupt silently on a startup path is its own small bug. Say the word if you would rather keep that diff minimal.

Nits

• Duplicate copyright — collapsed to Portions Copyright 2025-2026 3A Systems, LLC., matching the SearchOperationBasis / ReplicationDomain precedent.

• ProfileViewer — you were right that the comment promised more than the code delivered. Rather than reword it, both resources now live in a try-with-resources head, so the file stream really is closed if ASN1.getReader fails. The explicit close(reader, fileStream) in the finally is gone; agreed the fileStream argument was redundant given ASN1InputStreamReader.close() closes its parent, but the try-with-resources form does not depend on that implementation detail staying true.

• Fully-qualified call — org.forgerock.util.Utils is now imported; no clash, the file only imported SchemaUtils.

• Orphaned comment — deleted; the field javadoc and start() javadoc already say it.

• Dead null check — AsynchronousTextWriter.writerThread is final and the != null guard is gone. Verified the field has exactly one assignment.

• PR description — corrected. The six waitListen conversions moved out of the missed-wakeup list into the single-waiter group, with the reactive handler's zero waiters called out and pointed at LDAPConnectionHandler2 never waits for its listener: server startup can return before the LDAP port is open #794. Also added the ID2Entry rationale you supplied: no file descriptor is at stake, the gain is Inflater.end() releasing native zlib memory eagerly.

• Two-phase construction — applied to MakeLDIFInputStream: the constructor is private and newStartedInputStream() is the only entry point, so forgetting .start() is no longer expressible. That also cleared the one alert this PR had introduced — CodeQL flagged new MakeLDIFInputStream(f).start() inside the this(...) delegation at LDIFImportConfig:207 as a leaked resource (java/input-resource-leak, alert 1229); a resource returned from a factory does not trip the rule.

  I did not extend the pattern to the other six. MultifileTextWriter genuinely needs the gap — the rotation and retention policies are registered between construction and start(), which is what fixed the ordering bug in the first place — and the rest are single-expression new X(...).start() call sites where a factory buys less. Happy to convert them if you would rather have it uniform.

  The @throws IllegalThreadStateException note is the one item I left out: every current site calls start() exactly once and the javadoc already spells out the two-phase contract. Tell me if you want it documented anyway.