fix(backend): survive merge parents deleted from the unmerged graph#155
fix(backend): survive merge parents deleted from the unmerged graph#155harelb wants to merge 1 commit into
Conversation
The frontend can delete nodes (e.g. GraphUpdater delete updates for retracted object tracks), so a merge parent recorded by MergeTracker can vanish from the unmerged graph. Both applyMerges and updateAllMergeAttributes assumed parents persist and called the merge hook, which typically does getNode(nodes.front()) on the unmerged graph -> std::out_of_range -> std::terminate in the backend thread. Drop merge sets whose parent left either graph and skip installing null attributes returned by a hook.
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
LGTM, thanks! Would like to point out though that this probably was introduced one of the other claude changes (active nodes cannot be parents in a merge because the frontend can delete them whereas the frontend can't delete archived nodes)
| MergeTracker tracker; | ||
| const MergeTracker::MergeFunc null_attrs = [](const DynamicSceneGraph&, | ||
| const std::vector<NodeId>&) { | ||
| return NodeAttributes::Ptr(nullptr); |
There was a problem hiding this comment.
This is ugly; I'd use auto and at least not pass in nullptr to the constructor (though I'm pretty sure you can also specify the return type of the lambda as NodeAttributes::Ptr and return nullptr instead which is my preference)
| // the frontend can delete nodes, so the recorded parent may be gone from either | ||
| // graph; the merged attributes can never be rebuilt again in that case |
There was a problem hiding this comment.
drop uninformative comment
| const auto parent = iter->first; | ||
| // the frontend can delete nodes, so the recorded parent may be gone from either | ||
| // graph; the merged attributes can never be rebuilt again in that case | ||
| if (!unmerged.hasNode(parent) || !merged.hasNode(parent)) { |
There was a problem hiding this comment.
Okay to just check if unmerged doesn't have the parent; bad things have happened if the unmerged graph has the parent of a merge group and merged does not (which is fine to assert somewhere later)
| // the frontend can delete nodes, so the recorded parent may be gone from either | ||
| // graph; the merged attributes can never be rebuilt again in that case | ||
| if (!unmerged.hasNode(node) || !graph.hasNode(node)) { |
There was a problem hiding this comment.
ditto for comment and for checking the merged graph
2 participants
Problem
MergeTrackerprunes merge-set children that no longer exist in the unmerged graph, but never checks the parent. The frontend can delete nodes (e.g.GraphUpdaterdelete updates when an object track is retracted), so a previously-recorded merge parent can vanish from the unmerged graph. BothapplyMergesandupdateAllMergeAttributesthen call the merge hook, which typically starts withgetNode(nodes.front())on the unmerged graph:We hit this live ~18 s after a loop closure (the LC triggers
updateAllMergeAttributesover all recorded merge sets, including stale ones).Fix
setNodeAttributesso a hook may decline gracefully.Includes a regression test (
tests/backend/test_merge_tracker.cpp) exercising both entry points with a deleted parent plus the null-attrs path.Related to #154 only circumstantially (both found in the same profiling session); no code overlap.