Collected while fixing #6259 (merged as PR #6269, d6da8280a). Three independent items plus one open question that spans the first two.
Item 1 - Issue5279ConcurrentInsertTest.sustainedConcurrentInsertsOnASingleBucketNeverConflict asserts two things that cannot both hold
Observed on PR #6269's unit-tests lane:
expected: 2001L
but was: 2000L
at Issue5279ConcurrentInsertTest.lambda$sustainedConcurrentInsertsOnASingleBucketNeverConflict$6(Issue5279ConcurrentInsertTest.java:193)
The test runs 8 threads x 250 inserts at attempts=1, so a transaction that hits a ConcurrentModificationException is not retried - it is counted and its record is never written:
try {
database.transaction(() -> database.newDocument("Case").set("payload", payload).save(), true, 1);
} catch (final ConcurrentModificationException e) {
conflicts.incrementAndGet();
}It then tolerates a small residue of conflicts, and immediately afterwards demands the exact full count:
assertThat(conflicts.get()).as("pure inserts must not raise concurrent modifications").isLessThanOrEqualTo(2);
database.transaction(
() -> assertThat(database.countType("Case", false)).isEqualTo(1L + (long) threadCount * insertsPerThread));Those two assertions contradict each other. Expecting 2001 while allowing up to 2 conflicts asserts both that a conflict may happen and that it may not, so the run where the tolerance is actually used fails at the count (2000 against 2001) instead of passing at the tolerance meant to absorb it. That is exactly the observed failure: one tolerated conflict.
This is the same trap already fixed in the sibling class. Issue5279ConcurrentUpdateTest.concurrentEdgeCreationBetweenOwnVerticesNeverConflicts was repaired in PR #6223 by counting the conflicts out, with a comment saying precisely this:
// ...but the edge transaction runs at attempts=1, so a conflict tolerated above means that edge was NOT
// created. Counting the conflicts out is what makes the two assertions agree: expecting the full total while
// allowing one conflict asserts both that a conflict may happen and that it may not, and the run where one
// DOES happen then fails here (160 against 161) instead of at the tolerance meant to absorb it.
assertThat(database.countType("Link", false)).isEqualTo(1L + (long) threadCount * edgesPerThread - conflicts.get());The Insert class was missed by that sweep. The fix is the same one line:
assertThat(database.countType("Case", false)).isEqualTo(1L + (long) threadCount * insertsPerThread - conflicts.get());Worth sweeping the rest of both classes for the same shape while in there: the pattern is "tolerance > 0 at attempts=1" combined with an exact count.
The contradiction is a property of the test alone and holds regardless of what the engine does. What is not established is whether anything recently changed the conflict rate from effectively zero to occasionally one - see the open question below. 25/25 green locally on an idle machine, so the conflict is rare.
Item 2 - ACIDTransactionTest.indexCreationWhileAsyncMustFail leaves an index the integrity check cannot read
Observed on PR #6269's slow-unit-tests lane, failing in teardown rather than in the test body:
ACIDTransactionTest>TestHelper.afterTest:184->TestHelper.checkDatabaseIntegrity:233
[Warnings[index 'V_0_964838133748': the physical key order does not match the current comparator, the index must be rebuilt,
index 'V_0_964838133748': page 0: keys cannot be read (Invalid position 67109887 (size=262136))]]
expected: 0
but was: 2
The test deliberately races an index creation against in-flight async inserts, expects the creation to fail, and then simulates a crash:
try {
database.getSchema().getType("V").createTypeIndex(Schema.INDEX_TYPE.LSM_TREE, true, "id");
} catch (NeedRetryException e) {
//no action
}
db.async().waitCompletion();
...
((DatabaseInternal) db).kill();kill() discards whatever is still in the flush pipeline, so a half-built index over a killed database is by construction the state left behind, and whether checkDatabaseIntegrity finds it readable is timing-dependent. The test's own comment already concedes the race ("sometimes it doesn't happen, the async is finished").
Two readings, and they want different fixes, which is why this needs triage rather than a patch:
- The test is fragile by construction - a failed index creation followed by
kill() has no defined on-disk outcome, so asserting zero integrity warnings afterwards is asserting on undefined state. The fix is in the test (drop the index before kill(), or exclude this database from the teardown integrity check).
- CHECK DATABASE is right -
Invalid position 67109887 (size=262136) is a page-0 read of an index whose build was interrupted, and if that state can also be reached by a real crash mid-build then the recovery path, not the test, is the problem. That would matter well beyond this test.
ACIDTransactionTest has not been modified in years and neither test has any issue history, so whichever reading holds, it has been latent.
Item 3 - Per-database flush queue
Deliberately left open by #6269 and worth measuring on its own. arcadedb.pageFlushQueue is a single JVM-wide bound, so one database's burst still consumes the admission budget of every other database in the process - the committers of an idle database now wait outside the page-manager lock instead of inside it, which is what #6259 fixed, but they still wait.
A per-database queue bounds each database's backlog independently and removes the cross-database coupling at its source. Note it does not replace what #6269 landed: a committer that blocks on its own full queue while holding the JVM-wide lock still stalls everyone, so the pre-lock admission control is required either way and the two compose.
Open question spanning items 1 and 2
Both tests went red on PR #6269's CI while main's unit-tests and slow-unit-tests lanes were green (main's only red lane is the chronic ha-integration-tests, #5668/#5702), so neither can be dismissed as a known-red lane.
Causality was never established before the merge:
- A re-run of both jobs on the identical commit was still in progress when the PR merged.
- Local reproduction on an idle machine failed to distinguish anything:
ACIDTransactionTest 12/12 green on main and 11/11 green on the branch, Issue5279ConcurrentInsertTest 25/25 green on the branch.
- Review of the merged change found no mechanism that would explain either: no double-release of a queue reservation on any exit path, no new batch-drop path (the old
while (running) offer(...) and the new reserveQueueSlot() drop under the same running == false condition), and no enqueue reordering, since the reservation is taken before the lock while the offer still happens inside it, so enqueue order still follows lock-acquisition order.
That is reasoning, not evidence. The cleanest remaining signal is the post-merge main runs: if these two tests go red there, they are pre-existing flakes and items 1 and 2 stand on their own; if they stay green across several main runs having been red twice on the PR, the interaction deserves a real look before this is considered closed.
Collected while fixing #6259 (merged as PR #6269,
d6da8280a). Three independent items plus one open question that spans the first two.Item 1 -
Issue5279ConcurrentInsertTest.sustainedConcurrentInsertsOnASingleBucketNeverConflictasserts two things that cannot both holdObserved on PR #6269's
unit-testslane:The test runs 8 threads x 250 inserts at
attempts=1, so a transaction that hits aConcurrentModificationExceptionis not retried - it is counted and its record is never written:It then tolerates a small residue of conflicts, and immediately afterwards demands the exact full count:
Those two assertions contradict each other. Expecting 2001 while allowing up to 2 conflicts asserts both that a conflict may happen and that it may not, so the run where the tolerance is actually used fails at the count (2000 against 2001) instead of passing at the tolerance meant to absorb it. That is exactly the observed failure: one tolerated conflict.
This is the same trap already fixed in the sibling class.
Issue5279ConcurrentUpdateTest.concurrentEdgeCreationBetweenOwnVerticesNeverConflictswas repaired in PR #6223 by counting the conflicts out, with a comment saying precisely this:The
Insertclass was missed by that sweep. The fix is the same one line:Worth sweeping the rest of both classes for the same shape while in there: the pattern is "tolerance > 0 at
attempts=1" combined with an exact count.The contradiction is a property of the test alone and holds regardless of what the engine does. What is not established is whether anything recently changed the conflict rate from effectively zero to occasionally one - see the open question below. 25/25 green locally on an idle machine, so the conflict is rare.
Item 2 -
ACIDTransactionTest.indexCreationWhileAsyncMustFailleaves an index the integrity check cannot readObserved on PR #6269's
slow-unit-testslane, failing in teardown rather than in the test body:The test deliberately races an index creation against in-flight async inserts, expects the creation to fail, and then simulates a crash:
kill()discards whatever is still in the flush pipeline, so a half-built index over a killed database is by construction the state left behind, and whethercheckDatabaseIntegrityfinds it readable is timing-dependent. The test's own comment already concedes the race ("sometimes it doesn't happen, the async is finished").Two readings, and they want different fixes, which is why this needs triage rather than a patch:
kill()has no defined on-disk outcome, so asserting zero integrity warnings afterwards is asserting on undefined state. The fix is in the test (drop the index beforekill(), or exclude this database from the teardown integrity check).Invalid position 67109887 (size=262136)is a page-0 read of an index whose build was interrupted, and if that state can also be reached by a real crash mid-build then the recovery path, not the test, is the problem. That would matter well beyond this test.ACIDTransactionTesthas not been modified in years and neither test has any issue history, so whichever reading holds, it has been latent.Item 3 - Per-database flush queue
Deliberately left open by #6269 and worth measuring on its own.
arcadedb.pageFlushQueueis a single JVM-wide bound, so one database's burst still consumes the admission budget of every other database in the process - the committers of an idle database now wait outside the page-manager lock instead of inside it, which is what #6259 fixed, but they still wait.A per-database queue bounds each database's backlog independently and removes the cross-database coupling at its source. Note it does not replace what #6269 landed: a committer that blocks on its own full queue while holding the JVM-wide lock still stalls everyone, so the pre-lock admission control is required either way and the two compose.
Open question spanning items 1 and 2
Both tests went red on PR #6269's CI while
main'sunit-testsandslow-unit-testslanes were green (main's only red lane is the chronicha-integration-tests, #5668/#5702), so neither can be dismissed as a known-red lane.Causality was never established before the merge:
ACIDTransactionTest12/12 green on main and 11/11 green on the branch,Issue5279ConcurrentInsertTest25/25 green on the branch.while (running) offer(...)and the newreserveQueueSlot()drop under the samerunning == falsecondition), and no enqueue reordering, since the reservation is taken before the lock while theofferstill happens inside it, so enqueue order still follows lock-acquisition order.That is reasoning, not evidence. The cleanest remaining signal is the post-merge
mainruns: if these two tests go red there, they are pre-existing flakes and items 1 and 2 stand on their own; if they stay green across several main runs having been red twice on the PR, the interaction deserves a real look before this is considered closed.