[#962] Flag an admin action when a bounded storage write gives up on a configuration change

[maximthomas]

Summary

Storage.write() is bounded on both engines — JDBCStorage.write() has been for a while, and PDBStorage.write()
since #937 — so once the bound is spent, write() throws instead of replaying. Its javadoc already names the
obligation that creates for its callers:

A caller that mutates state around this method must handle that bound being spent. Removing an entry from an
in-memory map before the write so that a replay still finds the work to do, or reading configuration back out of
the operation once it returns, both assume the write is applied; when it is not, this method throws with that
state already changed and the transaction not applied, and the caller is the only place that can reconcile the two.

Four configuration change paths of the pluggable backends are exactly those callers, and none of them met the
obligation: each set the server error result code and handed back a bare single line stack trace. This PR makes
each of them name what diverged and ask for the administrative action which repairs it, and moves two of them so
that less diverges in the first place.

What each path leaves behind when the bound is spent

Path	Order	What is left
EntryContainer.AttributeIndexCfgManager.applyConfigurationDelete	maps updated before the write	the trees of the index, with no configuration naming them any more
EntryContainer.VLVIndexCfgManager.applyConfigurationDelete	the same	the two trees of the VLV index, vlv.<name> and its counter
AttributeIndex.applyConfigurationChange	three writes	write 2: the trees of the removed index types; write 3: the entry limit never applied to the index types which stay
EntryContainer.applyConfigurationChange	a sequence of assignments	the entries and the indexes encoded under settings which no longer agree

Every one of them reported it like this:

ccr.setResultCode(getCoreConfigManager().getServerErrorResultCode());
ccr.addMessage(LocalizableMessage.raw(StaticUtils.stackTraceToSingleLineString(de)));

No message id, nothing said about what diverged, and no adminActionRequired.

What the operator gets now

Four message ids (624-627 in backend.properties): ERR_CONFIG_INDEX_DELETE_FAILED,
ERR_CONFIG_VLV_INDEX_DELETE_FAILED, ERR_CONFIG_INDEX_CHANGE_FAILED, ERR_CONFIG_BACKEND_DATA_CHANGE_FAILED.
Each names the index or the base DN, keeps the stack trace as its trailing cause, and states the repair. Each site
also sets adminActionRequired and logs the message.

Three things about how this reaches the operator, since they shaped the change:

• adminActionRequired is not what the operator reads on a failed change. dsconfig prints the messages of
  the result; the flag is what the configuration framework and programmatic callers see. So the instruction has to
  be in the message text itself, and it is.
• The logger.error call is not redundant with the framework. ConfigurationHandler.handleConfigChangeResult
  does log a non-SUCCESS result, but as an argument of ERR_CONFIG_CHANGE_RESULT_ERROR, and
  TextErrorLogPublisher writes msgID=message.ordinal() of the outer message. Only the call at the site puts
  these ids into the error log, which is what makes the divergence findable after the session that caused it has
  ended — and it outlives that session by definition.
• stackTraceToSingleLineString walks getCause() only and never prints suppressed exceptions. That is why
  PDBStorage's give-up exception carries "attempt cap" / "retry window" in its own message rather than as a
  suppressed one: that message is how the operator learns which of the two bounds was spent, and therefore whether
  raising the attempts or the window would have helped. The comment there is corrected to say so.

What is deliberately not changed

The order of the two deletions. The index is taken out of the map before the write, and the comment already in
EntryContainer explains why: deleting inside the write would let a replayed attempt find nothing to delete and
commit an empty transaction, reporting success for work it did not do. The fix is to report the divergence, not to
trade it for a false success.

No storage-status or id2entry != null precondition on EntryContainer.applyConfigurationChange. No
neighbouring applyConfigurationChange has one (PDBStorage, JEStorage, JDBCStorage, RootContainer;
BackendImpl guards only rootContainer != null), and the one reachable way to call it on a half-open container
is a listener leak — filed separately as #993.

AttributeIndex.applyConfigurationChange publishes each part after the write which applied it

The three fields used to be swapped in one block between the first write and the second. They now go with the
write that earns them: the map of indexes and the indexing options once the deletion has committed, still inside
the entry container lock, and the configuration last, once the third write has committed and the entry limit has
been applied to the index types which stay. Since #997 that write only removes the TRUSTED flag of the index types
whose limit was raised, and is not opened at all when none was; the limit itself follows it, in memory.

The lock has drained every operation which enters through shared access, so the window in which the map names
trees the write has just deleted lies inside it and no search sees it. Publishing after write() rather than from
inside the WriteOperation also means an operation the storage replays publishes once, from the attempt which
committed.

EntryContainer.applyConfigurationChange: the transaction was a vestige

77131174b39 (2014-12-16) introduced the wrapper around transactional work — the opening of the subordinate
indexes and their entry limits; 9f0904fda8 (2015-04-24, OPENDJ-1725) took that work out of it when id2children
and id2subtree gave way to id2childrenCount, and the wrapper stayed. What remained inside it was two field
assignments — ID2Entry.setDataConfig and EntryContainer.this.config = cfg — with txn unused.

It is not a storage-availability probe either: on PDB in READ_ONLY, newStorageImpl() returns
ReadOnlyStorageImpl (:768), whose write is operation.run(this) with no transaction, and after close()
ReadOnlyEmptyStorageImpl (:870) does the same; TracedStorage.write adds nothing of its own for an operation
which never touches the transaction. No test referenced it.

So the write is removed, and the one step which can fail — newDataConfig(cfg) — now runs before the first
mutation, which makes the invariant structural rather than a matter of reading order.

The narrowing this causes, stated plainly: on JE, a call made while the storage is already closed used to
throw NullPointerException (JEStorage.beginTransaction reads envConfig, which close() sets to null) and be
reported as a failed dsconfig; it now applies the in-memory settings and returns SUCCESS. On a JE storage that
is open but read-only nothing changes: beginTransaction() already returned null, because that environment is
configured setTransactional(false). Read-only entry containers really do exist alongside the live ones —
BackendImpl.getReadOnlyRootContainer (:1204), used by exportLDIF (:625), verifyBackend (:732) and
BackendStat (:917) — and they register the same configuration listeners in their constructor.

The catch on that path is now close to unreachable: CryptoSuite.newParameters assigns a volatile field and
setDataConfig assigns a field. The message and the flag there are defensive, and the test reaches them by making
newDataConfig fail.

Tests

New ConfigChangeGivesUpTest (10 tests, groups precommit and pluggablebackend). PDBBackend is final, so the
test subclasses BackendImpl<PDBBackendCfg> and returns a decorator from configureStorage. The decorator runs
the operation inside the transaction the delegate opens and raises a checked exception from within it, so the
transaction is rolled back and the caller is handed the failure — which is exactly what the last attempt of a
bounded retry loop leaves behind. That models a spent bound without depending on the bound itself.

What is pinned:

• an index deletion, a VLV index deletion and a backend data change which give up: the result carries the right
  message id, names the index or the base DN, sets adminActionRequired, and the trees are still in
  storage.listTrees();
• an index change which gives up on the deletion still names the trees it could not delete, and one which gives up
  on the third write does not publish the configuration it could not apply — armed with a raised entry limit,
  which untrusts the index types which stay and so opens that write, where a lowered one does not since [#991] Decide and report an index configuration change outside the write which is replayed #997;
• the backend data change opens no transaction at all (the decorator counts writes);
• the success paths of all four ask for nothing, and the successful index change publishes every part of itself —
  the map, the indexing options and the configuration;
• ERR_CONFIG_INDEX_DELETE_FAILED claims that an index created again adopts the leftover trees and is trusted
  over their stale content: that claim is pinned, so it cannot become untrue in silence when Adding a backend index adopts the trees left behind by a previous index of the same attribute, keeps their TRUSTED flag, and reports nothing #990 is fixed.

Mutation checks were run by hand against the working tree: dropping setAdminActionRequired(true) at each site,
restoring the old position of the three-field swap, and restoring the removed storage.write each fail a test.

mvn -o -pl opendj-server-legacy -Pprecommit \
    -Dit.test='ConfigChangeGivesUpTest,ReplayedConfigChangeTest,PDBTestCase,JETestCase,OnDiskMergeImporterTest,PDBStorageTest' \
    -Dfailsafe.failIfNoSpecifiedTests=false \
    -Dtest=NoSuchSurefireTest -Dsurefire.failIfNoSpecifiedTests=false \
    -Dmaven.javadoc.skip=true verify

Tests run: 143, Failures: 0, Errors: 0, Skipped: 0

(rebased onto master at 1af0a1247d; ReplayedConfigChangeTest now carries the 20 cases #997 added, which pin the
count and the order of the three writes on the same road)

Adjacent defects found while doing this, filed separately

• Adding a backend index adopts the trees left behind by a previous index of the same attribute, keeps their TRUSTED flag, and reports nothing #990 — an index added over the trees a failed deletion left behind adopts them together with their TRUSTED
  flag and reports nothing. The two delete messages describe this behaviour, and the test above pins it.
• AttributeIndex.applyConfigurationChange reports its administrative actions from inside the WriteOperation, so a replayed write repeats them and can leave a stale index trusted #991 — createIndex / updateIndex report into the ConfigChangeResult from inside the WriteOperation,
  so a replay repeats the messages, and a replayed entry limit change can leave a stale index flagged TRUSTED.
  Fixed by [#991] Decide and report an index configuration change outside the write which is replayed #997, merged 2026-09-19; this PR is rebased over it.
• Changing confidentiality-enabled on a backend index applies nothing until the backend is restarted, and untrusts the index again on every later change #992 — DefaultIndex.setConfidential only compares, so confidentiality-enabled takes effect on no index
  until the backend is restarted, and untrusts the index again on every later change.
• A ConfigException while opening an EntryContainer leaves its five configuration listeners registered on a half-open container #993 — a ConfigException from EntryContainer.open leaves five configuration listeners registered on a
  half-open container. Fixed by [#993] Register an entry container's configuration listeners only once it has opened #999, merged 2026-09-19.

Closes #962.

[vharseko]

Code review

Found 1 issue:

1. The commit message and the PR description attribute the emptying of the storage.write wrapper in EntryContainer.applyConfigurationChange to the wrong commits. They say "77131174b39 added the wrapper, 20fdcbef0d and 7690cb21d0 took the transactional work out of it". 20fdcbef0d (2015-04-07) still has real transactional work inside that wrapper — openSubordinateIndexes(txn, cfg) and two id2children/id2subtree setIndexEntryLimit calls — and 7690cb21d0 (2015-03-14) predates it, so neither removed it. The commit that actually left only id2entry.setDataConfig(...) and EntryContainer.this.config = cfg behind is 9f0904fda8 (2015-04-24, OPENDJ-1725), which replaced id2children/id2subtree with id2childrenCount.

The conclusion the removal rests on is unaffected — the wrapper's body in the pre-PR code really is two field assignments with txn unused, and the in-code comment states that without naming commits — but the history in the permanent record is wrong.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java

Lines 2558 to 2568 in 3e3f94a

	try
	{
	// None of this is transactional: the entries and the indexes are handed the parameters to
	// encode with from now on, and neither of those is a record in a tree. The storage.write which
	// used to wrap it had a transactional body, removed long before the wrapper was; left behind,
	// it was a transaction a bounded storage could give up on, and giving up between the entries
	// and the indexes leaves the two encoded under settings which no longer agree. What can fail
	// is done first, before anything has been changed; publishing to the other threads is what the
	// entry container lock is held for here, and never was the transaction's doing.
	final String cipherTransformation = cfg.getCipherTransformation();
	final int cipherKeyLength = cfg.getCipherKeyLength();

[vharseko]

Requesting changes on the strength of the finding I left above.

The commit message and the PR description name the wrong commits for the emptying of the storage.write wrapper in EntryContainer.applyConfigurationChange. 20fdcbef0d still has real transactional work inside that wrapper and 7690cb21d0 predates it; the commit that left only id2entry.setDataConfig(...) and EntryContainer.this.config = cfg behind is 9f0904fda8 (2015-04-24, OPENDJ-1725).

The change itself stands — the wrapper's pre-PR body really is two field assignments with txn unused, and the in-code comment says so without naming commits. What needs correcting is the history that goes into the permanent record: the commit message and the PR description.

[vharseko]

@maximthomas this branch is reported as conflicting since today: #997 (b0970cc1a8, squash-merged 2026-09-19) restructured AttributeIndex.applyConfigurationChange, the method this change moves the publication of config in. Could you rebase it onto master? What the conflict is, and what #997 changed underneath, so that the rebase does not have to rediscover it:

The textual conflict is one hunk, at the tail of the third write. On master that write is opened only when there is something to untrust — if (!indexesToUntrust.isEmpty()) { … updatedIndex.setTrusted(txn, false) … } — and the entry limit is applied to the kept indexes in memory after it, by a loop of setIndexEntryLimit, not inside it; the updateIndex(updatedIndex, newConfiguration, ccr, txn) body this branch's hunk sits next to is gone (planIndexUpdates decides and reports the rebuilds before the first write). This branch puts config = newConfiguration right after that write, with the comment "the entry limit and the confidentiality this configuration declares are applied to the indexes which stay by the write above". The natural place now is after the setIndexEntryLimit loop, and the comment no longer holds as written: the write above removes the TRUSTED flag only, the limit is applied by the loop, and the confidentiality is applied by nothing on master — setConfidential only compares, the #992 defect this description names; #1000 (pending) is what applies it, with a give-up and a reopen write of its own.

The other two hunks of this branch in that file — the three-field block taken out from between the first and the second write, and indexingOptions / indexIdToIndexes published under the lock once the deletion has committed — merge on their own, and nothing in #997 argues against them.

One test loses its premise. anIndexChangeWhichGivesUpUpdatingDoesNotPublishWhatItCouldNotApply arms the third write with a lowered entry limit (4000 → 100) and an added presence index. On master a lowered limit untrusts nothing, so the third write is never opened, giveUpOnWrite(3) never fires, and the change succeeds — the case fails at isNotEqualTo(SUCCESS). It needs a change which does untrust a kept index to have a third write to give up on; a raised limit does. Its javadoc ("the third write is what applies the entry limit and the confidentiality") and the "write 3: the entry limit and the confidentiality never applied" row of the description want the same correction. The other index-change cases keep their premise: the first two writes are unconditional as before.

ReplayedConfigChangeTest, from #997, now pins the count and the order of those writes on the same road (writes() == before + 2 for a lowered limit — "the two writes which add and remove indexes, and no third" — and which write is armed), so it is worth running next to ConfigChangeGivesUpTest after the rebase.

Not a conflict yet, but next in line. #998 (approved, waiting) drops the leftover trees an index added again would adopt, and its description records the interlock with this PR: anIndexCreatedAgainAdoptsTheTreesAFailedDeletionLeftBehind pins the behaviour #998 removes, and messages 624–626 describe it, so whichever lands second inverts that test and corrects the three sentences. #998 also puts a drop write ahead of the write which opens the added indexes, which would move giveUpOnWrite(2) / (3) by one again. Ordinals 624–627 are still free on master; #998 took 628–629 around them.

The round-1 note on the commit history of the EntryContainer.applyConfigurationChange wrapper (9f0904fda8 rather than 20fdcbef0d / 7690cb21d0) still stands, and could go into the same push.

[maximthomas]

Rebased onto master (1af0a1247d) and force-pushed: 3e3f94ac69 → f39fffa615. Against the notes above:

The conflict. One hunk, the tail of the third write, resolved as you describe: master's structure is kept — the write is opened only when indexesToUntrust is non-empty, and the setIndexEntryLimit loop follows it — and config = newConfiguration now sits after that loop, the last step which applies a part of the configuration. The comment is rewritten to claim only what holds there: the limit reaches the kept index types once the write which untrusts them has committed, so a configuration published before that would declare a limit they do not hold yet. Nothing between the old position and the new one reads config — its readers are getAttributeType, isIndexed, isConfidentialityEnabled and getName, none of them called from inside the method. The other two hunks, and the EntryContainer / PDBStorage / backend.properties changes, merged on their own; the diff against master is the diff this PR had against its base, and 624–627 are still free (master's last ordinal is 623).

The test which lost its premise. anIndexChangeWhichGivesUpUpdatingDoesNotPublishWhatItCouldNotApply now arms the third write with a raised limit (4000 → 5000) rather than a lowered one; the javadoc and the comment say why. Both directions were measured on the rebased tree: with 5000 the class is 10/10 green; with the original 100 the case fails exactly as predicted — Expecting actual: Success not to be equal to: Success — the third write is never opened.

The history of the wrapper. Confirmed at the SHAs: the parent of 9f0904fda8 (d12a783ffe) still has openSubordinateIndexes(txn, cfg) and the two setIndexEntryLimit calls inside the write, 9f0904fda8 leaves id2entry.setDataConfig and EntryContainer.this.config = cfg alone, and 20fdcbef0d still has all three. The commit message now names 77131174b39 and 9f0904fda8 (OPENDJ-1725); the description is corrected the same way, together with the "write 3" row of the table, the sentence about when the configuration is published, and the tests bullet.

Runs on f39fffa615. ConfigChangeGivesUpTest,ReplayedConfigChangeTest,PDBTestCase,JETestCase,OnDiskMergeImporterTest,PDBStorageTest: 143 tests, 0 failures, 0 errors — ReplayedConfigChangeTest 20/20 next to ConfigChangeGivesUpTest 10/10, as suggested.

#998. Understood: whichever lands second inverts anIndexCreatedAgainAdoptsTheTreesAFailedDeletionLeftBehind, rewrites the three sentences of 624–626 and moves giveUpOnWrite(2)/(3) by one. Both are still open, so nothing here changes for that yet.

[vharseko]

Approving on f39fffa615. The round-1 request is met, the rebase is what the notes asked for, and what the change claims about itself holds at the SHAs and in the runs.

What changed since 3e3f94ac69. The range-diff of 2a7bb9d7ed..3e3f94ac69 against 1af0a1247d..f39fffa615 shows four deltas and nothing else: the commit message (history paragraph, the "write 3" bullet), the tail hunk of AttributeIndex.applyConfigurationChange (config = newConfiguration after the setIndexEntryLimit loop, comment rewritten), the copyright hunk of AttributeIndex.java gone because master carries that line since #997, and the test (5000, javadoc, comment). EntryContainer, PDBStorage, backend.properties and the other nine test cases are byte-for-byte round 1. merge-tree onto the current master d0082246b9 is clean; 624–627 are still free there (last ordinal 623, #1037 touched no properties).

The history, at the SHAs. 77131174b39^ (736f87ab0f) has the same body without a storage.write around it, so 77131174b39 did add the wrapper; 9f0904fda8^ (d12a783ffe) still has openSubordinateIndexes(txn, cfg) and the two setIndexEntryLimit calls inside it; 9f0904fda8 leaves id2entry.setDataConfig and EntryContainer.this.config = cfg. Commit message and description now name those two, with the right dates.

The new position of config. Between the old position and the new one there is the untrust write and the setIndexEntryLimit loop; the readers of config are open/close (listener registration), getAttributeType, isIndexed, isConfidentialityEnabled, getName and evaluateExtensibleFilter, none of them on that path — and the attribute type is the same in both configurations anyway, so the getAttributeType() in the catch reads nothing that differs.

The test premise. planIndexUpdates puts an index whose limit is raised into indexesToUntrust, so 4000 → 5000 opens the third write. Measured here as well: on f39fffa615, ConfigChangeGivesUpTest 10/10 and ReplayedConfigChangeTest 20/20 (reactor build up to opendj-server-legacy, JDK 26); with 5000 → 100 exactly the one case fails, at :236, Expecting actual: Success not to be equal to: Success, the other nine green. CI on the head: every build-maven leg green except ubuntu-latest, 17, still queued when I looked.

#998 and #1000 are both still open, so the interlock recorded above stands as written.