editor: Fix inlay map panic when stale inlays resolve out of order - #62636
Draft
mikayla-maki wants to merge 16 commits into
Draft
editor: Fix inlay map panic when stale inlays resolve out of order#62636mikayla-maki wants to merge 16 commits into
mikayla-maki wants to merge 16 commits into
Conversation
Extends the block-map fuzzing to drive excerpt removal, excerpt re-addition, path key reuse, and diff base changes against multibuffers that have diffs attached, including the inverted-diff shape used by the left hand side of a split diff. After each operation, every header block's buffer must still be present in the multibuffer snapshot, which is the invariant whose violation panics ZED-7G6 at render time.
Adds a second fuzz test that drives both sides of a split diff, removing and restoring files on both multibuffers and editing main buffers so the companion edit conversion is exercised, then asserts every header block still resolves its buffer. This test currently fails: converting companion edits can produce a wrap edit whose start row is greater than its end row, which underflows when Patch composes it. Whether the pairing of the two sides that produces this is one the split diff can actually reach still needs confirmation.
Excerpts each file into both multibuffers under the right side's path key, deriving the left side's rows through the diff, because the two sides are paired positionally per buffer. Building the sides independently produced excerpt pairings the split diff never creates, which was the source of the reversed companion edit the previous commit described. Also folds and unfolds buffers on either side, so folded buffer blocks are covered alongside headers and excerpt boundaries.
A snapshot holding an excerpt whose buffer is missing from its buffers map would panic anything that resolves a boundary's buffer, including buffer headers, even when the header was just computed. The existing invariants compared path keys only, so this went unchecked.
Extends the excerpt-removal fuzz with fold map and inlay map mutations, so fold and inlay edits propagate through the layer stack interleaved with structural multibuffer changes. With deeper settings (SEED=36 OPERATIONS=40) this reproduces a live crash family: InlayMap::sync panics with "cannot summarize backward" when an inlay whose anchor predates structural multibuffer changes resolves non-monotonically against edits derived from the new snapshot. Sentry has dozens of open issues with this panic message pointing at other anchor consumers (Excerpt::new via find-all-references in ZED-79W, vim marks in ZED-APE/ZED-AR0/ZED-APW), consistent with a shared root cause. Default settings stay green so this can land while the bug is fixed separately.
When a path key is reused for a different buffer, as happens when a diff's base buffer is recreated, inlays anchored in the departed buffer resolve to the end of the reused path's region while still sorting before anchors into the new buffer, because same-path anchors order by buffer id. InlayMap::sync binary searches self.inlays by resolved offset, so these stale entries can steer the search in front of a valid inlay belonging to a region before the edit being processed. Pushing that inlay again would duplicate the transform the preceding slice already carried over, and building its prefix panics with "cannot summarize backward" in the rope layer. Skip inlays that resolve before content that has already been rebuilt. This was minimized from the excerpt-removal fuzz (previously SEED=36 OPERATIONS=40) into a deterministic regression test. The same panic message accounts for dozens of open Sentry crashes blaming other anchor consumers (Excerpt::new via find-all-references in ZED-79W, vim marks in ZED-APE, ZED-AR0, ZED-APW), which likely need analogous guards at their call sites. Release Notes: - Fixed a crash that could occur when inlay hints were present while a multibuffer replaced the contents of a file, such as a diff view whose base changed.
Drives real DisplayMap entities, rather than hand-built layer stacks, through the transitions SplittableEditor performs: setting and clearing the companion, which stashes deferred edits and cleans up balancing blocks, while files enter and leave both multibuffers, main buffers are edited with their diffs recalculated, and buffers are folded on either side. After every operation, every header block must resolve its buffer against the display snapshot it came from, which is the invariant whose violation panics as ZED-7G6.
Adds a following test that removes files from a leader's multibuffer, restores them, reuses a path key for a different buffer, and edits excerpted buffers, asserting after each change that the follower's display snapshot never holds a header block for a buffer its multibuffer no longer contains. The follower applies these changes by deserializing the leader's excerpt updates, a path no local test could previously drive.
… missing The "buffer snapshot not found for excerpt boundary" panic has fired in the field for months without a local reproduction, and the fixed message is the only diagnostic a crash report carries. Enumerate the snapshot's buffers and excerpts, with their path indexes and the anchors' buffer ids, so the next report describes the inconsistent state instead of only proving it exists.
Point ranges reaching set_excerpts_for_path come from external sources like language server responses, which can produce ranges whose start lies after their end. build_excerpt_ranges derived the context start from the range's start row and the context end from its end row, so a reversal larger than twice the context line count produced an excerpt whose context anchors resolve backward, and summarizing that excerpt panics in the rope layer with "cannot summarize backward" (ZED-79W, reached from find-all-references). Order the endpoints before deriving the context and primary ranges. Release Notes: - Fixed a crash that could occur when opening search results or references in a multibuffer when a language server returned a reversed range.
A mark's anchors are stored in caller order: paired marks like [ and ] correspond by index, so storage cannot sort them, and multi-cursor callers do not guarantee sorted order. Serializing marks summarized them with summaries_for_anchors, which requires sorted input and panics with "cannot summarize backward" otherwise (ZED-APE, ZED-AR0, ZED-APW). Summarize each anchor independently instead. Release Notes: - Fixed a crash that could occur when setting vim marks with multiple cursors.
Diff views like the project diff and commit view show all hunks expanded, which materializes deleted rows from base buffers inline and makes anchor comparison consult diff base anchors. Enable that mode in the excerpt-churn fuzzers. At deeper settings this immediately found that replacing a diff's base text unsorts the fold map's persistent fold tree (SEED=144 OPERATIONS=50): fold anchors captured under the old base text compare differently once the base is swapped, and FoldMap seeks the tree assuming order. Documented on the fuzz test for follow-up alongside the other known seeds.
SIMULATE_PRODUCTION=1 disables the fold map's test-only invariants the way production builds do, so corrupted state propagates to its downstream symptoms instead of stopping at the first internal check. Under it, the fold tree unsorted by a diff base text swap (SEED=144) propagates until FoldMap::sync panics with "cannot seek backward" (SEED=332), the message behind a large family of open Sentry crashes, including ZED-95K with 200 events across 151 users via highlight anchors on the same render path.
Adds an edit coverage checker to the excerpt-churn fuzzer: applying the emitted wrap edits to the previous wrap text must reproduce the new wrap text exactly. Every layer below the block map propagates its changes only through these edits, so a changed row that no edit covers is precisely the under-invalidation that leaves the block map holding stale header blocks (ZED-7G6). With SIMULATE_PRODUCTION=1 extended to the wrap map's invariants, the fold tree disorder caused by a diff base swap propagates to a demonstrated coverage violation (SEED=612 OPERATIONS=50): the wrap layer emits edits that misdescribe the actual change. This completes the causal chain from the anchor ordering root cause to the block map's stale headers, one link short of the render-time panic itself, which requires a header row to fall in the uncovered region. Continuing past crashing seeds reproduces the field distribution: most corrupted runs die in the louder "cannot seek backward" and "cannot summarize backward" families first.
…e change Distills the seed-144 fuzz failure into a minimal unit test. Two folds inside an expanded deleted hunk share a text anchor and are ordered only by their diff_base_anchors. When the base text changes and deletes the region one anchor points into, comparing the still-valid anchor against the invalidated one falls back to the text anchor's bias, inverting the answer the comparator gave at insertion time. The persistent fold tree is then unsorted without having been touched. The test fails until multibuffer anchor comparison is made stable across diff base changes, or anchor-sorted structures are re-anchored when the base text changes. Also enriches the fold-order invariant's assertion message to dump both folds' anchor internals and resolved offsets, which is how the flipping comparator branch was identified.
Extends the deterministic fold-disorder repro into ZED-7G6's full causal chain. A new DisorderedFoldStack fixture builds a display map stack over a multibuffer whose fold tree has been unsorted by a diff base change, and a targeted fuzz test searches the short distance from that disorder to a stale header: because corruption is guaranteed from the first operation, seed hunting is dense where the fully randomized fuzzers were sparse. Seed 1316 reaches a removed buffer's surviving header block, and test_removing_buffer_removes_header_after_diff_base_changes replays that sequence with no randomness. The display map's test-only invariants are now skippable per-thread via production_simulation::SimulateProductionGuard rather than only process-wide via the SIMULATE_PRODUCTION environment variable, so a single test can let corruption propagate the way production builds do without leaking into tests running in parallel. The hunt also mapped how the corruption dies in different builds: debug builds fail at BlockMap::sync's row-accounting debug assertion or at subtract-with-overflow panics in the tab and wrap maps, while release builds (no debug assertions, no overflow checks) silently wrap those subtractions and continue to the stale header, matching how production's "cannot seek backward" crash families outnumber ZED-7G6 in Sentry. Both new deliberately failing tests stay red until the fold tree ordering root cause is fixed.
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.
While investigating ZED-7G6 ("buffer snapshot not found for excerpt boundary", 171 events), we added randomized coverage for a part of the display map space nothing previously tested: multibuffers whose excerpts are removed, re-added, and re-keyed while diffs (including all hunks expanded, as diff views configure), folded buffers, text folds, and inlays are present. Coverage spans hand-built layer stacks, real
DisplayMapentities driven through split and unsplit transitions, and a collab following test in which the follower applies the leader's excerpt churn by deserializing view updates.The investigation converged on a systemic root cause: multibuffer anchors do not preserve their comparison and resolution order across structural changes (a path key reused for a different buffer, a buffer leaving, a diff base text replaced), and many downstream consumers assume they do, either by feeding anchors to sorted-input-only APIs or by keeping persistent trees sorted by anchor comparison. Each such consumer is a separate crash family in Sentry, all panicking in the rope or sum-tree layers with "cannot summarize backward" or "cannot seek backward".
Three consumers are fixed here, each with a deterministic regression test that fails before its fix:
InlayMap::sync: inlays anchored in a buffer whose path key was reused resolve to the end of the reused path's region while sorting before anchors into the new buffer, corrupting the binary search over inlays. Fixed by skipping inlays that resolve before content already rebuilt.Excerpt::newvia reversed ranges (ZED-79W, 19 events / 17 users): a reversed range from a language server, reachable through find-all-references, produced an excerpt whose anchors resolve backward. Fixed by ordering the endpoints inbuild_excerpt_ranges.[and]correspond by index, so storage cannot sort), but serialization summarized them with a sorted-input-only API. Fixed by summarizing each anchor independently.ZED-7G6 itself is now reproduced end to end, with an artifact at every link of the chain:
test_folds_stay_sorted_when_diff_base_text_replaced. With all diff hunks expanded, folds inside a deleted hunk share a text anchor and are ordered only by theirdiff_base_anchors into the diff's base text. When the base text changes and deletes the region under one anchor, that anchor becomes invalid, and comparing a valid base anchor against an invalid one falls back to the text anchor's bias (ExcerptAnchor::cmp, the(Some, None)/(None, Some)arms). The comparator's answer is consistent with where the anchors resolve now, but opposite to the answer it gave when the folds were inserted — the comparator is time-variant, so the persistent fold tree becomes unsorted without being touched.test_removing_buffer_removes_header_after_diff_base_changes, which replays a sequence found by the new targeted fuzz testtest_stale_buffer_header_after_diff_base_change(seed 1316): because the fixture guarantees fold disorder from the first operation, seed hunting only has to search the short distance from disorder to staleness. After a handful of edits, base changes, and buffer churn, the block map's row accounting is displaced; removing a buffer then emits edits that miss its header row, and the block map keeps a header block referencing a buffer absent from the snapshot — resolving that header during render is ZED-7G6's exact panic. Build profiles determine which symptom surfaces, matching the field distribution: debug builds die earlier and louder (BlockMap::sync's row-accountingdebug_assert, or subtract-with-overflow in the tab/wrap maps), while release builds — no debug assertions, and no overflow checks in[profile.release], so those subtractions silently wrap — continue to the stale header. This mirrors how "cannot seek backward" families outnumber ZED-7G6's 171 events in Sentry.FoldMap::sync"cannot seek backward" (reachable from the same fixture, dozens of seeds): the unsorted fold tree propagates until a sync cursor seeks backward. The same panic message accounts for ~30 open Sentry issues, including ZED-95K (200 events / 151 users), whose stack blames highlight anchors on the same render path — another consumer of the same disease.Two tests fail on purpose and keep CI red until the root cause is fixed:
test_folds_stay_sorted_when_diff_base_text_replaced(the comparator instability) andtest_removing_buffer_removes_header_after_diff_base_changes(the stale header). Drop the corresponding commits if a green branch is needed in the meantime.Supporting infrastructure added along the way: the fold map's fold-order invariant dumps both offending folds' anchor internals and resolved offsets on failure (how the time-variant comparator branch was identified); two
MultiBufferSnapshotinvariants run inside every existing test; the "buffer snapshot not found" panic is self-describing, enumerating the snapshot's buffers, excerpts, path indexes, and anchor buffer ids; and the display map's test-only invariants can be skipped per-thread (production_simulation::SimulateProductionGuard) or process-wide (SIMULATE_PRODUCTION=1) so corruption propagates the way it does in production builds, where no invariants run.Testing:
test_sync_after_path_key_reused_for_different_buffer,test_set_excerpts_with_reversed_and_out_of_bounds_ranges, andtest_mark_serialization_with_unsorted_anchorsfail before their fixes and pass after.test_folds_stay_sorted_when_diff_base_text_replacedandtest_removing_buffer_removes_header_after_diff_base_changesdeterministically reproduce the unfixed root cause and its ZED-7G6 endpoint (red by design, see above).ITERATIONS=1000+runs of the three display-layer fuzzers pass atOPERATIONS=40..80.editor(897 of 899; the 2 failures are the deliberate red tests),multi_buffer(59),vim(564), andcollab(186) suites pass.Root cause analysis conclusion. The causal chain for ZED-7G6 is complete, with a deterministic test at each end and every intermediate link observable. The root cause is that multibuffer anchor comparison and resolution are not stable across structural changes (a path key reused for a different buffer, a buffer leaving, a diff base text replaced), while consumers throughout the display map assume they are. From that root: a diff base change unsorts the fold map's anchor-keyed fold tree; the disorder propagates through syncs that emit edits misdescribing the actual change (caught by the new
assert_wrap_edits_cover_changescoverage checker); the block map recomputes only rows the edits cover, by design, so a removed buffer's header block survives, referencing a departed buffer — ZED-7G6's exact panic. Most corrupted runs die earlier and louder in the "cannot seek backward" / "cannot summarize backward" families, matching their higher Sentry event counts relative to ZED-7G6.The curative fix is at the root, not the crash sites: either re-key anchor-sorted structures when their comparator's inputs change (e.g. re-anchor folds when a diff base is replaced), or give anchors an invalidation contract (a structure version that forces re-resolution). That is a design decision for the owners of
multi_buffer; this PR deliberately stops at diagnosis, reproduction, and the three caller-level fixes above.Release Notes: