fix(rollback): make topology mutations failure-atomic#480
Conversation
acgetchell
commented
Jun 26, 2026
acgetchell
commented
- Add scoped rollback guards for TDS, generic triangulation, and Delaunay mutation windows.
- Use the guards across insertion, deletion, local facet repair, and flip-repair retry paths.
- Restore insertion state and spatial-index caches according to the Delaunay rollback policy.
- Document rollback ownership rules for future mutation paths.
- Add scoped rollback guards for TDS, generic triangulation, and Delaunay mutation windows. - Use the guards across insertion, deletion, local facet repair, and flip-repair retry paths. - Restore insertion state and spatial-index caches according to the Delaunay rollback policy. - Document rollback ownership rules for future mutation paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds scoped rollback guards for Tds, Triangulation, and DelaunayTriangulation, rewires insertion, repair, and deletion flows to use commit/rollback transactions, and updates docs, tests, and module exports. ChangesRollback transaction migration
Sequence Diagram(s)Triangulation rollback flowsequenceDiagram
participant TdsOwnerRollbackTransaction
participant Tds
participant TriangulationRollbackTransaction
TdsOwnerRollbackTransaction->>Tds: capture canonical rollback state
TriangulationRollbackTransaction->>TdsOwnerRollbackTransaction: begin(owner)
TriangulationRollbackTransaction->>TriangulationRollbackTransaction: triangulation_mut() mutate
alt success
TriangulationRollbackTransaction->>TdsOwnerRollbackTransaction: commit()
else failure or drop
TriangulationRollbackTransaction->>TdsOwnerRollbackTransaction: restore()
end
Delaunay rollback flowsequenceDiagram
participant DelaunayRollbackTransaction
participant DelaunayTriangulation
participant TdsOwnerRollbackTransaction
DelaunayRollbackTransaction->>TdsOwnerRollbackTransaction: begin(owner)
DelaunayRollbackTransaction->>DelaunayTriangulation: save insertion_state and spatial index
DelaunayRollbackTransaction->>DelaunayTriangulation: delaunay_mut() mutate
alt success
DelaunayRollbackTransaction->>TdsOwnerRollbackTransaction: commit()
else failure or drop
DelaunayRollbackTransaction->>DelaunayTriangulation: restore()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 57 |
🟢 Coverage 93.95% diff coverage · +0.04% coverage variation
Metric Results Coverage variation ✅ +0.04% coverage variation (-1.00%) Diff coverage ✅ 93.95% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (60e99d1) 73579 67459 91.68% Head commit (c303f44) 74123 (+544) 67988 (+529) 91.72% (+0.04%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#480) 810 761 93.95% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
api
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/core/rollback.rs (1)
90-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand these rollback tests to 4D and 5D.
TriangulationRollbackTransactionis const-generic overD, but the new coverage only exercises 2D and 3D. A small dimension macro here would bring this file in line with the repo’s dimension-generic test policy.As per coding guidelines,
**/*.{rs,md}: Tests should verify mathematical, geometric, and topological invariants; dimension-generic tests should cover 2D through 5D whenever feasible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/rollback.rs` around lines 90 - 139, The rollback test coverage in TriangulationRollbackTransaction is still limited to 2D and 3D; extend the existing tests to also run for 4D and 5D. Rework the tests around the dimension-generic TriangulationRollbackTransaction and Triangulation types so the same assertions are exercised across D = 2..5, ideally by introducing a small dimension macro or equivalent shared helper to keep the test file concise.Source: Coding guidelines
src/core/repair.rs (1)
1346-1354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the rollback snapshot when there is nothing to repair.
repair_local_facet_issuesnow snapshots the full TDS beforerepair_local_facet_issues_with_frontierdiscovers an emptyissuesmap and returns0. Add an early return to avoid cloning the whole triangulation for the documented no-op path.♻️ Proposed fix
pub fn repair_local_facet_issues( &mut self, issues: &FacetIssuesMap, max_simplices_removed: usize, ) -> Result<usize, InsertionError> { + if issues.is_empty() { + return Ok(0); + } + let mut transaction = TriangulationRollbackTransaction::begin(self);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/repair.rs` around lines 1346 - 1354, The no-op path in repair_local_facet_issues still starts a TriangulationRollbackTransaction and snapshots the full triangulation even when there are no facet issues to repair. Add an early return at the start of repair_local_facet_issues, before creating TriangulationRollbackTransaction::begin(self), so an empty issues map returns 0 immediately and avoids cloning the TDS; use repair_local_facet_issues and repair_local_facet_issues_with_frontier as the key locations.src/delaunay/insertion.rs (1)
86-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate rollback snapshots on the actual repair schedule.
This currently starts a Delaunay rollback transaction whenever
delaunay_repair_policy != Never, even whenmaybe_repair_after_insertion_cappedwould skip repair for the next insertion. BecauseDelaunayRollbackTransaction::beginsnapshots the TDS, cadenced repair policies can pay a full-clone cost on insertions that cannot run post-repair/check work.♻️ Proposed fix
fn post_insertion_transaction_required(&self) -> bool { let next_insertion_count = self .insertion_state .delaunay_repair_insertion_count .saturating_add(1); let could_have_simplices_after_insertion = self.tri.tds.number_of_simplices() > 0 || self.tri.tds.number_of_vertices().saturating_add(1) > D; - could_have_simplices_after_insertion - && (self.insertion_state.delaunay_repair_policy != DelaunayRepairPolicy::Never - || self - .insertion_state - .delaunay_check_policy - .should_check(next_insertion_count)) + if !could_have_simplices_after_insertion { + return false; + } + + self.should_run_delaunay_repair_for(self.tri.topology_guarantee(), next_insertion_count) + || self + .insertion_state + .delaunay_check_policy + .should_check(next_insertion_count) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/delaunay/insertion.rs` around lines 86 - 99, Gate the rollback snapshot in post_insertion_transaction_required on the actual repair/check schedule rather than only on DelaunayRepairPolicy::Never. Update the logic in post_insertion_transaction_required so it mirrors the same cadence used by maybe_repair_after_insertion_capped, and only returns true when a post-insertion repair or check can actually run; keep DelaunayRollbackTransaction::begin untouched and use insertion_state.delaunay_repair_insertion_count plus the repair/check policy symbols to locate the change.src/delaunay/rollback.rs (1)
143-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover all transaction close paths and feasible dimensions.
The new guard has three distinct close paths (
commit, explicitrollback, and drop rollback), but these tests only exercise drop rollback in 2D. Please add focused coverage for committed mutations and explicit rollback, and extend the invariant checks through D=3–5 where feasible. As per coding guidelines, “Tests should verify mathematical, geometric, and topological invariants; dimension-generic tests should cover 2D through 5D whenever feasible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/delaunay/rollback.rs` around lines 143 - 204, The current rollback tests only cover the drop-based close path in 2D, so expand coverage in the same rollback test module around DelaunayRollbackTransaction to exercise all three close paths: commit, explicit rollback, and drop rollback. Add focused assertions for committed mutations and explicit rollback behavior using the existing helpers like DelaunayRollbackTransaction::begin, insert_uncommitted_vertex, and rollback/commit flows. Also generalize or duplicate the invariant checks across feasible dimensions, extending the same restore/invalidate expectations through 3D, 4D, and 5D where the triangulation test fixtures support it.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/rust.md`:
- Around line 246-247: Update the wording in the Rust docs so the phrase in the
“Higher level” section is hyphenated as a compound modifier, i.e. use
“Higher-level” in the DelaunayTriangulation guidance. Locate the text around the
DelaunayTriangulation rollback guard note and adjust only that heading/label
wording without changing the rest of the sentence.
---
Nitpick comments:
In `@src/core/repair.rs`:
- Around line 1346-1354: The no-op path in repair_local_facet_issues still
starts a TriangulationRollbackTransaction and snapshots the full triangulation
even when there are no facet issues to repair. Add an early return at the start
of repair_local_facet_issues, before creating
TriangulationRollbackTransaction::begin(self), so an empty issues map returns 0
immediately and avoids cloning the TDS; use repair_local_facet_issues and
repair_local_facet_issues_with_frontier as the key locations.
In `@src/core/rollback.rs`:
- Around line 90-139: The rollback test coverage in
TriangulationRollbackTransaction is still limited to 2D and 3D; extend the
existing tests to also run for 4D and 5D. Rework the tests around the
dimension-generic TriangulationRollbackTransaction and Triangulation types so
the same assertions are exercised across D = 2..5, ideally by introducing a
small dimension macro or equivalent shared helper to keep the test file concise.
In `@src/delaunay/insertion.rs`:
- Around line 86-99: Gate the rollback snapshot in
post_insertion_transaction_required on the actual repair/check schedule rather
than only on DelaunayRepairPolicy::Never. Update the logic in
post_insertion_transaction_required so it mirrors the same cadence used by
maybe_repair_after_insertion_capped, and only returns true when a post-insertion
repair or check can actually run; keep DelaunayRollbackTransaction::begin
untouched and use insertion_state.delaunay_repair_insertion_count plus the
repair/check policy symbols to locate the change.
In `@src/delaunay/rollback.rs`:
- Around line 143-204: The current rollback tests only cover the drop-based
close path in 2D, so expand coverage in the same rollback test module around
DelaunayRollbackTransaction to exercise all three close paths: commit, explicit
rollback, and drop rollback. Add focused assertions for committed mutations and
explicit rollback behavior using the existing helpers like
DelaunayRollbackTransaction::begin, insert_uncommitted_vertex, and
rollback/commit flows. Also generalize or duplicate the invariant checks across
feasible dimensions, extending the same restore/invalidate expectations through
3D, 4D, and 5D where the triangulation test fixtures support it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3e738d72-aaf6-43e8-af9e-cb58266fe3db
📒 Files selected for processing (11)
docs/dev/rust.mdsrc/core/algorithms/flips.rssrc/core/insertion.rssrc/core/repair.rssrc/core/rollback.rssrc/core/tds/rollback.rssrc/core/tds/storage.rssrc/delaunay/deletion.rssrc/delaunay/insertion.rssrc/delaunay/rollback.rssrc/lib.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
+ Coverage 91.66% 91.70% +0.04%
==========================================
Files 81 84 +3
Lines 73358 73902 +544
==========================================
+ Hits 67240 67769 +529
- Misses 6118 6133 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
- Share owner-bound TDS rollback guards across TDS, Triangulation, and Delaunay wrappers. - Restore insertion bookkeeping with rollback and restore or invalidate spatial indexes according to the mutation policy. - Avoid rollback snapshots when local facet repair has no issues or post-insertion repair/check cadence cannot run.
1 participant
