Stabilize flaky TransactionIndexTest.testDeadlockedWwDependency

[vharseko]

Problem

TransactionIndexTest.testDeadlockedWwDependency is flaky. It builds a three-transaction deadlock cycle (ts1 <- ts3 <- ts2 <- ts1) and asserted a specific thread would win the deadlock-detection race:

t3.join();
assertEquals("Deadlock not detected", UNCOMMITTED, result3.get()); // t3 closes the cycle
t1.join();
ts2.abort();
ti.notifyCompleted(ts2, ABORTED);
t2.join();
assertEquals(0, result2.get());                                    // t2 clears after ts2 aborts

But deadlock detection is distributed: every waiter re-checks for a cycle roughly every SHORT_TIMEOUT (10 ms), and the cycle is only broken in a finally block after the return value is decided. So any transaction on the cycle — not only the one that closes it — may report the deadlock, and more than one may do so within the brief window before the cycle is cleared. Under load a second thread (t2) detected the deadlock before ts2 was aborted, so result2 came back UNCOMMITTED instead of 0:

junit.framework.AssertionFailedError: expected:<0> but was:<9223372036854775807>
    at com.persistit.TransactionIndexTest.testDeadlockedWwDependency(TransactionIndexTest.java:266)

(9223372036854775807 is TransactionStatus.UNCOMMITTED.) Observed on build-maven (ubuntu-latest, 17); the same module passed on the other JDKs in the same run, confirming a timing flake rather than a real regression.

Fix

Assert the invariants that always hold instead of pinning the outcome to a particular thread:

• the deadlock is detected by at least one participant (result1 | result2 | result3 == UNCOMMITTED);
• ts3's wait on ts2 ends either by detecting the deadlock or by observing ts2's abort (result2 ∈ {0, UNCOMMITTED}) — both are correct per the wwDependency contract, since ts3 really is on the deadlock cycle;
• each waiter blocks until the cycle forms (elapsed1, elapsed2 >= 900).

The join/abort/notifyCompleted sequence is unchanged; only the assertions are adjusted (they have no side effects). The removed elapsed3 < 1000 check carried the same "t3 wins the race" assumption that caused the flake. A stricter regression (TIMED_OUT, or a bogus positive commit timestamp) still fails the test.

Verification

• testDeadlockedWwDependency run 20×: 20/20 pass.
• Same test run 8× under full CPU saturation (all cores busy): 8/8 pass.
• Full TransactionIndexTest class: 8 tests, 0 failures.

[maximthomas]

Review: request changes

The diagnosis is correct and the central relaxation is sound. But the stated motivation doesn't hold up against this repo's build config, and the change drops more coverage than it needs to.

What's right

The root-cause analysis matches the implementation. In TransactionIndex.wwDependency (persistit/core/src/main/java/com/persistit/TransactionIndex.java:850-903), each iteration calls
source.setDepends(target) then isDeadlocked(source), and the edge is cleared only in the finally at line 899 — after the return value is decided. Every transaction on the cycle can therefore
observe it, and more than one can return UNCOMMITTED before the cycle is broken. The old assertEquals(0, result2.get()) was genuinely unsound.

result2 ∈ {0, UNCOMMITTED} is also a sound relaxation, for a reason the description doesn't state: t2's timeout is 1,000,000 ms so it cannot return TIMED_OUT, and its target ts2 is aborted, so
the only non-deadlock exit is tc == ABORTED → 0 at line 867. Those really are the only two outcomes.

Blocking

1. This test never runs, so the cited failure can't be from CI here.

persistit/core/pom.xml:96 hardcodes surefire <skipTests>true</skipTests>, added in 6224b3d1c ("PDB deprecate build test", Sep 2021). It survives into the effective POM, and
.github/workflows/build.yml:36 only runs mvn package. TransactionIndexTest is not executed by any workflow in this repository.

So the quoted failure on build-maven (ubuntu-latest, 17) — and "the same module passed on the other JDKs in the same run" — cannot have come from this repo's CI on master; the module skips its tests
on every JDK. The fix also has no effect on CI as things stand.

Separately: <skipTests> is a literal true, not ${skipTests}, so -DskipTests=false won't override it. How was the "run 20×" verification performed?

Please either link the actual failing run, or remove <skipTests>true</skipTests> from persistit/core/pom.xml in this PR so the fix does something and CI validates it.

2. t3 is now entirely unasserted.

