Skip to content

Commit ca7ca52

Browse files
committed
fix(core): prevent catalog races in concurrent same-context transforms
apply_wren_on_ctx now builds derived session states on a per-call private copy of the base context's catalog list, so concurrent transform_sql_with_ctx calls on a shared context no longer observe each other's half-built wren catalog ("table not found" / "already exists" / cross-schema writes). Top-level catalog membership becomes an apply-time snapshot while copied providers' internals stay live-shared, so callers must finish registering physical catalogs before transforming. Removing wren-core-py's call_lock is a follow-up gated on this landing.
1 parent bb35d54 commit ca7ca52

2 files changed

Lines changed: 364 additions & 2 deletions

File tree

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

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@ use crate::mdl::type_planner::WrenTypePlanner;
1515
use crate::mdl::{AnalyzedWrenMDL, SessionStateRef};
1616
use async_trait::async_trait;
1717
use datafusion::arrow::datatypes::SchemaRef;
18-
use datafusion::catalog::memory::MemoryCatalogProvider;
18+
use datafusion::catalog::memory::{MemoryCatalogProvider, MemoryCatalogProviderList};
1919
use datafusion::catalog::CatalogProvider;
20+
use datafusion::catalog::CatalogProviderList;
2021
use datafusion::catalog::{MemorySchemaProvider, Session};
21-
use datafusion::common::Result;
22+
use datafusion::common::{internal_err, Result};
2223
use datafusion::datasource::{TableProvider, TableType, ViewTable};
2324
use datafusion::execution::session_state::SessionStateBuilder;
2425
use datafusion::logical_expr::Expr;
@@ -40,6 +41,37 @@ use parking_lot::RwLock;
4041

4142
pub type SessionPropertiesRef = Arc<HashMap<String, Option<String>>>;
4243

44+
/// Copies the top-level entries of a `CatalogProviderList` into a fresh,
45+
/// private `MemoryCatalogProviderList`.
46+
///
47+
/// Contract:
48+
/// 1. Top-level catalog-list membership/replacement is a snapshot at the
49+
/// time this is called: registering a brand-new top-level catalog on the
50+
/// original list afterward does not appear in the copy.
51+
/// 2. The internals of catalogs present at copy time are live-shared: each
52+
/// entry is the *same* `Arc<dyn CatalogProvider>` as the original, so a
53+
/// schema/table mutation on a catalog that already existed at copy time
54+
/// (e.g. registering a new table into an existing physical
55+
/// catalog/schema) is visible through both lists, because the provider's
56+
/// inner DashMap is shared (datafusion-catalog `catalog.rs`, `schema.rs`).
57+
/// 3. The enumeration is best-effort, not an atomic snapshot —
58+
/// `catalog_names()` and per-name `catalog()` are independent DashMap
59+
/// reads, so a concurrent `register_catalog` racing this copy may or may
60+
/// not be observed. Callers must finish top-level physical-catalog
61+
/// registration before starting a transform; this helper does not (and
62+
/// cannot) provide atomicity for same-named concurrent registrations.
63+
fn clone_catalog_list(
64+
existing: &Arc<dyn CatalogProviderList>,
65+
) -> Arc<dyn CatalogProviderList> {
66+
let private_list = MemoryCatalogProviderList::new();
67+
for name in existing.catalog_names() {
68+
if let Some(catalog) = existing.catalog(&name) {
69+
private_list.register_catalog(name, catalog);
70+
}
71+
}
72+
Arc::new(private_list)
73+
}
74+
4375
/// Apply Wren Rules to the context for sql generation.
4476
pub async fn apply_wren_on_ctx(
4577
ctx: &SessionContext,
@@ -75,10 +107,16 @@ pub async fn apply_wren_on_ctx(
75107
}
76108

77109
let type_planner = Arc::new(WrenTypePlanner::default());
110+
// Each apply call uses a private catalog-list snapshot. Both derived
111+
// SessionStates within the call share that snapshot, isolating in-flight
112+
// catalog registration from the base context and concurrent calls. See
113+
// `clone_catalog_list` for the exact sharing contract.
114+
let private_catalog_list = clone_catalog_list(ctx.state().catalog_list());
78115
let reset_default_catalog_schema = Arc::new(RwLock::new(
79116
SessionStateBuilder::new_from_existing(ctx.state())
80117
.with_config(config.clone())
81118
.with_type_planner(type_planner)
119+
.with_catalog_list(Arc::clone(&private_catalog_list))
82120
.build(),
83121
));
84122

@@ -109,6 +147,19 @@ pub async fn apply_wren_on_ctx(
109147
};
110148

111149
let new_state = new_state.with_config(config).build();
150+
// Guards the isolation invariant on the production wiring itself: the
151+
// final SessionState handed to `SessionContext` must hold the same
152+
// private Arc `clone_catalog_list` produced above. A violation here
153+
// means silently wrong query output, not just an internal inconsistency,
154+
// so this checks in every build, including release; the pointer
155+
// comparison is O(1).
156+
if !Arc::ptr_eq(new_state.catalog_list(), &private_catalog_list) {
157+
return internal_err!(
158+
"apply_wren_on_ctx: final SessionState's catalog_list is not the \
159+
private Arc from clone_catalog_list — the same-context isolation \
160+
invariant has been broken"
161+
);
162+
}
112163
let ctx = SessionContext::new_with_state(new_state);
113164
register_table_with_mdl(&ctx, analyzed_mdl, properties, mode).await?;
114165
Ok(ctx)

0 commit comments

Comments
 (0)