From 5b4444f4804789f2eefa934dc349a1c40b0048c6 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 11 Sep 2026 15:36:31 +0300 Subject: [PATCH 1/2] [#1026] Close the import config on every path an import task can end on ImportTask closed its LDIFImportConfig after the outer try/finally, so only a fully successful import reached it: a backend that could not be disabled, a lock that could not be taken, an importLDIF() that threw, a lock that could not be released and a backend that could not be re-enabled all returned above it. A backend closes the config together with its LDIF reader, so what this left open were the reject and skip files of an import that failed before that reader existed - the task opens them before the backend is touched - for as long as the completed task is retained. The close now opens the finally, ahead of the re-enable and the listener notification, whichever way the import ended; closing a config the backend already closed is a no-op. Fixes #1026. --- .../org/opends/server/tasks/ImportTask.java | 7 ++- .../opends/server/api/TestTaskListener.java | 5 ++ .../server/tasks/TestImportAndExport.java | 56 +++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java b/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java index aaebe55ee3..d4b6036a89 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java @@ -720,6 +720,11 @@ else if(backend != locatedBackend) } finally { + // Close the LDIF reader and the reject and skip files whichever way the import ended. + // The backend closes them with its reader, but an import which fails before that reader + // exists - or before the import is even launched - leaves them to this task. + importConfig.close(); + // Enable the backend, if it was this task which disabled it. boolean backendLeftDisabled = false; if (backendDisabled) @@ -748,8 +753,6 @@ else if(backend != locatedBackend) } } - // Clean up after the import by closing the import config. - importConfig.close(); return getFinalTaskState(); } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/api/TestTaskListener.java b/opendj-server-legacy/src/test/java/org/opends/server/api/TestTaskListener.java index 8a4f9e9ae9..597cfd3282 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/api/TestTaskListener.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/api/TestTaskListener.java @@ -13,10 +13,12 @@ * * Copyright 2006-2008 Sun Microsystems, Inc. * Portions Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.api; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.opends.server.core.DirectoryServer; import org.opends.server.types.BackupConfig; @@ -42,6 +44,8 @@ public class TestTaskListener public static final AtomicInteger importEndCount = new AtomicInteger(0); public static final AtomicInteger restoreBeginCount = new AtomicInteger(0); public static final AtomicInteger restoreEndCount = new AtomicInteger(0); + /** The import config the last import end notification carried, {@code null} until the first. */ + public static final AtomicReference lastImportEndConfig = new AtomicReference<>(); /** Registers the task listeners with the Directory Server. */ public static void registerListeners() @@ -107,5 +111,6 @@ public void processImportBegin(LocalBackend backend, LDIFImportConfig config) public void processImportEnd(LocalBackend backend, LDIFImportConfig config, boolean successful) { importEndCount.incrementAndGet(); + lastImportEndConfig.set(config); } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java b/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java index f54a243a4c..00232a0c26 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java @@ -18,6 +18,8 @@ package org.opends.server.tasks; import java.io.File; +import java.io.IOException; +import java.io.Writer; import java.util.UUID; import org.forgerock.opendj.ldap.ResultCode; @@ -29,6 +31,7 @@ import org.opends.server.core.BackendConfigManager; import org.opends.server.core.DirectoryServer; import org.opends.server.types.Entry; +import org.opends.server.types.LDIFImportConfig; import org.forgerock.opendj.ldap.schema.ObjectClass; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; @@ -450,6 +453,59 @@ public void testFailedImportEndsOnlyOnce() throws Exception assertEquals(TestTaskListener.importEndCount.get(), importEndCount + 1); } + /** + * A failed import must close the reject and skip files it opened. A backend closes the + * import config together with its LDIF reader, so an import which fails before that reader + * exists leaves the closing to the task, whose error returns must not skip it. + */ + @Test + public void testFailedImportClosesItsRejectAndSkipFiles() throws Exception + { + File skipFile = File.createTempFile("import-test-skipped", ".ldif"); + try + { + // A directory can be read, so the task accepts it, but it cannot be opened as LDIF: + // the import fails before the backend creates the reader which would close the config. + Entry taskEntry = TestCaseUtils.makeEntry( + "dn: ds-task-id=" + UUID.randomUUID() + ",cn=Scheduled Tasks,cn=Tasks", + "objectclass: top", + "objectclass: ds-task", + "objectclass: ds-task-import", + "ds-task-class-name: org.opends.server.tasks.ImportTask", + "ds-task-import-backend-id: userRoot", + "ds-task-import-ldif-file: " + ldifFile.getParent(), + "ds-task-import-reject-file: " + rejectFile.getPath(), + "ds-task-import-skip-file: " + skipFile.getPath(), + "ds-task-import-overwrite-rejects: TRUE"); + + TestTaskListener.lastImportEndConfig.set(null); + testTask(taskEntry, TaskState.STOPPED_BY_ERROR, 60); + + LDIFImportConfig importConfig = TestTaskListener.lastImportEndConfig.get(); + assertNotNull(importConfig, "The import end was not notified"); + assertClosed(importConfig.getRejectWriter(), "reject"); + assertClosed(importConfig.getSkipWriter(), "skip"); + } + finally + { + skipFile.delete(); + } + } + + private static void assertClosed(Writer writer, String name) + { + assertNotNull(writer, "The " + name + " writer was never opened"); + try + { + writer.write("still open"); + fail("The " + name + " writer is still open after the import ended"); + } + catch (IOException expected) + { + // A closed BufferedWriter refuses the write: that is the closed state being checked. + } + } + private void removeMemoryBackend(String backendID) throws Exception { BackendConfigManager backendConfigManager = TestCaseUtils.getServerContext().getBackendConfigManager(); From e4921bcac7d0cd7e798f258ccc3b63fad35c1abd Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 21 Sep 2026 23:12:03 +0300 Subject: [PATCH 2/2] [#1026] Close the reject file when the skip file is the one which cannot be opened The close this PR moved into the finally of runTask() is reached by every return below it, but the skip file is opened above it: writeRejectedEntries() opens the reject file the moment it is called, and when writeSkippedEntries() then fails - on a path the task never checks, it only stores the attribute as a string - the return goes above notifyImportBeginning() and above the try whose finally closes the config, leaving the reject writer open for as long as the completed task is retained. The catch closes the config itself. The two files are not opened inside that try instead, which would need no new close: a listener told an import began puts back what it took offline when it is told it ended - a replication domain reloads and rewinds its state - and an import which never reached a backend has nothing for it to put back. testImportWhichCannotOpenItsSkipFileClosesItsRejectFile pins it. That return is above the notification the other cases read the config through, so the config is read from the task the scheduler kept. testImportEndsWhenTheBackendCannotBeDisabled now carries a reject and a skip file too: it is the road which ends furthest from the backend, so a close which rides on the backend having been disabled, or on the import having been launched, passes the other cases and fails this one. --- .../org/opends/server/tasks/ImportTask.java | 8 +++ .../server/tasks/TestImportAndExport.java | 56 ++++++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java b/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java index d4b6036a89..779a8e1d60 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java @@ -617,6 +617,14 @@ else if(backend != locatedBackend) catch (Exception e) { logger.error(ERR_LDIFIMPORT_CANNOT_OPEN_SKIP_FILE, skipFile, getExceptionMessage(e)); + /* + * The reject file is already open and this return is the one which is above the try + * whose finally closes it. The two files are not opened inside that try instead: a + * listener told an import began puts back what it took offline when it is told the + * import ended - a replication domain reloads and rewinds its state - and an import + * which never reached a backend has nothing for it to put back. + */ + importConfig.close(); return TaskState.STOPPED_BY_ERROR; } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java b/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java index 00232a0c26..872ccdc488 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java @@ -20,12 +20,14 @@ import java.io.File; import java.io.IOException; import java.io.Writer; +import java.lang.reflect.Field; import java.util.UUID; import org.forgerock.opendj.ldap.ResultCode; import org.opends.server.TestCaseUtils; import org.opends.server.api.LocalBackend; import org.opends.server.api.TestTaskListener; +import org.opends.server.backends.task.Task; import org.opends.server.backends.task.TaskState; import org.opends.server.core.AddOperation; import org.opends.server.core.BackendConfigManager; @@ -390,7 +392,10 @@ public void testImportExport(Entry taskEntry, TaskState expectedState) /** * An import which cannot disable its backend must still tell the import task listeners * that the import is over: a listener which took something offline when the import began - * - a replication domain disables itself - has no other chance to put it back. + * - a replication domain disables itself - has no other chance to put it back. The reject + * and skip files it opened before touching the backend must be closed on that road too: + * it is the one which ends furthest from the backend, so a close which rides on the + * backend having been disabled, or on the import having been launched, misses it. */ @Test public void testImportEndsWhenTheBackendCannotBeDisabled() throws Exception @@ -401,6 +406,7 @@ public void testImportEndsWhenTheBackendCannotBeDisabled() throws Exception */ final String backendID = "importTaskUnconfiguredBackend"; TestCaseUtils.initializeMemoryBackend(backendID, "dc=unconfigured,dc=com", true); + File skipFile = File.createTempFile("import-test-skipped", ".ldif"); try { int importBeginCount = TestTaskListener.importBeginCount.get(); @@ -413,15 +419,24 @@ public void testImportEndsWhenTheBackendCannotBeDisabled() throws Exception "objectclass: ds-task-import", "ds-task-class-name: org.opends.server.tasks.ImportTask", "ds-task-import-backend-id: " + backendID, - "ds-task-import-ldif-file: " + ldifFile.getPath()); + "ds-task-import-ldif-file: " + ldifFile.getPath(), + "ds-task-import-reject-file: " + rejectFile.getPath(), + "ds-task-import-skip-file: " + skipFile.getPath(), + "ds-task-import-overwrite-rejects: TRUE"); + TestTaskListener.lastImportEndConfig.set(null); testTask(taskEntry, TaskState.STOPPED_BY_ERROR, 60); assertEquals(TestTaskListener.importBeginCount.get(), importBeginCount + 1); assertEquals(TestTaskListener.importEndCount.get(), importEndCount + 1); + LDIFImportConfig importConfig = TestTaskListener.lastImportEndConfig.get(); + assertNotNull(importConfig, "The import end was not notified"); + assertClosed(importConfig.getRejectWriter(), "reject"); + assertClosed(importConfig.getSkipWriter(), "skip"); } finally { + skipFile.delete(); removeMemoryBackend(backendID); } } @@ -492,6 +507,43 @@ public void testFailedImportClosesItsRejectAndSkipFiles() throws Exception } } + /** + * An import which cannot open its skip file must close the reject file it already opened. + * That failure returns before the import is announced, so the config cannot be read through + * the listener the other cases use, and is read from the task the scheduler kept instead. + */ + @Test + public void testImportWhichCannotOpenItsSkipFileClosesItsRejectFile() throws Exception + { + Entry taskEntry = TestCaseUtils.makeEntry( + "dn: ds-task-id=" + UUID.randomUUID() + ",cn=Scheduled Tasks,cn=Tasks", + "objectclass: top", + "objectclass: ds-task", + "objectclass: ds-task-import", + "ds-task-class-name: org.opends.server.tasks.ImportTask", + "ds-task-import-backend-id: userRoot", + "ds-task-import-ldif-file: " + ldifFile.getPath(), + "ds-task-import-reject-file: " + rejectFile.getPath(), + // The task stores the path as it is given, and a directory cannot be opened for writing. + "ds-task-import-skip-file: " + ldifFile.getParent(), + "ds-task-import-overwrite-rejects: TRUE"); + + testTask(taskEntry, TaskState.STOPPED_BY_ERROR, 60); + + LDIFImportConfig importConfig = importConfigOf(getDoneTask(taskEntry.getName())); + assertNotNull(importConfig, "The task never built an import config"); + assertClosed(importConfig.getRejectWriter(), "reject"); + assertNull(importConfig.getSkipWriter(), "The skip writer was opened after all"); + } + + /** The config an import task worked with, which the task keeps to itself. */ + private static LDIFImportConfig importConfigOf(Task task) throws Exception + { + Field importConfig = ImportTask.class.getDeclaredField("importConfig"); + importConfig.setAccessible(true); + return (LDIFImportConfig) importConfig.get(task); + } + private static void assertClosed(Writer writer, String name) { assertNotNull(writer, "The " + name + " writer was never opened");