fix(core): prevent catalog races in concurrent same-context transforms

[ttw225]

Problem

apply_wren_on_ctx builds derived DataFusion session states from a base SessionContext. SessionStateBuilder::new_from_existing shares the base context's top-level catalog_list.

register_table_with_mdl then registers a new catalog before adding its models and views. Concurrent calls using the same base context can therefore replace or observe each other's partially constructed catalog.

This can cause:

• table not found
• table already exists
• cross-call catalog/schema contamination
• mutation of the base context's catalog list

Fix

Create a private top-level catalog-list snapshot for each apply_wren_on_ctx call and attach it to the derived session states.

The isolation contract is:

• Top-level catalog membership and replacement are private to one apply call.
• The two session states created within one apply call share the same private list.
• Catalog providers that already exist at snapshot time remain Arc-shared, preserving access to live physical catalogs.
• Callers must complete top-level physical catalog registration before starting a transform.

An internal invariant check returns a DataFusion internal error if the final derived state does not retain the per-call private list.

Tests

Added coverage for:

• Preserving a sentinel catalog on the base context while the derived context receives the current MDL model and view.
• Concurrent calls using one base context and catalog key, with unique marker columns detecting cross-call contamination.
• Excluding top-level catalogs registered on the base context after the snapshot.
• Retaining live visibility into providers that existed before the snapshot.

The concurrent stress test uses eight OS threads with current-thread Tokio runtimes and repeatedly contends on the same catalog key.

Verification

• cargo fmt --all -- --check
• cargo check --all-targets
• RUST_MIN_STACK=8388608 cargo test --lib --tests --bins — 147 core tests and sqllogictests passed
• cargo clippy --all-targets --all-features -- -D warnings
• Concurrent isolation stress test repeated successfully

Related

#2485 exposed this latent race by allowing concurrent Python calls to enter the Rust execution path. This PR fixes the underlying catalog-list isolation in wren-core and can merge independently.

The per-context call_lock in #2485 remains in place. Removing it and measuring same-context throughput will be a separate follow-up after both PRs land.

Summary by CodeRabbit

• Bug Fixes
  • Improved catalog isolation during semantic model application by snapshotting the top-level catalog list for derived contexts.
  • Prevented concurrent applications on the same base context from interfering with each other.
  • Ensured derived contexts keep a consistent view of newly added models/views while still reflecting live updates inside catalogs that existed at apply time.
• Tests
  • Added contract-style coverage for catalog isolation and visibility, including concurrent stress scenarios and snapshot immutability.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6f72967a-795d-4764-b87a-97f6a897c859

📥 Commits

Reviewing files that changed from the base of the PR and between ca7ca52 and 60ade8f.

📒 Files selected for processing (2)

• core/wren-core/core/src/mdl/context.rs
• core/wren-core/core/src/mdl/mod.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• core/wren-core/core/src/mdl/context.rs

---

Walkthrough

Wren context application now clones top-level catalog membership into a private catalog list, shares existing catalog providers, validates catalog-list identity, and adds concurrent contract tests for isolation and visibility semantics.

Changes

Catalog isolation

Layer / File(s)	Summary
Catalog snapshot and session wiring core/wren-core/core/src/mdl/context.rs	Adds catalog-list cloning, wires the private snapshot into SessionStateBuilder, and verifies the resulting session retains the same catalog-list Arc.
Catalog isolation contract tests core/wren-core/core/src/mdl/mod.rs	Tests base-context immutability, concurrent-call isolation, post-apply catalog exclusion, and visibility of tables added to existing catalogs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant apply_wren_on_ctx
  participant CatalogProviderList
  participant SessionStateBuilder
  participant SessionState
  apply_wren_on_ctx->>CatalogProviderList: clone top-level catalogs
  CatalogProviderList-->>apply_wren_on_ctx: private catalog-list Arc
  apply_wren_on_ctx->>SessionStateBuilder: configure catalog list
  SessionStateBuilder-->>SessionState: build derived state
  apply_wren_on_ctx->>SessionState: validate Arc identity

Loading

Poem

I’m a rabbit guarding catalogs bright,
Cloning their names by moonlit light.
Old shelves share tables, new shelves stay apart,
Threads race clean with a careful heart.
Tests thump softly: isolation’s right!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main fix: preventing catalog races during concurrent transforms on the same context.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

core/wren-core/core/src/mdl/context.rs (1)

57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the registration precondition on apply_wren_on_ctx.

The correctness-critical requirement to finish top-level physical catalog registration is currently documented only on a private helper. Add it to the public function’s rustdoc so callers see the snapshot contract.

