Skip to content

[#1036] Assert that a parked change is handed out, not which thread hands it out - #1037

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/1036-handout-test-thread-race
Open

vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/1036-handout-test-thread-race

Conversation

@vharseko

Copy link
Copy Markdown
Member

Fixes #1036.

Two tests of #958 - aChangeHandedOutAsADependencyIsGivenBackWhenItsReplayIsUnwound and
theChangesParkedBehindAChangeWhoseAckFailedAreReplayed - assert isSameAs(replayingParent):
the child parked behind the parent must be replayed by the thread which committed the parent.
getNextUpdate() never guaranteed that. It hands a parked change to whichever replay thread calls
it first once the changes before it have left pendingChanges, and the thread which parked the
child calls it on its own way out, right after addDependency(). The parent commits in the
post-operation plugin, inside op.run(), so a parker delayed past the parent's write finds the
parent gone and takes the child back itself. That is the 0.26 s failure on #981 and #964 - and,
because the assertion sits before the by-hand redelivery of a child no replication server owns,
the child then stays uncommitted at the head of pendingChanges and the remaining 17 tests of the
class time out behind it.

The change

Test tree, plus one javadoc:

  • Both tests wait for the child to be seen parked (dependent-changes-size) before the parent
    is released. That is the property they are about - a parked change leaves by getNextUpdate()
    and by no other road - and it closes a second hole the thread check hid: released on the spot,
    the parent could commit before the child was taken off the queue at all, and the child would be
    replayed from the queue with nothing to wait for, a pass which proves nothing.
  • The assertion on which thread hands the child out is dropped, and the javadoc of both tests says
    why. The Replication: an Error in a replay kills a replay thread the pool never replaces #923 liveness assertion is made on the thread which met the Error, whichever one it was.
  • getNextUpdate()'s javadoc says what it guarantees: first caller once the changes before it are
    gone - as a rule the thread which cleared the dependency, but the parker itself when the clearing
    landed before it got there.

Testing

run result
mutant: the parker reaches getNextUpdate() 300 ms late, every other caller 500 ms late - original tests 🔴 2/2 - Replica replay thread 1 vs thread 0 "to refer to the same object", the CI failure verbatim, second test cascading
same mutant - tests of this PR 🟢 2/2 - the parker takes the child back, which is the road CI failed on
no mutant, whole UpdateOperationTest 🟢 31/31, 129 s

The mutant is not part of the PR. The branch sits directly on master at 776339a, one commit.

…, not which thread hands it out

Fixes OpenIdentityPlatform#1036.

getNextUpdate() hands a parked change to whichever replay thread calls it
first once the changes before it have left the pending changes, and the
thread which parked it calls it on its own way out - so the two OpenIdentityPlatform#958 tests
which asserted isSameAs(replayingParent) failed whenever the parent's
post-operation commit landed before the parker got there, and the child,
which no replication server owns, then held the ServerState back for the
rest of the class.

Both tests now wait for the child to be seen parked before the parent is
released - a parked change leaves by getNextUpdate() and by no other road,
which is the hand-out they are about - and no longer assert which thread
hands it out. The OpenIdentityPlatform#923 liveness assertion is made on the thread which met
the Error. The javadoc of getNextUpdate() says what it guarantees.
@vharseko vharseko added bug replication tests Test suites: fixing, enabling, un-disabling concurrency Thread-safety / race-condition bugs labels Sep 13, 2026
@vharseko

Copy link
Copy Markdown
Member Author

@maximthomas could you take this one ahead of the queue? It is test-only - two assertions dropped, two waits added, one javadoc - one commit on master, MERGEABLE, 24/24 checks green - and the failure it fixes has become the single largest cost of CI on the replication PRs.

What it is costing

The issue was filed on two hits (#981, #964, 2026-09-12). Since then the same assertion has taken down a leg on every day the replication PRs ran, on branches which touch nothing near it:

day PR under test leg first failure, then the cascade
09-15 #982 ubuntu-latest, 26 thread 20 vs 19 at 0.38 s, 14 of 35
09-15 #1019 ubuntu-latest, 11 thread 20 vs 19 at 0.27 s, 18 of 31
09-15 #1049 ubuntu-latest, 26 thread 21 vs 19 at 0.24 s, 18 of 32
09-16/17 #982 ubuntu-latest, 26 thread 20 vs 19 at 0.26 s, 15 of 39
09-16/17 #988 ubuntu-latest, 21 and 26 thread 20 vs 19 at 0.24 s, 20 of 35 on both
09-16/17 #1019 ubuntu-latest, 25 thread 20 vs 19 at 0.26 s, 17 of 33
09-16/17 #1057 ubuntu-latest, 26 thread 20 vs 19 at 0.25 s, 18 of 33

Ten legs in five days. Every one of them is the same shape - aChangeHandedOutAsADependencyIsGivenBackWhenItsReplayIsUnwound red in a quarter of a second on isSameAs(replayingParent), and because that assertion sits ahead of the by-hand redelivery, the child stays uncommitted at the head of pendingChanges and the rest of UpdateOperationTest times out behind it: 15 to 20 cases at 60 to 250 s each, 19 to 31 minutes of a leg spent on nothing, then the whole 2-hour leg run again. #1019 and #1057 do not touch LDAPReplicationDomain or the replay at all; they were hit because UpdateOperationTest runs in every leg.

Two things make it worse than a flake to re-run. The reruns are what a reviewer sees first: a red UpdateOperationTest on a replication PR is where a real regression would show, and a class which is red once a day for a known reason is one nobody reads closely anymore. And it is not independent of the queue: #985 and #988 add cases to the same class, and every rebase of the stack re-runs the roulette on every leg.

What the PR does

Nothing in main moves but a javadoc. getNextUpdate() hands a parked change to whichever thread calls it first once the changes ahead of it are gone; the two #958 cases asserted that this is the thread which committed the parent, which it usually is and never was guaranteed to be - the parker itself calls getNextUpdate() on its way out, and on a loaded runner it gets there after the parent's write. The cases now wait for the child to be seen parked before the parent is released, which is the property they are about and closes a second hole the thread check hid, and assert that the child is handed out rather than by whom. The mutant which reproduces the CI failure verbatim is in the description: red 2/2 on the original cases, green 2/2 on these.

Every day this waits is another row in that table.

@vharseko vharseko added the java Changes to Java sources label Sep 17, 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 java Changes to Java sources replication tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The #958 hand-out tests assert which thread replays a parked change, which getNextUpdate() does not guarantee

1 participant