result3 survives only inside the three-way disjunction, and elapsed3 is written by the helper and never read — a dead variable. A regression where the cycle-closing waiter never detects the deadlock
(returns TIMED_OUT after its full 10 s) while t1 happens to detect would still pass. elapsed3 < 1000 was the only latency guard on detection at all: raising SHORT_TIMEOUT from 10 ms to 5 s
would keep this test green.

The justification for removing it doesn't hold. elapsed3 < 1000 never assumed t3 wins the race — it only said that if t3 detects, it detects promptly. That's preservable with no race at all:

if (result3.get() == UNCOMMITTED) {
    assertTrue("deadlock detection must be prompt", elapsed3.get() < 1000);
}

This is safe precisely because of the mechanism you describe: once any waiter returns UNCOMMITTED, its finally { source.setDepends(null); } breaks the cycle permanently, so t3 can no longer detect
it and returns TIMED_OUT instead. There is no interleaving in which t3 detects the deadlock slowly.

Related side effect: in the interleavings this PR now tolerates, t3 blocks its full 10 s timeout, and t3.join() is the first thing the test does. Runtime becomes bimodal (~1 s vs ~10 s), with
nothing left to observe it.

3. The surviving assertion is still timing-dependent.

The comment and description present result1 | result2 | result3 == UNCOMMITTED as an invariant that "always holds." It doesn't. The cycle exists only between t≈1000 ms (when t3 starts) and t≈2000 ms
(when t1's timeout expires and its finally nulls ts2's edge for good). Within that window each waiter also nulls its own depends between iterations, so a complete cycle is only intermittently
visible. A stall long enough to swallow that window — GC, CI contention, the same conditions that produced the original flake — means nobody detects, and assertTrue("Deadlock not detected", ...)
fails.

Much less likely than the old assertion, which is a real improvement. But say so rather than claiming an invariant. Raising t1's 2000 ms timeout would widen the window cheaply.

Should fix

4. Diagnosability regressed. assertEquals → assertTrue discards the actual values. For a change whose whole purpose is coping with a rare, hard-to-reproduce failure, the next failure will print
only Deadlock not detected with nothing to go on:

final String state = " (result1=" + result1 + ", result2=" + result2 + ", result3=" + result3 + ")";
assertTrue("Deadlock not detected" + state, ...);

5. The verification doesn't exercise the branches it claims to fix. Twenty passing runs almost certainly all took the happy path (t3 detects, ~1 s). The interleavings that motivated the change
force t3 to block its full 10 s timeout and would be obvious in per-run wall time. The new assertions are unverified in exactly the branches they exist for. Report per-run timings, or force the
interleaving.

Nits

License header doesn't match convention. 141 files in this repo use exactly this form — appended at the end of the block, no trailing period, no blank separator line:

 * limitations under the License.
 * Portions Copyrighted 2026 3A Systems, LLC
 */

Zero use a trailing period. This PR places the line near the top, adds a period, and inserts two blank * lines. (license-maven-plugin is configured at persistit/core/pom.xml:83-87, header check
currently commented out in the parent, but the convention is unambiguous.) No persistit test file currently carries a 3A header at all.

Unrelated whitespace churn. Line 3 changes * → * while lines 7, 9 and the rest keep their trailing spaces, leaving the header internally inconsistent and adding diff noise.

Arrow notation contradicts itself. The block comment writes the cycle as ts1 <- ts3 <- ts2 <- ts1 (reading <- as "waits for"), while the assertion message six lines below writes ts3->ts2 for
the same relation. Pick one; A -> B meaning "A waits for B" is the standard reading.

The comment is disproportionate and will rot. Eighteen lines of comment for five lines of assertion. It hard-codes SHORT_TIMEOUT (10 ms), which goes stale when the constant changes, and embeds CI
provenance ("observed on ubuntu-latest JDK 17") that already lives in the commit message. Four or five lines stating the invariant would serve better.

Mixed reference style. New code uses the static import UNCOMMITTED while the untouched line just above still reads TransactionStatus.ABORTED. Since you're editing this block, unify them.

Summary

1. Link the failing CI run, or re-enable tests for persistit/core so this fix has effect.
2. Restore the detection-latency bound conditionally on result3 == UNCOMMITTED.
3. Include the observed values in the assertTrue failure messages.
4. Fix the license header to match the existing convention.
5. Soften the "always holds" claim, or widen the window by raising t1's timeout.

[vharseko]

Thanks — thorough and accurate. Addressed in 08a8bba.

