[#1025] Stop a restore which cannot lock its backend instead of completing it

[vharseko]

Fixes #1025.

Rebased onto master after #969 (e86f702f8e) landed; the branch is the single commit 3e600a73a1, its content unchanged from the original ad67ade1e9 (same patch-id).

The bug

RestoreTask.runTask() guarded the restore with if (verifyOnly || lockBackend(backend)) and had no else. When the backend lock was refused - another process holds it, or the lock file cannot be created - the whole restore block was skipped, errorsEncountered stayed false, the finally re-enabled the backend and told the listeners the restore succeeded, and the task returned getFinalTaskState(), i.e. COMPLETED_SUCCESSFULLY. The restore CLI printed the ERR_RESTOREDB_CANNOT_LOCK_BACKEND line from the task log and exited 0 right after it.

The change

A refused lock ends the task the way BackupTask and ImportTask already end theirs, and the way #969 already ends a failed disableBackend():

if (!verifyOnly && !lockBackend(backend))
{
  errorsEncountered = true;
  return TaskState.STOPPED_BY_ERROR;
}

The return passes through the finally, which re-enables the backend and notifies processRestoreEnd(..., false). The rest of the RestoreTask hunk is the de-indentation of the former if body - ?w=1 shows the five real lines.

restore --verifyOnly is unaffected: it never takes the lock, pinned by the second test below.

Test

TestBackupAndRestore.testRestoreEndsInErrorWhenTheBackendCannotBeLocked - self-contained: it backs userRoot up into a temporary directory, takes a shared lock on the backend's lock file and restores from that backup. LockFileManager reference-counts shared locks, so the task's disableBackend() releases the backend's own reference but not the test's, and the exclusive lock the restore needs is refused with ERR_FILELOCKER_LOCK_EXCLUSIVE_REJECTED_BY_SHARED. Asserted: STOPPED_BY_ERROR, one begin and one end notification, successful == false at the end notification, and userRoot registered again afterwards.

Before the change the test fails with expected [STOPPED_BY_ERROR] but found [COMPLETED_SUCCESSFULLY], the task log carrying the refused-lock error. It also runs the restore half of #969's finally - the backendDisabled re-enable and the !errorsEncountered notification - which the review of #969 noted no test executed.

TestBackupAndRestore.testVerifyOnlyRestoreDoesNotTakeTheBackendLock - added in review round 2, pins the !verifyOnly arm of the guard: it backs userRoot up and verifies that backup with ds-task-restore-verify-only: true. A verify-only restore never disables the backend, so the backend keeps the shared lock BackendConfigManager took when it was enabled and an exclusive lock request is refused over it. With the mutant if (!lockBackend(backend)) the case fails with expected [COMPLETED_SUCCESSFULLY] but found [STOPPED_BY_ERROR], while testRestoreEndsInErrorWhenTheBackendCannotBeLocked stays green under that same mutant - which is why the arm needed a case of its own.

Run: TestBackupAndRestore 14/14, TestImportAndExport 14/14, PrivilegeTestCase 185/185, ReSyncTest 2/2, LDIFBackendTestCase 22/22.

[vharseko]

@maximthomas rebased onto master now that #969 has landed - the merge conflict is gone. The branch is the single commit 3e600a73a1, the same patch as ad67ade1e9 (identical patch-id), so there is nothing new to review beyond that one commit. The description no longer mentions the stack.

[maximthomas]

praise: The change is exactly where the bug is and no wider.

• RestoreTask.runTask:294-298 — the refused lock now takes the road BackupTask/ImportTask and the disableBackend catch at :283-289 already take: errorsEncountered = true; return TaskState.STOPPED_BY_ERROR, through the finally which re-enables the backend and calls notifyRestoreEnded(..., false). git diff -w shows +7/−3 in RestoreTask.java; the rest is de-indentation, as the description says.
• TestBackupAndRestore.testRestoreEndsInErrorWhenTheBackendCannotBeLocked:266-277 — the shared lock is taken and released in a try/finally around the task, so the refcount LockFileManager keeps for userRoot is balanced for the classes after it in the same JVM.
• CI run 34607225854 is the head's (headSha 3e600a73); the five ubuntu failsafe cells are green.

---

suggestion (non-blocking): The !verifyOnly && arm of the rewritten guard is unpinned — no test in the repo schedules a verify-only restore.

opendj-server-legacy/src/main/java/org/opends/server/tasks/RestoreTask.java:294

git grep -i 'restore-verify-only\|verifyOnly' 3e600a73 outside src/main hits only the schema; every restore a test schedules has verifyOnly=false, on which !verifyOnly && !lockBackend(backend) and the mutant !lockBackend(backend) behave identically, so the mutant survives the whole suite by construction. Under it an online restore --verifyOnly skips disableBackend (:276), the enabled backend keeps the shared lock BackendConfigManager took on enable, LockFileManager.acquireExclusiveLock:198 refuses it, and every verify-only restore ends STOPPED_BY_ERROR. The gap predates this PR (verifyOnly || lockBackend(backend) was as unpinned), but the description's "restore --verifyOnly is unaffected: it never takes the lock" now rests on reading alone.

  /** A verify-only restore never takes the backend lock: the backend stays enabled and holds its own shared lock. */
  @Test
  public void testVerifyOnlyRestoreDoesNotTakeTheBackendLock() throws Exception
  {
    File backupDirectory = TestCaseUtils.createTemporaryDirectory("restore-verify-only");
    try
    {
      testTask(TestCaseUtils.makeEntry(
          "dn: ds-task-id=" + UUID.randomUUID() + ",cn=Scheduled Tasks,cn=Tasks",
          "objectclass: top",
          "objectclass: ds-task",
          "objectclass: ds-task-backup",
          "ds-task-class-name: org.opends.server.tasks.BackupTask",
          "ds-task-backup-backend-id: userRoot",
          "ds-backup-directory-path: " + backupDirectory.getPath()),
          TaskState.COMPLETED_SUCCESSFULLY, 30);

      testTask(TestCaseUtils.makeEntry(restoreTask(
          "ds-backup-directory-path: " + backupDirectory.getPath(),
          "ds-task-restore-verify-only: true")),
          TaskState.COMPLETED_SUCCESSFULLY, 30);
    }
    finally
    {
      TestCaseUtils.deleteDirectory(backupDirectory);
    }
  }

Pin: with the mutant if (!lockBackend(backend)) the enabled backend's own shared lock refuses the exclusive one and the case fails with expected [COMPLETED_SUCCESSFULLY] but found [STOPPED_BY_ERROR].

---

nitpick (non-blocking): The javadoc says the refused-lock road ends "exactly as when the restore itself fails"; the task state differs between the two roads.

opendj-server-legacy/src/test/java/org/opends/server/tasks/TestBackupAndRestore.java:220-223

A refused lock returns STOPPED_BY_ERROR (RestoreTask.java:294-298); a throwing restoreBackup sets errorsEncountered without returning (:306-317) and the task ends COMPLETED_WITH_ERRORS (:350-352). Only the listener call, notifyRestoreEnded(..., false), is the same on both roads, and the test body itself expects the state the restore-failure road never yields.

  /**
   * A restore which cannot lock its backend has restored nothing and must say so: the task
   * ends STOPPED_BY_ERROR and the restore task listeners are told the restore failed, as they
   * are when the restore itself fails.
   */

[vharseko]

Both points taken, round 2 is 09711e10d0.

suggestion — the !verifyOnly && arm is unpinned. Confirmed and pinned with your case, added as
testVerifyOnlyRestoreDoesNotTakeTheBackendLock. The reading behind it holds in the code:
BackendConfigManager.initializeBackend:275 takes the shared lock for an enabled backend ("prevent
operations like LDIF import or restore while the backend is active"), a verify-only restore never
reaches disableBackend (RestoreTask:276), and LockFileManager.acquireExclusiveLock refuses an
exclusive lock over that same-JVM shared entry.

Measured, not read:

run	tree	TestBackupAndRestore
A	this round	14/14 green
B	this round with the mutant if (!lockBackend(backend))	1 failure out of 14 - the new case only: testVerifyOnlyRestoreDoesNotTakeTheBackendLock -> TasksTestCase.testTask expected [COMPLETED_SUCCESSFULLY] but found [STOPPED_BY_ERROR]

Run B also shows what your point says: testRestoreEndsInErrorWhenTheBackendCannotBeLocked stays
green under that mutant, so it never covered the arm.

The case carries a two-line javadoc rather than your one-liner, naming why the exclusive lock would
be refused - that is the whole reason the case is red under the mutant.

nitpick — "exactly as when the restore itself fails". Correct, the two roads end differently: a
refused lock returns STOPPED_BY_ERROR (RestoreTask:294-298), a throwing restoreBackup only
sets errorsEncountered (:306-317) and the task ends COMPLETED_WITH_ERRORS (:352-354); the
shared part is notifyRestoreEnded(..., false) (:349). Javadoc rewritten to your wording.

Run on the round's head: TestBackupAndRestore 14/14, TestImportAndExport 14/14, PrivilegeTestCase 185/185, ReSyncTest 2/2, LDIFBackendTestCase 22/22.

[maximthomas]

praise: Both claims of the description are now measured, not read.

• TestBackupAndRestore.testVerifyOnlyRestoreDoesNotTakeTheBackendLock:298-322 kills the acquire-arm mutant: with if (!lockBackend(backend)) at RestoreTask.runTask:294 the case is red with expected [COMPLETED_SUCCESSFULLY] but found [STOPPED_BY_ERROR] and the other 13 cases stay green — the table in the description, reproduced (14 ran, 1 failed).
• testRestoreEndsInErrorWhenTheBackendCannotBeLocked:271 is red with RestoreTask.java at the base f559b090 and everything else at the head: expected [STOPPED_BY_ERROR] but found [COMPLETED_SUCCESSFULLY], 14 ran, 1 failed — round 1's open item, closed by a run.
• RestoreTask.java is unchanged since 3e600a73; the head's CI is green on every cell, the five ubuntu failsafe cells included.