fix: handle deleting dangling reference root node #1048
Conversation
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant deleteVersion
participant Cache
participant OrphanTraversal
Caller->>deleteVersion: call deleteVersion(version)
deleteVersion->>Cache: getRootKey(ndb, version)
Cache-->>deleteVersion: return rootKey or error
alt Error is ErrVersionDoesNotExist
deleteVersion->>deleteVersion: Continue processing
else Other error
deleteVersion->>Caller: return error
end
alt rootKey is nil
deleteVersion->>Caller: Skip orphan pruning
else Not nil
deleteVersion->>OrphanTraversal: traverseOrphansWithRootkeyCache
OrphanTraversal->>deleteVersion: Process orphans (reset nonce, delete prunings)
end
deleteVersion->>Caller: complete deletion process
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
iboss-ptk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nodedb.go (1)
466-468: Reassess skipping orphans whenrootKeyis nil.If
rootKeyis nil, the method bypasses orphan pruning entirely. Confirm that no stray orphans exist in cases of a missing or “dangling” root. If there’s a possibility that orphans remain, consider adding a fallback cleanup path.Would you like me to propose a workaround or open a new ticket to ensure zero leftover orphans?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodedb.go(1 hunks)
🔇 Additional comments (2)
nodedb.go (2)
462-463: Confirm ignoringErrVersionDoesNotExistis intentional.By continuing execution when
err == ErrVersionDoesNotExist, we skip returning early. This ensures dangling or non-existent nodes do not interrupt the pruning process. However, please verify this approach to avoid leaving obsolete data unpruned or masking unexpected errors.
471-483: Double-check legacy deletion logic for non-legacy orphans.When
orphan.nodeKey.nonce == 0 && !orphan.isLegacy, the code callsdeleteFromPruning(ndb.legacyNodeKey(orphan.hash)). While the comment explains this can be a reformatted root, this cross-handling between legacy and non-legacy may be error-prone. Please confirm that no unintended deletions occur if an orphan is purely new format.
iboss-ptk
changed the title
We found a case in Osmosis node where there is a root key that is points to a node that doesn't exists and it hangs the pruning process because fails at get root key (returns
ErrVersionDoesNotExist).There is already a code to clean the dangling ref node up, but it just never get there because it early returns
ErrVersionDoesNotExistbefore getting there.Summary by CodeRabbit
Bug Fixes
Refactor