Skip to content

fix(xorq): a recipe reads under compute_cache only what tallyman handed it (#228) - #246

Open
paddymul wants to merge 3 commits into
feat/adr-007-009-cache-redesignfrom
fix/228-compute-cache-read-exemption
Open

paddymul wants to merge 3 commits into
feat/adr-007-009-cache-redesignfrom
fix/228-compute-cache-read-exemption

Conversation

@paddymul

@paddymul paddymul commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #189: the base is feat/adr-007-009-cache-redesign (tip 1f8cb02), not main.

Fixes #228

This closes the issue only once #189 reaches main.

Terms: a snapshot is the parquet file tallyman writes for a worthy entry (one it materializes), at compute_cache/result_cache/<content_hash>.parquet. A cheap entry writes no file; reading it runs its small frozen plan over the files it reads. A source version is the entry an import mints for a raw file (ADR-011 D1), and its snapshot sits in the same directory. A parent edge is the {hash, ref, follow} record in manifest.parents that tracked_expr_from_alias and pinned_expr_from_alias write while a recipe runs. Reconstruction is result_cache._recipe_expr re-running an entry's recipe under the _RECONSTRUCTING contextvar, which the unfaithful-heal diagnostic does.

Problem

build._raw_parquet_read_check refused an xo.deferred_read_parquet of a file tallyman did not write, but it skipped every path under compute_cache/. So a recipe could read an entry's snapshot by its path, which names the entry by a bare content hash. ADR-011 D5 (every parent edge names an alias) refuses that in pinned_expr_from_alias. The build recorded no parent edge, so the child never went stale when the parent's alias moved. A source version's snapshot read the same way, and so did any parquet file copied under compute_cache/, with no import.

The exemption was there because a legitimate read of a worthy parent is itself a deferred_read_parquet of its snapshot: result_cache.cached_result_expr returns one, and tracked_expr_from_alias, pinned_expr_from_alias and a promoted diff's build_diff_expr all read through it.

Fix

  • Record what tallyman hands out. parent_capture gets a second collector, begin_reads / note_reads / end_reads, beside the parent-edge one. _build_and_persist arms both while the recipe runs. cached_result_expr notes the files behind each result it returns: the snapshot path for a worthy entry, and _ResultPlan.reads for a cheap one, so a cheap parent's plan brings its own parents' snapshots with it. Recording in cached_result_expr covers all three readers in one place, which is what the issue suggests.

  • Allow exactly those. _raw_parquet_read_check(expr, project, handed_out) compares resolved paths. A read in handed_out passes. Any other read_parquet is refused: outside compute_cache/ with the message it had before, and inside it with a new one from _cache_read_refusal.

  • Name what to write instead. A file under result_cache/ whose stem is an entry with a manifest is a snapshot. The message names the alias version that holds it (aliases.version_of_hash), for a catalog alias and a source alias alike. For example:

    the recipe reads …/result_cache/9d9b9c43ff05.parquet with xo.deferred_read_parquet, which is not allowed: it is the snapshot of agg-v1 (entry 9d9b9c43ff05). Reading it by its path names that entry by a bare content hash (ADR-011 D5), so the build would record no parent edge and the new entry would not go stale when agg moves. Read it with pinned_expr_from_alias('agg-v1'), or with tracked_expr_from_alias('agg') to follow the alias.

    An entry no alias holds (a catalog_run scratch entry) gets the catalog_alias-first advice pinned_expr_from_alias already gives for a bare hash. Any other file under compute_cache/ gets catalog_import_source('<path>', '<alias>').

  • Reconstruction records nothing. cached_result_expr skips note_reads under _RECONSTRUCTING, the same guard the readers use for note_parent. A heal during a build can run reconstruction, and its reads belong to the recipe it re-runs, not to the one being built.

The paths the issue asked to keep working

  • Recalc replay (recalc._replay_cone → build_and_persist) is an ordinary build, so the readers record as they run.
  • Reconstruction never calls the check. The guard above only keeps it from adding paths to an outer build's set.
  • A source entry's generated recipe (read_project_file under _SOURCE_ENTRY) is built by source_import._mint, which never calls _raw_parquet_read_check. A recalc naming a source entry as a root is refused earlier, by io._source_entry_read, as before. test_io.py::test_read_project_file_still_works_inside_a_generated_source_recipe and test_source_import.py pass unchanged.

No new .execute() or to_pyarrow_batches() call in src/ (for #118).

Tests

In tests/test_row_order.py, after the ADR-008 D12 raw-parquet tests.

Failing on 1f8cb02, each with DID NOT RAISE (the build succeeds):

  • test_a_raw_read_of_an_entry_snapshot_is_a_build_error_naming_its_version: the error names pinned_expr_from_alias('agg-v1').
  • test_a_raw_read_of_a_source_version_snapshot_is_a_build_error_naming_the_source_alias: names pinned_expr_from_alias('orders_src-v1').
  • test_a_raw_read_of_a_parquet_file_copied_under_compute_cache_is_a_build_error: a copy in result_cache/; names catalog_import_source and the path. The recipe aggregates, so it is worthy and does not fail on the unfixed code for a missing __row_order instead.

Guards, passing before and after, in their own commit:

  • a tracked child and a pinned child of a worthy entry, with their edges;
  • a child through a cheap parent, whose plan reads its own parent's snapshot;
  • a promoted diff built through catalog_promote_diff, then its recipe replayed through build_and_persist to the same hash;
  • a revise of agg whose auto-recalc replays agg → big (cheap) → top, with both rebuilt and their edges on the new heads;
  • _reconstructed_hash of a cheap entry equal to its content hash.

The promoted-diff guard lifts primary_key.PK_SEARCH_BUDGET_S from 1s to 60s. On its first local run, in a newly synced venv, catalog_promote_diff answered "primary key search … exceeded 1s". A second fresh venv reproduced it. cProfile put 3.27s of a 3.70s search in _imp.create_dynamic: _column_stats → execute() → ibis formats/pandas.convert_table imports geopandas, pyproj and shapely, and it was the first load of those 13 C extensions from the new venv. Warm, the whole search takes 0.12–0.16s. The builds before it never convert a result to pandas, so the import lands inside the budget. test_promote_diff.py has the same exposure. In a full-suite run, earlier tests in the same process have already converted a result to pandas, so the import is paid before any key search.

How it was built

  1. 241b454 adds the three failing tests, and efd5677 the six guards. CI run 36017746801: ruff passed, and the fast suite had 3 failed and 960 passed. The three failures were exactly the new tests, each DID NOT RAISE; the guards passed.
  2. fe9290c adds the fix. CI run 36019075904: ruff, the fast suite (963 passed) and the integration suite (7 passed) all pass.

Checked locally before each push: the three tests fail on the unfixed code with DID NOT RAISE and the guards pass. With the fix, uvx ruff check passes, and ruff format --diff shows the same pre-existing drift on each touched file as on 1f8cb02, none in the new lines (there is no .pre-commit-config.yaml on this base). The fast suite had 956 passed, 6 skipped and 1 failed: test_fouc::test_unknown_api_path_404s, which answers 503 "React app not built" because the worktree has no packages/app/dist. The integration suite had 7 passed. Both local runs set TALLYMAN_COMPANION_URL to a closed port, because the MCP tools' _notify POSTs to 127.0.0.1:7860 by default, and a real companion was listening there.

Docs

Not edited here, as in #222 and #223: the docs on this base predate ADR-011, and #216 describes the system as built. Once this lands, its architecture-new.md needs two changes. In section 4, "What a recipe may read", the row "calls xo.deferred_read_parquet on a file outside compute_cache/" becomes "a file tallyman did not hand the recipe". In section 13, the #228 line goes.

Not in this PR

  • A recipe that calls cached_result_expr(project, hash) or build_diff_expr(a_hash=..., b_hash=...) itself still names an entry by a bare hash with no edge. Those are the promoted diff's generated recipe, which the architecture doc lists as a deliberate exception, and an internal function. Closing that door means a promoted diff's recipe naming alias versions, which is a separate change.
  • a pinned child and a tracked child with the same query are one entry, so the first build's edge wins and a pin can move #229: a tracked and a pinned child with the same query are still one entry. This PR removes only the by-hash way into that collision.
  • An existing entry built by reading a snapshot by path now fails to replay in a recalc, with the new message. Rebuilding the corpus is the remedy.

🤖 Generated with Claude Code

paddymul and others added 3 commits September 24, 2026 11:05
Three failing cases: agg-v1's snapshot read by its path (the error must name
pinned_expr_from_alias('agg-v1')), a source version's snapshot read by its
path (must name orders_src-v1), and a parquet file copied into result_cache/
(must name the import). On 1f8cb02 all three build.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Guards for the fix, passing on 1f8cb02: a tracked child and a pinned child of
a worthy entry, a child through a cheap parent (whose plan reads its own
parent's snapshot), a promoted diff built and replayed, a recalc of a chain
through a cheap entry after its parent is revised, and reconstruction of a
cheap entry.

The promoted-diff test lifts the 1s primary-key search budget. The search's
first execute() in a process imports geopandas through ibis's pandas
conversion, and the first load of those C extensions in a new venv took
3.3s here (cProfile: _imp.create_dynamic), against 0.16s for the whole
search once warm.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed it (#228)

_raw_parquet_read_check skipped every path under compute_cache/, so a recipe
could read an entry's snapshot by its path: a bare content hash, which
ADR-011 D5 refuses, with no parent edge recorded.

cached_result_expr now records the files behind each result it returns
while a recipe runs (parent_capture.note_reads): a worthy entry's snapshot,
or the reads of a cheap entry's plan. tracked_expr_from_alias,
pinned_expr_from_alias and build_diff_expr all go through it. The check
allows exactly those paths. Any other deferred_read_parquet under
compute_cache/ is refused: a known snapshot names the alias version to read
instead (pinned_expr_from_alias('<alias>-v<N>')), or catalog_alias when no
alias holds the entry, and any other file names catalog_import_source.
A reconstruction (_RECONSTRUCTING) records nothing, as it records no edge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant