Fix concurrency and equals-contract CodeQL alerts

[vharseko]

Clears the remaining small concurrency and equals-contract alerts: java/unsafe-sync-on-field (#650), java/unsynchronized-getter (#651, #652), java/sleep-with-lock-held (#621, #622), java/unchecked-cast-in-equals (#599, #600, #601) and java/inconsistent-equals-and-hashcode (#578).

1. DirectoryServer synchronizes on fields it reassigns

getNewInstance() did:

synchronized (directoryServer)
{
    return directoryServer = new DirectoryServer(config);
}

The problem is wider than that one method: synchronized (directoryServer) appears in seven places — getNewInstance(), bootstrapClient(), bootstrapServer(), the end of bootstrapping, startServer(), setEntryCache() and shutDown(). Once the reference is swapped during an in-core restart, those blocks lock different monitors, so they provide no mutual exclusion at all.

All seven guard the same thing — the server life cycle flags (isRunning, isBootstrapped, shuttingDown) and the singleton wiring — so they now share a dedicated LIFECYCLE_LOCK. The directoryServer field is also made volatile, since it is read without any lock by getInstance(), getEnvironmentConfig() and many other accessors.

The lock only makes the guarded transitions atomic; it does not publish the flags to the code that reads them without holding it — isRunning(), isShuttingDown() and setEnvironmentConfig() read them unlocked and destroy() writes them unlocked. So isBootstrapped, isRunning and shuttingDown are volatile as well, along with serverLocked, which is written outside the lock on shutdown.

shutDown() also did directoryServer = getNewInstance(envConfig);, repeating outside the lock a write getNewInstance() already performs inside it. It now just calls the method, like reinitialize() does.

Three more fields were locked while being reassignable: authenticationPolicies, connectionHandlers and establishedConnections are the target of eight synchronized blocks but were replaced with fresh collections during bootstrap — the same defect as above. They are now final and cleared instead. (supportedControls and supportedFeatures were already final.)

2. DebugLogPublisher readers race with its synchronized writers

setClassSettings()/setMethodSettings() were synchronized, but getClassSettings(), getMethodSettings() and hasTraceSettings() were not, over plain HashMap/TreeMap instances created lazily. Two real hazards: the map reference is published unsafely, and a put can run concurrently with a get on a non-concurrent map.

The fields become volatile and the maps ConcurrentHashMap; removeTraceSettings() becomes synchronized like the other mutators. The readers stay lock-free: they are cheap, and DebugTracer caches their results in PublisherSettings, so there is nothing to gain from a monitor there. The nested map does not rely on ordering (only get/containsKey/remove), and DebugTracer keeps the map returned by getMethodSettings() and may iterate it while the publisher is reconfigured — which the old TreeMap did not survive — so ConcurrentHashMap replaces it.

One narrowed contract: addTraceSettings(scope, null) used to store a null value in a HashMap and now throws NullPointerException. No in-tree caller does this (TextDebugLogPublisher:81-89 guards both arguments) and every other accessor is package-private, but the javadoc now documents it.

3. Both PointAdder classes sleep holding a lock

org.opends.server.util.cli.PointAdder and Application.PointAdder are near-identical:

public synchronized void stop()
{
    stopPointAdder = true;
    while (!pointAdderStopped)
    {
        try { t.interrupt(); Thread.sleep(100); }
        catch (Throwable t) { }
    }
}

There is no deadlock here — the worker thread never takes the monitor — but the lock guards nothing (start() is not synchronized), the handshake flags are non-volatile, and catch (Throwable t) both swallows the caller's interruption and shadows the t thread field.

synchronized is dropped, the flags and the thread field become volatile, and InterruptedException is caught explicitly with the interrupt status restored.

The catch (Throwable) was also hiding a live bug. ReplicationCliMain.mergeRegistries() creates the adder at :8701 but starts it at :8747, and both a failure in createTopologyCache() (:8706) and a declined confirmation (:8735) reach the finally { pointAdder.stop(); }. With t == null the NPE was swallowed, pointAdderStopped never became true and the loop never reached Thread.sleep: declining the merge left dsreplication spinning at 100% CPU. stop() now returns early when the adder was never started.

The flag-based wait is kept rather than switching to Thread.join() on purpose: Application.PointAdder.run() sets pointAdderStopped before a final listenerDelegate.notifyListeners(...) call, so joining would make stop() wait for that notification too — a deadlock risk when stop() is called from the EDT.

4. equals() implementations that do not check the argument type

• ScheduleType.equals() was o != null && toString().equals(o.toString()), so a plain String with matching text compared equal to a schedule — and asymmetrically, since String.equals would return false the other way. Now guarded by instanceof ScheduleType.
• ProfileStack and ProfileStackFrame cast without an instanceof check and relied on catch (Exception) to handle the resulting ClassCastException, logging a stack trace via traceException on every comparison against another type. Both now check the type up front. Their logger fields became unused and were removed along with the imports.

5. SuffixDescriptor hashes and compares by id but compares equal by identity

The class overrides hashCode() and compareTo() in terms of getId(), but never overrode equals(), so equality stayed reference-based. It now compares ids.

getId() was not canonical: it iterated getReplicas(), a fresh HashSet<ReplicaDescriptor>, and ReplicaDescriptor overrides neither equals nor hashCode, so two descriptors with the same DN and the same servers could produce different ids depending on identity-hash iteration order. Now that the id is load-bearing for equality, the server ids are sorted, which makes equals, hashCode and compareTo well-defined at once.

Behaviour note: SuffixDescriptor is stored in HashSets in the installer (UserData, SuffixesToReplicateOptions, SuffixesToReplicatePanel), so two descriptors with the same id now collapse into one. An equal id means the same DN and the same set of replica servers, so they are genuine duplicates, and they were already indistinguishable elsewhere: SuffixesToReplicatePanel keys its checkboxes by getId(), and ReplicationCliMain:5233-5234 uses natural-ordering TreeSets that deduplicated via compareTo → getId() already.

Testing

• mvn -pl opendj-server-legacy test-compile — BUILD SUCCESS.
• New unit tests: SuffixDescriptorTest (id-based equality, HashSet collapse, id independent of replica order) and ScheduleTypeTest (including the ScheduleType-vs-String asymmetry) — 11 tests, all passing.
• Because the DirectoryServer changes touch bootstrap and shutdown: UnbindOperationTestCase (41 tests) and PluginConfigManagerTestCase (34 tests) pass, plus a scratch test forcing two in-core restarts through shutDown() → getNewInstance() → re-bootstrap.

[maximthomas]

Five well-scoped fixes, accurately diagnosed. Verified the risky parts: no wait()/notify() anywhere in DirectoryServer, so swapping the monitor can't break a pairing; all seven sites converted; startServer() → initializeDefaultEntryCache() → setEntryCache() re-enters LIFECYCLE_LOCK on the same thread, so it stays correct.

The SuffixDescriptor change you flagged is safe: opendj-server-legacy/src/main/java/org/opends/quicksetup/installer/ui/SuffixesToReplicatePanel.java:74,249,121 already keys checkboxes by getId(), so equal-id descriptors were already indistinguishable in the UI, and opendj-server-legacy/src/main/java/org/opends/server/tools/dsreplication/ReplicationCliMain.java:5233-5234 uses natural-ordering TreeSets that already deduplicated via compareTo → getId(). Dropping the catch (Exception) in ProfileStack.equals is safe too: decode()'s only caller (opendj-server-legacy/src/main/java/org/opends/server/plugins/profiler/ProfileViewer.java:237) NPEs earlier in ProfileStackFrame.hashCode() (:254) on a malformed capture, identically before and after this PR.

One blocker.

PointAdder.stop() NPEs when the adder was never started (high)

Moving t.interrupt() above the try exposes a live start-less path in opendj-server-legacy/src/main/java/org/opends/server/tools/dsreplication/ReplicationCliMain.java:

8701  PointAdder pointAdder = new PointAdder(this);
8702  try {
8735      throw new ReplicationCliException(ERR_REPLICATION_USER_CANCELLED.get(), USER_CANCELLED, null);
8747      pointAdder.start();
8867  } finally { pointAdder.stop(); }

Declining the confirmation prompt throws before start(), so stop() runs with t == null. Today the NPE is swallowed and the loop never reaches Thread.sleep — an infinite busy-spin, so dsreplication hangs at 100% CPU on a plain user cancel. After this PR the NPE escapes the finally and replaces the USER_CANCELLED exception. createTopologyCache(adsCtx1, cnx) at :8706 reaches the same finally.

Guard both classes:

public void stop()
{
  stopPointAdder = true;
  final Thread thread = t;
  if (thread == null)
  {
    return; // never started
  }
  while (!pointAdderStopped)
  {
    thread.interrupt();
    ...

Application.PointAdder has no such path today (all three Installer call sites pair start()/stop() under the same guard), but should match for symmetry.

PointAdder.t left non-volatile (medium)

t is written by start() and dereferenced by stop() — the same rationale used for the two flags. Leaving it plain also turns the issue above from "deterministically null" into "possibly stale null" across threads. Make it volatile; the local copy above then makes the read safe.

SuffixDescriptor.equals keys off a non-canonical, mutable id (low–medium)

getId() iterates getReplicas() (new HashSet<>(replicas)) and ReplicaDescriptor overrides neither equals nor hashCode, so ids are concatenated in identity-hash order — two descriptors with the same DN and servers but different replica instances can produce different ids. Separately, opendj-server-legacy/src/main/java/org/opends/admin/ads/TopologyCache.java:135 calls addReplica() on descriptors already in the HashSet at :69/:146, so the id changes after insertion.

Both pre-date this PR, but the new equals makes the id load-bearing for set membership. Sorting makes it canonical and fixes equals, hashCode and compareTo at once:

Set<String> ids = new TreeSet<>();
for (ReplicaDescriptor replica : getReplicas()) { ids.add(replica.getServer().getId()); }

LIFECYCLE_LOCK javadoc overstates what is guarded (low)

The flags it names are plain non-volatile fields, still read unlocked in isRunning() (opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:1123), isShuttingDown() (:4708) and setEnvironmentConfig() (:1096), and written unlocked in destroy() (:4339-4344). A volatile reference doesn't publish later writes to those fields. Either mark the three flags volatile — cheap, and it genuinely closes java/unsynchronized-getter here — or soften the comment to "the singleton wiring and the guarded transitions".

ConcurrentHashMap is null-hostile (low)

addTraceSettings is public on a @PublicAPI(mayExtend=true) class. addTraceSettings(scope, null), getClassSettings(null) and hasTraceSettings(null) were tolerated by HashMap/TreeMap and now throw NPE. No in-tree caller does this (opendj-server-legacy/src/main/java/org/opends/server/loggers/TextDebugLogPublisher.java:81-89 guards both args), but an external subclass could. A @throws NullPointerException note would document the narrowed contract.

Nits

• Redundant reassignment: opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:4331 does directoryServer = getNewInstance(envConfig);, but getNewInstance() already assigns under the lock — the outer write repeats it outside it. Just call the method.
• Leftover blank lines: removing the logger fields left four blank lines after { in opendj-server-legacy/src/main/java/org/opends/server/plugins/profiler/ProfileStack.java:31-34 and .../ProfileStackFrame.java:37-40, plus a gap at ProfileStackFrame.java:24-26.
• Same alert class elsewhere (follow-up): synchronized (directoryServer.authenticationPolicies) (DirectoryServer.java:2478,2509,2531,2548), .connectionHandlers (:3436,3455), .establishedConnections (:4448,4484) all lock non-final fields reassigned at :1155, :1162, :1217 — the same bug just fixed for directoryServer. (supportedControls/supportedFeatures are final, so those are fine.) Better as a follow-up issue.
• Hot-path rationale: getClassSettings/getMethodSettings are only called from DebugTracer.PublisherSettings's constructor (opendj-server-legacy/src/main/java/org/opends/server/loggers/DebugTracer.java:57-58) and updateSettings() (:212); results are cached, so a monitor there wouldn't be paid per traced call. Change is still right, the justification is just stronger than needed.
• No tests: nothing in the repo references any of the five touched classes. One case pinning SuffixDescriptor id-equality plus HashSet collapse, and one for the ScheduleType-vs-String asymmetry, would document the only behavioural change here.

[vharseko]

Thanks — all eight points are addressed in 9bcd4ad, including the two you offered to defer.

PointAdder.stop() NPE — fixed

Confirmed the path you found: mergeRegistries() creates the adder at :8701 and starts it at :8747, so both the declined confirmation at :8735 and a failure in createTopologyCache() at :8706 reach the finally at :8867 with t == null. Worth spelling out how bad the pre-PR behaviour was: the NPE was caught by catch (Throwable), pointAdderStopped stayed false forever, and the loop never reached Thread.sleep — so declining the merge left dsreplication spinning at 100% CPU. Both classes now take a local copy and return early. Checked the other four call sites (ReplicationCliMain:4863,4903, LocalPurgeHistorical:88, and all three in Installer); they all pair start()/stop(), as you said.

t is volatile now too.

SuffixDescriptor.getId() — sorted

Done, TreeSet<String> over replica.getServer().getId(). Agreed on the diagnosis: ReplicaDescriptor overrides neither equals nor hashCode, so the id was ordered by identity hash. Note this made the new dedup unreliable rather than dangerous — hashCode() was already computed from getId() before this PR, so the TopologyCache:135 mutation-after-insertion problem predates it and is unchanged. The sort makes equals, hashCode and compareTo all well-defined, which is what the new equals needed.

Life cycle flags — made volatile

Took the first option: isBootstrapped, isRunning and shuttingDown are volatile, and I added serverLocked, which is written outside the lock at :4304 and in main(). Javadoc on LIFECYCLE_LOCK now says explicitly that the lock makes the transitions atomic and the flags are volatile because they are read without it.

ConcurrentHashMap null-hostility — documented

@throws NullPointerException added to addTraceSettings. Two refinements while checking the blast radius: getClassSettings, getMethodSettings, hasTraceSettings and removeTraceSettings are all package-private, so addTraceSettings(scope, null) is the only narrowing an external subclass can hit; and hasTraceSettings(null) already threw NPE on scope.lastIndexOf('#') before this PR, so it isn't a regression.

Nits — all fixed

• :4331 — this one was slightly worse than a nit: the synchronized (LIFECYCLE_LOCK) block in shutDown() ends at :4106, so that second write happened outside the lock and could clobber a newer instance. Now just getNewInstance(envConfig);, matching reinitialize() at :4444.
• Blank lines removed in both profiler classes.
• Same alert class elsewhere — pulled into this PR rather than a follow-up, since the fix turned out mechanical: authenticationPolicies, connectionHandlers and establishedConnections are now final and clear()ed during bootstrap instead of reassigned. That is 8 synchronized blocks that no longer lock a swappable monitor. Confirmed supportedControls/supportedFeatures were already final.
• Hot-path rationale — corrected. DebugTracer caches the settings in PublisherSettings (:57-58, :212), so the getters are not on the per-call tracing path. Also documented why the nested map is concurrent: DebugTracer holds the map returned by getMethodSettings() and can iterate it while the publisher is being reconfigured, which the old TreeMap did not survive.
• Tests — added SuffixDescriptorTest (id equality, HashSet collapse, and a check that the id is independent of replica order) and ScheduleTypeTest (including the ScheduleType-vs-String asymmetry).

One residual worth naming

Dropping catch (Exception) from ProfileStack.equals also drops the accidental guard against ArrayIndexOutOfBoundsException: the private constructor sets numFrames = classNames.length while the lengths of methodNames/lineNumbers come from the decoder independently, so a truncated capture used to yield false and now propagates. Your argument still holds — ProfileViewer NPEs earlier on such a capture — so I left it, but the mechanism is a bit different from a plain ClassCastException.

Verification

mvn -pl opendj-server-legacy test-compile — BUILD SUCCESS. The two new test classes pass (11 tests). Since the DirectoryServer changes touch bootstrap and shutdown, I also ran UnbindOperationTestCase (41 tests) and PluginConfigManagerTestCase (34 tests) green, plus a scratch test forcing two in-core restarts through shutDown() → getNewInstance() → re-bootstrap, which is exactly the path the final collections and the removed reassignment run over.