A change is owned by the replay thread which took it, from RemotePendingChanges.markInProgress() until it is committed or released. That ownership is what keeps a change being replayed from being replayed a second time (OPENDJ-1115): putRemoteUpdate() refuses every delivery of a change a replay thread owns.
Ownership is released only on the roads which run to their end - a commit, recoverFromReplayFailure(), abandonReplay(). Anything which escapes LDAPReplicationDomain.replay() between markInProgress() and one of those leaves the change owned by a thread which is not replaying it anymore:
The change then stays listed and uncommitted with nobody able to replay it: every redelivery is refused as a duplicate, so this domain's ServerState - and every change behind it, from every master - stops advancing for as long as the server is up. Before #892 the same crash healed by accident, because putRemoteUpdate() overwrote the listed copy unconditionally; the ownership check that PR adds is what closes that door, which makes this worth opening a real one.
Why this is not a two-line fix
A safety net which gives the change back on the way out was written and reverted during the review of #892, because the net alone is not sound: RemotePendingChanges.replayFailed(csn) clears the mark without checking who set it, so a stale release takes ownership away from whatever thread owns the change now - the double replay the ownership exists to prevent. Every placement of the net traded one hazard for the other:
- release before the failure road runs → a throw inside that road leaves the change owned (the wedge again);
- release after it → a throw after the road already released the change hands it to a second thread.
The fix that removes the class of hazard is to give ownership an owner: PendingChange records the thread markInProgress() handed it to, and replayFailed() is a no-op for anyone else. A give-back on the way out is then safe wherever it is put, including for a change parked in dependentChanges.
Scope
PendingChange: the owner rather than a boolean, @GuardedBy the pending changes write lock as the rest of its state is.RemotePendingChanges.replayFailed(): no-op for a non-owner.LDAPReplicationDomain.replay(): give the change back on any throw, and take the ordinary failure road with it so that it keeps its give-up budget and the session restart keeps its backoff.- A test: a message whose
createOperation() throws an Error, then the delivery which follows must be able to replay the change. It fails by timing out without the give-back.
Found while reviewing #892; the code there is correct as it stands, this is the hole it makes reachable rather than one it opens.
A change is owned by the replay thread which took it, from
RemotePendingChanges.markInProgress()until it is committed or released. That ownership is what keeps a change being replayed from being replayed a second time (OPENDJ-1115):putRemoteUpdate()refuses every delivery of a change a replay thread owns.Ownership is released only on the roads which run to their end - a commit,
recoverFromReplayFailure(),abandonReplay(). Anything which escapesLDAPReplicationDomain.replay()betweenmarkInProgress()and one of those leaves the change owned by a thread which is not replaying it anymore:Error-ReplayThread.run()catchesException, so it unwinds the thread (see Replication: an Error in a replay kills a replay thread the pool never replaces #923);finallywhich publishes the ack, on a session which is being torn down.The change then stays listed and uncommitted with nobody able to replay it: every redelivery is refused as a duplicate, so this domain's ServerState - and every change behind it, from every master - stops advancing for as long as the server is up. Before #892 the same crash healed by accident, because
putRemoteUpdate()overwrote the listed copy unconditionally; the ownership check that PR adds is what closes that door, which makes this worth opening a real one.Why this is not a two-line fix
A safety net which gives the change back on the way out was written and reverted during the review of #892, because the net alone is not sound:
RemotePendingChanges.replayFailed(csn)clears the mark without checking who set it, so a stale release takes ownership away from whatever thread owns the change now - the double replay the ownership exists to prevent. Every placement of the net traded one hazard for the other:The fix that removes the class of hazard is to give ownership an owner:
PendingChangerecords the threadmarkInProgress()handed it to, andreplayFailed()is a no-op for anyone else. A give-back on the way out is then safe wherever it is put, including for a change parked independentChanges.Scope
PendingChange: the owner rather than a boolean,@GuardedBythe pending changes write lock as the rest of its state is.RemotePendingChanges.replayFailed(): no-op for a non-owner.LDAPReplicationDomain.replay(): give the change back on any throw, and take the ordinary failure road with it so that it keeps its give-up budget and the session restart keeps its backoff.createOperation()throws anError, then the delivery which follows must be able to replay the change. It fails by timing out without the give-back.Found while reviewing #892; the code there is correct as it stands, this is the hole it makes reachable rather than one it opens.