[#966] Tell the import and restore task listeners the task is over on every path

[vharseko]

Fixes #966.

The leak

ImportTask and RestoreTask notify their task listeners that the task is beginning before disabling the backend — a replication domain disables itself from that notification (MultimasterReplication.processImportBegin / processRestoreBegin → LDAPReplicationDomain.disable()) — and enable it back from a finally. A failed TaskUtils.disableBackend() returned from above that finally, so nothing ever put the domain back: replication for that base DN stayed silently off until the server was restarted, while the domain kept accepting local writes it never published.

ImportTask leaked a second time from inside the finally itself, where a failed enableBackend() returned before reaching the notification.

And the failure paths which did notify the end themselves notified it twice, since they return through the finally which notifies again. The first of the two ran while the backend was still disabled, so it could only reload an empty ServerState and rewind the CSN generator (adjust(null)) before failing in computeGenerationId(); the second one, after the backend was back, was what actually re-enabled the domain.

The change

Both tasks now put the backend disable inside the try whose finally notifies, remember in backendDisabled whether they were the ones who disabled it, and fire the end notification exactly once, after the backend is back:

• the early return of a failed disableBackend() now passes through the finally, which notifies without touching a backend it never disabled;
• ImportTask's failed enableBackend() no longer returns before the notification — the result state is unchanged, the return just moves after it;
• the notify*Ended(..., false) calls in the restore/import failure catches are gone, and the single notification carries the real outcome instead of a constant true.

restore --verifyOnly is unaffected: it never disables the backend, so backendDisabled stays false and the notification is sent as before.

Tests

Two regression tests in TestImportAndExport, both failing before the change and for different reasons:

• testImportEndsWhenTheBackendCannotBeDisabled — a backend registered at runtime has no entry in cn=config, so TaskUtils.disableBackend() cannot modify it; the import must still pair its begin with an end (before: the end never fired);
• testFailedImportEndsOnlyOnce — an import whose LDIF path is a directory passes the task validation and fails inside importLDIF(); the end must fire once (before: twice).

Run: TestImportAndExport 14/14, TestBackupAndRestore 12/12, plus ReSyncTest, LDIFBackendTestCase and PrivilegeTestCase as a regression on the paths that drive these tasks — 209 tests, no failures.

Out of scope

Where disableBackend() fails after the configuration change was applied — ConfigurationHandler.replaceEntry() runs applyConfigurationChange after writing the entry, and BackendConfigManager has deregistered and finalized the backend by then — the backend really is down, and LDAPReplicationDomain.enable() will still take its own catch and leave the domain disabled with no alert. That is a separate defect one level down, noted in #966.

[maximthomas]

Approving. The fix does what the description says and I could confirm it by running it, not only by reading it.

What I checked

• TestImportAndExport at HEAD (7850557): 14/14, both new cases executed.
• Same tree with ImportTask.java reverted to the base commit and the test file left at HEAD: exactly the two new cases fail, with the values the description predicts — testFailedImportEndsOnlyOnce gets 2 end notifications instead of 1, testImportEndsWhenTheBackendCannotBeDisabled gets 0 instead of 1. The other 12 stay green.
• Walked every exit path of both runTask methods (disable fails, lock fails, import/restore fails, unlock fails, success — each crossed with enable failing) at base and head: the TaskState escaping is identical on all of them, the end notification now fires exactly once on all of them, and no path ends in a worse state than before. restore --verifyOnly keeps backendDisabled == false and behaves as before.

One ask, not blocking

The restore half of the change is executed by no test. TestBackupAndRestore guards its restoreBegin/restoreEnd assertions to COMPLETED_SUCCESSFULLY / COMPLETED_WITH_ERRORS rows (:193-194), and its three failing rows die at RestoreTask:204 / :216, before notifyRestoreBeginning — so if (backendDisabled) replacing if (!verifyOnly) in the finally, and the new disable-failure route through it, run only where the two conditions coincide. A restore twin of testImportEndsWhenTheBackendCannotBeDisabled (runtime-registered backend, assert begin == end) would pin it. Fine as a follow-up if you prefer.

Follow-ups, none blocking

1. successful is not quite "the real outcome" yet. In ImportTask it is raised once at :676 and never lowered, so listeners get true when the re-enable fails (:740-747, task returns STOPPED_BY_ERROR with the backend still down), when the lock release fails (:709, COMPLETED_WITH_ERRORS) and on an administrator cancel (importLDIF returns normally, STOPPED_BY_ADMINISTRATOR). RestoreTask does the opposite and folds the unlock/enable failures into errorsEncountered (:317-320, :341), so a restore that wrote its data reports false. Nothing in-tree reads the flag, so this is a contract question rather than a bug: worth picking one meaning, stating it on ImportTaskListener / RestoreTaskListener, and computing it the same way in both tasks.
2. The two new tests pin "fires" and "once" but not "after the backend is back": moving the notify at ImportTask:744 above the enable block at :725 passes 14/14. Recording in the test listener whether userRoot is registered and enabled at processImportEnd time would pin the ordering, which is the point of the fix.
3. The comment at ImportTask:743 / RestoreTask:344 — "Notified once, after the backend is back, so that a listener can read it again" — is false on the enable-failure branch three lines below it, where backend is still the finalized pre-disable instance.
4. For #966, one more outcome of the enable-failure path: LDAPReplicationDomain.saveGenerationId() writes ds-sync-generation-id to the domain's config entry when the base entry does not exist (empty backend) and nothing removes it later; loadGenerationId() falls back to that entry on NO_SUCH_OBJECT. On a server whose domain was once enabled over an empty backend, the notification now brings the domain up live with that stale generation id instead of leaving it disabled — still terminal until a restart, and visible on the RS as a generation-id mismatch rather than silent, so not worse than before, but worth a line in the issue.

Pre-existing, outside this change — filed as #1025 and #1026:

• #1025 — RestoreTask:293 — if (verifyOnly || lockBackend(backend)) has no else, so a restore that cannot lock the backend skips everything, re-enables, and returns COMPLETED_SUCCESSFULLY with successful == true.
• #1026 — ImportTask:752 — importConfig.close() sits after the outer try/finally, so every error return (including the disable-failure one this change routes through the finally) leaves the LDIF reader and the reject/skip writers open and unflushed.