Stabilize flaky TransactionIndexTest.testDeadlockedWwDependency

[vharseko]

Problem

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

t3.join();
assertEquals("Deadlock not detected", UNCOMMITTED, result3.get()); // assumes t3 detects
...
assertEquals(0, result2.get());                                    // assumes t2 clears, not detects

Deadlock detection is distributed: in TransactionIndex.wwDependency each waiter calls source.setDepends(target) then isDeadlocked(source) every SHORT_TIMEOUT, and the edge that reveals a cycle is nulled in a finally only after the return value is decided. So any transaction on the cycle — not just the one that closes it — may return UNCOMMITTED, and more than one can. Under load a second waiter detected the deadlock before its target was aborted, so its result was UNCOMMITTED instead of 0:

junit.framework.AssertionFailedError: expected:<0> but was:<9223372036854775807>   (UNCOMMITTED)

Observed on build-maven (ubuntu-latest, 17).

Fix

Stop pinning the outcome to a particular thread, and resolve the cycle the way the deadlock dictates:

• Resolve by aborting each waiter's target, starting with ts1 — the transaction the deadlock tells to abort. This unblocks t1 at ~1 s instead of forcing it to time out, so both waiter timeouts stay at 1 000 000 ms, runtime is ~1 s (no bimodal 1 s/5 s), and the detection window widens to t3's own 10 s timeout.
• Assert value domains rather than exact per-thread outcomes:
  • a deadlock is detected by at least one participant (result1 | result2 | result3 == UNCOMMITTED) — highly likely, though the comment notes it is not strictly guaranteed under a long stall (the cycle is only live from t3's start until the first waiter exits);
  • t1/t2 cannot time out and their targets abort, so result1, result2 ∈ {0, UNCOMMITTED};
  • t3 either detects the deadlock or, if it stalled past the cycle's lifetime, times out: result3 ∈ {UNCOMMITTED, TIMED_OUT};
  • each waiter blocks until the cycle forms (elapsed1, elapsed2 >= 900).
• Failure messages carry a state string with all result*/elapsed* values for diagnosability.

No detection-latency (elapsed3 < 1000) guard: with a real detection window that can legitimately span several seconds, such a bound constrains the scheduler, not the product. A bogus positive commit timestamp or a spurious 0 from t3 still fails the domain assertions.

Verification

• testDeadlockedWwDependency run 10×: 10/10 pass, ~1.05–1.13 s each (the restructure removes the bimodal runtime; t3 detects promptly).
• 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.

[vharseko]

Thanks for the round-2 review — addressed in 4e1af23.

Finding 1 (elapsed3 < 1000 vs the 5000 ms timeout). You're right, and thanks for catching that the round-1 suggestion was itself unsound: nobody can see the cycle until t3 supplies the ts1 -> ts3 edge, so a t3 stall between start capture and its first isDeadlocked yields a legitimate late detection while the cycle is still live. Dropped the latency guard entirely. Replaced it with a value-domain check — result3 ∈ {UNCOMMITTED, TIMED_OUT} — which still catches a bogus positive commit timestamp or a spurious 0.

Finding 5 (restructure). Took the restructure rather than the minimal revert: resolve the cycle by aborting each waiter's target, starting with ts1 — the transaction the deadlock tells to abort. Both waiter timeouts go back to 1 000 000 ms; t1 is unblocked by ts1's abort at ~1 s instead of timing out, so runtime is ~1 s (no bimodal 1 s/5 s) and the detection window is bounded by t3's own 10 s timeout.

Finding 2. state now includes elapsed1 and elapsed2 (the values the last two assertions actually test).

Finding 4. result1 ∈ {0, UNCOMMITTED} asserted, mirroring result2.

Finding 3. PR description rewritten to match the head commit (no "always holds", no "removed elapsed3", correct verification).

Verification (per Finding 5 — report timings). 10 consecutive runs, all green, wall time 1.05–1.13 s each — t3 detects promptly every time here, so the TIMED_OUT/0 branches for result3/result1 are exercised by construction (they're the tolerated interleavings) rather than by a run I could force locally. Full TransactionIndexTest: 8/8.