graph: refuse a second node at a path another node already owns

[typeless]

add_file_node threw away the bool from both of its registrations. Neither is an insert-or-fail — SortedPairVec::insert overwrites the value on an existing key and returns false — so a second node at an occupied key won silently and the node that was there became unreachable by path while still sitting in the graph.

That is the shape behind both shared-namespace bugs found this week: #484 (putup's own tool fingerprints sharing the Tupfile export/import key space) and #486 (a config variable hiding a same-named source file). Both surfaced days later as a stale build, and both were found by accident.

Closes #487.

The fix

The check runs before graph.next_file_id++, so the error path leaves the graph untouched:

auto const names_a_path = !is_empty(node.name) && is_path_addressable(node.type);
if (names_a_path) {
    auto const* occupant = graph.path_to_node.find(to_underlying(node.path_id));
    if (!occupant && parent_idx < graph.dir_children.size()) {
        occupant = graph.dir_children[parent_idx].find(to_underlying(node.name));
    }
    if (occupant) { ... return make_error<NodeId>(ErrorCode::DuplicateNode, err.view()); }
}

Why both maps

They key on different functions of the same node:

map	key
path_to_node	intern(parent node's path_id, name)
dir_children	(parent node index, name)

Guarding only path_to_node would leave dir_children as a second, independently keyed resolution leg in ensure_file_node, and the losing node would still bind under a different PathId — one file resolving to two nodes depending on how it is spelled, which is worse than today's consistent-but-wrong.

set_build_root_name is the one state where the two genuinely disagree: it writes dir_children[0][<build root name>] -> BUILD_ROOT_ID with no path_to_node counterpart. The new test reaches the second leg through it — delete the leg and that section fails:

test_graph.cpp:1653: FAILED:
  REQUIRE_FALSE( clash.has_value() )
with expansion:
  !true

What is deliberately not changed

SortedPairVec::insert keeps its upsert contract. VarDb::set and VarDb::append depend on last-wins — that is what Tupfile VAR = and VAR += mean — and test_sorted_id_vec pins it. Making the container insert-or-fail, or marking insert [[nodiscard]], would push a cast onto every legitimate upsert site to fix one call site that can check for itself.

Upstream

tup makes the same uniqueness structural rather than conventional: unique(dir, name) on the node table, every insert through tup_db_node_insert_tent_display with a checked step, and tupid_tree_insert / string_tree_insert failing on a duplicate while a separate tupid_tree_add_dup opts into tolerance. A duplicate aborts the parse.

Reachability, stated honestly

The error is expected to be dead code on every project that builds today. Nine adversarial fixtures failed to reach the branch from the CLI:

fixture	result
a file literally named <g> against a group of that name	conflates onto one node, no second add
a source directory named $ against the env pseudo-directory	pre-check finds the real directory
a source directory named $ holding a file named FOO=bar, with export FOO	two nodes on one PathId, but Variable writes neither map
in-source -B .	duplicate label, distinct PathIds
a source subdirectory named like the build root	correct — same directory on disk
a config variable against a generated output in the config dir	same as the $ case

That is a failure to reach, not a proof of unreachability — which is the argument for an error over a diagnostic nobody would ever see. The four blast-radius shapes (<g>, $, -B ., -B build with a source build/) behave exactly as they did before the change; none produces a new DuplicateNode.

Verification

• Red-then-green: the new case fails on the pre-change binary (REQUIRE_FALSE(second.has_value()) → !true) and passes after.
• make check rc=0. 182526 assertions across 956 test cases, up from 182495 / 955 at dddfb5620.
• Comment sweep on added lines: 3, all /// doc headers conforming to the convention link_role / names_node_type / is_path_addressable already follow in types.hpp. 0 deleted.
• Cross-model pair review (Fable) returned no blocker; both of its findings — the unpinned dir_children leg and a message that named the same path twice — are fixed in this branch.

Not taken, and residuals

Folding the two maps into one binding table. A surface-compare verdict preferred it: it removes the disagreement structurally instead of checking for it, and dir_children has exactly one reader. It is not taken here because find_by_dir_name would then read a table that deliberately holds alias entries, and that equivalence is undischarged in precisely the variant-identity paths behind most of the incremental-correctness campaign's bugs. Filed as #493 with the ledger.

No spec requirement. spec/requirements/ has no sentence describing add_file_node's behaviour on a second node at one (dir, name), so nothing is altered and make spec-check is not triggered.

Found while investigating, filed separately, and not closed by this PR:

• A file named like a group in the same directory merges with the group, and which one wins depends on rule order #491: a file named like a group in the same directory merges with the group, and which one wins depends on rule order. Reproduces as a stale build today. This is the Group-stays-path-addressable residual from A config variable aliases a same-named source file in the config directory, and edits to that file become invisible #486.
• add_file_node re-roots a node whose parent it cannot resolve instead of refusing it #492: add_file_node re-roots a node whose parent it cannot resolve instead of refusing it. It's latent, with no production trigger found. This PR turns one consequence into a DuplicateNode that names the wrong path.
• Graph keeps two path indexes whose agreement no code enforces #493: path_to_node and dir_children must agree, and no code enforces that. This is the fold-into-one-table follow-up, with the surface-compare ledger.

Correction: an earlier version of this description listed set_build_root_name's one-sided write as a separate defect. It isn't one. Both callers run it on a fresh graph before parsing, and it's the deliberate aliasing that makes a source-relative build/... spelling reach the build root. It's the main example in #493, and it's still the state this PR's second lookup leg covers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01StgwMENEyfBnEoe4pvAdtQ

[github-actions]

PR metrics

Performance (gcc example, Linux)

Workload	Instructions	CPU time	Page faults	D1 miss	LL miss	Wall	Peak RSS
parse	1759 M (+0.1%)	0.63 s	15.2 k	0.7% (+0.1pp)	0.1% (+0.1pp)	0.651 s	35.1 MB (-0.1MB)
dry-run	2485 M	0.74 s	16.8 k	0.8% (+0.2pp)	0%	0.781 s	41.4 MB

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)

