From fd75cd794b0c91aa3b421c651f15e73f7b05039e Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 10 Jul 2026 23:09:31 +0300 Subject: [PATCH 1/2] [#730] Fix import/export context leak on failed initializeRemote validation ReplicationDomain.initializeRemote() acquired the import/export context before validating the request: when validation failed ("no remotes" for an initialize-all, or the target replica unknown to the domain - e.g. an InitializeTarget task racing topology propagation), the DirectoryException escaped without releasing the context. ieRunning() then stayed true forever and the domain rejected every subsequent total update as a simultaneous import/export until restart. The release site even carried an upstream "FIXME should not this be in a finally?". Validate the request before acquiring the context, and release the context in a finally around the extracted export body, resolving the FIXME. Also fix InitOnLineTest, where this leak cascaded over the whole class on CI (3 of the last tests failing in afterTest on the ubuntu-latest/11 leg of PR 714's run): afterTest asserted ieRunning() before any cleanup, leaking the domain config, brokers and three replication servers into the following tests. Assert after the cleanup instead, wait for the target replica to appear in the topology before scheduling InitializeTarget tasks, and add initializeTargetUnknownRemote which reproduces the leak deterministically - verified failing without the product fix and green with it (full class 10/10). --- .../service/ReplicationDomain.java | 58 +++++++++++---- .../server/replication/InitOnLineTest.java | 72 ++++++++++++++++++- 2 files changed, 115 insertions(+), 15 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java index 8781e700bf..a63f74a2cc 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/replication/service/ReplicationDomain.java @@ -1442,8 +1442,6 @@ protected void initializeRemote(int serverToInitialize, int serverRunningTheTask, Task initTask, int initWindow) throws DirectoryException { - final ImportExportContext ieCtx = acquireIEContext(false); - /* We manage the list of servers to initialize in order : - to test at the end that all expected servers have reconnected @@ -1451,7 +1449,12 @@ protected void initializeRemote(int serverToInitialize, - to update the task with the server(s) where this test failed */ - Map replicaInfos = getReplicaInfos(); + // Validate the request before acquiring the import/export context: a + // validation failure must not leave the context acquired, otherwise + // ieRunning() would remain true forever and the domain would reject every + // subsequent total update as a simultaneous import/export. + final Map replicaInfos = getReplicaInfos(); + final DSInfo targetDsi; if (serverToInitialize == RoutableMsg.ALL_SERVERS) { if (replicaInfos.isEmpty()) @@ -1459,7 +1462,44 @@ protected void initializeRemote(int serverToInitialize, throw new DirectoryException(UNWILLING_TO_PERFORM, ERR_FULL_UPDATE_NO_REMOTES.get(getBaseDN(), getServerId())); } + targetDsi = null; + } + else + { + targetDsi = getDsInfoOrNull(replicaInfos.values(), serverToInitialize); + if (targetDsi == null) + { + throw new DirectoryException(UNWILLING_TO_PERFORM, + ERR_FULL_UPDATE_MISSING_REMOTE.get(getBaseDN(), getServerId(), serverToInitialize)); + } + } + final ImportExportContext ieCtx = acquireIEContext(false); + try + { + initializeRemote(ieCtx, replicaInfos, targetDsi, serverToInitialize, + serverRunningTheTask, initTask, initWindow); + } + finally + { + // Release the context whatever the outcome, otherwise ieRunning() would + // remain true forever (resolves the historical "FIXME should not this + // be in a finally?"). + releaseIEContext(); + } + } + + /** + * Performs the remote initialization with the import/export context already + * acquired - and released - by the caller. + */ + private void initializeRemote(ImportExportContext ieCtx, + Map replicaInfos, DSInfo targetDsi, + int serverToInitialize, int serverRunningTheTask, Task initTask, + int initWindow) throws DirectoryException + { + if (serverToInitialize == RoutableMsg.ALL_SERVERS) + { logger.info(NOTE_FULL_UPDATE_ENGAGED_FOR_REMOTE_START_ALL, countEntries(), getBaseDN(), getServerId()); @@ -1475,18 +1515,11 @@ protected void initializeRemote(int serverToInitialize, } else { - DSInfo dsi = getDsInfoOrNull(replicaInfos.values(), serverToInitialize); - if (dsi == null) - { - throw new DirectoryException(UNWILLING_TO_PERFORM, - ERR_FULL_UPDATE_MISSING_REMOTE.get(getBaseDN(), getServerId(), serverToInitialize)); - } - logger.info(NOTE_FULL_UPDATE_ENGAGED_FOR_REMOTE_START, countEntries(), getBaseDN(), getServerId(), serverToInitialize); ieCtx.startList.add(serverToInitialize); - ieCtx.setAckVal(dsi.getDsId(), 0); + ieCtx.setAckVal(targetDsi.getDsId(), 0); } DirectoryException exportRootException = null; @@ -1625,9 +1658,6 @@ protected void initializeRemote(int serverToInitialize, ERR_INIT_NO_SUCCESS_END_FROM_SERVERS.get(getGenerationID(), ieCtx.failureList)); } - // Don't forget to release IEcontext acquired at beginning. - releaseIEContext(); // FIXME should not this be in a finally? - final String cause = exportRootException == null ? "" : exportRootException.getLocalizedMessage(); if (serverToInitialize == RoutableMsg.ALL_SERVERS) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/InitOnLineTest.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/InitOnLineTest.java index e2d0836b85..64f16958f5 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/replication/InitOnLineTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/InitOnLineTest.java @@ -656,6 +656,7 @@ public void initializeTargetExport() throws Exception } // Launch in S1 the task that will initialize S2 + waitForRemoteReplicas(server2ID); addTask(taskInitTargetS2, ResultCode.SUCCESS, null); // Signal RS we just entered the full update status @@ -714,6 +715,7 @@ public void initializeTargetExportAll() throws Exception } // Launch in S1 the task that will initialize S2 + waitForRemoteReplicas(server2ID, server3ID); addTask(taskInitTargetAll, ResultCode.SUCCESS, null); // Tests that entries have been received by S2 @@ -1004,6 +1006,7 @@ public void initializeTargetExportMultiSS() throws Exception // Launch in S1 the task that will initialize S2 log(testCase + " add task " + Thread.currentThread()); + waitForRemoteReplicas(server2ID); addTask(taskInitTargetS2, ResultCode.SUCCESS, null); log(testCase + " " + server2.getServerId() + " wait target " + Thread.currentThread()); @@ -1027,6 +1030,47 @@ public void initializeTargetExportMultiSS() throws Exception } } + /** + * Tests that an InitializeTarget task failing its validation (the remote + * replica is unknown to the domain) does not leave the import/export + * context acquired (issue #730): the domain used to reject every subsequent + * total update as a simultaneous import/export. + */ + @Test(enabled=true) + public void initializeTargetUnknownRemote() throws Exception + { + String testCase = "initializeTargetUnknownRemote"; + log("Starting " + testCase); + try + { + replServer1 = createReplicationServer(replServer1ID, testCase); + connectServer1ToReplServer(replServer1ID); + addTestEntriesToDB(); + + // DS(42424) is unknown in the topology: the task must fail to start... + Entry taskInitTargetUnknown = TestCaseUtils.makeEntry( + "dn: ds-task-id=" + UUID.randomUUID() + ",cn=Scheduled Tasks,cn=Tasks", + "objectclass: top", + "objectclass: ds-task", + "objectclass: ds-task-initialize-remote-replica", + "ds-task-class-name: org.opends.server.tasks.InitializeTargetTask", + "ds-task-initialize-domain-dn: " + EXAMPLE_DN, + "ds-task-initialize-replica-server-id: " + 42424); + addTask(taskInitTargetUnknown, ResultCode.SUCCESS, null); + waitTaskState(taskInitTargetUnknown, STOPPED_BY_ERROR, 20000, null); + + // ...but must not leave the import/export context acquired + assertFalse(replDomain.ieRunning(), + "ReplicationDomain: Import/Export is not expected to be running after a failed InitializeTarget"); + + log("Successfully ending " + testCase); + } + finally + { + afterTest(testCase); + } + } + private void waitForInitializeTargetMsg(String testCase, ReplicationBroker server) throws Exception { @@ -1293,9 +1337,30 @@ public void initializeSimultaneous() throws Exception * Disconnect broker and remove entries from the local DB * @param testCase The name of the test case. */ + /** + * Waits until the local replication domain sees the provided replicas in + * its topology view, so that an InitializeTarget task does not race the + * topology propagation and fail to start with "the remote directory server + * DS(x) is unknown" - which, combined with the import/export context leak + * (issue #730), used to poison the domain for the rest of the test class. + */ + private void waitForRemoteReplicas(Integer... serverIds) throws Exception + { + for (int serverId : serverIds) + { + for (int i = 0; i < 100 && !replDomain.getReplicaInfos().containsKey(serverId); i++) + { + sleep(100); + } + assertTrue(replDomain.getReplicaInfos().containsKey(serverId), + "DS(" + serverId + ") is not known to the local replication domain"); + } + } + private void afterTest(String testCase) throws Exception { // Check that the domain has completed the import/export task. + boolean ieStillRunning = false; if (replDomain != null) { // race condition could cause the main thread to reach @@ -1309,7 +1374,10 @@ private void afterTest(String testCase) throws Exception } sleep(500); } - assertFalse(replDomain.ieRunning(), "ReplicationDomain: Import/Export is not expected to be running"); + // asserted only after the cleanup below: failing before it would leak + // the domain config, the brokers and the replication servers into the + // following tests and cascade the failure over the whole class + ieStillRunning = replDomain.ieRunning(); } // Remove domain config super.cleanConfigEntries(); @@ -1330,6 +1398,8 @@ private void afterTest(String testCase) throws Exception Arrays.fill(replServerPort, 0); log("Successfully cleaned " + testCase); + + assertFalse(ieStillRunning, "ReplicationDomain: Import/Export is not expected to be running"); } /** From 4d67907b748c1d5a0901111f2211c9f63dce374f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 11 Jul 2026 09:39:28 +0300 Subject: [PATCH 2/2] Tolerate fast task failure in waitTaskState(RUNNING) (review of #731 CI) addTask() waits for the added task to reach RUNNING, tolerating a task fast enough to have already COMPLETED_SUCCESSFULLY - but not one fast enough to have already STOPPED_BY_ERROR. initializeTargetUnknownRemote's task fails its validation within milliseconds (by design), so on a fast CI runner the RUNNING phase may never be observed: the ubuntu-latest/11 leg failed with "Expected task state:RUNNING ... found [STOPPED_BY_ERROR]". Accept both terminal states in the RUNNING wait; callers interested in the final state already wait for it explicitly afterwards (the only direct RUNNING waiter outside addTask does exactly that). --- .../opends/server/replication/ReplicationTestCase.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/replication/ReplicationTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/replication/ReplicationTestCase.java index 3f302f834a..54d858cb82 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/replication/ReplicationTestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/replication/ReplicationTestCase.java @@ -13,6 +13,7 @@ * * Copyright 2006-2010 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems, LLC. */ package org.opends.server.replication; @@ -726,11 +727,13 @@ public Entry call() throws Exception } } - if (expectedTaskState == RUNNING && taskState == COMPLETED_SUCCESSFULLY) + if (expectedTaskState == RUNNING + && (taskState == COMPLETED_SUCCESSFULLY || taskState == STOPPED_BY_ERROR)) { // We usually wait the running state after adding the task - // and if the task is fast enough then it may be already done - // and we can go on. + // and if the task is fast enough then it may already be done + // (successfully or not) and we can go on: callers interested in the + // final state wait for it explicitly afterwards. } else {