Fold the rule's directory into command identity (fixes #167) - #173
Merged
Conversation
Command text is Tupfile-relative, so the same rule in sibling directories renders the same string. compute_command_identity hashed that text plus the sticky Variable values and nothing else, so those distinct rules shared one identity. find_by_identity is a lower_bound, so one twin silently won every lookup. Identity is the cross-build join key for expand_implicit_deps, preserve_old_implicit_edges, merge_out_of_scope_commands, remove_stale_outputs, detect_new_commands and reconcile_input_set, so the collision misrouted all of them. Two observed consequences: A scoped build dropped every out-of-scope command from the saved index -- merge_out_of_scope_commands saw the twin's identity already present and concluded the command was carried forward. Afterwards a full build detected nothing at all: not the out-of-scope edits, not entirely new directories. Implicit header edges attached to the wrong twin. Two directories compiling a same-named source with the same command line is an ordinary layout, and editing one's header left its object byte-identical, in a plain full build with no scoping, globs or variant directory involved. The hash was simply missing a term of the rule's definition; identity still deliberately excludes the input set, which #166 handled on the state side. INDEX_VERSION 14 -> 15. Without the bump every stored identity would stop matching at once, so remove_stale_outputs would delete every output and rebuild it; the bump discards v14 indexes cleanly instead. The entry also records that v14 predates the OrderOnly edges persisted for #166 -- that content change shipped without a bump, and this gate is what closes it, so a future revert must not silently drop it. Also from the systemic review of this change: reconcile_input_set no longer admits LinkType::Group. Group edges run file -> group and the group -> command hop is a separate OrderOnly edge, so find_command_by_id rejected every one of them: the arm was dead code that read as coverage. Removed and filed as #169 rather than replaced with a two-hop walk I could not demonstrate fixing anything. merge_out_of_scope_commands' header said it copies "Normal/Sticky edges"; it has carried everything except Implicit since #166, OrderOnly included, and that one is load-bearing for removal routing after a scoped build. Filed separately, all pre-existing: #170 (identity injectivity is assumed by every consumer and never enforced -- this bug would have been an error, not a stale object), #171 (glob results sort by StringId handle, so %f order follows readdir order and command lines are not reproducible across machines), #172 (remove_stale_outputs runs after all detection, so its own deletions are never routed to consumers). Verified: 615 cases / 142,305 assertions / 32 shards. The former #167 repro now keeps the index at 11 files/2 commands where it fell to 7/1, sees the out-of-scope edit and the new directory, and settles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDzr9JV1ofTZKCL1H2qDPg
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Test coverage (lines)
103 files · 14983/17313 lines covered Deltas vs main@787968b7e. Updated for 9b448ed |
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.
Fixes #167.
The bug
Command text is Tupfile-relative, so the same rule in sibling directories renders the same string.
compute_command_identityhashed that text plus sticky Variable values and nothing else, so two distinct rules shared one identity.find_by_identityis alower_bound, so one twin silently won every lookup.Identity is the cross-build join key for
expand_implicit_deps,preserve_old_implicit_edges,merge_out_of_scope_commands,remove_stale_outputs,detect_new_commandsandreconcile_input_set— the collision misrouted all of them.Lost header dependencies, in a plain full build. Two directories compiling a same-named source with the same command line is an ordinary layout:
b's command textb/main.oafter editingb/dep.ha's-O0added)No scoping, no globs, no
-B. Byte-identical object after the header changed, with no diagnostic.Index truncation. A scoped build dropped every out-of-scope command, because
merge_out_of_scope_commandssaw the twin's identity already present and concluded the command was carried forward. Afterwards a full build detected nothing at all — not out-of-scope edits, not entirely new directories. That was #167 as originally filed; it turned out to be a symptom, and the-Bimmunity I first reported was a coincidence of the repro (variant builds render distinct%opaths, so the text differs).The fix
Fold the rule's source directory into the hash. The join key must denote which rule this is, and a Tupfile-relative rendering under-determines that — the hash was missing a term of the rule's definition. Identity still deliberately excludes the input set; #166 handled that side separately.
INDEX_VERSION14 → 15. Without the bump every stored identity stops matching at once, soremove_stale_outputswould delete every output and rebuild it; the bump discards v14 indexes cleanly instead. Verified by patching a live index's version byte back to 14 — one rebuild, then settles.The version entry also records that v14 predates the
OrderOnlyedges persisted for #166. That content change shipped without a bump, and this gate is what closes it — so a future revert of this PR must not silently drop it.From the systemic review of this change
reconcile_input_setno longer admitsLinkType::Group. Group edges run file → group, and the group → command hop is a separateOrderOnlyedge, sofind_command_by_idrejected every one: the arm was dead code that read as coverage. Removed and filed as Group-member removal is not routed: reconcile_input_set walks one hop, group membership needs two #169 rather than replaced with a two-hop walk I could not demonstrate fixing anything.merge_out_of_scope_commands' header claimed it copies "Normal/Sticky edges". It has carried everything exceptImplicitsince Globs re-expand only on Tupfile content change, so newly added files are invisible #166,OrderOnlyincluded — and that one is load-bearing for removal routing after a scoped build.Filed separately (all pre-existing, none fixed here)
build_identity_mapalready sorts, so adjacent-equal detection is ~free and would have made this bug an error instead of a stale object.StringIdhandle, not path, so%forder followsreaddirorder. Measured: putup's%fmatchesls -Uexactly, neverls. Command lines are not reproducible across machines, which undercuts the premise of the identity hash.remove_stale_outputsruns after all detection, so its own deletions are never routed to consumers. One build exits 0 with a stale output; the next heals it.Tests
Both observed RED before the fix:
test_graph.cpp) — same text, differentsource_dir, must differ. RED showed byte-identical hashes.identity_collision) — two directories with byte-identical Tupfiles and sources; editing onlyb/config.hmust moveb/programto "Version 2" whileastays at 1. RED showedbstuck at 1.Verification
615 cases / 142,305 assertions / 32 shards;
format,tidy,iwyuclean; no-op stability holds; no bootstrap drift. The original #167 repro now keeps the index at 11 files/2 commands where it fell to 7/1, sees the out-of-scope edit and the new directory, and settles.🤖 Generated with Claude Code
https://claude.ai/code/session_01HDzr9JV1ofTZKCL1H2qDPg