1. "This test never runs" — resolved, though not by this PR. skipTests=true was removed from persistit/core/pom.xml by #240 (701221a75, merged earlier the same day, ~6h before this review), and this branch already contains it. So on current master the workflow's mvn package does run the module's tests — confirmed in a completed run: the JDK-25 build-maven job logs Running com.persistit.TransactionIndexTest and Tests run: 565 for persistit/core. The fix therefore has effect and CI validates it; no pom change needed here. With the literal <skipTests> gone, mvn -pl persistit/core test -Dtest=TransactionIndexTest also works, which is how the verification below was run.

2. t3 unasserted / elapsed3 dead — fixed. Restored the latency bound, guarded on the detection actually happening:

if (result3.get() == UNCOMMITTED) {
    assertTrue("deadlock detection must be prompt" + state, elapsed3.get() < 1000);
}

As you note, this is sound precisely because once any waiter returns UNCOMMITTED its finally { source.setDepends(null); } breaks the cycle permanently, so t3 cannot detect it slowly — it detects soon after the cycle forms or times out. elapsed3 is read again.

3. Surviving assertion is still timing-dependent — reworded + window widened. Dropped the "always holds" claim (now "highly likely but not strictly guaranteed under a long stall"). I also took the cheap widening: t1's timeout 2000 → 5000. This matters for exactly the reason the mechanism implies — the cycle is live only until t1 times out and permanently clears its ts2 -> ts1 edge, so the detection window is [t3 start, t1 timeout]. Widening it 1 s → 4 s gives t3 ~4× the retries and needs a ~4 s stall (not ~1 s) to make nobody detect. Cost is ~3 s/run since t1 is not the closing edge and blocks its full timeout; acceptable for an anti-flake test.

4. Diagnosability — fixed. Every assertTrue now appends state = (result1=…, result2=…, result3=…, elapsed3=…).

5. Verification exercises the relevant branch. Re-ran testDeadlockedWwDependency 12× on JDK 26 under full CPU saturation (all 8 cores busy): 12/12 pass, wall time a consistent ~5 s — i.e. t3 detects on its first iteration (result3 == UNCOMMITTED, elapsed3 < 1000), so the restored conditional bound is actually evaluated every run, and t1 then blocks its full 5 s. Plus the whole class via Maven on JDK 11: Tests run: 8, Failures: 0. I could not force the rare t3-times-out interleaving (~10 s) even under saturation; noted honestly rather than claimed.

Nits — all applied. License header now matches the repo convention (single line appended at the end of the block, no trailing period, no blank * separators, line-3 whitespace churn reverted); arrow notation unified on A -> B = "A waits for B" in both the comment and the message; comment trimmed to the invariant (no hardcoded 10 ms, CI provenance moved to the commit message); ABORTED now uses the existing static import.

[maximthomas]

Verified — claims that hold, no action

