fix: avoid recursive teardown of deep AVL node chains - #26
Open
a-shannon wants to merge 2 commits into
Open
Conversation
a-shannon
force-pushed
the
fix/iterative-node-drop-upstream
branch
from
August 19, 2026 19:48
a48703f to
1ede0fc
Compare
a-shannon
marked this pull request as ready for review
August 20, 2026 21:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InternalNodechains with an explicit worklist instead of recursiveRcdestructionRefCellborrowsDependency
#24 has merged. This branch is rebased onto its resulting
mainat9231458and now contains only the two iterative-teardown commits:a306d57- iterative teardown and regression matrix1ede0fc- shared-child fast path and allocation/alias regressionsThe validation below was rerun on the rebased head.
Problem
#24 makes subtree labeling iterative, but releasing the resulting
Rctree can still recursively destroy a last-owner chain ofInternalNodevalues. A sufficiently deep proof spine can therefore exhaust the process stack during cleanup, including after constructor rejection.Imposing a proof-depth cap is not acceptance-compatible: hash-matching explicit spines at depths 255, 256, and 300 are accepted by both this verifier and the JVM reference when operation limits are absent.
Implementation
Drop for InternalNodedetaches internal child edges into a worklist and descends only whenRc::try_unwrapproves unique ownership. Shared allocations are released normally and remain controlled by their other owners.A fast path returns before allocating the worklist when both children are distinct and each has another strong owner. The
Rc::ptr_eqcondition is required: if both fields alias one allocation with a strong count of two, releasing both edges can still reach zero and must use iterative teardown.Adding
Dropto the publicInternalNodetype is source-breaking for downstream Rust code that moves its fields out by value. The known sigma-rust and node call sites do not do so, but unknown external consumers remain possible; this compatibility tradeoff is intentionally exposed for review in a separate PR.No parser predicate, proof-depth bound, verifier acceptance rule, serialization, operation limit, or public function signature is changed.
Validation
Rc::ptr_eqreproducesSTATUS_STACK_OVERFLOW; restoring it passes.tree.copyownership shape goes from two allocations to zero with the fast path.Lookupcases at depths 255, 256, and 300 remain accepted.cargo check --libandcargo test --all-targetspass.The release benchmark evidence and review history are recorded in mwaddip#2.
Scope boundary
This closes recursive ownership teardown. It does not claim that every recursive lookup, modification, or deletion operation is stack-safe.