Bound range-diff squash fabrication in rewrite mapping derivation#1978
Draft
scouredimage wants to merge 1 commit into
Draft
Bound range-diff squash fabrication in rewrite mapping derivation#1978scouredimage wants to merge 1 commit into
scouredimage wants to merge 1 commit into
Conversation
A restack undo (git reset --keep <pre-rebase-tip>, what Graphite runs when undoing a restack) makes derive_mappings_from_range_diff compare merge-base..rebased-tip against merge-base..orig-tip. The merge base is the branch's fork point, so the old range contains every trunk commit landed since the fork — and parse_range_diff_output mapped every unmatched '<' commit onto a neighboring kept commit, fabricating one mapping per trunk commit (a 4-commit branch move producing 600+ mappings). Each fabricated mapping whose source carries a note becomes a full-root-tree diff pair that reverses the entire trunk delta; the parsed per-pair structures accumulate unboundedly. Production incident on a large monorepo: ~30 GB RSS and machine-wide memory pressure. This fires despite the streamed diff fix (b0efde3): the raw text is streamed, the parsed structures are not. Two independent bounds: - parse_range_diff_output now drops the fabricated squash mappings wholesale when the dropped-commit count dwarfs the matched pairs (limit: max(64, 8 x matched)). A real squash folds a handful of commits; thousands of unmatched commits mean the ranges are not two versions of the same branch, and there is nothing meaningful to map. Matched pairs survive, so genuine note migration is unaffected. - derive_mappings_from_range_diff skips derivation entirely when either range side exceeds GIT_AI_RANGE_DIFF_COMMIT_LIMIT (default 1000) commits, as an outer wall for the range-diff/diff-tree spawns themselves (one rev-list --count per side). scripts/repro/daemon-restack-undo-mapping-bomb/ contains a fully synthetic, isolated reproduction with RSS measurements and knobs. Co-Authored-By: Claude Fable 5 <[email protected]>
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.
Problem
A restack undo —
git reset --keep <pre-rebase-tip>, exactly what Graphite runs when a restack is undone or aborted — drives the daemon's non-fast-forward rewrite handling withold_tip = rebased tip,new_tip = original tip. The merge base of those two is the branch's fork point, so the old side of the range-diff contains every trunk commit landed since the branch forked — thousands on a busy monorepo.parse_range_diff_outputmapped every unmatched<commit onto a neighboring kept commit (the squash heuristic, applied without any bound). The daemon literally derives 604 mappings for a 4-commit branch move. Each fabricated mapping whose source commit carries an authorship note (on a fleet with notes on effectively every commit: all of them) becomes a full-root-tree-U0 -Mdiff pair that reverses the entire trunk delta, and the parsed per-pair structures (added_lines_by_file: oneu32per+line, plus hunks) accumulate until every pair is processed:This fires despite the streamed-diff fix (#1902 / b0efde3) — the raw patch text is streamed, the parsed structures are not. Production incident: ~30 GB RSS on a large-monorepo deployment, with machine-wide memory pressure making every git command crawl. Fully synthetic, isolated repro with RSS measurements included in this PR under
scripts/repro/daemon-restack-undo-mapping-bomb/(150 fabricated mappings at SCALE=small; 604 at default, +206 MB RSS from a 28 MB baseline on a debug build).Fix
Two independent bounds:
parse_range_diff_outputdrops fabricated squash mappings wholesale when the dropped-commit count dwarfs the matched pairs (limit:max(64, 8 × matched)). A genuine squash folds a handful of commits into an adjacent kept one; thousands of unmatched commits mean the two ranges are not versions of the same branch, and mapping them is meaningless — a restack undo is not a squash of 3,000 trunk commits into a 4-commit stack. Matched (=/!) pairs always survive, so real note migration is unaffected — and on a restack undo the original tips already hold their notes, so nothing is lost.derive_mappings_from_range_diffskips derivation when either range side exceedsGIT_AI_RANGE_DIFF_COMMIT_LIMIT(default 1000) commits — an outer wall for therange-diff/diff-treespawn cost itself. Onegit rev-list --countper side (constant spawns).Deliberately not included (noted as follow-up): chunking
compute_diff_trees_batchsoDiffTreeResults are dropped as applied. With fabrication bounded, pair counts return to real stack sizes and per-pair diffs return to commit-sized deltas, so the accumulation ceases to be the dominant factor.Tests
parse_range_diff_keeps_proportionate_squash_mappings— genuine squash fabrication preserved (existing squash tests also still pass).parse_range_diff_drops_disproportionate_squash_mappings— 70 dropped vs 2 matched: only the matched pairs survive. Red without the fix.derive_mappings_restack_undo_yields_only_stack_mappings— real repo, repro-shaped at small scale: 2-commit branch, 70-commit trunk advance, rebase, then mapping derivation in the undo direction returns exactly the 2 stack mappings with zero trunk leakage. Red without the fix.derive_mappings_skips_when_range_exceeds_commit_limit— the outer wall, env-overridable limit. Red without the guard.authorship::rewriteunit tests green.Related: #1902 fixed the raw-text buffering on this same path; #1677 reported the same user-visible symptom from an earlier variant.
🤖 Generated with Claude Code