fix(core): non-matching apply=1 query rule silently bypasses fast-routing (#5620)

[renecannao]

Closes #5620.

Summary

Query_Processor<QP_DERIVED>::process_query (lib/Query_Processor.cpp) silently bypasses mysql_query_rules_fast_routing when no mysql_query_rules rule matches the query but the last iterated rule in the chain has apply=1. Reported by @burnison against 3.0.3 and 3.0.7.

In production this manifests as traffic landing on the user's default_hostgroup instead of the fast-routing destination — silently when the default hostgroup is routable, or as ERROR 9001 Max connect timeout reached while reaching hostgroup N when it is not.

Root cause

In the rules loop at lib/Query_Processor.cpp:1775-1944:

QP_rule_t *qr = NULL;
...
__internal_loop:
    for (auto it = _thr_SQP_rules->begin(); it != _thr_SQP_rules->end(); ++it) {
        qr = *it;                              // (1) assigned every iteration
        if (rule_matches_query(qr, ...) == false) {
            continue;                          // (2) qr leaks past the continue
        }
        // ... match body, may goto __exit_process_mysql_query if qr->apply==true ...
    }

__exit_process_mysql_query:
    if (qr == NULL || qr->apply == false) {    // (3) reads the leaked qr
        // fast-routing path
    }

The loop assigns qr = *it at the top of every iteration (1), before the match check. When the rule doesn't match, the body continues (2) without resetting qr. After the loop ends without ever finding a match, qr points at the last iterated non-matching rule. The exit check at (3) reads that leftover pointer's apply field; if it happens to be 1, the fast-routing branch is skipped entirely.

The exit-block check if (qr == NULL || qr->apply == false) was written assuming qr represents the last applied rule (or NULL if none applied). The loop, however, treats qr as the last iterated rule. The fix realigns the loop with the check's assumption.

The fix

One-line reset of qr to NULL when the match check fails:

if (rule_matches_query(qr, ...) == false) {
    qr = NULL;        // <- new
    continue;
}

After the loop, qr is now NULL iff no rule matched, or points at the last matched rule that fell through without apply=1. Both cases exit-block check (qr == NULL || qr->apply == false) handles correctly.

The change lives in the templated implementation Query_Processor<QP_DERIVED>::process_query, which is instantiated for both MySQL_Query_Processor and PgSQL_Query_Processor, so the fix covers both protocols. (Confirmed via grep -n 'Query_Processor<' lib/MySQL_Query_Processor.cpp lib/PgSQL_Query_Processor.cpp.)

Why a one-liner — and why not narrower

• A narrower fix (e.g. tracking via a separate bool matched_and_applied) was considered; it adds a parallel state variable to a function that already has qr, set_flagOUT, reiterate, and the goto labels. The exit-block predicate was already correct if qr meant what it was named — restoring that meaning is simpler than introducing another variable.
• Moving the qr = *it assignment after the match check would also work, but every line in the match body uses qr and would need to switch to a parameter or local. The reset before continue is a single-line change with the smallest blast radius.
• The fix is purely additive (one assignment); it cannot regress any pre-existing match behavior.

Test

New regression test test/tap/tests/reg_test_5620_fast_routing_qr_leak-t.cpp, registered in legacy-g6 (the same group as reg_test_2233_mirror_fast_routing-t, which uses the same SQLite3-server-as-backend pattern).

The test uses ProxySQL's built-in SQLite3 server (port 6030) so both default_hostgroup and fast_routing_hostgroup are reachable — that lets us verify which hostgroup the query actually went to via stats_mysql_query_digest rather than relying on connect failure as a proxy signal.

• Phase 1 — baseline. User + fast-routing rule, no mysql_query_rules. Issue a query, assert it landed in the fast-routing hostgroup. Catches setup mistakes before testing the bug condition.
• Phase 2 — bug trigger. Insert one mysql_query_rules row with flagIN=99999, apply=1 (matches the original repro). Reset digest stats. Issue the same query.
  • Pre-fix: query lands in default_hostgroup (fast-routing skipped). Assertion fails with expected 100 got 0.
  • Post-fix: query lands in fast_routing_hostgroup. Assertion passes.

Both phases check stats_mysql_query_digest filtered by the test user, so the test is self-isolating.

Files changed

• lib/Query_Processor.cpp — the one-line reset + a 6-line comment explaining why.
• test/tap/tests/reg_test_5620_fast_routing_qr_leak-t.cpp — new regression test.
• test/tap/groups/groups.json — registers the test in legacy-g6.

Test plan

• make build_tap_tests && WORKSPACE=$(pwd) INFRA_ID=dev-$USER TAP_GROUP=legacy-g6 test/infra/control/run-tests-isolated.bash passes including the new test.
• Revert just lib/Query_Processor.cpp (keep the test) and confirm the new test fails at Phase 2 with got 0 expected 100. This is the ground-truth regression check.
• Run legacy-g6 once on v3.0 HEAD to confirm no other test in the group regresses.
• Manual repro per Fast routing rules skipped due to logic bug when last query rule in chain has apply=1 #5620: mysql_query_rules with flagIN=99999, apply=1 no longer breaks fast-routing for an unrelated user.

Risk

Low. Single-line addition in a hot path, but the line only executes on the no-match continue branch (which previously did nothing with qr either) and the change is to clear stale state, not introduce new state. No callers, no signatures, no public API touched.

Summary by CodeRabbit

• Bug Fixes

  • Fixed an issue where fast-routing could be unexpectedly bypassed when the last query rule in a chain did not match an incoming query.

• Tests

  • Added a regression TAP test and test-group mapping to ensure fast-routing behavior remains correct in this edge case.

[gemini-code-assist]

Code Review

This pull request addresses issue #5620, where fast-routing rules were silently skipped if the final query rule in the evaluation chain had apply=1 but did not match the query. The fix ensures the query rule pointer is reset to NULL upon a non-match, preventing stale state from affecting subsequent routing logic. A new regression test and corresponding group configuration have been added to validate this behavior. I have no further feedback to provide.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 76604831-4a6e-47aa-a3a8-94cbed331be5

📥 Commits

Reviewing files that changed from the base of the PR and between abc5437 and e5a31ef.

📒 Files selected for processing (1)

• test/tap/groups/groups.json

✅ Files skipped from review due to trivial changes (1)

• test/tap/groups/groups.json

---

📝 Walkthrough

Walkthrough

Clears a stale current-rule pointer in query processing so fast-routing is not skipped when the last rule has apply=1 but doesn't match. Adds a TAP regression test that verifies routing before and after inserting a non-matching apply=1 rule, and registers the test.

Changes

Query Routing Fast-routing Fix and Validation

Layer / File(s)	Summary
Core bug fix lib/Query_Processor.cpp	When a query rule fails to match, explicitly reset qr = NULL to prevent fast-routing exit logic from observing a stale matched rule state from the last loop iteration.
Regression test test/tap/tests/reg_test_5620_fast_routing_qr_leak-t.cpp	New TAP test with two phases: (1) baseline asserts fast-routing works without query rules, (2) adds a non-matching rule with apply=1 and asserts fast-routing still routes correctly. Uses admin-side stats_mysql_query_digest to validate hostgroup routing.
Test registration test/tap/groups/groups.json	Register new test reg_test_5620_fast_routing_qr_leak-t in the legacy-g6 test group.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Unit Testing Phase 2.4: Query_Processor Rule Matching Unit Tests #5476: Same code area — issue describes a fast-routing skip when the last rule has apply=1, which this PR fixes.

Possibly related PRs

• sysown/proxysql#5503: Modifies the same rule-matching path in process_query() and is related at the code boundary.

Poem

🐰 A pointer once lingered where it shouldn't dwell,
Through loops and exits it cast a brief spell.
Set it to NULL, let the fast lanes resume,
Now queries find home and the digests bloom. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main bug fix: addressing a logic issue where non-matching apply=1 query rules bypass fast-routing.
Linked Issues check	✅ Passed	The PR fully implements all coding objectives from issue #5620: identifies the root cause (stale qr pointer), applies a minimal one-line fix (qr = NULL before continue) to the templated Query_Processor, and includes a comprehensive regression test.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue #5620: one-line fix in Query_Processor.cpp, new regression test file, and test group mapping. No unrelated changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-5620-fast-routing-qr-leak

---

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 and usage tips.}

[burnison]

I can confirm that this fixes the original bug reported. Thank you!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud