Skip to content

fix(core): keep RLAC cycle-detection state per analyzer invocation - #2619

Open
ttw225 wants to merge 1 commit into
Canner:mainfrom
ttw225:fix/core-analyzer-request-local-cycle-state
Open

fix(core): keep RLAC cycle-detection state per analyzer invocation#2619
ttw225 wants to merge 1 commit into
Canner:mainfrom
ttw225:fix/core-analyzer-request-local-cycle-state

Conversation

@ttw225

@ttw225 ttw225 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Why

ModelAnalyzeRule kept its RLAC cycle-detection stack (building_models) as an instance field, cleared at the top of every analyze(). A derived SessionContext holds one rule instance for its whole lifetime, so concurrent optimize() calls on the same context share — and corrupt — that stack: an 8-thread stress run of a valid acyclic query reliably fails within ~2 iterations with a spurious "Detected a cycle in row level access control conditions" error, and a late clear() can equally erase the state that would catch a real cycle.

Today the wren-core-py binding masks this with a per-context call lock; removing that lock (#2504, step 3) is only safe once this state is per-invocation.

What

  • analyze() allocates the cycle stack per invocation and threads it through the model-rewrite path (analyze_modelanalyze_table_scan / analyze_subquery_alias_modelbuild_model_plan_nodeanalyze_rlac_subqueriesanalyze_subquery_plan). RLAC subquery recursion passes the caller's stack, so transitive cycles (A → B → A) are still detected. analyze_scope never touches cycle state and is unchanged.
  • ModelStackGuard borrows the stack (&ModelStack) instead of sharing ownership; cleanup-on-drop behavior is unchanged.
  • analyze_table_scan drops two parameters that every caller filled with clones of the rule's own fields.

Detection logic, error message, and ModelAnalyzeRule::new's signature are unchanged; single-query behavior is identical.

Test Plan

  • New analyzer_concurrency regression: one shared LocalRuntime derived context, 8 Barrier-started threads × 50 iterations planning an acyclic RLAC chain through SessionState::optimize (the Analyzer only runs there; transform_sql_with_ctx would build fresh rule instances per call and cannot observe the race). Fails 3/3 on the unfixed rule, passes 5/5 with the fix, ~0.1s.
  • Serial cycle tests (test_rlac_subquery_cycle_detected, test_rlac_self_reference_is_cycle) still pass — real cycles are still rejected.
  • Full gates: 148 lib tests, clippy --all-targets --all-features -D warnings, cargo fmt --check, sqllogictest suite.

Part of #2504 (step 2 of the plan in the issue comments).

Summary by CodeRabbit

  • Bug Fixes

    • Improved row-level access control cycle detection during concurrent query processing.
    • Prevented false cycle errors when multiple queries are optimized at the same time.
    • Ensured cycle detection remains consistent across nested access-control subqueries.
  • Tests

    • Added coverage for concurrent query planning and optimization using shared session context.

@github-actions github-actions Bot added rust Pull requests that update rust code core labels Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b2c5b4f3-9f54-4f46-9359-c4bf0b02e671

📥 Commits

Reviewing files that changed from the base of the PR and between 99f1dbd and d4da5de.

📒 Files selected for processing (2)
  • core/wren-core/core/src/logical_plan/analyze/model_anlayze.rs
  • core/wren-core/core/src/mdl/mod.rs

Walkthrough

ModelAnalyzeRule now uses a fresh cycle-detection stack for each analysis invocation, threads it through recursive RLAC subquery rewriting, and adds a concurrency test covering shared derived contexts.

Changes

RLAC cycle detection

Layer / File(s) Summary
Invocation-scoped stack and guarded model tracking
core/wren-core/core/src/logical_plan/analyze/model_anlayze.rs
Creates a per-invocation ModelStack, removes the rule-level building_models state, and uses guarded stack entries during model construction.
Recursive model and RLAC subquery propagation
core/wren-core/core/src/logical_plan/analyze/model_anlayze.rs
Passes the same cycle stack through model analysis, table scans, joins, subquery aliases, and RLAC scalar, IN, and EXISTS subqueries.
Concurrent analysis validation
core/wren-core/core/src/mdl/mod.rs
Runs planning and optimization concurrently on a shared derived context and verifies that an acyclic RLAC chain succeeds.

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

Sequence Diagram(s)

sequenceDiagram
  participant Session
  participant ModelAnalyzeRule
  participant RLACSubqueries
  participant Optimizer
  Session->>ModelAnalyzeRule: create logical plan
  ModelAnalyzeRule->>ModelAnalyzeRule: create invocation cycle stack
  ModelAnalyzeRule->>RLACSubqueries: rewrite RLAC subqueries with stack
  RLACSubqueries->>ModelAnalyzeRule: analyze nested model plan
  ModelAnalyzeRule->>Optimizer: return analyzed plan
  Optimizer-->>Session: optimized plan
Loading

Possibly related PRs

  • Canner/WrenAI#2335: Adds RLAC cycle detection during subquery model expansion, related to this PR’s stack propagation.

Suggested reviewers: goldmedal

Poem

A rabbit hops through models deep,
With stacks that guard the paths they keep.
RLAC loops now know their track,
Each query gets its own neat stack.
Threads may race, yet plans stay bright—
Acyclic hops resolve just right.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: moving RLAC cycle-detection state to per-invocation scope.
Description check ✅ Passed The description is detailed and covers the change, failure, and tests, but it omits the duplicate-check section.
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant