From 9b448ede747f2e29ad185dcc9a2a36b49bb20efb Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:54:07 +0800 Subject: [PATCH] Fold the rule's directory into command identity (fixes #167) 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) Claude-Session: https://claude.ai/code/session_01HDzr9JV1ofTZKCL1H2qDPg --- include/pup/index/format.hpp | 7 +++- src/cli/cmd_build.cpp | 11 ++--- src/graph/dag.cpp | 7 +++- .../fixtures/identity_collision/Tupfile.ini | 0 .../identity_collision/a/Tupfile.fixture | 2 + .../fixtures/identity_collision/a/config.h | 4 ++ test/e2e/fixtures/identity_collision/a/main.c | 3 ++ .../identity_collision/b/Tupfile.fixture | 2 + .../fixtures/identity_collision/b/config.h | 4 ++ test/e2e/fixtures/identity_collision/b/main.c | 3 ++ test/unit/test_e2e.cpp | 38 +++++++++++++++++ test/unit/test_graph.cpp | 42 +++++++++++++++++++ 12 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 test/e2e/fixtures/identity_collision/Tupfile.ini create mode 100644 test/e2e/fixtures/identity_collision/a/Tupfile.fixture create mode 100644 test/e2e/fixtures/identity_collision/a/config.h create mode 100644 test/e2e/fixtures/identity_collision/a/main.c create mode 100644 test/e2e/fixtures/identity_collision/b/Tupfile.fixture create mode 100644 test/e2e/fixtures/identity_collision/b/config.h create mode 100644 test/e2e/fixtures/identity_collision/b/main.c diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index 874df617..a18f03b3 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -36,7 +36,12 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// 14 - Guard-unsatisfied commands are no longer serialized; v13 indexes carry /// their identities and would mask reactivated output-less commands as /// already known, so they never run (issue #118) -inline constexpr auto INDEX_VERSION = std::uint32_t { 14 }; +/// 15 - Command identity folds the rule's source directory; v14 identities are +/// directory-blind, so identical rules in sibling directories collide and +/// resolve to one another through the identity join (issue #167). Also +/// covers OrderOnly edges, persisted since issue #166 and load-bearing for +/// removal routing; v14 indexes predate them and would lose one removal. +inline constexpr auto INDEX_VERSION = std::uint32_t { 15 }; /// Index file header (56 bytes) - v9 struct alignas(8) RawHeader { diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index 8dc64ab1..3fbf81d3 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -979,7 +979,8 @@ auto is_dir_authoritative( /// Carry forward old-index commands from directories this run has no /// authoritative knowledge of, so a scoped build's saved index still describes /// the whole project (issue #125). Copies each command with its operand files -/// (stat data preserved) and Normal/Sticky edges, remapping ids to the new +/// (stat data preserved) and every edge except Implicit — OrderOnly among them, +/// which carries removal routing for order-only inputs — remapping ids to the new /// index's dense sequences; implicit edges are re-attached afterwards by /// preserve_old_implicit_edges through the identity match. auto merge_out_of_scope_commands( @@ -1469,11 +1470,11 @@ auto reconcile_input_set( if (!file) { continue; } - // The complementary half of the input edge types; expand_implicit_deps - // routes Implicit and Sticky through the same index for any changed file. + // Edge types that reach a command in one hop; expand_implicit_deps routes + // Implicit and Sticky. Group is neither: it runs file -> group, and the + // group -> command hop is a separate OrderOnly edge, so it needs two (#169). for (auto const* edge : idx.edges_from(pup::NodeId { file->id })) { - if (edge->type != pup::LinkType::Normal && edge->type != pup::LinkType::OrderOnly - && edge->type != pup::LinkType::Group) { + if (edge->type != pup::LinkType::Normal && edge->type != pup::LinkType::OrderOnly) { continue; } auto const* cmd = idx.find_command_by_id(pup::NodeId { edge->to }); diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 2f244c1f..7b9a85d7 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -1035,10 +1035,16 @@ auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cach { auto& pool = global_pool(); auto state = sha256_init(); + auto constexpr SEP = std::byte { 0 }; // Base: the fully-expanded command text (instruction + operand paths + in-text vars). state = sha256_update(state, pool.get(expand_instruction(graph, cmd_id, cache))); + // Command text is Tupfile-relative, so the same rule in sibling directories renders + // identically; without the directory those distinct rules share one identity. + state = sha256_update(state, std::span { &SEP, 1 }); + state = sha256_update(state, pool.get(get(graph, cmd_id))); + // Fold in (name, value-hash) of each Variable node reached via a Sticky edge. // This captures vars that affect output without appearing in the rendered text — // exported env vars the subprocess reads as $VAR, config vars gating an export, etc. @@ -1057,7 +1063,6 @@ auto compute_command_identity(Graph const& graph, NodeId cmd_id, PathCache& cach vars.end() ); - auto constexpr SEP = std::byte { 0 }; for (auto const& [name, value_hash] : vars) { state = sha256_update(state, std::span { &SEP, 1 }); state = sha256_update(state, name); diff --git a/test/e2e/fixtures/identity_collision/Tupfile.ini b/test/e2e/fixtures/identity_collision/Tupfile.ini new file mode 100644 index 00000000..e69de29b diff --git a/test/e2e/fixtures/identity_collision/a/Tupfile.fixture b/test/e2e/fixtures/identity_collision/a/Tupfile.fixture new file mode 100644 index 00000000..d7a4438e --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/Tupfile.fixture @@ -0,0 +1,2 @@ +: main.c |> gcc -c %f -o %o |> main.o +: main.o |> gcc %f -o %o |> program diff --git a/test/e2e/fixtures/identity_collision/a/config.h b/test/e2e/fixtures/identity_collision/a/config.h new file mode 100644 index 00000000..411dc946 --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/config.h @@ -0,0 +1,4 @@ +#ifndef CONFIG_H +#define CONFIG_H +#define VERSION 1 +#endif diff --git a/test/e2e/fixtures/identity_collision/a/main.c b/test/e2e/fixtures/identity_collision/a/main.c new file mode 100644 index 00000000..2ae5408d --- /dev/null +++ b/test/e2e/fixtures/identity_collision/a/main.c @@ -0,0 +1,3 @@ +#include +#include "config.h" +int main(void) { printf("Version %d\n", VERSION); return 0; } diff --git a/test/e2e/fixtures/identity_collision/b/Tupfile.fixture b/test/e2e/fixtures/identity_collision/b/Tupfile.fixture new file mode 100644 index 00000000..d7a4438e --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/Tupfile.fixture @@ -0,0 +1,2 @@ +: main.c |> gcc -c %f -o %o |> main.o +: main.o |> gcc %f -o %o |> program diff --git a/test/e2e/fixtures/identity_collision/b/config.h b/test/e2e/fixtures/identity_collision/b/config.h new file mode 100644 index 00000000..411dc946 --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/config.h @@ -0,0 +1,4 @@ +#ifndef CONFIG_H +#define CONFIG_H +#define VERSION 1 +#endif diff --git a/test/e2e/fixtures/identity_collision/b/main.c b/test/e2e/fixtures/identity_collision/b/main.c new file mode 100644 index 00000000..2ae5408d --- /dev/null +++ b/test/e2e/fixtures/identity_collision/b/main.c @@ -0,0 +1,3 @@ +#include +#include "config.h" +int main(void) { printf("Version %d\n", VERSION); return 0; } diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index c6d1481e..1ca228e4 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -1107,6 +1107,44 @@ SCENARIO("Implicit dependencies track header changes", "[e2e][incremental]") } } +SCENARIO("Implicit deps survive identical rules in sibling directories", "[e2e][incremental][identity]") +{ + // Command text is Tupfile-relative, so these two rules render the same string. + // If identity ignores the directory they collide, and one directory's header + // edges get attached to the other's command. + auto env = EnvGuard { "PUP_IMPLICIT_DEPS", "1" }; + + GIVEN("two directories whose rules and sources are byte-identical") + { + auto f = E2EFixture { "identity_collision" }; + REQUIRE(f.init().success()); + REQUIRE(f.build().success()); + REQUIRE(f.run("a/program").stdout_output == "Version 1\n"); + REQUIRE(f.run("b/program").stdout_output == "Version 1\n"); + + WHEN("only the second directory's header changes") + { + f.write_file("b/config.h", "#ifndef CONFIG_H\n" + "#define CONFIG_H\n" + "#define VERSION 2\n" + "#endif\n"); + auto result = f.build(); + + THEN("that directory rebuilds") + { + REQUIRE(result.success()); + REQUIRE(f.run("b/program").stdout_output == "Version 2\n"); + } + + THEN("the other directory is untouched") + { + REQUIRE(result.success()); + REQUIRE(f.run("a/program").stdout_output == "Version 1\n"); + } + } + } +} + SCENARIO("Implicit deps cover every source of a multi-source command", "[e2e][incremental]") { // gcc -M emits one rule per source; stopping at the first leaves b.h untracked and the program silently stale diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index 6c1997c7..bfa3c129 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1636,3 +1636,45 @@ TEST_CASE("node_id encoding: 30-bit index roundtrips and kinds are mutually excl REQUIRE_FALSE(pup::node_id::is_file(pup::INVALID_NODE_ID)); } + +TEST_CASE("compute_command_identity separates rules by directory", "[graph][identity]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + // Command text is Tupfile-relative, so identical rules in sibling directories + // render the same string; identity must still tell them apart. + auto in_a = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("cat src.txt > out.txt"), + }); + auto in_b = add_command_node(g, CommandNode { + .source_dir = intern("b"), + .instruction_id = intern("cat src.txt > out.txt"), + }); + REQUIRE(in_a.has_value()); + REQUIRE(in_b.has_value()); + + SECTION("same text in different directories yields different identities") + { + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + != compute_command_identity(g, *in_b, bs.path_cache)); + } + + SECTION("identity is stable for the same command") + { + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + == compute_command_identity(g, *in_a, bs.path_cache)); + } + + SECTION("different text in the same directory yields different identities") + { + auto other = add_command_node(g, CommandNode { + .source_dir = intern("a"), + .instruction_id = intern("cat other.txt > out.txt"), + }); + REQUIRE(other.has_value()); + CHECK(compute_command_identity(g, *in_a, bs.path_cache) + != compute_command_identity(g, *other, bs.path_cache)); + } +}