deleteVertex can delete a vertex without collecting its edges when a chunk is transiently unreadable (the #5670 window, vertex side)

[lvca]

Summary

#5670 (PR #5678) closed the window where deleting an EDGE could leave its back-reference behind: a
chunk that is momentarily unreadable - a concurrent commit publishes its pages one at a time, so a
vertex page can expose a new edge-list head RID before that head's page is visible, and an emptied
chunk is relinked out of the chain under a walker's feet - was read as "nothing to remove here"
while the edge record was deleted anyway.

GraphEngine.deleteVertex still reads its own edge lists through the best-effort
getEdgeHeadChunk, and wraps the collection loop in catch (Exception):

try {
  outEdges = getEdgeHeadChunk(vertex, Vertex.DIRECTION.OUT);
  if (outEdges != null) { ... collect edges to delete ... }
} catch (Exception e) {
  // LINKED LIST COULD BE BROKEN
  LogManager.instance().log(this, Level.WARNING, "Error on deleting outgoing edges connected to vertex %s", e, ...);
}
...
vertex.getDatabase().getSchema().getBucketById(...).deleteRecord(vertex.getIdentity(), force);

So if the head chunk is unreadable at that instant, outEdges is null, no edges are collected,
and the vertex record is deleted regardless - leaving edges pointing at a vertex that no longer
exists (ghost edges), reported by check database as invalid links.

Why it was left out of #5678

This is a deliberate, tested contract, not an oversight: deleteVertex's javadoc states that edge
disconnection is best-effort, and Issue4420TolerantDeleteTest / Issue4432CorruptVertexDeleteTest
pin that a vertex whose chain is structurally broken stays deletable. Making the collection strict
would trade repairability for strictness, and that is a decision worth taking on its own merits
rather than as a side effect of an edge-delete fix.

Note that deleteVertex does get #5670's fix transitively where it matters most: every edge it does
collect is removed via edge.delete() -> deleteEdge, which is now strict, so a NEIGHBOUR's
back-reference is no longer left dangling.

What a fix would look like

Distinguish the two cases the single getEdgeHeadChunk call currently conflates:

• force == true (explicit repair of a structurally broken record): keep today's tolerance.
• force == false (ordinary delete): use getEdgeHeadChunkForWrite, so a transiently unreadable
  head is a retryable conflict and the transaction re-reads a consistent view - the same line fix(engine) #5670: an unreadable edge-list chunk retries the delete instead of skipping it #5678
  drew for deleteEdge. The surrounding catch (Exception) would have to let NeedRetryException
  through.

The open question is what an ordinary deleteVertex should do when the chain is genuinely broken
(not transiently unreadable), since the two are indistinguishable at that point: today it deletes and
warns; strict handling would require force to get the vertex out. That is the trade-off to decide.

Verification

A test in the shape of Issue5670EdgeDeleteDanglingBackRefTest - make the vertex's head chunk
unreadable, delete the vertex, and assert its edges do not outlive it - would pin whichever answer is
chosen.

[lvca]

Coupling worth recording here, raised in the #5678 review.

GraphEngine.getEdgeHeadChunkForWrite (and EdgeLinkedList.readChunk / loadChunkForWrite) cannot tell a TRANSIENT cross-page publication race from a chunk that is GENUINELY lost. So #5678 also changed what happens on real corruption, in the opposite direction to the tolerance this issue is about:

• before - edge.delete() on an edge whose endpoint list is genuinely broken completed best-effort: the edge record went away, the dangling back-reference stayed, the call succeeded.
• after - it raises ConcurrentModificationException on every attempt and fails once TX_RETRIES is exhausted. CHECK DATABASE is the repair path (it rebuilds a chain that cannot be loaded and drops the references into it, see GraphDatabaseCheckerChainRebuildTest).

That is the correct trade for the concurrency bug - a delete that silently leaves corruption behind is worse than one that refuses - but it is the mirror image of the best-effort tolerance deleteVertex keeps, and the two now disagree. Whichever way this issue is resolved should settle both together rather than leaving edge deletion strict and vertex deletion tolerant by accident:

• if deleteVertex goes strict for force == false, the two agree and force becomes the single documented escape hatch for a broken chain;
• if deleteVertex stays tolerant, it is worth stating explicitly why deleting an edge and deleting its endpoint answer a broken chain differently.

Either way the "can a genuinely broken graph still be cleaned up without force?" question is now the same question for both paths.

[lvca]

Second review of #5678 surfaced a further case that belongs on this issue: the strict removal reaches the neighbour, not just the vertex being deleted.

deleteVertex deletes each collected edge via edge.asEdge().delete() → deleteEdge, which disconnects both endpoints. So a healthy vertex whose neighbour has an unreadable edge list is now refused. Measured on the same fixture (a vertex whose only edge points at a hub whose IN head chunk was removed):

before: delete SUCCEEDED   (edge record gone, neighbour still pointing at it)
after:  ConcurrentModificationException

Note the loop only catches RecordNotFoundException, so the CME propagates out of deleteVertex:

for (Identifiable edge : edgesToDelete)
  try {
    edge.asEdge().delete();
  } catch (RecordNotFoundException e) {
    // ALREADY DELETED, IGNORE IT
  }

#5678 keeps this strict on purpose - succeeding would dangle a reference on a vertex the caller never named - and pins it with deletingAVertexWhoseNeighbourListIsUnreadableReportsAConflictRatherThanDanglingTheReference. But it widens the scope of the decision this issue tracks, in one specific way worth calling out:

force does not currently help. deleteVertex(vertex, force) promises "delete this even though its record cannot be assembled", yet the per-edge loop above is outside that promise - a broken neighbour blocks a forced delete just as it blocks an ordinary one. (That was already true before #5678 for promoted super-nodes, where StripedEdgeList.addChain has raised CME on a strict chain for some time; #5678 widens it to classic chains.) If this issue resolves toward keeping a tolerant path, making force absorb NeedRetryException in that loop is the natural shape: strict by default, best-effort only when the operator explicitly asked for it.

Also checked, since the review suspected it: Issue4432CorruptVertexDeleteTest is not order-fragile. LocalBucket detects the invalid record size on first read and deletes the record (Invalid record size 33554444 for record #1:0: deleting record), so the corrupt vertex fails at resolution - which stays tolerant - and never reaches the strict head lookup. Deleting the neighbour first, with the corrupt vertex still present, raises the identical RecordNotFoundException with and without #5678.

[lvca]

This is not only a tolerance question — there is a live concurrency bug here, verified. Worth reframing the issue, now that #5678 is merged.

deleteVertex reads its own edge lists through the best-effort getEdgeHeadChunk and wraps the collection loops in catch (Exception). So when the head chunk is momentarily unreadable — the same transient publication window #5670 was about, nothing to do with corruption — outEdges is null, no edges are collected, and the vertex record is deleted anyway.

Measured on current main (vertex A --E--> B, As OUT head chunk made unreadable, A deleted):

DELETE VERTEX with own unreadable OUT list SUCCEEDED
vertex A still exists = false
EDGE record still exists = true      <-- edge outlived its OUT vertex
B in-degree           = 1            <-- B still points at it

So the delete reports success while leaving a ghost edge whose out names a record that no longer exists, plus a back-reference on an innocent neighbour. That is the same defect class #5670 fixed for edge.delete(), in the same shape (a timing fact read as a graph fact), still open on the vertex path — and note the catch (Exception) would swallow a NeedRetryException here too, so even making the read strict is not sufficient on its own.

This changes the balance of the decision this issue tracks. It is not "should vertex deletion keep a best-effort escape hatch for corrupt graphs" alone; it is also "vertex deletion silently loses edges under ordinary concurrency". Those want different answers:

• the transient case should retry, exactly as deleteEdge now does — this is a bug, not a tolerance;
• the genuinely-corrupt case is where the tolerance argument applies, and force is the natural place for it (it currently does not help, as noted above).

Suggested shape, matching #5678: collect through a strict read when force == false and let NeedRetryException out of the collection loops; keep todays behaviour under force == true. A regression test in the shape of Issue5670EdgeDeleteDanglingBackRefTest — make the vertexs own head chunk unreadable, delete it, assert its edges do not outlive it — pins whichever answer is chosen.