Skip to content

[#967] Keep the bookkeeping of a domain whose base entry is missing out of its configuration entry - #972

Merged
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/967-state-out-of-config-entry
Sep 11, 2026
Merged

vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/967-state-out-of-config-entry

Conversation

@vharseko

@vharseko vharseko commented Sep 8, 2026

Copy link
Copy Markdown
Member

Fixes #967.

The deadlock

disable() holds serviceStateLock across state.save(), and that save fell back to the
domain's own configuration entry whenever the base entry of the suffix was missing:

ResultCode result = runUpdateStateEntry(baseDN);
if (result == ResultCode.NO_SUCH_OBJECT)
{
  SearchResultEntry configEntry = searchConfigEntry();
  if (configEntry != null)
  {
    result = runUpdateStateEntry(configEntry.getName());   // under serviceStateLock
  }
}

A modify of a cn=config entry takes configLock, and ConfigurationHandler.replaceEntry()
calls every ConfigChangeListener of the entry while holding it. ConfigChangeListenerAdaptor
forwards unconditionally - it diffs nothing, so a ds-sync-state write which changes no
configuration property at all still runs the callback - and LDAPReplicationDomain is
registered on exactly that entry, with applyConfigurationChange() taking serviceStateLock.

T1  ServerStateFlush, or a dsconfig on the domain entry
      configLock          -> applyConfigurationChange -> WANTS serviceStateLock

T2  disable()
      serviceStateLock    -> state.save() -> config entry modify -> WANTS configLock

Both are plain monitors, so neither is released on a timeout: the import never gets past
processImportBegin, the configuration write never returns, and shutdown then blocks behind
them. The flush thread's if (!disabled && !ieRunning()) guard does not keep T1 out, because
disable() sets disabled only after state.save() has returned.

disable() has exactly two production callers, processImportBegin() and
processRestoreBegin(). Both notify before the backend is disabled, so the precondition is
strictly that the base entry is not there - a suffix configured for replication and waiting to
be initialized by an online import-ldif, which is the ordinary way a new replica is loaded
from LDIF.

enable() closes the same cycle from the other end, which the issue does not mention:
loadDataState() -> loadGenerationId() stores a generationId it had to compute, and
saveGenerationId() carried the same baseDN -> configuration entry fallback, under the same
lock. processImportEnd() and processRestoreEnd() call it, so fixing only disable() would
have left the headline scenario - a dsconfig on the domain while an import runs - alive.

The change

Break the cycle by never writing a configuration entry while serviceStateLock is held. The
other order is the configuration framework's and cannot be reversed.

  • PersistentServerState.updateStateEntry() no longer writes the state to the domain
    configuration entry when the base entry is missing.
  • saveGenerationId() no longer writes the generationId there either. A base entry which is
    not in the backend is also no longer reported as a failed write: it is the normal state of a
    suffix waiting to be initialized, and postOperation() stores the generationId again on
    every operation once the base entry has been deleted, so it would have logged
    ERR_UPDATING_GENERATION_ID for each of them. runUpdateStateEntry() already keeps quiet
    in the same case.
  • Both values are still read back from the configuration entry, by loadState() and by
    loadGenerationId(), so what a former version left there is not lost on an upgrade.
  • The order the two locks are taken in is written down on serviceStateLock, so the
    convenient fallback does not come back.

Nothing is checkpointed away by this. A suffix whose base entry is missing holds no entry at
all - a child cannot be added without its parent, and the base entry cannot be deleted while it
has children - so no change of this replica is in that state, and a change from another one
cannot be replayed into it either (and since #889, a replay which failed stays out of the
ServerState anyway). The generationId of an empty suffix is the checksum of a fixed export,
so loadGenerationId() computes the same value again for free on the next start. cn=schema,
the one domain whose base entry always exists, never reached the fallback.

Two things go away with it that are worth naming. Every fallback write reached
ConfigurationHandler.writeUpdatedConfig(), which rewrites config.ldif whole - digest,
possible config.manualedit-* copy and admin alert, LDIF export and rename - once a second for
as long as the state stayed dirty. And ds-sync-state is not in the MAY list of
ds-cfg-replication-domain; the write only ever got through because the operation is marked as
a synchronization operation, which turns schema checking off.

Testing

StateWithoutBaseEntryTest runs a domain over o=test with the backend initialized without
its base entry. Every production change was watched failing first, through a direct mutation of
the committed code:

disabled fails
the fallback in updateStateEntry() aStateSaveWithoutABaseEntryWritesNothingToTheConfigurationEntry - Expecting empty but was: [Attribute(ds-sync-state, {000001a0822a456a002a00000001})]
the fallback in saveGenerationId() aDomainWithoutABaseEntryWritesNoGenerationIdToItsConfigurationEntry - Expecting empty but was: [Attribute(ds-sync-generation-id, {48})]
the NO_SUCH_OBJECT guard on the error log aBaseEntryWhichIsNotThereIsNoFailureToStoreTheGenerationId

The log assertion matches the id of the message rather than its text, so it does not depend on
the locale the tests run under.

What the tests pin is the invariant which breaks the cycle - that no write to a configuration
entry leaves serviceStateLock - rather than the deadlock itself. Driving two monitors into
each other from a test would be the kind of timing construction that reports a false green when
the window is missed.

Green on JDK 21 (mvn -Pprecommit verify), 158 tests over 17 classes:
StateWithoutBaseEntryTest 3/3, PersistentServerStateTest 2/2, GenerationIdTest 4/4,
InitOnLineTest 10/10, ReSyncTest 2/2, SchemaReplicationTest 3/3, UpdateOperationTest
15/15, ReplicationDomainTest 12/12, ChangelogBackendTestCase 30/30,
AssuredReplicationPluginTest 14/14, FractionalReplicationTest 36/36,
GenerationIdChecksumTest 14/14, HistoricalTest 4/4, IsolationTest 1/1,
ReplicationServerFailoverTest 2/2, StateMachineTest 5/5, TopologyViewTest 1/1.

Not covered here

  • The configLock -> serviceStateLock order itself is untouched: a dsconfig on a domain
    entry still runs the domain's callback under the configuration backend's lock, and still has
    to. Filtering the callback out when no property changed would not have helped anyway - a real
    dsconfig during an import must take serviceStateLock.
  • [#916] Keep an update that lands during a ServerState save out of the saved flag #948 is affected: with no save() ever reaching configLock, the second cycle its saveLock
    opened is gone, and the tryLock() it uses to step around this deadlock can go back to a
    plain lock(). Whichever of the two lands first, the other wants a look at that.

…e entry is missing out of its configuration entry

An online import-ldif or restore on a replicated backend could deadlock against a write to
that domain's configuration entry. disable() holds serviceStateLock across state.save(), and
that save fell back to the domain configuration entry when the base entry of the suffix was
missing - which is what processImportBegin() and processRestoreBegin() run on. The write goes
through the configuration backend, which holds configLock while it calls every change listener
of the entry back, and LDAPReplicationDomain.applyConfigurationChange() takes serviceStateLock.
Two monitors, taken in both orders, neither released on a timeout.

enable() closed the same cycle from the other end: loadGenerationId() stores a generationId it
had to compute, and saveGenerationId() had the same baseDN -> configuration entry fallback.
processImportEnd() and processRestoreEnd() call it.

* PersistentServerState.updateStateEntry() no longer writes the state to the domain
  configuration entry when the base entry is missing.
* saveGenerationId() no longer writes the generationId there either, and a base entry which is
  not in the backend is no longer reported as a failed write: it is the normal state of a
  suffix waiting to be initialized, and postOperation() would report it on every operation.
* Both values are still read back from the configuration entry, so what a former version wrote
  there is not lost.
* The order the two locks are taken in is written down on serviceStateLock.

Nothing is checkpointed away by this. A suffix whose base entry is missing holds no entry at
all, so no change of this replica is in that state, and a change from another one can not be
replayed into it either; the generationId of an empty suffix is a constant loadGenerationId()
computes again for free.

It also stops config.ldif from being rewritten once a second - every fallback write reached
ConfigurationHandler.writeUpdatedConfig() - and keeps ds-sync-state, which the configuration
schema does not allow on ds-cfg-replication-domain, out of it.

Fixes OpenIdentityPlatform#967.
@vharseko
vharseko requested a review from maximthomas September 8, 2026 18:55
@vharseko vharseko added bug java replication concurrency Thread-safety / race-condition bugs tests Test suites: fixing, enabling, un-disabling and removed java labels Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug concurrency Thread-safety / race-condition bugs replication tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An import which disables a replication domain can deadlock against a write to that domain's configuration entry

2 participants