From 231731f2de05d734dfcf92e0edc3d35152c9e3ea Mon Sep 17 00:00:00 2001 From: Josh Heinrichs Date: Sat, 22 Aug 2026 10:01:06 -0600 Subject: [PATCH] tecnix: fix target result values getting GC'd mid-call prim_tecnixTargets and the dependency path kept the only references to result Values in plain std::vector buffers, which Boehm GC does not scan. Recycled cells crashed at the ValueStorage::finish pdThunk unreachable ("Unexpected condition in ... finish(...)") or silently corrupted results. Root them in ValueVector / traceable_allocator storage. Regression test: tests/functional/tecnix/gc.sh. --- .../tecnix-target-eval-caching/guardrails.md | 7 + src/libexpr/primops/tecnix.cc | 24 +++- tests/functional/tecnix/gc.sh | 136 ++++++++++++++++++ tests/functional/tecnix/meson.build | 1 + 4 files changed, 162 insertions(+), 6 deletions(-) create mode 100644 tests/functional/tecnix/gc.sh diff --git a/plans/tecnix-target-eval-caching/guardrails.md b/plans/tecnix-target-eval-caching/guardrails.md index 7aa6aaf60..96ed8b8a3 100644 --- a/plans/tecnix-target-eval-caching/guardrails.md +++ b/plans/tecnix-target-eval-caching/guardrails.md @@ -40,6 +40,13 @@ Use this as a review checklist for source-dependency tracking and target-eval ca - **Repo-root source access remains unrepresentable unless the closure format grows an explicit representation.** - Until then, repo-root access must fail closed. +## Value lifetime and GC + +- **Every live `Value *` must be reachable by the conservative collector for its whole lifetime.** + - Boehm GC scans thread stacks, GC-heap objects, and explicitly traceable allocations — not ordinary malloc'd buffers. A plain `std::vector` (or any heap container holding `Value *`, directly or inside a struct) is invisible to it. + - Builtin orchestration that pre-allocates result cells, or holds worker-produced values until a coordinator consumes them, must keep those pointers in GC-visible storage: `ValueVector`, a `traceable_allocator` container, `RootValue`, or a live stack frame. + - A collected-and-recycled cell surfaces as the `ValueStorage::finish` pdThunk panic at best, and as silently wrong target values at worst (the fatal failure mode). `tests/functional/tecnix/gc.sh` holds this guardrail under forced GC pressure. + ## Cache validity - **A persistent cache hit requires one complete stored closure candidate to match current fingerprints.** diff --git a/src/libexpr/primops/tecnix.cc b/src/libexpr/primops/tecnix.cc index 534c0158b..05b62d9aa 100644 --- a/src/libexpr/primops/tecnix.cc +++ b/src/libexpr/primops/tecnix.cc @@ -614,7 +614,13 @@ static void prim_tecnixTargets(EvalState & state, const PosIdx pos, Value ** arg auto & resolveFn = getResolveFunction(state, pos, tArgs); - std::vector values(tArgs.targets.size()); + /* These cells are the only reference to each target's result until the + output bindings are built, so they must live in GC-scanned storage: a + plain std::vector's heap buffer is invisible to the conservative + collector, which would recycle the cells mid-evaluation (observed as + the ValueStorage::finish pdThunk panic, or as silently corrupted + results). */ + ValueVector values(tArgs.targets.size()); for (size_t i = 0; i < tArgs.targets.size(); i++) values[i] = state.allocValue(); @@ -670,6 +676,14 @@ struct TargetDependencyResult bool cacheNeedsUpsert = false; }; +/** + * `targetValue` may be the only reference to a worker-evaluated target value + * until the coordinator assembles the output records, so the results buffer + * must be GC-scanned (see the ValueVector comment in prim_tecnixTargets). + */ +using TargetDependencyResults = + std::vector, traceable_allocator>>; + struct PreparedTrackedResolveFunction { Value * resolveFn; @@ -735,9 +749,7 @@ static TargetDependencyResult evalTargetDependencies( } static void finalizeSourceAccessSetDependencies( - EvalState & state, - std::vector> & results, - DependencyFingerprintCache & fingerprintCache) + EvalState & state, TargetDependencyResults & results, DependencyFingerprintCache & fingerprintCache) { if (!trackedSourceAccessSetGraph(state)->isEnabled()) return; @@ -765,7 +777,7 @@ static void printTecnixAccessSetStats(EvalState & state, std::string_view opName sourceAccessSetStats.accessSetItems); } -static std::vector> evaluateTecnixTargetDependencies( +static TargetDependencyResults evaluateTecnixTargetDependencies( EvalState & state, const PosIdx pos, const TecnixArgs & args, @@ -779,7 +791,7 @@ static std::vector> evaluateTecnixTargetDe useCache ? "enabled" : "disabled", state.executor->evalCores); - std::vector> results(args.targets.size()); + TargetDependencyResults results(args.targets.size()); std::vector misses; size_t cacheHits = 0; diff --git a/tests/functional/tecnix/gc.sh b/tests/functional/tecnix/gc.sh new file mode 100644 index 000000000..5a8cb55a1 --- /dev/null +++ b/tests/functional/tecnix/gc.sh @@ -0,0 +1,136 @@ +#!/usr/bin/env bash +# Regression test: Value cells owned by the Tecnix target builtins must stay +# reachable by the conservative garbage collector for their whole lifetime. +# +# prim_tecnixTargets pre-allocates one result cell per target, and the +# dependency path holds worker-produced target values until the coordinator +# consumes them. Both used to keep the only reference to those cells in plain +# std::vector heap buffers, which Boehm GC does not scan. Under allocation +# pressure the collector recycled the cells mid-call; a recycled cell that had +# become another thread's thunk crashed at the ValueStorage::finish pdThunk +# tripwire ("Unexpected condition in ... finish(...)"), and a recycled cell +# holding an unrelated finished value produced silently wrong results. +# +# This test evaluates many allocation-heavy targets with a tiny initial GC +# heap (forcing frequent collections during the call) and asserts the exact +# expected content of every target, sequentially and in parallel, with and +# without dependency records. + +source "$(dirname "${BASH_SOURCE[0]}")/common.sh" + +requireGit + +NTARGETS=400 + +GC_WORLD="$TEST_ROOT/tecnix-gc-world" + +create_tecnix_gc_test_world() { + local dir="$1" + + git init -q "$dir" + + mkdir -p "$dir/system/tectonix" + cat > "$dir/system/tectonix/resolve.nix" << 'RESOLVE_EOF' +{ n }: +let + # Allocation-heavy per-target computation: enough thunks, strings, and + # lists per target to force GC cycles while the builtin is mid-call. + mkTarget = target: + let + junk = builtins.genList (i: { + s = "x-${target}-${toString i}"; + l = builtins.genList (j: j) 30; + }) 300; + in { + name = target; + marker = builtins.concatStringsSep "," (map (x: x.s) junk); + drvPath = "/nix/store/00000000000000000000000000000000-${target}.drv"; + }; +in +{ + resolve = mkTarget; + allTargetNames = builtins.genList (i: "t${toString i}") n; +} +RESOLVE_EOF + + git -C "$dir" -c user.email=test@example.com -c user.name=Test add -A + git -C "$dir" -c user.email=test@example.com -c user.name=Test commit -qm 'tecnix gc test world' +} + +create_tecnix_gc_test_world "$GC_WORLD" +GC_HEAD_SHA=$(git -C "$GC_WORLD" rev-parse HEAD) + +gc_args() { + cat </dev/null <<< "$json"; then + printf '%.2000s\n' "$json" >&2 + fail "tecnixTargets under GC pressure returned wrong results ($mode)" + fi +} + +echo "Testing tecnixTargets result cells survive GC (sequential)..." +seq_targets=$(gc_eval_json_sequential "builtins.tecnixTargets ($(gc_args))") +assert_gc_targets_intact "$seq_targets" "sequential" + +echo "Testing tecnixTargets result cells survive GC (parallel)..." +par_targets=$(gc_eval_json_parallel "builtins.tecnixTargets ($(gc_args))") +assert_gc_targets_intact "$par_targets" "parallel" + +[[ "$seq_targets" == "$par_targets" ]] || fail "sequential and parallel tecnixTargets output should be identical" + +# The dependency path holds each worker-produced target value until the +# coordinator assembles the records; those cells must be rooted across the +# fingerprinting work in between. +echo "Testing tecnixTargets includeDependencies target values survive GC (parallel)..." +dep_records=$(gc_eval_json_parallel " + builtins.listToAttrs (map (record: { + name = record.target; + value = record.value // { deps = record.dependencies; }; + }) (builtins.tecnixTargets (($(gc_args)) // { includeDependencies = true; })))") +assert_gc_targets_intact "$dep_records" "includeDependencies" +if ! jq -e '[.[] | .deps | has("system/tectonix/resolve.nix")] | all' >/dev/null <<< "$dep_records"; then + fail "includeDependencies records should carry the resolver dependency for every target" +fi diff --git a/tests/functional/tecnix/meson.build b/tests/functional/tecnix/meson.build index e3b79c0e4..8501d81a1 100644 --- a/tests/functional/tecnix/meson.build +++ b/tests/functional/tecnix/meson.build @@ -3,6 +3,7 @@ suites += { 'deps' : [], 'tests' : [ 'builtins.sh', + 'gc.sh', ], 'workdir' : meson.current_source_dir(), }