• skipTests. My round-1 objection ("this test never runs") was wrong, based on a stale local
  checkout. <skipTests>true</skipTests> is absent from persistit/core/pom.xml on origin/master,
  and 701221a75 (#240) is an ancestor. The module's tests run in CI. Withdrawn.
• License header. Placement matches repo convention exactly (cf.
  persistit/core/src/main/java/com/persistit/Persistit.java:15).
• Static imports. ABORTED / UNCOMMITTED now consistent with the rest of the file
  (lines 71, 75, 88, 105).
• Cycle notation. ts1 -> ts3 -> ts2 -> ts1 is correct given tryWwDependency's reversed
  (target, source) parameter order, and the "ts3 -> ts2" message on the result2 assertion is
  correct.
• Assertion 2's value domain. result2 ∈ {0, UNCOMMITTED} is exhaustive: t2's 1000000 ms
  timeout rules out TIMED_OUT, and ts2 aborts so no positive tc.
• All 11 CI checks pass; mergeStateStatus: CLEAN.

Finding 1 (blocking) — restored elapsed3 < 1000 conflicts with 2000 -> 5000

TransactionIndexTest.java:288 vs :254.

The mechanism: in TransactionIndex.wwDependency (lines 850-902), source.setDepends(null) sits in
the finally of every loop iteration, and source.setDepends(target) re-arms it at the top of
the next one. So t1 keeps re-arming the ts2 -> ts1 edge every ~SHORT_TIMEOUT until its
do/while exits at t1start + timeout. The cycle is therefore visible to t3 from wall ≈1000 ms
until wall ≈ t1's timeout.

• With t1 timeout = 2000, a detection by t3 implied elapsed3 ≲ 1010. The bound was structural.
• With t1 timeout = 5000, a legitimate detection by t3 can occur at elapsed3 up to ≈4010.
  assertTrue(elapsed3.get() < 1000) can now fail on a correct run.

The stated justification — "once any waiter returns UNCOMMITTED its finally breaks the cycle for
good, so if t3 detects at all it must be prompt" — is unsound, and it was my reasoning, not the
author's. It presumes some waiter returns UNCOMMITTED early. But nobody can see the cycle until
t3 executes source.setDepends(target): t3 supplies the ts1 -> ts3 edge. So if t3's thread
stalls (GC pause, CPU starvation on a loaded runner) for >1 s between the helper's
start = System.currentTimeMillis() and its first isDeadlocked call, then by construction no
other waiter has detected either; the cycle is still live (now for 4 s instead of 1 s); t3 detects;
elapsed3 >= 1000; the assertion fails.

That is a single-thread stall, not a conspiracy of missed checks — the same class of assumption the
PR exists to remove, made ~4x more reachable by the PR's own timeout widening. The code comment at
:270 already states the refutation ("The cycle is live only between t3's start and t1's timeout")
directly above the comment that contradicts it.

Fix: drop the elapsed3 < 1000 guard. It constrains the scheduler, not the product. The
structural bound (elapsed3 < 4000) is implied by t1's timeout and asserts nothing. If a
regression guard on t3 is wanted, assert the value domain instead:

assertTrue("ts1 -> ts3 must detect the deadlock or time out" + state,
        result3.get() == UNCOMMITTED || result3.get() == TIMED_OUT);

which still catches a bogus positive commit timestamp or a spurious 0.
(TransactionStatus.TIMED_OUT is package-private; the test is in com.persistit.)

Finding 2 (blocking) — state omits elapsed1 / elapsed2

TransactionIndexTest.java:275. The state string carries result1..3 and elapsed3, but the
final two assertions test elapsed1 and elapsed2, so their failure messages print everything
except the value that failed. Also relevant: tryWwDependency only catches IllegalArgumentException
and InterruptedException, so an unexpected RuntimeException (e.g. IllegalStateException "Commit incomplete") kills the waiter and leaves elapsed at -42 — precisely the case where the
message needs to show it. Add elapsed1 and elapsed2 to state.

Finding 3 (should fix) — PR description is stale

The body still says "The removed elapsed3 < 1000 check carried the same 't3 wins the race'
assumption that caused the flake" — but 08a8bba8 restored it. It says "Assert the invariants that
always hold", while the code comment now correctly says "highly likely but not strictly
guaranteed". Verification says 20x; the follow-up comment says 12x. Update the body so it describes
the head commit.

Finding 4 (nit) — result1 is under-constrained

Only the three-way disjunction touches it; its expected value is TIMED_OUT. Mirror the result2
assertion: result1 ∈ {TIMED_OUT, UNCOMMITTED}.

Finding 5 (optional, design) — the +3 s is avoidable and buys less than a restructure

2000 -> 5000 costs ~3 s on every run (t1 cannot detect once t3 broke the cycle, so it blocks its
full timeout) and buys a 4 s detection window. It exists only because the test unblocks t1 by
letting it time out. Completing ts1 instead — which is also the semantically correct response to
t3 reporting the deadlock, since ts1 is the transaction told to abort — gives a 10 s window
(bounded by t3's own timeout) at ~1 s runtime, and restores a single expected value for both
result1 and result2:

final Thread t1 = tryWwDependency(ti, ts1, ts2, 1000000, result1, elapsed1); // ts2 -> ts1
final Thread t2 = tryWwDependency(ti, ts2, ts3, 1000000, result2, elapsed2); // ts3 -> ts2
Thread.sleep(1000);
final Thread t3 = tryWwDependency(ti, ts3, ts1, 10000, result3, elapsed3);  // ts1 -> ts3
t3.join();
ts1.abort(); ti.notifyCompleted(ts1, ABORTED);
t1.join();   // t1's target aborted -> 0
ts2.abort(); ti.notifyCompleted(ts2, ABORTED);
t2.join();   // t2's target aborted -> 0

elapsed1 >= 900 still holds (t1 blocks wall 0 -> ~1010). If the author prefers the minimal change,
reverting the timeout to 2000 and dropping the elapsed3 guard is acceptable.