Skip to content

[#870] Stop replaying a rolled back read in the PersistIt backend - #871

Merged
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/870-pdb-read-replay
Aug 19, 2026
Merged

[#870] Stop replaying a rolled back read in the PersistIt backend#871
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/870-pdb-read-replay

Conversation

@vharseko

Copy link
Copy Markdown
Member

Fixes#870.

Problem

PDBStorage.read() replays a rolled back read for as long as it takes:

// opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:599finalTransactiontxn = db.getTransaction();
for (;;)
{
txn.begin();
try
{
finalTresult = operation.run(this);
txn.commit(commitPolicy);
returnresult;
}
catch (finalRollbackExceptione)
{
// retry
}

It implements what Storage.read asks for — "in case of a read operation rollback, implementations must ensure the read operation is retried until it succeeds" — on a premise the SPI never states. WriteOperation documents that an implementation "must be idempotent since operation might be retried"; ReadOperation says nothing of the kind, and four of them are not idempotent:

callerwhat a second attempt does
ExportJob.java:114entry.toLDIF() (:227) has already written entries 1..N-1, and LDIFExportConfig.getWriter() (:184-206) opens the stream once, so OVERWRITE truncates on the first attempt only: the replay appends. An LDIF with duplicates, reported as a successful export.
VerifyJob.java:130keyCount (:75) and attrIndexList (:103) are instance fields no attempt resets, so the count doubles and ERR_VERIFY_WRONG_ENTRY_COUNT (:394-397) fires on a healthy backend — which verify-index --countErrors returns as its exit code.
BackendStat.java:1278prints each record to out inside the read (:1337), so the replay prints what it already printed.
BackendStat.java:1103appends to undefinedKeys, a map owned by the caller; its own counters are locals and would reset, the key list would not.

Nothing keeps an export off a live backend either: ExportTask.java:363 takes no more than acquireSharedLock, which the running server itself holds.

Why it is latent rather than live

Checked against org.openidentityplatform.commons.persistit:core:2.1.5, which this build resolves: a read-only transaction has no path to a RollbackException.

  • Exchange$MvvVisitor.sawVersion is the only site on the MVCC path that creates one, and its lookupswitch on Usage sends FETCH to a branch that contains none; the TransactionIndex.wwDependency check followed by Transaction.rollback() and the throw sits in the STORE branch. The index-to-constant mapping is the synthetic Exchange$1.$SwitchMap.
  • Transaction.checkPendingRollback() is called from begin, commit, setStep, store, remove, removeTree, run and beginCheckpoint — not from fetch or traverse.
  • TimelyResource.getVersion throws only by way of addVersion, i.e. when a version has to be created; reads take their exchange with create == false (PDBStorage.java:371, 383).
  • txn.begin() sits outside the try, so even an inherited pending rollback would propagate rather than be replayed.

That leaves commit(), which throws only when a rollback is already pending, and only Transaction.rollback() marks one — reachable from write paths alone. So this PR changes no behaviour today. It removes a replay that is wrong whenever it does happen, and that the JDBC backend had to remove for real in #867, where SQL Server deadlocks reach reads and made it reachable.

Fix

PDBStorage.read() runs the operation once and propagates a rollback to the caller. RollbackException extends RuntimeException, so it now lands in the existing catch (final Exception e), which rolls back and rethrows; calling rollback() there after a commit() that threw is safe — the transaction is still active, end() runs in the finally, and a second rollback short-circuits on the pending flag rather than aborting twice.

This aligns the four backends: JEStorage.read() (:855) and CASStorage.read() (:189) never replayed, and JDBCStorage.read() stopped in #867. The contract in Storage.read is corrected to say so, and ReadOperation now states that an implementation need not be idempotent — which is what its callers have always assumed.

Deliberately not done: making the four callers replay-safe. It is the larger job for a trigger that does not exist, and the export cannot un-write the bytes it already handed to the LDIF stream.

Verification

No behavioural change to exercise — the removed branch is unreachable — so this is regression coverage only:

suiteresult
PDBTestCase34/34
PDBStorageTest3/3
TestImportAndExport12/12
TestRebuildTask3/3

TestImportAndExport is the suite that drives ExportJob over the changed read().

…ersistIt backend
PDBStorage.read() replayed a rolled back read until it succeeded, which is what
Storage.read asks for, on a premise the SPI never states. WriteOperation documents
that an implementation must be idempotent because it might be retried;
ReadOperation says nothing of the kind, and four read operations of this server are
not idempotent. ExportJob has already written entries 1..N-1 to an LDIF stream whose
writer is opened once, so a replay appends instead of truncating; VerifyJob
accumulates its counters in instance fields that no attempt resets, so a replay
trips ERR_VERIFY_WRONG_ENTRY_COUNT on a healthy backend; and the two reads of
BackendStat print records and append to a map owned by their caller.
The read now runs once and propagates a rollback to the caller. RollbackException
extends RuntimeException, so it lands in the existing catch, which rolls back and
rethrows: the transaction is still active there, end() runs in the finally, and a
second rollback short-circuits on the pending flag.
This changes no behaviour today. A read-only PersistIt transaction has no path to a
RollbackException: the FETCH branch of Exchange$MvvVisitor.sawVersion contains none,
checkPendingRollback is called from begin, commit, setStep and the write methods
rather than from fetch or traverse, reads take their exchange with create == false
so TimelyResource.addVersion is not entered, and begin() sits outside the try. Only
commit() can raise one, and only when a rollback is already pending, which none but
a write marks. The replay is removed because it is wrong whenever it does happen -
JDBCStorage.read() had to remove it for real, where SQL Server deadlocks reach reads.
Storage.read and ReadOperation are corrected to say that a read is not replayed and
its implementation need not be idempotent, which is what the callers always assumed
and what the JE and Cassandra backends already did.
@vharsekovharseko added bug java Pull requests that update java code concurrency Thread-safety / race-condition bugs labels Aug 18, 2026
@vharseko
vharseko merged commit d6a5316 into OpenIdentityPlatform:masterAug 19, 2026
18 checks passed
@vharseko
vharseko deleted the issues/870-pdb-read-replay branch August 19, 2026 08:37
vharseko added a commit to vharseko/OpenDJ that referenced this pull request Aug 20, 2026
master added the table stamping of OpenIdentityPlatform#866, which closes the comment connection of a
transaction in a finally block, while this branch wrapped the same transaction in a
retry loop. The transaction object is now bound per attempt and its stamp session
closed with it, so a replay stamps on a session of its own.
Both sides also grew a helper returning the driver name behind a connection -
driverNameOf() on master, getDriverName() here. Collapsed into master's name,
keeping the body of this branch, which tolerates a connection that did not come
from the pool.
The read() javadoc no longer says that Storage#read asks for a replay: OpenIdentityPlatform#871 turned
that contract around on master, and it now forbids one, which is what this
implementation already did.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugconcurrencyThread-safety / race-condition bugsjavaPull requests that update java code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PDBStorage.read() may replay a read whose body is not idempotent (export, verify, backendstat)

2 participants

@vharseko@maximthomas