[#896] Pin the replay of RootContainer.open, and the give-back of an open which is not replayed

[vharseko]

Fixes #896

RootContainer.open opens and registers the entry container of every base DN inside a single
storage.write, and Storage.write replays its operation after a transaction conflict - "In case of
a write operation rollback, implementations must ensure the write operation is retried until it
succeeds", whose WriteOperation is required to be idempotent. No rollback reaches the registry, so
a replay found every base DN the attempt it replaced had reached already registered and failed with
ERR_ENTRY_CONTAINER_ALREADY_REGISTERED: the backend did not open at all, and what the operator saw
said nothing about the conflict that caused the replay. Every attempt also left the configuration
listeners of its entry containers - five per container, plus one per attribute index and per VLV
index - registered on the backend configuration.

What is in the PR now

#999 (issue #993) merged first and closes this road in RootContainer itself: every attempt of
openAndRegisterEntryContainers begins by unregistering and closing what the attempt it replaces had
registered, a container is registered before its first read (getHighestEntryID) can throw, and
giveUpAfterFailedOpen gives back the containers of an open which fails together with the root
container's own listener and the storage. Rebased over it, the RootContainer change this PR carried
(open inside the write, register after the commit) is gone; what is left is the pin, which master
lacked:

• ReplayedOpenTest, built on the harness ReplayedConfigChangeTest introduced in [#907] Change the base DNs of a pluggable backend outside the write the storage replays #914 - a storage
  which raises PersistIt's own RollbackException, wrapped the way PDBStorage delivers it, from
  inside the operation, so the replay is driven by PDBStorage.write's retry loop rather than by a
  second call to it:
  • a conflict raised while the second base DN is opened, with the first one already registered by the
    attempt being replaced - the case the report is written from;
  • a conflict raised once the operation has run to completion, before the commit, which replays an
    operation that ran to completion, and asserts that the replay gave up both containers of the
    previous attempt;
  • a failure which is not replayed at all, which must leave no entry container behind - and, since
    [#993] Register an entry container's configuration listeners only once it has opened #999, no root container listener either: three removePluggableChangeListener calls, not two.
• JDBCStorage.replayReason's javadoc, and its copy in JDBCStorageRetryTest, described the failure
  of a replayed RootContainer.open as its current contract; they give it as history now.

The first two cases fail on master before #999 with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED wrapped in
ERR_OPEN_ENV_FAIL, the third with no listener ever deregistered; all three are green on master as it
is, with no change to RootContainer.

Which engines reached it

• persistit replays a RollbackException, bounded since PDBStorage.write() replays a rolled-back transaction without any bound, and configuration changes hold an entry container's exclusive lock across it #921 / [#921] Bound the transaction replay of PDBStorage.write() #937 but replayed all the same.
• jdbc reaches it on an existing backend, which is the ordinary restart: every DDL of
  openTree is guarded (isExistsTable, isExistsIndex, the catalog row committed on a connection
  of its own), so the attempt commits nothing, partlyCommitted stays false and replayReason
  replays both a class 40 conflict and a dropped connection. The storage layer keeps that write
  replayable deliberately - see the comments at openTree and enrolInCatalog about "a deadlock at
  the twentieth tree" - so the Java side was the only thing breaking the contract.
• je and cassandra replay no write, and never reached this.

Tests

Run green on master + this PR: ReplayedOpenTest (3), ReplayedConfigChangeTest (20),
FailedBackendOpenTest (8), PersistentCompressedSchemaTest (9), PDBStorageTest (14),
JDBCStorageRetryTest (99). The jdbc engine suites need Docker and were not run locally; the change
carries no jdbc specific code.

Review round 2

• openEntryContainers added a container to opened before getHighestEntryID ran rather than
  after: a failure of that read left the container in neither the give-back nor the registry, the
  same shape [#993] Register an entry container's configuration listeners only once it has opened #999 already used for the sibling leak of A ConfigException while opening an EntryContainer leaves its five configuration listeners registered on a half-open container #993.
• open's two catch blocks, which differed only in whether the exception they were handed already
  was a StorageRuntimeException, became one.
• ReplayedOpenTest raises the conflict the way PDBStorage actually delivers it - a
  RollbackException wrapped in a StorageRuntimeException, not the bare one EntryContainer.open's
  own catch cannot unwind on - and the first case asserts the give-back of both entry containers a
  replayed attempt leaves behind, not only the second case's.
• JDBCStorage.replayReason's javadoc, and its copy in JDBCStorageRetryTest, describe
  RootContainer.open's prior behaviour as history rather than as current fact.

Review round 3

#999 merged with the shape the round 1 note pointed at ("whichever of the two rebases over the other,
keep the add/register ahead of getHighestEntryID"), and with the root container's own listener in
the give-back - the non-blocking item of round 1, out of scope here then. The RootContainer change
of this PR is dropped for it (see above), the third case expects the third listener removal, and the
title and this description say what the PR is now. One commit.

[maximthomas]

praise: The shape is the right one and the description argues it from the code, not from intent.

• Opening inside the write and registering after the commit follows #907 / #914 exactly, and "cannot fail: the base DNs come from a set, and the map of a root container being opened is empty until here" (RootContainer.java:160-161) is a proof, not a hope — ERR_ENTRY_CONTAINER_ALREADY_REGISTERED really is unreachable from this path now.
• closeSilently(opened); opened.clear() at the top of every attempt (:154-155) is the give-back BackendImpl.changeBaseDNTrees already does, applied where the replay loop actually re-enters.
• ReplayedOpenTest drives the replay through PDBStorage.write's own loop rather than a second write() call, so the case reads the engine's contract, not a model of it; and the "Which engines reached it" section names the jdbc guards (isExistsTable, isExistsIndex) that keep partlyCommitted false — I checked, postgres's create index if not exists is guarded too (JDBCStorage.java:3627).

---

issue (blocking): A container enters opened only after getHighestEntryID returns, so a failure of that read leaves it outside every unwind.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:253-255, :154, :174, :179

openEntryContainer (:253) constructs the container — its constructor registers config.addPluggableChangeListener(this) (EntryContainer.java:470) and open() registers one listener per index — then ec.getHighestEntryID(txn) (:254) runs txn.openCursor(id2entry) + positionToLastKey (EntryContainer.java:699-702), a storage read, and only then opened.add(ec) (:255). If the read throws, ec is in neither opened nor the registry. On jdbc, a restart of an existing backend with a dropped connection (or a class-40 conflict) on that SELECT has committing=false, partlyCommitted=false, so replayReason (JDBCStorage.java:3117) replays: the replay closes base DNs 1..k-1, opens base DN k again and registers a second container — two containers for one base DN listening on the backend cfg, the stale one applying changes to index objects the live one does not own. On the give-up road (PDB: PersistitException from the traverse) the container's listeners simply leak — the "wrong error for a silent leak" trade the description says the PR avoids. Pre-existing on the give-up road; new in shape on the replay road, where master failed loudly.

EntryContainer ec = openEntryContainer(baseDN, txn, accessMode);
opened.add(ec);
EntryID id = ec.getHighestEntryID(txn);

Pin: a twin of ConflictingAtTreesOf whose openCursor throws on SECOND's id2entry, then verify(backend.configuredWith, times(2)).removePluggableChangeListener(any()) after the open — red at head (only FIRST is given back), green with the add ahead of the read.

Note: #999 (fd50d19, open) closes the same window on the same lines by registering before the read (RootContainer.java:295-296 there); whichever of the two rebases over the other, keep the add/register ahead of getHighestEntryID.

---

suggestion (non-blocking): The catch (StorageRuntimeException) arm's closeSilently(opened) is reached by no case; deleting it leaves all three green.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:170-175, opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedOpenTest.java:285, :379

Cases 1-2 replay and commit, entering neither arm. Case 3 throws UnreplayableFailure extends Exception (:285); PDBStorage.write:692 rethrows it unchanged, so it lands in the catch (Exception) arm (:177-180), whose closeSilently satisfies the times(2). Production's give-up road is the other arm: PDB's spent-cap StorageRuntimeException (PDBStorage.java:718-737, no cause, so unwrap rethrows it as is) and jdbc's rethrown transaction failure (JDBCStorage.java:3060-3061). Traced by exception type, not run.

// ReplayingStorage.ConflictPoint
/** Once the operation has run to completion, as the failure an engine reports when its replays are spent. */
SPENT

void failAsSpent()
{
  arm(ConflictPoint.SPENT, 1);
}

// ReplayingStorage.write, after writeOperation.run(txn)
if (armed == ConflictPoint.SPENT)
{
  throw new StorageRuntimeException("the replays are spent"); // no cause: PDBStorage.write rethrows it as is
}

A copy of anOpenWhichIsNotReplayedGivesUpTheEntryContainersItOpened armed with failAsSpent() and the same times(2) kills the delete mutant at :174. Or: one arm, so the existing case pins the only door —

catch (Exception e)
{
  closeSilently(opened);
  throw e instanceof StorageRuntimeException ? (StorageRuntimeException) e : new StorageRuntimeException(e);
}

---

suggestion (non-blocking): The first case injects a bare RollbackException from openTree, a shape the engine never delivers, and asserts nothing of the give-back on its own road.

opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedOpenTest.java:471-478, :130-141

PDBStorage wraps every PersistitException | RollbackException into StorageRuntimeException at each store site (:445, :604, …), and EntryContainer.open's catch (StorageRuntimeException) (EntryContainer.java:559-564) closes the half-open container before PDBStorage.write:683-686 unwraps the cause and :688 replays. Bare, that catch does not match: SECOND of attempt 1 is neither closed nor in opened, its five constructor listeners stay on the mock cfg for the rest of the class, and the case checks attempts, base DNs and trees only — the delete closeSilently(opened) mutant at :154 is green on this case (case 2 kills it).

// ConflictingAtTreesOf.openTree
if (conflictingPrefix.equals(name.getBaseDN()))
{
  // what PDBStorage delivers: the engine's RollbackException wrapped, which EntryContainer.open unwinds on
  throw new StorageRuntimeException(new RollbackException());
}

// openIsReplayableWhenTheTransactionConflictsWhileTheSecondBaseDNIsOpened, after openBackend()
// SECOND of the first attempt by its own close(), FIRST by the replay's closeSilently(opened)
verify(backend.configuredWith, times(2)).removePluggableChangeListener(any());

---

issue (non-blocking): The root container's own listener leaks on a failed open().

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:108, :366, opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java:1240-1249

The constructor registers config.addPluggableChangeListener(this) (:108); only close() removes it (:366); newRootContainer drops the root container on a failed open() without close(). The new catch arms give back the entry containers and leave this one, so every failed enable keeps one RootContainer on the backend cfg for the JVM's life. Pre-existing; not among the #993 items the description names, and the times(2) in the third case fixes it as expected behaviour.

catch (StorageRuntimeException e)
{
  closeSilently(opened);
  config.removePluggableChangeListener(this);
  throw e;
}

(same in the catch (Exception) arm; the case's times(2) becomes times(3)). Or: rc.close() in newRootContainer's catch, which also takes #993's "storage left open" — entryContainers is empty on that road, so close() only removes the listener and closes the storage.

---

nitpick (non-blocking): JDBCStorage.replayReason's javadoc, and its copy in JDBCStorageRetryTest, state the pre-PR contract of RootContainer.open as current fact.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:3099-3102, opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java:276-279

"opens and registers every entry container of every base DN in one write: replayed after the trees of the first base DN were created and committed, it registers that base DN a second time and fails with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED" — this PR makes every clause false and touches neither file. The partlyCommitted rationale stands on the mysql/oracle DDL auto-commit alone; rewrite the example as history ("before #896, …") or drop it.

---

nitpick (non-blocking): ConflictPoint.COMMIT and the second case say the conflict is "reported by commit()"; the harness raises it after run() returns, before the commit.

opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedOpenTest.java:302-303, :149, :381

throw new RollbackException() (:381) runs while PDBStorage.write:679 operation.run(this) is still on the stack; :680 txn.commit is never reached on the conflicting attempt. Same catch arm on PDB, so wording only — but a jdbc-shaped harness, where committing changes the answer, would be modelled wrongly from it. "Once the operation has run to completion, before the commit" is what the code does.

[vharseko]

Fixed in 4916312.

• issue (blocking) - opened.add(ec) now runs before getHighestEntryID(txn) in openEntryContainers (RootContainer.java:248-252), matching the shape [#993] Register an entry container's configuration listeners only once it has opened #999 already uses for the sibling leak of A ConfigException while opening an EntryContainer leaves its five configuration listeners registered on a half-open container #993.

• suggestion (dead catch arm) - merged open()'s two catch blocks into one (catch (Exception e) { closeSilently(opened); throw e instanceof StorageRuntimeException ? (StorageRuntimeException) e : new StorageRuntimeException(e); }) rather than adding a failAsSpent() case: the two arms did the same thing modulo the wrap, so there is no branch left for a mutant to hide in.

• suggestion (bare RollbackException) - ConflictingAtTreesOf.openTree now throws new StorageRuntimeException(new RollbackException()), the shape PDBStorage actually delivers, and the first case now also asserts verify(times(2)).removePluggableChangeListener(any()) (SECOND by its own close() through EntryContainer.open's catch, FIRST by the replay's closeSilently(opened)).

  One correction on the supporting claim: I ran the delete-closeSilently(opened)-at-:154 mutant against the test as it stood (bare RollbackException) to check "green on this case" - both openIsReplayableWhenTheTransactionConflictsWhileTheSecondBaseDNIsOpened and ...AtCommitTime turned red, with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED (the stale opened list from the rolled-back attempt registers FIRST a second time once the retry reopens it). The shape critique stands and is fixed above; the coverage gap it was diagnosed from wasn't there.

• issue (non-blocking, RootContainer's own listener) - agreed, pre-existing and outside A replayed WriteOperation registers an entry container twice: RootContainer.open fails with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED and masks the failure that caused the replay #896; left untouched here.

• nitpick (JDBCStorage.replayReason javadoc + JDBCStorageRetryTest) - reworded as history ("Before OpenDJ issue A replayed WriteOperation registers an entry container twice: RootContainer.open fails with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED and masks the failure that caused the replay #896, ...").

• nitpick (ConflictPoint.COMMIT / "reported by commit()") - reworded to "before the commit" in both the enum comment and the case javadoc.

Re-run green: ReplayedOpenTest (3), ReplayedConfigChangeTest (12), PersistentCompressedSchemaTest (9), PDBStorageTest (10), JETestCase (35). jdbc suites still need Docker, unchanged by this round.

[vharseko]

#999 merged first (1af0a1247d) and takes the road this PR was built for, in the shape your round 1 note pointed at: openAndRegisterEntryContainers now begins every attempt by unregistering and closing what the attempt it replaces had registered, registers a container before getHighestEntryID can throw, and giveUpAfterFailedOpen gives back the containers of a failed open together with the root container's own listener and the storage - the non-blocking item of round 1 included.

Rebased over it, the RootContainer change of this PR is gone, and what is left is the pin, which master still lacked:

• ReplayedOpenTest runs against master's RootContainer as it is. The two replay cases are green unchanged; the third now expects three removePluggableChangeListener calls rather than two, the third being the root container's listener [#993] Register an entry container's configuration listeners only once it has opened #999 added to the give-back.
• The javadoc of JDBCStorage.replayReason and its copy in JDBCStorageRetryTest, reworded as history - and now saying that RootContainer.open still opens and registers inside one write, since it does; only the failure of a replay is past.

One commit now, bb5d3afe3f; title and description rewritten to say what the PR is.

Re-run green on master + this head: ReplayedOpenTest (3), ReplayedConfigChangeTest (20), FailedBackendOpenTest (8), PersistentCompressedSchemaTest (9), PDBStorageTest (14), JDBCStorageRetryTest (99).