From ab9212f0f5bdc8e5cbd2f574b06d4a6fbfabd654 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 17:14:18 -0400 Subject: [PATCH 1/6] docs(163): normative system contract, primitives, and cache-soundness audit docs/system-contract.md states how the system is supposed to work, in response to #163: the xorq substrate from first principles, the tallyman primitives, the binding-time rule (names resolve exactly once, at build), the read/write/cache contracts, and invariants I1-I5. Written as the pre-grilling proposal: it describes the post-fix world, with current-main deviations and open questions collected at the end. plans/cache-soundness-audit.md inventories the same bug class beyond #163's axis: the project binding (W1/W2/W8), CSV ingest identity (W3), primary-key lineage (W4), the Buckaroo handoff (B1/B3), and Buckaroo 0.15.4's own caches. W1/W3/B3 verified directly in source. docs/architecture.md gains an index entry pointing at the contract. Refs #163, #166. Co-Authored-By: Claude Fable 5 --- docs/architecture.md | 4 + docs/system-contract.md | 658 +++++++++++++++++++++++++++++++++ plans/cache-soundness-audit.md | 254 +++++++++++++ 3 files changed, 916 insertions(+) create mode 100644 docs/system-contract.md create mode 100644 plans/cache-soundness-audit.md diff --git a/docs/architecture.md b/docs/architecture.md index f49bdce..4633506 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -369,6 +369,10 @@ when in doubt, the code wins. ### Architecture docs (`docs/`) — describe the current system +- [system-contract.md](system-contract.md) — **normative**: the invariants and + binding rules the system is supposed to guarantee (identity, read/write/cache + contracts). Where the descriptive docs and this contract disagree, the + difference is a bug (#163). **Current.** - [expression-lifecycle.md](expression-lifecycle.md) — one expression from MCP ingest to rendered rows, naming every artifact and cache write. **Current.** - [reactive-recalc.md](reactive-recalc.md) — revise an alias, recompute its diff --git a/docs/system-contract.md b/docs/system-contract.md new file mode 100644 index 0000000..84123eb --- /dev/null +++ b/docs/system-contract.md @@ -0,0 +1,658 @@ +# Tallyman: primitives and the system contract + +- **Status:** Proposed design (2026-07-30), written in response to #163. This + doc describes the system **as it will be** once the #163 fix lands. It is the + pre-grilling proposal: read it as a claim about how things should work, not a + description of `main`. The places where `main` currently disagrees are + collected in [Deviations on main today](#deviations-on-main-today), and the + genuinely unsettled points in [Open questions](#open-questions). +- **Audience:** no prior xorq knowledge assumed. The xorq section below covers + exactly as much of xorq as the rest of the doc needs, and no more. +- **Relation to other docs:** `architecture.md`, `caching.md`, and + `expression-lifecycle.md` describe the as-built system and will be corrected + when the fix lands. Until then, where they contradict this doc, the + contradiction is the bug. + +## What tallyman is, in three sentences + +Tallyman is a deconstructed notebook: instead of cells in a file, the unit of +work is a **catalog entry** — one dataframe computation, compiled and stored on +disk under a content hash, in a git-backed catalog. An LLM authors entries +through an MCP server; a web UI and the Buckaroo data grid render them. The +catalog is append-only and content-addressed: entries are immutable values, +names point at them, and history is never rewritten. + +--- + +# Part 1 — The xorq substrate + +Tallyman's compute layer is xorq, a deferred dataframe engine (an ibis fork +with its own serialization and caching). Five ideas from xorq carry everything +tallyman does. + +## 1. Deferred expressions + +An **expression** is an immutable graph of dataframe operations — read this +parquet file, filter, join, aggregate. Constructing it executes nothing. +Calling `.execute()` compiles the graph to a query plan and runs it on an +engine (an embedded DataFusion, by default), returning a dataframe. + +Because expressions are inert values, they can be walked, rewritten, hashed, +and serialized. Everything below exploits that. + +## 2. Backends, and the object-identity trap + +Every leaf of an expression (a file read, a table) is bound to a **backend** — +the connection object that will execute it. A backend is described by a +**profile**: the backend's name plus its connection kwargs. Two facts about +backends shape tallyman's design: + +- **One expression, one backend.** xorq refuses to execute a graph whose + leaves span more than one backend (`Multiple backends found`). "More than + one" is decided by **Python object identity**, not configuration: two + separately-created connections with byte-identical profiles still count as + two backends. +- **Profiles carry a serial-number field that is noise.** `Profile.idx` is a + process-global counter stamped on each connection. Two profiles are the + *same profile* for every purpose tallyman cares about iff they are equal + with `idx` removed. Any code that compares or dedups profiles must strip + `idx` first (xorq's own build path does this via `normalize_profiles`). + +The remedy for the identity trap is **`replace_sources`**: given a mapping +{old backend object → new backend object}, it rewrites an expression rebinding +every leaf and every cache reference onto the new object. This is the +sanctioned way to take expressions that arrived on different backend objects +and compose them onto one shared backend. + +**Rebinding is zero-copy.** A file read or cache node is a path plus a recipe +for reading it; swapping which connection executes it moves no data — the new +backend reads the same files at execute time. (Tallyman's backends are +stateless in-process engines; all data state lives in files.) The one node +type that *would* require a copy is a raw database table — rows living inside +the old connection's session rather than in a file — and `replace_sources` +refuses those unless passed an explicit transfer flag. Tallyman never passes +it: the build forbids the authoring patterns that create such nodes +(in-memory reads are a build error), so every leaf is a file read and the +refusal is a loud assertion that the build gate failed, never a live copy +path. (Whether xorq's build bundling can smuggle such a node in is +[open question 6](#open-questions).) + +## 3. Builds: freezing an expression to disk + +`build_expr(expr, builds_dir)` serializes an expression into a **build +directory**: + +``` +// + expr.yaml # the whole graph, self-contained + profiles.yaml # backend descriptions (content only — no live objects) + *metadata.json # library version, schema, etc. +``` + +Three properties matter: + +- **The build is closed.** `expr.yaml` contains the entire graph: every + upstream operation is written into the file (as an inline node or an + intra-file reference), functions are pickled inline, and the chain bottoms + out at concrete file reads. Loading a build needs nothing but the build + directory and the files its reads point at. There are no references to + anything outside the file except those read paths and the profiles. +- **The directory name is the expression's hash** — a 12-hex-char token over + the normalized graph. Two structurally identical expressions over the same + read paths produce the same hash, in any process (profile `idx` is + normalized out before hashing). +- **File reads are hashed by path string only.** The hash does not look at a + file's bytes, size, or mtime — just the path.[^mtime] This is deliberate + (build artifacts should be reproducible from what the expression *is*), and + it is the single most consequential xorq design choice for tallyman: **if + you want content identity, you must put the content in the path.** Tallyman + does (Part 2, CAS). + +`load_expr(build_dir, cache_dir=...)` is the inverse: it reads the yaml, +mints **fresh backend objects** from the profiles (nothing is memoized — two +loads of the same build yield two distinct backend objects, which is the +identity trap again), and rebases cache storage into `cache_dir` (next +section). The `cache_dir` parameter is the seam that lets the same build run +against any cache directory, including an empty one. + +## 4. Caching: snapshot keys, and existence as the only check + +Calling `.cache()` on an expression wraps it in a **cache node**. At execution +time xorq computes a **key** from the wrapped subgraph and looks for +`//.parquet`: + +- file exists → read it back; the subgraph does not run; +- file missing → execute the subgraph, write the file (atomically), read it. + +That is the entire mechanism. There is no invalidation step and no validation +of the file's contents: **a cache hit is decided by filename existence +alone**, and the stored bytes are trusted by assumption. Consequently the +cache is only as honest as two things: the key derivation, and whoever writes +the files. + +Tallyman uses xorq's **snapshot** keying everywhere: the key is a token over +the subgraph's structure and its read *paths* — again deliberately blind to +file content and mtime.[^mtime] Under CAS paths (Part 2) this blindness is +harmless: content is in the path, so the key is content-honest. The cache node survives +serialization: a build of a cached expression carries the cache node in its +yaml, so *any* loader of that build reads through the same cache key. What is +**not** serialized is the cache's base directory — every loader must supply +`cache_dir`, which is why the parameter exists. + +One xorq wart tallyman must own: xorq's load-time cache-dir rewrite walks the +graph shallowly and misses cache nodes nested *inside* another cache node's +subgraph. Tallyman's loader therefore does its own deep rewrite (over xorq's +deep-walking primitives) so that every cache node in a loaded build — nested +or not — lands in the project's cache directory, never in the global default +(`~/.cache/xorq`). + +## 5. What the hash sees, and what it cannot + +The expression hash covers structure and declared inputs: operations, schemas, +literals, pickled function bodies, read paths. It cannot see **execution +behavior**: a graph containing `sample()`, `now()`, an unordered `limit`, or +an impure UDF hashes identically every time while executing to different +bytes. The hash is an *input* identity, never an *output* identity. Tallyman +adds the output axis itself (Part 2, `result_digest`). + +[^mtime]: xorq also ships an mtime-keyed strategy: each source file's stat + (mtime/size/inode) folds into the cache key, so editing the file changes + the key and the next read recomputes. Tallyman doesn't use it because it + answers a different question. That cache is a memoization of the present: + the key follows the data (approximately — stat, not bytes), a miss *is* + the change signal, and the recompute silently replaces the old answer. It + keeps no record that anything changed and no stable name connecting old + answer to new — built to serve the current file's answer fast, nothing + more. Tallyman needs the opposite: entries are history, so identity must + hold still while data moves (that is what makes V3-vs-V4 meaningful), and + change is handled explicitly instead — the manifest records what the + world *was* (source digests, parent hashes), the staleness scan detects + by comparing that record against the live world, recalc mints new entries + and advances aliases, and old entries keep serving their old bytes + forever. A key whose job is to move when data moves has nothing to hang + that on. Bolting it under tallyman's caches would also collide with + identity mechanically: the expression hash is hardwired to path-only + normalization (measured in the source-identity ADR), so mtime-keyed + caches would move while entry hashes stood still — fresh bytes under an + old name, the #163 failure shape. CAS serves both needs with one + mechanism: content in the path gives the hash and every cache key an + identity that moves exactly when content moves, and the recorded digests + give staleness something durable to compare. (One stat use survives, as + an accelerator: the source-digest memo skips re-hashing files whose stat + is unchanged; content stays the truth.) + +--- + +# Part 2 — The tallyman primitives + +## Project, sources, and CAS + +A **project** is a directory (`~/.tallyman-notebooks/projects//`) holding +user data files (`data/`) and the catalog (`artifacts/catalog/`, a git repo). + +A **source** is a user-provided data file. Sources are mutable on disk — the +user can overwrite `trips.parquet` any time — so tallyman never lets an entry +depend on a live source path. At build time each source is: + +1. digested (md5, memoized on stat so unchanged files hash once), +2. cloned copy-on-write to `data/.cas/`, +3. read through the clone. + +This is **CAS** (content-addressed sources), and it is the answer to xorq's +path-only hashing: the path *is* the digest, so every xorq-level key — the +expression hash, every cache key — becomes content-honest for free. Edit a +source and rebuild: the digest changes, the path changes, the hash changes, a +new entry forks. The clone is the entry's immutable input forever; the live +file is merely where the *next* build will look. + +## Recipe + +A **recipe** (`expr.py`) is the LLM-authored Python that defines a +computation. It must bind a variable `expr`, built from: + +- `read_project_file("trips.parquet")` — read a source (through CAS); +- `tracked_expr_from_alias("trips")` — build on another entry, named by alias, + recording a `follow=True` parent edge: this expression depends on the parent + alias, and when that alias advances, recalc mints a new version of this + expression; +- `pinned_expr_from_alias()` — same, but `follow=False`: + recalc never touches this expression when the parent moves. Pins name an + exact version — a hash, or an explicit `"trips-v3"` version reference; a + bare alias is rejected as ambiguous (it would silently pin whatever the + head happened to be when the recipe was built — #166). + +The defining property of a recipe: **it binds by name.** "trips.parquet" +means whatever bytes sit there right now; "trips" means whatever entry that +alias points at right now. A recipe therefore has a different meaning at +different moments. That is exactly what you want when *authoring* — and +exactly what you must never consult again afterward. + +## Entry + +An **entry** is the immutable unit of the catalog: one computation, built +once, over inputs fixed forever. On disk: + +``` +entries// + expr.py # the recipe, verbatim (paths made portable) + xorq_build/ # the frozen build (Part 1 §3), paths made portable + manifest.json # the closure record (below) + schema.json +entries/.zip # git-tracked durable form, written at checkpoint +``` + +An entry carries **two representations of its computation, with different +authority**: + +| | `expr.py` (recipe) | `xorq_build/` (build) | +|---|---|---| +| binds inputs by | name (aliases, live paths) | value (parent graphs inlined, CAS paths) | +| meaning over time | drifts as names move | fixed forever | +| authoritative for | authoring: revise, display, the *next* build | semantics: every read, forever | + +**The authority rule: after build time, the build is the entry's semantics +and the recipe is documentation.** Re-executing a recipe on behalf of an +existing entry asks "what would this code produce *today*" when the question +is "what did this entry produce *then*." The recipe is re-executed in exactly +one situation: minting a **new** entry (create, revise, recalc), where +resolving names to current heads is the point. (#163 is what happens when +this rule is broken: reads re-ran recipes, so historical entries silently +re-bound to today's parents.) + +## Content hash + +An entry's `content_hash` is xorq's expression hash (Part 1 §3), taken at +build time. Two details determine what it covers: + +- the expression hashed is the **rewritten** one — cache injection (below) + has already happened — so an entry's caching shape is part of its identity; +- every file read in the graph points at a CAS clone, so each read path + carries a digest of the input bytes; and since parent expressions are + inlined into the graph, the same holds all the way up the lineage. + +The hash therefore names "this computation over these exact input bytes." +That one property makes builds idempotent and history append-only. Rebuild +the same computation over unchanged inputs and you land on the existing +entry: the build recognizes the hash and stops. Change anything that alters +the computation or its inputs — the recipe's logic, a source's bytes, a +parent's graph — and a new entry forks under a new hash, while every existing +entry keeps its name and its meaning. + +Two limits are deliberate. The hash cannot see execution behavior (Part 1 +§5): a recipe calling `sample()` or `now()` hashes identically run to run, +which is the gap `result_digest` (below) exists to police. And it makes no +attempt at cross-machine portability: absolute path prefixes participate in +the hash. + +## Manifest: the closure record + +`manifest.json` records everything about the build moment that the build +itself doesn't state, so that no later operation ever needs to resolve a name: + +| field | meaning | +|---|---| +| `content_hash` | the entry's identity | +| `parents` | `[{hash, ref, follow}]` — each alias reference, **resolved to the exact hash it meant at build time** | +| `sources` | `{rel_path: digest}` — each source, pinned to the bytes read | +| `result_digest` | SHA-256 of the baked result snapshot (worthy entries) — the output identity | +| `snapshot_key` | the baked snapshot's cache key, recorded at build (new; see write path) | +| `cache_worthy`, `cache_worthy_why`, `cache_bytes` | the worthiness verdict and its evidence | +| `row_count`, `execute_seconds`, `compile_seconds`, timings | build measurements | + +The manifest is written last, atomically: its presence is the "this entry is +complete" sentinel. An entry directory without one is treated as absent. + +## Alias + +An **alias** is a mutable name: `{alias, latest, history}` in a git-tracked +file. `latest` is the head; `history` is every hash it has pointed at (V1…Vn, +oldest first). Revising an alias mints a new entry, advances `latest`, appends +to `history`. Old entries remain, immutable, as the version history. + +**Where alias resolution is legal** — names resolve in exactly three +situations, all of them *about* choosing or minting, never about serving: + +1. **Minting** (build / revise / recalc): resolve heads, record the resolved + hashes in the new entry's manifest. +2. **Selecting** (UI, diff version arithmetic): resolve "by_hour" or "V-1" to + a hash, *then* serve that hash. +3. **Judging** (staleness scan): compare recorded parent hashes and source + digests against current heads and current files — read-only, executing + nothing. + +Once an entry is selected, serving it consults no name again. `follow` is a +**recalc** policy (should a parent's advance mint a new version of this +entry?), never a read policy: reads are lineage-faithful for followed and +pinned parents alike. + +## Worthiness: cheap and expensive entries + +At build time, tallyman rewrites the author's expression before freezing it +(**the worthiness rewrite**): + +- every non-parquet file read gets a cache node injected after the parse (so + a CSV is parsed once, shared by every entry reading it); +- if the expression contains an expensive operation — Aggregate, Join, Sort, + Window, or a UDF — the whole expression is wrapped in a top-level cache + node. Such an entry is **worthy**: its result is materialized once at build + (**the baked snapshot**, a parquet under the project's + `compute_cache/result_cache/`) and read back ever after. +- everything else is **cheap**: a projection/filter/rename over columnar + sources re-executes in pushdown time comparable to reading a copy, so no + copy is kept. + +The rewrite happens *before* hashing, so the cache nodes are part of the +entry's identity and travel inside `xorq_build/` — which is why every loader +of the build, in any process, reads through the same snapshot key. + +## Result digest + +For worthy entries, the build records `result_digest`: the SHA-256 of the +baked snapshot file, taken over a canonically-ordered materialization (so an +engine's parallel-scan row reshuffling cannot move it — see write path). It is +the **output** identity axis, and it has exactly one job: witnessing that a +later rematerialization reproduced the original bytes. It is never a +staleness input (an entry whose recompute differs is *nondeterministic*, not +stale — recomputing cannot make it fresh) and never part of the entry's name. + +It exists because recipes are LLM-authored: an LLM can write `sample()` or an +impure UDF, the hash cannot see it (Part 1 §5), and the digest is the runtime +backstop that catches it. + +--- + +# Part 3 — The lifecycle + +## The write path (build) + +`build_and_persist(project, code)`: + +1. **Import the recipe** — the single moment of name resolution. During the + import, `read_project_file` digests and clones each source (CAS) and + records `{rel_path: digest}`; `tracked_expr_from_alias` resolves each + alias to its current head, records the parent edge, and returns the + parent's expression (loaded from the parent's frozen build — see read + path). +2. **Rewrite** — the worthiness rewrite (cache injection). +3. **Freeze** — `build_expr` serializes the rewritten expression; + `content_hash` = the build's name. If an entry with this hash already + exists, stop: append the prompt, return the existing entry (idempotency). +4. **Lay down the entry** — copy the build in, make paths portable + (`${TALLYMAN_PROJECT_ROOT}` placeholders), write `expr.py`. +5. **Execute once** — load the just-written build against the project's + compute cache and run it. Worthy: the top cache node materializes the + baked snapshot (canonically ordered), and the build records its digest + **and its snapshot key** in the manifest. Cheap: one full streaming pass + (honest evaluation, fails fast, keeps nothing). +6. **Record** — schema, manifest (written last, atomic). +7. **Checkpoint** — when the MCP tool returns: recipe zip, tracked pointers, + one git commit. + +The build's obligation in one line: **record everything a reader will ever +need, because the reader is forbidden from resolving anything.** + +## The read path + +One canonical read, used by every consumer: + +``` +read(project, content_hash, cache_dir=): + expand the entry's xorq_build (placeholders → real paths, stable per-entry dir) + expr = load_expr(expanded, cache_dir=cache_dir) # + tallyman's deep cache-dir rewrite + return expr +``` + +- **Worthy entry:** the loaded graph carries its cache node, so execution + reads the baked snapshot by key. If the file is missing (evicted), the + cache node re-executes the *frozen* subgraph — whose leaves are CAS clones + and inlined parent graphs, all pinned — writes the snapshot, and the result + is verified against `result_digest` (below). Single-flighted per entry so + concurrent cold readers don't race. +- **Cheap entry:** execution re-runs the frozen graph in pushdown time, + reading CAS clones. Same bytes every time, by construction. +- The `cache_dir` parameter is the **cold seam**: any entry can be + materialized against an empty temp dir, and must produce the same bytes as + the warm read. That property is the standing regression test for every + read-path change. + +`cached_result_expr(project, hash)` remains as the function every in-process +consumer calls, but it is demoted to an optimization: it performs the +canonical read once per `(project, content_hash)` and keeps the loaded +expression in a bounded in-process LRU, so repeated reads (every pagination +request, both sides of a diff) skip the load. Removing it must change latency +and nothing else. That is what finally makes its key honest: the cached value +is a pure function of the frozen build the key names, where today the same +key stores a value that depends on live alias state. The per-call +snapshot-existence check and the single-flighted heal stay outside the LRU, +because file existence is the one input that remains mutable (eviction). + +**Chaining** (`tracked_expr_from_alias` at build time) uses the same read: the +parent's frozen build is loaded and its graph — including, for a worthy +parent, its cache node — is composed into the child. The child's build +therefore carries its own regeneration knowledge. If the parent's snapshot is +missing when the child executes, the parent's cache node re-runs its frozen +subgraph and rewrites the file, the same as any cache miss; nothing outside +the execution has to arrange for the file to exist.[^preheal] + +The recipe-reconstruction machinery survives only as a fallback (for an entry +whose build is missing or unloadable) and as a diagnostic — never as a path +for serving results while a build exists.[^recon] + +## Composition: diff and beyond + +To compose two entries (diff's outer join, a union, any multi-entry +expression): + +1. read each entry (canonical read above) — each load minted fresh backend + objects; +2. **rebind onto shared backends, one per distinct content profile** (profile + identity = profile minus `idx`), using `replace_sources`. For today's + catalogs every profile is the same embedded engine, so this collapses to + one shared backend; +3. compose. The result is a single-backend expression: it executes + in-process, and `build_expr` serializes it into a normal single-profile + build that Buckaroo's `/load_expr` accepts — the same handoff as an + ordinary entry grid. + +Composition of frozen builds was never the problem; backend object identity +was (#75, rediagnosed in #163). The rebind is cheap graph surgery, no data +moves. + +A **promoted diff** is just an entry whose recipe pins two hashes +(`build_diff_expr(a_hash, b_hash)`) — name-free, deterministic, and built +through the ordinary write path. + +## Staleness and recalc (unchanged, stated for completeness) + +**Staleness** is a read-only judgment: an entry is stale on the alias axis +when a `follow=True` parent's recorded hash no longer equals that alias's +head, and on the source axis when a recorded digest no longer matches the +live file's digest. Computing staleness executes nothing and mutates nothing. + +**Recalc** is the one sanctioned re-execution of recipes. When an alias head +advances (a revise), the entries that may be affected form its **cone**: +every entry reachable by walking `follow=True` parent edges backwards from +that alias — its followers, their followers, and so on. Pinned +(`follow=False`) edges are not in the cone. It runs automatically after a +revise when the project enables auto-recalc, or on demand. + +Recalc rebuilds the cone in topological order, parents before children. For +each member it re-imports the member's *recipe* — the one situation where +name resolution is the point, since the goal is a new version against the +new heads — builds the result as an ordinary new entry, and advances the +member's alias before any of its children replay, so each child chains off +its parent's fresh head. Old entries are untouched; every member gains a +version, none loses one. + +Two disciplines keep it predictable. **Scope:** recalc touches only followers +of the alias that moved; pre-existing staleness elsewhere is reported, not +swept up in the cascade. **Atomicity:** the head advance and its cascade land +as a single checkpoint, so one reset-to-revision undoes the whole event, and +the UI receives a single remap (`{old hash: new hash}` per member) to follow +open entries to their new versions. + +## Verification + +`verify_result_faithful(project, hash)` means exactly: *executing this +entry's frozen build reproduces its recorded `result_digest`.* It runs in +production, not only in tests: + +- on every self-heal (a healed snapshot is checked before it is served); +- on demand, corpus-wide, via the staleness-scan surface. + +A failure is surfaced loudly (event log / UI, not a perf logger), and its +attribution has three classes with three different fixes: + +| class | detector | meaning | response | +|---|---|---|---| +| structural (#88) | recipe re-derives a different hash, sources unchanged | author-time value baked into the graph (`pd.Timestamp.now()`) | lint; rewrite recipe | +| execution (#83) | fixed graph, digest moves across runs | `sample()`, `now()`, impure UDF | lint; pin entry to its snapshot, mark non-evictable | +| lineage (#163) | a read resolved a name post-build | machinery bug | impossible by construction under this contract | + +The third row is the point of the whole design: with reads going through the +build, lineage drift has no mechanism left. + +[^preheal]: This replaces a two-layer workaround on `main`. There, chaining + strips the parent's cache node down to a bare file read of its snapshot + (the #75 single-backend fix), so the child's build points at a parquet + without knowing the file is regenerable — evict the snapshot and the + child reads nothing. To compensate, the companion calls + `cached_result_expr` on an entry just before handing its build to + Buckaroo and discards the result: the call exists only for its side + effect, which is that reconstructing the entry walks its recipe chain and + forces any missing ancestor snapshots onto disk in time for Buckaroo's + replay. Under this design the stripping and the pre-heal call are both + deleted. + +[^recon]: The subsystem being demoted: `_recipe_expr` re-imports an entry's + `expr.py` to recover its expression, patching over the recipe's + name-binding with context variables for the duration of the import. + `_RECON_SOURCES` carries the entry's recorded `{path: digest}` map so + that `read_project_file`, when called inside a reconstruction, resolves + each source to its frozen `.cas/` clone instead of re-digesting + the live file (#115). #162 proposed a `_RECON_PARENTS` sibling to pin + parent aliases to their recorded hashes the same way, and + `_resolve_noncyclic_hash` walks a self-referencing alias back to its + previous version to break read-time cycles (#74). Each hook re-derives at + read time a binding the frozen build already contains, which is why the + fix retires them rather than adding a fourth. The diagnostic use stays: + `recipe_is_structurally_nondeterministic` re-derives the hash from the + recipe on purpose, because re-running the recipe is exactly how you + detect that a recipe fails to reproduce its own graph. + +--- + +# Part 4 — The invariants + +Everything above compresses to five statements. Any change that breaks one is +wrong even if every test passes. + +- **I1 — A content hash names a fixed result.** Materializing an entry yields + bytes consistent with its build — any consumer, any cache state, any time. + The only tolerated residual is execution nondeterminism *inside a recipe* + (#83), which the digest detects; no violation may originate in tallyman's + own machinery. +- **I2 — Caches affect latency, never results.** Every answer must be + byte-identical with all caches empty. Corollaries: cache keys derive only + from immutable inputs; cached values are reproducible from the frozen build + alone; every read path has a cold seam (`cache_dir`); a self-heal is + reproduce-and-verify, never manufacture. Result bytes are manufactured + exactly once, at build — a read path that writes bytes the build didn't + define has become a second, unaudited build path. +- **I3 — One read semantics.** Every consumer that materializes an entry + reads the frozen build through the one canonical read. The recipe is never + re-executed on behalf of an existing entry. +- **I4 — Names resolve exactly once.** At build, resolutions are recorded in + the manifest; after that the entry is closed. Aliases at read time select + *which* entry to serve, never *what* an entry means. (Grep-able form: + `get_alias`/`previous_version`/`version_of_hash` appear only in minting, + selecting, and judging code — never in materializing code.) +- **I5 — One question, one path.** Any question answerable two ways (grid vs + API bytes, manifest row count vs live count) either shares one canonical + path or carries an explicit check tying the two together; disagreement is + surfaced, never averaged over. + +--- + +# Deviations on main today + +As of `main` @ d7e0867 — the gap this proposal closes (all tracked in #163): + +- No in-process consumer loads the build; the only `load_expr` call in the + codebase is at build time (`build.py:496`). Every in-process read + (`cached_result_expr` and its ~14 consumers: diff, `/api/data`, + post-processing, primary-key inference, promote-diff, warmup) re-imports + `expr.py`; only Buckaroo's out-of-process grid reads the build. I3 broken. +- `tracked_expr_from_alias` resolves live heads inside reconstruction + (`io.py:686`), so historical entries materialize against today's parents; + the `(project, content_hash)` LRU asserts an immutability its value lacks. + I1/I4 broken. +- The self-heal manufactures: a wrong snapshot key heals by executing the + live-head expression and serving the result under the historical hash + (`result_cache.py:557`). I2 broken. +- No cold seam: `cached_result_expr` bakes an absolute snapshot path and + accepts no cache-dir override, so I2's regression test cannot be written. +- `verify_result_faithful` has no caller in `src/`, and locates the snapshot + through today's alias state. I4 (verification) unmet. +- The manifest records no snapshot key; build and read derive the snapshot + path independently through different routes, which is how they came to + disagree silently. +- The canonical-ordering claim behind `result_digest` is implemented only for + CSV ingest (`original_row_order` via `tallyman_read_csv`); `build.py`'s + comments claim a sort-before-bake that does not exist, so parquet-sourced + worthy entries can show digest drift from engine scan order alone — false + positives in the one audit signal I1 leans on. +- Worthy parents are chained by *stripping* their cache node to a bare + snapshot read (#75 workaround), which is why `ensure_session` pre-heals + parents before Buckaroo loads; under this proposal the cache node stays in + the child's graph and the pre-heal retires. +- `diff_stat_cache/` has no invalidation path in `src/`; its current contents + were computed over #163's self-join diffs. Wipe it when the fix lands. + +Existing catalogs are affected but rebuildable; per project policy there is no +migration path — rebuild the corpus. + +Beyond #163's axis, a full audit of the same bug class (mutable state behind +immutable keys — the project binding, CSV ingest identity, primary-key +lineage, and Buckaroo's own caches) is in +[`plans/cache-soundness-audit.md`](../plans/cache-soundness-audit.md). The +contract's rules apply to those axes too; the audit is the worklist. + +# Open questions + +For the grilling session; none block understanding the design, all block +calling it finished: + +1. **Canonical ordering for parquet-sourced bakes.** CSV ingest injects + `original_row_order`; parquet sources have no such column. Sort every bake + over all columns (cost?), inject an order column at read time, or scope + the digest's order-insensitivity claim to CSV-sourced entries? +2. **Nested cache-node rebase.** With parent cache nodes kept inline, builds + now contain nested cache nodes, and xorq's shallow load-time cache-dir + rewrite misses the inner ones. Proposal: a tallyman-side deep rewrite over + xorq's deep-walking primitives. Verify no path still relies on xorq's + shallow one. +3. **Child hashes change.** Keeping the parent's cache node in the child's + graph (rather than a stripped snapshot read) changes child content hashes + → corpus rebuild. Accepted per project policy; confirm nothing else keys + off the old chaining shape. +4. **Cheap entries have no digest.** Their faithfulness rests entirely on + construction (frozen graph over CAS clones). Is that argument airtight + enough to leave them unaudited, or should cheap entries record a digest at + build too (cost: one hashing pass they currently skip)? +5. **Fallback scope.** When exactly may the recipe-reconstruction fallback + fire (build dir missing? unloadable after an xorq upgrade?), and does it + warn loudly enough that it never silently becomes a load-bearing path + again? +6. **`replace_sources` edge.** It refuses raw `DatabaseTable` leaves (data + living inside a backend). Tallyman's build rejects in-memory reads, so + these shouldn't occur — but memtables serialized by xorq + (`memtables/*.parquet` in a build) load back as real tables. Confirm no + tallyman entry can produce one, or define the composition story if it can. +7. **Eviction policy for unfaithful entries.** When a heal fails verification + (execution nondeterminism), the entry's bytes are not regenerable. The + #83 position — pin to snapshot, mark non-evictable, steer the author — + needs wiring into the eviction/bullpen story. +8. **`diff_stat_cache` ownership.** Pair-keyed is honest once diff inputs are + lineage-faithful (I1), but the cache still needs an owner: size cap, + eviction, and a wipe on this fix's landing. diff --git a/plans/cache-soundness-audit.md b/plans/cache-soundness-audit.md new file mode 100644 index 0000000..0ed254d --- /dev/null +++ b/plans/cache-soundness-audit.md @@ -0,0 +1,254 @@ +# Cache-soundness audit: #163's bug class across tallyman and Buckaroo + +- **Status:** Research note (2026-07-30). Companion to `docs/system-contract.md` + (the proposed contract) — this is the inventory of where the codebase + currently violates it *beyond* the instances #163 already lists. +- **Template:** the #163 disease — a cache or code path whose key claims + immutability while its value depends on mutable state; read-time resolution + of a live name on behalf of a frozen artifact; self-heal that manufactures + data without verification; one question answered by two disagreeing paths. +- **Method:** two exploration passes (tallyman `src/`; buckaroo **0.15.4** + wheel in the tallyman venv, byte-identical to `~/buckaroo` @ `3313534a` — + note `~/code/buckaroo` has uncommitted server edits that tallyman does NOT + run). Findings marked **[verified]** were re-checked directly in source; + the rest are careful code reads with citations, not yet reproduced live. +- **Do not re-file #163's known set** (recipe re-import, live-head alias + resolution, `_resolve_result_plan`/`_build_compare_expr` LRUs, + `diff_stat_cache` contents, `_loaded_diff_sessions`). Everything below is + additional. + +## Tallyman — wrong-data findings + +### W1. Recipes bind to the *active project* at read time [verified] + +`read_project_file(rel_path, project=None)` → `resolve_project(project)` +(`io.py:59`); same in `tracked_expr_from_alias` (`io.py:680`) and +`pinned_expr_from_alias` (`io.py:718`); `build_diff_expr` — the recipe body of +every promoted diff entry — calls a bare `resolve_project()` +(`companion/diff.py:226`). `_recipe_expr` execs `expr.py` with no project +binding (`result_cache.py:184-236`); only `${TALLYMAN_PROJECT_ROOT}` string +substitution carries project identity, and a relative +`read_project_file("x.parquet")` has no placeholder. The #115 digest pin even +disables itself cross-project: `_reconstructing_source_digest` returns `None` +on project mismatch (`io.py:630-647`) and falls through to the live path. + +**Predicate:** any read where `resolve_project() != ` — two +browser tabs on different projects, a tab left open across a switch, any MCP +call after an external switch (see W8). Same relative path exists in the +active project → the *other project's rows* served under this project's URL; +else `ProjectDataNotFound`. Reachable from `/api/data`, the `ensure_session` +pre-heal, `/api/diff_data`, and every MCP reconstruction consumer. + +This is #163's shape on a second axis: alias heads were one mutable name in +the recipe; the project is another. The contract's I4 ("names resolve exactly +once") must cover both — and does, once reads load builds, because the build's +expanded paths are absolute. The *fallback* recipe path keeps this hole. + +### W2. `_resolve_result_plan` memoizes the active project into its value + +Key `(project, content_hash)` (`result_cache.py:440`), but the `"recompute"` +branch stores an expression built under whatever project was active at fill +time (W1). Neither switch path clears it: `api_projects_switch` +(`app.py:1790-1808`) and MCP `project_switch` (`server.py:1561-1565`) clear +nothing. Fill `(A, H)` while B is active, switch back to A: every read of +`(A, H)` serves B-flavored results for the process lifetime (the recompute +branch has no exists-check to trip a heal). + +### W3. `tallyman_read_csv` sits entirely outside source identity [verified] + +- Intermediate keyed `md5(abs_path | schema | scan_kwargs)` — content-blind by + documented intent (`io.py:515-527`); refreshed by an mtime sidecar via + **in-place `os.replace` of the same path** (`io.py:575-578`). +- Never calls `si.note_source` (only `read_project_file` does, `io.py:76,81`) + → `manifest.sources = None` for every CSV-rooted entry. + +Three consequences: **(a)** edit the CSV, resubmit the same recipe → same +`Read` path → same `content_hash` → the build dedups to the stale entry +(`build.py:447`), exactly the `test_append_invalidates` failure `cas` was +built to close — and `salt` can't rescue it (`build.py:441` gates on +`and sources`, which is empty). **(b)** after re-materialization, every +historical entry rooted in that CSV reads the new intermediate — retroactive +mutation under fixed hashes, unverified (W6). **(c)** staleness is blind: +`sources_of` → `None` → the source axis lands in `unknown_axes` +(`staleness.py:96-98`), so scans, recalc, and the UI report CSV-rooted entries +permanently clean. CSV is the *mandated* ingest path (raw `deferred_read_csv` +is a build error), and no test covers a CSV content change. + +Also the root of a real cross-project hash collision: the intermediate lives +in `TALLYMAN_HOME/csv_ordered/` (project-independent by design, +`io.py:499-512`), so the same recipe over the same CSV in two projects builds +the **identical content hash** — which arms B1 below. + +### W4. Primary-key inheritance resolves lineage by alphabetical accident + +`_parent_hash` calls `previous_version(project, hash)` with no alias hint +(`primary_key.py:61-69`); `version_of_hash`'s fallback scans aliases in dict +order, and the file is written sorted, so a hash in ≥2 alias histories +inherits from the **alphabetically-first** alias's lineage (`aliases.py:101-117`). +The inherited key is validated only by column-name membership, never +uniqueness (`primary_key.py:98`). A non-unique inherited key becomes the diff +join key → outer-join fan-out and mislabeled add/delete/change rows. +Multi-history hashes are ordinary (`catalog_alias` points new aliases at +existing hashes; `catalog_rename` reorders the scan). `primary_key.json` is +gitignored and restored verbatim from the bullpen, so `reset_to` cannot roll +it back. + +### W5. The MCP process never hears any invalidation + +`reset_to` clears the plan LRU in its own process; the companion clears via +`/internal/notify`; the MCP server only POSTs *out* — nothing posts in. A +long-lived MCP process that memoized a cheap entry's plan keeps serving the +old alias resolution after any browser/CLI reset. (Under the contract this +memo becomes pure and the issue reduces to existence-staleness, but the +process-boundary gap is worth naming in the fix.) + +### W6. Digest checked only inside the heal branch; delete leaves stat caches + +The self-heal compares against `result_digest` only when *this process* healed +(`result_cache.py:547-569`); a snapshot already on disk, or healed by a peer, +is never checked. `verify_result_faithful` has no caller in `src/`. +`DELETE /api/result_cache/{hash}` unlinks the snapshot but clears neither +`.buckaroo_stat_cache` nor the live Buckaroo session (`app.py:1509-1536`) — +after a heal to different bytes, the grid's stats describe the pre-delete +bytes (see Buckaroo #1: nothing on the Buckaroo side will notice either). + +### W7. Row counts are split-brained + +`api_data`'s `total` and `api_entry_detail`'s `total_rows` come from the +manifest; the rows come from a live execute (`app.py:884-893`, `:968`); +`schema_diff`'s before/after counts are build-time while diff stats are live +(`diff.py:114-129`). Any W1/W3/W6 drift shows header N over grid M. + +### W8. The project-change guardrail is dead code + +`_tag_project` seeds `_mcp_active_project` on first call and thereafter +compares a constant to itself (`server.py:104-122`), so the design-decision-14 +"active project changed under you" warning can never fire — the exact +condition that arms W1 goes unreported. + +## Tallyman — secondary findings + +- **B1. Buckaroo session map is cross-project on a false premise.** + `_sessions` keys by content hash only, justified "bytes identical by + construction" (`buckaroo_lifecycle.py:528-531`) — W3 breaks the premise via + `csv_ordered`. When two projects share H: B's grid uses A's session, A's + stat-cache dir, A's telemetry; `reload_project_sessions(B)` filters on the + *recorded* project and skips H, so B's klass changes never reload B's grid. +- **B3. Buckaroo never finds project klasses [verified].** Tallyman passes + `project_root = artifacts_dir(project)` (`buckaroo_lifecycle.py:598`); + Buckaroo scans `/stats/*.py` and returns silently on a missing + dir (`xorq_loading.py:154-155` in the wheel); tallyman writes stats to + `catalog_dir/stats` = `artifacts/catalog/stats` (`paths.py:245-252`). The + `/reload_expr` POST carries no body to compensate. Project-authored summary + stats and grid post-processing klasses appear to be invisible to the grid + always. (The post-processing *tab* uses tallyman's own server-side runner, + which is why the feature can look alive. Needs one end-to-end check: + `catalog_add_summary_stat` → does the stat render in the grid?) +- **B5.** `ensure_expanded_build` trusts the `.complete` marker without + checking the directory exists (`portable.py:105-107`); `ENTRY_CACHE_NAMES` + names the dir but not the sibling marker, so cache-filtering copiers can + preserve the marker and drop the dir → unhealable empty grid. +- **B4.** `BuckarooManager._expanded_dirs` is never written — dead + bookkeeping (`buckaroo_lifecycle.py:147`). +- **R1.** Display klasses live outside the catalog repo and the tracked + surface but their MCP tools checkpoint — `reset_to` claims to undo a step + that still renders. Also written non-atomically. +- **R2.** `_force_source_rehash` mutates `os.environ` process-wide from + threadpool routes (`staleness.py:62-72`); two overlapping scans can drop + the rehash flag mid-loop and pass off a same-stat in-place edit as fresh. +- **R3.** `source_digests.json` read-modify-write is non-atomic and + two-process (companion + MCP); a torn file degrades to full rehash (perf + only). +- **R4.** `.cas` `.tmp` leftovers are never GC'd (`gc_cas` matches on stem). +- **Reset survivors:** across every reset path, these carry stale state by + construction — MCP-process LRUs (W5), `primary_key.json` (W4), + `.buckaroo_stat_cache` and `.xorq_build_expanded` (restored verbatim from + bullpen), `diff_stat_cache/`, `csv_ordered/`, `/tmp/tallyman_diff_builds`. + Project **switch** invalidates nothing at all, in either process. + +## Buckaroo (0.15.4) — "is buckaroo doing something stupid with caching?" + +Yes — the worst instance is #163 verbatim, and it sits directly under +tallyman's grid. + +1. **Summary-stats snapshot cache is path-keyed over mutable bytes.** + Buckaroo caches per-column stats in a xorq `ParquetSnapshotCache` + (`xorq_loading.py:43-45`) whose key is the stat query's *expression token* + — structure + leaf path strings, no content, no mtime; xorq documents the + strategy as "source data changes do not invalidate cached results." The + lookup is filename-existence (`xorq_stat_pipeline.py:197-209`), and the + local docstring's claim that the key is "content-addressed on `query`" is + false. **Predicate:** same build dir + same stat-cache dir loaded twice + with any reachable parquet's bytes changed at a stable path between loads + — exactly what a tallyman snapshot self-heal produces (stable expanded + build path, stable `entry_stat_cache_dir`) — serves the previous dataset's + min/max/nunique/histograms beside the new rows, counted as a cache *hit*. + Under the tallyman contract this is masked while I1 holds upstream (bytes + never change under a hash) but is armed the moment any heal drifts: + tallyman's failure containment ends at the `/load_expr` boundary. +2. **The JS row cache's data-identity signal is never armed.** Client row + caches partition on `outside_df_params`, whose one data-identity field, + `dataframe_id`, is passed by no caller in server mode + (`BuckarooView.tsx:287-299`). On `/reload_expr` (which tallyman issues on + every klass hot-reload) the server resets session state to defaults and + rebroadcasts, the recomputed params are byte-identical, no purge fires, + and `SmartRowCache` serves the previous expression's rows from memory + under new stats. One prop (a server load-generation counter) closes it. +3. **`/reload_expr` silently drops config.** The rebuilt dataflow loses + `cache_storage_path`, `column_config_overrides`, `extra_grid_config` + (`handlers.py:807-808`) — after any reload, stat caching is off and the + caller's column config is gone. Tallyman reloads on every stat/klass + change. +4. **`_expr_count_cache`** (`xorq_buckaroo.py:44-70`): a WeakKeyDictionary + keyed by immutable *expressions* caching data-dependent *counts*; while a + session holds the expr the entry never dies, so a byte change under a + stable path serves stale `total_rows`/scroll bounds. Same key-vs-value + confusion as #1 in miniature. +5. **Warm-session short-circuit compares a path string** (`handlers.py:451-470`): + same `session` + same `build_dir` path with rewritten contents → old + expression served. Dormant for tallyman (it never reuses session ids) but + it is the same disease server-side. +6. **Pandas-path caches** (not on tallyman's xorq path): the series-stat LRU + stashes a hash on the Series object and never rehashes after in-place + mutation, and equality is a wraparound *sum* of row hashes (permutations + collide) (`utils.py:85-123`); the file-metadata cache validates by + `current_mtime <= cached_mtime` (same-mtime swap and backwards-mtime both + pass); failed column hashes all merge into one row keyed `0` + (`paf_column_executor.py:225-231`). +7. **Sound, notably:** the `(id(df), id(klasses))` dedup — the previous + object is still strongly referenced while the new one is constructed, so + id-reuse is unreachable; unsound only under in-place mutation, which the + xorq path can't do. + +## Reading the pattern + +Every wrong-data finding above is one of three shapes: (1) a key that omits a +real dependency (path-keyed stats over mutable bytes; content-blind CSV keys; +hash-only session maps crossing projects), (2) a name resolved after binding +time (active project in recipes; alphabetical alias lineage; live heads — +#163 itself), or (3) a regenerated artifact served unverified (self-heal, +`recon_cas_path`'s live fallback). The contract in `docs/system-contract.md` +(I1–I5) eliminates shape 2 and 3 for the alias axis by construction; this +audit shows the same rules must also be applied to the **project** binding +(W1/W2/W8), the **CSV ingest** identity (W3), and the **Buckaroo handoff** +(whose caches assume I1 holds upstream — fix #1/#2 there or wipe stat caches +whenever a heal writes). + +## Triage order + +1. **W3** — put CSV content into the identity (digest into `_ordered_csv_key`, + `note_source` from `tallyman_read_csv`): fixes stale-dedup, retroactive + mutation, staleness blindness, and disarms B1's cross-project collision. +2. **W1/W2/W8** — thread `project` explicitly into recipe execution (or a + contextvar pinned for the exec), clear plan LRUs on project switch, revive + the change warning. Fold into the #163 read-path fix, which removes the + primary consumer of the broken binding. +3. **B3** — one end-to-end check, then point `project_root` at + `catalog_dir(project)`. +4. **Buckaroo #1/#2/#3** — fold a content signal into the stat-cache key (or + have tallyman wipe stat caches on any heal-write), pass a load-generation + `dataframe_id`, persist config across `/reload_expr`. +5. **W4** — pass the alias hint into `_parent_hash`; validate inherited keys + for uniqueness. +6. **W6** — verify digests on read (not just own-process heal); make the + result-cache DELETE also clear stat caches + sessions. From 9234daf0133ae779bd3b150ad5703be800e39322 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 17:14:25 -0400 Subject: [PATCH 2/6] test(163): failing lineage-faithfulness tests encoding the contract invariants Five tests over the #163 shape (two revisions of one recipe, distinct only by recorded parent) asserting system-contract invariants: a cheap historical read matches its recorded row_count (I1/I4), two worthy revisions resolve distinct snapshots (I1), an untouched entry verifies faithful (I4), a diff of two revisions is not a self-join (I5), and a cold-cache read reproduces build bytes (I2). EXPECTED TO FAIL on this branch: all five fail on main (assert 37 == 200; identical snapshot paths; verify False; all deltas 0.0; heal manufactures head bytes). Per TDD they must be seen failing on CI before the fix lands. The cold-cache failure also demonstrates the taxonomy gap: the heal logs the lineage bug as execution nondeterminism (#83). Refs #163. Co-Authored-By: Claude Fable 5 --- tests/test_lineage_faithful_reads.py | 161 +++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 tests/test_lineage_faithful_reads.py diff --git a/tests/test_lineage_faithful_reads.py b/tests/test_lineage_faithful_reads.py new file mode 100644 index 0000000..2b43426 --- /dev/null +++ b/tests/test_lineage_faithful_reads.py @@ -0,0 +1,161 @@ +"""Lineage-faithful reads (#163): a content hash names a fixed result. + +These tests encode the invariants of ``docs/system-contract.md`` against the +#163 failure shape — two revisions of one recipe that are distinct entries +only because their recorded parent moved — and are EXPECTED TO FAIL on +current main (TDD: they must be seen failing on CI before the fix lands). + +The shape mirrors the taxi2 ``by_hour`` case from the issue: + + trips v1 = full orders (a1) + by_region v1 = child recipe (b1, parents=[{trips, a1}]) + trips v2 = boots-only rows (a2) revise → auto-recalc mints + by_region v2 = same recipe bytes (b2, parents=[{trips, a2}]) + +After the cascade the ``trips`` head is a2, so b1 is a historical entry whose +recorded parent no longer sits at its alias head. Each test asserts a form of +contract invariant I1: materializing b1 yields b1's bytes — regardless of +where the alias head is now (I4), which consumer asks (I3/I5), or what state +the caches are in (I2). +""" + +from __future__ import annotations + +import shutil + +from tallyman_companion.diff import build_diff_expr +from tallyman_core.aliases import get_alias +from tallyman_core.manifest import read_manifest +from tallyman_core.paths import compute_cache_dir, entry_dir +from tallyman_mcp.server import catalog_create, catalog_revise +from tallyman_xorq.result_cache import ( + baked_snapshot_path, + cached_result_expr, + verify_result_faithful, +) + +# --------------------------------------------------------------------------- +# recipe helpers (mirror test_auto_recalc.py) +# --------------------------------------------------------------------------- + + +def _parent_v1_code(project: str) -> str: # full orders + return f""" +from tallyman_xorq.io import read_project_file +t = read_project_file("orders.parquet", project={project!r}) +expr = t.select("region", "category", "price") +""" + + +def _parent_v2_code(project: str) -> str: # same source, boots only — fewer rows + return f""" +from tallyman_xorq.io import read_project_file +t = read_project_file("orders.parquet", project={project!r}) +expr = t.filter(t.category == "boots").select("region", "category", "price") +""" + + +def _cheap_child_code() -> str: # row-preserving → no snapshot, recomputes on read + return """ +from tallyman_xorq.io import tracked_expr_from_alias +t = tracked_expr_from_alias("trips") +expr = t.mutate(doubled=t.price * 2) +""" + + +def _worthy_child_code() -> str: # Aggregate → bakes a result snapshot at build + return """ +from tallyman_xorq.io import tracked_expr_from_alias +t = tracked_expr_from_alias("trips") +expr = t.group_by("region").aggregate(n=t.count(), total=t.price.sum()) +""" + + +def _lineage(project: str, monkeypatch, child_code: str) -> tuple[str, str]: + """Build the #163 shape; return (child_v1_hash, child_v2_hash).""" + monkeypatch.delenv("TALLYMAN_AUTO_RECALC", raising=False) # default-ON cascade + monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") + a1 = catalog_create("trips", _parent_v1_code(project))["hash"] + b1 = catalog_create("by_region", child_code)["hash"] + catalog_revise("trips", _parent_v2_code(project)) + b2 = get_alias(project, "by_region") + # Sanity on the setup itself (not the invariant under test): the cascade + # minted a distinct child version off the new head, and b1's binding to a1 + # is durably recorded. + assert b2 is not None and b2 != b1 + assert get_alias(project, "trips") != a1 + m1 = read_manifest(entry_dir(project, b1)) + assert m1.parents and m1.parents[0].hash == a1 + return b1, b2 + + +# --------------------------------------------------------------------------- +# the invariant tests — each expected to fail on current main +# --------------------------------------------------------------------------- + + +def test_cheap_historical_read_uses_recorded_parent(project, orders_parquet, monkeypatch): + """I1/I4: a cheap entry rematerializes from its frozen lineage, not the live head. + + b1 was built over the full parent (recorded row_count). On main, reading b1 + re-imports its recipe and ``tracked_expr_from_alias`` resolves ``trips`` to + the *current* head, so the count comes back boots-only. + """ + b1, _b2 = _lineage(project, monkeypatch, _cheap_child_code()) + recorded_rows = read_manifest(entry_dir(project, b1)).row_count + live_rows = int(cached_result_expr(project, b1).count().execute()) + assert live_rows == recorded_rows + + +def test_worthy_revisions_resolve_distinct_snapshots(project, orders_parquet, monkeypatch): + """I1: two distinct entries must never resolve to one snapshot file. + + On main both revisions' recipes reconstruct against the same live head, so + both derive the same snapshot key — the "both reconstruct onto the same + file" collapse from #163. + """ + b1, b2 = _lineage(project, monkeypatch, _worthy_child_code()) + p1 = baked_snapshot_path(project, b1) + p2 = baked_snapshot_path(project, b2) + assert p1 is not None and p2 is not None + assert p1 != p2 + + +def test_untouched_historical_entry_verifies_faithful(project, orders_parquet, monkeypatch): + """I4-verification: an entry nobody touched must reproduce its recorded digest. + + ``verify_result_faithful`` must mean "this entry's build reproduces its + result_digest". On main it locates the snapshot through today's alias + state, so an ordinary parent advance makes every non-head revision report + unfaithful (or resolve to a sibling's snapshot). + """ + b1, _b2 = _lineage(project, monkeypatch, _worthy_child_code()) + assert verify_result_faithful(project, b1) is True + + +def test_diff_of_two_revisions_is_not_a_self_join(project, orders_parquet, monkeypatch): + """I5: the diff consumer must see the two revisions' real, distinct results. + + v2's parent filters to boots, so per-region counts genuinely differ. On + main both diff sides reconstruct against the same head and the outer join + compares a result to itself — every delta exactly zero (#162's symptom). + """ + b1, b2 = _lineage(project, monkeypatch, _worthy_child_code()) + df = build_diff_expr(b1, b2, keys=["region"]).execute() + deltas = df["n_abs_delta"].fillna(0) + assert (deltas != 0).any() + + +def test_cold_cache_read_reproduces_build_bytes(project, orders_parquet, monkeypatch): + """I2: wiping every cache may change latency only — never the bytes served. + + With the compute cache emptied, reading b1 self-heals its snapshot. The + heal must re-execute b1's *frozen* computation and land bytes matching the + recorded digest. On main it executes the live-head reconstruction and + manufactures today's bytes under b1's historical hash. + """ + b1, _b2 = _lineage(project, monkeypatch, _worthy_child_code()) + shutil.rmtree(compute_cache_dir(project), ignore_errors=True) + cached_result_expr.cache_clear() + cached_result_expr(project, b1) # cold read — forces the self-heal + assert verify_result_faithful(project, b1) is True From 222de710af0d17fd135c9c81f99ddadfdd34a321 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 19:39:16 -0400 Subject: [PATCH 3/6] docs(163): ADR for the read-path decisions; sync contract; link filed issues plans/adr-read-path-loads-builds.md records the eight decisions from the grilling session over the system contract: D1 one PR + one rebuild; D2 keep the cached_result_expr/baked_snapshot_path facade, replace internals with the canonical read; D3 rebind composition onto the default backend with a one-content-profile assertion; D4 chaining inlines parent cache nodes (pre-heal retired, deep cache-dir rewrite, corpus rebuild); D5 digest order-insensitivity scoped to CSV (#171 tracks the parquet residual); D6 missing/unloadable build is a hard error, no recipe fallback; D7 verify failures to errors.jsonl + SSE plus a scan sweep flag; D8 manifest records the snapshot key, reads assert it. docs/system-contract.md amended to match (fallback clause, open questions 1/3/5 resolved). plans/cache-soundness-audit.md findings linked to their filed issues: #168 (CSV identity), #169 (PK lineage), #170 (klass scan path), #172 (cross-project sessions), buckaroo#955/#956/#957. Co-Authored-By: Claude Fable 5 --- docs/system-contract.md | 31 ++--- plans/adr-read-path-loads-builds.md | 172 ++++++++++++++++++++++++++++ plans/cache-soundness-audit.md | 16 +-- 3 files changed, 197 insertions(+), 22 deletions(-) create mode 100644 plans/adr-read-path-loads-builds.md diff --git a/docs/system-contract.md b/docs/system-contract.md index 84123eb..f84c02c 100644 --- a/docs/system-contract.md +++ b/docs/system-contract.md @@ -434,9 +434,11 @@ missing when the child executes, the parent's cache node re-runs its frozen subgraph and rewrites the file, the same as any cache miss; nothing outside the execution has to arrange for the file to exist.[^preheal] -The recipe-reconstruction machinery survives only as a fallback (for an entry -whose build is missing or unloadable) and as a diagnostic — never as a path -for serving results while a build exists.[^recon] +The recipe-reconstruction machinery survives only as a diagnostic. An entry +whose build is missing or unloadable is a **hard error** naming the entry and +the remedy (rebuild) — there is no automatic recipe fallback, because a +warning on a background read is exactly how #163-class behavior stays +invisible (decided in `plans/adr-read-path-loads-builds.md`, D6).[^recon] ## Composition: diff and beyond @@ -623,27 +625,26 @@ contract's rules apply to those axes too; the audit is the worklist. For the grilling session; none block understanding the design, all block calling it finished: -1. **Canonical ordering for parquet-sourced bakes.** CSV ingest injects - `original_row_order`; parquet sources have no such column. Sort every bake - over all columns (cost?), inject an order column at read time, or scope - the digest's order-insensitivity claim to CSV-sourced entries? +1. **Canonical ordering for parquet-sourced bakes.** *Resolved (ADR D5):* + the order-insensitivity claim is scoped to CSV-sourced entries; parquet + digests are documented order-sensitive with advisory drift warnings, a + tracked issue, and provocation tests. Sort-all-columns and multiset + digests remain the fallback options if the advisory fires in practice. 2. **Nested cache-node rebase.** With parent cache nodes kept inline, builds now contain nested cache nodes, and xorq's shallow load-time cache-dir rewrite misses the inner ones. Proposal: a tallyman-side deep rewrite over xorq's deep-walking primitives. Verify no path still relies on xorq's shallow one. -3. **Child hashes change.** Keeping the parent's cache node in the child's - graph (rather than a stripped snapshot read) changes child content hashes - → corpus rebuild. Accepted per project policy; confirm nothing else keys - off the old chaining shape. +3. **Child hashes change.** *Resolved (ADR D1/D4):* accepted — the read-path + fix, chaining inline, and digest re-scope land in one PR followed by one + corpus rebuild. 4. **Cheap entries have no digest.** Their faithfulness rests entirely on construction (frozen graph over CAS clones). Is that argument airtight enough to leave them unaudited, or should cheap entries record a digest at build too (cost: one hashing pass they currently skip)? -5. **Fallback scope.** When exactly may the recipe-reconstruction fallback - fire (build dir missing? unloadable after an xorq upgrade?), and does it - warn loudly enough that it never silently becomes a load-bearing path - again? +5. **Fallback scope.** *Resolved (ADR D6):* there is no fallback. A missing + or unloadable build is a hard error; recipe re-execution survives only in + minting and the structural-nondeterminism diagnostic. 6. **`replace_sources` edge.** It refuses raw `DatabaseTable` leaves (data living inside a backend). Tallyman's build rejects in-memory reads, so these shouldn't occur — but memtables serialized by xorq diff --git a/plans/adr-read-path-loads-builds.md b/plans/adr-read-path-loads-builds.md new file mode 100644 index 0000000..1047f50 --- /dev/null +++ b/plans/adr-read-path-loads-builds.md @@ -0,0 +1,172 @@ +# ADR: Reads load the frozen build — the #163 read-path decisions + +- **Status:** Accepted (2026-07-30) — decided in the grilling session over + `docs/system-contract.md`. Supersedes the read-path behavior introduced by + #74/#75 (recipe re-import on read). Four follow-up questions remain open, + listed at the end. +- **Context ticket:** buckaroo-data/tallyman#163 (read path reconstructs from + `expr.py`; a content hash does not name a fixed result). Normative design: + `docs/system-contract.md`. Bug-class survey: + `plans/cache-soundness-audit.md`. +- **Affected code:** `src/tallyman_xorq/result_cache.py` (canonical read, + facade internals, verify), `src/tallyman_xorq/io.py` + (`tracked_expr_from_alias` / `pinned_expr_from_alias` chaining), + `src/tallyman_xorq/portable.py` (deep cache-dir rewrite), + `src/tallyman_core/manifest.py` (`snapshot_key` field), + `src/tallyman_xorq/staleness.py` (verify sweep), + `src/tallyman_companion/app.py` + `diff.py` (composition rebind), + `src/tallyman_companion/buckaroo_lifecycle.py` (pre-heal removal). +- **Related ADRs:** `plans/adr-source-identity-content-hash.md` (CAS — why + content lives in the path), `plans/adr-result-digest-canonical-ordering.md` + (the digest whose ordering claim D5 re-scopes). +- **Evidence:** #163's verification on the `taxi2` corpus (frozen builds + execute to distinct correct results; composition after rebinding works; + warm and cold runs byte-identical), and + `tests/test_lineage_faithful_reads.py` — five invariant tests red on CI + (PR #167) against `main`. + +## Problem + +An entry has two representations: the recipe (`expr.py`, binds by name) and +the frozen build (`xorq_build/`, binds by value). On `main`, every in-process +read re-executes the recipe, so alias names re-resolve to their live heads at +read time and a historical entry silently materializes today's data under its +recorded hash. The full statement of the intended semantics is +`docs/system-contract.md`; this ADR records the implementation decisions for +restoring them. + +## Decisions + +### D1. One PR, one rebuild + +The read-path fix, the chaining change (D4), and the digest re-scope (D5) +land together on PR #167, flipping the five red tests green in a single +change, followed by one corpus rebuild (chaining changes child content +hashes). *Rejected:* staged landing (read-path first, hash-changing PRs +after) — smaller diffs and easier bisection, but two rebuild events and an +interim state where builds are read but not yet self-contained. + +### D2. Keep the facade; replace the internals + +`cached_result_expr(project, hash)` and `baked_snapshot_path(project, hash)` +keep their names and signatures; their internals become the canonical read — +`ensure_expanded_build` → `load_expr(expanded, cache_dir=…)` → tallyman's +deep cache-dir rewrite — with the snapshot path derived from the *loaded* +expression (the same derivation the build uses). The `cache_dir` cold seam +lives on a lower-level read function the facade wraps. The in-process LRU +stays, now sound: its `(project, content_hash)` key finally determines its +value. *Rejected:* a new `read_entry()` API with ~14 call-site migrations — +cleaner name, same semantics, larger diff for no behavioral gain. + +### D3. Rebind composition onto the default backend, with a one-group guard + +Composing loaded builds (diff, chaining) collapses backends via +`replace_sources` onto xorq's process default backend. The contract's +"one backend per distinct content profile" is enforced as an assertion: +group profiles by `tokenize(profile minus idx)`; if more than one distinct +group ever appears, fail loudly rather than misbind. *Rejected:* a fresh con +per composition (setup cost, no sharing) and a full per-profile registry +(machinery with exactly one occupant — every profile in every catalog today +is content-identical `xorq_datafusion`). + +### D4. Chaining inlines the parent's cache node + +`tracked_expr_from_alias` composes the parent's loaded build — cache node +included — rebound onto the default backend. Child builds become +self-contained and self-healing: a cold read regenerates ancestor snapshots +through ordinary cache mechanics. The `ensure_session` pre-heal (a +discarded-result call whose only purpose was forcing ancestor snapshots onto +disk before Buckaroo's replay) is deleted, along with the #75 stripping. +Cost: builds now contain *nested* cache nodes, which xorq's shallow +load-time cache-dir rewrite cannot reach — tallyman ships a deep rewrite +over xorq's `walk_nodes`/`replace_nodes` (the traversal that descends into +`CachedNode.parent`-style opaque fields). Child hashes change; the corpus +rebuilds. *Rejected:* keep stripping — no nesting and no rebuild, but builds +stay non-self-contained and the pre-heal choreography stays load-bearing +forever. + +### D5. Digest order-insensitivity is scoped to CSV-sourced entries + +`result_digest` keeps its file-hash definition. CSV-sourced entries keep the +canonical `original_row_order` sort (the existing ADR). For parquet-sourced +worthy entries the order-insensitivity claim is **withdrawn**, not fixed: +their digests are documented as order-sensitive, heal-drift warnings on them +are advisory, and #171 plus provocation tests (attempt to trigger DataFusion +parallel-scan reordering; demonstrate either stability or the advisory +firing) capture the residual. *Rejected for now:* sort-by-all-columns +before the bake (deterministic bytes everywhere, but a sort on every +bake/heal and pinned-null-ordering complexity) and a multiset digest (order +insensitivity by definition, but verify must read every row and the digest +stops being a hash of the artifact). Either remains available if the +advisory fires in practice. + +### D6. A missing or unloadable build is a hard error + +Serving results for an entry whose `xorq_build/` is absent or fails to load +raises, naming the entry and the remedy (rebuild). There is no automatic +recipe fallback — not even env-gated. Recipe re-execution survives in +exactly two places: minting (build/revise/recalc) and the +structural-nondeterminism diagnostic. This amends the contract's original +"fallback, loudly" clause: a warning on a background read is how #163-class +behavior stays invisible, and the fallback still carried the audit's W1 +project-binding bug. + +### D7. Verification runs in production and is loud + +`verify_result_faithful` re-anchors to "executing this entry's frozen build +reproduces its recorded digest." Wiring: every self-heal verifies before the +result is served, with failures written durably to `errors.jsonl` and pushed +over SSE so the UI can badge the entry; `catalog_scan_staleness` gains an +opt-in `verify_results=True` corpus sweep. *Rejected:* verify-on-every-read +(a full file hash on the hottest path, guarding against drift only heals can +introduce) and log-only surfacing (invisible by construction). + +### D8. The manifest records the snapshot key; reads assert it + +The build writes the baked snapshot's cache key into the manifest; the +canonical read asserts its own derivation matches and raises on mismatch. +With build and read now sharing one derivation route the original +disagreement mechanism is gone; the recorded key is a tripwire that turns +any future divergence (an xorq tokenization change under an upgrade, a +rewrite drift) into an immediate, attributable failure instead of a silent +wrong-file read. + +## Consequences + +- **Retired:** read-time recipe re-import and its pins (`_RECON_SOURCES` + stays only for the diagnostic path), the #74 cycle-walk's read-time role, + #162's proposed `_RECON_PARENTS` (never built), the #75 stripping, the + `ensure_session` pre-heal. +- **Corpus rebuild** required at landing (D4 changes child hashes); per + project policy there is no migration path. `diff_stat_cache/` is wiped and + Buckaroo bounced at the same time (its current contents were computed over + #163's self-join diffs). +- **Buckaroo's caches become sound again for CSV-sourced entries:** with + verify enforced, a faithful heal is byte-identical, so expression-keyed + stat caches over stable paths are honest. For parquet-sourced entries a + heal may legitimately reorder rows (D5): aggregate summary stats are + order-insensitive and stay correct, but an open session's row cache can + show the new order — accepted until the D5 issue is resolved. +- **The audit's W5 (MCP-process invalidation) largely dissolves:** cached + values become pure functions of immutable builds, so cross-process + staleness reduces to the existence checks the read already performs. +- `docs/system-contract.md` is amended where these decisions sharpen it + (fallback clause → hard error; open questions 1, 3, and 5 resolved). + `docs/caching.md` / `expression-lifecycle.md` / `architecture.md` get + corrected when the fix lands, as planned. + +## Open (deliberately not decided here) + +1. **Cheap-entry digests** — record an output digest for cheap entries, or + rely on the by-construction argument (frozen graph over CAS clones)? +2. **Audit issue filing** — which of W3 (CSV identity), W4 (PK lineage), + B3 (klass scan path), B1 (cross-project session map) get GitHub issues + now, and which fold into others. +3. **Buckaroo upstream** — filing the three buckaroo-repo issues (stats + cache key, `dataframe_id` never passed, `/reload_expr` config loss) and + whether the tallyman fix PR carries the interim mitigation (wipe an + entry's stat cache and evict its session when a heal fails verification). +4. **Second red-test commit** — extend PR #167's failing suite (hard error + on missing build, cold chained read heals without pre-heal, D5 + provocation tests) before the fix commit, or add them as regression + guards after. diff --git a/plans/cache-soundness-audit.md b/plans/cache-soundness-audit.md index 0ed254d..d688196 100644 --- a/plans/cache-soundness-audit.md +++ b/plans/cache-soundness-audit.md @@ -54,7 +54,7 @@ nothing. Fill `(A, H)` while B is active, switch back to A: every read of `(A, H)` serves B-flavored results for the process lifetime (the recompute branch has no exists-check to trip a heal). -### W3. `tallyman_read_csv` sits entirely outside source identity [verified] +### W3. `tallyman_read_csv` sits entirely outside source identity [verified] — filed #168 - Intermediate keyed `md5(abs_path | schema | scan_kwargs)` — content-blind by documented intent (`io.py:515-527`); refreshed by an mtime sidecar via @@ -79,7 +79,7 @@ in `TALLYMAN_HOME/csv_ordered/` (project-independent by design, `io.py:499-512`), so the same recipe over the same CSV in two projects builds the **identical content hash** — which arms B1 below. -### W4. Primary-key inheritance resolves lineage by alphabetical accident +### W4. Primary-key inheritance resolves lineage by alphabetical accident — filed #169 `_parent_hash` calls `previous_version(project, hash)` with no alias hint (`primary_key.py:61-69`); `version_of_hash`'s fallback scans aliases in dict @@ -128,13 +128,13 @@ condition that arms W1 goes unreported. ## Tallyman — secondary findings -- **B1. Buckaroo session map is cross-project on a false premise.** +- **B1 (filed #172). Buckaroo session map is cross-project on a false premise.** `_sessions` keys by content hash only, justified "bytes identical by construction" (`buckaroo_lifecycle.py:528-531`) — W3 breaks the premise via `csv_ordered`. When two projects share H: B's grid uses A's session, A's stat-cache dir, A's telemetry; `reload_project_sessions(B)` filters on the *recorded* project and skips H, so B's klass changes never reload B's grid. -- **B3. Buckaroo never finds project klasses [verified].** Tallyman passes +- **B3 (filed #170). Buckaroo never finds project klasses [verified].** Tallyman passes `project_root = artifacts_dir(project)` (`buckaroo_lifecycle.py:598`); Buckaroo scans `/stats/*.py` and returns silently on a missing dir (`xorq_loading.py:154-155` in the wheel); tallyman writes stats to @@ -171,7 +171,8 @@ condition that arms W1 goes unreported. Yes — the worst instance is #163 verbatim, and it sits directly under tallyman's grid. -1. **Summary-stats snapshot cache is path-keyed over mutable bytes.** +1. **Summary-stats snapshot cache is path-keyed over mutable bytes** + (filed buckaroo#955)**.** Buckaroo caches per-column stats in a xorq `ParquetSnapshotCache` (`xorq_loading.py:43-45`) whose key is the stat query's *expression token* — structure + leaf path strings, no content, no mtime; xorq documents the @@ -186,7 +187,8 @@ tallyman's grid. Under the tallyman contract this is masked while I1 holds upstream (bytes never change under a hash) but is armed the moment any heal drifts: tallyman's failure containment ends at the `/load_expr` boundary. -2. **The JS row cache's data-identity signal is never armed.** Client row +2. **The JS row cache's data-identity signal is never armed** + (filed buckaroo#956)**.** Client row caches partition on `outside_df_params`, whose one data-identity field, `dataframe_id`, is passed by no caller in server mode (`BuckarooView.tsx:287-299`). On `/reload_expr` (which tallyman issues on @@ -194,7 +196,7 @@ tallyman's grid. rebroadcasts, the recomputed params are byte-identical, no purge fires, and `SmartRowCache` serves the previous expression's rows from memory under new stats. One prop (a server load-generation counter) closes it. -3. **`/reload_expr` silently drops config.** The rebuilt dataflow loses +3. **`/reload_expr` silently drops config** (filed buckaroo#957)**.** The rebuilt dataflow loses `cache_storage_path`, `column_config_overrides`, `extra_grid_config` (`handlers.py:807-808`) — after any reload, stat caching is off and the caller's column config is gone. Tallyman reloads on every stat/klass From 6bdc97c58d290107c5630a91f28c4b9969b7ead8 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 19:49:51 -0400 Subject: [PATCH 4/6] =?UTF-8?q?test(163):=20second=20red=20wave=20?= =?UTF-8?q?=E2=80=94=20D4/D6/D10=20behaviors=20from=20the=20ADR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more failing-first tests: a missing xorq_build/ must raise naming the entry (D6 — main silently reconstructs, DID NOT RAISE); a cold load of a chained child's build must heal the worthy parent's snapshot with no pre-heal (D4 — main dies on the bare read: 'At least one path is required'); an unfaithful heal must wipe the entry's .buckaroo_stat_cache (D10 — main only logs, sentinel survives). All three seen failing locally for these exact reasons; 8 red + probe green. Refs #163, plans/adr-read-path-loads-builds.md. Co-Authored-By: Claude Fable 5 --- tests/test_lineage_faithful_reads.py | 99 +++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-) diff --git a/tests/test_lineage_faithful_reads.py b/tests/test_lineage_faithful_reads.py index 2b43426..bf489b6 100644 --- a/tests/test_lineage_faithful_reads.py +++ b/tests/test_lineage_faithful_reads.py @@ -21,13 +21,25 @@ from __future__ import annotations +import json import shutil +import pytest +from xorq.ibis_yaml.compiler import load_expr + from tallyman_companion.diff import build_diff_expr from tallyman_core.aliases import get_alias from tallyman_core.manifest import read_manifest -from tallyman_core.paths import compute_cache_dir, entry_dir +from tallyman_core.paths import ( + compute_cache_dir, + entry_build_dir, + entry_dir, + entry_expanded_build_dir, + entry_stat_cache_dir, + project_dir, +) from tallyman_mcp.server import catalog_create, catalog_revise +from tallyman_xorq.portable import ensure_expanded_build from tallyman_xorq.result_cache import ( baked_snapshot_path, cached_result_expr, @@ -159,3 +171,88 @@ def test_cold_cache_read_reproduces_build_bytes(project, orders_parquet, monkeyp cached_result_expr.cache_clear() cached_result_expr(project, b1) # cold read — forces the self-heal assert verify_result_faithful(project, b1) is True + + +# --------------------------------------------------------------------------- +# second wave — behaviors decided in plans/adr-read-path-loads-builds.md +# (D4, D6, D10); also expected to fail on current main +# --------------------------------------------------------------------------- + + +def _worthy_parent_code(project: str) -> str: # Aggregate at the parent → worthy parent + return f""" +from tallyman_xorq.io import read_project_file +t = read_project_file("orders.parquet", project={project!r}) +expr = t.group_by("region").aggregate(n=t.count(), total=t.price.sum()) +""" + + +def test_missing_build_is_hard_error(project, orders_parquet, monkeypatch): + """ADR D6: an entry whose xorq_build/ is gone must raise, not reconstruct. + + On main the read path never touches the build — it re-imports expr.py — + so deleting xorq_build/ goes unnoticed and the read silently succeeds. + The error must name the entry so the remedy (rebuild) is actionable. + """ + monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") + h = catalog_create("plain", _parent_v1_code(project))["hash"] + shutil.rmtree(entry_build_dir(project, h)) + cached_result_expr.cache_clear() + with pytest.raises(Exception, match=h[:12]): + cached_result_expr(project, h) + + +def test_chained_child_build_heals_parent_without_preheal(project, orders_parquet, monkeypatch): + """ADR D4: a child's build is self-contained — a cold load heals ancestors. + + Simulates exactly what Buckaroo does (expand the child's build, load it, + execute) with every snapshot evicted and NO pre-heal call. On main the + child's build holds a bare read of the worthy parent's snapshot — a file + reference with no regeneration knowledge — so the cold load reads a + missing file. Under the contract the parent's cache node travels in the + child's build and heals through ordinary cache mechanics. + """ + monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") + catalog_create("trips", _worthy_parent_code(project)) + child_code = """ +from tallyman_xorq.io import tracked_expr_from_alias +t = tracked_expr_from_alias("trips") +expr = t.mutate(doubled=t.total * 2) +""" + child = catalog_create("by_region", child_code)["hash"] + recorded = read_manifest(entry_dir(project, child)).row_count + shutil.rmtree(compute_cache_dir(project), ignore_errors=True) + cached_result_expr.cache_clear() + expanded = ensure_expanded_build( + entry_build_dir(project, child), + project_dir(project), + entry_expanded_build_dir(project, child), + ) + loaded = load_expr(expanded, cache_dir=compute_cache_dir(project)) + assert int(loaded.count().execute()) == recorded + + +def test_unfaithful_heal_wipes_stat_cache(project, orders_parquet, monkeypatch): + """ADR D10: a heal that fails verification wipes the entry's stat cache. + + Buckaroo's summary-stats cache is keyed on expression structure and paths + (buckaroo#955), so bytes that change under a stable path leave stale stats + rendering beside fresh rows. A verified-faithful heal is byte-identical + and needs no wipe; an UNFAITHFUL one must clear the entry's + .buckaroo_stat_cache. The recorded digest is tampered here so the heal is + unfaithful by construction. On main the mismatch only logs a warning. + """ + monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") + h = catalog_create("by_region", _worthy_parent_code(project))["hash"] + mpath = entry_dir(project, h) / "manifest.json" + doc = json.loads(mpath.read_text()) + doc["result_digest"] = "0" * 64 + mpath.write_text(json.dumps(doc)) + stat_dir = entry_stat_cache_dir(project, h) / "parquet" + stat_dir.mkdir(parents=True, exist_ok=True) + sentinel = stat_dir / "stale-stats.parquet" + sentinel.write_text("stale") + shutil.rmtree(compute_cache_dir(project), ignore_errors=True) + cached_result_expr.cache_clear() + cached_result_expr(project, h) # heal fires; digest cannot match the tamper + assert not sentinel.exists() From 1e7e5f6ec92c86b7bfdd4fa47e05fe6c08f9bf37 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 19:49:59 -0400 Subject: [PATCH 5/6] test(163): #171 digest-order probe; ADR round-3 decisions D9-D12 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe builds a 250k-group aggregate over a >1MiB parquet source and heals it three times: locally it observes THREE distinct snapshot digests, none reproducing the build digest, with the advisory firing each time. #171's drift is not a tail risk — at scale it fires on every heal. The probe pins the mechanism (verify agrees with bytes; mismatch surfaces the advisory) and passes on main; the empirical result argues for revisiting ADR D5, since D10/D12 would false-positive on every heal of large parquet-sourced entries. ADR gains the round-3 decisions: D9 no cheap-entry digests; D10 unfaithful heal wipes Buckaroo state; D11 second red-test commit; D12 unfaithful entries pinned + badged. Contract open questions 4/7/8 marked resolved. Refs #163, #171. Co-Authored-By: Claude Fable 5 --- docs/system-contract.md | 19 +++--- plans/adr-read-path-loads-builds.md | 60 ++++++++++++++----- tests/test_parquet_digest_order_probe.py | 75 ++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 tests/test_parquet_digest_order_probe.py diff --git a/docs/system-contract.md b/docs/system-contract.md index f84c02c..9199c46 100644 --- a/docs/system-contract.md +++ b/docs/system-contract.md @@ -638,10 +638,9 @@ calling it finished: 3. **Child hashes change.** *Resolved (ADR D1/D4):* accepted — the read-path fix, chaining inline, and digest re-scope land in one PR followed by one corpus rebuild. -4. **Cheap entries have no digest.** Their faithfulness rests entirely on - construction (frozen graph over CAS clones). Is that argument airtight - enough to leave them unaudited, or should cheap entries record a digest at - build too (cost: one hashing pass they currently skip)? +4. **Cheap entries have no digest.** *Resolved (ADR D9):* they stay + digest-free — with reads loading builds, their bytes are deterministic by + construction, so the audit would have nothing to catch. 5. **Fallback scope.** *Resolved (ADR D6):* there is no fallback. A missing or unloadable build is a hard error; recipe re-execution survives only in minting and the structural-nondeterminism diagnostic. @@ -650,10 +649,8 @@ calling it finished: these shouldn't occur — but memtables serialized by xorq (`memtables/*.parquet` in a build) load back as real tables. Confirm no tallyman entry can produce one, or define the composition story if it can. -7. **Eviction policy for unfaithful entries.** When a heal fails verification - (execution nondeterminism), the entry's bytes are not regenerable. The - #83 position — pin to snapshot, mark non-evictable, steer the author — - needs wiring into the eviction/bullpen story. -8. **`diff_stat_cache` ownership.** Pair-keyed is honest once diff inputs are - lineage-faithful (I1), but the cache still needs an owner: size cap, - eviction, and a wipe on this fix's landing. +7. **Eviction policy for unfaithful entries.** *Resolved (ADR D12):* pin + non-evictable, badge via the error record; full bullpen wiring deferred. +8. **`diff_stat_cache` ownership.** *Resolved (ADR post-acceptance):* wiped + at the fix's landing and wired into reset invalidation; size caps + deferred. diff --git a/plans/adr-read-path-loads-builds.md b/plans/adr-read-path-loads-builds.md index 1047f50..dc2858b 100644 --- a/plans/adr-read-path-loads-builds.md +++ b/plans/adr-read-path-loads-builds.md @@ -155,18 +155,48 @@ wrong-file read. `docs/caching.md` / `expression-lifecycle.md` / `architecture.md` get corrected when the fix lands, as planned. -## Open (deliberately not decided here) - -1. **Cheap-entry digests** — record an output digest for cheap entries, or - rely on the by-construction argument (frozen graph over CAS clones)? -2. **Audit issue filing** — which of W3 (CSV identity), W4 (PK lineage), - B3 (klass scan path), B1 (cross-project session map) get GitHub issues - now, and which fold into others. -3. **Buckaroo upstream** — filing the three buckaroo-repo issues (stats - cache key, `dataframe_id` never passed, `/reload_expr` config loss) and - whether the tallyman fix PR carries the interim mitigation (wipe an - entry's stat cache and evict its session when a heal fails verification). -4. **Second red-test commit** — extend PR #167's failing suite (hard error - on missing build, cold chained read heals without pre-heal, D5 - provocation tests) before the fix commit, or add them as regression - guards after. +## Post-acceptance decisions (grilling round 3, 2026-07-30) + +The four items originally left open here were resolved in the final round: + +### D9. No cheap-entry digests + +Cheap entries record no `result_digest`. With reads loading builds, a cheap +entry's bytes are the deterministic pushdown of a frozen graph over +content-pinned clones — no snapshot to drift, no second manufacture point — +so a digest would cost a canonical materialization at build and at every +verify to audit a hole that no longer exists. Resolves the contract's open +question 4. + +### D10. An unfaithful heal wipes the entry's Buckaroo state + +When a heal fails verification, the fix PR wipes that entry's +`.buckaroo_stat_cache` and evicts its Buckaroo session, so stale stats never +render beside fresh rows while buckaroo#955/#956 await a wheel bump. +Faithful heals need no wipe: enforced verification makes them +byte-identical, which is exactly the assumption Buckaroo's caches key on. + +### D11. Second red-test commit before the fix + +PR #167's failing suite is extended with the newly-decided behaviors before +the fix commit: hard error on a missing build (D6), a cold load of a +chained child's build healing the parent snapshot with no pre-heal (D4), +and the D10 wipe. The #171 provocation probes ride in a separate commit — +they may pass on `main` (whether the reorder fires at test scale is the +empirical question they exist to answer), so per the failing-tests +discipline they don't belong in the red commit. Tests that cannot fail +first (the D8 snapshot-key assert needs the new manifest field) ride with +the fix commit. + +### D12. Unfaithful entries are pinned and badged + +An entry whose heal fails verification has bytes that are not regenerable: +it is marked non-evictable (its snapshot is retained, not treated as +reclaimable cache) and surfaced with a UI badge via the D7 error record — +the #83 position. Full bullpen/eviction wiring is deferred; the marking and +badge land with the fix. Resolves the contract's open question 7. + +Two defaults adopted without ceremony: `diff_stat_cache/` joins +`_invalidate_reset_caches` and the reset prune (size caps deferred), +resolving the contract's open question 8; and #166 (`pinned_expr_from_alias` +version references) lands as its own PR after the fix. diff --git a/tests/test_parquet_digest_order_probe.py b/tests/test_parquet_digest_order_probe.py new file mode 100644 index 0000000..c16feab --- /dev/null +++ b/tests/test_parquet_digest_order_probe.py @@ -0,0 +1,75 @@ +"""#171 probe: can DataFusion's parallel scan/aggregation reorder a +parquet-sourced bake, and does the advisory fire correctly when it does? + +Deliberately NOT a red test. Whether the reorder fires at this scale is the +empirical question the probe exists to answer — the source is written above +``repartition_file_min_size`` (1 MiB; files below it are never byte-range +split, which is why small fixtures look deterministic — see +``plans/datafusion-scan-order-findings.md``) and the aggregate has one group +per ``order_id`` so the hash aggregation has room to vary. The assertions pin +the MECHANISM, not the outcome: ``verify_result_faithful`` must agree with +the byte comparison, and any mismatch must surface the advisory warning +rather than pass silently (ADR D5, ``plans/adr-read-path-loads-builds.md``). +""" + +from __future__ import annotations + +import logging + +from tallyman_cli.fixtures import write_shoe_orders +from tallyman_core import data_dir +from tallyman_core.manifest import read_manifest +from tallyman_core.paths import entry_dir +from tallyman_mcp.server import catalog_create +from tallyman_xorq.result_cache import ( + baked_snapshot_path, + cached_result_expr, + snapshot_file_digest, + verify_result_faithful, +) + +_HEALS = 3 + + +def _big_agg_code(project: str) -> str: + return f""" +from tallyman_xorq.io import read_project_file +t = read_project_file("orders_big.parquet", project={project!r}) +expr = t.group_by("order_id").aggregate(total=t.price.sum(), n=t.count()) +""" + + +def test_parquet_bake_digest_stability_probe(project, monkeypatch, caplog): + monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") + src = write_shoe_orders(data_dir(project) / "orders_big.parquet", n_rows=250_000, seed=7) + # Below repartition_file_min_size the file is never split and the probe + # proves nothing — fail loudly rather than pass vacuously. + assert src.stat().st_size > 1_048_576, "fixture too small to provoke a partitioned scan" + + h = catalog_create("big_by_order", _big_agg_code(project))["hash"] + recorded = read_manifest(entry_dir(project, h)).result_digest + assert recorded is not None + + digests: set[str] = set() + for _ in range(_HEALS): + snap = baked_snapshot_path(project, h) + if snap is not None and snap.exists(): + snap.unlink() + cached_result_expr.cache_clear() + with caplog.at_level(logging.WARNING, logger="tallyman.perf"): + cached_result_expr(project, h) # heal fires + healed = baked_snapshot_path(project, h) + assert healed is not None and healed.exists() + d = snapshot_file_digest(healed) + digests.add(d) + # Mechanism consistency: the audit must agree with the bytes. + assert verify_result_faithful(project, h) is (d == recorded) + if d != recorded: + # Drift occurred: it must be surfaced, not silent. + assert any("UNFAITHFUL" in r.getMessage() for r in caplog.records) + + # Document the empirical outcome in the test output either way. + print( + f"digest-order probe: {len(digests)} distinct digest(s) over {_HEALS} heals; " + f"build digest reproduced: {recorded in digests}" + ) From ae584848400e31fba27e4917496426c101932e1e Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 30 Jul 2026 22:08:54 -0400 Subject: [PATCH 6/6] =?UTF-8?q?fix(163):=20reads=20load=20the=20frozen=20b?= =?UTF-8?q?uild=20=E2=80=94=20canonical=20read,=20self-contained=20chainin?= =?UTF-8?q?g,=20canonical-order=20bake?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements plans/adr-read-path-loads-builds.md (D1-D12, incl. the D5 amendment) and flips the eight red lineage-faithfulness tests green. Read path (D2/D6): cached_result_expr / baked_snapshot_path internals become the canonical read — ensure_expanded_build -> load_expr(cache_dir=compute cache) -> portable.rewrite_cache_dirs, a deep cache-dir rewrite over xorq's replace_nodes that reaches CachedNodes nested inside CachedNode.parent, which xorq's own shallow rewrite misses. A missing or unloadable build is a hard error naming the entry; recipe re-execution survives only at minting and in the structural-nondeterminism diagnostic. Chaining (D3/D4): tracked/pinned_expr_from_alias return entry_graph_expr — the parent's loaded build, cache node included, rebound onto the process default backend via replace_sources with a one-content-profile guard. Child builds are self-contained and self-healing, so the ensure_session pre-heal and the #75 stripping are deleted. Child manifests still record ancestors' source digests (io._note_parent_sources) so gc_cas keeps a child's CAS leaves alive independently of its parent entry. Canonical-order bake (D5, amended): rewrite_for_build sorts a worthy expression by all sortable columns before the result-cache wrap (author sort keys stay primary, then original_row_order, then schema order; constant projections skipped — datafusion rejects ORDER BY over a bare literal). The #171 probe that showed 3 distinct digests over 3 heals is now a regression test pinning byte-equality. Verification (D7/D8/D10/D12): every self-heal verifies the healed bytes against result_digest before serving; a mismatch records a durable unfaithful_heal error (the pin marking + UI badge source), wipes the entry's .buckaroo_stat_cache, and fires hooks the companion registers for session eviction + an SSE badge event. The build records snapshot_key in the manifest and the read asserts its own derivation matches. catalog_scan_staleness gains verify_results=True (staleness.verify_sweep); _invalidate_reset_caches now also wipes diff_stat_cache/. Test updates riding with the fix (behavior changes, cannot fail-first): the pre-heal unit test now pins the self-contained-build replacement at the same seam; the structural-drift-on-read test pins its inversion (a baked-literal entry reads faithfully from its frozen build); the cross-process heal stub uses the new _ResultPlan shape. Requires a corpus rebuild at landing (chaining and the sort change content hashes) per the project's no-migration policy. Closes #163. Closes #171. Co-Authored-By: Claude Fable 5 --- docs/architecture.md | 6 +- docs/caching.md | 110 ++--- docs/expression-lifecycle.md | 66 +-- docs/system-contract.md | 38 +- plans/adr-read-path-loads-builds.md | 76 ++-- src/tallyman_companion/app.py | 52 ++- src/tallyman_companion/buckaroo_lifecycle.py | 39 +- src/tallyman_core/manifest.py | 8 + src/tallyman_mcp/server.py | 15 +- src/tallyman_xorq/build.py | 17 +- src/tallyman_xorq/io.py | 49 ++- src/tallyman_xorq/portable.py | 31 ++ src/tallyman_xorq/result_cache.py | 421 ++++++++++++++----- src/tallyman_xorq/source_cache.py | 90 +++- src/tallyman_xorq/staleness.py | 31 ++ tests/test_buckaroo.py | 53 ++- tests/test_parquet_digest_order_probe.py | 45 +- tests/test_result_cache.py | 29 +- tests/test_serve.py | 2 +- 19 files changed, 829 insertions(+), 349 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 4633506..b13d3a8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -370,9 +370,9 @@ when in doubt, the code wins. ### Architecture docs (`docs/`) — describe the current system - [system-contract.md](system-contract.md) — **normative**: the invariants and - binding rules the system is supposed to guarantee (identity, read/write/cache - contracts). Where the descriptive docs and this contract disagree, the - difference is a bug (#163). **Current.** + binding rules the system guarantees (identity, read/write/cache contracts). + Where the descriptive docs and this contract disagree, the difference is a + bug. The read path implements it as of the #163 fix (PR #167). **Current.** - [expression-lifecycle.md](expression-lifecycle.md) — one expression from MCP ingest to rendered rows, naming every artifact and cache write. **Current.** - [reactive-recalc.md](reactive-recalc.md) — revise an alias, recompute its diff --git a/docs/caching.md b/docs/caching.md index dc5f7a8..5ccb749 100644 --- a/docs/caching.md +++ b/docs/caching.md @@ -103,10 +103,13 @@ path), so there is no separate `result_cache/` directory; both land under exempt (`_EXEMPT_READS`): re-reading a columnar source is already a pushdown. - **Baked result snapshot** — an expression is *worthy* when it contains an Aggregate / Join / Sort / window / UDF (`_EXPENSIVE_OPS`, - `result_cache.py:49`). A worthy expression is wrapped in a result cache with - `relative_path="result_cache"` (`source_cache.py:160`), so its snapshot lands - under `compute_cache/result_cache/`; executing the entry at build time - materializes it once (`build.py:491`). A non-parquet read is *not* by itself + `result_cache.py:49`). A worthy expression is first put in a canonical total + order (`_canonical_sorted`: the author's own `order_by` keys, then + `original_row_order`, then the remaining sortable columns — ADR D5, amended) + so the baked bytes are deterministic across the build and every later heal, + then wrapped in a result cache with `relative_path="result_cache"`, so its + snapshot lands under `compute_cache/result_cache/`; executing the entry at + build time materializes it once. A non-parquet read is *not* by itself worthy (`classify_build`, `result_cache.py:62`) — its parse is already handled by the source-read cache. Worthiness is decided by two predicates that must agree: `_is_worthy_expr` gates the bake (`source_cache.py:62`) and @@ -125,24 +128,33 @@ written for any entry, cheap or expensive, at build time or on demand (#73 and follow-up). Every consumer reads an entry's result through one function, -`cached_result_expr` (`result_cache.py:403`): +`cached_result_expr`, whose internals are the canonical read of +`docs/system-contract.md` (#163): expand the entry's frozen `xorq_build/`, +`load_expr(expanded, cache_dir=compute_cache)`, then the deep cache-dir +rewrite (`portable.rewrite_cache_dirs`). A missing or unloadable build is a +hard error naming the entry — reads never re-import `expr.py` (ADR D6): - Expensive entry → `deferred_read_parquet` of the baked snapshot, on the default backend. The snapshot was written when the entry built, so reading it - skips re-running the graph. If it was evicted since, the cache node recomputes - it once and the read proceeds — a self-heal re-checked on every call, - single-flighted per `(project, content_hash)` so concurrent cold readers don't - both run the shared cache op; after a repopulate the snapshot's digest is - checked against the recorded `result_digest` and a mismatch is logged as - recompute drift (`result_cache.py:451-473`, `_heal_lock`, - `_warn_if_self_heal_unfaithful`; #79, #83). -- Cheap entry → the live expression, re-imported from the entry's `expr.py` - and recomputed on read. + skips re-running the graph; the read asserts its derived snapshot key matches + the one the manifest recorded (ADR D8). If the file was evicted since, the + *frozen build* is executed once and the read proceeds — a self-heal + re-checked on every call, single-flighted per `(project, content_hash)` so + concurrent cold readers don't both run the shared op; the healed bytes are + verified against the recorded `result_digest` before they are served, and a + mismatch wipes the entry's Buckaroo stat cache, records a durable + `unfaithful_heal` error, and evicts its Buckaroo session (`_verify_self_heal`; + ADR D7/D10/D12, #79, #83). +- Cheap entry → the loaded build's graph, rebound onto the default backend and + recomputed on read from its content-pinned sources. Either shape is a single-backend expression, so two entries compose (`union`, -`join`, a diff) without tripping xorq's "multiple backends" guard (#75). The -viewer's paginated reads, diffs, and post-processing all go through it; nothing -reads a pre-existing `result.parquet`, because none is written. +`join`, a diff) without tripping xorq's "multiple backends" guard. Chaining +(`tracked_expr_from_alias`) uses the sibling `entry_graph_expr` instead: the +parent's cache node stays in the graph, so a child's build is self-contained +and self-healing (ADR D4). The viewer's paginated reads, diffs, and +post-processing all go through `cached_result_expr`; nothing reads a +pre-existing `result.parquet`, because none is written. Snapshot strategy is the right one here because an entry is an immutable, content-addressed artifact — its result must not invalidate just because an @@ -188,17 +200,17 @@ re-added entry still computes honestly cold. `reset_to` also reclaims orphaned All bounded LRUs over immutable keys, so eviction means a cheap rebuild and staleness is impossible: -- `cached_result_expr` — its reconstruction work is memoized on - `_resolve_result_plan`, `lru_cache(256)` keyed `(project, content_hash)` - (`result_cache.py:345`); `cached_result_expr` re-exposes that memo's - `cache_clear` / `cache_info`. The memo saves re-importing the entry's - `expr.py` and, for an expensive entry, re-deriving its baked-snapshot path. - `cached_result_expr` itself is a thin wrapper that re-checks the snapshot's - on-disk presence on every call, so an evicted snapshot self-heals; the heal - runs under a per-entry lock (`_heal_lock`, `result_cache.py:393`) so only the - first of several concurrent cold readers executes it, and a peer process that - wins xorq's fixed-`.tmp` rename is caught and retried once if the snapshot - landed, else re-raised (#79). +- `cached_result_expr` — its build-loading work is memoized on + `_resolve_result_plan`, `lru_cache(256)` keyed `(project, content_hash)`; + `cached_result_expr` re-exposes that memo's `cache_clear` / `cache_info`. + The key finally determines the value — the plan is a pure function of the + immutable build — and the memo saves expanding + loading the build and + re-deriving its baked-snapshot path. `cached_result_expr` itself is a thin + wrapper that re-checks the snapshot's on-disk presence on every call, so an + evicted snapshot self-heals; the heal runs under a per-entry lock + (`_heal_lock`) so only the first of several concurrent cold readers executes + it, and a peer process that wins xorq's fixed-`.tmp` rename is caught and + retried once if the snapshot landed, else re-raised (#79). - `_build_compare_expr` — `lru_cache(128)` keyed `(project, a_hash, b_hash, keys)`; saves rebuilding diff outer-join expressions (`src/tallyman_companion/app.py:189`). Build dirs land under @@ -336,23 +348,19 @@ all rely on "same key, same bytes, forever". Cleanup for those is a space concern (manual delete, bullpen moves on reset), and a deleted file self-heals by recomputing. -Two documented holes break "never stale". The first is execution -nondeterminism: an entry whose recipe calls `now()` / `random()` / an unseeded -`sample()` produces different bytes each run under one content hash, so a cold +One documented hole breaks "never stale": execution nondeterminism. An entry +whose recipe calls `now()` / `random()` / an unseeded `sample()` or an impure +UDF produces different bytes each run under one content hash, so a cold recompute can disagree with what was built (#88). The build flags these as advisory lint warnings (`_nondeterminism_warnings`, `build.py`); it does not -block them. As of #83 the build also records a `result_digest` of the executed -bytes in the manifest, and the self-heal compares the repopulated snapshot -against it (`verify_result_faithful`, `result_cache.py:299`; -`_warn_if_self_heal_unfaithful`, `result_cache.py:315`), so this drift is now -detected on recompute even though it is still not prevented. The second is -cold-reconstruction faithfulness under the `cas` -default: `cas` makes *build* identity content-aware (a rebuild over an edited -source forks the hash) but not *reconstruction*. `cached_result_expr` re-runs a -cheap entry's `expr.py` on every read, and `read_project_file` re-digests the live -source, so a cheap entry — or an expensive entry self-healing an evicted -snapshot — serves the edited bytes under its original `content_hash`. Closing -this needs digest-pinned reconstruction, tracked in #115. +block them. The runtime backstop is `result_digest`: every self-heal verifies +the repopulated snapshot against it before serving (`_verify_self_heal`), and +an unfaithful heal wipes the entry's stat cache, records a durable error, and +evicts its Buckaroo session (ADR D7/D10/D12). The second hole this section +used to document — cold reads re-running `expr.py` and re-digesting live +sources, serving edited bytes under the original hash (#115/#163) — is closed: +reads load the frozen build, whose leaves are content-pinned `.cas` clones and +inlined parent graphs, so a cold read cannot see a post-build edit at all. Where staleness is actually possible, it is handled explicitly: @@ -366,11 +374,11 @@ Where staleness is actually possible, it is handled explicitly: Buckaroo restart, and the stat-cache deletion on reload. The one rule to remember: snapshot-strategy caches do not notice upstream data -changes. The `cas` default papers over this at *build* time — an edited source -forks the entry hash rather than deduping to the stale one — but a cold read of -a cheap or evicted entry still re-digests the live source and can serve edited -bytes under the old hash (#74, above). Anyone who needs reconstruction to be -content-faithful today should treat a source as immutable once an entry is built. -`off` reverts to xorq's path-only identity (an edit collides silently); `salt` -mixes digests into the entry hash but bakes no snapshot cache at all (path-only -keys would collide across salted entries) and recomputes on every read. +changes. The `cas` default makes that safe end-to-end: an edited source forks +the entry hash at build rather than deduping to the stale one, and reads — +warm, cold, or self-healing — resolve through the frozen build to the +content-addressed clone the entry was built from, never the live file. `off` +reverts to xorq's path-only identity (an edit collides silently, and a loaded +build reads whatever bytes sit at the recorded path); `salt` mixes digests +into the entry hash but bakes no snapshot cache at all (path-only keys would +collide across salted entries) and recomputes on every read. diff --git a/docs/expression-lifecycle.md b/docs/expression-lifecycle.md index 17c65f8..86c23d1 100644 --- a/docs/expression-lifecycle.md +++ b/docs/expression-lifecycle.md @@ -194,17 +194,13 @@ The SPA issues `GET /{project}/api/entry/{hash-or-alias}` 1. `_maybe_restart()` (revives a crashed subprocess, throttled); bail to `None` if Buckaroo isn't running — the SPA then falls back to a `pandas.to_html` preview. -2. **Snapshot self-heal** (`buckaroo_lifecycle.py:518-533`): before anything - Buckaroo-facing, `ensure_session` calls `cached_result_expr(project, hash)`. - For an expensive entry this guarantees the baked snapshot exists on disk — on - a cold compute cache (a fresh clone, or an entry the startup warmup didn't - reach) it recomputes it once, single-flighted per `(project, content_hash)` - under `_heal_lock` so two tabs hitting the same cold entry don't both run the - shared cache op (#79). This matters because a `tracked_expr_from_alias` chain off an - expensive parent embeds a *bare* read of the parent's snapshot (#75 strips - the parent's cache node to keep one backend), so without the snapshot the - replay below would read zero rows. Done outside the session lock so a cold - recompute doesn't block other sessions. +2. **No pre-heal** (ADR D4): entry builds are self-contained — a + `tracked_expr_from_alias` chain keeps the parent's cache node in the child's + build — so Buckaroo's replay of the posted build regenerates any evicted + snapshot itself through ordinary cache mechanics on first query. (Before + #163's fix, chaining stripped the parent's cache node to a bare snapshot + read, and `ensure_session` had to pre-execute `cached_result_expr` here so a + cold grid wasn't empty; both the stripping and the pre-heal are retired.) 3. **Session-map check** (`buckaroo_lifecycle.py:535`+): for a new entry this misses, so a session is created. 4. **Stable build expansion**: `ensure_expanded_build` materializes @@ -239,23 +235,27 @@ by `GET /{project}/api/session/{hash}`, `app.py:617`, which is just row window and the summary-stats payload. The JS `SmartRowCache` holds row segments client-side from here on (see `caching.md`). -The **result cache** is read through one function, `cached_result_expr` -(`result_cache.py:403`), and — unlike the stat cache — it was already populated -at build (step 5), so a read never writes it except to self-heal an eviction. -For an expensive entry (`classify_build` worthy — Aggregate / Join / Sort / -window / UDF, `result_cache.py:49`) it returns a `deferred_read_parquet` of the -baked snapshot under `compute_cache/result_cache/`, on the default backend; if -the snapshot was evicted it runs the cache node once to repopulate, then reads -(`result_cache.py:451-473`). That heal is single-flighted per `(project, -content_hash)` under `_heal_lock`, so concurrent cold readers don't both execute -the shared cache op and trip DataFusion's "Already borrowed", and a peer process -losing xorq's fixed-`.tmp` rename retries once if the snapshot landed (#79). The -repopulated snapshot is then checked against the `result_digest` recorded at -build, logging recompute drift on a mismatch (`_warn_if_self_heal_unfaithful`; -#83). A cheap entry returns the live expression re-imported from `expr.py`, -recomputed on read. Every reader takes this path: the paginated viewer -(`api_data` calls `cached_result_expr(...).limit(...).execute()`, `app.py:517`), -both sides of a diff, and post-processing. +The **result cache** is read through one function, `cached_result_expr`, whose +internals load the entry's frozen build (the canonical read of +`docs/system-contract.md`) — and, unlike the stat cache, it was already +populated at build (step 5), so a read never writes it except to self-heal an +eviction. For an expensive entry (`classify_build` worthy — Aggregate / Join / +Sort / window / UDF) it returns a `deferred_read_parquet` of the baked snapshot +under `compute_cache/result_cache/`, on the default backend, after asserting +the derived snapshot key matches the one the manifest recorded (ADR D8); if +the snapshot was evicted it executes the frozen build once to repopulate, then +reads. That heal is single-flighted per `(project, content_hash)` under +`_heal_lock`, so concurrent cold readers don't both execute the shared op and +trip DataFusion's "Already borrowed", and a peer process losing xorq's +fixed-`.tmp` rename retries once if the snapshot landed (#79). The healed +bytes are verified against the `result_digest` recorded at build before they +are served; a mismatch records a durable `unfaithful_heal` error, wipes the +entry's stat cache, and evicts its Buckaroo session (`_verify_self_heal`; ADR +D7/D10/D12). A cheap entry returns the loaded build's graph rebound onto the +default backend, recomputed on read from content-pinned sources. Every reader +takes this path: the paginated viewer (`api_data` calls +`cached_result_expr(...).limit(...).execute()`), both sides of a diff, and +post-processing. --- @@ -332,6 +332,10 @@ Everything keyed on the content hash is immutable and self-heals on deletion; the only invalidations are Buckaroo restart (RAM session map) and a klass-changing edit/reset (stat cache). A `reset_to` additionally garbage-collects orphaned `data/.cas` source clones, which live outside the -catalog git repo (`source_identity.gc_cas`). See `caching.md` for the full -invalidation table, including the cold-reconstruction staleness hole the `cas` -default does not close (#74/#115). +catalog git repo (`source_identity.gc_cas`) — an entry's own +`manifest.sources` records every clone its frozen build reads, ancestors' +included, so gc keeps a child's leaves alive even if the parent entry is +evicted. See `caching.md` for the full invalidation table. (The +cold-reconstruction staleness hole this section used to reference — #74/#115 — +is closed: reads load the frozen build, so a cold read cannot see a post-build +source edit.) diff --git a/docs/system-contract.md b/docs/system-contract.md index 9199c46..d6d1156 100644 --- a/docs/system-contract.md +++ b/docs/system-contract.md @@ -578,7 +578,7 @@ wrong even if every test passes. # Deviations on main today -As of `main` @ d7e0867 — the gap this proposal closes (all tracked in #163): +As of `main` @ d7e0867 — the gap PR #167 closes (all tracked in #163): - No in-process consumer loads the build; the only `load_expr` call in the codebase is at build time (`build.py:496`). Every in-process read @@ -625,16 +625,20 @@ contract's rules apply to those axes too; the audit is the worklist. For the grilling session; none block understanding the design, all block calling it finished: -1. **Canonical ordering for parquet-sourced bakes.** *Resolved (ADR D5):* - the order-insensitivity claim is scoped to CSV-sourced entries; parquet - digests are documented order-sensitive with advisory drift warnings, a - tracked issue, and provocation tests. Sort-all-columns and multiset - digests remain the fallback options if the advisory fires in practice. -2. **Nested cache-node rebase.** With parent cache nodes kept inline, builds - now contain nested cache nodes, and xorq's shallow load-time cache-dir - rewrite misses the inner ones. Proposal: a tallyman-side deep rewrite over - xorq's deep-walking primitives. Verify no path still relies on xorq's - shallow one. +1. **Canonical ordering for parquet-sourced bakes.** *Resolved (ADR D5, + amended):* originally scoped to CSV with advisory warnings, but the #171 + probe showed the drift fires on every heal at scale (3 heals, 3 distinct + digests), so the escape clause was exercised: the worthiness rewrite + imposes a sort-by-all-columns total order under the result-cache node + (author's `order_by` keys first, then `original_row_order`, then the rest), + making every worthy bake byte-deterministic. The probe is now a regression + test pinning byte-equality across heals. +2. **Nested cache-node rebase.** *Resolved (implemented):* + `portable.rewrite_cache_dirs` runs the storage rewrite through xorq's + `replace_nodes` (which descends `CachedNode.parent`), and the canonical + read applies it after every `load_expr`. xorq's shallow rewrite still runs + first via `load_expr(cache_dir=…)`; the deep pass is idempotent over what + it already fixed. 3. **Child hashes change.** *Resolved (ADR D1/D4):* accepted — the read-path fix, chaining inline, and digest re-scope land in one PR followed by one corpus rebuild. @@ -644,11 +648,13 @@ calling it finished: 5. **Fallback scope.** *Resolved (ADR D6):* there is no fallback. A missing or unloadable build is a hard error; recipe re-execution survives only in minting and the structural-nondeterminism diagnostic. -6. **`replace_sources` edge.** It refuses raw `DatabaseTable` leaves (data - living inside a backend). Tallyman's build rejects in-memory reads, so - these shouldn't occur — but memtables serialized by xorq - (`memtables/*.parquet` in a build) load back as real tables. Confirm no - tallyman entry can produce one, or define the composition story if it can. +6. **`replace_sources` edge.** *Resolved (implemented):* the build already + rejects in-memory reads (`InMemoryReadError`), so no tallyman entry can + serialize a memtable, and the rebind keeps `replace_sources`'s default + refusal of raw `DatabaseTable` leaves — if one ever appears, the read fails + loudly naming the table rather than silently copying data between + backends. The one-content-profile guard (ADR D3) sits beside it: more than + one distinct backend profile in a build also fails loudly. 7. **Eviction policy for unfaithful entries.** *Resolved (ADR D12):* pin non-evictable, badge via the error record; full bullpen wiring deferred. 8. **`diff_stat_cache` ownership.** *Resolved (ADR post-acceptance):* wiped diff --git a/plans/adr-read-path-loads-builds.md b/plans/adr-read-path-loads-builds.md index dc2858b..32aaac0 100644 --- a/plans/adr-read-path-loads-builds.md +++ b/plans/adr-read-path-loads-builds.md @@ -2,8 +2,8 @@ - **Status:** Accepted (2026-07-30) — decided in the grilling session over `docs/system-contract.md`. Supersedes the read-path behavior introduced by - #74/#75 (recipe re-import on read). Four follow-up questions remain open, - listed at the end. + #74/#75 (recipe re-import on read). All follow-up questions resolved + (D9–D12, and the D5 amendment); implemented on PR #167. - **Context ticket:** buckaroo-data/tallyman#163 (read path reconstructs from `expr.py`; a content hash does not name a fixed result). Normative design: `docs/system-contract.md`. Bug-class survey: @@ -21,9 +21,10 @@ (the digest whose ordering claim D5 re-scopes). - **Evidence:** #163's verification on the `taxi2` corpus (frozen builds execute to distinct correct results; composition after rebinding works; - warm and cold runs byte-identical), and - `tests/test_lineage_faithful_reads.py` — five invariant tests red on CI - (PR #167) against `main`. + warm and cold runs byte-identical); + `tests/test_lineage_faithful_reads.py` — eight invariant tests red on CI + (PR #167) against `main`; and the #171 probe (3 distinct digests over 3 + heals at 250k rows) that forced the D5 amendment. ## Problem @@ -39,11 +40,11 @@ restoring them. ### D1. One PR, one rebuild -The read-path fix, the chaining change (D4), and the digest re-scope (D5) -land together on PR #167, flipping the five red tests green in a single -change, followed by one corpus rebuild (chaining changes child content -hashes). *Rejected:* staged landing (read-path first, hash-changing PRs -after) — smaller diffs and easier bisection, but two rebuild events and an +The read-path fix, the chaining change (D4), and the canonical-sort bake (D5) +land together on PR #167, flipping the eight red tests green in a single +change, followed by one corpus rebuild (chaining and the sort both change +content hashes). *Rejected:* staged landing (read-path first, hash-changing +PRs after) — smaller diffs and easier bisection, but two rebuild events and an interim state where builds are read but not yet self-contained. ### D2. Keep the facade; replace the internals @@ -85,20 +86,35 @@ rebuilds. *Rejected:* keep stripping — no nesting and no rebuild, but builds stay non-self-contained and the pre-heal choreography stays load-bearing forever. -### D5. Digest order-insensitivity is scoped to CSV-sourced entries - -`result_digest` keeps its file-hash definition. CSV-sourced entries keep the -canonical `original_row_order` sort (the existing ADR). For parquet-sourced -worthy entries the order-insensitivity claim is **withdrawn**, not fixed: -their digests are documented as order-sensitive, heal-drift warnings on them -are advisory, and #171 plus provocation tests (attempt to trigger DataFusion -parallel-scan reordering; demonstrate either stability or the advisory -firing) capture the residual. *Rejected for now:* sort-by-all-columns -before the bake (deterministic bytes everywhere, but a sort on every -bake/heal and pinned-null-ordering complexity) and a multiset digest (order -insensitivity by definition, but verify must read every row and the digest -stops being a hash of the artifact). Either remains available if the -advisory fires in practice. +### D5. The bake is canonically ordered: sort-by-all-columns (amended) + +**Amended 2026-07-30, same day, on probe evidence.** As originally decided, +the order-insensitivity claim for parquet-sourced worthy entries was +withdrawn rather than fixed: digests documented as order-sensitive, heal-drift +warnings advisory, #171 + provocation tests capturing the residual — with +sort-by-all-columns and a multiset digest noted as the escape hatches "if the +advisory fires in practice." + +It fires on every heal. The #171 probe (250k rows, above DataFusion's 1 MiB +`repartition_file_min_size`) produced three distinct digests over three heals, +none matching the build. At that rate the advisory posture is untenable: D10 +would wipe stat caches on every heal and D12 would pin and badge every large +parquet-sourced entry, drowning the signal both exist to carry. + +So the escape clause is exercised: `rewrite_for_build` imposes a canonical +total order under the result-cache node before every bake. Key priority: the +author's own top-level `order_by` keys stay primary (served row order is part +of what they asked for), then `original_row_order` (the canonical CSV file +order), then the remaining sortable columns in schema order; nested/geospatial +columns are skipped as keys (rows still tied after all sortable columns are +byte-identical, so residual ties can't change the file's bytes). The sort node +lands in the build, so the build's bake and every later heal execute the same +ordered plan — the probe test now pins byte-equality across heals instead of +probing for drift. `result_digest` keeps its file-hash definition and is sound +for every worthy entry, CSV- and parquet-sourced alike. *Rejected:* the +multiset digest (order insensitivity by definition, but verify must read every +row and the digest stops being a hash of the artifact). Cost accepted: a sort +on every bake/heal — D4 already forces the corpus rebuild this rides on. ### D6. A missing or unloadable build is a hard error @@ -141,12 +157,12 @@ wrong-file read. project policy there is no migration path. `diff_stat_cache/` is wiped and Buckaroo bounced at the same time (its current contents were computed over #163's self-join diffs). -- **Buckaroo's caches become sound again for CSV-sourced entries:** with - verify enforced, a faithful heal is byte-identical, so expression-keyed - stat caches over stable paths are honest. For parquet-sourced entries a - heal may legitimately reorder rows (D5): aggregate summary stats are - order-insensitive and stay correct, but an open session's row cache can - show the new order — accepted until the D5 issue is resolved. +- **Buckaroo's caches become sound again:** with the canonical sort (D5, + amended) and verify enforced, a faithful heal is byte-identical for every + worthy entry — CSV- and parquet-sourced alike — so expression-keyed stat + caches over stable paths are honest. The unfaithful residual (a genuinely + nondeterministic recompute: an impure UDF, source drift under `off`) is the + D10/D12 path, not a routine occurrence. - **The audit's W5 (MCP-process invalidation) largely dissolves:** cached values become pure functions of immutable builds, so cross-process staleness reduces to the existence checks the read already performs. diff --git a/src/tallyman_companion/app.py b/src/tallyman_companion/app.py index 271f1b9..80ae385 100644 --- a/src/tallyman_companion/app.py +++ b/src/tallyman_companion/app.py @@ -6,6 +6,7 @@ import logging import os import re +import shutil import time from dataclasses import asdict from pathlib import Path @@ -440,7 +441,7 @@ def _build_compare_expr(project: str, a_hash: str, b_hash: str, keys: tuple[str, return build_path, overrides -def _invalidate_reset_caches() -> None: +def _invalidate_reset_caches(project: str | None = None) -> None: """Drop the companion's process-global result/compare caches after a reset. A reset prunes baked snapshots (``catalog_state.reset_to`` → @@ -452,6 +453,11 @@ def _invalidate_reset_caches() -> None: but we clear it for parity. Blunt global clear is correct and cheap: entries are content-addressed, so the next call rebuilds an identical expression. + When *project* is given, the per-pair Buckaroo diff stat cache + (``diff_stat_cache/``) is wiped too: its stats were computed over diff joins + whose inputs the reset/recalc just moved, and Buckaroo repopulates it on the + next compare view (ADR follow-up to D10; size caps deferred). + Called from BOTH reset paths — the in-server ``/api/reset`` and the cross-process CLI ``reset-to`` that arrives as a ``project_reset`` on ``/internal/notify`` — so the two can't drift. (The in-server path having the @@ -459,6 +465,10 @@ def _invalidate_reset_caches() -> None: """ _build_compare_expr.cache_clear() cached_result_expr.cache_clear() + if project is not None: + from tallyman_core.paths import diff_stat_cache_root # noqa: PLC0415 + + shutil.rmtree(diff_stat_cache_root(project), ignore_errors=True) def _recalc_sse_event(remap: dict, step: int | None) -> dict: @@ -661,6 +671,38 @@ async def publish(event: dict): for q in list(subscribers): await q.put(event) + # ADR D7/D10: when a self-heal fails verification (result_cache writes the + # durable errors.jsonl record and wipes the entry's stat cache itself), the + # companion additionally evicts the entry's Buckaroo session — an open + # session's row cache would show the pre-heal rows beside fresh stats — and + # pushes the SSE event the UI badges from. Registered on startup (the hook + # needs the running loop to publish from the sync heal path) and removed on + # shutdown so test-created apps don't pile up dead hooks. + @app.on_event("startup") + async def _register_unfaithful_heal_hook(): + from tallyman_xorq.result_cache import UNFAITHFUL_HEAL_HOOKS # noqa: PLC0415 + + loop = asyncio.get_running_loop() + + def _on_unfaithful_heal(project: str, content_hash: str) -> None: + if buckaroo: + buckaroo.evict_session(content_hash) + asyncio.run_coroutine_threadsafe( + publish({"kind": "unfaithful_heal", "hash": content_hash, "project": project}), + loop, + ) + + UNFAITHFUL_HEAL_HOOKS.append(_on_unfaithful_heal) + app.state.unfaithful_heal_hook = _on_unfaithful_heal + + @app.on_event("shutdown") + async def _unregister_unfaithful_heal_hook(): + from tallyman_xorq.result_cache import UNFAITHFUL_HEAL_HOOKS # noqa: PLC0415 + + hook = getattr(app.state, "unfaithful_heal_hook", None) + if hook is not None and hook in UNFAITHFUL_HEAL_HOOKS: + UNFAITHFUL_HEAL_HOOKS.remove(hook) + async def _auto_recalc_after_head_advance(project: str, name: str, *, tool: str) -> dict | None: """Cascade-recompute *name*'s stale followers after an in-browser head advance (PUT /api/code, promote_diff), in the SAME revision the checkpoint @@ -679,7 +721,7 @@ async def _auto_recalc_after_head_advance(project: str, name: str, *, tool: str) return None report = await run_in_threadpool(auto_recalc, project, name, tool=tool) if report.get("remap"): - _invalidate_reset_caches() + _invalidate_reset_caches(project) if buckaroo: buckaroo.reload_project_sessions(project) # checkpoint_step is None on the auto path: the checkpoint middleware @@ -1658,7 +1700,7 @@ async def api_reset(project: str, payload: dict): # reset_to ran in-process and cleared the result-plan memo; the # compare-expr LRU is a companion-layer cache it can't reach, so invalidate # the companion caches here (#80). Shared with the cross-process notify path. - _invalidate_reset_caches() + _invalidate_reset_caches(project) reloaded = buckaroo.reload_project_sessions(project) if buckaroo else 0 step = current_step(project) await publish({"kind": "project_reset", "step": step}) @@ -1723,7 +1765,7 @@ async def api_recalc(project: str, payload: dict | None = None): return {"status": "ok", "roots": [], "cone": [], "entries": [], "dry_run": dry_run, "remap": {}} report = await run_in_threadpool(recalc, project, roots, dry_run=dry_run) if not dry_run and report.remap: - _invalidate_reset_caches() + _invalidate_reset_caches(project) if buckaroo: buckaroo.reload_project_sessions(project) await publish(_recalc_sse_event(report.remap, report.checkpoint_step)) @@ -1763,7 +1805,7 @@ async def notify(payload: NotifyPayload): # doesn't leave the viewer serving stale state (#80's communication # gap). Not gated on `buckaroo`: the LRUs are process memory # independent of the buckaroo subprocess. - _invalidate_reset_caches() + _invalidate_reset_caches(project_name) if buckaroo: reloaded = buckaroo.reload_project_sessions(project_name) log.info("reloaded klasses in %d buckaroo session(s) for project %s", reloaded, project_name) diff --git a/src/tallyman_companion/buckaroo_lifecycle.py b/src/tallyman_companion/buckaroo_lifecycle.py index a4f885f..df1f044 100644 --- a/src/tallyman_companion/buckaroo_lifecycle.py +++ b/src/tallyman_companion/buckaroo_lifecycle.py @@ -436,6 +436,23 @@ def reload_project_sessions(self, project: str) -> int: self._persist_sessions() return reloaded + def evict_session(self, content_hash: str) -> bool: + """Drop the cached session for one entry so its next view reloads fresh. + + Used after an unfaithful self-heal (ADR D10): the entry's snapshot bytes + changed under a stable path, so an open session's row cache would show + the old rows beside freshly computed stats. Returns whether a session + was actually evicted. The Buckaroo-side session object is left to its + own idle reaping — only the hash→session mapping is dropped here, which + is what makes the next ``load_session`` create a new one. + """ + with self._session_lock: + removed = self._sessions.pop(content_hash, None) is not None + if removed: + self._persist_sessions() + log.info("evicted buckaroo session for %s (unfaithful heal)", content_hash) + return removed + def _clear_stat_cache(self, project: str, content_hash: str) -> None: """Delete cached stat parquet files for one entry. @@ -550,22 +567,12 @@ def load_session( "session_id": None, "detail": "This entry has no xorq build to load.", } - # Materialise the entry's baked snapshot before buckaroo replays the - # build via /load_expr. A tracked_expr_from_alias chain off an expensive parent - # embeds a *bare* read of the parent's snapshot (#75 strips the parent - # CachedNode to keep the composed expression on one backend), so on a - # cold compute cache — a fresh clone, or an entry the startup warmup - # didn't reach — the replay reads zero rows and the grid renders empty. - # cached_result_expr repopulates it (idempotent; lru-cached, so a warmed - # entry is ~free), the same heal the paginated read (api_data) relies on. - # Done before the lock: a cold entry's recompute mustn't block other - # sessions, and a warm one is a cheap cache hit. - try: - from tallyman_xorq.result_cache import cached_result_expr # noqa: PLC0415 - - cached_result_expr(project, content_hash) - except Exception: - log.debug("snapshot self-heal before /load_expr failed for %s", content_hash, exc_info=True) + # No pre-heal here (ADR D4): entry builds are self-contained — a chained + # child's build carries its ancestors' cache nodes — so Buckaroo's replay + # of the build regenerates any evicted snapshot through ordinary cache + # mechanics on first query. The discarded-result cached_result_expr call + # that used to force ancestor snapshots onto disk before the replay is + # retired with the #75 stripping that made it necessary. with self._session_lock: cached = self._sessions.get(content_hash) if cached: diff --git a/src/tallyman_core/manifest.py b/src/tallyman_core/manifest.py index 2fccbd3..12a9d8b 100644 --- a/src/tallyman_core/manifest.py +++ b/src/tallyman_core/manifest.py @@ -55,6 +55,14 @@ class Manifest(BaseModel): # UDF) the structural hash can't see — caught when a self-healed snapshot is # re-checked. Absent on entries built before #83. result_digest: str | None = None + # Filename of the baked result-cache snapshot (the xorq cache key + .parquet), + # recorded at build so the canonical read can assert its own derivation matches + # (ADR D8, plans/adr-read-path-loads-builds.md). Build and read share one + # derivation route; this is the tripwire that turns any future divergence (an + # xorq tokenization change, a rewrite drift) into an immediate, attributable + # failure instead of a silent wrong-file read. None for a cheap entry (bakes + # no snapshot) and under salt identity mode. + snapshot_key: str | None = None # rel data path -> content md5, recorded when a source-identity mode is # active (tallyman_xorq.source_identity); absent under mode=off. sources: dict[str, str] | None = None diff --git a/src/tallyman_mcp/server.py b/src/tallyman_mcp/server.py index 642e29b..b2c4cf4 100644 --- a/src/tallyman_mcp/server.py +++ b/src/tallyman_mcp/server.py @@ -1022,7 +1022,7 @@ def _resolve(v): @mcp.tool() @_tag_project -def catalog_scan_staleness() -> dict: +def catalog_scan_staleness(verify_results: bool = False) -> dict: """Scan every catalog entry for staleness against its recorded inputs. Read-only. An entry is *stale* when an input it was built against moved: @@ -1034,12 +1034,20 @@ def catalog_scan_staleness() -> dict: and is never listed, since recomputing it re-points no alias. Use ``catalog_recalc`` to act. + ``verify_results=True`` additionally sweeps result faithfulness: every entry + with a recorded ``result_digest`` has its baked snapshot re-hashed and + compared. Adds a ``verify`` key — ``{results: {hash: bool|null}, unfaithful: + [hashes], errors: {hash: message}}`` — where ``false`` means the snapshot's + bytes are not what the build recorded (a nondeterministic recompute healed it + to different bytes). Hashes every snapshot file, so opt-in. + Returns: stale: hashes that are directly stale (the natural recalc roots). transitively_stale: clean descendants carried by a stale ancestor. entries: ``{hash: verdict}`` for every live entry, where a verdict is ``{stale, live, reasons:[{axis, ref, was, now}], unknown_axes, transitively_stale}``. + verify: only when ``verify_results=True`` (see above). """ project = _resolve_active_project() verdicts = staleness.scan(project) @@ -1049,12 +1057,15 @@ def catalog_scan_staleness() -> dict: # owns its own staleness and a directly-stale follower is expected there, not # a bug to flag. Same classifier auto-recalc uses, so the verdict is identical. orphan_stale = classify_orphans(project, verdicts) if auto_recalc_enabled(project) else [] - return { + out = { "stale": sorted(h for h, v in verdicts.items() if v.stale), "transitively_stale": sorted(h for h, v in verdicts.items() if v.transitively_stale), "entries": {h: asdict(v) for h, v in verdicts.items()}, "orphan_stale": orphan_stale, } + if verify_results: + out["verify"] = staleness.verify_sweep(project) + return out @mcp.tool() diff --git a/src/tallyman_xorq/build.py b/src/tallyman_xorq/build.py index c06eaea..43b055c 100644 --- a/src/tallyman_xorq/build.py +++ b/src/tallyman_xorq/build.py @@ -531,20 +531,24 @@ def build_and_persist( if cache_worthy_v: # count() bakes the snapshot (the cache node forces full # materialisation); then hash the baked file for the digest. - # The sort-before-bake is applied to author_expr before - # rewrite_for_build (see above), so the CachedNode already - # wraps the sorted expression and loaded bakes in canonical order. + # rewrite_for_build injected the canonical sort under the + # CachedNode (ADR D5, amended), so the bake lands in a + # deterministic total order and the file hash is stable + # across the build and every later heal. snapshot_key + # records the baked file's name so the canonical read can + # assert its own derivation matches (ADR D8). row_count = int(loaded.count().execute()) snap_path = _cached_node_path(loaded) - result_digest_v: str | None = ( - snapshot_file_digest(snap_path) if snap_path and snap_path.exists() else None - ) + baked_ok = snap_path is not None and snap_path.exists() + result_digest_v: str | None = snapshot_file_digest(snap_path) if baked_ok else None + snapshot_key_v: str | None = snap_path.name if baked_ok else None else: # Stream full result to force row-level evaluation (catch a # failing cast / arithmetic at build time) and count rows. # No digest for cheap entries — no snapshot to hash. row_count = stream_row_count(loaded) result_digest_v = None + snapshot_key_v = None except Exception as exc: hint = _ibis_import_hint(str(exc), code) raise BuildError(f"build execution failed: {exc}{hint}\n{traceback.format_exc()}") from exc @@ -606,6 +610,7 @@ def build_and_persist( cache_worthy_why=cache_worthy_why, cache_bytes=cache_bytes, result_digest=result_digest_v, + snapshot_key=snapshot_key_v, sources=sources or None, parents=parents or None, ) diff --git a/src/tallyman_xorq/io.py b/src/tallyman_xorq/io.py index ca2f503..7049351 100644 --- a/src/tallyman_xorq/io.py +++ b/src/tallyman_xorq/io.py @@ -647,6 +647,27 @@ def _reconstructing_source_digest(proj: str, rel_path: str) -> str | None: return sources.get(rel_path) +def _note_parent_sources(proj: str, content_hash: str) -> None: + """Fold the parent's recorded source digests into the current build's collector. + + A child's build inlines its parent's frozen graph — CAS clone paths included + — so the child's manifest must record those digests too: ``manifest.sources`` + is the closure record ``gc_cas`` walks to keep clones alive, and the child + must keep its own leaves alive even if the parent entry is later evicted. + ``note_source`` is a no-op outside a build's collect window, so this costs + nothing on a plain read. + """ + from tallyman_core import read_manifest + from tallyman_xorq import source_identity as si + + try: + sources = read_manifest(entry_dir(proj, content_hash)).sources or {} + except (OSError, ValueError): + return + for rel_path, digest in sources.items(): + si.note_source(rel_path, digest) + + def tracked_expr_from_alias(alias: str, project: str | None = None): """Load a catalog entry by alias and record the parent edge in the manifest DAG. @@ -659,23 +680,25 @@ def tracked_expr_from_alias(alias: str, project: str | None = None): current head entry. Pass the alias name as it appears in ``catalog_list``. To read by hash without tracking, use ``pinned_expr_from_alias``. - Returns the entry's expression on the in-process default backend — an - expensive parent (Aggregate / Join / ...) resolves to a direct read of its - baked result_cache snapshot, a cheap parent re-runs its recipe (pushdown - makes that ~free). Either way the materialization detail is hidden from - the caller. + Returns the parent entry's *frozen build graph* on the in-process default + backend (``entry_graph_expr``, ADR D4): parents are inlined by value — + content-pinned sources, and an expensive parent's baked ``CachedNode`` + travels along — so the child's own build becomes self-contained and + self-healing. When this alias is revised, recalc mints a new version of the + child expression; the child entry built *now* stays bound to the parent + revision recorded at build time forever. The parent edge is suppressed during reconstruction (when ``_RECONSTRUCTING`` - is True) so that re-running a child recipe to compute its expression doesn't - accidentally record grandparents as direct parents of the entry under - construction. + is True — the structural-nondeterminism diagnostic re-running a recipe) so a + diagnostic re-run doesn't accidentally record grandparents as direct parents + of the entry under construction. Args: alias: A catalog alias (e.g. "shoe_sales"). Must be an alias, not a content hash — pass hashes to pinned_expr_from_alias instead. project: Project name override (defaults to active TALLYMAN_PROJECT). """ - from tallyman_xorq.result_cache import _RECONSTRUCTING, _resolve_noncyclic_hash, cached_result_expr + from tallyman_xorq.result_cache import _RECONSTRUCTING, _resolve_noncyclic_hash, entry_graph_expr proj = resolve_project(project) if entry_dir(proj, alias).exists(): @@ -691,7 +714,8 @@ def tracked_expr_from_alias(alias: str, project: str | None = None): from tallyman_xorq import parent_capture as pc pc.note_parent(content_hash, ref=alias, follow=True) - return cached_result_expr(proj, content_hash) + _note_parent_sources(proj, content_hash) + return entry_graph_expr(proj, content_hash) def pinned_expr_from_alias(alias_or_hash: str, project: str | None = None): @@ -713,7 +737,7 @@ def pinned_expr_from_alias(alias_or_hash: str, project: str | None = None): alias_or_hash: An alias (e.g. "shoe_sales") or a content hash. project: Project name override (defaults to active TALLYMAN_PROJECT). """ - from tallyman_xorq.result_cache import _RECONSTRUCTING, _resolve_noncyclic_hash, cached_result_expr + from tallyman_xorq.result_cache import _RECONSTRUCTING, _resolve_noncyclic_hash, entry_graph_expr proj = resolve_project(project) is_hash = entry_dir(proj, alias_or_hash).exists() @@ -725,4 +749,5 @@ def pinned_expr_from_alias(alias_or_hash: str, project: str | None = None): from tallyman_xorq import parent_capture as pc pc.note_parent(content_hash, ref=alias_or_hash, follow=False) - return cached_result_expr(proj, content_hash) + _note_parent_sources(proj, content_hash) + return entry_graph_expr(proj, content_hash) diff --git a/src/tallyman_xorq/portable.py b/src/tallyman_xorq/portable.py index 58a17cf..c466f9d 100644 --- a/src/tallyman_xorq/portable.py +++ b/src/tallyman_xorq/portable.py @@ -75,6 +75,37 @@ def expand_into_dir(build_dir: Path, project_root: Path, target: Path) -> None: (target / item.name).write_text(text.replace(PLACEHOLDER, project_root_str)) +def rewrite_cache_dirs(expr, cache_dir: Path): + """Point every ``CachedNode``'s storage at *cache_dir* — nested nodes included. + + xorq's own ``load_expr(cache_dir=…)`` rewrite (``ExprLoader.replace_base_path``) + walks the graph with vendored ibis's ``.replace``, which does not descend into + opaque ``Expr``-typed fields — so a cache node nested inside another cache + node's ``parent`` (a chained parent's cache traveling in a child's build, ADR + D4) keeps whatever base path the build serialized. This deep variant runs the + same storage rewrite through xorq's ``replace_nodes``, whose traversal does + descend ``CachedNode.parent``, so every cache node in the closure lands in the + project's compute cache regardless of nesting depth or where the build was + written. + """ + from attr import evolve + from xorq.common.utils.graph_utils import replace_nodes + from xorq.expr.relations import CachedNode + + cache_dir = Path(cache_dir) + + def replacer(node, kwargs): + if isinstance(node, CachedNode): + storage = getattr(node.cache, "storage", None) + if storage is not None and Path(str(storage.base_path)) != cache_dir: + evolved = evolve(node.cache, storage=evolve(storage, base_path=cache_dir)) + return node.__recreate__(dict(zip(node.__argnames__, node.__args__)) | {"cache": evolved}) + return node + return node.__recreate__(kwargs) if kwargs else node + + return replace_nodes(replacer, expr).to_expr() + + def ensure_expanded_build(build_dir: Path, project_root: Path, expanded: Path) -> Path: """Expand `${TALLYMAN_PROJECT_ROOT}` placeholders into a stable, persistent dir. diff --git a/src/tallyman_xorq/result_cache.py b/src/tallyman_xorq/result_cache.py index 081b2fa..ac1619f 100644 --- a/src/tallyman_xorq/result_cache.py +++ b/src/tallyman_xorq/result_cache.py @@ -1,20 +1,29 @@ """xorq-backed result cache for catalog entries, gated by a structural rubric. -The entry's *expression* (its portable ``xorq_build``) is the source of truth. -Whether we keep a materialised copy of its result depends on how expensive the +The entry's frozen build (``xorq_build/``) is the source of truth, and **every +read loads it** — ``ensure_expanded_build`` → ``load_expr(cache_dir=…)`` → the +deep cache-dir rewrite (``portable.rewrite_cache_dirs``). This is the canonical +read of ``docs/system-contract.md`` (#163): a content hash names the fixed +result its build freezes, so reads never re-import ``expr.py`` — recipe +re-execution happens only at minting (build / revise / recalc) and in the +structural-nondeterminism diagnostic below. A missing or unloadable build is a +hard error (ADR D6), never a fallback. + +Whether we keep a materialised copy of the result depends on how expensive the computation is — caching only pays off when recompute costs far more than reading a cached parquet: * Expensive entries — the expression contains an Aggregate / Join / Sort / window / UDF — are materialised in xorq's ``ParquetSnapshotCache``. Reads - hit the cache; a deleted cache file self-heals by recomputing from the - build. + hit the cache; a deleted cache file self-heals by re-executing the frozen + build, and the healed bytes are verified against the recorded + ``result_digest`` before they are served (ADR D7/D10). * Cheap entries — a source read plus projections / renames / row-wise scalar math (the citibike column-reorder/derive case) — materialise nothing (#73). Recompute ≈ re-reading a columnar source (pushdown), so a stored copy would burn work and storage for ~no savings; a non-parquet source's parse is already cached (see ``tallyman_xorq.source_cache``). Reads of a cheap entry - recompute the expression directly — no per-entry ``result.parquet`` is ever + execute the loaded build directly — no per-entry ``result.parquet`` is ever written, on demand or otherwise. Every consumer (the viewer, paginated reads, diffs, post-processing) reads ``cached_result_expr``. @@ -39,6 +48,7 @@ import threading import time from pathlib import Path +from typing import NamedTuple # Perf instrumentation rides a dedicated child namespace so it can be dialed up # independently of the rest of tallyman's logging (#60), via TALLYMAN_LOG_LEVEL. @@ -251,25 +261,139 @@ def _cached_node_path(baked) -> Path | None: return Path(node.cache.storage.get_path(node.cache.calc_key(node.parent))) -def baked_snapshot_path(project: str, content_hash: str) -> Path | None: - """Path of the entry's baked result-cache snapshot, or None if it bakes none. +def load_entry_expr(project: str, content_hash: str): + """Load the entry's frozen build as a live expression — the canonical read (#163). + + ``ensure_expanded_build`` → ``load_expr(expanded, cache_dir=compute_cache)`` + → ``rewrite_cache_dirs`` (the deep rewrite xorq's shallow load-time one + misses for nested cache nodes, ADR D4). The build binds by value — parents + inlined, sources as content-pinned paths — so the returned expression is the + entry's fixed computation regardless of where alias heads sit today. + + A missing or unloadable build raises ``BuildError`` naming the entry and the + remedy (ADR D6). There is no recipe fallback: ``expr.py`` binds by name and + re-executing it is how #163's lineage drift happened. + """ + from xorq.ibis_yaml.compiler import load_expr + + from tallyman_core.paths import ( + compute_cache_dir, + entry_build_dir, + entry_expanded_build_dir, + project_dir, + ) + from tallyman_xorq.build import BuildError + from tallyman_xorq.portable import ensure_expanded_build, rewrite_cache_dirs + + build_dir = entry_build_dir(project, content_hash) + if not (build_dir / "expr.yaml").is_file(): + raise BuildError( + f"entry {content_hash} in {project!r} has no loadable xorq_build/ " + f"(expected {build_dir / 'expr.yaml'}). The frozen build is the read " + "path's only source of truth — rebuild the entry (catalog_revise / " + "catalog_recalc) to restore it; reads never fall back to expr.py." + ) + cache_dir = compute_cache_dir(project) + cache_dir.mkdir(parents=True, exist_ok=True) + try: + expanded = ensure_expanded_build( + build_dir, project_dir(project), entry_expanded_build_dir(project, content_hash) + ) + loaded = load_expr(expanded, cache_dir=cache_dir) + return rewrite_cache_dirs(loaded, cache_dir) + except Exception as exc: + raise BuildError( + f"entry {content_hash} in {project!r}: loading its frozen build failed " + f"({type(exc).__name__}: {exc}) — rebuild the entry (catalog_revise / catalog_recalc)" + ) from exc + + +def _profile_content_token(backend) -> str: + """A backend profile's content identity: its tokenized dict minus ``idx``. - Mirrors ``cached_result_expr``'s derivation — re-import the recipe, rewrite - it for build, read the top-level ``CachedNode``'s storage path — so it names - the very file a cold read returns. None for a cheap entry (bakes nothing), a - worthiness disagreement, or salt mode (bakes no cache — see - ``rewrite_for_build``). The build uses it to size the snapshot (#87, the - value-per-byte denominator); other callers use it to find the snapshot - without reconstructing the read expression. + ``Profile.idx`` is a process-local counter (creation-order noise), so two + backends are the same *kind* of connection exactly when their idx-stripped + profiles tokenize identically — the equality ``Profile.__eq__`` itself uses. """ - from tallyman_xorq import source_identity as si + import toolz + from xorq.common.utils.dasher import tokenize - if si.mode() == "salt" or not cache_worthy(project, content_hash): - return None + return tokenize(toolz.dissoc(backend._profile.as_dict(), "idx")) + + +def _rebind_to_default_backend(expr): + """Collapse every backend in a loaded build onto the process default backend. + + ``load_expr`` mints fresh backend objects per profile, so two loaded builds — + or a loaded build and a recipe's ``read_project_file`` — span distinct + backend objects and composition raises "Multiple backends found". The + contract allows the collapse because every profile in a tallyman build is + content-identical no-arg ``xorq_datafusion``; that assumption is enforced + here (ADR D3): more than one distinct content profile fails loudly rather + than misbinding a node onto the wrong kind of connection. A raw + ``DatabaseTable`` (bundled data that would need a copy) also fails loudly — + ``replace_sources`` raises unless told to transfer, and tallyman builds must + never contain one (in-memory reads are rejected at build). + """ + from xorq.common.utils.graph_utils import find_all_sources, replace_sources + from xorq.config import default_backend + + target = default_backend() + others = [s for s in find_all_sources(expr) if s is not target] + if not others: + return expr + tokens = {_profile_content_token(s) for s in (*others, target)} + if len(tokens) > 1: + from tallyman_xorq.build import BuildError + + raise BuildError( + f"loaded build spans {len(tokens)} distinct backend content profiles; " + "rebinding onto the default backend would misbind — every profile in a " + "tallyman build must be content-identical no-arg xorq_datafusion (ADR D3)" + ) + return replace_sources({id(s): target for s in others}, expr) + + +def _assert_recorded_snapshot_key(project: str, content_hash: str, path: Path) -> None: + """ADR D8 tripwire: the read's snapshot derivation must match the build's. - from tallyman_xorq.source_cache import rewrite_for_build + The build records the baked snapshot's filename (``manifest.snapshot_key``); + the canonical read asserts its own derivation lands on the same file. With + both sides sharing one derivation route the original disagreement mechanism + is gone — a mismatch means an xorq tokenization change or a rewrite drift, + and reading on would serve the wrong file silently. Manifests without the + field (mid-rebuild corpus) are skipped. + """ + from tallyman_core import read_manifest + from tallyman_core.paths import entry_dir + + try: + recorded = read_manifest(entry_dir(project, content_hash)).snapshot_key + except (OSError, ValueError): + return + if recorded is None or path.name == recorded: + return + from tallyman_xorq.build import BuildError - return _cached_node_path(rewrite_for_build(_recipe_expr(project, content_hash), project)) + raise BuildError( + f"entry {content_hash} in {project!r}: the read derives snapshot key " + f"{path.name!r} but the build recorded {recorded!r} — the two derivations " + "have diverged (an xorq upgrade or a rewrite drift); rebuild the entry " + "rather than read the wrong snapshot" + ) + + +def baked_snapshot_path(project: str, content_hash: str) -> Path | None: + """Path of the entry's baked result-cache snapshot, or None if it bakes none. + + Derived from the entry's *loaded build* — the same derivation + ``cached_result_expr`` serves and the build recorded — so it names the very + file a cold read returns. None for a cheap entry (bakes nothing), a + worthiness disagreement, or a build written under salt mode (no cache node — + see ``rewrite_for_build``). Raises like ``load_entry_expr`` when the build is + missing (ADR D6). + """ + return _resolve_result_plan(project, content_hash).path def snapshot_file_digest(path: Path) -> str: @@ -317,18 +441,19 @@ def _recorded_result_digest(project: str, content_hash: str) -> str | None: def verify_result_faithful(project: str, content_hash: str) -> bool | None: - """Whether the entry's baked snapshot matches its build-time digest. - - Only meaningful for worthy (snapshot-baking) entries: returns True when the - snapshot file's SHA-256 matches the recorded ``result_digest``, False on - drift, and None when no digest was recorded (cheap entry, or built before - the canonical-order ADR — ``adr-result-digest-canonical-ordering.md``). - Cheap entries record no digest, so this always returns None for them. + """Whether executing this entry's frozen build reproduces its recorded digest. + + Re-anchored by ADR D7: the snapshot is located through the entry's *own + loaded build* — never today's alias state — so the verdict is about this + entry's bytes, not a sibling's. Returns True when the snapshot file's + SHA-256 matches the recorded ``result_digest``, False on drift, and None + when there is nothing to check (no recorded digest — a cheap entry — or no + snapshot on disk yet; a cold entry heals on its next read, which verifies). """ recorded = _recorded_result_digest(project, content_hash) if not recorded: return None - snap = baked_snapshot_path(project, content_hash) + snap = _resolve_result_plan(project, content_hash).path if snap is None or not snap.exists(): return None return snapshot_file_digest(snap) == recorded @@ -398,20 +523,34 @@ def recipe_is_structurally_nondeterministic(project: str, content_hash: str) -> return a is not None and b is not None and a != b -def _warn_if_self_heal_unfaithful(project: str, content_hash: str, path: Path) -> None: - """Compare a just-repopulated snapshot to the build-time digest, warn on drift. +# Called (project, content_hash) after an UNFAITHFUL self-heal, best-effort. +# The companion registers a hook that evicts the entry's Buckaroo session and +# pushes the SSE badge event (ADR D7/D10); processes without an SSE bus (the +# MCP server) still get the durable errors.jsonl record written below. +UNFAITHFUL_HEAL_HOOKS: list = [] + + +def _verify_self_heal(project: str, content_hash: str, path: Path) -> None: + """Verify a just-repopulated snapshot against the build-time digest (ADR D7). Eviction's load-bearing assumption is that an evicted snapshot recomputes to - what was evicted (the result-cache + source-identity ADRs both call this out). - For an execution-nondeterministic entry that is false: the snapshot self-heals - to *different* bytes, silently. This fires the soundness signal — a - ``tallyman.perf`` warning — without ever breaking the read (best-effort), - attributing the drift as structural (#88: the recipe's graph hash itself moves) - vs execution (#83: a fixed graph that runs differently). - - The digest is now the SHA-256 of the snapshot file bytes (ADR - ``adr-result-digest-canonical-ordering.md``), so we hash the newly-landed - file directly — no extra expression execution. + what was evicted. A faithful heal is byte-identical (the canonical sort makes + the bake deterministic); a mismatch means this entry's recompute is genuinely + nondeterministic and the heal just manufactured different bytes under the + entry's recorded hash. The read is still served — the bytes are the honest + output of the frozen build — but never silently: + + * a ``tallyman.perf`` UNFAITHFUL warning, attributing the drift as + structural (#88: the recipe's graph hash itself moves) vs execution + (#83: a fixed graph that runs differently); + * a durable ``errors.jsonl`` record (``code="unfaithful_heal"``) — the UI + badge's source, and the ADR D12 pin marking (the entry's bytes are not + regenerable, so eviction machinery must treat its snapshot as retained, + not reclaimable); + * the entry's ``.buckaroo_stat_cache`` is wiped (ADR D10): Buckaroo's + summary stats key on expression structure and stable paths + (buckaroo#955), so stale stats would render beside the fresh rows; + * registered hooks fire (companion: session eviction + SSE). """ recorded = _recorded_result_digest(project, content_hash) if not recorded: @@ -420,31 +559,73 @@ def _warn_if_self_heal_unfaithful(project: str, content_hash: str, path: Path) - actual = snapshot_file_digest(path) except Exception: return - if actual != recorded: - kind = ( - "structural (#88) — the recipe bakes a nondeterministic literal that re-derives a different graph hash" - if recipe_is_structurally_nondeterministic(project, content_hash) - else "execution (#83) — a fixed graph that runs differently each execute, or source drift under off" - ) - perf_log.warning( - "cached_result_expr self-heal %s: UNFAITHFUL recompute [%s] — result " - "digest %s != recorded %s; eviction self-healed it to different bytes " - "than were built", - content_hash, - kind, - actual, - recorded, + if actual == recorded: + return + kind = ( + "structural (#88) — the recipe bakes a nondeterministic literal that re-derives a different graph hash" + if recipe_is_structurally_nondeterministic(project, content_hash) + else "execution (#83) — a fixed graph that runs differently each execute, or source drift under off" + ) + perf_log.warning( + "cached_result_expr self-heal %s: UNFAITHFUL recompute [%s] — result " + "digest %s != recorded %s; eviction self-healed it to different bytes " + "than were built", + content_hash, + kind, + actual, + recorded, + ) + import shutil + + from tallyman_core.paths import entry_stat_cache_dir + + shutil.rmtree(entry_stat_cache_dir(project, content_hash), ignore_errors=True) + try: + from tallyman_core.errors import record_error + + record_error( + project, + code="unfaithful_heal", + message=( + f"self-heal produced different bytes than were built [{kind}]: " + f"digest {actual} != recorded {recorded}" + ), + hash=content_hash, ) + except Exception: + perf_log.debug("unfaithful-heal error record failed for %s", content_hash, exc_info=True) + for hook in list(UNFAITHFUL_HEAL_HOOKS): + try: + hook(project, content_hash) + except Exception: + perf_log.debug("unfaithful-heal hook failed for %s", content_hash, exc_info=True) + + +class _ResultPlan(NamedTuple): + """How an entry's result is read, resolved once from its frozen build. + + ``loaded`` is the build as ``load_entry_expr`` returned it — per-load backend + objects, exactly the expression shape the build executed, so a heal re-runs + the build's own computation. ``graph`` is the same graph rebound onto the + process default backend (ADR D3) — the composable form chaining and cheap + reads serve. ``path`` is the baked snapshot's location, None when the build + bakes none (cheap entry, worthiness disagreement, or a salt-mode build). + """ + + kind: str # "baked" | "recompute" + loaded: object + graph: object + path: Path | None @functools.lru_cache(maxsize=256) -def _resolve_result_plan(project: str, content_hash: str): - """The expensive, memoisable half of ``cached_result_expr``: reconstruct the - recipe and decide how the entry's result is read. Returns either - ``("recompute", expr)`` for a cheap/fallback entry, or ``("baked", baked, path)`` - for an expensive entry whose snapshot ``cached_result_expr`` re-checks on every - call (so a snapshot evicted *after* a warm read still self-heals rather than - returning a dangling ``deferred_read_parquet``). +def _resolve_result_plan(project: str, content_hash: str) -> _ResultPlan: + """The expensive, memoisable half of ``cached_result_expr``: load the entry's + frozen build and decide how its result is read. The LRU is sound because its + key finally determines its value — the build is immutable and the plan is a + pure function of it. ``cached_result_expr`` re-checks the snapshot's on-disk + presence on every call, so a snapshot evicted *after* a warm read still + self-heals rather than returning a dangling ``deferred_read_parquet``. """ t0 = time.monotonic() @@ -456,26 +637,19 @@ def _cold(tag): (time.monotonic() - t0) * 1000, ) - raw = _recipe_expr(project, content_hash) - if not cache_worthy(project, content_hash): - _cold("cheap-recompute") - return ("recompute", raw) - - # Expensive: locate the baked snapshot via the same rewrite the build used. - # rewrite_for_build wraps the expression in a ParquetSnapshotCache whose - # structural key matches the file baked at build time, so storage/calc_key - # give its on-disk path without re-executing. - from tallyman_xorq.source_cache import rewrite_for_build - - baked = rewrite_for_build(raw, project) - path = _cached_node_path(baked) + loaded = load_entry_expr(project, content_hash) + graph = _rebind_to_default_backend(loaded) + path = _cached_node_path(loaded) if path is None: - # classify_build (serialized) and _is_worthy_expr (live) disagreed — fall - # back to recompute rather than dereference a cache that wasn't baked. - _cold("worthy-recompute-fallback") - return ("recompute", raw) + # No baked result cache in the build: a cheap entry, a + # classify_build/_is_worthy_expr disagreement, or a salt-mode build + # (rewrite_for_build bakes no cache under salt). Execute the frozen + # graph directly. + _cold("cheap-recompute") + return _ResultPlan("recompute", loaded, graph, None) + _assert_recorded_snapshot_key(project, content_hash, path) _cold("baked-read") - return ("baked", baked, path) + return _ResultPlan("baked", loaded, graph, path) # Per-(project, content_hash) locks serialise the snapshot heal in cached_result_expr @@ -499,54 +673,53 @@ def _heal_lock(project: str, content_hash: str) -> threading.Lock: def cached_result_expr(project: str, content_hash: str): """The entry's result as a single-backend expression on the default backend. - Reconstituting a catalog entry for chaining (``tracked_expr_from_alias``) or diffing - must yield an expression rooted on the *one* in-process backend, so two - entries combined in a ``union`` / ``join`` resolve to a single backend - instead of raising "Multiple backends found" (#75). #73's predecessor loaded - each entry's build into its own backend, so any composition spanned two. - Both paths below root on the default backend and materialise no entry - ``result.parquet``: - - * Cheap entry — re-import its recipe (``expr.py``) and hand back the live - expression. Recompute on read is ~free (pushdown over a columnar source; - a non-parquet parse is cached at the read), and the recipe binds to the - default backend. + Both paths below serve the entry's *frozen build* (``load_entry_expr``), + root on the default backend, and materialise no entry ``result.parquet``: + + * Cheap entry — the loaded build's graph, rebound onto the default backend + (ADR D3). Recompute on read is ~free (pushdown over content-pinned + columnar sources; a non-parquet parse is cached at the read), and the + graph is frozen, so the bytes are the entry's recorded result — never + today's alias heads. * Expensive entry — read its baked ``result_cache`` snapshot directly as a single ``deferred_read_parquet`` on the default backend. The snapshot was materialised when the entry built (``rewrite_for_build``'s top-level - result cache); reading it skips re-running the DAG *and* keeps the cache - node's own backend out of the caller's expression (a baked - ``CachedNode`` carries its own storage connection, which is the second - backend #73 tripped over). Self-heals: an evicted snapshot is recomputed - once via the cache node, then read. + result cache); reading it skips re-running the DAG and keeps the cache + node's own storage connection out of the caller's expression. Self-heals: + an evicted snapshot is re-executed once *from the frozen build*, verified + against the recorded digest (ADR D7), then read. + + For composition that must stay self-healing — chaining a child recipe off + this entry — use ``entry_graph_expr`` instead: it returns the graph with the + cache node still in place, so the child's build carries its parents' healing + knowledge (ADR D4). Bounded LRU so the cache can't grow without limit; entries are - content-addressed, so an evicted hash rebuilds an identical expression. + content-addressed and builds immutable, so an evicted hash reloads an + identical expression. - salt source-identity mode bakes no cache (path-only snapshot keys would + A salt-mode build carries no cache nodes (path-only snapshot keys would collide across salted entries — see ``rewrite_for_build``), so a salted entry - falls through to recompute: a cheap one via ``cheap-recompute``, an expensive - one via ``worthy-recompute-fallback`` (its ``rewrite_for_build`` returns the - expression uncached under salt, so there is no snapshot to dereference). Both - are single-backend recompute expressions — content-honest, no parquet. - - The expensive reconstruction is memoised in ``_resolve_result_plan``; the - snapshot's on-disk presence is re-checked HERE on every call, so an expensive - entry whose snapshot is evicted *after* a warm read self-heals on the next - read instead of handing back a stale ``deferred_read_parquet`` (which would + falls through to the recompute path — single-backend, content-honest, no + parquet. + + The build load is memoised in ``_resolve_result_plan``; the snapshot's + on-disk presence is re-checked HERE on every call, so an expensive entry + whose snapshot is evicted *after* a warm read self-heals on the next read + instead of handing back a stale ``deferred_read_parquet`` (which would ``ValueError: At least one path is required`` at execute time). The perf-namespace cold-read tag (#87) is emitted from the memoised half, so it - still fires once per genuine reconstruct. + still fires once per genuine load. """ from xorq.expr.api import deferred_read_parquet plan = _resolve_result_plan(project, content_hash) - if plan[0] == "recompute": - return plan[1] - _, baked, path = plan + if plan.kind == "recompute": + return plan.graph + path = plan.path if not path.exists(): # Single-flight the heal per (project, content_hash). _resolve_result_plan is - # lru-cached, so concurrent cold readers share one `baked` op; without this lock + # lru-cached, so concurrent cold readers share one loaded op; without this lock # both miss the evicted snapshot and both execute it, and the loser raises — # DataFusion "Already borrowed" on the shared in-process op, or FileNotFoundError # on xorq ParquetStorage's fixed .parquet.tmp across processes — surfacing @@ -554,7 +727,10 @@ def cached_result_expr(project: str, content_hash: str): with _heal_lock(project, content_hash): if not path.exists(): # a peer thread may have healed it while we waited try: - baked.count().execute() # evicted snapshot: repopulate once, then read + # Evicted snapshot: re-execute the frozen build once, then read. + # plan.loaded is the exact expression shape the build executed, + # so a faithful heal lands byte-identical parquet. + plan.loaded.count().execute() except (FileNotFoundError, ValueError): # A peer *process* (MCP build vs companion heal sharing compute_cache) # can win the shared-tmp rename out from under us. If the snapshot @@ -562,13 +738,26 @@ def cached_result_expr(project: str, content_hash: str): if not path.exists(): raise perf_log.debug("cached_result_expr self-heal %s: evicted-self-heal", content_hash) - # The repopulated snapshot must match the digest recorded at build; a - # mismatch means this entry's recompute is execution-nondeterministic and - # eviction just served different bytes than were built (#83). - _warn_if_self_heal_unfaithful(project, content_hash, path) + _verify_self_heal(project, content_hash, path) return deferred_read_parquet(str(path)) +def entry_graph_expr(project: str, content_hash: str): + """The entry's frozen graph — cache nodes included — on the default backend. + + The chaining primitive behind ``tracked_expr_from_alias`` / + ``pinned_expr_from_alias`` (ADR D4). Where ``cached_result_expr`` serves a + worthy entry as a bare read of its snapshot file (fast, but a file reference + with no regeneration knowledge), this returns the loaded build itself: the + entry's ``CachedNode`` stays in the graph, so a child recipe built over it + freezes its parents' healing knowledge into its own build — a cold load of + the child regenerates ancestor snapshots through ordinary cache mechanics, + with no pre-heal choreography. Rebound onto the process default backend so + it composes with ``read_project_file`` and other entries as one backend. + """ + return _resolve_result_plan(project, content_hash).graph + + # Existing callers clear the result-expr memo via cached_result_expr.cache_clear() # (companion app, conftest, cache tests); the memo now lives on the plan resolver, # so re-expose its cache controls on the public name. diff --git a/src/tallyman_xorq/source_cache.py b/src/tallyman_xorq/source_cache.py index 765f644..6bb4364 100644 --- a/src/tallyman_xorq/source_cache.py +++ b/src/tallyman_xorq/source_cache.py @@ -45,6 +45,86 @@ def _should_cache_read(method_name: str) -> bool: return method_name not in _EXEMPT_READS +def _sortable(dtype) -> bool: + """Whether a column can serve as a canonical-order sort key. + + Nested / geospatial types can't be sort keys in datafusion; leaving them out + of the tie-breakers is safe — any remaining ties are between rows identical + on every sortable column, and identical prefixes write identical bytes. + """ + for pred in ("is_array", "is_map", "is_struct", "is_json", "is_geospatial"): + if getattr(dtype, pred, None) and getattr(dtype, pred)(): + return False + return True + + +def _constant_names(rel) -> set[str]: + """Columns the relation defines as scalars (projected literals/constants). + + A constant column adds no ordering information — every row ties on it — and + ibis dereferences a sort key straight through to the defining value, so + ``ORDER BY `` reaches datafusion's planner, which rejects a bare + literal there ("invalid digit found in string": it reads a numeric literal + as a column ordinal). Skipping them is both necessary and free. + """ + values = getattr(rel, "values", None) + if not values: + return set() + out = set() + for name, v in values.items(): + shape = getattr(v, "shape", None) + if shape is not None and shape.is_scalar(): + out.add(name) + return out + + +def _tie_break_order(names, keyed: set[str], schema, rel) -> list[str]: + """Sortable, non-constant columns not already keyed, ``original_row_order`` first. + + ``original_row_order`` is tallyman's canonical total order for CSV-sourced + entries (``tallyman_read_csv``); leading with it keeps a baked CSV result in + file order rather than lexicographic-by-first-column order. + """ + skip = keyed | _constant_names(rel) + remaining = [n for n in names if n not in skip and _sortable(schema[n])] + remaining.sort(key=lambda n: n != "original_row_order") # stable: row order first, schema order after + return remaining + + +def _canonical_sorted(expr): + """Impose a deterministic total order before the result-cache bake. + + ADR D5 (amended, plans/adr-read-path-loads-builds.md): datafusion's parallel + scan/aggregation reorders rows run-to-run above its 1 MiB repartition + threshold, so an unsorted bake produces different bytes on every heal and + ``result_digest`` stops naming the entry's result. Sorting by every column + makes the bytes deterministic: rows still tied after all sortable columns are + byte-identical, so any permutation among them writes the same file. + + Key priority: the author's own top-level ``order_by`` keys stay primary (the + served row order is part of what they asked for), then ``original_row_order`` + (the canonical CSV file order), then the remaining sortable columns in schema + order. An author sort is extended in place rather than wrapped, so the graph + carries one Sort node. + """ + import xorq.vendor.ibis.expr.operations as ops + + node = expr.op() + if isinstance(node, ops.Sort): + parent = node.parent + keyed = {k.expr.name for k in node.keys if isinstance(k.expr, ops.Field)} + remaining = _tie_break_order(parent.schema.names, keyed, parent.schema, parent) + if not remaining: + return expr + extra = tuple(ops.SortKey(ops.Field(parent, n)) for n in remaining) + return ops.Sort(parent, tuple(node.keys) + extra).to_expr() + schema = expr.schema() + names = _tie_break_order(schema.names, set(), schema, node) + if not names: + return expr # nothing sortable — the digest stays best-effort for this entry + return expr.order_by(names) + + _IN_MEMORY_MSG = ( "expression reads in-memory data (read_in_memory / ibis.memtable). This " "usually means the source was loaded into pandas (e.g. pd.read_csv) and " @@ -153,11 +233,13 @@ def replacer(node, kwargs): expr = replace_nodes(replacer, expr).to_expr() - # 3. Bake the result cache for an expensive expression. A distinct - # relative_path keeps baked results inspectable apart from source parses - # under the same compute-cache root. + # 3. Bake the result cache for an expensive expression, canonically ordered + # first so the snapshot's bytes — and therefore result_digest — are + # deterministic across the build and every later heal (ADR D5, amended). + # A distinct relative_path keeps baked results inspectable apart from + # source parses under the same compute-cache root. if _is_worthy_expr(expr): result_cache = ParquetSnapshotCache.from_kwargs(source=connect(), base_path=base, relative_path="result_cache") - expr = expr.cache(cache=result_cache) + expr = _canonical_sorted(expr).cache(cache=result_cache) return expr diff --git a/src/tallyman_xorq/staleness.py b/src/tallyman_xorq/staleness.py index ccdda4d..897cc77 100644 --- a/src/tallyman_xorq/staleness.py +++ b/src/tallyman_xorq/staleness.py @@ -115,6 +115,37 @@ def entry_staleness(project: str, content_hash: str) -> StaleVerdict: ) +def verify_sweep(project: str) -> dict: + """Opt-in corpus verification (ADR D7): do baked snapshots still match their + recorded ``result_digest``? + + For every entry that recorded a digest, ``verify_result_faithful`` locates + the snapshot through the entry's own frozen build and compares file hashes. + Returns ``{"results": {hash: bool|None}, "unfaithful": [...], "errors": + {hash: message}}`` — ``None`` means nothing to check yet (snapshot not on + disk; the next read heals and verifies), ``errors`` carries entries whose + build failed to load (the D6 hard error, reported per-entry so one broken + entry can't abort a corpus sweep). + """ + from tallyman_xorq.result_cache import verify_result_faithful + + results: dict[str, bool | None] = {} + errors: dict[str, str] = {} + for entry in list_entries(project): + if not entry.get("result_digest"): + continue + h = entry["content_hash"] + try: + results[h] = verify_result_faithful(project, h) + except Exception as exc: + errors[h] = str(exc) + return { + "results": results, + "unfaithful": sorted(h for h, ok in results.items() if ok is False), + "errors": errors, + } + + def scan(project: str) -> dict[str, StaleVerdict]: """Actionable staleness for every live entry, tagging direct vs transitive. diff --git a/tests/test_buckaroo.py b/tests/test_buckaroo.py index cb98506..496398f 100644 --- a/tests/test_buckaroo.py +++ b/tests/test_buckaroo.py @@ -336,21 +336,24 @@ def fake_post(url, json=None, timeout=None): assert not (entry_dir(project, res.content_hash) / "result.parquet").exists() -def test_unit_ensure_session_self_heals_evicted_snapshot_on_cold_cache(project, orders_parquet, monkeypatch): - """ensure_session must materialise an entry's baked snapshot before POSTing - to ``/load_expr``, so the buckaroo grid isn't empty on a cold compute cache. - - The build buckaroo replays embeds a *bare* read of an expensive parent's - snapshot (a ``tracked_expr_from_alias`` chain — #75 strips the parent ``CachedNode`` to - keep the composed expression on one backend). On a cold cache (fresh clone / - not-yet-warmed entry) that read resolves to zero files, so the grid renders - empty — the same zero-row symptom as the unexpanded-build_dir case above, a - different cause. ensure_session must self-heal it first - (``cached_result_expr`` repopulates the snapshot), the same heal the - paginated REST read (api_data) relies on. +def test_unit_ensure_session_posts_self_contained_build_on_cold_cache(project, orders_parquet, monkeypatch): + """The build ensure_session posts must feed a cold grid with no pre-heal. + + Historically ensure_session pre-executed ``cached_result_expr`` before the + ``/load_expr`` POST: chaining stripped an expensive parent's ``CachedNode`` + to a *bare* snapshot read (#75), so on a cold compute cache the replay read + zero files and the grid rendered empty unless something healed first. Under + ADR D4 the pre-heal is deleted and the guarantee moves into the artifact: + the child's build carries the parent's cache node, so Buckaroo's replay of + the posted build regenerates the snapshot itself through ordinary cache + mechanics. This pins both halves at the same seam: the POST goes out with + the cache still cold (no discarded-result pre-execution), and replaying the + posted build dir yields the rows and lands the parent snapshot on disk. """ import shutil + from xorq.ibis_yaml.compiler import load_expr + from tallyman_core.paths import compute_cache_dir from tallyman_mcp.server import catalog_create from tallyman_xorq.build import list_entries @@ -362,7 +365,7 @@ def test_unit_ensure_session_self_heals_evicted_snapshot_on_cold_cache(project, child_h = list_entries(project)[0]["content_hash"] # most-recent build == chain # Cold start, as a fresh clone / not-yet-warmed entry has: evict every baked - # snapshot and clear the reconstruction lru the in-process build warmed. + # snapshot and clear the plan lru the in-process build warmed. shutil.rmtree(compute_cache_dir(project), ignore_errors=True) cached_result_expr.cache_clear() @@ -370,6 +373,8 @@ def test_unit_ensure_session_self_heals_evicted_snapshot_on_cold_cache(project, mgr.bound_port = 65000 mgr.proc = type("FakeProc", (), {"poll": staticmethod(lambda: None)})() + posted: dict = {} + class _Resp: status_code = 200 @@ -379,16 +384,24 @@ def raise_for_status(self): def json(self): return {"session": "sess-cold-heal"} - monkeypatch.setattr(mgr._client, "post", lambda *a, **kw: _Resp()) + def _capture_post(url, json=None, timeout=None): + posted.update(json or {}) + return _Resp() + + monkeypatch.setattr(mgr._client, "post", _capture_post) mgr.ensure_session(child_h, project) - # The heal must repopulate the snapshot the *build replay* reads — the path - # buckaroo's /load_expr resolves — before POSTing, so the cold grid isn't - # empty. ensure_session ran cached_result_expr(child_h), which recomputes the - # evicted parent's baked snapshot; assert that snapshot is back on disk. (The - # snapshot key embeds the build path, so this holds when build and read share a - # project root, i.e. the same-machine cold cache; cross-machine clone is #77.) + # The POST happened on a genuinely cold cache — no pre-heal materialised + # anything first. + assert posted.get("build_dir"), "no /load_expr POST captured" + assert not any(compute_cache_dir(project).rglob("result_cache/*.parquet")) + + # Replay the posted build exactly as Buckaroo does: load it and execute. The + # child's build carries the parent's cache node (D4), so the replay heals + # the evicted parent snapshot itself and the grid has rows. + replayed = load_expr(posted["build_dir"], cache_dir=compute_cache_dir(project)) + assert int(replayed.count().execute()) > 0 assert any(compute_cache_dir(project).rglob("result_cache/*.parquet")) diff --git a/tests/test_parquet_digest_order_probe.py b/tests/test_parquet_digest_order_probe.py index c16feab..22d3c0d 100644 --- a/tests/test_parquet_digest_order_probe.py +++ b/tests/test_parquet_digest_order_probe.py @@ -1,15 +1,15 @@ -"""#171 probe: can DataFusion's parallel scan/aggregation reorder a -parquet-sourced bake, and does the advisory fire correctly when it does? +"""#171 regression: a parquet-sourced bake's bytes are stable across heals. -Deliberately NOT a red test. Whether the reorder fires at this scale is the -empirical question the probe exists to answer — the source is written above +History: as a *probe*, this test demonstrated the drift — on pre-fix main, +DataFusion's parallel scan/aggregation reordered the bake on every heal above ``repartition_file_min_size`` (1 MiB; files below it are never byte-range -split, which is why small fixtures look deterministic — see -``plans/datafusion-scan-order-findings.md``) and the aggregate has one group -per ``order_id`` so the hash aggregation has room to vary. The assertions pin -the MECHANISM, not the outcome: ``verify_result_faithful`` must agree with -the byte comparison, and any mismatch must surface the advisory warning -rather than pass silently (ADR D5, ``plans/adr-read-path-loads-builds.md``). +split, which is why small fixtures looked deterministic — see +``plans/datafusion-scan-order-findings.md``): 3 heals produced 3 distinct +digests, none matching the build. That evidence flipped ADR D5 +(``plans/adr-read-path-loads-builds.md``) to the canonical sort-by-all-columns +bake, which this test now pins: every heal must reproduce the build's exact +bytes, ``verify_result_faithful`` must agree, and the UNFAITHFUL advisory must +stay silent. """ from __future__ import annotations @@ -39,10 +39,10 @@ def _big_agg_code(project: str) -> str: """ -def test_parquet_bake_digest_stability_probe(project, monkeypatch, caplog): +def test_parquet_bake_digest_stable_across_heals(project, monkeypatch, caplog): monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "cas") src = write_shoe_orders(data_dir(project) / "orders_big.parquet", n_rows=250_000, seed=7) - # Below repartition_file_min_size the file is never split and the probe + # Below repartition_file_min_size the file is never split and the test # proves nothing — fail loudly rather than pass vacuously. assert src.stat().st_size > 1_048_576, "fixture too small to provoke a partitioned scan" @@ -50,7 +50,6 @@ def test_parquet_bake_digest_stability_probe(project, monkeypatch, caplog): recorded = read_manifest(entry_dir(project, h)).result_digest assert recorded is not None - digests: set[str] = set() for _ in range(_HEALS): snap = baked_snapshot_path(project, h) if snap is not None and snap.exists(): @@ -60,16 +59,10 @@ def test_parquet_bake_digest_stability_probe(project, monkeypatch, caplog): cached_result_expr(project, h) # heal fires healed = baked_snapshot_path(project, h) assert healed is not None and healed.exists() - d = snapshot_file_digest(healed) - digests.add(d) - # Mechanism consistency: the audit must agree with the bytes. - assert verify_result_faithful(project, h) is (d == recorded) - if d != recorded: - # Drift occurred: it must be surfaced, not silent. - assert any("UNFAITHFUL" in r.getMessage() for r in caplog.records) - - # Document the empirical outcome in the test output either way. - print( - f"digest-order probe: {len(digests)} distinct digest(s) over {_HEALS} heals; " - f"build digest reproduced: {recorded in digests}" - ) + # The canonical sort (ADR D5, amended) makes the bake a deterministic + # total order: every heal reproduces the build's exact bytes. + assert snapshot_file_digest(healed) == recorded + assert verify_result_faithful(project, h) is True + # Scoped to tallyman.perf: xorq's startup logging captures the repo's + # uncommitted diff into an INFO record, which can itself contain the string. + assert not any("UNFAITHFUL" in r.getMessage() for r in caplog.records if r.name == "tallyman.perf") diff --git a/tests/test_result_cache.py b/tests/test_result_cache.py index b3cbfb3..13e9d6f 100644 --- a/tests/test_result_cache.py +++ b/tests/test_result_cache.py @@ -925,7 +925,10 @@ def execute(self): def _stub_plan(on_execute): # Stand in for the lru-memoised plan so the heal hits our controlled execute() # against the REAL evicted path — the only thing the except arm reads. - monkeypatch.setattr(rc, "_resolve_result_plan", lambda _proj, _h: ("baked", _FakeBaked(on_execute), p)) + fake = _FakeBaked(on_execute) + monkeypatch.setattr( + rc, "_resolve_result_plan", lambda _proj, _h: rc._ResultPlan("baked", fake, fake, p) + ) # Landed: the peer's rename completed (snapshot back on disk) but our execute still # raised on the vanished shared tmp — swallow it and read the materialised file. @@ -1425,15 +1428,20 @@ def test_deterministic_udf_entry_is_not_structurally_nondeterministic(project, o assert recipe_is_structurally_nondeterministic(project, h) is False -def test_self_heal_warning_labels_structural_for_baked_literal(project, orders_parquet, monkeypatch, caplog): - # #88 part 2, end to end: an entry that bakes a random literal reconstructs to a - # graph whose snapshot key differs from the build-time one, so its very first cold - # read self-heals to bytes that differ from the recorded digest. The faithfulness - # warning must fire AND attribute the drift to the structural axis (#88), not - # execution — the literal genuinely moves the graph hash between imports. +def test_baked_literal_entry_reads_faithfully_from_frozen_build(project, orders_parquet, monkeypatch, caplog): + # Inversion of the pre-#163 behavior this test used to pin. On main, reading + # an entry whose recipe bakes a random literal re-imported expr.py, re-rolled + # the literal, derived a *different* snapshot key, and self-healed to bytes + # that weren't the recorded ones — so the read had to fire an UNFAITHFUL + # warning with the structural (#88) attribution. Under the contract the read + # loads the frozen build, where the literal is fixed forever: the original + # snapshot resolves, nothing heals, no warning fires, and the entry is + # byte-faithful (I1) despite the structurally nondeterministic recipe. The + # nondeterminism now surfaces only where it belongs — re-minting — and the + # detector test above still covers the diagnostic. import logging - from tallyman_xorq.result_cache import cached_result_expr + from tallyman_xorq.result_cache import cached_result_expr, verify_result_faithful monkeypatch.setenv("TALLYMAN_SOURCE_IDENTITY", "off") monkeypatch.setenv("TALLYMAN_PROJECT", project) @@ -1443,10 +1451,11 @@ def test_self_heal_warning_labels_structural_for_baked_literal(project, orders_p caplog.clear() with caplog.at_level(logging.WARNING, logger="tallyman.perf"): - df = cached_result_expr(project, h).execute() # reconstructs a new graph → self-heals + df = cached_result_expr(project, h).execute() assert len(df) > 0 + assert verify_result_faithful(project, h) is True msgs = [r.getMessage() for r in caplog.records if r.name == "tallyman.perf"] - assert any("UNFAITHFUL" in m and h in m and "structural (#88)" in m for m in msgs), msgs + assert not any("UNFAITHFUL" in m for m in msgs), msgs # --------------------------------------------------------------------------- diff --git a/tests/test_serve.py b/tests/test_serve.py index d63dafb..7f31a59 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -105,7 +105,7 @@ def test_notify_recalc_reloads_sessions_and_invalidates_caches(project: str, mon import tallyman_companion.app as appmod invalidated: list[bool] = [] - monkeypatch.setattr(appmod, "_invalidate_reset_caches", lambda: invalidated.append(True)) + monkeypatch.setattr(appmod, "_invalidate_reset_caches", lambda project=None: invalidated.append(True)) stub = _TrackingBuckaroo() app = create_app(project, buckaroo=stub) c = TestClient(app)