Record which walk owns focus_iter_token, so to_next_val stops resuming stale state - #74
Open
imlvts wants to merge 1 commit into
Open
Conversation
…g stale state `to_next_get_val` *resumes* from `focus_iter_token` whenever it is not `NODE_ITER_INVALID`, rather than starting from the focus, and so does `to_next_k_path`. So the token is not scratch space: it is a promise that an iteration is in progress and positioned where the reader expects. Several movement operations broke that promise, and after each a following `to_next_val` carried on from the wrong place and reported that nothing was left: * `descend_first_byte` (and so `to_next_step`) stored the token `next_items` had already advanced past the item it descended into, and `NODE_ITER_FINISHED` on the branch that did not move. * `descend_first_k_path` leaves a token that belongs to the k-path walk, which `to_next_k_path` legitimately resumes from. * `reset` cleared the token only on the branch that popped an ancestor, so a zipper whose whole subtrie lives in one node kept a spent one. * `ascend`, `ascend_byte`, `ascend_until` and `ascend_until_branch` restored or left a token that no longer described the focus. Clearing the token everywhere is the obvious fix and costs ~10% on a `to_next_step` walk, because `to_next_sibling_byte` resumes from the same field and `to_next_step` alternates the two. Instead `IterOwner` records which walk the token belongs to: the k-path pair claims it, every ascent and `descend_first_byte` disown it, `reset` clears it, and `to_next_get_val` restarts unless the token is its own. The sibling path keeps its fast resume. `descend_first_byte` stores the token *before* its child-descent block, which may replace it with the child's own; storing after clobbers it. No API change. Squashed from five commits on the bugfixes branch, all one defect. Regression test: `read_zipper_to_next_val_after_every_movement`, nine routes onto the same location followed by a full value walk, plus the `reset` and four-ascent sequences and a k-path walk. Fails before this change. lean/FINDINGS.md finding 2 on the lean-fuzzer-restage branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZmoASqM5FUuzvJeJaYQjR
Collaborator
Author
|
Alternative: store iteration owner as a part of the iteration token, if there's a bit to spare. |
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.
to_next_get_valresumes fromfocus_iter_tokenwhenever it is notNODE_ITER_INVALID, rather than starting from the focus, and so doesto_next_k_path. So the token is not scratch space: it is a promise that an iteration is in progress and positioned where the reader expects. Several movement operations broke that promise, and after each a followingto_next_valcarried on from the wrong place and reported that nothing was left:descend_first_byte(and soto_next_step) stored the tokennext_itemshad already advanced past the item it descended into, andNODE_ITER_FINISHEDon the branch that did not move.descend_first_k_pathleaves a token that belongs to the k-path walk, whichto_next_k_pathlegitimately resumes from.resetcleared the token only on the branch that popped an ancestor, so a zipper whose whole subtrie lives in one node kept a spent one.ascend,ascend_byte,ascend_untilandascend_until_branchrestored or left a token that no longer described the focus.Clearing the token everywhere is the obvious fix and costs ~10% on a
to_next_stepwalk, becauseto_next_sibling_byteresumes from the same field andto_next_stepalternates the two. InsteadIterOwnerrecords which walk the token belongs to: the k-path pair claims it, every ascent anddescend_first_bytedisown it,resetclears it, andto_next_get_valrestarts unless the token is its own. The sibling path keeps its fast resume.descend_first_bytestores the token before its child-descent block, which may replace it with the child's own; storing after clobbers it. No API change.Squashed from five commits on the bugfixes branch, all one defect. Regression test:
read_zipper_to_next_val_after_every_movement, nine routes onto the same location followed by a full value walk, plus theresetand four-ascent sequences and a k-path walk. Fails before this change.lean/FINDINGS.md finding 2 on the lean-fuzzer-restage branch.