Also applies to: 110-113

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren-core/core/src/mdl/context.rs` around lines 57 - 62, Update the
public function `apply_wren_on_ctx` rustdoc to explicitly state that callers
must complete top-level physical-catalog registration before starting a
transform and that its catalog enumeration is only a best-effort, non-atomic
snapshot. Keep the existing private-helper documentation unchanged.

core/wren-core/core/src/mdl/mod.rs (1)

5294-5333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the replacement half of the snapshot contract.

This verifies post-apply additions, but not replacement of a catalog that existed at apply time. Replace an existing base catalog after applying, then assert the derived context retains the original Arc while the base exposes the replacement.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/wren-core/core/src/mdl/mod.rs` around lines 5294 - 5333, Extend the test
new_top_level_catalog_after_apply_is_absent_from_derived_ctx to register a
catalog before apply, retain its original Arc, replace that catalog on base_ctx
after apply, and assert base_ctx exposes the replacement while derived_ctx still
exposes the original Arc. Preserve the existing post-apply addition assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@core/wren-core/core/src/mdl/context.rs`:
- Around line 57-62: Update the public function `apply_wren_on_ctx` rustdoc to
explicitly state that callers must complete top-level physical-catalog
registration before starting a transform and that its catalog enumeration is
only a best-effort, non-atomic snapshot. Keep the existing private-helper
documentation unchanged.

In `@core/wren-core/core/src/mdl/mod.rs`:
- Around line 5294-5333: Extend the test
new_top_level_catalog_after_apply_is_absent_from_derived_ctx to register a
catalog before apply, retain its original Arc, replace that catalog on base_ctx
after apply, and assert base_ctx exposes the replacement while derived_ctx still
exposes the original Arc. Preserve the existing post-apply addition assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a5e83453-c573-4b76-a1ff-8686d601e428

📥 Commits

Reviewing files that changed from the base of the PR and between bb35d54 and ca7ca52.

📒 Files selected for processing (2)

• core/wren-core/core/src/mdl/context.rs
• core/wren-core/core/src/mdl/mod.rs

[goldmedal]

Nice fix — the root cause is real: new_from_existing moves catalog_list: Some(existing.catalog_list), so the derived state shares the base context's list Arc and register_table_with_mdl's register_catalog mutates it. The private per-call snapshot is the right level to isolate it, and having both derived states share the same private Arc (enforced by the Arc::ptr_eq invariant) is exactly what keeps the analyzer-rule state and the registration ctx consistent.

One thing worth calling out for the consumer side (wren-core-py), which is the path that actually depends on this behavior:

register_parquet / register_csv document "Tables registered on base_ctx are visible to exec_ctx via shared catalog." Before this change that was trivially true — exec_ctx literally shared base_ctx's list. After this change exec_ctx holds a private snapshot, so that contract now survives only via the Layer-2 semantics (internals of catalogs that existed at snapshot time stay Arc-shared): register_parquet/csv target base_ctx's default catalog, which exists at apply time, so mutations remain visible.

Two implications:

1. The invariant is now load-bearing but implicit on the Python side: physical tables must land in a catalog that already exists at apply time. If a caller ever registered a new top-level catalog on base_ctx after exec_ctx is built, it would silently not be visible (it postdates the snapshot). Might be worth mirroring the "complete top-level physical-catalog registration before starting a transform" note onto the Python methods too.

2. There's currently no wren-core-py test exercising register_parquet/csv → query via exec_ctx. The new Layer-2 test here is a good proxy, but a small Python integration test (register a parquet, load_mdl, assert the table resolves through exec_ctx) would lock the contract at the boundary that actually relies on it.

Neither blocks this PR (it's wren-core-only and correct as-is) — just flagging the follow-up so the guarantee doesn't quietly regress later.

[ttw225]

Thanks for the careful review — this is very helpful. Your reading is correct: the current Python methods register physical tables into base_ctx’s existing default catalog, so visibility through exec_ctx is preserved by the shared internals of that catalog provider even though the top-level catalog list is snapshotted.

I’d like to address this in a separate wren-core-py follow-up. The original goal was to remove the per-context call_lock and enable true same-context concurrency. Your suggestions add an important consumer-side safeguard to that work: I’ll clarify the register_parquet / register_csv documentation and add integration coverage for physical-table visibility through exec_ctx, including registration after exec_ctx has been created. The lock removal would also retain the same-context concurrency regression coverage and include throughput validation.

I’ll keep this PR scoped to wren-core. Thanks again for identifying the missing boundary coverage.

[goldmedal]

Filed #2504 to track the follow-up task.
Thanks @ttw225, nice work 👍

[ttw225]

Thanks for filing #2504 and for the thoughtful review!

I also opened #2510 to share one Tokio runtime across Python session
contexts and keep native thread growth bounded. It is related to the
same concurrency work, but it does not address #2504: the call_lock
remains in place, and the catalog documentation and integration
coverage are unchanged.

Addressing #2504 after #2510 should be cleaner, since removing the
call_lock will then build on a single shared worker pool instead of
per-context runtimes. I’ll handle those items separately.