fix(cache): single-flight the cold-entry snapshot heal (#79)

[paddymul]

Fixes #79.

Problem

cached_result_expr heals an evicted baked snapshot on the request thread with a bare check-then-write and no single-flight lock (src/tallyman_xorq/result_cache.py:321-322):

if not path.exists():
    baked.count().execute()  # evicted snapshot: repopulate once, then read

_resolve_result_plan is @functools.lru_cached, but the memo is on the plan, not the heal — so concurrent readers share one baked op, both miss the evicted snapshot, and both run the heal. The loser raises:

• In-process (two browser tabs, api_data + api_diff_data): both .execute() the shared op → DataFusion RuntimeError: "Already borrowed".
• Cross-process (MCP build vs companion heal sharing compute_cache): xorq ParquetStorage.put writes a fixed <key>.parquet.tmp then renames it (storage.py:134-137); the loser hits the missing tmp → FileNotFoundError.

buckaroo_lifecycle.ensure_session calls cached_result_expr before the session lock inside except Exception: log.debug(...) (buckaroo_lifecycle.py:528-533), so the loser's failure is swallowed and Buckaroo replays against a half/never-written snapshot — an empty/wrong grid.

The #104 removal of the on-demand result.parquet layer dissolved the issue's cheap-entry path (ensure_result is gone); this PR closes the remaining expensive-entry heal race.

Change

Serialize the heal per (project, content_hash): a keyed threading.Lock (a dict under a master lock) wraps the exists()-check + heal in cached_result_expr, re-checking exists() inside the lock so the second caller waits, then reads — "first request serviced, the rest get the materialised snapshot." For the cross-process case (no shared in-process lock), the heal's execute() catches FileNotFoundError/ValueError and retries once if the snapshot landed (a peer process won the rename), else re-raises.

Status — TDD split

Per the repo's test-vs-fix rule, the first commit is the failing test (watched red on CI — RuntimeError: Already borrowed); the fix follows in the next commit (CI green). test_concurrent_cold_reads_of_expensive_entry_do_not_race fires two barrier-synced readers at the same evicted entry over a few rounds.

Context

Surfaced during a multi-agent review of #74. Pre-#73 result.parquet was written once at build, so reads were never materialisers; #73 moved materialisation onto the read path, which opened the race. The fixed-tmp root is upstream in xorq ParquetStorage; this guards tallyman's side.

🤖 Generated with Claude Code

[paddymul]

Scope note found while building this fix — a separate concurrent-read race remains

This PR single-flights the materialisation (heal) race that #79 describes, and verifies it: two barrier-synced cold readers that trigger the heal no longer collide, and the snapshot is materialised exactly once.

But while building it I confirmed a separate, broader concurrency bug that this fix does not address: concurrent .execute() on the shared single DataFusion backend raises RuntimeError: Already borrowed for any two concurrent reads — warm or cold, heal or no heal. Measured directly:

• two concurrent cached_result_expr(...).execute() on a warm entry → Already borrowed;
• two concurrent heals (no .execute() in the workers), with this fix → clean, snapshot written once.

api_data is a threadpooled sync endpoint with no execute lock, so the "two browser tabs → 500" symptom in #79's intro has two distinct causes: the materialisation race (fixed here) and this shared-backend concurrent-.execute() race (not fixed here — it's a backend-thread-safety / connection-pooling concern, arguably rooted in xorq's single shared connection).

The test is deliberately scoped to the materialisation race (workers build the expr but don't execute it) so it isolates what this fix actually guarantees. The shared-backend concurrent-read race should be tracked separately.

[paddymul]

Filed the shared-backend concurrent-.execute() follow-up described above as #118.

[paddymul]

Code review — single-flight the cold-entry snapshot heal (#79)

CI is green and the TDD split is clean: the test commit (1733345) was watched red on CI with exactly the predicted RuntimeError('Already borrowed'), and the fix commit (e8d294b) turned it green.

Overview

cached_result_expr heals an evicted baked snapshot on the read thread. Because _resolve_result_plan is lru_cached, concurrent cold readers share one baked op, so a bare check-then-execute() lets both fire the materialisation: in-process they collide on the shared DataFusion op (Already borrowed), cross-process they collide on xorq's fixed <key>.parquet.tmp rename (FileNotFoundError). The loser's failure is swallowed by ensure_session, yielding an empty grid. The fix is a per-(project, content_hash) threading.Lock (keyed dict under a master guard) with double-checked path.exists(), plus a cross-process fallback that tolerates a peer winning the rename.

Correctness — looks right

• Double-checked locking is implemented correctly: the outer unlocked exists() keeps the warm path lock-free, the inner re-check under the lock means the waiter sees the winner's completed write and skips the redundant execute(). That kills the in-process Already borrowed.
• Per-entry locking is the right granularity — distinct entries never block each other, and _HEAL_LOCKS_GUARD is held only for the dict get/create, never across the expensive execute().
• No deadlock / lock-ordering risk: the guard is released before the heal lock is acquired; they're never nested.
• Lock held during execute() is intentional and correct — that is the single-flight.

Issues / suggestions

• The cross-process branch (except (FileNotFoundError, ValueError)) has no test. The in-process race is well covered, but the entire except body — re-check exists(), read-or-re-raise — is exercised only in the multi-process scenario the unit test explicitly scopes out. That's the trickier half of the change and it's landing untested. It can be covered without real multiprocessing by monkeypatching baked.count().execute to raise FileNotFoundError once: assert it re-raises when the path is absent, and falls through to a read when the snapshot is pre-created. Worth adding given this branch guards a real production path (MCP build vs companion heal).
• ValueError is a broad catch over a full DataFusion query. baked.count().execute() can raise ValueError for reasons unrelated to the tmp-rename race. The if not path.exists(): raise gate makes this safe in practice (a genuine failure won't have materialised the snapshot, so it re-raises), but it means a real ValueError that coincides with an existing snapshot would be silently swallowed and the snapshot read. Acceptable given the gate, but a one-line note that the gate — not the exception type — is what makes this safe would age better. The docstring's "ValueError: At least one path is required" rationale is about the deferred read, not baked.count(), so that connection reads a little loose.
• Residual cross-process window vs. the PR wording. The PR body says the heal "retries once if the snapshot landed," but the code doesn't retry execute() — it re-checks exists() once and reads or re-raises. If a peer is mid-rename (tmp written, final not yet in place) at the instant of the check, this re-raises spuriously. Narrow, and the real root is the upstream xorq fixed-tmp bug correctly called out-of-scope — but "retries once" oversells what the code does (no sleep/re-poll).
• _HEAL_LOCKS never evicts. The comment acknowledges it's bounded by catalog size and stale locks are harmless — fine here. Note it diverges from _resolve_result_plan's lru_cache(maxsize=256): the plan memo evicts, the lock dict doesn't. Not worth fixing; just flagging the asymmetry.
• Minor behavior change: the self-heal perf-debug line now fires only on the thread that actually heals (inside the inner exists() guard); a waiter that finds the snapshot already present emits nothing. Arguably more correct, but it's a silent change to the perf-log cadence (perf/caching: log the cache admission decision — structural cache_worthy verdict + measured value-per-byte (compile_seconds, bytes) side by side — so the #30 flip is data-driven #87) — assumed intended.

Conventions / security

• Matches the file's idiom (module-level guarded registry, dense comments tying back to issue numbers, perf_log.debug tags), threading import placed correctly, no security surface. ruff, fast suite, integration suite, and perf report all green.

Verdict

Correct, minimal, and TDD-clean — good to merge. The one change I'd push for first is a test for the cross-process except branch: it's the only untested logic in the diff and the part most likely to regress unnoticed. The ValueError-scope and comment-wording notes are polish, not blockers.

🤖 Generated with Claude Code