fix: node invariant at exception by barendgehrels · Pull Request #1454 · boostorg/geometry

barendgehrels · 2026-03-30T15:12:08Z

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 · 2026-03-30T15:12:56Z


-        for ( typename elements_type::const_iterator it = elements.begin() ;
-              it != elements.end() ; ++it )
+        BOOST_TRY


For review, best to ignore white space changes

barendgehrels · 2026-03-30T15:22:02Z

+
+            BOOST_RETHROW
        }
+        BOOST_CATCH_END


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

tinko92 · 2026-04-14T05:44:57Z

+            //  the behavior of the split exception handler.
+            result_elements.clear();
+
+            auto & elems = rtree::elements(n);


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.

good point I will check it

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

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

A bit delayed but I tested it using elements instead, all fine. Will update PR.

tinko92 · 2026-04-14T05:53:57Z

+            result_elements.clear();
+
+            auto & elems = rtree::elements(n);
+            if (elems.size() > parameters.get_max_elements())


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.

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

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

It is the catch block where, in the block above, 4 comments are made with MAY THROW.
So I think and prefer the condition should stay...

The specific comment is added to state that there are not more than 1 (earlier I want to make it a while - but that's not necessary)

The test passes, also without condition - but still I prefer to have the condition there.

Additional AI input about both:

Reusing elements: Yes, perfectly fine. elements is defined at line 102 as elements_type & elements = rtree::elements(n); — a reference that remains valid throughout the function since n isn't reassigned. Reusing it makes the catch block read more naturally alongside the surrounding code (which uses elements everywhere), and it's one less name to follow.

Keeping the if check: Good call. Even though we can prove it's always exactly max+1 given the current call flow, defensive checks like this cost nothing at runtime and protect against:

Future refactors that might change the call flow

The catch block being exercised in some unexpected way (e.g., a BOOST_TRY reentry scenario)

Matching the pattern in visitors/insert.hpp:210 which also has a size() > max_size guard before popping

If someone later changes how overflow is triggered, the guard means the code degrades gracefully instead of corrupting via an unconditional pop_back on an already-valid node.

tinko92 · 2026-04-14T06:12:03Z

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 · 2026-04-15T15:49:38Z

@@ -125,32 +125,56 @@ class remove_elements_to_reinsert
        // If constructor is used instead of resize() MS implementation leaks here
        sorted_elements.reserve(elements_count);                                                         // MAY THROW, STRONG (V, E: alloc, copy)


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.

Good one. Fixed. AI again:

moved sorted_elements.reserve(elements_count) inside the BOOST_TRY block so that an allocation failure now triggers the catch handler that pops the overflow element, and updated the comment from (V, E: alloc, copy) to (alloc) since reserving on an empty vector doesn't copy anything — it only allocates.

barendgehrels · 2026-04-20T16:01:34Z

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 · 2026-04-20T16:08:34Z

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

Fixes: boostorg#1399

tinko92

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

barendgehrels requested review from awulkiew, tinko92 and vissarion March 30, 2026 15:12

barendgehrels self-assigned this Mar 30, 2026

barendgehrels commented Mar 30, 2026

View reviewed changes

barendgehrels mentioned this pull request Mar 30, 2026

Prevent FP contractions to FMA in index/.../content.hpp to avoid numerical instability in rtree #1453

Merged

barendgehrels commented Mar 30, 2026

View reviewed changes

tinko92 reviewed Apr 14, 2026

View reviewed changes

tinko92 reviewed Apr 15, 2026

View reviewed changes

barendgehrels force-pushed the fix-rst branch from ad2761f to fa7f23d Compare April 20, 2026 16:07

fix: node invariant at exception

cfcac15

Fixes: boostorg#1399

barendgehrels force-pushed the fix-rst branch from fa7f23d to cfcac15 Compare April 20, 2026 16:15

tinko92 approved these changes Apr 20, 2026

View reviewed changes

		@@ -125,32 +125,56 @@ class remove_elements_to_reinsert
		// If constructor is used instead of resize() MS implementation leaks here
		sorted_elements.reserve(elements_count); // MAY THROW, STRONG (V, E: alloc, copy)

Conversation

barendgehrels commented Mar 30, 2026

Description

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinko92 Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinko92 commented Apr 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barendgehrels commented Apr 20, 2026

Uh oh!

barendgehrels commented Apr 20, 2026

Uh oh!

tinko92 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tinko92 Apr 14, 2026 •

edited

Loading