fix: node invariant at exception

[barendgehrels]

Fixes: #1399

Description

This fixes the errors in boost_geometry_index_rtree_exceptions_rst that I encountered before.

Now with help of Claude Code, reporting:

This is a genuine exception-safety bug in the R-tree insert*, and the reason it only fails on your machine is that the exact number of
   value copies differs between compilers/standard libraries, which determines where in the insert algorithm the exception fires.

  Root cause

  The R*-tree has a reinsert mechanism (unique to R*-tree, not used by linear/quadratic). When a node overflows, instead of immediately
  splitting, it first tries to remove some elements and reinsert them. The function remove_elements_to_reinsert at
  index/detail/rtree/rstar/insert.hpp:82-181 does this:

  1. Copies all elements from the node into a sorted_elements temporary (line 128-137 -- copies here)
  2. Copies elements-to-reinsert into result_elements (line 149-153 -- copies here)
  3. Clears the original node (line 158)
  4. Copies remaining elements back into the node (line 160-164 -- copies here, MAY THROW)

  If an exception fires at step 4, the catch handler (line 166-178) clears the node entirely and destroys all sorted_elements, then
  rethrows. The node is now empty, and the exception propagates up.

Solution

By Claude Code:

The fix

  The problem was in remove_elements_to_reinsert (rstar/insert.hpp). The first phase — copying elements into sorted_elements and
  result_elements — involves value copies that can throw. But at that point the node still has max+1 elements (overflow state). If any
  copy threw, the exception propagated with the node left in overflow, and nobody cleaned it up.

  The fix wraps that first phase in a try-catch that:
  1. Clears result_elements (partial copies)
  2. Pops the overflow element from the node (matching the established pattern in the split exception handler)
  3. Rethrows

  This ensures the node never has more than max_elements after an exception, regardless of where the throw occurs.

I reviewed it myself and it looks completely right, and it solves the problem.

@awulkiew can you verify this as well?

[barendgehrels]

For review, best to ignore white space changes

[barendgehrels]

See also the part below, it is similar (but not identical). It was just missing here.

[tinko92]

Is there a reason for redefining this here? It seems to be the same as elements on line 102, https://github.com/barendgehrels/geometry/blob/ad2761fe3582714d9a6efea8189f2ea25125bda3/include/boost/geometry/index/detail/rtree/rstar/insert.hpp#L102 , which is above the scope of the try-catch block.

[barendgehrels]

good point I will check it

[barendgehrels]

The n is a non-const reference so that might be why it is repeated here

[barendgehrels]

elements.clear(); is also used below.
So probably we should indeed omit it - that makes the two blocks more consistent
I'll try tomorrow

[tinko92]

Is this not already implied by the combination of L102, L104, L108? https://github.com/barendgehrels/geometry/blob/ad2761fe3582714d9a6efea8189f2ea25125bda3/include/boost/geometry/index/detail/rtree/rstar/insert.hpp#L102-L108 , i.e. elements is rtree::elements(n), elements_count is max_elements + 1, then we assert elements.size is elements_count which is max_elements+1. Maybe I am missing something here that can change the value of n but it all looks non-modifying.

[barendgehrels]

This is the essence of the fix - so yes, it is necessary.
The point is that it is only necessary if something was thrown.
And (apparently) I was (already for a year) the only one who got this, in a unit test, always failing for me...

[tinko92]

Sorry, I didn't mean the entire block. I mean maybe remove just the elems.size() > parameters.get_max_elements() check because it is already guaranteed true. For the destroy_element and the pop_back, I understand those are the essence of the fix. I.e. I propose

--- a/include/boost/geometry/index/detail/rtree/rstar/insert.hpp
+++ b/include/boost/geometry/index/detail/rtree/rstar/insert.hpp
@@ -165,12 +165,8 @@ public:
             //  the behavior of the split exception handler.
             result_elements.clear();

-            auto & elems = rtree::elements(n);
-            if (elems.size() > parameters.get_max_elements())
-            {
-                destroy_element<MembersHolder>::apply(elems.back(), allocators);
-                elems.pop_back();
-            }
+            destroy_element<MembersHolder>::apply(elements.back(), allocators);
+            elements.pop_back();

As you state in the new comment https://github.com/barendgehrels/geometry/blob/ad2761fe3582714d9a6efea8189f2ea25125bda3/include/boost/geometry/index/detail/rtree/rstar/insert.hpp#L162

// [...] At this point the node
// has not been modified yet and still contains exactly max+1 elements

This was asserted in L108 before the try catch block and nothing that follows is modifying n or elements (the code only takes const iterators).

[tinko92]

The overall change seems consistent with the other exception handling case to me, so I am in favor. I left two comments on aspect that are unclear to me.

Since you are already touching this function, can you make sense of https://github.com/barendgehrels/geometry/blob/ad2761fe3582714d9a6efea8189f2ea25125bda3/include/boost/geometry/index/detail/rtree/rstar/insert.hpp#L109 ? It seems dubious since it checks an unsigned value for positivity and that unsigned value is also purely a function of parameters, seems like it should move to https://github.com/boostorg/geometry/blame/develop/include/boost/geometry/index/parameters.hpp .

[tinko92]

Should this line also go inside the TRY CATCH block since it may also throw and would still leave the node in the invalid state? We can also remove the "copy" from the comment, since reserving on an empty vector should never copy anything. This should only throw if the alloc fails.