You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found while investigating discussion #5597 (663M vertices / 14B edges). The reporter tried multiple concurrent GraphBatchLoad streams, hit ConcurrentModificationException and page-version mismatches, and fell back to a single writer. The fallback is correct, but the reason is worse than contention: two concurrent GraphBatch instances on the same database can silently lose edges, and nothing stops a caller from creating them.
Failure scenario
database.batch() (LocalDatabase:1806-1811) is an unguarded factory. Two batches A and B run concurrently and both touch vertex V:
A appends V's first edge. getOrCreateOutSegmentDeferred() reads V, finds getOutEdgesHeadChunk() == null, creates segment C1, and calls persistNewSegment() (:1620-1632), which records deferredOutHead[V] = C1in A's own map. V's record on disk is untouched.
B appends its own edge for V. It reads V, still sees getOutEdgesHeadChunk() == null (A's pointer is deferred), creates segment C2, records deferredOutHead[V] = C2 in B's map.
Both close. batchUpdateVertexHeadChunks() (:1004-1063) writes each batch's head pointer onto V. Last writer wins.
The loser's whole segment chain for V is orphaned: the edges are persisted, reachable from nothing, and the integrity checker will report V's edges as missing. No exception anywhere.
The head pointer being deferred to close() is what makes this invisible; sequential batches are fine because close() publishes before the next one reads.
Secondary interference on the same path
Both batches mutate the executor's global WAL flags (:1300-1301, :1982-1983) and restore whatever they happened to read on entry, so B can run with A's durability settings. On a replicated database that means writes that never reach followers.
async.waitCompletion() (:1324, :2005) is executor-wide, so each batch blocks on the other's tasks.
Slot assignment is bucket % parallelLevel (:1318, :1997), so two batches touching the same vertex bucket serialize on the same slot regardless. There is no parallelism to be won here.
Make the single-writer contract explicit rather than folklore. LocalDatabase.batch() should refuse a second concurrent GraphBatch on the same database with a message that says why and points at the intended pattern (one batch, parallelFlush for the fan-out), released on close(). On the server side, ArcadeDbGrpcService.graphBatchLoad should surface that as a clean FAILED_PRECONDITION instead of the current interleaving.
The alternative, making concurrent batches actually safe, means publishing head pointers eagerly and sharing the chunk caches, which gives up most of what makes the bulk path fast. Not worth it: the fan-out already exists inside one batch.
Verification
Test: two GraphBatch instances on one database, both adding edges to the same vertex, asserting the second construction is rejected. Plus a regression test that the documented single-batch path still connects both directions and passes the integrity check.
Found while investigating discussion #5597 (663M vertices / 14B edges). The reporter tried multiple concurrent
GraphBatchLoadstreams, hitConcurrentModificationExceptionand page-version mismatches, and fell back to a single writer. The fallback is correct, but the reason is worse than contention: two concurrentGraphBatchinstances on the same database can silently lose edges, and nothing stops a caller from creating them.Failure scenario
database.batch()(LocalDatabase:1806-1811) is an unguarded factory. Two batches A and B run concurrently and both touch vertex V:getOrCreateOutSegmentDeferred()reads V, findsgetOutEdgesHeadChunk() == null, creates segment C1, and callspersistNewSegment()(:1620-1632), which recordsdeferredOutHead[V] = C1in A's own map. V's record on disk is untouched.getOutEdgesHeadChunk() == null(A's pointer is deferred), creates segment C2, recordsdeferredOutHead[V] = C2in B's map.batchUpdateVertexHeadChunks()(:1004-1063) writes each batch's head pointer onto V. Last writer wins.The loser's whole segment chain for V is orphaned: the edges are persisted, reachable from nothing, and the integrity checker will report V's edges as missing. No exception anywhere.
The head pointer being deferred to
close()is what makes this invisible; sequential batches are fine becauseclose()publishes before the next one reads.Secondary interference on the same path
:1300-1301,:1982-1983) and restore whatever they happened to read on entry, so B can run with A's durability settings. On a replicated database that means writes that never reach followers.async.waitCompletion()(:1324,:2005) is executor-wide, so each batch blocks on the other's tasks.bucket % parallelLevel(:1318,:1997), so two batches touching the same vertex bucket serialize on the same slot regardless. There is no parallelism to be won here.Proposal
Make the single-writer contract explicit rather than folklore.
LocalDatabase.batch()should refuse a second concurrentGraphBatchon the same database with a message that says why and points at the intended pattern (one batch,parallelFlushfor the fan-out), released onclose(). On the server side,ArcadeDbGrpcService.graphBatchLoadshould surface that as a cleanFAILED_PRECONDITIONinstead of the current interleaving.The alternative, making concurrent batches actually safe, means publishing head pointers eagerly and sharing the chunk caches, which gives up most of what makes the bulk path fast. Not worth it: the fan-out already exists inside one batch.
Verification
Test: two
GraphBatchinstances on one database, both adding edges to the same vertex, asserting the second construction is rejected. Plus a regression test that the documented single-batch path still connects both directions and passes the integrity check.