fix(server): /load_expr cache_dir — read the embedder's baked snapshots (#972)

[paddymul]

Fixes #972. POST /load_expr rehydrated builds with no cache_dir, so every CachedNode resolved to ~/.cache/xorq. An embedder's baked snapshots were never read, and the first query re-executed the cached sub-graph and wrote a second copy there.

What

• /load_expr takes an optional cache_dir, which must be an absolute path (400 invalid_cache_dir otherwise). load_expr_build_dir(build_dir, cache_dir=...) redirects every parquet-backed cache node to it (redirect_cache_dir). Unset, behaviour is unchanged.
• The redirect uses xorq's replace_nodes rather than load_expr(cache_dir=...). xorq's ExprLoader.replace_base_path doesn't descend into CachedNode.parent, so a nested node keeps base_path=None. Since a cache key hashes its parent's storage, the outer node then misses too.
• cache_dir is used as given, not resolved. xorq hashes base_path into the key of every cache node above another, so it has to be spelled the way the embedder baked with. tallyman bakes with the unresolved compute_cache_dir.
• After loading, the handler runs heal_missing_snapshots in its own firstpull.cache_heal span (attribute snapshots_written), before the dataflow queries anything. The heal follows xorq's lazy read path: it stops at a cache node whose snapshot exists, and writes a missing node's inner snapshots before the node itself. The root's snapshot gets xorq's provenance metadata. Each write is logged, because a heal against a baked cache usually means cache_dir is spelled differently from the bake.
• The heal writes each snapshot itself, to a temp file unique to that write, then moves it into place with os.replace. xorq's ParquetStorage.put writes through a fixed <key>.parquet.tmp, so two writers of one key can corrupt the file. With a unique temp file, a concurrent writer costs duplicate work but can't corrupt the snapshot. No lock is taken, so nothing an embedder holds can stall the server's IOLoop, and embedders don't have to follow any locking protocol.
• SessionState.cache_dir stores the value. A re-POST that omits cache_dir keeps the session's value, and one that changes it reloads. /reload_expr reuses session.expr, which already carries the redirect.
• The warm-session early-exit also requires session.backend == "xorq". /load leaves build_dir in place, so xorq, then pandas, then xorq again on one session used to return the pandas metadata.
• build_dir_not_found (404) is now decided by an isdir check before loading. xorq raises OSError for a missing build dir, so the old except FileNotFoundError mapping never fired, and a FileNotFoundError from the heal would have been reported as a missing build dir.

{"session": "...", "build_dir": "/path/to/build", "cache_dir": "/project/artifacts/catalog/compute_cache"}

Tests

TestLoadExprCacheDir in tests/unit/server/test_load_expr.py, using cached builds and xorq's default cache dir patched to a temp dir:

• nested and outer nodes both resolve under cache_dir and find their snapshots; without cache_dir, default resolution is unchanged
• POST with cache_dir writes nothing to the default dir and nothing new to the host cache
• a warm re-POST with a new cache_dir reloads; one that omits it keeps the session's value, warm or forced
• a relative or non-string cache_dir is a 400, a missing build dir is a 404, and a FileNotFoundError from the heal is a 500
• the heal leaves another writer's <key>.parquet.tmp alone and leaves no .lock files
• a missing inner snapshot under a baked outer one is not recomputed
• in a DAG where a cached filter is shared, nothing is written through xorq's put
• a healed root snapshot has the same provenance as a baked one, each heal write is logged, and the heal has its own span

Plus test_warm_exit_requires_a_xorq_session in TestLoadExprPerfFixes.

The first round of tests landed in df7ef98 and failed on CI before 5517352. The review round's tests landed in 9b8716a and failed on CI (run 36010547106, on the main merge 4e30137) before the fix in ac2262b. Tests pass on the locked xorq 0.3.25 and on xorq 0.4.4 (the Max Versions job).

🤖 Generated with Claude Code

[github-actions]

📦 TestPyPI package published

pip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.15.6.dev36011242915

or with uv:

uv pip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.15.6.dev36011242915

MCP server for Claude Code

claude mcp add buckaroo-table -- uvx --from "buckaroo[mcp]==0.15.6.dev36011242915" --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo-table

📖 Docs preview

🎨 Storybook preview

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df7ef98fc3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Exercise the cache miss instead of only deserializing

The worker only calls load_expr_build_dir, whose implementation simply returns xorq.api.load_expr(build_dir) and therefore rehydrates a bare expression; cached snapshots are read or written when the expression is executed (which this test does only after releasing the lock). Consequently, a correct lazy implementation should not block on this lock, so this test will remain red or require an unnecessary eager cache fill. Exercise the POST/dataflow path or execute the expression in the worker while the lock is held.

Useful? React with 👍 / 👎.

[paddymul]

