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?

[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 .

[barendgehrels]

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 .

It is not advised.

Good observation, but moving it to parameters.hpp doesn't quite work. 

Here's the analysis:

reinserted_elements_count = min(get_reinserted_elements(), max+1 - min) is positive when:
  - get_reinserted_elements() > 0, AND
  - max + 1 > min

The second condition is already guaranteed by the existing parameter static_assert 
at parameters.hpp:138: 

2*MinElements <= MaxElements+1 combined with 0 < MinElements implies 
max+1 >= 2*min >= min+1 > min. So that part is redundant.

The first condition, reinserted_elements > 0, cannot go into parameters.hpp 
because 0 is a documented valid value meaning "disable forced reinsertions" 
(see parameters.hpp:124). So we can't assert it at construction time.

So the assertion is really a function precondition: 
"this function must only be called when reinsertion is enabled". 
That precondition belongs here, not in parameters.hpp.

[barendgehrels]

Pushed the two additional fixes or amendments, @tinko92 thanks again for your comments.

[tinko92]

Thanks for addressing everything and all the work in the PR! I'm fine with merging.