AttributeIndex.applyConfigurationChange reports its administrative actions from inside the WriteOperation, so a replayed write repeats them and can leave a stale index trusted #991

(all line numbers are on master, 2a7bb9d)

Describe the bug

Storage.write() may replay its WriteOperation after a transaction conflict — PDBStorage.write() and
JDBCStorage.write() both do. AttributeIndex.applyConfigurationChange calls two helpers from inside such a
WriteOperation, and both write into the caller's ConfigChangeResult and mutate in-memory state that no
rollback undoes.

// AttributeIndex.java:928-940 — the added indexes
storage.write(txn -> { for (MatchingRuleIndex added : addedIndexes.values()) createIndex(txn, added, ccr); });
// AttributeIndex.java:965-976 — the indexes that stay
storage.write(txn -> { for (Index updated : updatedIndexes.values()) updateIndex(updated, newConfiguration, ccr, txn); });

// AttributeIndex.java:986-993
private static void createIndex(WriteableTransaction txn, MatchingRuleIndex index, ConfigChangeResult ccr)
{
  index.open(txn, true);
  if (!index.isTrusted())
  {
    ccr.setAdminActionRequired(true);
    ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(index.getName()));       // :992
  }
}

// AttributeIndex.java:996-1017
boolean newLimitRequiresRebuild = updatedIndex.setIndexEntryLimit(newConfig.getIndexEntryLimit());
if (newLimitRequiresRebuild)
{
  ccr.setAdminActionRequired(true);
  ccr.addMessage(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.get(updatedIndex.getName()));   // :1004
}
boolean newConfidentialityRequiresRebuild = updatedIndex.setConfidential(newConfig.isConfidentialityEnabled());
if (newConfidentialityRequiresRebuild)
{
  ccr.setAdminActionRequired(true);
  ccr.addMessage(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.get(updatedIndex.getName())); // :1011
}
if (newLimitRequiresRebuild || newConfidentialityRequiresRebuild)
{
  updatedIndex.setTrusted(txn, false);
}

Two consequences follow from the same root.

1. Every replayed attempt repeats the messages

ConfigChangeResult.addMessage appends to a list. An attempt that rolls back leaves its messages in the result,
and the next attempt adds them again, so the operator is told "index ... must be rebuilt" once per attempt.
EntryContainer.applyConfigurationAdd (:197-227) was already restructured for exactly this reason: it captures
index.isTrusted() into an AtomicBoolean inside the write and reports outside it, with a comment saying so.

2. An index that stays TRUSTED although it must be rebuilt

DefaultIndex.setIndexEntryLimit (:302-307) is a non-transactional field assignment whose return value means
"the limit went up":

final boolean rebuildRequired = this.indexEntryLimit < indexEntryLimit;
this.indexEntryLimit = indexEntryLimit;
return rebuildRequired;

Raise index-entry-limit from 100 to 200 and let the first attempt hit a transaction conflict:

• attempt 1: 100 < 200 is true → message added → setTrusted(txn, false) → state.removeFlagsFromIndex(txn, ...).
  The transaction rolls back, so the TRUSTED flag stays in the state tree; the in-memory field
  this.indexEntryLimit = 200 and this.trusted = false (DefaultIndex.java:322-333) do not roll back.
• attempt 2 (the replay): the field is already 200, so 200 < 200 is false. setConfidential returns what it
  returned before, so nothing calls setTrusted(txn, false) and the attempt commits.

The committed state is an index whose entry limit was raised — i.e. whose keys dropped under the old limit are
missing — and which is still flagged TRUSTED on disk. The running server does not use it, because the rolled-back
attempt left trusted = false in memory; after the next restart DefaultIndex.afterOpen (:97-105) reads TRUSTED
back out of state and the stale index is used as if it were complete, silently returning incomplete search
results. The ConfigChangeResult did carry the rebuild message to the operator, but nothing enforces the rebuild
and nothing warns again at startup — EntryContainer.open logs NOTE_INDEX_ADD_REQUIRES_REBUILD only for an
index that is not trusted (:536-540).

Expected behavior

A replayed write leaves the same result as a single attempt: each message once, and a persisted TRUSTED flag that
agrees with the configuration that was committed.

Suggested fix

Keep reporting and non-idempotent in-memory mutation out of the WriteOperation:

• decide what changed before the write (compare newConfig.getIndexEntryLimit() and
  newConfig.isConfidentialityEnabled() with the current values), pass the decision into the write, and let the
  write do only setTrusted(txn, false);
• add the messages and setAdminActionRequired(true) after write() returns, as
  EntryContainer.applyConfigurationAdd:220-225 already does;
• assign the in-memory fields after the write has committed, so that a rollback cannot make the next attempt
  compute a different answer.

Related

Found while working on #962, which addresses a different failure of the same call paths (a bounded
Storage.write that gives up leaves the in-memory state and the storage disagreeing). This one is independent of
that: it needs a single replay, which every storage engine may perform at any time.