Fix generateLargeGraphStore() building edges over detached nodes - #292
Merged
Conversation
…ched nodes generateLargeEdgeList() creates edges against its own independently- sized, throwaway NodeStore rather than the real one - so edge.source/ target only lined up with generateLargeGraphStore()'s actual inserted nodes by numeric coincidence (matching storeId, different objects). removeNode()'s cascade-edge-removal walks the real node's own adjacency links, which were never wired to these edges, so removing a referenced node silently left a dangling edge behind instead of cascading. That only surfaced once something both spanned multiple storage blocks and had elements removed afterward - traced back to plain, pre-existing sequential serialization code, not anything specific to threading. Root cause confirmed directly: graphStore.nodeStore.get(edge.source. storeId) != edge.source for every edge before this fix. Fix: build edges via generateEdgeList(graphStore.nodeStore, ...) so they reference the real nodes. generateLargeNodeList()/ generateLargeEdgeList() are left untouched since other tests use them independently; generateLargeGraphStore() was unused before this session's tests, so nothing depended on the old behavior.
The fix touches GraphGenerator, not Serialization, so the test should exercise that directly rather than proving it indirectly through a full serialize/deserialize round-trip. Asserts the actual invariant that broke: every edge's source/target is the same object registered in the store's own nodeStore, not just one with a matching storeId. Confirmed red on the pre-fix generator, green after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root-caused the "pre-existing bug" flagged in #291: it's not a
Serialization/GraphStorecorrectness issue, it's a test-helper construction flaw.GraphGenerator.generateLargeGraphStore()built its node list viagenerateLargeNodeList()and its edge list viagenerateLargeEdgeList()independently. The latter internally creates its own, differently-sized, throwawayNodeStoreto source edge endpoints from — soedge.source/edge.targetnever actually pointed at the nodes inserted into the realGraphStore. They only lined up numerically (samestoreIdvalue) by coincidence of both being simple sequential-counter allocations, which is why everything looked fine as long as no node was ever removed afterward.Confirmed directly:
graphStore.nodeStore.get(edge.source.storeId) != edge.sourcefor every edge, before this fix.Once a referenced node is removed,
GraphStore.removeNode()'s cascade (which walks the real node's own in/out adjacency links to find and remove incident edges) finds nothing to remove — because those links were never wired to the edges that "coincidentally" reference thatstoreId— leaving a dangling edge behind. That only became visible once something both spanned multiple storage blocks and had elements removed afterward, which is exactly what came up while testing #291.Fix: build edges via
generateEdgeList(graphStore.nodeStore, ...)so they reference the real, inserted nodes.generateLargeNodeList()/generateLargeEdgeList()are untouched (other tests use them independently, as standalone lists, not paired) — andgenerateLargeGraphStore()itself was unused anywhere in the suite before this session's new tests, so nothing depended on the old behavior.Test plan
generateLargeGraphStore()+ scattered node/edge removal (multi-block, matches the scenario that surfaced this) + full serialize/deserialize round-trip, assertingdeepEquals. Fails red on the old generator, passes green with the fix.🤖 Generated with Claude Code