From 604c003f3ce7fa1f8599ee502da5b4e7f5c7fac9 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Fri, 25 Sep 2026 18:25:00 -0400 Subject: [PATCH 01/10] tecnix: track manifest entries, keys and ids instead of the whole manifest Add builtins.tectonixManifestEntry, tectonixManifestKeys and tectonixManifestIdToPath. Each records a synthetic tracked path (`.meta/manifest.json#`, `#keys` or `#id/`) instead of `.meta/manifest.json`, so a target depends on the part of the manifest it read, not on the whole file. dependencyFingerprintCached fingerprints a synthetic path by reading the manifest from the accessor and hashing the entry's JSON, the sorted key list, or the zone path an id maps to. Recording and validation go through the same function, so an eval-cache row stays valid across commits that change other parts of the manifest (for example, adding an unrelated zone). The parsed manifest is kept per fingerprint-cache generation, so it always belongs to the accessor being fingerprinted. --- src/libexpr/primops/tectonix.cc | 105 +++++++++++++++++++++++++++++++ src/libexpr/tecnix/eval-cache.cc | 64 ++++++++++++++++++- 2 files changed, 168 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index da7066a22..4bdf61bfb 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -107,6 +107,111 @@ static RegisterPrimOp primop_worldManifestInverted({ .impl = prim_worldManifestInverted, }); +// ============================================================================ +// builtins.tectonixManifestEntry zonePath +// Returns null or { id = "W-xxxxxx" } for a single zone, recording only that +// entry as a tracked dependency (synthetic path .meta/manifest.json#) +// instead of the whole manifest file. +// ============================================================================ +static void prim_tectonixManifestEntry(EvalState & state, const PosIdx pos, Value ** args, Value & v) +{ + auto zonePath = state.forceStringNoCtx( + *args[0], pos, "while evaluating the 'zonePath' argument to builtins.tectonixManifestEntry"); + if (auto ctx = currentTecnixThreadState.trackingContext; ctx) + ctx->recordAccess(".meta/manifest.json#" + std::string(zonePath)); + auto & manifest = getManifest(state); + auto it = manifest.find(std::string(zonePath)); + if (it == manifest.end() || !it->is_object() || !it->contains("id") || !(*it).at("id").is_string()) { + v.mkNull(); + return; + } + auto zoneAttrs = state.buildBindings(1); + zoneAttrs.alloc("id").mkString((*it).at("id").get(), state.mem); + v.mkAttrs(zoneAttrs); +} + +static RegisterPrimOp primop_tectonixManifestEntry({ + .name = "__tectonixManifestEntry", + .args = {"zonePath"}, + .doc = R"( + Get a single zone's manifest entry as a Nix attrset { id = "W-xxxxxx"; }, + or null if the zone does not exist. Records only that entry as a Tecnix + tracked dependency (synthetic path .meta/manifest.json#), not + the whole manifest file. + Example: `builtins.tectonixManifestEntry "//areas/tools/dev"` returns `{ id = "W-123456"; }`. + )", + .impl = prim_tectonixManifestEntry, +}); + +// ============================================================================ +// builtins.tectonixManifestKeys +// Returns the sorted list of zone paths (manifest keys), recording only the +// key set as a tracked dependency (synthetic path .meta/manifest.json#keys). +// ============================================================================ +static void prim_tectonixManifestKeys(EvalState & state, const PosIdx pos, Value ** args, Value & v) +{ + if (auto ctx = currentTecnixThreadState.trackingContext; ctx) + ctx->recordAccess(".meta/manifest.json#keys"); + auto & manifest = getManifest(state); + std::vector keys; + keys.reserve(manifest.size()); + for (auto & [path, value] : manifest.items()) + keys.push_back(path); + std::sort(keys.begin(), keys.end()); + auto list = state.buildList(keys.size()); + for (size_t i = 0; i < keys.size(); i++) { + auto * val = state.allocValue(); + val->mkString(keys[i], state.mem); + list[i] = val; + } + v.mkList(list); +} + +static RegisterPrimOp primop_tectonixManifestKeys({ + .name = "__tectonixManifestKeys", + .args = {}, + .doc = R"( + Get the sorted list of zone paths (manifest keys). Records only the key + set as a Tecnix tracked dependency (synthetic path .meta/manifest.json#keys), + not the whole manifest file. Only world-wide folds that enumerate every + zone should use this; per-target resolution should use tectonixManifestEntry. + )", + .impl = prim_tectonixManifestKeys, +}); + +// ============================================================================ +// builtins.tectonixManifestIdToPath zoneId +// Returns null or the zone path for a zone ID, recording only that id lookup +// as a tracked dependency (synthetic path .meta/manifest.json#id/). +// ============================================================================ +static void prim_tectonixManifestIdToPath(EvalState & state, const PosIdx pos, Value ** args, Value & v) +{ + auto zoneId = state.forceStringNoCtx( + *args[0], pos, "while evaluating the 'zoneId' argument to builtins.tectonixManifestIdToPath"); + if (auto ctx = currentTecnixThreadState.trackingContext; ctx) + ctx->recordAccess(".meta/manifest.json#id/" + std::string(zoneId)); + auto & manifest = getManifest(state); + for (auto & [path, value] : manifest.items()) { + if (value.contains("id") && value.at("id").is_string() && value.at("id").get() == zoneId) { + v.mkString(path, state.mem); + return; + } + } + v.mkNull(); +} + +static RegisterPrimOp primop_tectonixManifestIdToPath({ + .name = "__tectonixManifestIdToPath", + .args = {"zoneId"}, + .doc = R"( + Get the zone path for a zone ID, or null if not found. Records only that + id lookup as a Tecnix tracked dependency (synthetic path + .meta/manifest.json#id/), not the whole manifest file. + Example: `builtins.tectonixManifestIdToPath "W-123456"` returns `"//areas/tools/dev"`. + )", + .impl = prim_tectonixManifestIdToPath, +}); + static std::string normalizeTrackedRepoPath(std::string_view path); // ============================================================================ diff --git a/src/libexpr/tecnix/eval-cache.cc b/src/libexpr/tecnix/eval-cache.cc index 9bbaece8b..9e41aba25 100644 --- a/src/libexpr/tecnix/eval-cache.cc +++ b/src/libexpr/tecnix/eval-cache.cc @@ -29,6 +29,8 @@ #include #include #include +#include +#include "nix/util/hash.hh" #include namespace nix { @@ -47,6 +49,9 @@ struct DependencyFingerprintThreadLocalCache { uint64_t generation = 0; boost::unordered_flat_map, StringViewHash, std::equal_to<>> fingerprints; + /** Cached parsed manifest JSON for synthetic path fingerprinting (per-batch). */ + std::optional cachedManifest; + bool manifestChecked = false; DependencyFingerprintThreadLocalCache() { @@ -1334,10 +1339,48 @@ static DependencyFingerprintThreadLocalCache & getDependencyFingerprintThreadCac if (threadCache.generation != cache.generation) { threadCache.generation = cache.generation; threadCache.fingerprints.clear(); + // The parsed manifest belongs to the accessor this generation reads. + threadCache.cachedManifest.reset(); + threadCache.manifestChecked = false; } return threadCache; } +static std::optional manifestPathFingerprint(const nlohmann::json & manifest, std::string_view suffix) +{ + constexpr std::string_view keysToken = "keys"; + if (suffix == keysToken) { + // Fingerprint the sorted key list + std::vector keys; + keys.reserve(manifest.size()); + for (auto & [k, v] : manifest.items()) + keys.push_back(k); + std::sort(keys.begin(), keys.end()); + std::string concatenated; + for (auto & k : keys) + concatenated += k + "\n"; + auto hash = hashString(HashAlgorithm::SHA256, concatenated); + return "manifest-keys:" + hash.to_string(HashFormat::Base64, false); + } + constexpr std::string_view idPrefix = "id/"; + if (suffix.starts_with(idPrefix)) { + auto zoneId = suffix.substr(idPrefix.size()); + for (auto & [path, value] : manifest.items()) { + if (value.contains("id") && value.at("id").is_string() && value.at("id").get() == zoneId) + return "manifest-id:" + std::string(zoneId) + "=" + path; + } + return "manifest-id:" + std::string(zoneId) + "=absent"; + } + // suffix is a zone path like "//zone/path" + auto it = manifest.find(std::string(suffix)); + if (it == manifest.end()) + return "manifest-entry:" + std::string(suffix) + "=absent"; + // Entries are { "id": "W-xxxxxx" }; fingerprint from the entry JSON + auto entryStr = it->dump(); + auto hash = hashString(HashAlgorithm::SHA256, entryStr); + return "manifest-entry:" + std::string(suffix) + "=" + hash.to_string(HashFormat::Base64, false); +} + static const std::optional * dependencyFingerprintCached(ref accessor, std::string_view path, DependencyFingerprintCache & cache) { @@ -1347,7 +1390,26 @@ dependencyFingerprintCached(ref accessor, std::string_view path, if (it != threadCache.fingerprints.end()) return &it->second; - auto [_, fp] = accessor->getFingerprint(CanonPath(path)); + std::optional fp; + constexpr std::string_view manifestPrefix = ".meta/manifest.json#"; + if (path.starts_with(manifestPrefix)) { + // Synthetic manifest path: fingerprint from manifest content, not the + // filesystem. This lets per-entry tracking avoid whole-file invalidation. + if (!threadCache.manifestChecked) { + threadCache.manifestChecked = true; + try { + auto content = accessor->readFile(CanonPath(".meta/manifest.json")); + threadCache.cachedManifest = nlohmann::json::parse(content); + } catch (...) { + threadCache.cachedManifest = std::nullopt; + } + } + if (threadCache.cachedManifest) + fp = manifestPathFingerprint(*threadCache.cachedManifest, path.substr(manifestPrefix.size())); + } else { + auto [_, rawFp] = accessor->getFingerprint(CanonPath(path)); + fp = std::move(rawFp); + } auto inserted = threadCache.fingerprints.emplace(std::string(path), std::move(fp)).first; return &inserted->second; } From 3442e26e5ae1923a3321d2afa6a540288be20905 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Tue, 29 Sep 2026 16:20:02 -0400 Subject: [PATCH 02/10] libexpr: size the base environment for every builtin The base environment and the `builtins` attribute set it mirrors were allocated with a fixed 140 slots each, written without a bounds check. Every global constant and primop takes one slot of each. With the new manifest and memo builtins, `nix __dump-language` (run by the manual build; it registers every builtin, including those gated behind experimental features) needs 141: ERROR: AddressSanitizer: heap-buffer-overflow WRITE of size 8 ... 0 bytes after 1128-byte region nix::EvalState::addConstant eval.cc nix::EvalState::createBaseEnv primops.cc An unsanitized build does not notice. Give both the same named capacity (`baseEnvCapacity`, now 160) and make registration throw a clear error instead of writing past the end. Test: tests/functional/tecnix/base-env.sh runs `nix __dump-language`. With the check in place and the old size of 140 it fails with "the base environment has only 140 slots", so the test catches this in an ordinary build too. --- src/libexpr/eval.cc | 17 +++++++++++++---- src/libexpr/include/nix/expr/eval.hh | 13 +++++++++++++ src/libexpr/primops.cc | 2 +- tests/functional/tecnix/base-env.sh | 13 +++++++++++++ tests/functional/tecnix/meson.build | 1 + 5 files changed, 41 insertions(+), 5 deletions(-) create mode 100755 tests/functional/tecnix/base-env.sh diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 33360d6f9..ebb20af4d 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -246,8 +246,6 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env) } } -static constexpr size_t BASE_ENV_SIZE = 140; - EvalMemory::EvalMemory() { assertGCInitialized(); @@ -335,10 +333,10 @@ EvalState::EvalState( , regexCache(makeRegexCache()) , tecnixData(std::make_unique()) #if NIX_USE_BOEHMGC - , baseEnvP(std::allocate_shared(traceable_allocator(), &mem.allocEnv(BASE_ENV_SIZE))) + , baseEnvP(std::allocate_shared(traceable_allocator(), &mem.allocEnv(baseEnvCapacity))) , baseEnv(**baseEnvP) #else - , baseEnv(mem.allocEnv(BASE_ENV_SIZE)) + , baseEnv(mem.allocEnv(baseEnvCapacity)) #endif , staticBaseEnv{std::make_shared(nullptr, nullptr)} , countCalls(getEnv("NIX_COUNT_CALLS").value_or("0") != "0") @@ -497,6 +495,15 @@ void EvalState::checkURI(const std::string & uri0) throw RestrictedPathError("access to URI '%s' is forbidden in restricted mode", uri0); } +void EvalState::checkBaseEnvHasRoom(std::string_view name) const +{ + if (baseEnvDispl >= baseEnvCapacity) + throw Error( + "cannot register the builtin '%s': the base environment has only %d slots; increase baseEnvCapacity in eval.hh", + name, + baseEnvCapacity); +} + Value * EvalState::addConstant(const std::string & name, Value & v, Constant info) { Value * v2 = allocValue(); @@ -526,6 +533,7 @@ void EvalState::addConstant(const std::string & name, Value * v, Constant info) } /* Install value the base environment. */ + checkBaseEnvHasRoom(name); staticBaseEnv->vars.emplace_back(symbols.create(name), baseEnvDispl); baseEnv.values[baseEnvDispl++] = v; const_cast(getBuiltins().attrs())->push_back(Attr(symbols.create(name2), v)); @@ -595,6 +603,7 @@ Value * EvalState::addPrimOp(PrimOp && primOp) if (primOp.internal) internalPrimOps.emplace(primOp.name, RootValue(v)); else { + checkBaseEnvHasRoom(primOp.name); staticBaseEnv->vars.emplace_back(envName, baseEnvDispl); baseEnv.values[baseEnvDispl++] = v; const_cast(getBuiltins().attrs())->push_back(Attr(symbols.create(primOp.name), v)); diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 68d907d78..4ccdb7cea 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -859,8 +859,21 @@ public: private: + /** + * Number of slots in the base environment, and the capacity of the + * `builtins` attribute set that mirrors it. Every global constant and + * every primop takes one of each (see `addConstant` and `addPrimOp`), so + * registering more builtins than this would write past the end of both. + * `nix __dump-language` registers all of them, including the ones gated + * behind experimental features; keep some headroom above what it needs. + */ + static constexpr size_t baseEnvCapacity = 160; + unsigned int baseEnvDispl = 0; + /** Throw instead of writing past the end of the base environment. */ + void checkBaseEnvHasRoom(std::string_view name) const; + void createBaseEnv(const EvalSettings & settings); Value * addConstant(const std::string & name, Value & v, Constant info); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 4f4a39ef3..364122a0c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -5367,7 +5367,7 @@ void EvalState::createBaseEnv(const EvalSettings & evalSettings) Value v; /* `builtins' must be first! */ - v.mkAttrs(buildBindings(140).finish()); + v.mkAttrs(buildBindings(baseEnvCapacity).finish()); addConstant( "builtins", v, diff --git a/tests/functional/tecnix/base-env.sh b/tests/functional/tecnix/base-env.sh new file mode 100755 index 000000000..fd12a148d --- /dev/null +++ b/tests/functional/tecnix/base-env.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +# Every builtin must fit the evaluator's fixed-size base environment. The manual +# build runs `nix __dump-language`, which registers all of them, including the +# ones gated behind experimental features. Too many builtins used to write past +# the end of the environment (a heap buffer overflow only a sanitizer noticed). + +source "$(dirname "${BASH_SOURCE[0]}")/common.sh" + +nix __dump-language > "$TEST_ROOT/language.json" +jq -e 'length > 100' "$TEST_ROOT/language.json" > /dev/null \ + || fail "__dump-language should describe every global builtin" + +echo "base environment holds every builtin" diff --git a/tests/functional/tecnix/meson.build b/tests/functional/tecnix/meson.build index 8501d81a1..9fa4ac6ef 100644 --- a/tests/functional/tecnix/meson.build +++ b/tests/functional/tecnix/meson.build @@ -2,6 +2,7 @@ suites += { 'name' : 'tecnix', 'deps' : [], 'tests' : [ + 'base-env.sh', 'builtins.sh', 'gc.sh', ], From 4e59ea25c7d6c8ae9be4eb56748340225654d776 Mon Sep 17 00:00:00 2001 From: burke Date: Fri, 25 Sep 2026 19:27:31 -0400 Subject: [PATCH 03/10] tecnix: add builtins.tectonixMemo, a memoized loader that shares tracked dependencies `builtins.tectonixMemo namespace key f` evaluates `f key` once per EvalState and returns the shared value. Under Tecnix source tracking, the one evaluation runs inside a TrackedSourceDepsScope so its accesses intern into a reusable label stored alongside the value; every later consumer records that label as a child of its own frame (and inherits the value's label through the value-copy hooks), so all consumers share the same tracked dependencies without re-evaluating `f key`. A thread-local in-progress guard turns a self-referential `f` (a zone loading itself transitively) into a catchable AssertionError instead of unbounded recursion through fresh thunks. Re-entrant misses for distinct keys (zone A loading zone B) evaluate outside any concurrent_flat_map bucket lock, so they never deadlock. Functional test: tests/functional/tectonix/memo.sh. --- src/libexpr/primops/tectonix.cc | 125 ++++++++++++++++++++++++++ src/libexpr/tecnix/eval-data.hh | 27 ++++++ tests/functional/tectonix/memo.sh | 64 +++++++++++++ tests/functional/tectonix/meson.build | 1 + 4 files changed, 217 insertions(+) create mode 100755 tests/functional/tectonix/memo.sh diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index 4bdf61bfb..dcbcf2c15 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -212,6 +212,131 @@ static RegisterPrimOp primop_tectonixManifestIdToPath({ .impl = prim_tectonixManifestIdToPath, }); +// Re-entrancy guard for `builtins.tectonixMemo`: keys whose `f key` evaluation +// is currently in progress on this thread. Tracked evaluation is +// single-threaded, so a same-key re-entry can only be a genuine cycle (zone A +// loading itself transitively); the guard turns it into a catchable error +// instead of unbounded recursion through fresh thunks each call. +static thread_local std::vector tectonixMemoInProgress; + +// ============================================================================ +// builtins.tectonixMemo namespace key f +// Evaluates `f key` once per EvalState and returns the shared value. While +// tracked, the one evaluation records its source accesses into a label stored +// alongside the value; every later consumer records that label as a child of +// its own frame (and inherits the value's label through the value-copy hooks), +// so all consumers share the same tracked dependencies without re-evaluating +// `f key`. +// ============================================================================ +static void prim_tectonixMemo(EvalState & state, const PosIdx pos, Value ** args, Value & v) +{ + auto ns = + state.forceStringNoCtx(*args[0], pos, "while evaluating the 'namespace' argument to builtins.tectonixMemo"); + auto keyStr = state.forceStringNoCtx(*args[1], pos, "while evaluating the 'key' argument to builtins.tectonixMemo"); + + auto cacheKey = std::string(ns) + '\0' + std::string(keyStr); + auto & memoCache = *state.tecnixEvalData().tecnixMemoCache; + auto * trackingCtx = currentTecnixThreadState.trackingContext; + + Value * stored = nullptr; + EvalSourceAccessSetId sourceDeps = emptyEvalSourceAccessSetId; + + if (trackingCtx) { + bool hit = false; + memoCache.cvisit(cacheKey, [&](auto & i) { + stored = *i.second.value; + sourceDeps = i.second.sourceDeps; + hit = true; + }); + if (hit) { + recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); + v = *stored; + return; + } + } else { + bool hit = false; + memoCache.cvisit(cacheKey, [&](auto & i) { + stored = *i.second.value; + hit = true; + }); + if (hit) { + v = *stored; + return; + } + } + + // Miss. Guard against same-key re-entry (a cycle through `f`). + for (const auto & inProgress : tectonixMemoInProgress) { + if (inProgress == cacheKey) + // AssertionError so builtins.tryEval can catch a circular zone + // dependency the same way it catches a missing zone, instead of + // crashing the whole evaluation. + state + .error( + "builtins.tectonixMemo: circular evaluation detected for namespace '%s' key '%s'", ns, keyStr) + .atPos(pos) + .debugThrow(); + } + + tectonixMemoInProgress.push_back(cacheKey); + Finally popInProgress([&]() { tectonixMemoInProgress.pop_back(); }); + + // Evaluate `f key` outside any map bucket lock so re-entrant misses (zone A + // loading zone B) can insert their own entries without deadlock. Under + // tracking, scope the evaluation so its accesses intern into one reusable + // label that every later consumer records. + if (trackingCtx) { + TrackedSourceDepsScope scope(*trackingCtx); + stored = state.allocValue(); + state.callFunction(*args[2], *args[1], *stored, pos); + state.forceValue(*stored, pos); + sourceDeps = scope.finish(stored); + } else { + stored = state.allocValue(); + state.callFunction(*args[2], *args[1], *stored, pos); + state.forceValue(*stored, pos); + } + + // Insert, but if another evaluation raced ahead (untracked parallel + // evaluation), reuse its entry so the table keeps a single shared value. + memoCache.try_emplace_and_cvisit( + cacheKey, + EvalTecnixMemoCacheEntry{}, + [&](auto & i) { + i.second.value = RootValue(stored); + i.second.sourceDeps = sourceDeps; + }, + [&](auto & i) { + stored = *i.second.value; + sourceDeps = i.second.sourceDeps; + }); + + if (trackingCtx) + recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); + v = *stored; +} + +static RegisterPrimOp primop_tectonixMemo({ + .name = "__tectonixMemo", + .args = {"namespace", "key", "f"}, + .doc = R"( + Evaluate `f key` once per evaluation and return the shared value. Later + calls with the same `namespace` and `key` return the cached value without + re-invoking `f`, and (under Tecnix source tracking) every consumer + inherits the cached value's recorded source dependencies, so the work and + the tracked closure are both shared. + + `namespace` and `key` must be strings; `f` is called as `f key`. A + self-referential `f` (one whose evaluation re-enters `tectonixMemo` with + the same namespace and key) throws a catchable circular-evaluation error. + + Example: + `builtins.tectonixMemo "zones" "//a/b" (path: loadZone path)` + evaluates `loadZone "//a/b"` once and shares the result. + )", + .impl = prim_tectonixMemo, +}); + static std::string normalizeTrackedRepoPath(std::string_view path); // ============================================================================ diff --git a/src/libexpr/tecnix/eval-data.hh b/src/libexpr/tecnix/eval-data.hh index d4140875f..227cdca11 100644 --- a/src/libexpr/tecnix/eval-data.hh +++ b/src/libexpr/tecnix/eval-data.hh @@ -47,6 +47,22 @@ struct EvalTecnixModuleCacheEntry using EvalTecnixModuleCache = boost::concurrent_flat_map; +/** + * A memoized `builtins.tectonixMemo` result: the shared finished value plus + * the source-access-set label recorded the one time `f key` was evaluated. + * Every later consumer records `sourceDeps` as a child of its own frame (and + * inherits the value's label through the normal value-copy hooks), so the + * shared value's tracked dependencies propagate to all consumers without + * re-evaluating `f key`. + */ +struct EvalTecnixMemoCacheEntry +{ + RootValue value; + EvalSourceAccessSetId sourceDeps = emptyEvalSourceAccessSetId; +}; + +using EvalTecnixMemoCache = boost::concurrent_flat_map; + struct EvalState::TecnixEvalData { /** @@ -138,6 +154,17 @@ struct EvalState::TecnixEvalData */ const ref tecnixModuleCache = make_ref(); + /** + * Memoization table for `builtins.tectonixMemo`, keyed by + * `namespace\0key`. Each entry holds the single shared result of `f key` + * and its recorded source-access-set label, so the (potentially + * expensive) `f key` evaluation runs once per EvalState and every + * consumer inherits the same tracked dependencies. Tracked evaluation + * never spawns parallel work, so re-entrant misses (zone A loading zone + * B) evaluate outside any bucket lock and never deadlock. + */ + const ref tecnixMemoCache = make_ref(); + /** Lazy-initialized set of zone IDs in sparse checkout (thread-safe via once_flag) */ mutable std::once_flag tectonixSparseCheckoutRootsFlag; mutable std::set tectonixSparseCheckoutRoots; diff --git a/tests/functional/tectonix/memo.sh b/tests/functional/tectonix/memo.sh new file mode 100755 index 000000000..ffae824a3 --- /dev/null +++ b/tests/functional/tectonix/memo.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# Test builtins.tectonixMemo: evaluate `f key` once per EvalState and share the +# result (and its tracked source deps) across consumers within one evaluation. + +# shellcheck disable=SC2016 +source "$(dirname "${BASH_SOURCE[0]}")/common.sh" + +TEST_WORLD="$TEST_ROOT/world" +create_test_world "$TEST_WORLD" +HEAD_SHA=$(get_head_sha "$TEST_WORLD") + +# 1. Correctness: tectonixMemo returns f key. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'builtins.tectonixMemo "ns" "k" (x: x + "!")') +[[ "$out" == "k!" ]] || fail "expected 'k!', got '$out'" +echo "correctness ok" + +# 2. Within-process memoization: force the first call (caching it), then a +# second call with the same (namespace,key) but a different f must return the +# cached value, proving the second f was never invoked. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns" "k" (x: "WRONG"); in builtins.seq a b') +[[ "$out" == "k!" ]] || fail "expected cached 'k!', got '$out'" +echo "memoization (same key, same eval) ok" + +# 3. Distinct keys evaluate independently within one eval. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns" "k2" (x: x + "!"); in "${a}|${b}"') +[[ "$out" == "k!|k2!" ]] || fail "expected 'k!|k2!', got '$out'" +echo "distinct key ok" + +# 4. Distinct namespaces are independent within one eval. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns2" "k" (x: x + "?"); in "${a}|${b}"') +[[ "$out" == "k!|k?" ]] || fail "expected 'k!|k?', got '$out'" +echo "distinct namespace ok" + +# 5. Sharing: two consumers of the same memo entry get equal values. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let f = x: x + "-shared"; a = builtins.tectonixMemo "share" "z" f; b = builtins.tectonixMemo "share" "z" f; in builtins.seq a (if a == b then a else "MISMATCH")') +[[ "$out" == "z-shared" ]] || fail "expected 'z-shared', got '$out'" +echo "sharing ok" + +# 6. The memoized value can be an attrset and is shared structurally. +out=$(tectonix_eval_json "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let f = x: { path = x; tag = "node"; }; a = builtins.tectonixMemo "attrs" "//a" f; b = builtins.tectonixMemo "attrs" "//a" f; in builtins.seq a { a = a; sameRef = a == b; }') +echo "$out" | grepQuiet '"sameRef":true' +echo "$out" | grepQuiet '"path":"//a"' +echo "attrset sharing ok" + +# 7. Circular self-reference throws a catchable error (not a crash). +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let f = k: builtins.tectonixMemo "cyc" k f; r = builtins.tryEval (builtins.tectonixMemo "cyc" "a" f); in if r.success then "NO-THROW" else "THREW"') +[[ "$out" == "THREW" ]] || fail "expected circular memo to throw, got '$out'" +echo "cycle detection ok" + +# 8. Re-entrant miss for a DIFFERENT key (a dependency) works: zone-like +# recursion through distinct keys resolves without a false cycle error. +out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ + 'let load = k: if k == "root" then builtins.tectonixMemo "dep" "child" load else k + "-val"; in builtins.tectonixMemo "dep" "root" load') +[[ "$out" == "child-val" ]] || fail "expected 'child-val', got '$out'" +echo "re-entrant distinct-key ok" + +echo "tectonixMemo test passed!" diff --git a/tests/functional/tectonix/meson.build b/tests/functional/tectonix/meson.build index 72fd3f982..76056c289 100644 --- a/tests/functional/tectonix/meson.build +++ b/tests/functional/tectonix/meson.build @@ -6,6 +6,7 @@ suites += { 'errors.sh', 'deduplication.sh', 'dirty-zones.sh', + 'memo.sh', ], 'workdir' : meson.current_source_dir(), } From aa7083be876529a577d3a6912ae7168f3b501017 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Tue, 29 Sep 2026 14:09:55 -0400 Subject: [PATCH 04/10] tests: cover per-entry manifest tracking and manifest fingerprints Functional test tests/functional/tecnix/manifest-tracking.sh: what each of tectonixManifestEntry, tectonixManifestKeys and tectonixMemo records, and how eval-cache rows behave across commits. Adding an unrelated zone invalidates only the target that enumerates the keys and one that reads the whole manifest; changing a zone's entry invalidates that zone's target; moving an id invalidates its lookup; changing a file read inside a memoized load invalidates every consumer; an unrelated file invalidates nothing. Unit tests src/libexpr-tests/tecnix-manifest-fingerprint.cc: the synthetic fingerprints follow only the part of the manifest they name, and follow the accessor across fingerprint caches on one thread. --- src/libexpr-tests/meson.build | 1 + .../tecnix-manifest-fingerprint.cc | 110 +++++++++++ tests/functional/tecnix/manifest-tracking.sh | 180 ++++++++++++++++++ tests/functional/tecnix/meson.build | 1 + 4 files changed, 292 insertions(+) create mode 100644 src/libexpr-tests/tecnix-manifest-fingerprint.cc create mode 100755 tests/functional/tecnix/manifest-tracking.sh diff --git a/src/libexpr-tests/meson.build b/src/libexpr-tests/meson.build index 289660047..b1fae20fd 100644 --- a/src/libexpr-tests/meson.build +++ b/src/libexpr-tests/meson.build @@ -68,6 +68,7 @@ sources = files( 'search-path.cc', 'tecnix-dependency-tracking.cc', 'tecnix-eval-cache-row.cc', + 'tecnix-manifest-fingerprint.cc', 'tecnix-worldtree.cc', 'tectonix.cc', 'trivial.cc', diff --git a/src/libexpr-tests/tecnix-manifest-fingerprint.cc b/src/libexpr-tests/tecnix-manifest-fingerprint.cc new file mode 100644 index 000000000..c7b6d999f --- /dev/null +++ b/src/libexpr-tests/tecnix-manifest-fingerprint.cc @@ -0,0 +1,110 @@ +#include +#include +#include + +#include + +#include "nix/expr/tecnix/eval-cache.hh" +#include "nix/util/memory-source-accessor.hh" + +namespace nix { + +namespace { + +/** A source tree whose only file is `.meta/manifest.json` with the given content. */ +ref treeWithManifest(std::string manifest) +{ + auto accessor = make_ref(); + accessor->addFile(CanonPath(".meta/manifest.json"), std::move(manifest)); + return accessor; +} + +/** Fingerprint one synthetic manifest path, with a fresh cache like a fresh evaluation. */ +std::optional fingerprint(ref accessor, std::string_view syntheticPath) +{ + DependencyFingerprintCache cache; + return dependencyFingerprint(accessor, syntheticPath, cache); +} + +constexpr auto twoZones = R"({ "//zones/a": { "id": "W-000001" }, "//zones/b": { "id": "W-000002" } })"; +constexpr auto threeZones = + R"({ "//zones/a": { "id": "W-000001" }, "//zones/b": { "id": "W-000002" }, "//zones/c": { "id": "W-000003" } })"; +constexpr auto bRenumbered = R"({ "//zones/a": { "id": "W-000001" }, "//zones/b": { "id": "W-000009" } })"; + +} // namespace + +TEST(TecnixManifestFingerprint, EntryFingerprintIgnoresOtherEntries) +{ + auto entryA = ".meta/manifest.json#//zones/a"; + auto before = fingerprint(treeWithManifest(twoZones), entryA); + ASSERT_TRUE(before); + EXPECT_EQ(fingerprint(treeWithManifest(threeZones), entryA), before); + EXPECT_EQ(fingerprint(treeWithManifest(bRenumbered), entryA), before); +} + +TEST(TecnixManifestFingerprint, EntryFingerprintFollowsItsOwnEntry) +{ + auto entryB = ".meta/manifest.json#//zones/b"; + EXPECT_NE(fingerprint(treeWithManifest(twoZones), entryB), fingerprint(treeWithManifest(bRenumbered), entryB)); + EXPECT_NE( + fingerprint(treeWithManifest(twoZones), ".meta/manifest.json#//zones/a"), + fingerprint(treeWithManifest(twoZones), entryB)); +} + +TEST(TecnixManifestFingerprint, AbsentEntryHasItsOwnFingerprint) +{ + auto entryC = ".meta/manifest.json#//zones/c"; + auto absent = fingerprint(treeWithManifest(twoZones), entryC); + ASSERT_TRUE(absent); + EXPECT_NE(fingerprint(treeWithManifest(threeZones), entryC), absent); +} + +TEST(TecnixManifestFingerprint, KeyFingerprintTracksTheKeySetOnly) +{ + auto keys = ".meta/manifest.json#keys"; + auto before = fingerprint(treeWithManifest(twoZones), keys); + ASSERT_TRUE(before); + EXPECT_NE(fingerprint(treeWithManifest(threeZones), keys), before); + // Renumbering a zone changes no key. + EXPECT_EQ(fingerprint(treeWithManifest(bRenumbered), keys), before); +} + +TEST(TecnixManifestFingerprint, KeyFingerprintIgnoresKeyOrder) +{ + auto reordered = R"({ "//zones/b": { "id": "W-000002" }, "//zones/a": { "id": "W-000001" } })"; + EXPECT_EQ( + fingerprint(treeWithManifest(twoZones), ".meta/manifest.json#keys"), + fingerprint(treeWithManifest(reordered), ".meta/manifest.json#keys")); +} + +TEST(TecnixManifestFingerprint, IdFingerprintTracksWhereTheIdPoints) +{ + auto idOfB = ".meta/manifest.json#id/W-000002"; + auto before = fingerprint(treeWithManifest(twoZones), idOfB); + ASSERT_TRUE(before); + // Adding an unrelated zone leaves the lookup alone; renumbering removes the id. + EXPECT_EQ(fingerprint(treeWithManifest(threeZones), idOfB), before); + EXPECT_NE(fingerprint(treeWithManifest(bRenumbered), idOfB), before); +} + +TEST(TecnixManifestFingerprint, FollowsTheAccessorAcrossCaches) +{ + // The parsed manifest must not outlive the cache it was read for: another + // evaluation on the same thread fingerprints against its own commit. + auto entryB = ".meta/manifest.json#//zones/b"; + auto first = fingerprint(treeWithManifest(twoZones), entryB); + auto second = fingerprint(treeWithManifest(bRenumbered), entryB); + EXPECT_NE(first, second); + EXPECT_NE( + fingerprint(treeWithManifest(twoZones), ".meta/manifest.json#keys"), + fingerprint(treeWithManifest(threeZones), ".meta/manifest.json#keys")); +} + +TEST(TecnixManifestFingerprint, NoManifestMeansNoFingerprint) +{ + EXPECT_EQ(fingerprint(make_ref(), ".meta/manifest.json#keys"), std::nullopt); + // ...and that must not leak into the next tree either. + EXPECT_TRUE(fingerprint(treeWithManifest(twoZones), ".meta/manifest.json#keys")); +} + +} // namespace nix diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh new file mode 100755 index 000000000..d44c92a90 --- /dev/null +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# Per-entry manifest tracking and the memoized zone loader. +# +# A target that reads one zone's manifest entry (`tectonixManifestEntry`), the +# key set (`tectonixManifestKeys`) or one id (`tectonixManifestIdToPath`) +# depends on exactly that, not on the whole `.meta/manifest.json`. So adding an +# unrelated zone leaves such a target's eval-cache row valid. `tectonixMemo` +# shares one evaluation, and the source dependencies recorded during it, among +# every consumer. + +source "$(dirname "${BASH_SOURCE[0]}")/common.sh" + +assert_jq() { + local json="$1" + local filter="$2" + local message="$3" + if ! jq -e "$filter" >/dev/null <<< "$json"; then + echo "$json" >&2 + fail "$message" + fi +} + +WORLD="$TEST_ROOT/manifest-world" +createGitRepo "$WORLD" + +write_manifest() { + # $1: extra manifest lines, already comma-terminated + cat > "$WORLD/.meta/manifest.json" < "$WORLD/shared.txt" +echo "unrelated one" > "$WORLD/unrelated.txt" + +cat > "$WORLD/resolve.nix" <<'EOF' +args: +let + drv = name: suffix: { + drvPath = "/nix/store/00000000000000000000000000000000-${name}-${suffix}.drv"; + outputName = "out"; + }; + entry = name: builtins.tectonixManifestEntry "//zones/${name}"; + # Two consumers of one memoized load: the load reads shared.txt once. + loaded = builtins.tectonixMemo "loader" "shared" (_: builtins.readFile ./shared.txt); +in +{ + allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" ]; + resolve = name: + if name == "whole" then drv name (toString (builtins.length (builtins.attrNames builtins.unsafeTectonixInternalManifest))) + else if name == "keys" then drv name (toString (builtins.length (builtins.tectonixManifestKeys))) + else if name == "id" then drv name ( + let path = builtins.tectonixManifestIdToPath "W-000001"; in if path == null then "absent" else path + ) + else if name == "m1" || name == "m2" then drv name (builtins.hashString "sha256" loaded) + else drv name (entry name).id; +} +EOF + +commit_world() { + git -C "$WORLD" add -A + git -C "$WORLD" commit -q -m "$1" + git -C "$WORLD" rev-parse HEAD +} + +REV1=$(commit_world "two zones") + +TARGETS='[ "a" "b" "keys" "id" "m1" "m2" "whole" ]' + +# Dependency sets (target -> path -> fingerprint) of every target at $1. +# Extra nix flags follow. +deps_at() { + local rev="$1" + shift + nix eval --json -v \ + --extra-experimental-features 'nix-command' \ + --option lazy-trees true \ + --option tectonix-git-dir "$WORLD/.git" \ + --option tectonix-git-sha "$rev" \ + "$@" \ + --expr "builtins.listToAttrs (map (r: { name = r.target; value = r.dependencies; }) (builtins.tecnixTargets { + gitDir = \"$WORLD/.git\"; resolver = \"resolve.nix\"; args = { system = \"test-system\"; }; + rev = \"$rev\"; targets = $TARGETS; includeDependencies = true; includeTargets = false; }))" +} + +nocache_deps_at() { + deps_at "$1" --option tecnix-eval-cache false +} + +CACHE_HOME="$TEST_ROOT/manifest-cache-home" +cached_deps_at() { + XDG_CACHE_HOME="$CACHE_HOME" deps_at "$1" --option tecnix-eval-cache true --pure-eval +} + +# `hits ` asserts each target was served from the cache; +# `misses` asserts each was re-evaluated. +hits() { + local err="$1"; shift + for t in "$@"; do + grepQuiet "dependency cache hit for '$t'" "$err" || fail "$t should be a cache hit ($err)" + done +} +misses() { + local err="$1"; shift + for t in "$@"; do + grepQuiet "dependency cache miss, evaluating '$t'" "$err" || fail "$t should be re-evaluated ($err)" + done +} + +echo "Testing what each target depends on..." +deps1=$(nocache_deps_at "$REV1") +assert_jq "$deps1" '.a | keys == [".meta/manifest.json#//zones/a", "resolve.nix"]' \ + "a should depend on its own manifest entry, not the whole manifest" +assert_jq "$deps1" '.b | keys == [".meta/manifest.json#//zones/b", "resolve.nix"]' \ + "b should depend on its own manifest entry, not the whole manifest" +assert_jq "$deps1" '.keys | keys == [".meta/manifest.json#keys", "resolve.nix"]' \ + "keys should depend on the key set only" +assert_jq "$deps1" '.id | keys == [".meta/manifest.json#id/W-000001", "resolve.nix"]' \ + "id should depend on one id lookup only" +assert_jq "$deps1" '[.a, .b, .keys, .id, .m1, .m2 | has(".meta/manifest.json")] | any | not' \ + "the per-entry targets should not depend on the whole manifest file" +assert_jq "$deps1" '.whole | has(".meta/manifest.json")' \ + "reading the whole manifest (builtins.unsafeTectonixInternalManifest) should still depend on the file" +assert_jq "$deps1" '.a[".meta/manifest.json#//zones/a"] != .b[".meta/manifest.json#//zones/b"]' \ + "entry fingerprints should differ between zones" +assert_jq "$deps1" '.m1 == .m2 and (.m1 | has("shared.txt"))' \ + "both consumers of a memoized load should inherit the load's dependencies" + +echo "Testing the eval cache across commits..." +cached_deps_at "$REV1" > "$TEST_ROOT/cold.json" 2> "$TEST_ROOT/cold.err" +misses "$TEST_ROOT/cold.err" a b keys id m1 m2 whole +[[ "$(jq -S . < "$TEST_ROOT/cold.json")" == "$(jq -S . <<< "$deps1")" ]] \ + || fail "the cached evaluation should report the same dependencies" + +# Adding an unrelated zone changes the manifest file and the key set. Only the +# targets that enumerate the keys or read the whole file are invalidated. +write_manifest ' "//zones/c": { "id": "W-000003" },' +REV2=$(commit_world "add an unrelated zone") +cached_deps_at "$REV2" > /dev/null 2> "$TEST_ROOT/add-zone.err" +hits "$TEST_ROOT/add-zone.err" a b id m1 m2 +misses "$TEST_ROOT/add-zone.err" keys whole + +# Changing one zone's entry invalidates that zone's target and nothing else. +sed -i.bak 's/W-000002/W-000009/' "$WORLD/.meta/manifest.json" && rm "$WORLD/.meta/manifest.json.bak" +REV3=$(commit_world "change b's id") +cached_deps_at "$REV3" > "$TEST_ROOT/change-b.json" 2> "$TEST_ROOT/change-b.err" +hits "$TEST_ROOT/change-b.err" a id keys m1 m2 +misses "$TEST_ROOT/change-b.err" b whole +assert_jq "$(cat "$TEST_ROOT/change-b.json")" \ + ".b[\".meta/manifest.json#//zones/b\"] != $(jq '.b[".meta/manifest.json#//zones/b"]' <<< "$deps1")" \ + "b's recorded entry fingerprint should follow its entry" + +# An id lookup is invalidated when the id moves, and only then. +sed -i.bak 's/W-000001/W-000010/' "$WORLD/.meta/manifest.json" && rm "$WORLD/.meta/manifest.json.bak" +REV4=$(commit_world "renumber a") +cached_deps_at "$REV4" > /dev/null 2> "$TEST_ROOT/renumber-a.err" +hits "$TEST_ROOT/renumber-a.err" b keys m1 m2 +misses "$TEST_ROOT/renumber-a.err" a id whole + +# A memoized load's dependencies reach every consumer: changing what the load +# read invalidates both, not just whichever consumer evaluated it first. +echo "shared two" > "$WORLD/shared.txt" +REV5=$(commit_world "change the shared file") +cached_deps_at "$REV5" > /dev/null 2> "$TEST_ROOT/shared.err" +hits "$TEST_ROOT/shared.err" a b id keys whole +misses "$TEST_ROOT/shared.err" m1 m2 + +# A file nothing read leaves every row valid. +echo "unrelated two" > "$WORLD/unrelated.txt" +REV6=$(commit_world "change an unrelated file") +cached_deps_at "$REV6" > /dev/null 2> "$TEST_ROOT/unrelated.err" +hits "$TEST_ROOT/unrelated.err" a b keys id m1 m2 whole + +echo "All manifest tracking tests passed!" diff --git a/tests/functional/tecnix/meson.build b/tests/functional/tecnix/meson.build index 9fa4ac6ef..8fa603d25 100644 --- a/tests/functional/tecnix/meson.build +++ b/tests/functional/tecnix/meson.build @@ -5,6 +5,7 @@ suites += { 'base-env.sh', 'builtins.sh', 'gc.sh', + 'manifest-tracking.sh', ], 'workdir' : meson.current_source_dir(), } From 9d519ddb39853828d24117a18c600df55f320683 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Wed, 30 Sep 2026 18:13:54 -0400 Subject: [PATCH 05/10] libexpr: never share tectonixMemo results between tracked and untracked calls A result computed outside Tecnix source tracking was stored with no source dependencies, and a later tracked call reused it and recorded nothing, so the target's eval-cache row lacked the manifest entry the result came from: renumbering the zone left a stale warm hit. The reverse direction lost dependencies too: an untracked call could force the lazy parts of a tracked result, leaving them unlabelled for every later tracked consumer. Tracked and untracked calls now use separate tables, as the file-eval and import-resolution caches already do. Untracked results are still memoized, in their own table. The resolver-module cache's other rule (do not cache an untracked build while tracking is on) does not carry over: `nix eval` runs pure with the eval cache on by default, so that condition would turn memoization off for every untracked call, and a recursive loader (zone A loads zone B) would re-walk everything it shares on each one. The new manifest-tracking cases fail before this change (both targets lack .meta/manifest.json#//zones/a, and after renumbering zone a the warm run is a cache hit) and pass after it. They also pin that a tracked memo hit replays the dependencies recorded when its entry was filled, which already held. --- src/libexpr/primops/tectonix.cc | 52 +++++++------ src/libexpr/tecnix/eval-data.hh | 15 ++-- tests/functional/tecnix/manifest-tracking.sh | 80 +++++++++++++++++--- 3 files changed, 107 insertions(+), 40 deletions(-) diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index 43aed66e4..7fbb2cd9e 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -235,34 +235,34 @@ static void prim_tectonixMemo(EvalState & state, const PosIdx pos, Value ** args auto keyStr = state.forceStringNoCtx(*args[1], pos, "while evaluating the 'key' argument to builtins.tectonixMemo"); auto cacheKey = std::string(ns) + '\0' + std::string(keyStr); - auto & memoCache = *state.tecnixEvalData().tecnixMemoCache; auto * trackingCtx = currentTecnixThreadState.trackingContext; + /* A result computed outside tracking carries no label, and the lazy parts + of a tracked result must only ever be forced under tracking, so tracked + and untracked calls never share a table: a tracked call reusing an + untracked result would record none of its sources, and an untracked call + forcing part of a tracked result would leave that part unlabelled for + every later tracked consumer. Untracked results are still memoized, in + their own table: `f` is typically recursive (zone A loads zone B), so + building them afresh would re-walk everything they share. */ + auto & memoCache = + trackingCtx ? *state.tecnixEvalData().trackedTecnixMemoCache : *state.tecnixEvalData().tecnixMemoCache; + Value * stored = nullptr; EvalSourceAccessSetId sourceDeps = emptyEvalSourceAccessSetId; - if (trackingCtx) { - bool hit = false; - memoCache.cvisit(cacheKey, [&](auto & i) { - stored = *i.second.value; - sourceDeps = i.second.sourceDeps; - hit = true; - }); - if (hit) { + bool hit = false; + memoCache.cvisit(cacheKey, [&](auto & i) { + stored = *i.second.value; + sourceDeps = i.second.sourceDeps; + hit = true; + }); + if (hit) { + // Replay into this consumer the sources the entry's evaluation read. + if (trackingCtx) recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); - v = *stored; - return; - } - } else { - bool hit = false; - memoCache.cvisit(cacheKey, [&](auto & i) { - stored = *i.second.value; - hit = true; - }); - if (hit) { - v = *stored; - return; - } + v = *stored; + return; } // Miss. Guard against same-key re-entry (a cycle through `f`). @@ -297,8 +297,8 @@ static void prim_tectonixMemo(EvalState & state, const PosIdx pos, Value ** args state.forceValue(*stored, pos); } - // Insert, but if another evaluation raced ahead (untracked parallel - // evaluation), reuse its entry so the table keeps a single shared value. + // Insert, but if another evaluation raced ahead (parallel evaluation), + // reuse its entry so the table keeps a single shared value. memoCache.try_emplace_and_cvisit( cacheKey, EvalTecnixMemoCacheEntry{}, @@ -330,6 +330,10 @@ static RegisterPrimOp primop_tectonixMemo({ self-referential `f` (one whose evaluation re-enters `tectonixMemo` with the same namespace and key) throws a catchable circular-evaluation error. + Results computed outside Tecnix source tracking and results computed + under it are kept apart: neither is ever returned to the other kind of + call. + Example: `builtins.tectonixMemo "zones" "//a/b" (path: loadZone path)` evaluates `loadZone "//a/b"` once and shares the result. diff --git a/src/libexpr/tecnix/eval-data.hh b/src/libexpr/tecnix/eval-data.hh index 227cdca11..5d966fdc4 100644 --- a/src/libexpr/tecnix/eval-data.hh +++ b/src/libexpr/tecnix/eval-data.hh @@ -155,15 +155,18 @@ struct EvalState::TecnixEvalData const ref tecnixModuleCache = make_ref(); /** - * Memoization table for `builtins.tectonixMemo`, keyed by + * Memoization tables for `builtins.tectonixMemo`, keyed by * `namespace\0key`. Each entry holds the single shared result of `f key` - * and its recorded source-access-set label, so the (potentially - * expensive) `f key` evaluation runs once per EvalState and every - * consumer inherits the same tracked dependencies. Tracked evaluation - * never spawns parallel work, so re-entrant misses (zone A loading zone - * B) evaluate outside any bucket lock and never deadlock. + * and, in the tracked table, its recorded source-access-set label, so the + * (potentially expensive) `f key` evaluation runs once per EvalState and + * every tracked consumer inherits the same tracked dependencies. Tracked + * and untracked calls use separate tables, like `trackedFileEvalCache` + * and `fileEvalCache`, so a result never crosses between them. Misses + * evaluate outside any bucket lock, so re-entrant misses (zone A loading + * zone B) never deadlock. */ const ref tecnixMemoCache = make_ref(); + const ref trackedTecnixMemoCache = make_ref(); /** Lazy-initialized set of zone IDs in sparse checkout (thread-safe via once_flag) */ mutable std::once_flag tectonixSparseCheckoutRootsFlag; diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh index d44c92a90..7ae394cf9 100755 --- a/tests/functional/tecnix/manifest-tracking.sh +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -6,7 +6,8 @@ # depends on exactly that, not on the whole `.meta/manifest.json`. So adding an # unrelated zone leaves such a target's eval-cache row valid. `tectonixMemo` # shares one evaluation, and the source dependencies recorded during it, among -# every consumer. +# every consumer, but never shares a result computed outside tracking with a +# tracked target (or the other way round). source "$(dirname "${BASH_SOURCE[0]}")/common.sh" @@ -49,9 +50,13 @@ let entry = name: builtins.tectonixManifestEntry "//zones/${name}"; # Two consumers of one memoized load: the load reads shared.txt once. loaded = builtins.tectonixMemo "loader" "shared" (_: builtins.readFile ./shared.txt); + # Memoized lookups that every consumer calls afresh, so every consumer after + # the first is a memo hit rather than a force of one shared thunk. + zoneId = name: builtins.tectonixMemo "zone-id" name (n: (entry n).id); + zoneRecord = name: builtins.tectonixMemo "zone-record" name (n: { id = (entry n).id; }); in { - allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" ]; + allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" ]; resolve = name: if name == "whole" then drv name (toString (builtins.length (builtins.attrNames builtins.unsafeTectonixInternalManifest))) else if name == "keys" then drv name (toString (builtins.length (builtins.tectonixManifestKeys))) @@ -59,6 +64,12 @@ in let path = builtins.tectonixManifestIdToPath "W-000001"; in if path == null then "absent" else path ) else if name == "m1" || name == "m2" then drv name (builtins.hashString "sha256" loaded) + else if name == "za1" || name == "za2" || name == "prefilled" then drv name (zoneId "a") + # Hands both memoized lookups, unforced, to whoever reads the value. + else if name == "peek" then drv name "peek" // { zoneId = zoneId "a"; recordId = (zoneRecord "a").id; } + # Fills the memoized record without forcing its field. + else if name == "fill-record" then builtins.seq (zoneRecord "a") (drv name "record") + else if name == "record-field" then drv name (zoneRecord "a").id else drv name (entry name).id; } EOF @@ -71,7 +82,7 @@ commit_world() { REV1=$(commit_world "two zones") -TARGETS='[ "a" "b" "keys" "id" "m1" "m2" "whole" ]' +TARGETS='[ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" ]' # Dependency sets (target -> path -> fingerprint) of every target at $1. # Extra nix flags follow. @@ -123,7 +134,7 @@ assert_jq "$deps1" '.keys | keys == [".meta/manifest.json#keys", "resolve.nix"]' "keys should depend on the key set only" assert_jq "$deps1" '.id | keys == [".meta/manifest.json#id/W-000001", "resolve.nix"]' \ "id should depend on one id lookup only" -assert_jq "$deps1" '[.a, .b, .keys, .id, .m1, .m2 | has(".meta/manifest.json")] | any | not' \ +assert_jq "$deps1" '[.a, .b, .keys, .id, .m1, .m2, .za1, .za2 | has(".meta/manifest.json")] | any | not' \ "the per-entry targets should not depend on the whole manifest file" assert_jq "$deps1" '.whole | has(".meta/manifest.json")' \ "reading the whole manifest (builtins.unsafeTectonixInternalManifest) should still depend on the file" @@ -131,10 +142,12 @@ assert_jq "$deps1" '.a[".meta/manifest.json#//zones/a"] != .b[".meta/manifest.js "entry fingerprints should differ between zones" assert_jq "$deps1" '.m1 == .m2 and (.m1 | has("shared.txt"))' \ "both consumers of a memoized load should inherit the load's dependencies" +assert_jq "$deps1" '[.za1, .za2 | keys == [".meta/manifest.json#//zones/a", "resolve.nix"]] | all' \ + "a memo hit should replay the dependencies recorded when its entry was filled" echo "Testing the eval cache across commits..." cached_deps_at "$REV1" > "$TEST_ROOT/cold.json" 2> "$TEST_ROOT/cold.err" -misses "$TEST_ROOT/cold.err" a b keys id m1 m2 whole +misses "$TEST_ROOT/cold.err" a b keys id m1 m2 whole za1 za2 [[ "$(jq -S . < "$TEST_ROOT/cold.json")" == "$(jq -S . <<< "$deps1")" ]] \ || fail "the cached evaluation should report the same dependencies" @@ -143,14 +156,14 @@ misses "$TEST_ROOT/cold.err" a b keys id m1 m2 whole write_manifest ' "//zones/c": { "id": "W-000003" },' REV2=$(commit_world "add an unrelated zone") cached_deps_at "$REV2" > /dev/null 2> "$TEST_ROOT/add-zone.err" -hits "$TEST_ROOT/add-zone.err" a b id m1 m2 +hits "$TEST_ROOT/add-zone.err" a b id m1 m2 za1 za2 misses "$TEST_ROOT/add-zone.err" keys whole # Changing one zone's entry invalidates that zone's target and nothing else. sed -i.bak 's/W-000002/W-000009/' "$WORLD/.meta/manifest.json" && rm "$WORLD/.meta/manifest.json.bak" REV3=$(commit_world "change b's id") cached_deps_at "$REV3" > "$TEST_ROOT/change-b.json" 2> "$TEST_ROOT/change-b.err" -hits "$TEST_ROOT/change-b.err" a id keys m1 m2 +hits "$TEST_ROOT/change-b.err" a id keys m1 m2 za1 za2 misses "$TEST_ROOT/change-b.err" b whole assert_jq "$(cat "$TEST_ROOT/change-b.json")" \ ".b[\".meta/manifest.json#//zones/b\"] != $(jq '.b[".meta/manifest.json#//zones/b"]' <<< "$deps1")" \ @@ -161,20 +174,67 @@ sed -i.bak 's/W-000001/W-000010/' "$WORLD/.meta/manifest.json" && rm "$WORLD/.me REV4=$(commit_world "renumber a") cached_deps_at "$REV4" > /dev/null 2> "$TEST_ROOT/renumber-a.err" hits "$TEST_ROOT/renumber-a.err" b keys m1 m2 -misses "$TEST_ROOT/renumber-a.err" a id whole +misses "$TEST_ROOT/renumber-a.err" a id whole za1 za2 # A memoized load's dependencies reach every consumer: changing what the load # read invalidates both, not just whichever consumer evaluated it first. echo "shared two" > "$WORLD/shared.txt" REV5=$(commit_world "change the shared file") cached_deps_at "$REV5" > /dev/null 2> "$TEST_ROOT/shared.err" -hits "$TEST_ROOT/shared.err" a b id keys whole +hits "$TEST_ROOT/shared.err" a b id keys whole za1 za2 misses "$TEST_ROOT/shared.err" m1 m2 # A file nothing read leaves every row valid. echo "unrelated two" > "$WORLD/unrelated.txt" REV6=$(commit_world "change an unrelated file") cached_deps_at "$REV6" > /dev/null 2> "$TEST_ROOT/unrelated.err" -hits "$TEST_ROOT/unrelated.err" a b keys id m1 m2 whole +hits "$TEST_ROOT/unrelated.err" a b keys id m1 m2 whole za1 za2 + +# A memoized result computed outside tracking must never reach a tracked +# target without its dependencies, in either direction: an untracked call that +# comes first, and an untracked call that forces part of an entry a tracked +# target filled. `peek` hands both lookups, unforced, to the top-level +# expression, which forces them outside tracking before the tracked targets +# run in the same evaluation. Extra nix flags follow the rev. +prefill_deps_at() { + local rev="$1" + shift + nix eval --json -v \ + --extra-experimental-features 'nix-command' \ + --option lazy-trees true \ + --option tectonix-git-dir "$WORLD/.git" \ + --option tectonix-git-sha "$rev" \ + "$@" \ + --expr "let + call = includeTargets: targets: builtins.tecnixTargets { + gitDir = \"$WORLD/.git\"; resolver = \"resolve.nix\"; args = { system = \"test-system\"; }; + rev = \"$rev\"; inherit targets includeTargets; includeDependencies = true; }; + peek = (builtins.head (call true [ \"peek\" ])).value; + outsideTracking = builtins.seq peek.zoneId (builtins.seq (call false [ \"fill-record\" ]) peek.recordId); + in builtins.seq outsideTracking (builtins.listToAttrs (map (r: { name = r.target; value = r.dependencies; }) + (call false [ \"prefilled\" \"record-field\" ])))" +} + +echo "Testing memoized results computed outside tracking..." +PREFILLED_DEPS='[.prefilled, ."record-field" | keys == [".meta/manifest.json#//zones/a", "resolve.nix"]] | all' +# First with tracking from includeDependencies alone, then through the eval +# cache, where a missing dependency turns into a stale warm hit. +assert_jq "$(prefill_deps_at "$REV6" --option tecnix-eval-cache false)" "$PREFILLED_DEPS" \ + "tracked targets should depend on the entry behind memoized values first touched outside tracking" + +PREFILL_CACHE_HOME="$TEST_ROOT/manifest-prefill-cache-home" +XDG_CACHE_HOME="$PREFILL_CACHE_HOME" prefill_deps_at "$REV6" --option tecnix-eval-cache true --pure-eval \ + > "$TEST_ROOT/prefill-cold.json" 2> "$TEST_ROOT/prefill-cold.err" +misses "$TEST_ROOT/prefill-cold.err" prefilled record-field +assert_jq "$(cat "$TEST_ROOT/prefill-cold.json")" "$PREFILLED_DEPS" \ + "cached tracked targets should depend on the entry behind memoized values first touched outside tracking" + +# The reviewer's reproduction: renumber zone a, and the warm run must +# re-evaluate instead of serving the old id. +sed -i.bak 's/W-000010/W-000011/' "$WORLD/.meta/manifest.json" && rm "$WORLD/.meta/manifest.json.bak" +REV7=$(commit_world "renumber a again") +XDG_CACHE_HOME="$PREFILL_CACHE_HOME" prefill_deps_at "$REV7" --option tecnix-eval-cache true --pure-eval \ + > /dev/null 2> "$TEST_ROOT/prefill-warm.err" +misses "$TEST_ROOT/prefill-warm.err" prefilled record-field echo "All manifest tracking tests passed!" From 1931407688d0ce705463cbab8e20bd754dc5deae Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Wed, 30 Sep 2026 18:23:23 -0400 Subject: [PATCH 06/10] libexpr: replace tectonixMemo with builtins.tecnixMemoize `builtins.tectonixMemo namespace key f` shared results through a global string namespace, so the key had to determine the result on its own: callers that used the same namespace and key with different functions (one loader per system, say) got each other's values. `builtins.tecnixMemoize f` instead returns `g`, a memoized `f`: `g k` is always `f k`, keyed by `f` itself (the address of its value, which each entry roots) and the string `k`. It lives in primops/tecnix.cc with the other Tecnix builtins; `tectonixMemo` is removed, as it had no released user. Tracked and untracked calls keep the separate tables of the previous commit, lookups borrow the key instead of copying it, and the primop documentation states what `k` may be, how long results live and how tracking interacts. The memo test moves to tests/functional/tecnix/memoize.sh and counts `f`'s evaluations with builtins.trace, instead of relying on a second `f` being ignored; manifest-tracking.sh uses the new builtin. --- plans/tecnix-target-eval-caching/explainer.md | 2 +- src/libexpr/primops/tecnix.cc | 141 ++++++++++++++++++ src/libexpr/primops/tectonix.cc | 129 ---------------- src/libexpr/tecnix/eval-data.hh | 92 +++++++++--- tests/functional/tecnix/manifest-tracking.sh | 15 +- tests/functional/tecnix/memoize.sh | 50 +++++++ tests/functional/tecnix/meson.build | 1 + tests/functional/tectonix/memo.sh | 64 -------- tests/functional/tectonix/meson.build | 1 - 9 files changed, 274 insertions(+), 221 deletions(-) create mode 100755 tests/functional/tecnix/memoize.sh delete mode 100755 tests/functional/tectonix/memo.sh diff --git a/plans/tecnix-target-eval-caching/explainer.md b/plans/tecnix-target-eval-caching/explainer.md index 1f37036f4..937a05566 100644 --- a/plans/tecnix-target-eval-caching/explainer.md +++ b/plans/tecnix-target-eval-caching/explainer.md @@ -327,7 +327,7 @@ Clean paths are served from git's object store — no checkout is required, and In a worldtree sandbox (`tectonix-worldtree-socket` set) there is no git repository to read; the clean tree is instead the daemon's immutable FUSE projection of the pinned commit, with repo-relative paths mapped through the committed manifest to per-zone views. The fingerprint vocabulary is unchanged: directories read their exact committed tree oid from the projection's `user.worldtree.tree-oid` xattr, and regular files read the daemon's `user.worldtree.blob-oid` xattr when it is served — one O(1) metadata read each. Symlinks (which cannot carry user xattrs; `getxattr` would follow the link and answer for its target, a different git object) and files under daemons that do not serve blob oids fall back to hashing their bytes as git blobs — a blob oid is a pure function of content — memoized in memory for the accessor's lifetime, which the immutable projection makes sound: each unique file is hashed at most once per evaluation. Because every mechanism emits identical fingerprint strings, closures produced under one backend validate under the other. Two caveats follow from the projection being zone-granular: committed paths outside every visible zone do not exist in this view and observe as `absent`, and zone-ancestor directories are synthesized with a composite `worldtree-union:` fingerprint outside the git vocabulary (their listing genuinely differs from the full git tree, so cross-backend cache misses there are correct, not conservative). -The evaluator's own caches require care, since any of them could silently absorb a read. Each — the file-evaluation cache, the import-resolution cache, the source-to-store copy cache — either maintains a separate tracked-domain instance or replays its provenance on a hit. The general rule: *any cache capable of skipping a physical read must replay the provenance of that read.* +The evaluator's own caches require care, since any of them could silently absorb a read. Each — the file-evaluation cache, the import-resolution cache, the source-to-store copy cache, the `builtins.tecnixMemoize` tables — maintains a separate tracked-domain instance, replays its provenance on a hit, or both. The general rule: *any cache capable of skipping a physical read must replay the provenance of that read.* The failure policy throughout is to fail closed. If `git status` fails, evaluation raises an error rather than assuming a clean tree, because the dirty overlay is load-bearing for closure validity. If a path cannot be fingerprinted, evaluation raises an error rather than emitting a partial closure. If evaluation reaches something the closure format cannot represent, it raises an error rather than under-tracking. Every failure mode resolves to a cache miss or a visible error; none resolves to a plausible wrong answer. diff --git a/src/libexpr/primops/tecnix.cc b/src/libexpr/primops/tecnix.cc index 06f8035fa..48f18031b 100644 --- a/src/libexpr/primops/tecnix.cc +++ b/src/libexpr/primops/tecnix.cc @@ -16,6 +16,7 @@ #include "nix/expr/tecnix/eval-cache.hh" #include "nix/expr/tecnix/source-accessors.hh" #include "nix/store/store-api.hh" +#include "nix/util/finally.hh" #include "nix/util/strings.hh" #include "nix/util/util.hh" @@ -121,6 +122,146 @@ static RegisterPrimOp primop_tecnixInternalSourceDepsList({ .impl = prim_tecnixInternalSourceDepsList, }); +// ============================================================================ +// builtins.tecnixMemoize f k +// `builtins.tecnixMemoize f` is `f`, memoized: calling it with a string `k` +// returns `f k`, evaluated at most once per EvalState for that `f` and `k` +// (once under tracking, once without). Under tracking, that evaluation +// records its source accesses into a label stored alongside the value; every +// later tracked consumer records that label as a child of its own frame (and +// inherits the value's label through the value-copy hooks), so all consumers +// share the same tracked dependencies without re-evaluating `f k`. +// ============================================================================ + +/* Calls whose `f k` evaluation is in progress on this thread. Re-entering one + (zone A loading itself transitively) can only be a genuine cycle; the guard + turns it into a catchable error instead of unbounded recursion. The key + views borrow from the calls' own arguments, which outlive their entries. */ +static thread_local std::vector tecnixMemoizeInProgress; + +static void prim_tecnixMemoize(EvalState & state, const PosIdx pos, Value ** args, Value & v) +{ + auto * function = args[0]; + auto key = state.forceStringNoCtx( + *args[1], pos, "while evaluating the key passed to a function memoized by builtins.tecnixMemoize"); + EvalTecnixMemoizeKey::Lookup lookup{function, key}; + + auto * trackingCtx = currentTecnixThreadState.trackingContext; + + /* A result computed outside tracking carries no label, and the lazy parts + of a tracked result must only ever be forced under tracking, so tracked + and untracked calls never share a table: a tracked call reusing an + untracked result would record none of its sources, and an untracked call + forcing part of a tracked result would leave that part unlabelled for + every later tracked consumer. Untracked results are still memoized, in + their own table: `f` is typically recursive (zone A loads zone B), so + building them afresh would re-walk everything they share. */ + auto & memoCache = + trackingCtx ? *state.tecnixEvalData().trackedTecnixMemoizeCache : *state.tecnixEvalData().tecnixMemoizeCache; + + Value * result = nullptr; + auto sourceDeps = emptyEvalSourceAccessSetId; + if (memoCache.cvisit(lookup, [&](const auto & entry) { + result = *entry.second.value; + sourceDeps = entry.second.sourceDeps; + })) { + // Replay into this consumer the sources the entry's evaluation read. + if (trackingCtx) + recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); + v = *result; + return; + } + + for (auto & inProgress : tecnixMemoizeInProgress) + if (EvalTecnixMemoizeKey::Equal{}(inProgress, lookup)) + // AssertionError, so builtins.tryEval catches a circular zone + // dependency the way it catches a missing zone. + state.error("builtins.tecnixMemoize: circular evaluation detected for key '%s'", key) + .atPos(pos) + .debugThrow(); + + tecnixMemoizeInProgress.push_back(lookup); + Finally popInProgress([&]() { tecnixMemoizeInProgress.pop_back(); }); + + // Evaluate `f k` outside any map bucket lock, so re-entrant misses (zone A + // loading zone B) can insert their own entries without deadlock. Under + // tracking, scope the evaluation so its accesses intern into one reusable + // label that every later consumer records. + result = state.allocValue(); + if (trackingCtx) { + TrackedSourceDepsScope scope(*trackingCtx); + state.callFunction(*function, *args[1], *result, pos); + state.forceValue(*result, pos); + sourceDeps = scope.finish(result); + } else { + state.callFunction(*function, *args[1], *result, pos); + state.forceValue(*result, pos); + } + + // Insert, but if a parallel evaluation raced ahead, reuse its entry so the + // table keeps a single shared value. + memoCache.try_emplace_and_cvisit( + EvalTecnixMemoizeKey{function, std::string(key)}, + EvalTecnixMemoizeEntry{}, + [&](auto & entry) { + entry.second.function = RootValue(function); + entry.second.value = RootValue(result); + entry.second.sourceDeps = sourceDeps; + }, + [&](const auto & entry) { + result = *entry.second.value; + sourceDeps = entry.second.sourceDeps; + }); + + if (trackingCtx) + recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); + v = *result; +} + +static RegisterPrimOp primop_tecnixMemoize({ + .name = "__tecnixMemoize", + .args = {"f", "k"}, + .doc = R"( + Memoize the function `f`: `builtins.tecnixMemoize f` is a function `g` + such that `g k` is always `f k`, but each result is computed once and + then shared. Bind `g` once and call it wherever `f k` is needed. + + Results are keyed by `f` and `k`. `f` is identified by its value (the + same binding), not by what it computes: every `g` made from the same + value of `f` shares its results, and a different function (another + system's loader, say) never sees them, even for equal keys. `k` must be + a string without string context; two keys are equal when they are the + same string. `f` receives `k` itself. + + Results live as long as the evaluator (one `EvalState`) and are never + freed. A call whose `f k` throws stores nothing, so the next call + evaluates `f k` again. + + Under Tecnix source tracking, the evaluation of `f k` records the + sources it reads, and every tracked call that reuses the result records + them too, so each target depends on everything the shared value was + computed from. Tracked and untracked calls keep separate results, each + computed once: neither kind of call is ever handed the other's, since + an untracked result has no sources recorded, and forcing parts of a + tracked result outside tracking would lose theirs. + + If computing `f k` calls `g k` again, that call throws an error that + `builtins.tryEval` can catch. + + Example: + + ```nix + let + zone = builtins.tecnixMemoize (zonePath: import ./load-zone.nix zonePath); + in + [ (zone "//areas/tools/dev") (zone "//areas/tools/dev") ] + ``` + + evaluates `import ./load-zone.nix "//areas/tools/dev"` once. + )", + .impl = prim_tecnixMemoize, +}); + // ============================================================================ // Shared helpers for Tecnix target evaluation and dependency-path tracking. // ============================================================================ diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index 7fbb2cd9e..604b064a6 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -212,135 +212,6 @@ static RegisterPrimOp primop_tectonixManifestIdToPath({ .impl = prim_tectonixManifestIdToPath, }); -// Re-entrancy guard for `builtins.tectonixMemo`: keys whose `f key` evaluation -// is currently in progress on this thread. Tracked evaluation is -// single-threaded, so a same-key re-entry can only be a genuine cycle (zone A -// loading itself transitively); the guard turns it into a catchable error -// instead of unbounded recursion through fresh thunks each call. -static thread_local std::vector tectonixMemoInProgress; - -// ============================================================================ -// builtins.tectonixMemo namespace key f -// Evaluates `f key` once per EvalState and returns the shared value. While -// tracked, the one evaluation records its source accesses into a label stored -// alongside the value; every later consumer records that label as a child of -// its own frame (and inherits the value's label through the value-copy hooks), -// so all consumers share the same tracked dependencies without re-evaluating -// `f key`. -// ============================================================================ -static void prim_tectonixMemo(EvalState & state, const PosIdx pos, Value ** args, Value & v) -{ - auto ns = - state.forceStringNoCtx(*args[0], pos, "while evaluating the 'namespace' argument to builtins.tectonixMemo"); - auto keyStr = state.forceStringNoCtx(*args[1], pos, "while evaluating the 'key' argument to builtins.tectonixMemo"); - - auto cacheKey = std::string(ns) + '\0' + std::string(keyStr); - auto * trackingCtx = currentTecnixThreadState.trackingContext; - - /* A result computed outside tracking carries no label, and the lazy parts - of a tracked result must only ever be forced under tracking, so tracked - and untracked calls never share a table: a tracked call reusing an - untracked result would record none of its sources, and an untracked call - forcing part of a tracked result would leave that part unlabelled for - every later tracked consumer. Untracked results are still memoized, in - their own table: `f` is typically recursive (zone A loads zone B), so - building them afresh would re-walk everything they share. */ - auto & memoCache = - trackingCtx ? *state.tecnixEvalData().trackedTecnixMemoCache : *state.tecnixEvalData().tecnixMemoCache; - - Value * stored = nullptr; - EvalSourceAccessSetId sourceDeps = emptyEvalSourceAccessSetId; - - bool hit = false; - memoCache.cvisit(cacheKey, [&](auto & i) { - stored = *i.second.value; - sourceDeps = i.second.sourceDeps; - hit = true; - }); - if (hit) { - // Replay into this consumer the sources the entry's evaluation read. - if (trackingCtx) - recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); - v = *stored; - return; - } - - // Miss. Guard against same-key re-entry (a cycle through `f`). - for (const auto & inProgress : tectonixMemoInProgress) { - if (inProgress == cacheKey) - // AssertionError so builtins.tryEval can catch a circular zone - // dependency the same way it catches a missing zone, instead of - // crashing the whole evaluation. - state - .error( - "builtins.tectonixMemo: circular evaluation detected for namespace '%s' key '%s'", ns, keyStr) - .atPos(pos) - .debugThrow(); - } - - tectonixMemoInProgress.push_back(cacheKey); - Finally popInProgress([&]() { tectonixMemoInProgress.pop_back(); }); - - // Evaluate `f key` outside any map bucket lock so re-entrant misses (zone A - // loading zone B) can insert their own entries without deadlock. Under - // tracking, scope the evaluation so its accesses intern into one reusable - // label that every later consumer records. - if (trackingCtx) { - TrackedSourceDepsScope scope(*trackingCtx); - stored = state.allocValue(); - state.callFunction(*args[2], *args[1], *stored, pos); - state.forceValue(*stored, pos); - sourceDeps = scope.finish(stored); - } else { - stored = state.allocValue(); - state.callFunction(*args[2], *args[1], *stored, pos); - state.forceValue(*stored, pos); - } - - // Insert, but if another evaluation raced ahead (parallel evaluation), - // reuse its entry so the table keeps a single shared value. - memoCache.try_emplace_and_cvisit( - cacheKey, - EvalTecnixMemoCacheEntry{}, - [&](auto & i) { - i.second.value = RootValue(stored); - i.second.sourceDeps = sourceDeps; - }, - [&](auto & i) { - stored = *i.second.value; - sourceDeps = i.second.sourceDeps; - }); - - if (trackingCtx) - recordTrackedSourceAccessSetDependency(*trackingCtx, sourceDeps); - v = *stored; -} - -static RegisterPrimOp primop_tectonixMemo({ - .name = "__tectonixMemo", - .args = {"namespace", "key", "f"}, - .doc = R"( - Evaluate `f key` once per evaluation and return the shared value. Later - calls with the same `namespace` and `key` return the cached value without - re-invoking `f`, and (under Tecnix source tracking) every consumer - inherits the cached value's recorded source dependencies, so the work and - the tracked closure are both shared. - - `namespace` and `key` must be strings; `f` is called as `f key`. A - self-referential `f` (one whose evaluation re-enters `tectonixMemo` with - the same namespace and key) throws a catchable circular-evaluation error. - - Results computed outside Tecnix source tracking and results computed - under it are kept apart: neither is ever returned to the other kind of - call. - - Example: - `builtins.tectonixMemo "zones" "//a/b" (path: loadZone path)` - evaluates `loadZone "//a/b"` once and shares the result. - )", - .impl = prim_tectonixMemo, -}); - static std::string normalizeTrackedRepoPath(std::string_view path); // ============================================================================ diff --git a/src/libexpr/tecnix/eval-data.hh b/src/libexpr/tecnix/eval-data.hh index 5d966fdc4..920ef53cb 100644 --- a/src/libexpr/tecnix/eval-data.hh +++ b/src/libexpr/tecnix/eval-data.hh @@ -5,6 +5,7 @@ #include "nix/expr/root-value.hh" #include "nix/expr/tecnix/access-set-graph.hh" +#include #include #include #include @@ -14,6 +15,7 @@ #include #include #include +#include #include namespace nix { @@ -48,20 +50,72 @@ struct EvalTecnixModuleCacheEntry using EvalTecnixModuleCache = boost::concurrent_flat_map; /** - * A memoized `builtins.tectonixMemo` result: the shared finished value plus - * the source-access-set label recorded the one time `f key` was evaluated. - * Every later consumer records `sourceDeps` as a child of its own frame (and - * inherits the value's label through the normal value-copy hooks), so the - * shared value's tracked dependencies propagate to all consumers without - * re-evaluating `f key`. + * A `builtins.tecnixMemoize` table key: the memoized function, identified by + * the address of its value, and the key it was called with. `Lookup` is the + * non-owning form, so a hit does not copy the key. */ -struct EvalTecnixMemoCacheEntry +struct EvalTecnixMemoizeKey { + const Value * function; + std::string key; + + struct Lookup + { + const Value * function; + std::string_view key; + }; + + struct Hash + { + using is_transparent = void; + + std::size_t operator()(const Lookup & k) const noexcept + { + std::size_t seed = 0; + boost::hash_combine(seed, k.function); + boost::hash_combine(seed, k.key); + return seed; + } + + std::size_t operator()(const EvalTecnixMemoizeKey & k) const noexcept + { + return (*this)(Lookup{k.function, k.key}); + } + }; + + struct Equal + { + using is_transparent = void; + + bool operator()(const auto & a, const auto & b) const noexcept + { + return a.function == b.function && a.key == b.key; + } + }; +}; + +/** + * A memoized `builtins.tecnixMemoize` result: the shared finished value of + * `f k` plus, in the tracked table, the source-access-set label recorded the + * one time it was evaluated. Every later tracked consumer records + * `sourceDeps` as a child of its own frame (and inherits the value's label + * through the normal value-copy hooks), so the shared value's tracked + * dependencies propagate to all consumers without re-evaluating `f k`. + */ +struct EvalTecnixMemoizeEntry +{ + /** Roots `f`, so that while this entry exists no other value can be + allocated at `f`'s address and inherit the entry. */ + RootValue function; RootValue value; EvalSourceAccessSetId sourceDeps = emptyEvalSourceAccessSetId; }; -using EvalTecnixMemoCache = boost::concurrent_flat_map; +using EvalTecnixMemoizeCache = boost::concurrent_flat_map< + EvalTecnixMemoizeKey, + EvalTecnixMemoizeEntry, + EvalTecnixMemoizeKey::Hash, + EvalTecnixMemoizeKey::Equal>; struct EvalState::TecnixEvalData { @@ -155,18 +209,18 @@ struct EvalState::TecnixEvalData const ref tecnixModuleCache = make_ref(); /** - * Memoization tables for `builtins.tectonixMemo`, keyed by - * `namespace\0key`. Each entry holds the single shared result of `f key` - * and, in the tracked table, its recorded source-access-set label, so the - * (potentially expensive) `f key` evaluation runs once per EvalState and - * every tracked consumer inherits the same tracked dependencies. Tracked - * and untracked calls use separate tables, like `trackedFileEvalCache` - * and `fileEvalCache`, so a result never crosses between them. Misses - * evaluate outside any bucket lock, so re-entrant misses (zone A loading - * zone B) never deadlock. + * Memoization tables for `builtins.tecnixMemoize`, keyed by the memoized + * function and the key. Each entry holds the single shared result of + * `f k` and, in the tracked table, its recorded source-access-set label, + * so the (potentially expensive) `f k` evaluation runs once per EvalState + * and every tracked consumer inherits the same tracked dependencies. + * Tracked and untracked calls use separate tables, like + * `trackedFileEvalCache` and `fileEvalCache`, so a result never crosses + * between them. Misses evaluate outside any bucket lock, so re-entrant + * misses (zone A loading zone B) never deadlock. */ - const ref tecnixMemoCache = make_ref(); - const ref trackedTecnixMemoCache = make_ref(); + const ref tecnixMemoizeCache = make_ref(); + const ref trackedTecnixMemoizeCache = make_ref(); /** Lazy-initialized set of zone IDs in sparse checkout (thread-safe via once_flag) */ mutable std::once_flag tectonixSparseCheckoutRootsFlag; diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh index 7ae394cf9..c1561cf27 100755 --- a/tests/functional/tecnix/manifest-tracking.sh +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -4,10 +4,11 @@ # A target that reads one zone's manifest entry (`tectonixManifestEntry`), the # key set (`tectonixManifestKeys`) or one id (`tectonixManifestIdToPath`) # depends on exactly that, not on the whole `.meta/manifest.json`. So adding an -# unrelated zone leaves such a target's eval-cache row valid. `tectonixMemo` -# shares one evaluation, and the source dependencies recorded during it, among -# every consumer, but never shares a result computed outside tracking with a -# tracked target (or the other way round). +# unrelated zone leaves such a target's eval-cache row valid. A function +# memoized with `tecnixMemoize` shares one evaluation, and the source +# dependencies recorded during it, among every consumer, but never shares a +# result computed outside tracking with a tracked target (or the other way +# round). source "$(dirname "${BASH_SOURCE[0]}")/common.sh" @@ -49,11 +50,11 @@ let }; entry = name: builtins.tectonixManifestEntry "//zones/${name}"; # Two consumers of one memoized load: the load reads shared.txt once. - loaded = builtins.tectonixMemo "loader" "shared" (_: builtins.readFile ./shared.txt); + loaded = builtins.tecnixMemoize (_: builtins.readFile ./shared.txt) "shared"; # Memoized lookups that every consumer calls afresh, so every consumer after # the first is a memo hit rather than a force of one shared thunk. - zoneId = name: builtins.tectonixMemo "zone-id" name (n: (entry n).id); - zoneRecord = name: builtins.tectonixMemo "zone-record" name (n: { id = (entry n).id; }); + zoneId = builtins.tecnixMemoize (name: (entry name).id); + zoneRecord = builtins.tecnixMemoize (name: { id = (entry name).id; }); in { allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" ]; diff --git a/tests/functional/tecnix/memoize.sh b/tests/functional/tecnix/memoize.sh new file mode 100755 index 000000000..715a3311a --- /dev/null +++ b/tests/functional/tecnix/memoize.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash +# builtins.tecnixMemoize: `g = builtins.tecnixMemoize f` is `f`, with each +# `f k` evaluated once per evaluation and shared. What tracked calls record, and +# that tracked and untracked results stay apart, is in manifest-tracking.sh. + +# shellcheck disable=SC2016 +source "$(dirname "${BASH_SOURCE[0]}")/common.sh" + +memo_eval() { + nix eval --raw --extra-experimental-features 'nix-command' --expr "$1" +} + +# How often `f` ran for `k`, per the `computing ` traces in file $1. +computed() { + grep -c "trace: computing $2\$" "$1" || true +} + +out=$(memo_eval 'builtins.tecnixMemoize (x: x + "!") "k"') +[[ "$out" == "k!" ]] || fail "g k should be f k, got '$out'" + +# Every call for a key after the first reuses its result; each key is computed once. +memo_eval 'let g = builtins.tecnixMemoize (x: builtins.trace "computing ${x}" (x + "!")); + in builtins.concatStringsSep "|" [ (g "k") (g "k") (g "k2") (g "k") ]' \ + > "$TEST_ROOT/once.out" 2> "$TEST_ROOT/once.err" +[[ "$(cat "$TEST_ROOT/once.out")" == "k!|k!|k2!|k!" ]] || fail "unexpected results: $(cat "$TEST_ROOT/once.out")" +[[ "$(computed "$TEST_ROOT/once.err" k)" == 1 ]] || fail "f should run once for 'k'" +[[ "$(computed "$TEST_ROOT/once.err" k2)" == 1 ]] || fail "f should run once for 'k2'" + +# Results belong to the function: another function never sees them, even for an equal key. +out=$(memo_eval 'let g1 = builtins.tecnixMemoize (x: x + "!"); g2 = builtins.tecnixMemoize (x: x + "?"); + in builtins.seq (g1 "k") "${g1 "k"}|${g2 "k"}"') +[[ "$out" == "k!|k?" ]] || fail "each function should get its own results, got '$out'" + +# Every g made from the same value of f shares them. +memo_eval 'let f = x: builtins.trace "computing ${x}" (x + "!"); g1 = builtins.tecnixMemoize f; g2 = builtins.tecnixMemoize f; + in "${g1 "k"}|${g2 "k"}"' > "$TEST_ROOT/same-f.out" 2> "$TEST_ROOT/same-f.err" +[[ "$(cat "$TEST_ROOT/same-f.out")" == "k!|k!" ]] || fail "unexpected results: $(cat "$TEST_ROOT/same-f.out")" +[[ "$(computed "$TEST_ROOT/same-f.err" k)" == 1 ]] || fail "memoizing the same f twice should share results" + +# Only strings are keys. +expectStderr 1 nix eval --extra-experimental-features 'nix-command' --expr 'builtins.tecnixMemoize (x: x) 1' \ + | grepQuiet "expected a string but found an integer" + +# A call that needs its own result throws, catchably, instead of recursing forever. +out=$(memo_eval 'let g = builtins.tecnixMemoize (k: g k); in if (builtins.tryEval (g "a")).success then "no-throw" else "threw"') +[[ "$out" == "threw" ]] || fail "a circular call should throw a catchable error, got '$out'" + +# Re-entering for a different key is not a cycle. +out=$(memo_eval 'let g = builtins.tecnixMemoize (k: if k == "root" then g "child" else k + "-val"); in g "root"') +[[ "$out" == "child-val" ]] || fail "expected 'child-val', got '$out'" diff --git a/tests/functional/tecnix/meson.build b/tests/functional/tecnix/meson.build index a60d2faae..540675a5b 100644 --- a/tests/functional/tecnix/meson.build +++ b/tests/functional/tecnix/meson.build @@ -7,6 +7,7 @@ suites += { 'gc.sh', 'manifest-tracking.sh', 'ingestion-stats.sh', + 'memoize.sh', ], 'workdir' : meson.current_source_dir(), } diff --git a/tests/functional/tectonix/memo.sh b/tests/functional/tectonix/memo.sh deleted file mode 100755 index ffae824a3..000000000 --- a/tests/functional/tectonix/memo.sh +++ /dev/null @@ -1,64 +0,0 @@ -#!/usr/bin/env bash -# Test builtins.tectonixMemo: evaluate `f key` once per EvalState and share the -# result (and its tracked source deps) across consumers within one evaluation. - -# shellcheck disable=SC2016 -source "$(dirname "${BASH_SOURCE[0]}")/common.sh" - -TEST_WORLD="$TEST_ROOT/world" -create_test_world "$TEST_WORLD" -HEAD_SHA=$(get_head_sha "$TEST_WORLD") - -# 1. Correctness: tectonixMemo returns f key. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'builtins.tectonixMemo "ns" "k" (x: x + "!")') -[[ "$out" == "k!" ]] || fail "expected 'k!', got '$out'" -echo "correctness ok" - -# 2. Within-process memoization: force the first call (caching it), then a -# second call with the same (namespace,key) but a different f must return the -# cached value, proving the second f was never invoked. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns" "k" (x: "WRONG"); in builtins.seq a b') -[[ "$out" == "k!" ]] || fail "expected cached 'k!', got '$out'" -echo "memoization (same key, same eval) ok" - -# 3. Distinct keys evaluate independently within one eval. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns" "k2" (x: x + "!"); in "${a}|${b}"') -[[ "$out" == "k!|k2!" ]] || fail "expected 'k!|k2!', got '$out'" -echo "distinct key ok" - -# 4. Distinct namespaces are independent within one eval. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let a = builtins.tectonixMemo "ns" "k" (x: x + "!"); b = builtins.tectonixMemo "ns2" "k" (x: x + "?"); in "${a}|${b}"') -[[ "$out" == "k!|k?" ]] || fail "expected 'k!|k?', got '$out'" -echo "distinct namespace ok" - -# 5. Sharing: two consumers of the same memo entry get equal values. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let f = x: x + "-shared"; a = builtins.tectonixMemo "share" "z" f; b = builtins.tectonixMemo "share" "z" f; in builtins.seq a (if a == b then a else "MISMATCH")') -[[ "$out" == "z-shared" ]] || fail "expected 'z-shared', got '$out'" -echo "sharing ok" - -# 6. The memoized value can be an attrset and is shared structurally. -out=$(tectonix_eval_json "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let f = x: { path = x; tag = "node"; }; a = builtins.tectonixMemo "attrs" "//a" f; b = builtins.tectonixMemo "attrs" "//a" f; in builtins.seq a { a = a; sameRef = a == b; }') -echo "$out" | grepQuiet '"sameRef":true' -echo "$out" | grepQuiet '"path":"//a"' -echo "attrset sharing ok" - -# 7. Circular self-reference throws a catchable error (not a crash). -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let f = k: builtins.tectonixMemo "cyc" k f; r = builtins.tryEval (builtins.tectonixMemo "cyc" "a" f); in if r.success then "NO-THROW" else "THREW"') -[[ "$out" == "THREW" ]] || fail "expected circular memo to throw, got '$out'" -echo "cycle detection ok" - -# 8. Re-entrant miss for a DIFFERENT key (a dependency) works: zone-like -# recursion through distinct keys resolves without a false cycle error. -out=$(tectonix_eval "$TEST_WORLD/.git" "$HEAD_SHA" \ - 'let load = k: if k == "root" then builtins.tectonixMemo "dep" "child" load else k + "-val"; in builtins.tectonixMemo "dep" "root" load') -[[ "$out" == "child-val" ]] || fail "expected 'child-val', got '$out'" -echo "re-entrant distinct-key ok" - -echo "tectonixMemo test passed!" diff --git a/tests/functional/tectonix/meson.build b/tests/functional/tectonix/meson.build index e577a7986..c6a3a29ea 100644 --- a/tests/functional/tectonix/meson.build +++ b/tests/functional/tectonix/meson.build @@ -6,7 +6,6 @@ suites += { 'errors.sh', 'deduplication.sh', 'dirty-zones.sh', - 'memo.sh', 'raw-zone-trees.sh', ], 'workdir' : meson.current_source_dir(), From ee40917090fd21af9d1d2f3fbad92aa1e7a27653 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Wed, 30 Sep 2026 18:26:45 -0400 Subject: [PATCH 07/10] libexpr: read the per-entry manifest builtins through the Tecnix repo accessor tectonixManifestEntry, tectonixManifestKeys and tectonixManifestIdToPath read .meta/manifest.json through the legacy loader, while the fingerprints of the dependency keys they record read it through the Tecnix repo accessor, like every other tracked read. The two can serve different manifests: from a checkout whose HEAD has moved past the evaluated rev, the legacy loader reads the working tree's file and the accessor serves the rev's, so a target's value and its recorded fingerprints described different manifests. The builtins now read the manifest through the accessor as well, parsed once per evaluation and without recording the whole file (they record their own keys). The new manifest-tracking case evaluates the first commit from a checkout at a later one; before this change the builtins report the checkout's ids and key set while the resolver reads the commit's manifest, after it they agree. The legacy unsafeTectonixInternalManifest builtins keep the legacy loader. --- .../nix/expr/tecnix/source-accessors.hh | 6 ++++ src/libexpr/primops/tectonix.cc | 16 +++++++---- src/libexpr/tecnix/eval-data.hh | 5 ++++ src/libexpr/tecnix/repo-accessor.cc | 23 +++++++++++++++ tests/functional/tecnix/manifest-tracking.sh | 28 ++++++++++++++++++- 5 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/libexpr/include/nix/expr/tecnix/source-accessors.hh b/src/libexpr/include/nix/expr/tecnix/source-accessors.hh index 7899883da..9f9451881 100644 --- a/src/libexpr/include/nix/expr/tecnix/source-accessors.hh +++ b/src/libexpr/include/nix/expr/tecnix/source-accessors.hh @@ -17,6 +17,12 @@ const std::string & getManifestContent(const EvalState & state); const nlohmann::json & getManifestJson(const EvalState & state); StorePath getLegacyTectonixZoneStorePath(EvalState & state, std::string_view zonePath); ref getTecnixRepoAccessor(EvalState & state); +/** + * `.meta/manifest.json` as the Tecnix repo accessor serves it (the view that + * also fingerprints Tecnix dependencies), parsed once per evaluation. The + * read records nothing: callers record the part of the manifest they observe. + */ +const nlohmann::json & getTecnixManifestJson(EvalState & state); /** Resolve a checkout's HEAD to a commit SHA (throws if it has none). */ std::string resolveCheckoutHeadRev(const std::string & checkoutPath); diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index 604b064a6..408566636 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -119,7 +119,7 @@ static void prim_tectonixManifestEntry(EvalState & state, const PosIdx pos, Valu *args[0], pos, "while evaluating the 'zonePath' argument to builtins.tectonixManifestEntry"); if (auto ctx = currentTecnixThreadState.trackingContext; ctx) ctx->recordAccess(".meta/manifest.json#" + std::string(zonePath)); - auto & manifest = getManifest(state); + auto & manifest = getTecnixManifestJson(state); auto it = manifest.find(std::string(zonePath)); if (it == manifest.end() || !it->is_object() || !it->contains("id") || !(*it).at("id").is_string()) { v.mkNull(); @@ -137,7 +137,9 @@ static RegisterPrimOp primop_tectonixManifestEntry({ Get a single zone's manifest entry as a Nix attrset { id = "W-xxxxxx"; }, or null if the zone does not exist. Records only that entry as a Tecnix tracked dependency (synthetic path .meta/manifest.json#), not - the whole manifest file. + the whole manifest file. Reads the manifest through the Tecnix + repository view (the evaluated rev plus the checkout's uncommitted + changes) that every tracked read and every dependency fingerprint uses. Example: `builtins.tectonixManifestEntry "//areas/tools/dev"` returns `{ id = "W-123456"; }`. )", .impl = prim_tectonixManifestEntry, @@ -152,7 +154,7 @@ static void prim_tectonixManifestKeys(EvalState & state, const PosIdx pos, Value { if (auto ctx = currentTecnixThreadState.trackingContext; ctx) ctx->recordAccess(".meta/manifest.json#keys"); - auto & manifest = getManifest(state); + auto & manifest = getTecnixManifestJson(state); std::vector keys; keys.reserve(manifest.size()); for (auto & [path, value] : manifest.items()) @@ -175,6 +177,8 @@ static RegisterPrimOp primop_tectonixManifestKeys({ set as a Tecnix tracked dependency (synthetic path .meta/manifest.json#keys), not the whole manifest file. Only world-wide folds that enumerate every zone should use this; per-target resolution should use tectonixManifestEntry. + Reads the manifest through the same Tecnix repository view as + tectonixManifestEntry. )", .impl = prim_tectonixManifestKeys, }); @@ -190,7 +194,7 @@ static void prim_tectonixManifestIdToPath(EvalState & state, const PosIdx pos, V *args[0], pos, "while evaluating the 'zoneId' argument to builtins.tectonixManifestIdToPath"); if (auto ctx = currentTecnixThreadState.trackingContext; ctx) ctx->recordAccess(".meta/manifest.json#id/" + std::string(zoneId)); - auto & manifest = getManifest(state); + auto & manifest = getTecnixManifestJson(state); for (auto & [path, value] : manifest.items()) { if (value.contains("id") && value.at("id").is_string() && value.at("id").get() == zoneId) { v.mkString(path, state.mem); @@ -206,7 +210,9 @@ static RegisterPrimOp primop_tectonixManifestIdToPath({ .doc = R"( Get the zone path for a zone ID, or null if not found. Records only that id lookup as a Tecnix tracked dependency (synthetic path - .meta/manifest.json#id/), not the whole manifest file. + .meta/manifest.json#id/), not the whole manifest file. Reads the + manifest through the same Tecnix repository view as + tectonixManifestEntry. Example: `builtins.tectonixManifestIdToPath "W-123456"` returns `"//areas/tools/dev"`. )", .impl = prim_tectonixManifestIdToPath, diff --git a/src/libexpr/tecnix/eval-data.hh b/src/libexpr/tecnix/eval-data.hh index 920ef53cb..b29c6bc87 100644 --- a/src/libexpr/tecnix/eval-data.hh +++ b/src/libexpr/tecnix/eval-data.hh @@ -176,6 +176,11 @@ struct EvalState::TecnixEvalData mutable std::once_flag tecnixRepoAccessorFlag; mutable std::optional> tecnixRepoAccessor; + /** `.meta/manifest.json` as `tecnixRepoAccessor` serves it, parsed + (thread-safe via once_flag). */ + mutable std::once_flag tecnixManifestJsonFlag; + mutable std::unique_ptr tecnixManifestJson; + /** * Virtual store path where the Tecnix repo-wide accessor is lazily mounted. * All repo subtree store paths are subpaths of this mount. diff --git a/src/libexpr/tecnix/repo-accessor.cc b/src/libexpr/tecnix/repo-accessor.cc index 98ff3734f..05dddcfd3 100644 --- a/src/libexpr/tecnix/repo-accessor.cc +++ b/src/libexpr/tecnix/repo-accessor.cc @@ -324,6 +324,13 @@ struct TecnixSourceAccessor : SourceAccessor return (isDirty(path) ? disk : clean)->readFile(path, sink, sizeCallback); } + /** `readFile` without recording the access, for a caller that records + its own, finer-grained observation of the file instead. */ + std::string readFileUnrecorded(const CanonPath & path) + { + return (isDirty(path) ? disk : clean)->readFile(path); + } + std::string readLink(const CanonPath & path) override { if (dumpPathDepth == 0) @@ -762,6 +769,22 @@ ref getTecnixRepoAccessor(EvalState & state) return *tecnixData(state)->tecnixRepoAccessor; } +const nlohmann::json & getTecnixManifestJson(EvalState & state) +{ + std::call_once(tecnixData(state)->tecnixManifestJsonFlag, [&state]() { + // getTecnixRepoAccessor always builds a TecnixSourceAccessor. + auto & accessor = static_cast(*getTecnixRepoAccessor(state)); + auto content = accessor.readFileUnrecorded(CanonPath(".meta/manifest.json")); + try { + tecnixData(state)->tecnixManifestJson = std::make_unique(nlohmann::json::parse(content)); + } catch (nlohmann::json::parse_error & e) { + throw Error( + "cannot parse '.meta/manifest.json' at commit '%s': %s", requireTectonixGitSha(state), e.what()); + } + }); + return *tecnixData(state)->tecnixManifestJson; +} + StorePath mountTecnixRepoAccessor(EvalState & state) { std::call_once(tecnixData(state)->tecnixRepoMountFlag, [&state]() { diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh index c1561cf27..15306604b 100755 --- a/tests/functional/tecnix/manifest-tracking.sh +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -57,7 +57,7 @@ let zoneRecord = builtins.tecnixMemoize (name: { id = (entry name).id; }); in { - allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" ]; + allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" "agree" ]; resolve = name: if name == "whole" then drv name (toString (builtins.length (builtins.attrNames builtins.unsafeTectonixInternalManifest))) else if name == "keys" then drv name (toString (builtins.length (builtins.tectonixManifestKeys))) @@ -71,6 +71,18 @@ in # Fills the memoized record without forcing its field. else if name == "fill-record" then builtins.seq (zoneRecord "a") (drv name "record") else if name == "record-field" then drv name (zoneRecord "a").id + # What the manifest builtins report about zone a, beside what reading + # .meta/manifest.json itself shows. + else if name == "agree" then + let + manifest = builtins.fromJSON (builtins.readFile ./.meta/manifest.json); + id = manifest."//zones/a".id; + in drv name "agree" // { + seen = { + builtins = { inherit (entry "a") id; keys = builtins.tectonixManifestKeys; path = builtins.tectonixManifestIdToPath id; }; + file = { inherit id; keys = builtins.attrNames manifest; path = "//zones/a"; }; + }; + } else drv name (entry name).id; } EOF @@ -238,4 +250,18 @@ XDG_CACHE_HOME="$PREFILL_CACHE_HOME" prefill_deps_at "$REV7" --option tecnix-eva > /dev/null 2> "$TEST_ROOT/prefill-warm.err" misses "$TEST_ROOT/prefill-warm.err" prefilled record-field +# The manifest builtins read .meta/manifest.json through the repo accessor, +# like every file the resolver reads and like the fingerprints of their +# dependency keys. So evaluating an older rev from a checkout whose HEAD has +# moved on shows them that rev's manifest, not the working tree's. +echo "Testing the manifest builtins at an older rev of a checkout..." +seen=$(nix eval --json \ + --extra-experimental-features 'nix-command' \ + --option lazy-trees true \ + --option tecnix-eval-cache false \ + --expr "(builtins.tecnixTargets { gitDir = \"$WORLD/.git\"; resolver = \"resolve.nix\"; args = { system = \"test-system\"; }; + rev = \"$REV1\"; checkoutPath = \"$WORLD\"; targets = [ \"agree\" ]; }).agree.seen") +assert_jq "$seen" '.file.id == "W-000001"' "the resolver should read the evaluated rev's manifest" +assert_jq "$seen" '.builtins == .file' "the manifest builtins should see the manifest the resolver reads" + echo "All manifest tracking tests passed!" From 4f0c5b3ff9e1c906cb919e29787622521bd4648d Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Wed, 30 Sep 2026 18:30:54 -0400 Subject: [PATCH 08/10] libexpr, doc: state the manifest dependency-key grammar in one place The synthetic keys the manifest builtins record (.meta/manifest.json#, #keys and #id/) are a contract with anything that reads closures, such as a tool that maps changed files to affected targets. Their exact grammar, what each fingerprint tracks, and the rule for consumers (such a key depends on .meta/manifest.json, and a consumer that cannot interpret the fragment must treat any change to that file as affecting it) are now stated in the tectonixManifestEntry documentation and in a new explainer section, 4.2. Only keys that begin with `.meta/manifest.json#` are synthetic: repo paths may contain `#`, so a consumer must not split arbitrary keys at it. One key changes. tectonixManifestEntry given an argument that does not start with `//`, which is never a manifest key, now records the whole file: recorded verbatim, such an argument could spell another form (`keys`, say) whose fingerprint tracks something else. Zone-path arguments record the same keys as before. --- plans/tecnix-target-eval-caching/explainer.md | 18 +++++++ src/libexpr/primops/tectonix.cc | 51 +++++++++++++++---- tests/functional/tecnix/manifest-tracking.sh | 9 +++- 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/plans/tecnix-target-eval-caching/explainer.md b/plans/tecnix-target-eval-caching/explainer.md index 937a05566..25218af3a 100644 --- a/plans/tecnix-target-eval-caching/explainer.md +++ b/plans/tecnix-target-eval-caching/explainer.md @@ -202,6 +202,24 @@ This is the precise sense in which the system works across commits. After a comm The same property makes the system fail-safe. A row is never believed on the strength of its key; it is believed only when proven. Format changes, corruption, and storage defects therefore all degrade into cache misses, never into wrong answers. In practice, the difference between a cold run and a warm run is the difference between roughly a minute of full tracked evaluation and well under a second — several orders of magnitude, with the gap consisting precisely of "all of evaluation" versus "fingerprint checks and output construction." +### 4.2 Synthetic keys: parts of the manifest + +Not every closure key is a file. A target that looks up one zone in `.meta/manifest.json` depends on that zone's entry, not on the whole file, and recording the file would invalidate every such target whenever any zone is added or renumbered. The manifest builtins therefore record *synthetic* keys, each naming one observation of the manifest. This is the complete list: + +| key | recorded by | fingerprint changes exactly when | +|---|---|---| +| `.meta/manifest.json#` | `builtins.tectonixManifestEntry zonePath` | the JSON value of that zone's entry (any field, not only `id`) changes, appears, or disappears | +| `.meta/manifest.json#keys` | `builtins.tectonixManifestKeys` | the set of zone paths (manifest keys) changes | +| `.meta/manifest.json#id/` | `builtins.tectonixManifestIdToPath zoneId` | a different zone path, or none, is the first in sorted order whose entry has the id `zoneId` | + +The builtins read the manifest through the same accessor as every other tracked read (§7), so a value and the fingerprint of its key always describe the same manifest. The grammar is exact: + +- A key is synthetic if and only if it begins with `.meta/manifest.json#`. Every other key is a repo-relative path, even one that contains `#`; splitting arbitrary keys at `#` is wrong. +- The fragment after that prefix is `keys`, or `id/` followed by a zone id (everything after `id/`, verbatim), or a zone path, which begins with `//`. `tectonixManifestEntry` given an argument that does not begin with `//` records the whole file, `.meta/manifest.json`, so the three forms never overlap. +- A synthetic key depends on the file `.meta/manifest.json`. A consumer of closure keys (for example, one mapping changed files to affected targets) that cannot interpret a fragment must treat any change to that file as affecting the key; one that can compares the named observation of the manifest before and after the change. + +Reading the whole manifest (the `unsafeTectonixInternalManifest*` builtins, or `builtins.readFile` of the file) still records the plain path `.meta/manifest.json`. + --- ## 5. A Worked Example diff --git a/src/libexpr/primops/tectonix.cc b/src/libexpr/primops/tectonix.cc index 408566636..00f235813 100644 --- a/src/libexpr/primops/tectonix.cc +++ b/src/libexpr/primops/tectonix.cc @@ -111,14 +111,21 @@ static RegisterPrimOp primop_worldManifestInverted({ // builtins.tectonixManifestEntry zonePath // Returns null or { id = "W-xxxxxx" } for a single zone, recording only that // entry as a tracked dependency (synthetic path .meta/manifest.json#) -// instead of the whole manifest file. +// instead of the whole manifest file. The doc below is the one statement of +// the synthetic-key grammar; eval-cache.cc fingerprints the keys. // ============================================================================ static void prim_tectonixManifestEntry(EvalState & state, const PosIdx pos, Value ** args, Value & v) { auto zonePath = state.forceStringNoCtx( *args[0], pos, "while evaluating the 'zonePath' argument to builtins.tectonixManifestEntry"); - if (auto ctx = currentTecnixThreadState.trackingContext; ctx) - ctx->recordAccess(".meta/manifest.json#" + std::string(zonePath)); + if (auto ctx = currentTecnixThreadState.trackingContext; ctx) { + // Manifest keys start with `//`. Anything else could spell one of the + // other key forms (`keys`, `id/...`), so it records the whole file. + if (zonePath.starts_with("//")) + ctx->recordAccess(".meta/manifest.json#" + std::string(zonePath)); + else + ctx->recordAccess(".meta/manifest.json"); + } auto & manifest = getTecnixManifestJson(state); auto it = manifest.find(std::string(zonePath)); if (it == manifest.end() || !it->is_object() || !it->contains("id") || !(*it).at("id").is_string()) { @@ -134,12 +141,34 @@ static RegisterPrimOp primop_tectonixManifestEntry({ .name = "__tectonixManifestEntry", .args = {"zonePath"}, .doc = R"( - Get a single zone's manifest entry as a Nix attrset { id = "W-xxxxxx"; }, - or null if the zone does not exist. Records only that entry as a Tecnix - tracked dependency (synthetic path .meta/manifest.json#), not - the whole manifest file. Reads the manifest through the Tecnix - repository view (the evaluated rev plus the checkout's uncommitted - changes) that every tracked read and every dependency fingerprint uses. + Get a single zone's manifest entry as a Nix attrset `{ id = "W-xxxxxx"; }`, + or null if the manifest has no entry for `zonePath` (or its entry has no + string `id`). Reads `.meta/manifest.json` through the Tecnix repository + view (the evaluated rev plus the checkout's uncommitted changes) that + every tracked read and every dependency fingerprint uses. + + Under Tecnix source tracking, the manifest builtins record what they + read of `.meta/manifest.json` as synthetic dependency keys, not the + whole file. These are all of them: + + - `.meta/manifest.json#`, from this builtin: the entry for + `zonePath`. Its fingerprint changes when the entry's JSON value (any + field, not only `id`) changes, appears or disappears. `zonePath` is + recorded verbatim; manifest keys start with `//`, and an argument + that does not records the whole file, `.meta/manifest.json`, instead. + - `.meta/manifest.json#keys`, from `builtins.tectonixManifestKeys`: the + set of zone paths (manifest keys). + - `.meta/manifest.json#id/`, from + `builtins.tectonixManifestIdToPath`: which zone path has the id + `zoneId` (the first in sorted order), or that none has. `` is + everything after `id/`, verbatim. + + Only keys that begin with `.meta/manifest.json#` are synthetic; every + other dependency key is a repo-relative path, even one that contains + `#`. A synthetic key depends on the file `.meta/manifest.json`: a + consumer of dependency keys that cannot interpret the part after the + `#` must treat any change to that file as affecting the key. + Example: `builtins.tectonixManifestEntry "//areas/tools/dev"` returns `{ id = "W-123456"; }`. )", .impl = prim_tectonixManifestEntry, @@ -178,7 +207,7 @@ static RegisterPrimOp primop_tectonixManifestKeys({ not the whole manifest file. Only world-wide folds that enumerate every zone should use this; per-target resolution should use tectonixManifestEntry. Reads the manifest through the same Tecnix repository view as - tectonixManifestEntry. + tectonixManifestEntry, whose documentation lists every synthetic key. )", .impl = prim_tectonixManifestKeys, }); @@ -212,7 +241,7 @@ static RegisterPrimOp primop_tectonixManifestIdToPath({ id lookup as a Tecnix tracked dependency (synthetic path .meta/manifest.json#id/), not the whole manifest file. Reads the manifest through the same Tecnix repository view as - tectonixManifestEntry. + tectonixManifestEntry, whose documentation lists every synthetic key. Example: `builtins.tectonixManifestIdToPath "W-123456"` returns `"//areas/tools/dev"`. )", .impl = prim_tectonixManifestIdToPath, diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh index 15306604b..fb29ea6a8 100755 --- a/tests/functional/tecnix/manifest-tracking.sh +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -57,7 +57,7 @@ let zoneRecord = builtins.tecnixMemoize (name: { id = (entry name).id; }); in { - allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" "agree" ]; + allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" "agree" "not-a-zone" ]; resolve = name: if name == "whole" then drv name (toString (builtins.length (builtins.attrNames builtins.unsafeTectonixInternalManifest))) else if name == "keys" then drv name (toString (builtins.length (builtins.tectonixManifestKeys))) @@ -83,6 +83,8 @@ in file = { inherit id; keys = builtins.attrNames manifest; path = "//zones/a"; }; }; } + # Every manifest key starts with `//`; "keys" is not one. + else if name == "not-a-zone" then drv name (if builtins.tectonixManifestEntry "keys" == null then "none" else "some") else drv name (entry name).id; } EOF @@ -157,6 +159,11 @@ assert_jq "$deps1" '.m1 == .m2 and (.m1 | has("shared.txt"))' \ "both consumers of a memoized load should inherit the load's dependencies" assert_jq "$deps1" '[.za1, .za2 | keys == [".meta/manifest.json#//zones/a", "resolve.nix"]] | all' \ "a memo hit should replay the dependencies recorded when its entry was filled" +# An argument that is not a zone path could spell another key form (here +# `#keys`, the key set), so it records the whole manifest instead. +not_a_zone=$(TARGETS='[ "not-a-zone" ]' nocache_deps_at "$REV1") +assert_jq "$not_a_zone" '."not-a-zone" | keys == [".meta/manifest.json", "resolve.nix"]' \ + "looking up a non-zone path should depend on the whole manifest" echo "Testing the eval cache across commits..." cached_deps_at "$REV1" > "$TEST_ROOT/cold.json" 2> "$TEST_ROOT/cold.err" From 07e9df4997aa72e780b40aa63f6a65b011806ee5 Mon Sep 17 00:00:00 2001 From: Josh Heinrichs Date: Thu, 1 Oct 2026 12:30:19 -0600 Subject: [PATCH 09/10] libexpr: keep a caller's key provenance out of a tecnixMemoize entry On a miss, `f` was called with the caller's own key value. If the caller computed `k` from a source read, that value carries the read's label, and `f` forcing it inside the memo scope stored the label with the shared entry, so every later caller replayed the first caller's dependencies. Call `f` with a fresh, unlabelled string equal to `k`. --- src/libexpr/primops/tecnix.cc | 24 ++++++++++++++------ tests/functional/tecnix/manifest-tracking.sh | 12 +++++++++- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/libexpr/primops/tecnix.cc b/src/libexpr/primops/tecnix.cc index 48f18031b..a831b8d43 100644 --- a/src/libexpr/primops/tecnix.cc +++ b/src/libexpr/primops/tecnix.cc @@ -187,14 +187,22 @@ static void prim_tecnixMemoize(EvalState & state, const PosIdx pos, Value ** arg // loading zone B) can insert their own entries without deadlock. Under // tracking, scope the evaluation so its accesses intern into one reusable // label that every later consumer records. + // + // `f` gets a fresh copy of the key, not `args[1]`: that value carries the + // label of however this caller computed `k` (already recorded for this + // caller by the force above), and `f` forcing it inside the scope would + // store that label with the shared entry and replay it into every later + // caller. The key is a string without context, so the copy is equal. + auto * keyArg = state.allocValue(); + keyArg->mkString(key, state.mem); result = state.allocValue(); if (trackingCtx) { TrackedSourceDepsScope scope(*trackingCtx); - state.callFunction(*function, *args[1], *result, pos); + state.callFunction(*function, *keyArg, *result, pos); state.forceValue(*result, pos); sourceDeps = scope.finish(result); } else { - state.callFunction(*function, *args[1], *result, pos); + state.callFunction(*function, *keyArg, *result, pos); state.forceValue(*result, pos); } @@ -231,7 +239,7 @@ static RegisterPrimOp primop_tecnixMemoize({ value of `f` shares its results, and a different function (another system's loader, say) never sees them, even for equal keys. `k` must be a string without string context; two keys are equal when they are the - same string. `f` receives `k` itself. + same string. `f` is called with a string equal to `k`. Results live as long as the evaluator (one `EvalState`) and are never freed. A call whose `f k` throws stores nothing, so the next call @@ -240,10 +248,12 @@ static RegisterPrimOp primop_tecnixMemoize({ Under Tecnix source tracking, the evaluation of `f k` records the sources it reads, and every tracked call that reuses the result records them too, so each target depends on everything the shared value was - computed from. Tracked and untracked calls keep separate results, each - computed once: neither kind of call is ever handed the other's, since - an untracked result has no sources recorded, and forcing parts of a - tracked result outside tracking would lose theirs. + computed from. The sources a caller read to compute `k` count for that + caller only; they are never stored with the shared result. Tracked and + untracked calls keep separate results, each computed once: neither kind + of call is ever handed the other's, since an untracked result has no + sources recorded, and forcing parts of a tracked result outside tracking + would lose theirs. If computing `f k` calls `g k` again, that call throws an error that `builtins.tryEval` can catch. diff --git a/tests/functional/tecnix/manifest-tracking.sh b/tests/functional/tecnix/manifest-tracking.sh index fb29ea6a8..dc307c9c7 100755 --- a/tests/functional/tecnix/manifest-tracking.sh +++ b/tests/functional/tecnix/manifest-tracking.sh @@ -40,6 +40,9 @@ mkdir -p "$WORLD/.meta" write_manifest "" echo "shared one" > "$WORLD/shared.txt" echo "unrelated one" > "$WORLD/unrelated.txt" +# Two files naming zone a, one memo key read from each by its own target. +printf a > "$WORLD/key1.txt" +printf a > "$WORLD/key2.txt" cat > "$WORLD/resolve.nix" <<'EOF' args: @@ -57,7 +60,7 @@ let zoneRecord = builtins.tecnixMemoize (name: { id = (entry name).id; }); in { - allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" "agree" "not-a-zone" ]; + allTargetNames = [ "a" "b" "keys" "id" "m1" "m2" "whole" "za1" "za2" "peek" "fill-record" "prefilled" "record-field" "agree" "not-a-zone" "key1" "key2" ]; resolve = name: if name == "whole" then drv name (toString (builtins.length (builtins.attrNames builtins.unsafeTectonixInternalManifest))) else if name == "keys" then drv name (toString (builtins.length (builtins.tectonixManifestKeys))) @@ -85,6 +88,7 @@ in } # Every manifest key starts with `//`; "keys" is not one. else if name == "not-a-zone" then drv name (if builtins.tectonixManifestEntry "keys" == null then "none" else "some") + else if name == "key1" || name == "key2" then drv name (zoneId (builtins.readFile ./${name}.txt)) else drv name (entry name).id; } EOF @@ -164,6 +168,12 @@ assert_jq "$deps1" '[.za1, .za2 | keys == [".meta/manifest.json#//zones/a", "res not_a_zone=$(TARGETS='[ "not-a-zone" ]' nocache_deps_at "$REV1") assert_jq "$not_a_zone" '."not-a-zone" | keys == [".meta/manifest.json", "resolve.nix"]' \ "looking up a non-zone path should depend on the whole manifest" +# How a caller computed a memo key is its own dependency, never the shared +# entry's. Either target may fill the entry first, so check both. +via=$(TARGETS='[ "key1" "key2" ]' nocache_deps_at "$REV1") +assert_jq "$via" '(.key1 | keys) == [".meta/manifest.json#//zones/a", "key1.txt", "resolve.nix"] + and (.key2 | keys) == [".meta/manifest.json#//zones/a", "key2.txt", "resolve.nix"]' \ + "a memo caller should depend on the file it read the key from, and on no other caller's" echo "Testing the eval cache across commits..." cached_deps_at "$REV1" > "$TEST_ROOT/cold.json" 2> "$TEST_ROOT/cold.err" From 6f2366a68730f015772aa51f8a8c4ce2a637a6f4 Mon Sep 17 00:00:00 2001 From: Josh Heinrichs Date: Thu, 1 Oct 2026 17:00:18 -0600 Subject: [PATCH 10/10] libexpr: make a tecnixMemoize cycle uncatchable, like infinite recursion A call that re-entered its own `f k` threw AssertionError, which builtins.tryEval catches. A loader that wraps dependency loads in tryEval then survived a zone cycle with whichever zone the evaluation reached second stored without the other, so results depended on evaluation order, and the eval cache could serve such a result after the cycle was gone, since the order is recorded nowhere. Throw InfiniteRecursionError instead: a value that needs itself fails as it does anywhere else in Nix. --- src/libexpr/primops/tecnix.cc | 11 ++++++----- tests/functional/tecnix/memoize.sh | 6 +++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libexpr/primops/tecnix.cc b/src/libexpr/primops/tecnix.cc index a831b8d43..21cf78389 100644 --- a/src/libexpr/primops/tecnix.cc +++ b/src/libexpr/primops/tecnix.cc @@ -174,9 +174,10 @@ static void prim_tecnixMemoize(EvalState & state, const PosIdx pos, Value ** arg for (auto & inProgress : tecnixMemoizeInProgress) if (EvalTecnixMemoizeKey::Equal{}(inProgress, lookup)) - // AssertionError, so builtins.tryEval catches a circular zone - // dependency the way it catches a missing zone. - state.error("builtins.tecnixMemoize: circular evaluation detected for key '%s'", key) + // A value that needs itself: fail as infinite recursion does, which + // builtins.tryEval cannot catch. + state + .error("builtins.tecnixMemoize: circular evaluation detected for key '%s'", key) .atPos(pos) .debugThrow(); @@ -255,8 +256,8 @@ static RegisterPrimOp primop_tecnixMemoize({ sources recorded, and forcing parts of a tracked result outside tracking would lose theirs. - If computing `f k` calls `g k` again, that call throws an error that - `builtins.tryEval` can catch. + If computing `f k` calls `g k` again, evaluation fails as it does for + infinite recursion, and `builtins.tryEval` cannot catch the error. Example: diff --git a/tests/functional/tecnix/memoize.sh b/tests/functional/tecnix/memoize.sh index 715a3311a..a90525e21 100755 --- a/tests/functional/tecnix/memoize.sh +++ b/tests/functional/tecnix/memoize.sh @@ -41,9 +41,9 @@ memo_eval 'let f = x: builtins.trace "computing ${x}" (x + "!"); g1 = builtins.t expectStderr 1 nix eval --extra-experimental-features 'nix-command' --expr 'builtins.tecnixMemoize (x: x) 1' \ | grepQuiet "expected a string but found an integer" -# A call that needs its own result throws, catchably, instead of recursing forever. -out=$(memo_eval 'let g = builtins.tecnixMemoize (k: g k); in if (builtins.tryEval (g "a")).success then "no-throw" else "threw"') -[[ "$out" == "threw" ]] || fail "a circular call should throw a catchable error, got '$out'" +# A call that needs its own result fails like infinite recursion: tryEval can't catch it. +expectStderr 1 nix eval --extra-experimental-features 'nix-command' --expr 'let g = builtins.tecnixMemoize (k: g k); in builtins.tryEval (g "a")' \ + | grepQuiet "circular evaluation detected" # Re-entering for a different key is not a cycle. out=$(memo_eval 'let g = builtins.tecnixMemoize (k: if k == "root" then g "child" else k + "-val"); in g "root"')