Declining — this reads the tests-only commit (df7ef98), the red half of the TDD split. There load_expr_build_dir was indeed a bare load_expr(build_dir). The fix commit 5517352 makes it eager: load_expr_build_dir(build_dir, cache_dir=...) redirects the cache nodes and then calls heal_missing_snapshots(expr), which writes each missing snapshot under the flock. The worker blocks inside that call, and the test passes on xorq 0.3.25 and 0.4.3.

The eager fill is the intended contract, not an artifact of the test:

• Deferring to execute time isn't reachable from here. The write happens in xorq's _register_and_transform_cache_tables, which calls cache.set_default unlocked; the only point buckaroo controls is before the expression is handed off.
• /load_expr executes in the same request regardless — get_xorq_metadata calls _expr_count(expr) and the dataflow runs the stat pipeline (handlers.py:504-520). The heal moves that compute a few ms earlier and wraps it in the lock. With the snapshots present it is existence checks only.

test_load_expr_reads_embedder_snapshots covers the POST path: it asserts nothing lands in the default cache dir and the host cache gains no files.

[paddymul]

/code-review of #973: findings and resolution

The failing tests went in first as 9b8716a. CI run 36010547106, on the main merge 4e30137, showed all 11 failing on xorq 0.3.25 and 0.4.4. The fix is ac2262b, and the test cleanup is 541b7a6.

1. Heal order with a shared cache node. reversed(walk_nodes(...)) is not descendants-first in a DAG, so a shared inner snapshot was written inside the outer node's lock with no lock of its own. Fixed in ac2262b: the heal recurses into a missing node's parent before writing the node, so a shared node is always written first. Test: test_heal_writes_shared_inner_before_outer.
2. flock with no timeout blocked the IOLoop. Fixed in ac2262b by removing the lock (see 10).
3. Inner snapshots under a baked outer one were recomputed. Fixed in ac2262b: the walk stops at any cache node whose snapshot exists, as xorq's execute() does. Test: test_heal_leaves_inner_snapshot_under_baked_outer.
4. The warm-session early-exit didn't check for a xorq session. This predates the PR. Fixed in ac2262b: the exit now requires backend == "xorq". Test: test_warm_exit_requires_a_xorq_session.
5. The outer cache key depends on how cache_dir is spelled. Partly addressed. cache_dir is still used as given, on purpose: tallyman bakes with the unresolved compute_cache_dir, so resolving on the server would make every outer node miss. Each heal write is now logged with its path and counted in the span's snapshots_written, so a spelling mismatch shows up. Test: test_heal_logs_each_snapshot_it_writes.
6. cache_dir wasn't validated. Fixed in ac2262b: anything other than an absolute string path gets 400 invalid_cache_dir. Test: test_cache_dir_must_be_an_absolute_path.
7. A FileNotFoundError from the heal was reported as a 404. Fixed in ac2262b: the 404 is now decided by an isdir check before loading. This also fixes an older bug: xorq raises OSError for a missing build dir, so the old except FileNotFoundError never fired and a missing dir returned a 500. Tests: test_heal_file_not_found_is_a_load_error, test_missing_build_dir_is_not_found.
8. cache_dir was dropped when a re-POST omitted it. Fixed in ac2262b: an omitted cache_dir now keeps the session's value. Test: test_repost_without_cache_dir_keeps_the_sessions.
9. A healed root snapshot had no provenance. Fixed in ac2262b: the root gets build_provenance_metadata, as xorq's executor does. Test: test_heal_stamps_root_provenance.
10. Altitude: the lock only covered load-time writes, and the real cause is the fixed tmp name. Fixed in ac2262b: the heal writes each snapshot to a temp file unique to that write and moves it into place with os.replace, with no lock. Test: test_heal_writes_through_its_own_tmp_file. One gap remains: query-time writes through xorq's executor, such as a snapshot deleted or TTL-expired after load, still use xorq's fixed tmp. That is xorq's code, and this PR doesn't change it.
11. The lock test was timing-dependent. No longer applies: the test was removed along with the lock in ac2262b.
12. calc_key ran again for each node. Fixed in ac2262b: each visited node's key is computed once and reused for its write, and nodes under an existing snapshot are never keyed.
13. Heal time counted inside firstpull.expr_load. Fixed in ac2262b: the heal has its own firstpull.cache_heal span, and docs/perf-testing-guide.md is updated. Test: test_cache_heal_has_its_own_span.
14. Imports inside test functions. Fixed in 541b7a6.
15. Redundant skipif and leftover .lock files. Both went away with the lock test and the lock in ac2262b.

Also checked outside the unit tests:

• Two processes healing the same missing snapshots at once both finished, leaving two valid snapshots and no stray files.
• Against a real server on :8791:
  • A baked cache was served with no writes.
  • A deleted outer snapshot was healed, with provenance and a log line naming it.
  • A re-POST without cache_dir took the warm-session exit.
  • A relative cache_dir returned 400, and a missing build dir returned 404.

🤖 Generated with Claude Code