Metric	Value
Tupfiles parsed	24
Commands	3545
Commands scheduled	0
Files checked	5834
Files changed	0
Files in index	6188
Graph edges	384927
Index size (bytes)	7869819
Implicit deps	344126
Hash computations	197
Hashes skipped (stat cache)	5636
Stat calls	5885
Parse time (ms)	577.8
Total time (ms)	746.7
Runner CPU	AMD EPYC 7763 64-Core Processor

Counters from putup -n --stat on the fully-built gcc example (up-to-date dry run): deterministic work measures — a jump in commands scheduled, hash computations, or stat calls is a real behavior change, not noise. Timings are the minimum over repeated runs, compared only against a baseline from the same CPU model; the counters are the regression signal.
Timing deltas suppressed: baseline ran on different hardware (INTEL(R) XEON(R) PLATINUM 8573C).

Binary size (Linux)

Binary	.text	.data	.bss	File
putup	602.4 KB (+0.2%)	2.4 KB	98.8 KB	711.9 KB

Code churn (whole codebase, last 30d)

Files	Lines written	Still present	Churned	Churn rate
32	2498	2161	337	13.5%

Of the lines written across the codebase in the last 30 days, how many are already gone — work that was written and then discarded or rewritten inside the same window. This is the state of the tree including this PR, not a measure of the PR itself. Only code we write is counted: tests, examples, vendored and generated files, CI plumbing and prose are excluded. 1613 lines were deleted in the window in total, most of them older than it.

Where the churn is

File	Lines written then discarded
src/parser/eval.cpp	77
src/graph/builder.cpp	72
src/graph/dag.cpp	56
src/index/entry.cpp	44
include/pup/core/token_list.hpp	23
include/pup/parser/eval.hpp	16
src/cli/cmd_build.cpp	8
src/index/reader.cpp	8
include/pup/core/instruction.hpp	7
src/core/instruction.cpp	7

Test coverage (lines)

Overall	Median file	Min file	Max file
88.8% (-0.1pp)	96.7% (-0.2pp)	14.7% include/pup/parser/token.hpp	100.0% include/pup/core/arena.hpp

105 files · 17899/20149 lines covered

Deltas vs main@dddfb5620.

Updated for 31d4b12