Skip to content

Remove the unfixable testStateMachineFull and fix the dead replay pool - #700

Merged
vharseko merged 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/statemachine-full-test
Jul 8, 2026
Merged

Remove the unfixable testStateMachineFull and fix the dead replay pool#700
vharseko merged 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/statemachine-full-test

Conversation

@vharseko

Copy link
Copy Markdown
Member

Context

StateMachineTest.testStateMachineFull — a walk through all replication state machine transitions (NC→D→N→NC→N→D→NC→D→N→BG→NC→N→D→BG→FU→...) — has been disabled since the ForgeRock era. A deep attempt to revive it (part of the slow group audit) showed it cannot work in its historical form:

  • The degraded status only appears while the RS queues changes faster than the DS replays them (checkDSDegradedStatus samples the RS-side queue every 5 seconds against a threshold of 1). On modern hardware the publisher and the replay run at comparable rates, so every degraded transition is a race: bounded change batches get replayed before the analyzer samples a non-empty queue, and unbounded floods exhaust the 512m test JVM heap with real backend entries first (reproduced consistently).
  • On 2009-era hardware the replay was durably slower than the publisher, which is what the scenario silently relied upon.

The test and its private helpers (resetGenId, BrokerInitializer) are removed with an explanatory note in the class; the degraded/normal transitions remain covered deterministically by testStateMachineStatusAnalyzer.

Real bug found and fixed along the way

The class helper invoked applyConfigurationChange() with a bare Mockito mock. mock(ReplicationSynchronizationProviderCfg.class).getNumUpdateReplayThreads() returns 0 (Mockito's default for Integer, not null), so the null-check in getNumberOfReplayThreadsOrDefault() never fired and the call silently killed the shared replay thread pool of MultimasterReplication for every test in the class — a regression from the refactoring that replaced MultimasterReplicationFakeConf with a bare mock (thread dumps showed zero Replica replay threads; the DS listener queued updates nobody replayed, window acknowledgements never went out and the RS writer froze on acquirePermitInSendWindow).

  • The mock is now stubbed to use the default pool.
  • Product hardening: getNumberOfReplayThreadsOrDefault() falls back to the default for non-positive values — the configuration schema enforces a minimum of 1, so a zero can only come from a broken caller and would silently disable replication replay.

Verification

StateMachineTest: 2 consecutive runs, 5/5 green (~125s) — testStateMachineStatusAnalyzer now exercises a live replay pool for the first time.

testStateMachineFull (a walk through all replication state machine
transitions) has been disabled since the ForgeRock era. Investigation
showed it cannot work in its historical form: the degraded status only
appears while the replication server queues changes faster than the DS
replays them, which makes every degraded transition a race between the
publisher and the replay on modern hardware, and the unbounded floods
needed to force it exhaust the 512m test JVM heap with real backend
entries before the status analyzer (5 second period) can observe a
non-empty queue with the degraded threshold of 1. Remove the test and
its private helpers; the degraded/normal transitions remain covered
deterministically by testStateMachineStatusAnalyzer.
The investigation also uncovered a real bug in the test helper shared
by the whole class: applyConfigurationChange() was invoked with a bare
Mockito mock whose getNumUpdateReplayThreads() returns 0 (Mockito's
default for Integer, instead of null), silently killing the shared
replay thread pool of MultimasterReplication - a regression from the
refactoring that replaced MultimasterReplicationFakeConf with a mock.
Stub the mock so that the default pool is used, and harden
getNumberOfReplayThreadsOrDefault() to fall back to the default for
non-positive values, since a zero would silently disable replication
replay.
@@ -1,3 +1,4 @@
/*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line should be removed

Suggested change
/*

@vharsekovharseko added tests Test suites: fixing, enabling, un-disabling replication labels Jul 6, 2026
@vharseko
vharseko merged commit dc20037 into OpenIdentityPlatform:masterJul 8, 2026
28 checks passed
@vharseko
vharseko deleted the features/statemachine-full-test branch July 8, 2026 16:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CIreplicationtestsTest suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@vharseko@maximthomas