WIP: TAP test linting and partial cleanup#5606
Merged
renecannao merged 35 commits intov3.0from Apr 14, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new TAP-test linting workflow (clang-tidy normalization + runner), a Makefile target, and numerous test-code fixes: resource cleanup, null-checks, explicit casts, operator-precedence corrections, and small refactors across many TAP/unit test files and build helpers. Changes
Sequence DiagramsequenceDiagram
actor Dev as Developer
participant Make as Makefile
participant Runner as scripts/lint/run_tap_tests.py
participant TidyUtils as scripts/lint/clang_tidy_utils.py
participant ClangTidy as clang-tidy
participant FS as RepositoryFS
participant Out as lint/<br/>clang-tidy-tests.txt
Dev->>Make: make lint-tests FILES=...
Make->>Runner: start runner with args
Runner->>FS: discover/select `*-t.cpp` sources
Runner->>Runner: build_compile_database() → `lint/compile_commands.json`
loop for each source
Runner->>ClangTidy: run clang-tidy (compile DB + header filter)
ClangTidy-->>Runner: stdout/stderr diagnostics
Runner->>TidyUtils: normalize_clang_tidy_content(output)
TidyUtils-->>Runner: normalized, deduped list
end
Runner->>Out: write aggregated diagnostics
Runner-->>Dev: exit code (0 ok / 1 diagnostics)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a static analysis framework for TAP tests, including a new lint-tests Makefile target and Python scripts to execute clang-tidy against test sources. The implementation provides utilities for normalizing diagnostics and a script to dynamically generate a compilation database for the tests. Additionally, several C++ test files were refactored for better type safety and style consistency. Feedback suggests optimizing performance through parallelization, improving efficiency by de-duplicating source file inputs, and preventing potential resource leaks during temporary directory management.
Comment on lines
+49
to
+56
| selected: list[Path] = [] | ||
| for item in args: | ||
| path = Path(item) | ||
| if not path.is_absolute(): | ||
| path = (repo_root / path).resolve() | ||
| if path.is_file() and path.name.endswith("-t.cpp"): | ||
| selected.append(path) | ||
| return selected |
There was a problem hiding this comment.
The select_test_sources function does not de-duplicate the list of source files. If the same file is passed multiple times in the command-line arguments, the script will run clang-tidy multiple times on the same file, which is inefficient.
Suggested change
| selected: list[Path] = [] | |
| for item in args: | |
| path = Path(item) | |
| if not path.is_absolute(): | |
| path = (repo_root / path).resolve() | |
| if path.is_file() and path.name.endswith("-t.cpp"): | |
| selected.append(path) | |
| return selected | |
| selected: set[Path] = set() | |
| for item in args: | |
| path = Path(item) | |
| if not path.is_absolute(): | |
| path = (repo_root / path).resolve() | |
| if path.is_file() and path.name.endswith("-t.cpp"): | |
| selected.add(path) | |
| return sorted(selected) |
| def build_compile_database(repo_root: Path, sources: Sequence[Path]) -> Path: | ||
| lint_dir = repo_root / "lint" | ||
| lint_dir.mkdir(exist_ok=True) | ||
| temp_dir = Path(tempfile.mkdtemp(prefix="tap-clang-tidy-", dir=str(lint_dir))) |
There was a problem hiding this comment.
There is a potential resource leak here. If an exception occurs within build_compile_database after mkdtemp is called (for example, during the write_text call on line 139), the temporary directory will not be cleaned up because the finally block in main is only entered if this function returns successfully. Consider using tempfile.TemporaryDirectory as a context manager or wrapping the logic inside build_compile_database in a try...except block to ensure cleanup on failure.
Comment on lines
+161
to
+170
| for source_file in sources: | ||
| diagnostics.extend( | ||
| run_clang_tidy_for_file( | ||
| clang_tidy_bin, | ||
| repo_root, | ||
| db_dir, | ||
| source_file, | ||
| header_filter, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Running clang-tidy sequentially for each file can be significantly slow as the number of test files increases. Since each clang-tidy invocation is independent, this process can be parallelized using a process pool (e.g., concurrent.futures.ProcessPoolExecutor) to improve performance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Makefile (1)
30-30: Remove duplicate.PHONY: lint-testsdeclaration.
lint-testsis already declared phony on Line 17, so Line 30 is redundant noise.♻️ Proposed cleanup
-.PHONY: lint-tests lint-tests: `@echo` "Running TAP test static analysis" ./scripts/lint/run_tap_tests.py $(FILES)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 30, Remove the duplicate phony declaration for the lint-tests target by deleting the redundant ".PHONY: lint-tests" entry so the Makefile only declares lint-tests as phony once (retain the original .PHONY line at its existing location); this cleans up the file without changing targets or behavior.scripts/lint/run_tap_tests.py (1)
66-78: Add a timeout for eachclang-tidyinvocation.A per-file timeout improves resilience when a single translation unit hangs or becomes extremely slow.
♻️ Suggested hardening
- proc = subprocess.run( - [ - clang_tidy_bin, - "-p", - str(compile_db_dir), - f"--header-filter={header_filter}", - str(source_file), - ], - cwd=str(repo_root), - text=True, - capture_output=True, - check=False, - ) + try: + proc = subprocess.run( + [ + clang_tidy_bin, + "-p", + str(compile_db_dir), + f"--header-filter={header_filter}", + str(source_file), + ], + cwd=str(repo_root), + text=True, + capture_output=True, + check=False, + timeout=300, + ) + except subprocess.TimeoutExpired: + return [f"{source_file}:0: clang-tidy-timeout - timed out after 300s"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint/run_tap_tests.py` around lines 66 - 78, The clang-tidy subprocess.run invocation can hang; add a per-invocation timeout (e.g., timeout_seconds constant or env-configurable) to the subprocess.run call and handle subprocess.TimeoutExpired around the call. Modify the invocation that uses clang_tidy_bin, compile_db_dir, header_filter, source_file and repo_root to pass timeout=timeout_seconds, and catch subprocess.TimeoutExpired to produce a sensible proc-like result (or log/mark the file as timed out) so downstream logic does not break.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lint/run_tap_tests.py`:
- Around line 33-41: The current discovery only globs "*-t.cpp" and misses files
named "test_*.cpp"; update the selection logic around the loops that populate
candidates (the blocks using base.glob and unit.glob) to include both patterns
(test_*.cpp and *-t.cpp) when iterating, e.g., perform two globs or a combined
pattern for each of the locations referenced by base and unit so both naming
conventions are appended to candidates; apply the same change to the other
occurrence noted (the block at the later lines around the second glob usage).
In `@test/tap/tests/test_empty_query-t.cpp`:
- Around line 30-31: The test declares exp_myerr and act_myerr as unsigned int
but later prints them with a signed specifier; update the printf/fprintf call
that prints these variables (the call referencing exp_myerr and act_myerr in
test_empty_query-t.cpp) to use the unsigned format specifier %u for both
variables so the format matches their type (replace any "%d" occurrences used
for these variables with "%u").
In `@test/tap/tests/unit/query_cache_unit-t.cpp`:
- Around line 58-63: The helper dup_fill does an unchecked malloc in dup_fill
and can dereference NULL; add a null-pointer check after malloc in dup_fill
(check if buf == NULL) and return NULL (or handle error) before calling
memset/assigning buf[len], ensuring callers can handle a NULL return; update any
callers if needed to handle the possible NULL return.
In `@test/tap/tests/vector_features-t.cpp`:
- Around line 234-235: The test uses unsigned long for the return value of
mysql_num_rows(), which may be 32-bit on some platforms; change the variable
declarations named rows (and the other occurrence around the second check) to
use my_ulonglong (the project typedef for unsigned long long) so the 64-bit
return from mysql_num_rows() is preserved; update both places where
mysql_num_rows(result) is assigned and used (e.g., the ok(rows > 0, ...) checks)
to use my_ulonglong rows.
---
Nitpick comments:
In `@Makefile`:
- Line 30: Remove the duplicate phony declaration for the lint-tests target by
deleting the redundant ".PHONY: lint-tests" entry so the Makefile only declares
lint-tests as phony once (retain the original .PHONY line at its existing
location); this cleans up the file without changing targets or behavior.
In `@scripts/lint/run_tap_tests.py`:
- Around line 66-78: The clang-tidy subprocess.run invocation can hang; add a
per-invocation timeout (e.g., timeout_seconds constant or env-configurable) to
the subprocess.run call and handle subprocess.TimeoutExpired around the call.
Modify the invocation that uses clang_tidy_bin, compile_db_dir, header_filter,
source_file and repo_root to pass timeout=timeout_seconds, and catch
subprocess.TimeoutExpired to produce a sensible proc-like result (or log/mark
the file as timed out) so downstream logic does not break.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74af7db5-1513-4093-abab-b8b58610e64b
📒 Files selected for processing (15)
.gitignoreMakefilescripts/lint/clang_tidy_utils.pyscripts/lint/run_tap_tests.pyscripts/lint/test_clang_tidy_utils.pyscripts/lint/test_run_tap_tests.pytest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use UPPER_SNAKE_CASE for constants and macros
Files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must link againstlibproxysql.aand usetest_globals.handtest_init.h
Files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
test/tap/tests/**/*(test_*.cpp|*-t.cpp)
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow the naming pattern
test_*.cppor*-t.cpp
Files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
test/tap/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/**/*(test_*.cpp|*-t.cpp) : Test files follow the naming pattern `test_*.cpp` or `*-t.cpp`
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/query_processor_firewall_unit-t.cpptest/tap/tests/unit/pgsql_variables_validator_unit-t.cpptest/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cpptest/tap/tests/unit/pgsql_tokenizer_unit-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Applied to files:
test/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cppscripts/lint/test_run_tap_tests.pyscripts/lint/run_tap_tests.pyMakefile
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/**/*(test_*.cpp|*-t.cpp) : Test files follow the naming pattern `test_*.cpp` or `*-t.cpp`
Applied to files:
test/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/unit/query_processor_unit-t.cppscripts/lint/test_run_tap_tests.pyscripts/lint/run_tap_tests.py
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
test/tap/tests/unit/statistics_unit-t.cpptest/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cpptest/tap/tests/unit/query_cache_unit-t.cppscripts/lint/test_run_tap_tests.pyMakefile
📚 Learning: 2026-03-26T16:38:58.553Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:38:58.553Z
Learning: In `lib/mysql_connection.cpp`, when reviewing `SHOW WARNINGS` handling, treat the digest source as an intentional design choice: both `update_warning_count_from_connection()` and the `add_eof()` call under `ASYNC_USE_RESULT_CONT` detect warnings using `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This is expected to fail/behave differently when `mysql-query_digests_keep_comment=1` (digest_text includes comments) or when `mysql-query_digests=0` (digest_text unavailable). Do not require a change unless the regression test coverage is expanded (noting `reg_test_5306-show_warnings_with_comment-t` explicitly excludes these configurations as an accepted limitation).
Applied to files:
.gitignore
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cppMakefile
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, and similar feature flags for feature tiers
Applied to files:
test/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpptest/tap/tests/unit/query_processor_unit-t.cppMakefile
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/vector_features-t.cpptest/tap/tests/test_empty_query-t.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/unit/pgsql_tokenizer_unit-t.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: See `doc/agents/project-conventions.md` for ProxySQL-specific rules including directories, build, test harness, and git workflow
Applied to files:
Makefile
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/query_cache_unit-t.cpp
[warning] 60-60: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 61-61: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/pgsql_tokenizer_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 Ruff (0.15.9)
scripts/lint/run_tap_tests.py
[error] 66-66: subprocess call: check for execution of untrusted input
(S603)
scripts/lint/clang_tidy_utils.py
[error] 23-23: Starting a process with a partial executable path
(S607)
[warning] 28-28: Do not catch blind exception: Exception
(BLE001)
[warning] 75-75: Do not catch blind exception: Exception
(BLE001)
[warning] 139-139: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
test/tap/tests/unit/query_processor_firewall_unit-t.cpp (1)
499-507: Bracing cleanup preserves behavior.This change is formatting-only and keeps the same conditional-delete semantics.
.gitignore (1)
5-5: Root-anchored ignore rule looks correct.Ignoring
/lint/at repo root is appropriate for the new lint artifact location.test/tap/tests/unit/pgsql_variables_validator_unit-t.cpp (1)
20-22: Good readability improvement with no behavior change.Conditional free logic remains correct after bracing.
test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp (1)
102-104: No concerns on this cleanup.Braced conditional free is equivalent and clearer.
test/tap/tests/unit/statistics_unit-t.cpp (1)
51-53: Bracing-only refactor is safe here.All three conditionals preserve prior logic and improve consistency.
Also applies to: 57-59, 70-72
test/tap/tests/unit/query_processor_unit-t.cpp (1)
284-286: Consistent bracing updates look good.These edits are non-functional and keep cleanup/assertion behavior intact.
Also applies to: 397-399, 423-425, 447-449, 493-495, 506-508, 710-715, 855-857, 861-863, 905-907, 944-946, 1343-1345
scripts/lint/test_clang_tidy_utils.py (1)
24-33: Good focused normalization coverage.This test cleanly verifies that check name/message survive normalization in deterministic output form.
test/tap/tests/unit/pgsql_tokenizer_unit-t.cpp (1)
64-87:DigestOptionsrefactor is a solid cleanup.Moving from positional booleans/ints to a named options struct makes these tests safer to read and maintain.
Also applies to: 181-183, 354-356, 366-368, 380-382
scripts/lint/clang_tidy_utils.py (1)
129-146: Nice extraction and normalization layering.YAML/text fallback plus repo-aware filtering/header-filter generation gives stable, reusable lint output.
Also applies to: 163-166
test/tap/tests/vector_features-t.cpp (1)
318-323: Code is correct; no changes needed.The MySQL C API's
mysql_real_connect()modifies the MYSQL structure in place rather than creating a new handle. The current pattern—checking!mysql_real_connect()for failure and returning early—is correct, idiomatic, and matches all established patterns in ProxySQL test files.> Likely an incorrect or invalid review comment.
Comment on lines
+33
to
+41
| for path in sorted(base.glob("*-t.cpp")): | ||
| if path.is_file(): | ||
| candidates.append(path) | ||
|
|
||
| unit = root / "test/tap/tests/unit" | ||
| if unit.exists(): | ||
| for path in sorted(unit.glob("*-t.cpp")): | ||
| if path.is_file(): | ||
| candidates.append(path) |
There was a problem hiding this comment.
Include both TAP naming patterns during source selection.
Current discovery/selection misses test_*.cpp, so part of TAP tests will never be linted.
🔧 Proposed fix
+def _is_tap_test_source(path: Path) -> bool:
+ return path.name.endswith("-t.cpp") or (
+ path.name.startswith("test_") and path.suffix == ".cpp"
+ )
+
def discover_test_sources(repo_root: str | Path) -> list[Path]:
root = Path(repo_root)
candidates: list[Path] = []
base = root / "test/tap/tests"
if base.exists():
- for path in sorted(base.glob("*-t.cpp")):
- if path.is_file():
- candidates.append(path)
+ for pattern in ("*-t.cpp", "test_*.cpp"):
+ for path in sorted(base.glob(pattern)):
+ if path.is_file():
+ candidates.append(path)
unit = root / "test/tap/tests/unit"
if unit.exists():
- for path in sorted(unit.glob("*-t.cpp")):
- if path.is_file():
- candidates.append(path)
+ for pattern in ("*-t.cpp", "test_*.cpp"):
+ for path in sorted(unit.glob(pattern)):
+ if path.is_file():
+ candidates.append(path)
return candidates
@@
- if path.is_file() and path.name.endswith("-t.cpp"):
+ if path.is_file() and _is_tap_test_source(path):
selected.append(path)As per coding guidelines, test/tap/tests/**/*(test_*.cpp|*-t.cpp): test files follow the naming pattern test_*.cpp or *-t.cpp.
Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lint/run_tap_tests.py` around lines 33 - 41, The current discovery
only globs "*-t.cpp" and misses files named "test_*.cpp"; update the selection
logic around the loops that populate candidates (the blocks using base.glob and
unit.glob) to include both patterns (test_*.cpp and *-t.cpp) when iterating,
e.g., perform two globs or a combined pattern for each of the locations
referenced by base and unit so both naming conventions are appended to
candidates; apply the same change to the other occurrence noted (the block at
the later lines around the second glob usage).
Comment on lines
+30
to
+31
| const unsigned int exp_myerr = 1065U; | ||
| unsigned int act_myerr = 0U; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the unsigned declarations and current format string usage
rg -n -C2 'exp_myerr|act_myerr|MySQL error equals expected' test/tap/tests/test_empty_query-t.cppRepository: sysown/proxysql
Length of output: 495
🏁 Script executed:
# Find TAP framework implementation and check ok() signature
fd -t f "tap\." test/tap/include/ | head -5Repository: sysown/proxysql
Length of output: 163
🏁 Script executed:
# Check the entire test file for format specifier patterns
rg -n '%d|%u|%x' test/tap/tests/test_empty_query-t.cppRepository: sysown/proxysql
Length of output: 256
🏁 Script executed:
# Find TAP header files
fd -t f "tap\.h" test/Repository: sysown/proxysql
Length of output: 77
🏁 Script executed:
# Search for ok() function definition to understand its format handling
rg -n 'ok\(' test/ -t cpp | head -3Repository: sysown/proxysql
Length of output: 449
🏁 Script executed:
# Find the TAP framework header and check ok() implementation
fd -type f "\.h$" test/ | xargs rg -l "ok\(" 2>/dev/null | head -3Repository: sysown/proxysql
Length of output: 412
🏁 Script executed:
# Read the TAP header to check ok() function signature
head -100 test/tap/tap/tap.hRepository: sysown/proxysql
Length of output: 2548
🏁 Script executed:
# Search for the ok() function implementation in tap.h
rg -A 10 'define ok\(|^ok\(' test/tap/tap/tap.hRepository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Search for ok macro definition more broadly
rg -n 'ok' test/tap/tap/tap.h | grep -i 'define\|ok('Repository: sysown/proxysql
Length of output: 580
🏁 Script executed:
# Get the full tap.h file size and read the section with ok()
wc -l test/tap/tap/tap.hRepository: sysown/proxysql
Length of output: 81
🏁 Script executed:
# Get the full ok() function implementation to confirm it uses printf-style
sed -n '141,160p' test/tap/tap/tap.hRepository: sysown/proxysql
Length of output: 515
Fix format specifiers after switching to unsigned int.
Line 30 and Line 31 declare unsigned int types, but Line 43 prints them with %d (signed format). Use %u to match the type and avoid printf-style format mismatch:
🔧 Proposed fix
- ok(
- exp_myerr == act_myerr, "MySQL error equals expected - exp_err: '%d', act_err: '%d' error: `%s`",
- exp_myerr, act_myerr, mysql_error(proxy)
- );
+ ok(
+ exp_myerr == act_myerr, "MySQL error equals expected - exp_err: '%u', act_err: '%u' error: `%s`",
+ exp_myerr, act_myerr, mysql_error(proxy)
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_empty_query-t.cpp` around lines 30 - 31, The test
declares exp_myerr and act_myerr as unsigned int but later prints them with a
signed specifier; update the printf/fprintf call that prints these variables
(the call referencing exp_myerr and act_myerr in test_empty_query-t.cpp) to use
the unsigned format specifier %u for both variables so the format matches their
type (replace any "%d" occurrences used for these variables with "%u").
Comment on lines
+58
to
+63
| static unsigned char *dup_fill(size_t len, char ch) { | ||
| unsigned char *buf = static_cast<unsigned char *>(malloc(len + 1)); | ||
| memset(buf, ch, len); | ||
| buf[len] = '\0'; | ||
| return buf; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current helper implementation has no allocation guard before writes
rg -n -C3 'static unsigned char \*dup_fill|malloc\(|memset\(|buf\[len\]' test/tap/tests/unit/query_cache_unit-t.cppRepository: sysown/proxysql
Length of output: 320
Add null-pointer check after malloc in dup_fill helper.
The malloc(len + 1) call at line 59 is not guarded. If allocation fails, lines 60–61 will dereference a null pointer, causing undefined behavior.
🔧 Suggested fix
static unsigned char *dup_fill(size_t len, char ch) {
unsigned char *buf = static_cast<unsigned char *>(malloc(len + 1));
+ if (buf == nullptr) {
+ return nullptr;
+ }
memset(buf, ch, len);
buf[len] = '\0';
return buf;
}🧰 Tools
🪛 Cppcheck (2.20.0)
[warning] 60-60: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 61-61: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/unit/query_cache_unit-t.cpp` around lines 58 - 63, The helper
dup_fill does an unchecked malloc in dup_fill and can dereference NULL; add a
null-pointer check after malloc in dup_fill (check if buf == NULL) and return
NULL (or handle error) before calling memset/assigning buf[len], ensuring
callers can handle a NULL return; update any callers if needed to handle the
possible NULL return.
Comment on lines
+234
to
235
| unsigned long rows = mysql_num_rows(result); | ||
| ok(rows > 0, "ai_detected_anomalies status variable exists"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file at the specified lines
wc -l test/tap/tests/vector_features-t.cppRepository: sysown/proxysql
Length of output: 99
🏁 Script executed:
# Read around lines 234-235 and 250-251 to see the context
head -260 test/tap/tests/vector_features-t.cpp | tail -40Repository: sysown/proxysql
Length of output: 1305
🏁 Script executed:
# Also check what mysql_num_rows() actually returns by searching the codebase
rg "mysql_num_rows" -B 2 -A 2 --no-heading | head -50Repository: sysown/proxysql
Length of output: 3379
🏁 Script executed:
# Check for my_ulonglong definition in the codebase
rg "typedef.*my_ulonglong|my_ulonglong.*typedef" -nRepository: sysown/proxysql
Length of output: 149
🏁 Script executed:
# Search for mysql_num_rows definition or declaration
rg "mysql_num_rows\s*\(" -A 1 | head -20Repository: sysown/proxysql
Length of output: 1550
🏁 Script executed:
# Check for MySQL client headers in the repository
fd -e h -e hpp | xargs grep -l "mysql_num_rows" 2>/dev/null | head -10Repository: sysown/proxysql
Length of output: 120
🏁 Script executed:
# Check if there are any includes of mysql.h or similar
rg "#include.*mysql" test/tap/tests/vector_features-t.cpp -nRepository: sysown/proxysql
Length of output: 109
🏁 Script executed:
# Let's check the actual MySQL C API documentation by searching web for the signature
# But first, let me check what header files are included in the test file
head -50 test/tap/tests/vector_features-t.cppRepository: sysown/proxysql
Length of output: 1185
🌐 Web query:
mysql_num_rows return type MySQL C API documentation
💡 Result:
The mysql_num_rows function from the MySQL C API returns a uint64_t (unsigned 64-bit integer), which represents the number of rows in the result set. This is confirmed in the official MySQL documentation for versions 8.0 and 9.6, as well as MariaDB's compatible API documentation which uses my_ulonglong (an alias for uint64_t). There are no errors returned by this function; it always provides the row count (behavior depends on buffered vs. unbuffered results).
Citations:
- 1: https://dev.mysql.com/doc/c-api/9.6/en/mysql-num-rows.html
- 2: https://dev.mysql.com/doc/c-api/8.0/en/mysql-num-rows.html
- 3: https://mariadb.com/docs/connectors/mariadb-connector-c/api-functions/mysql_num_rows
Use my_ulonglong for mysql_num_rows() results.
The MySQL C API's mysql_num_rows() returns uint64_t; unsigned long is platform-dependent and may be 32-bit on some systems, losing precision for large result sets. my_ulonglong (defined in the codebase as unsigned long long) correctly handles the full 64-bit return value.
Fix
- unsigned long rows = mysql_num_rows(result);
+ my_ulonglong rows = mysql_num_rows(result);Also applies to: lines 250-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/vector_features-t.cpp` around lines 234 - 235, The test uses
unsigned long for the return value of mysql_num_rows(), which may be 32-bit on
some platforms; change the variable declarations named rows (and the other
occurrence around the second check) to use my_ulonglong (the project typedef for
unsigned long long) so the 64-bit return from mysql_num_rows() is preserved;
update both places where mysql_num_rows(result) is assigned and used (e.g., the
ok(rows > 0, ...) checks) to use my_ulonglong rows.
test_stats_proxysql_message_metrics-t.cpp: - Fix UB: mysql_error() called on NULL handle when mysql_init() fails. - Fix proxy/proxy_admin leaks on every exit path (MYSQL_QUERY macro silently returns EXIT_FAILURE from the enclosing function, bypassing any end-of-scope cleanup); use std::unique_ptr with mysql_close deleter. - Fix result-set leaks: second mysql_store_result() assignment dropped the previous MYSQL_RES*, and neither was ever freed. Switch to a unique_ptr<MYSQL_RES> and reset() at reassignment. - Guard out-of-bounds extract_mysql_rows(...)[0] with an empty-result check; previously only the first call was guarded by num_rows==1. - Fix latent error-propagation bug: if (rc == EXIT_FAILURE) only caught rc == 1; mysql_query() can return other non-zero values. Use rc != EXIT_SUCCESS instead. test_cluster1-t.cpp: - Fix three NULL-deref UB sites: mysql_error() on NULL after mysql_init(). - Fix leaks in create_connections(): on mysql_real_connect() failure the just-initialized MYSQL* was never closed. - Fix leak of the global conns[] vector: connections were never closed on exit. Added close_all_connections() called on all exit paths. - Fix leak of proxysql_admin (never closed); wrapped in unique_ptr with mysql_close deleter so destructor runs even on MYSQL_QUERY early-return. - Remove dead variable MYSQL_RES* proxy_res (declared, never used). - Fix two signed/unsigned comparisons (int loop counter vs size_t size). reg_test_4556-ssl_error_queue-t.cpp (selective fixes — file has more issues left for follow-up): - create_server_conn: add mysql_init NULL check; close handle on mysql_real_connect failure (was leaked). - create_test_database: close handle on query failure (was leaked). - create_ssl_conn_inv_cert: add mysql_init NULL check; free the tempnam path (malloc'd, never freed); close myconn on all paths. - create_ssl_conn_missing_cert: same as above, plus check fopen return, always fclose tmp_file (was only on success path), unlink temp file, and use explicit "%s" format instead of passing the data buffer as the format string. - clean_conn_pool: fix three MYSQL_RES leaks (mysql_store_result with no matching mysql_free_result). - warmup_conn_pool: on any error, the just-opened MYSQL* AND all previously-pushed conns leaked (the function returned a fresh empty vector, stranding everything already allocated). Added a cleanup_and_fail lambda that closes everything before returning. - Fix signed int loop counter vs uint32_t CONNS_TOTAL. No behavioral changes to test assertions; only leak/UB fixes. Verified with -fsyntax-only on all three files.
test_binlog_reader-t.cpp: - Fix four leak paths in main(): when mysql_init of proxysql_mysql or proxysql_admin failed, or when mysql_real_connect of either failed, the other (already-initialized/connected) handle was leaked. Cannot use goto cleanup because C++ forbids jumping past the non-trivial std::vector<> initializations farther down; use explicit mysql_close on each error path instead. - Fix two leaks in perform_rnd_selects(): select_conn was leaked on mysql_real_connect failure and on mysql_query failure inside the loop. - Remove dead mysql_store_result(proxysql_admin)/extract_mysql_rows block in the main loop: the result of the previous admin query had already been consumed upstream, so store_result returned NULL and the extracted 'pre_select_rows' vector was written but never read. - Add guard for out-of-bounds res_row[0][1] access: previously unchecked even though an empty or short result would UB. test_ssl_fast_forward-1-t.cpp: - Fix massive leak pattern: main() had 22 'return exit_status()' early exits, none of which closed the 5 per-iteration mysqls[] handles or mysqladmin. The happy path also leaked all 5 mysqls[] (only mysqladmin was closed at the end). - Restructure main() to declare mysqladmin, mysqls[5], and a close_all lambda at the top so every exit path can uniformly release every non-NULL handle. Each close_all() call NULLs the slot, so repeated calls are no-ops and no double-free risk. - Wrap braceless 'if (cond) return exit_status();' bodies in braces where the scripted edit would otherwise put close_all() inside the if body while leaving 'return' unconditional. - NULL mysqls[i] in the inter-iteration close loop so a later close_all() on a failing iteration doesn't double-close stale pointers. No behavioral changes to assertions; compile-verified with -fsyntax-only on both files.
WIP.md explicitly flagged this file as having 'deeper issues than
formatting' and was reset to HEAD rather than touched by the prior
cleanup attempt. These are the substantive bugs:
- get_connection():
- Fix UB on mysql_init failure: mysql_error() was being called on
a NULL MYSQL* when mysql_init returned NULL.
- Fix leak of 'proxysql' handle on mysql_real_connect failure (for
both kAdmin and kMySQL branches) - the function returned NULL
without ever closing the successfully initialized handle.
- get_warnings_count():
- Fix NULL deref: mysql_fetch_row() can return NULL (empty result
or error), and the row[0] access that followed was unguarded.
Now returns -1 and logs diagnostic on empty-row case.
- Simplify the redundant 'if (mysql_result) { ... result = nullptr; }'
since mysql_result was already checked non-NULL above.
- get_warnings():
- Fix potentially-NULL mysql_use_result being passed to
mysql_fetch_row in a while-loop. Added NULL check after the
mysql_use_result call.
- main() admin_tmp discovery block:
- Fix UB: mysql_real_connect() was called on admin_tmp without
first checking that mysql_init() returned non-NULL.
- Fix leak of admin_tmp when mysql_real_connect fails - the whole
if-body (including the mysql_close at the end) was skipped,
leaking the initialized handle. Always close admin_tmp whether
or not the connect succeeds.
- Add row[0] NULL guard before atoi() in the hostgroup-discovery
query to avoid segfault on unexpected NULL column.
- main() dbg_admin debug dump block:
- Same fix as above: dbg_admin handle was leaked when real_connect
failed inside the compound if-condition.
- main() hostgroup fixup loop:
- Fix strdup() leak pattern: each rewrite strdup'd the query string
into info.query_info.query with no matching free. Worse, running
both rewrites on the same query overwrote the first strdup'd
pointer with the second strdup'd one, leaking the first. Replaced
with a std::list<std::string> that owns the rewritten strings
and produces stable c_str() pointers.
- Remove dead MYSQL_QUERY__ macro (zero callers, and the name uses
a reserved-identifier double-underscore suffix).
Compile-verified with -fsyntax-only.
Both kill_connection2-t.cpp and kill_connection3-t.cpp share the same
structure and had the same bugs:
- create_connections(): fix NULL-deref UB when mysql_init returns NULL
(mysql_error was called on the NULL handle) and fix leaks on both
mysql_init failure and mysql_real_connect failure. On any error the
function now closes the in-flight handle and any previously-pushed
conns[] slots before returning.
- find_tids():
- Initialize 'tid' to 0: it was declared uninitialized and read on
the ok() line below, which would read garbage if mysql_fetch_row
returned NULL on the first iteration.
- Check mysql_store_result for NULL before passing to mysql_fetch_row
(was UB on NULL).
- NULL-guard row[0] before atoll to avoid crashes on unexpected NULL
columns.
- main(): fix leaks at all error-return paths:
- NULL-deref UB when mysql_init(proxysql_admin) returned NULL.
- proxysql_admin leaked on mysql_real_connect failure.
- conns[] (5 or 35 connections) leaked across every error-return.
- Most importantly: after the 'wait_timeout' / 'kill' cycle kills
the first batch of conns server-side, main() called create_
connections() a SECOND time, overwriting every conns[i] slot
without closing the now-dead client-side MYSQL* structs. That
dropped all NUM_CONNS MYSQL handles on the floor. Added an
explicit close_all_conns() call between the two batches.
- Happy-path exit now also runs close_all_conns() + mysql_close
(proxysql_admin); previously everything leaked at normal exit.
A new static helper close_all_conns() centralizes the cleanup;
conns[] is file-scope and zero-initialized, so the NULL check in
close_all_conns() is correct at entry and after overwrite.
Compile-verified with -fsyntax-only.
test_connection_annotation-t.cpp:
- Fix wrong-handle diagnostic: after mysql_real_connect(proxysql_admin)
failed, the error path reported mysql_error(proxysql_mysql) — the
handle that had already connected successfully. Reports the failed
admin handle now.
- Fix atoi(row[2]) reading 'srv_host' (a string) as srv_port: the
SELECT column order is ConnUsed, ConnFree, srv_host, srv_port, so
row[3] is the port. The diagnostic was reporting garbage values
like 127 (for host "127.0.0.1") as the port.
- Add mysql_init NULL checks for both proxysql_mysql and
proxysql_admin; fix cross-leaks when one succeeds and the other
fails.
- Fix proxysql_mysql/proxysql_admin leaks on all real_connect and
store_result error paths, plus at happy-path exit.
- NULL-check both mysql_store_result calls before passing the result
to mysql_fetch_row (was UB on NULL).
- Guard the vector subtraction loop against mismatched row counts
using std::min of both vector sizes; previously a shorter 'new_
cur_connections' would be read out of bounds.
mysql-fast_forward-t.cpp:
- Same leak pattern as kill_connection{2,3}-t.cpp: NULL-deref UB on
mysql_init failure, create_connections leaking the in-flight handle
on real_connect failure, and the entire conns[NUM_CONNS] + admin
handle leaked on every exit path.
- Between the two create_connections calls, add explicit
close_all_conns() so the second batch doesn't orphan the first.
- Add NULL check on mysql_store_result(conns[0]) before passing to
mysql_fetch_row in the SELECT * FROM test.sbtest1 loop.
Compile-verified with -fsyntax-only.
- create_connections(): on every failure path (mysql_init failure,
mysql_real_connect failure, NULL ssl cipher, plugin mismatch) the
function returned false without closing the proxy[0..i] slots it
had already opened. Added close_connections() calls so partial
batches don't leak across iterations.
- Reorder the file so close_connections() is defined BEFORE
create_connections(), needed by the new cleanup calls above.
- TestSet1(): each run_queries_sets()/create_connections() error
path now calls close_connections() before returning so we don't
carry dead proxy[] slots into the next TestSet1 invocation.
- main():
- Add mysql_init NULL check on 'admin' before passing to
mysql_real_connect (was UB on NULL).
- Close admin on the 'real_connect failed' error path.
- Fix dead 'cleanup:' label: previously main() ended with a
'return exit_status();' followed by a cleanup: label and a
close_connections() call that was unreachable. Replaced every
error-path 'return exit_status();' inside main() with
'goto cleanup;' so the cleanup label is actually used, and
the label now also closes 'admin'.
Compile-verified with -fsyntax-only.
test_ssl_large_query-1-t.cpp: - Fix leak pattern: mysqladmin + mysql were each leaked on every error- return path (real_connect failure, mysql_init failure, run_queries_sets failure). Introduced a close_all lambda and called it before every early return. test_ssl_large_query-2-t.cpp: - Fix same leak pattern with three handles: mysqladmin, mysqladmin2, mysqllite3. The happy path at end of main() also leaked mysqladmin2 and mysqllite3 (only mysqladmin was closed). - Fix NULL-deref UB inside the per-iteration SELECT loop: the result of mysql_store_result was passed to mysql_fetch_row, then row[0] was passed to strlen(), both without NULL checks. Any failure mode (empty result, connection error) would crash the test. Now guards the result and the row before computing the length. Compile-verified with -fsyntax-only.
…240-}t.cpp
All three files had the same incorrectly-grouped failure check at the
heart of their variable-comparison loop. The intended semantics were:
if special_sqlmode is true AND verified_special_sqlmode is false, fail
or if k == mysql_vars.end(), fail
or if s == proxysql_vars["conn"].end(), fail
or if special_sqlmode is false AND the inner disjunction holds, fail
where the inner disjunction is
(el.key() != "session_track_gtids" && values differ)
OR
(el.key() == "session_track_gtids" && check_session_track_gtids(...) fails)
Because C++ '&&' binds tighter than '||', the source
(special_sqlmode == false &&
(el.key() != "session_track_gtids" && (...)) ||
(el.key() == "session_track_gtids" && !check_session_track_gtids(...))
)
actually parsed as
( special_sqlmode == false && (el.key() != "session_track_gtids" && (...)) )
||
(el.key() == "session_track_gtids" && !check_session_track_gtids(...))
i.e. the 'session_track_gtids' branch was promoted to a standalone
failure condition that fired even when special_sqlmode == true (or in
set_testing-240-t.cpp, also when parsing_optimizer_switch == true).
This caused tests to flag spurious failures whenever session_track_gtids
disagreed in the "we intentionally don't care about this iteration"
cases.
-Wparentheses flagged all three files. Fix is one extra pair of
parentheses around the inner disjunction so it's truly guarded by the
outer special_sqlmode / parsing_optimizer_switch condition.
Compile-verified with -fsyntax-only -Wall on all three files.
…+ leak cleanup
test_ssl_fast_forward-1-t.cpp:
- Fix a bug I introduced in the prior batch: my scripted wrapping of
braceless 'if (cond) return exit_status();' bodies skipped the one
'if' line that had a trailing '// comment' (it didn't end with ')').
The result was a misleading-indentation case where only close_all()
was inside the if body and return exit_status() ran unconditionally.
Added explicit { } around that block.
test_digest_umap_aux-t.cpp:
- Fix real logic bug: 'tests_failed == 0' (missing parentheses) compared
the *address* of the function against zero. The address is never NULL,
so the second operand of the '&&' was always false, and the
TRUNCATE TABLE stats.stats_mysql_query_digest cleanup at end of main
never actually ran. Fixed to 'tests_failed() == 0'. Surfaced by
-Waddress.
test_default_conn_collation-t.cpp:
- Fix NULL-deref UB: mysql_init was not checked for NULL before being
passed to mysql_real_connect.
- Fix 'admin' leak on mysql_real_connect failure.
- Remove the dead 'cleanup:' label: the label's cleanup code
(mysql_close + return exit_status) was unreachable because the
preceding happy-path flow was a plain fall-through into it. Moved
the cleanup body inline and deleted the label.
Compile-verified with -fsyntax-only (-Wall for the first two files).
…rs_defaults-t.cpp
-Wunused-label flagged 'cleanup:' as defined-but-not-used. This isn't
just stylistic: the cleanup block contained the critical disk-state
restore (DELETE FROM disk.mysql_hostgroup_attributes + INSERT FROM
backup mysql_hostgroup_attributes_0508) plus mysql_close(admin). Any
test failure (the four MYSQL_QUERY_T calls in main, which return
EXIT_FAILURE on query failure, or a real_connect failure) would
return from main() without running it, leaving:
1. the admin MYSQL* leaked
2. disk.mysql_hostgroup_attributes in whatever half-modified state
the test left it in - corrupting subsequent tests that read the
same table.
Also fixed NULL-deref UB: mysql_init's return value was not checked
before being passed to mysql_real_connect.
Restructure main():
- NULL-check mysql_init, close admin on real_connect failure.
- Track whether the backup table has been created with a 'backup_
created' flag; the restore path only runs when it is.
- Introduce a run_cleanup_and_exit lambda that runs the restore (if
needed) and closes admin. Every error path that can reach it -
plus the happy-path return - goes through it.
- Replace the MYSQL_QUERY_T calls at the top of main with explicit
mysql_query_t + run_cleanup_and_exit, so query failures no longer
skip the restore.
- Delete the now-dead 'cleanup:' label.
Compile-verified with -fsyntax-only.
…g_file-t.cpp Same pattern as the sibling servers_defaults test: -Wunused-label flagged the 'cleanup:' block, which held the only restore-from-backup path for the mysql_hostgroup_attributes runtime table. Nothing ever jumped to it, so every MYSQL_QUERY_T early-return from the test body skipped the restore and leaked the admin MYSQL* handle. - Add mysql_init NULL check and close admin on real_connect failure. - Introduce a run_cleanup_and_exit lambda that performs the DELETE/INSERT restore (conditional on backup_created) and closes admin. - Replace the MYSQL_QUERY_T backup-creation calls with explicit mysql_query_t + run_cleanup_and_exit so any failure goes through the cleanup path. - Delete the dead 'cleanup:' label.
reg_test_mariadb_metadata_check-t.cpp: - Remove the dead 'cleanup:' label in main(). The label was empty (no cleanup body, just fell through to return exit_status()), so deleting it is purely warning cleanup; no behavior change. test_rw_binary_data-t.cpp: - Fix NULL-deref UB and cross-leak: proxy and admin were each mysql_ init()'d without a NULL check, then immediately passed to mysql_ real_connect. On either side failing, the other side's already- initialized handle was leaked because every error path did a bare 'return EXIT_FAILURE'. - Remove the unused 'cleanup:' label at the end of main(). The label was fall-through-only (nothing ever jumped to it) which is why -Wunused-label fired; behavior is unchanged because the happy path still runs mysql_close on both handles. - Added a TODO comment noting that many MYSQL_QUERY calls in the test body still leak proxy/admin on query failure; restructuring the whole main() is out of scope for this batch.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp (1)
159-227:⚠️ Potential issue | 🟠 MajorUse the lambda's status argument and fail on restore errors.
run_cleanup_and_exit(EXIT_FAILURE)currently discards the requested status and still returnsexit_status(). If the restore back intodisk.mysql_hostgroup_attributesfails after the TAP checks passed, this test can still exit 0 while leaving shared disk state dirty.Suggested fix
- auto run_cleanup_and_exit = [&](int /*unused*/) -> int { + auto run_cleanup_and_exit = [&](int rc) -> int { + bool cleanup_failed = false; if (backup_created) { if (mysql_query_t(admin, "DELETE FROM disk.mysql_hostgroup_attributes")) { fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + cleanup_failed = true; } if (mysql_query_t(admin, "INSERT INTO disk.mysql_hostgroup_attributes SELECT * FROM mysql_hostgroup_attributes_0508")) { fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); + cleanup_failed = true; } } mysql_close(admin); - return exit_status(); + if (rc != EXIT_SUCCESS || cleanup_failed) { + return EXIT_FAILURE; + } + return exit_status(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp` around lines 159 - 227, The cleanup lambda run_cleanup_and_exit currently ignores its int parameter and always returns exit_status(), so restore failures can be masked; change run_cleanup_and_exit to honor the int status argument (return that value) and inside its restore block set that status to EXIT_FAILURE if any mysql_query_t (DELETE/INSERT restoring disk.mysql_hostgroup_attributes) fails so failures during cleanup propagate; also update all callers (e.g., the early returns after mysql_query_t failures and the final return) to pass the desired EXIT_SUCCESS/EXIT_FAILURE into run_cleanup_and_exit instead of relying on exit_status().test/tap/tests/kill_connection3-t.cpp (1)
60-74:⚠️ Potential issue | 🟠 MajorFail fast when
SELECT CONNECTION_ID()returns no usable row.Initializing
tidto0removes the uninitialized read, but this path still succeeds when the result set is empty orrow[0]isNULL. That writes0intomythreadid[i], so the laterKILL CONNECTION ...loop starts operating on an invalid thread id and the test can cascade into misleading failures instead of stopping at the real problem.Suggested fix
int find_tids() { for (int i = 0; i < NUM_CONNS ; i++) { MYSQL * mysql = conns[i]; - unsigned long tid = 0; // initialize: avoid uninit read if loop empty + unsigned long tid = 0; + bool found_tid = false; MYSQL_ROW row; MYSQL_QUERY(mysql, "SELECT CONNECTION_ID()"); MYSQL_RES * proxy_res = mysql_store_result(mysql); if (!proxy_res) { fprintf(stderr, "File %s, line %d, Error: mysql_store_result returned NULL: %s\n", __FILE__, __LINE__, mysql_error(mysql)); return EXIT_FAILURE; } while ((row = mysql_fetch_row(proxy_res))) { - if (row[0] != NULL) { tid = atoll(row[0]); } + if (row[0] != NULL) { + tid = atoll(row[0]); + found_tid = true; + } } mysql_free_result(proxy_res); + if (!found_tid) { + fprintf(stderr, "File %s, line %d, Error: empty result from SELECT CONNECTION_ID()\n", + __FILE__, __LINE__); + return EXIT_FAILURE; + } ok(tid == mysql_thread_id(mysql), "tid: %lu, mysql_thread_id(): %lu", tid, mysql_thread_id(mysql)); mythreadid[i] = tid; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/kill_connection3-t.cpp` around lines 60 - 74, The test currently initializes tid to 0 and proceeds even if SELECT CONNECTION_ID() yields no rows or row[0] is NULL, causing an invalid thread id to be stored in mythreadid[i]; change the logic around the mysql_store_result/mysql_fetch_row loop (the block using MYSQL_RES *proxy_res, mysql_fetch_row and tid) so that if no row is fetched or row[0] is NULL you print a descriptive error (using the existing fprintf or ok macro) and fail fast (return EXIT_FAILURE) instead of writing 0 into mythreadid[i]; only assign mythreadid[i] = tid after confirming a valid non-NULL row and a successful atoll conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/mysql_hostgroup_attributes_config_file-t.cpp`:
- Around line 258-289: run_cleanup_and_exit currently always returns
exit_status(), hiding failures from mysql_query_t during backup/restore; change
it to track and propagate errors by introducing a local status variable (e.g.,
int cleanup_rc = 0) inside run_cleanup_and_exit and set cleanup_rc to a non-zero
value when mysql_query_t(admin, ...) fails (and preserve/propagate
mysql_error(admin) info), then return cleanup_rc instead of always returning
exit_status(); also ensure callers (the DROP/CREATE failure branches and final
return) return the run_cleanup_and_exit() value so setup/cleanup failures from
mysql_query_t and restore attempts are surfaced (references:
run_cleanup_and_exit, backup_created, mysql_query_t, exit_status,
validate_mysql_hostgroup_attributes_from_config,
write_mysql_hostgroup_attributes_to_config).
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp`:
- Around line 213-217: The temp-file write currently ignores errors from
fprintf/fflush/fclose; modify the helper so that you check each return value
(fprintf should return a non-negative value, fflush returns 0, fclose returns 0)
after writing inv_cert to tmp_file, and on any failure unlink the temp path and
fail fast (return an error code or assert/fail the test) instead of proceeding;
reference the local tmp_file, inv_cert, and the calls to fprintf/fflush/fclose
to locate where to add the checks and cleanup.
In `@test/tap/tests/set_testing-multi-t.cpp`:
- Around line 283-288: The condition dereferences iterators k and s without
ensuring they are valid when special_sqlmode is false, causing crashes if those
keys are missing; update the compound if to first check that k and s were found
(e.g., k != vars.end() and s != vars.end()) before calling k.value() or
s.value(), and only perform values_equiv(...) or check_session_track_gtids(...)
when the corresponding iterators are valid; modify the branches around el, k, s
so the non-session_track_gtids path requires both k and s to be present before
comparing, and the session_track_gtids path similarly guards calls to
check_session_track_gtids(el.value(), s.value(), k.value()) with existence
checks for k and s.
In `@test/tap/tests/test_connection_annotation-t.cpp`:
- Around line 197-203: Add the missing `#include` <algorithm> and stop silently
truncating when computing deltas: instead of taking std::min over
cur_connections and new_cur_connections, assert or fail fast if
cur_connections.size() != new_cur_connections.size() (so the test detects
backend count changes), then compute new_total_conn by iterating both vectors of
equal length; reference the existing symbols compare_n, cur_connections,
new_cur_connections, and new_total_conn when locating the change. Optionally, if
preserving correctness across reorderings is desired, replace index-based delta
logic with a match-by-identity approach using backend identifiers
(srv_host/srv_port) to pair entries before summing differences.
In `@test/tap/tests/test_stats_proxysql_message_metrics-t.cpp`:
- Around line 44-45: The test calls mysql_store_result(proxy_admin)
unconditionally and assigns it to MysqlRes reset_res before checking the query
return code (rc), which can hide failures; update the code in the test so that
you check rc immediately after the query and only call
mysql_store_result(proxy_admin) when rc == EXIT_SUCCESS, then validate the
returned MYSQL_RES* (e.g., confirm reset_res is non-null or use
mysql_field_count/mysql_errno) before using it and return EXIT_FAILURE or handle
the error if the result is null; refer to mysql_store_result, reset_res and
proxy_admin to locate where to move the call and add the null/result checks.
- Around line 107-109: The test dereferences the result of mysql_store_result()
without checking for nullptr (in the block using MYSQL_QUERY(proxy_admin.get(),
...) and creating MysqlRes), so wrap the calls that build MysqlRes for both
occurrences (around mysql_store_result(...) used with MysqlRes and later when
calling extract_mysql_rows) with a null check: after calling
mysql_store_result(proxy_admin.get()) save the raw MYSQL_RES* into a variable,
if it is nullptr fail the test with a clear diagnostic (or return/ASSERT/FAIL
per the test harness) and only then construct MysqlRes and call mysql_num_rows
or extract_mysql_rows; reference mysql_store_result, MysqlRes, mysql_num_rows,
extract_mysql_rows, MYSQL_QUERY and proxy_admin in your changes.
---
Outside diff comments:
In `@test/tap/tests/kill_connection3-t.cpp`:
- Around line 60-74: The test currently initializes tid to 0 and proceeds even
if SELECT CONNECTION_ID() yields no rows or row[0] is NULL, causing an invalid
thread id to be stored in mythreadid[i]; change the logic around the
mysql_store_result/mysql_fetch_row loop (the block using MYSQL_RES *proxy_res,
mysql_fetch_row and tid) so that if no row is fetched or row[0] is NULL you
print a descriptive error (using the existing fprintf or ok macro) and fail fast
(return EXIT_FAILURE) instead of writing 0 into mythreadid[i]; only assign
mythreadid[i] = tid after confirming a valid non-NULL row and a successful atoll
conversion.
In `@test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp`:
- Around line 159-227: The cleanup lambda run_cleanup_and_exit currently ignores
its int parameter and always returns exit_status(), so restore failures can be
masked; change run_cleanup_and_exit to honor the int status argument (return
that value) and inside its restore block set that status to EXIT_FAILURE if any
mysql_query_t (DELETE/INSERT restoring disk.mysql_hostgroup_attributes) fails so
failures during cleanup propagate; also update all callers (e.g., the early
returns after mysql_query_t failures and the final return) to pass the desired
EXIT_SUCCESS/EXIT_FAILURE into run_cleanup_and_exit instead of relying on
exit_status().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e456d9fa-570c-40bc-8038-446f83dd38f1
📒 Files selected for processing (22)
test/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql-fast_forward-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/reg_test_mariadb_metadata_check-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpp
💤 Files with no reviewable changes (1)
- test/tap/tests/reg_test_mariadb_metadata_check-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use UPPER_SNAKE_CASE for constants and macros
Files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
test/tap/tests/**/*(test_*.cpp|*-t.cpp)
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow the naming pattern
test_*.cppor*-t.cpp
Files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
test/tap/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
🧠 Learnings (10)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/**/*(test_*.cpp|*-t.cpp) : Test files follow the naming pattern `test_*.cpp` or `*-t.cpp`
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Applied to files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-2-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, and similar feature flags for feature tiers
Applied to files:
test/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_ssl_fast_forward-1-t.cpptest/tap/tests/test_ssl_large_query-1-t.cpptest/tap/tests/mysql_hostgroup_attributes_config_file-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_change_user-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/set_testing-240-t.cpptest/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/set_testing-multi-t.cpptest/tap/tests/set_testing-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_connection_annotation-t.cpptest/tap/tests/test_warnings-t.cpp
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to lib/**/*.{cpp,h,hpp} : Write class names in PascalCase with protocol prefixes (`MySQL_`, `PgSQL_`, `ProxySQL_`)
Applied to files:
test/tap/tests/test_rw_binary_data-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_stats_proxysql_message_metrics-t.cpptest/tap/tests/test_warnings-t.cpptest/tap/tests/mysql-fast_forward-t.cpp
📚 Learning: 2026-02-13T09:29:39.713Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.
Applied to files:
test/tap/tests/test_connection_annotation-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/test_cluster1-t.cpp
[error] 1-1: 'memory' file not found
(clang-diagnostic-error)
test/tap/tests/test_stats_proxysql_message_metrics-t.cpp
[error] 7-7: 'memory' file not found
(clang-diagnostic-error)
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
[error] 21-21: 'cerrno' file not found
(clang-diagnostic-error)
🔇 Additional comments (28)
test/tap/tests/test_digest_umap_aux-t.cpp (1)
318-321: Correct fix fortests_failedinvocation and cleanup gating.Calling
tests_failed()here is the right fix; this now correctly allows theTRUNCATEonly when all planned tests ran and none failed.test/tap/tests/set_testing-t.cpp (1)
313-326: Grouping fix looks correct.This keeps the
session_track_gtidsbranch inside thespecial_sqlmode == falsepath while preserving the earlierk/send-check short-circuiting.test/tap/tests/set_testing-240-t.cpp (1)
412-426: Precedence fix is solid here too.The extra grouping correctly limits the
session_track_gtidsfallback to thespecial_sqlmode == false && parsing_optimizer_switch == falsecase.test/tap/tests/test_change_user-t.cpp (3)
69-78: Idempotent proxy teardown looks good.The null check plus slot reset makes
close_connections()safe to reuse from multiple error paths without double-closing handles.
89-127: Nice coverage of early-return cleanup paths.Adding
close_connections()before these failure returns keeps later test iterations isolated from partially opened state.Also applies to: 153-167
254-265: Themain()teardown flow is much clearer now.The explicit
admin == NULLguard and sharedcleanup:block make the shutdown path reachable and consistent for the batch-return cases.Also applies to: 283-347
test/tap/tests/test_default_conn_collation-t.cpp (1)
89-106: Good defensive handling around admin connection setup.The added
mysql_init()guard and the explicitmysql_close(admin)onmysql_real_connect()failure make the test setup more robust without changing the happy path.test/tap/tests/mysql_hostgroup_attributes_config_file-t.cpp (1)
237-247: Nice setup hardening for the admin handle.This makes the failure mode around
mysql_init()/mysql_real_connect()explicit and keeps the connection path tidy.test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp (1)
137-147: Good guardrails on admin initialization.The null-check after
mysql_init()and the close on failedmysql_real_connect()are solid additions.test/tap/tests/test_cluster1-t.cpp (3)
143-171: Good hardening increate_connections()error paths.Null-checking
mysql_init()and closing handles onmysql_real_connect()failure improves test robustness without changing assertions.
179-186: Cleanup flow is solid and deterministic.
close_all_connections()plus calls on both failure and success paths makes teardown behavior clearer and safer.Also applies to: 293-296, 333-333
1-1: No changes required for#include <memory>inclusion.The file
test_cluster1-t.cppalready contains#include <memory>(not newly introduced by this PR). The build system is properly configured with C++17 support via the$(STDCPP)variable, which resolves the standard library headers. The generic compilation rule%-t: %-t.cppuses this configuration and includes the necessary flags. If this were blocking compilation, the file would not have been buildable in prior commits. No action needed.test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp (5)
21-21: Helpful errno-based diagnostics on the temp-file path.This makes setup failures much easier to root-cause during TAP runs.
Also applies to: 205-212
151-163: Nice hardening of the SSL failure helpers.The
mysql_init/tempnam/fopenguards keep these negative tests from crashing or contaminating later checks when setup itself fails.Also applies to: 173-174, 193-212, 228-230
378-383: Good catch closing backend handles on setup/query failure.This keeps the helper behavior deterministic when the backend is unavailable or the database creation step fails.
Also applies to: 402-416
447-483: Centralizing warmup rollback improves pool-setup determinism.
cleanup_and_failmakes the partial-warmup path much easier to reason about, and the unsigned loop/format cleanup removes a common source of noisy warnings.
535-562: Freeing the stats result sets here is a solid cleanup.These diagnostics run multiple times in one process, so releasing each
MYSQL_RESavoids carrying stale allocations across phases.test/tap/tests/test_stats_proxysql_message_metrics-t.cpp (2)
21-25: Nice RAII cleanup here.Wrapping
MYSQL*andMYSQL_RES*instd::unique_ptrkeeps the early-return-heavy test flow much safer and easier to follow.
7-7:<memory>is properly included with correct C++ standard library configuration.The build system is configured correctly. Line 7's
#include <memory>is compiled with proper C++ standard flags: the Makefile (line 188-189) uses$(CXX) $< $(IDIRS) $(LDIRS) $(OPT)..., where$(OPT)includes$(STDCPP), which expands to-std=c++XX -DCXXXX. No static analysis failure with'memory' file not foundwas found in the repository, and the compilation configuration includes the necessary C++ standard library headers.> Likely an incorrect or invalid review comment.test/tap/tests/test_rw_binary_data-t.cpp (1)
397-420: Good partial-init cleanup.The new
mysql_init/mysql_real_connectfailure handling now tears down the other handle consistently, which makes the early-exit path much safer.test/tap/tests/test_ssl_large_query-1-t.cpp (1)
60-65: Centralizing the connection teardown here is a nice improvement.
close_all()makes the early-return paths consistent and avoids leaving stale handles behind in the happy path too.Also applies to: 74-75, 79-82, 85-88, 94-95, 101-104, 119-125
test/tap/tests/test_ssl_large_query-2-t.cpp (1)
49-58: Nice hardening.The shared cleanup plus the explicit
mysql_store_result()guards remove the main null/partial-init failure cases in this test.Also applies to: 67-68, 73-76, 79-83, 89-90, 98-102, 108-109, 124-145, 150-150
test/tap/tests/test_ssl_fast_forward-1-t.cpp (1)
98-113: The sharedclose_all()helper is a solid fit here.Nulling the slots after close makes the repeated early-return paths and the second connection batch much less error-prone.
Also applies to: 172-175, 223-225, 247-249, 321-323
test/tap/tests/test_binlog_reader-t.cpp (1)
161-178: Nice hardening around the failure paths and result handling.Closing the partially-initialized handles and validating the result shape before indexing into
res_row[0][1]removes a couple of real crash paths.Also applies to: 359-370, 404-426, 456-482
test/tap/tests/mysql-fast_forward-t.cpp (1)
26-35: This connection-array cleanup is a worthwhile improvement.Closing and nulling the stored handles before reuse makes the two-phase setup much safer, and the new
mysql_store_result()guard removes a null-result crash path.Also applies to: 41-50, 170-172, 208-216, 254-255
test/tap/tests/kill_connection2-t.cpp (1)
25-36: Good defensive cleanup pass.
close_all_conns()plus thefind_tids()null/initialization guards make the second connection phase and error handling much more robust.Also applies to: 61-72, 170-173, 220-221
test/tap/tests/test_warnings-t.cpp (2)
177-210: Good hardening aroundSHOW COUNT(*) WARNINGS/SHOW WARNINGS.The added NULL/empty-result checks and unconditional
mysql_free_result()on the acquired-result path make these helpers much safer without changing their contract.
592-618: The rewritten-query ownership fix looks solid.Keeping the rewritten SQL in
std::list<std::string>is a clean way to preserve stablec_str()pointers across bothexecute_tests()passes and avoid the old overwrite-and-leak pattern.
Comment on lines
+258
to
+289
| auto run_cleanup_and_exit = [&]() -> int { | ||
| if (backup_created) { | ||
| if (mysql_query_t(admin, "DELETE FROM mysql_hostgroup_attributes")) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", | ||
| __FILE__, __LINE__, mysql_error(admin)); | ||
| } | ||
| if (mysql_query_t(admin, "INSERT INTO mysql_hostgroup_attributes SELECT * FROM mysql_hostgroup_attributes_0508")) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", | ||
| __FILE__, __LINE__, mysql_error(admin)); | ||
| } | ||
| } | ||
| mysql_close(admin); | ||
| return exit_status(); | ||
| }; | ||
|
|
||
| // For cleanup: create a backup of the current runtime state. Use | ||
| // explicit mysql_query_t so failures go through run_cleanup_and_exit | ||
| // rather than being swallowed by MYSQL_QUERY_T's early return. | ||
| if (mysql_query_t(admin, "DROP TABLE IF EXISTS mysql_hostgroup_attributes_0508")) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); | ||
| return run_cleanup_and_exit(); | ||
| } | ||
| if (mysql_query_t(admin, "CREATE TABLE mysql_hostgroup_attributes_0508 AS SELECT * FROM mysql_hostgroup_attributes")) { | ||
| fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin)); | ||
| return run_cleanup_and_exit(); | ||
| } | ||
| backup_created = true; | ||
|
|
||
| validate_mysql_hostgroup_attributes_from_config(admin); | ||
| write_mysql_hostgroup_attributes_to_config(admin); | ||
|
|
||
| cleanup: | ||
|
|
||
| MYSQL_QUERY_T(admin, "DELETE FROM mysql_hostgroup_attributes"); | ||
| MYSQL_QUERY_T(admin, "INSERT INTO mysql_hostgroup_attributes SELECT * FROM mysql_hostgroup_attributes_0508"); | ||
| mysql_close(admin); | ||
|
|
||
| return exit_status(); | ||
| return run_cleanup_and_exit(); |
There was a problem hiding this comment.
Propagate cleanup/setup failures instead of always returning exit_status().
run_cleanup_and_exit() currently returns exit_status() even when backup creation or restore queries fail. That can report a green test while the cleanup actually failed and mysql_hostgroup_attributes stayed modified for subsequent tests.
Suggested fix
- auto run_cleanup_and_exit = [&]() -> int {
+ auto run_cleanup_and_exit = [&](int rc) -> int {
+ bool cleanup_failed = false;
if (backup_created) {
if (mysql_query_t(admin, "DELETE FROM mysql_hostgroup_attributes")) {
fprintf(stderr, "File %s, line %d, Error: %s\n",
__FILE__, __LINE__, mysql_error(admin));
+ cleanup_failed = true;
}
if (mysql_query_t(admin, "INSERT INTO mysql_hostgroup_attributes SELECT * FROM mysql_hostgroup_attributes_0508")) {
fprintf(stderr, "File %s, line %d, Error: %s\n",
__FILE__, __LINE__, mysql_error(admin));
+ cleanup_failed = true;
}
}
mysql_close(admin);
- return exit_status();
+ if (rc != EXIT_SUCCESS || cleanup_failed) {
+ return EXIT_FAILURE;
+ }
+ return exit_status();
};
@@
- return run_cleanup_and_exit();
+ return run_cleanup_and_exit(EXIT_FAILURE);
@@
- return run_cleanup_and_exit();
+ return run_cleanup_and_exit(EXIT_FAILURE);
@@
- return run_cleanup_and_exit();
+ return run_cleanup_and_exit(EXIT_SUCCESS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/mysql_hostgroup_attributes_config_file-t.cpp` around lines 258
- 289, run_cleanup_and_exit currently always returns exit_status(), hiding
failures from mysql_query_t during backup/restore; change it to track and
propagate errors by introducing a local status variable (e.g., int cleanup_rc =
0) inside run_cleanup_and_exit and set cleanup_rc to a non-zero value when
mysql_query_t(admin, ...) fails (and preserve/propagate mysql_error(admin)
info), then return cleanup_rc instead of always returning exit_status(); also
ensure callers (the DROP/CREATE failure branches and final return) return the
run_cleanup_and_exit() value so setup/cleanup failures from mysql_query_t and
restore attempts are surfaced (references: run_cleanup_and_exit, backup_created,
mysql_query_t, exit_status, validate_mysql_hostgroup_attributes_from_config,
write_mysql_hostgroup_attributes_to_config).
Comment on lines
+213
to
+217
| // Use an explicit format string; inv_cert is a const buffer but passing it | ||
| // as the format argument is a format-string pattern smell. | ||
| fprintf(tmp_file, "%s", inv_cert); | ||
| fflush(tmp_file); | ||
| fclose(tmp_file); |
There was a problem hiding this comment.
Check the temp-file write before using it.
If fprintf, fflush, or fclose fails here, this helper can silently stop exercising the intended invalid-cert path and collapse into a different failure mode. Please fail fast and clean up the temp path on write/flush/close errors.
Proposed fix
// Use an explicit format string; inv_cert is a const buffer but passing it
// as the format argument is a format-string pattern smell.
- fprintf(tmp_file, "%s", inv_cert);
- fflush(tmp_file);
- fclose(tmp_file);
+ int write_rc = fprintf(tmp_file, "%s", inv_cert);
+ int flush_rc = fflush(tmp_file);
+ int close_rc = fclose(tmp_file);
+ if (write_rc < 0 || flush_rc != 0 || close_rc != 0) {
+ fprintf(stderr, "File %s, line %d, Error: writing '%s' failed: %s\n",
+ __FILE__, __LINE__, inv_cert_path, strerror(errno));
+ mysql_close(myconn);
+ unlink(inv_cert_path);
+ free(inv_cert_path);
+ return NULL;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp` around lines 213 - 217,
The temp-file write currently ignores errors from fprintf/fflush/fclose; modify
the helper so that you check each return value (fprintf should return a
non-negative value, fflush returns 0, fclose returns 0) after writing inv_cert
to tmp_file, and on any failure unlink the temp path and fail fast (return an
error code or assert/fail the test) instead of proceeding; reference the local
tmp_file, inv_cert, and the calls to fprintf/fflush/fclose to locate where to
add the checks and cleanup.
Comment on lines
283
to
+288
| if ( | ||
| (special_sqlmode == true && verified_special_sqlmode == false) || | ||
| (special_sqlmode == false && | ||
| (special_sqlmode == false && ( | ||
| (el.key() != "session_track_gtids" && (!values_equiv(k.value(), el.value()) || !values_equiv(s.value(), el.value()))) || | ||
| (el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value())) | ||
| ) | ||
| )) |
There was a problem hiding this comment.
Guard missing JSON lookups before calling value().
Lines 233-239 only log when k or s is missing; this condition still dereferences both iterators whenever special_sqlmode == false. That turns a missing variable into a crash/abort instead of a normal TAP failure. The sibling set-testing variants already guard these lookups before comparing values.
Proposed fix
if (
(special_sqlmode == true && verified_special_sqlmode == false) ||
+ (k == mysql_vars.end()) ||
+ (s == proxysql_vars["conn"].end()) ||
(special_sqlmode == false && (
(el.key() != "session_track_gtids" && (!values_equiv(k.value(), el.value()) || !values_equiv(s.value(), el.value()))) ||
(el.key() == "session_track_gtids" && !check_session_track_gtids(el.value(), s.value(), k.value()))
))
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/set_testing-multi-t.cpp` around lines 283 - 288, The condition
dereferences iterators k and s without ensuring they are valid when
special_sqlmode is false, causing crashes if those keys are missing; update the
compound if to first check that k and s were found (e.g., k != vars.end() and s
!= vars.end()) before calling k.value() or s.value(), and only perform
values_equiv(...) or check_session_track_gtids(...) when the corresponding
iterators are valid; modify the branches around el, k, s so the
non-session_track_gtids path requires both k and s to be present before
comparing, and the session_track_gtids path similarly guards calls to
check_session_track_gtids(el.value(), s.value(), k.value()) with existence
checks for k and s.
Comment on lines
+197
to
203
| // Sum the differences between previous free and new free connections. | ||
| // If either side came back shorter (row count mismatch), use the | ||
| // smaller size so we don't read past the end of new_cur_connections. | ||
| size_t compare_n = std::min(cur_connections.size(), new_cur_connections.size()); | ||
| for (size_t i = 0; i < compare_n; i++) { | ||
| new_total_conn += new_cur_connections[i] - cur_connections[i]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test/tap/tests/test_connection_annotation-t.cpp"
echo "std::min usage:"
rg -n '\bstd::min\s*\(' "$FILE"
echo
echo "Direct standard-library includes in file:"
rg -n '^#include\s*<[^>]+>' "$FILE"
echo
echo "Expected result:"
echo "- std::min is used in the comparison logic"
echo "- `#include` <algorithm> is absent"Repository: sysown/proxysql
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test/tap/tests/test_connection_annotation-t.cpp"
echo "=== Context around line 197-203 (lines 190-210) ==="
sed -n '190,210p' "$FILE"
echo
echo "=== Full header includes ==="
head -20 "$FILE"
echo
echo "=== Check if std::min is available via transitive includes ==="
echo "Checking if <vector> or <string> transitively includes <algorithm>"
# In standard C++, neither <vector> nor <string> directly includes <algorithm>
# But some implementations might. We'll check the actual behavior.
rg -B5 -A5 'std::min' "$FILE"Repository: sysown/proxysql
Length of output: 2321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="test/tap/tests/test_connection_annotation-t.cpp"
echo "=== Checking if this is a TAP test ==="
rg -n 'plan\(|diag\(|ok\(|bail_out' "$FILE" | head -20
echo
echo "=== Examining the full comparison logic context ==="
sed -n '195,210p' "$FILE"
echo
echo "=== Check what happens with size mismatch ==="
sed -n '185,215p' "$FILE"Repository: sysown/proxysql
Length of output: 3540
Add <algorithm> include and fail fast on backend count mismatch.
The code uses std::min() on line 200 but doesn't include <algorithm> directly. More critically, silently truncating to the shorter vector masks topology changes—if backends are added or removed between queries, the delta calculation will be wrong and the test assertion could pass with incorrect values. Either check that both vectors have equal size and fail if they don't, or match backends by identity (srv_host/srv_port) instead of relying on vector index order.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_connection_annotation-t.cpp` around lines 197 - 203, Add
the missing `#include` <algorithm> and stop silently truncating when computing
deltas: instead of taking std::min over cur_connections and new_cur_connections,
assert or fail fast if cur_connections.size() != new_cur_connections.size() (so
the test detects backend count changes), then compute new_total_conn by
iterating both vectors of equal length; reference the existing symbols
compare_n, cur_connections, new_cur_connections, and new_total_conn when
locating the change. Optionally, if preserving correctness across reorderings is
desired, replace index-based delta logic with a match-by-identity approach using
backend identifiers (srv_host/srv_port) to pair entries before summing
differences.
Comment on lines
+44
to
+45
| MysqlRes reset_res{mysql_store_result(proxy_admin), &mysql_free_result}; | ||
| if (rc != EXIT_SUCCESS) { return EXIT_FAILURE; } |
There was a problem hiding this comment.
Check the query result before calling mysql_store_result().
Line 44 runs mysql_store_result() even when the reset query failed, and it never validates the returned MYSQL_RES*. That makes the failure path harder to reason about and can hide a real fetch/result error.
Proposed fix
int rc = mysql_query(proxy_admin, "SELECT * FROM stats.stats_proxysql_message_metrics_reset");
ok(rc == EXIT_SUCCESS, "Successfully queries 'stats.stats_proxysql_message_metrics_reset'");
- MysqlRes reset_res{mysql_store_result(proxy_admin), &mysql_free_result};
- if (rc != EXIT_SUCCESS) { return EXIT_FAILURE; }
+ if (rc != EXIT_SUCCESS) { return EXIT_FAILURE; }
+ MysqlRes reset_res{mysql_store_result(proxy_admin), &mysql_free_result};
+ if (!reset_res) {
+ diag("mysql_store_result() failed for reset query: %s", mysql_error(proxy_admin));
+ return EXIT_FAILURE;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| MysqlRes reset_res{mysql_store_result(proxy_admin), &mysql_free_result}; | |
| if (rc != EXIT_SUCCESS) { return EXIT_FAILURE; } | |
| int rc = mysql_query(proxy_admin, "SELECT * FROM stats.stats_proxysql_message_metrics_reset"); | |
| ok(rc == EXIT_SUCCESS, "Successfully queries 'stats.stats_proxysql_message_metrics_reset'"); | |
| if (rc != EXIT_SUCCESS) { return EXIT_FAILURE; } | |
| MysqlRes reset_res{mysql_store_result(proxy_admin), &mysql_free_result}; | |
| if (!reset_res) { | |
| diag("mysql_store_result() failed for reset query: %s", mysql_error(proxy_admin)); | |
| return EXIT_FAILURE; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_stats_proxysql_message_metrics-t.cpp` around lines 44 -
45, The test calls mysql_store_result(proxy_admin) unconditionally and assigns
it to MysqlRes reset_res before checking the query return code (rc), which can
hide failures; update the code in the test so that you check rc immediately
after the query and only call mysql_store_result(proxy_admin) when rc ==
EXIT_SUCCESS, then validate the returned MYSQL_RES* (e.g., confirm reset_res is
non-null or use mysql_field_count/mysql_errno) before using it and return
EXIT_FAILURE or handle the error if the result is null; refer to
mysql_store_result, reset_res and proxy_admin to locate where to move the call
and add the null/result checks.
Comment on lines
+107
to
+109
| MYSQL_QUERY(proxy_admin.get(), "SELECT * FROM stats.stats_proxysql_message_metrics"); | ||
| MysqlRes my_res{mysql_store_result(proxy_admin.get()), &mysql_free_result}; | ||
| uint64_t num_rows = mysql_num_rows(my_res.get()); |
There was a problem hiding this comment.
Guard both mysql_store_result() calls before dereferencing them.
mysql_num_rows(my_res.get()) and extract_mysql_rows(my_res.get()) both assume mysql_store_result() succeeded. If the client returns nullptr here, this test will crash instead of failing cleanly with diagnostics.
Proposed fix
MYSQL_QUERY(proxy_admin.get(), "SELECT * FROM stats.stats_proxysql_message_metrics");
MysqlRes my_res{mysql_store_result(proxy_admin.get()), &mysql_free_result};
+ if (!my_res) {
+ diag("mysql_store_result() failed for initial metrics query: %s", mysql_error(proxy_admin.get()));
+ return EXIT_FAILURE;
+ }
uint64_t num_rows = mysql_num_rows(my_res.get());
@@
MYSQL_QUERY(proxy_admin.get(), "SELECT * FROM stats.stats_proxysql_message_metrics");
// unique_ptr::reset() frees the previous result set before taking
// ownership of the new one.
my_res.reset(mysql_store_result(proxy_admin.get()));
+ if (!my_res) {
+ diag("mysql_store_result() failed for refreshed metrics query: %s", mysql_error(proxy_admin.get()));
+ return EXIT_FAILURE;
+ }
vector<vector<string>> refreshed_rows = extract_mysql_rows(my_res.get());Also applies to: 158-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_stats_proxysql_message_metrics-t.cpp` around lines 107 -
109, The test dereferences the result of mysql_store_result() without checking
for nullptr (in the block using MYSQL_QUERY(proxy_admin.get(), ...) and creating
MysqlRes), so wrap the calls that build MysqlRes for both occurrences (around
mysql_store_result(...) used with MysqlRes and later when calling
extract_mysql_rows) with a null check: after calling
mysql_store_result(proxy_admin.get()) save the raw MYSQL_RES* into a variable,
if it is nullptr fail the test with a clear diagnostic (or return/ASSERT/FAIL
per the test harness) and only then construct MysqlRes and call mysql_num_rows
or extract_mysql_rows; reference mysql_store_result, MysqlRes, mysql_num_rows,
extract_mysql_rows, MYSQL_QUERY and proxy_admin in your changes.
Builds ProxySQL with PROXYSQLGENAI=1 + WITHASAN=1 + WITHGCOV=1 (and the NOJEMALLOC=1 that ASAN forces), then builds and runs every unit test under test/tap/tests/unit/ and publishes an LCOV coverage report. Self-contained design: unlike the existing CI-taptests-asan workflow this one runs entirely on the GitHub runner. It does not depend on the private proxysql/jenkins-build-scripts repo or any backend MySQL/Docker infrastructure because unit tests link against libproxysql.a via the stub test_globals.o and run as standalone binaries. Full job definition lives in this file rather than being delegated to a reusable workflow on the GH-Actions branch. Key points: - Deps are built with 'make build_deps' (optimized, with debug symbols) rather than build_deps_debug: third-party code is not what we want to instrument, and a non-debug deps build is significantly faster. - lib + src are built with 'make debug' so ASAN / gcov instrument ProxySQL's own code only. - Only test/tap/tests/unit/ is built via 'make unit_tests'; the integration TAP tests under test/tap/tests/ are intentionally skipped because they require running backends. - ASAN_OPTIONS starts with detect_leaks=0 because the existing unit tests were not written leak-free. Flipping this to =1 once those leaks are cleaned up will immediately turn this workflow into a leak regression guard. - vm.mmap_rnd_bits is lowered to 28 at the start of the job to satisfy ASAN's shadow-memory requirement on modern kernels (include/makefiles_vars.mk explicitly warns about this). - Trigger uses the same workflow_run: [CI-trigger] pattern as the rest of the CI-* files in this directory, plus workflow_dispatch for manual runs. Artifacts on every run (pass or fail): - unit-tests-coverage-lcov-<sha>: the merged lcov .info file - unit-tests-coverage-html-<sha>: genhtml output directory - unit-tests-logs-<sha>: per-test stdout/stderr captures Security: every 'run:' step only consumes values from env: (SHA, ASAN_OPTIONS, etc.) or GitHub-provided variables like \$GITHUB_WORKSPACE. No untrusted user input (PR titles, issue bodies, commit messages, fork branch names, etc.) is interpolated into shell commands, so there is no command-injection surface.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/CI-unit-tests-asan-coverage.yml (1)
98-101:fastcovis installed but never used.The coverage capture step (lines 206-243) exclusively uses
lcovandgenhtml. Unless there's a planned follow-up to switch to fastcov for faster coverage collection, this installation can be removed to save CI time.♻️ Suggested simplification
- name: Install coverage tooling run: | sudo apt-get -y install lcov - sudo pip3 install --break-system-packages fastcov || sudo pip3 install fastcov🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/CI-unit-tests-asan-coverage.yml around lines 98 - 101, Remove the unnecessary installation of fastcov from the "Install coverage tooling" step: keep the apt-get install lcov invocation but delete the sudo pip3 install --break-system-packages fastcov || sudo pip3 install fastcov line since the later coverage steps (lcov/genhtml) under the workflow use lcov exclusively and fastcov is never referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/CI-unit-tests-asan-coverage.yml:
- Around line 98-101: Remove the unnecessary installation of fastcov from the
"Install coverage tooling" step: keep the apt-get install lcov invocation but
delete the sudo pip3 install --break-system-packages fastcov || sudo pip3
install fastcov line since the later coverage steps (lcov/genhtml) under the
workflow use lcov exclusively and fastcov is never referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5af13583-96e9-4580-81c7-eaef9167489c
📒 Files selected for processing (1)
.github/workflows/CI-unit-tests-asan-coverage.yml
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/**/*(test_*.cpp|*-t.cpp) : Test files follow the naming pattern `test_*.cpp` or `*-t.cpp`
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/**/*.cpp : Use TAP (Test Anything Protocol) for tests with Docker-based backend infrastructure
Applied to files:
.github/workflows/CI-unit-tests-asan-coverage.yml
📚 Learning: 2026-04-11T05:43:20.598Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-11T05:43:20.598Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in `test/tap/tests/unit/` must link against `libproxysql.a` and use `test_globals.h` and `test_init.h`
Applied to files:
.github/workflows/CI-unit-tests-asan-coverage.yml
🔇 Additional comments (5)
.github/workflows/CI-unit-tests-asan-coverage.yml (5)
1-43: Well-documented workflow header with appropriate security considerations.The ASAN_OPTIONS configuration is sensible—
detect_leaks=0with clear documentation that it can be flipped once known leaks are addressed is a pragmatic approach. The security note accurately reflects that no untrusted input flows into shell commands.
45-78: LGTM!The job condition correctly gates on CI-trigger success or manual dispatch. The ASLR relaxation (
vm.mmap_rnd_bits=28) is necessary for ASAN compatibility on modern kernels and matches the project's documented requirements.
111-145: LGTM!The build configuration is correct. Setting WITHASAN/WITHGCOV for deps (even though the comment notes instrumentation is primarily for lib/src) is harmless—deps Makefiles may or may not propagate these flags, but the important point is that lib/src will be properly instrumented. The deliberate use of
make unit_testsinstead ofbuild_tap_test_debugto avoid building integration tests is the right call.
157-205: Well-structured test runner.The shell scripting is robust:
set +eensures all tests run regardless of individual failures,shopt -s nullglobhandles empty directories gracefully, and proper quoting is used throughout. TheGITHUB_STEP_SUMMARYoutput provides good visibility into results.
206-277: LGTM!Coverage handling is robust with appropriate error suppression (
|| true) to prevent CI failures from non-critical lcov issues. The path exclusions (/usr/*,*/deps/*,*/test/*) correctly focus coverage on ProxySQL's own code. Artifact uploads are properly conditional withif: always().
…r, add braces, fix enum types, fix narrowing conversions
…lptr, convert loops to range-based
…loops, NOLINT for parallel-array loops
- fix uninitialized loop var (UB) in test_digest_umap_aux-t.cpp - fix dead stores and unused vars in test_binlog_reader-t.cpp - rename reserved identifiers (__timer, _mysql_hdr, th_args__in_t, etc.) - fix narrowing conversions (ssize_t->int, uint32_t->int, size_t->int) - fix implicit widening of multiplication (overflow risk) - consolidate duplicated conditional branch in reg_test_4556 - pass modname by const ref in test_cluster1-t.cpp - fix narrowing in test_default_conn_collation-t.cpp
Batch 1 - dead stores/unused vars (11 files): - remove dead test_count assignments in genai_module/async/embedding_rerank/mcp_module - remove unused result strings in ai_llm_retry_scenarios - remove dead jitter_ms in ai_llm_retry_scenarios - remove unused row assignments in mysql-fast_forward/mysql-mirror1 - remove dead rc, sock, num_rows in mysql-protocol_compression/malformed/ssl_CA Batch 2 - narrowing conversions (~100 fixes): - pgsql-extended_query_protocol_test: uint8_t->char, ssize_t->int casts - genai_module/async/embedding_rerank: recv()/mysql_num_rows() casts - mysql-protocol_compression_level: various narrowing casts - max_connections_ff: timeout/duration casts - mysql_query_logging_memory: errno/plan() casts - pgsql-connection_parameters: htonl/ntohl casts - multiple_prepared_statements, mysql-reg_test_4716, nl2sql_internal, etc. Batch 3 - macros + branch clone: - fix macro parens in mysql-reg_test_4867_query_rules - fix macro parens in proxysql_debug.h - fix branch clone in pgsql-connection_parameters_test - fix branch clone in pgsql-copy_to_test - fix narrowing in pgsql-copy_from_test Batch 4 - value params + widening + reserved IDs: - pass string params by const ref in admin_various_commands2, mysql_hostgroup_attributes-servers_defaults, mysql-protocol_compression_level - fix implicit widening overflow in mcp_mysql_concurrency_stress - fix widening in mysql_stmt_send_long_data_large - rename reserved identifiers (__1, _PtrSize_t) - fix narrowing in pg_lite_client.h
… anomaly_detector_unit
…XX flags This flag is only valid for C/ObjC compilers, not C++. It was silently ignored by the C++ compiler, producing a warning on every cmake build invocation.
…Lite3_Server.cpp Fixes undefined reference to SQLite3_Server_session_handler in: - anomaly_detection-t - galera_1_timeout_count - galera_2_timeout_no_count The tap/SQLite3_Server.cpp defines its own stub using PtrSize_t*, while src/SQLite3_Server.cpp uses _PtrSize_t*. Using the src version ensures type consistency with the actual implementation.
…ld every run The preclean-libtap-archives PHONY target deleted libtap_*.a on every invocation, causing all TAP tests to relink from scratch. Replace with an order-only prerequisite on Makefile so archives only rebuild when the Makefile itself changes.
- Replace diag("") with diag("%s", "") to avoid zero-length format warnings
- Increase buffer and use unsigned format in unique_ip() to fix sprintf overflow
- Use printf-style format strings instead of .c_str() for ok() calls
- Fix %zu for size_t loop variable in fast_forward_switch_replication_deprecate_eof
- Use explicit function pointer type instead of decltype(&pclose)
- Replace string_format with plain assignment when no format args needed
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test/tap/tests/test_digest_umap_aux-t.cpp (1)
148-154:⚠️ Potential issue | 🟠 MajorSynchronize startup before measuring the admin loop.
Now that this loop deterministically runs three probes, it can finish before
run_dummy_queries()records any sample. In that schedule,run_dummy_queries()reachesstd::max_element()with an emptyexecution_timesvector and the test becomes flaky/undefined. Please add a start barrier, or at least fail cleanly when no timings were collected.Minimal guard to avoid UB when no timings are collected
@@ mysql_close(proxy_mysql); + if (execution_times.empty()) { + slowest_query = -1.0; + return; + } slowest_query = *std::max_element(execution_times.begin(), execution_times.end());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/test_digest_umap_aux-t.cpp` around lines 148 - 154, The test can call run_stats_digest_query() before run_dummy_queries() has any samples, causing execution_times to be empty and std::max_element() UB; fix by synchronizing startup or guarding the empty case: add a start barrier or handshake between run_stats_digest_query and run_dummy_queries (e.g., notify when the first dummy query is recorded) or, at minimum, check execution_times.empty() in run_dummy_queries and fail the test cleanly (assert or return an explicit error) before calling std::max_element; reference the functions run_stats_digest_query, run_dummy_queries and the variables execution_times and num_queries when implementing the guard.test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp (1)
304-326:⚠️ Potential issue | 🔴 CriticalFix byte tracking and buffer arithmetic in
read_srv_handshake().Lines 313 and 326 use
sizeof(buf_pos)(a pointer, 8 bytes) instead of remaining buffer space, and the loop conditions check only the lastread()result instead of total bytes received. This causes severe buffer underflow in read operations and incorrect packet parsing for fragmented handshakes. Additionally, the second loop should terminate when the full packet (header + payload) is received, not when the payload alone is complete.Proposed fix
int read_srv_handshake(int fd) { - char* buf_pos = net_buf; - - int r = static_cast<int>(read(fd, buf_pos, sizeof(net_buf))); - if (r == -1) { - perror("'read' failed"); - return r; - } - - buf_pos += r; - - while (r > 0 && r < NET_HEADER_SIZE) { - r = static_cast<int>(read(fd, buf_pos + r, sizeof(buf_pos))); - buf_pos += r; - - if (r == -1) { - perror("'read' failed"); - return r; - } - } + size_t total = 0; + + ssize_t chunk = read(fd, net_buf, sizeof(net_buf)); + if (chunk < 0) { + perror("'read' failed"); + return -1; + } + if (chunk == 0) { + diag("Socket closed before receiving handshake header"); + return -1; + } + total += static_cast<size_t>(chunk); + + while (total < NET_HEADER_SIZE) { + chunk = read(fd, net_buf + total, sizeof(net_buf) - total); + if (chunk < 0) { + perror("'read' failed"); + return -1; + } + if (chunk == 0) { + diag("Socket closed before receiving full handshake header"); + return -1; + } + total += static_cast<size_t>(chunk); + } mysql_hdr myhdr; memcpy(&myhdr, net_buf, sizeof(mysql_hdr)); + const size_t packet_size = NET_HEADER_SIZE + static_cast<size_t>(myhdr.pkt_length); + if (packet_size > sizeof(net_buf)) { + diag("Unexpected handshake packet size packet_size=%zu", packet_size); + return -1; + } - while (r > 0 && r < myhdr.pkt_length) { - r = static_cast<int>(read(fd, buf_pos + r, sizeof(buf_pos))); - buf_pos += r; - - if (r == -1) { - perror("'read' failed"); - return r; - } - } + while (total < packet_size) { + chunk = read(fd, net_buf + total, sizeof(net_buf) - total); + if (chunk < 0) { + perror("'read' failed"); + return -1; + } + if (chunk == 0) { + diag("Socket closed before receiving full handshake packet"); + return -1; + } + total += static_cast<size_t>(chunk); + } return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp` around lines 304 - 326, The read_srv_handshake() routine incorrectly uses sizeof(buf_pos) (a pointer) and checks only the last read() return value; fix it by introducing a bytes_received variable that tracks total bytes written into net_buf, replace all sizeof(buf_pos) with the actual remaining space calculation (sizeof(net_buf) - bytes_received) when calling read(), change the first loop condition to while (bytes_received > 0 && bytes_received < NET_HEADER_SIZE) and after copying mysql_hdr change the second loop condition to while (bytes_received > 0 && bytes_received < NET_HEADER_SIZE + myhdr.pkt_length) so you read only the remaining payload bytes (use remaining = NET_HEADER_SIZE + myhdr.pkt_length - bytes_received for read size) and update bytes_received += r after each successful read; ensure error handling still returns on r == -1.test/tap/tests/pgsql-connection_parameters_test-t.cpp (2)
543-554:⚠️ Potential issue | 🟠 MajorValidate
field_lenbefore slicing the row payload.
field_lenis trusted immediately afterntohl(). If a malformed or truncatedDataRowadvertises more bytes than remain inpayload,row.emplace_back(data + pos, field_len)reads past the buffer instead of producing a clean test failure.🛡️ Minimal guard for malformed
DataRowframesfor (int i = 0; i < num_fields; i++) { int32_t field_len; + if (pos + sizeof(field_len) > payload.size()) { + result->error = "Malformed DataRow payload"; + return result; + } memcpy(&field_len, data + pos, 4); pos += 4; field_len = static_cast<int32_t>(ntohl(field_len)); + if (field_len < -1 || + (field_len >= 0 && pos + static_cast<size_t>(field_len) > payload.size())) { + result->error = "Malformed DataRow payload"; + return result; + } + if (field_len == -1) { row.push_back("NULL"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-connection_parameters_test-t.cpp` around lines 543 - 554, The loop reads a declared field_len and immediately slices data with row.emplace_back(data + pos, field_len); add a guard after field_len = ntohl(field_len) to validate that field_len >= 0 and that pos + field_len does not exceed the remaining payload length (the total buffer length for data/payload used in this test). If the check fails, return a clear test failure/assertion (or push a sentinel value) instead of doing the out-of-bounds slice; update the code around the variables field_len, pos, data and the row.emplace_back(...) call to perform this bounds check before advancing pos or constructing the string.
408-458:⚠️ Potential issue | 🟠 MajorFix bounds check and handle empty message buffer on EAGAIN.
The PostgreSQL protocol message length (4 bytes) excludes the type byte, so the total frame is
1 + messageLengthbytes. The bounds checkoffset + messageLength > bytes_receivedshould beoffset + messageLength + 1 > bytes_received. Additionally, if the first nonblockingrecv(MSG_DONTWAIT)returnsEAGAIN, the loop breaks withmsg_listempty, causing undefined behavior onmsg_list.back(). Either accumulate bytes across multiplerecv()calls until a full frame arrives, or check!msg_list.empty()before accessing the vector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-connection_parameters_test-t.cpp` around lines 408 - 458, The frame bounds check and empty-buffer handling are wrong: change the bounds check that currently tests offset + messageLength > bytes_received to account for the 1-byte type prefix (use offset + messageLength + 1 > bytes_received) when validating a full PostgreSQL frame in the recv parsing loop, and guard against reading msg_list.back() after an EAGAIN that yielded no messages — either accumulate bytes across successive recv() calls until at least one full PgSQLResponse is parsed into msg_list, or check !msg_list.empty() before using msg_list.back() (and return/continue with an appropriate error) so parse_error(payload) and the subsequent type checks never read from an empty vector.test/tap/tests/pgsql-copy_to_test-t.cpp (1)
48-67:⚠️ Potential issue | 🟠 MajorFix C++17 compliance and single-token query truncation in
fnResultType.The variable-length array
char buf[qtlen]is not standard C++17; it's a GNU extension. Additionally, the logicmemcpy(buf, query, qtlen - 1)truncates the final character for single-token queries—e.g.,"SELECT"becomes"SELEC". Use a standardstd::stringinstead.♻️ Suggested rewrite
auto fnResultType = [](const char* query) -> int { const char* fs = strchr(query, ' '); - size_t qtlen = strlen(query); - if (fs != NULL) { - qtlen = (fs - query) + 1; - } - char buf[qtlen]; - memcpy(buf, query, qtlen - 1); - buf[qtlen - 1] = 0; + const size_t token_len = fs ? static_cast<size_t>(fs - query) : strlen(query); + const std::string buf(query, token_len); - if (strncasecmp(buf, "SELECT", sizeof("SELECT") - 1) == 0) { + if (strncasecmp(buf.c_str(), "SELECT", sizeof("SELECT") - 1) == 0) { return PGRES_TUPLES_OK; } - if (strncasecmp(buf, "COPY", sizeof("COPY") - 1) == 0) { + if (strncasecmp(buf.c_str(), "COPY", sizeof("COPY") - 1) == 0) { return PGRES_COPY_OUT; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-copy_to_test-t.cpp` around lines 48 - 67, fnResultType currently uses a non-standard VLA (char buf[qtlen]) and copies qtlen-1 bytes which drops the last character for single-token queries; replace that logic with a std::string: compute token length as (fs? fs - query : strlen(query)), construct std::string token(query, token_len), and pass token.c_str() (and the usual comparison length) to strncasecmp to check "SELECT" and "COPY"; update references in executeQueries/fnResultType to use this std::string-based approach so the code is C++17-compliant and preserves the full token.
♻️ Duplicate comments (1)
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp (1)
211-215:⚠️ Potential issue | 🟡 MinorCheck the temp-cert write before using it.
If
fprintf,fflush, orfclosefails here, the helper can silently stop exercising the invalid-cert path and drift into a different failure mode. Fail fast and clean up the temp path on any write/flush/close error.Proposed fix
- fprintf(tmp_file, "%s", inv_cert); - fflush(tmp_file); - fclose(tmp_file); + int write_rc = fprintf(tmp_file, "%s", inv_cert); + int flush_rc = fflush(tmp_file); + int close_rc = fclose(tmp_file); + if (write_rc < 0 || flush_rc != 0 || close_rc != 0) { + fprintf(stderr, "File %s, line %d, Error: writing '%s' failed: %s\n", + __FILE__, __LINE__, inv_cert_path, strerror(errno)); + mysql_close(myconn); + unlink(inv_cert_path); + free(inv_cert_path); + return NULL; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp` around lines 211 - 215, The temp-cert write currently ignores errors from fprintf/fflush/fclose (writing inv_cert to tmp_file); update the block that calls fprintf(tmp_file, "%s", inv_cert), fflush(tmp_file) and fclose(tmp_file) to check each call's return value, and on any failure remove the temp file (cleanup the temp path) and fail fast (return an error or abort the helper path) so the test cannot silently continue on a write/close error; reference the tmp_file, inv_cert and the fprintf/fflush/fclose calls when locating the change.
🧹 Nitpick comments (5)
test/tap/tests/test_binlog_reader-t.cpp (1)
70-70: Remove dead random variable initialization.
rnd_ais still computed and never used in both functions;[[maybe_unused]]only silences the warning and keeps dead code.Proposed cleanup
int insert_random_data(MYSQL* proxysql_mysql, uint32_t rows) { - [[maybe_unused]] int rnd_a = rand() % 1000; string rnd_c = random_string(rand() % 100 + 5); string rnd_pad = random_string(rand() % 50 + 5); @@ int perform_update(MYSQL* proxysql_mysql, uint32_t rows) { - [[maybe_unused]] int rnd_a = rand() % 1000; - string query { "UPDATE test.gtid_test SET a=a+1, c=REVERSE(c)" }; MYSQL_QUERY(proxysql_mysql, query.c_str());Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/test_binlog_reader-t.cpp` at line 70, The variable rnd_a is computed but never used (declared as [[maybe_unused]] int rnd_a = rand() % 1000;) in two places; remove these dead variable initializations to clean up unused code—locate the rnd_a declarations in the two test functions around the occurrences shown (lines near 70 and 87) and delete those declarations so the tests no longer compute an unused random value.test/tap/tests/mysql-test_malformed_packet-t.cpp (1)
86-87: Remove the unnecessarysock_unusedassignment afterclose(sock).Line 87 creates an unused variable that reads from the closed descriptor, adding noise without changing behavior. After the
close(sock)call, no further references tosockoccur in the function, and all error-handling paths (which useREPORT_ERROR_AND_EXITand accesssock) occur earlier in the function.Suggested cleanup
close(sock); - [[maybe_unused]] int sock_unused = sock;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-test_malformed_packet-t.cpp` around lines 86 - 87, Remove the needless [[maybe_unused]] int sock_unused = sock; declaration that comes immediately after close(sock); simply delete the sock_unused variable (it reads the closed descriptor and is never used); leave the existing close(sock) call and keep all earlier error paths that reference REPORT_ERROR_AND_EXIT unchanged.test/tap/tests/genai_async-t.cpp (1)
852-852: Keeptest_countaccumulation consistent in Part 11.Line 852 stops accumulating return values while other parts still do, which makes the local bookkeeping pattern inconsistent.
♻️ Optional consistency tweak
- test_concurrent_connections(cl.host, cl.username, cl.password, cl.port); + test_count += test_concurrent_connections(cl.host, cl.username, cl.password, cl.port);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/genai_async-t.cpp` at line 852, Part 11 stops adding the return from test_concurrent_connections to the accumulating test_count, breaking the local bookkeeping pattern; change the call to add its return to test_count (e.g., replace the standalone call to test_concurrent_connections(cl.host, cl.username, cl.password, cl.port) with an additive update to test_count) so it matches the accumulation used in other parts and keep the bookkeeping consistent.test/tap/tests/genai_embedding_rerank-t.cpp (1)
713-713: Use a single counting convention inmain().Line 713 ignores the helper return value while previous parts still accumulate into
test_count, which makes the flow harder to follow.♻️ Optional consistency tweak
- test_genai_syntax(client); + test_count += test_genai_syntax(client);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/genai_embedding_rerank-t.cpp` at line 713, main currently mixes two conventions: some helpers return an int added into test_count while the call to test_genai_syntax(client) on line 713 ignores its return value; update the call to follow the single counting convention by using the helper's return and adding it to test_count (e.g., replace the bare call to test_genai_syntax(client) with an expression that accumulates its int return into test_count) so main consistently uses test_count for aggregate results.test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp (1)
150-172: Replace manual cleanup with RAII foradminconnection.The current approach with
backup_createdflag andrun_cleanup_and_exitlambda violates RAII principles and is fragile. Usestd::unique_ptr<MYSQL, decltype(&mysql_close)>for the admin connection, following the pattern already established intest_cluster1-t.cppand other test files in the codebase. This automatically closes the connection on all exit paths—including early returns—without requiring manual bookkeeping.For the conditional backup restoration logic, consider refactoring it into a separate RAII guard or cleanup routine that can be invoked selectively rather than relying on the lambda's conditional check of the
backup_createdflag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp` around lines 150 - 172, Replace the manual admin cleanup and backup flag with RAII: wrap the MYSQL* admin in a std::unique_ptr<MYSQL, decltype(&mysql_close)> (use the same pattern as test_cluster1-t.cpp) so mysql_close is called automatically, remove the run_cleanup_and_exit lambda and the backup_created boolean, and move the conditional backup-restore SQL into a small RAII guard or a separate cleanup function that runs only when a backup exists; keep references to the original symbols admin, run_cleanup_and_exit, backup_created, mysql_close and the SQL statements so you can replace their usages with the unique_ptr and the new guard/function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tap/Makefile`:
- Around line 145-147: The rule for rebuilding archives uses an order-only
prerequisite ("$(LIBTAP_ARCHIVES): | Makefile") which prevents a rebuild when
Makefile's timestamp changes; update the target declaration for LIBTAP_ARCHIVES
to use a normal prerequisite ("$(LIBTAP_ARCHIVES): Makefile") so changes to the
Makefile will trigger rebuilding the libtap_mariadb.a, libtap_mysql57.a, and
libtap_mysql8.a archives.
In `@test/tap/tests/genai_module-t.cpp`:
- Around line 138-141: The test calls mysql_num_rows(res) without checking that
mysql_store_result() returned a non-NULL MYSQL_RES* (res), which can segfault;
update each affected block that uses res (e.g., the sequence where res is
obtained via mysql_store_result() and then mysql_num_rows(res) is called) to
first check if (res != NULL) before calling mysql_num_rows() and only call
mysql_free_result(res) when res is non-NULL, otherwise handle the NULL case
(fail the test or log the error) so every use of MYSQL_RES* from
mysql_store_result() is guarded.
In `@test/tap/tests/kill_connection2-t.cpp`:
- Around line 201-205: The test currently asserts failure too early by using
ok(rc != 0, ...) in the first "DO 1" loop; change the assertion to expect
success there (use ok(rc == 0, ...) or equivalent) and update the diagnostic
string to reflect success vs failure for the run_q call that uses conns / MYSQL
* mysql / run_q; ensure only the later timeout phase asserts failure for the
connection.
In `@test/tap/tests/max_connections_ff-t.cpp`:
- Around line 411-417: The timeout calculations use narrowing casts to int that
overflow (e.g., duration_s computed from duration.count() and
srv_conn_to_s/poll_to_s from connect_timeout/poll_timeout); update the math to
use a wider type or floating point instead of casting to int — e.g., treat
duration.count() as an int64_t or double and divide by 1e9, and convert
connect_timeout/poll_timeout to double (or divide by 1000.0) when computing
srv_conn_to_s and poll_to_s so values cannot wrap; change the expressions that
compute duration_s, srv_conn_to_s and poll_to_s accordingly.
In `@test/tap/tests/mcp_module-t.cpp`:
- Around line 159-160: After each call that assigns MYSQL_RES* res =
mysql_store_result(admin) (and similar temporary result variables), check if res
== nullptr before calling mysql_num_rows(res); if res is nullptr handle the
error path (e.g., log/return/fail the test) and only call mysql_num_rows(res)
when res is non-null, then always mysql_free_result(res) after use; apply this
null-check + free pattern to every occurrence using mysql_store_result/admin and
mysql_num_rows in this file (matching the existing pattern used near lines
~103–105 and in utils.cpp).
In `@test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp`:
- Around line 159-171: run_cleanup_and_exit currently only restores
disk.mysql_hostgroup_attributes; update it so when backup_created is true it
also restores the live table and reloads runtime: execute "DELETE FROM
mysql_hostgroup_attributes" and then "INSERT INTO mysql_hostgroup_attributes
SELECT * FROM mysql_hostgroup_attributes_0508" using the same
mysql_query_t/admin checks, ensure each failing mysql_query_t logs the error
with fprintf (include mysql_error(admin)) and causes the test to fail (return
non-zero or exit with failure) instead of continuing, and after successful
repopulation invoke the runtime reload command used elsewhere in the test suite
to ensure the admin/runtime reflects the restored state before calling
mysql_close(admin) and returning exit_status().
In `@test/tap/tests/mysql-protocol_compression_level-t.cpp`:
- Line 55: The division by its when computing delay_us is unsafe if its can be
zero; before computing uint64_t delay_us = static_cast<uint64_t>(std::pow(10, 6)
/ its) check that its != 0 and handle the zero case (e.g., set delay_us = 0,
skip the loop, or return/throw an error) so the calculation in this file using
the variable its does not perform a divide-by-zero; update the code around the
delay_us computation to perform this guard and use delay_us only after the
check.
In `@test/tap/tests/mysql-reg_test_4867_query_rules-t.cpp`:
- Line 395: The code narrows the atoll(row[0]) result into rule_ids[i] via
static_cast<int> without a range check; update the conversion around rule_ids[i]
so you parse the value (e.g. using atoll/strtoll) into a long long, verify it is
within INT_MIN..INT_MAX and non-negative (since IDs are AUTOINCREMENT), and only
then assign static_cast<int>(value); if out of range or parsing fails, handle it
defensively (log/error/abort or clamp) so rule_ids[i] cannot receive a
truncated/invalid int.
In `@test/tap/tests/nl2sql_internal-t.cpp`:
- Line 66: The code passes a plain char to toupper/tolower which is UB for
negative values; update the two occurrences (the assignment using toupper on
upper_sql[i] and the similar tolower use at the other location) to call
toupper/tolower with static_cast<unsigned char>(upper_sql[i]) (or the
corresponding buffer element) and then static_cast<char> the result for the
assignment so the call is made with unsigned char and the stored value remains
char.
In `@test/tap/tests/pgsql-copy_from_test-t.cpp`:
- Line 190: The NUMERIC sign encoding double-converts the 16-bit sign by calling
htons(sign) and then write_int16 (which already emits network-order bytes);
remove the htons call and pass the raw int16_t sign to write_int16 (e.g.,
write_int16(out, static_cast<int16_t>(sign))) so the sign is only converted once
by write_int16 and negative NUMERIC values are encoded correctly.
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp`:
- Around line 463-468: The diagnostic log in the mysql_real_connect failure path
currently includes the plaintext frontend password (cl.password); update the
diag call in the block that checks mysql_real_connect (the same block that calls
cleanup_and_fail on error) to stop logging cl.password — either remove that
parameter from the format string or replace it with a redaction token like
"[REDACTED]" so the message retains context (host, port, user, error via
mysql_error(myconn)) but never prints cl.password.
In `@test/tap/tests/test_binlog_reader-t.cpp`:
- Around line 165-167: The random row selection uses "int r_row = rand() %
static_cast<int>(NUM_ROWS); if (r_row == 0) { r_row = 1; }" which never yields
id == NUM_ROWS and overweights id==1; replace with a uniform 1..NUM_ROWS
selection (e.g. compute r_row = (rand() % static_cast<int>(NUM_ROWS)) + 1) in
the test code (the same change should be applied to the other occurrence
referenced at lines ~457-458) so both SELECT paths get unbiased coverage; update
the r_row assignment in test_binlog_reader-t.cpp accordingly.
In `@test/tap/tests/test_cluster1-t.cpp`:
- Around line 234-236: The calculation assumes at least 4 nodes and does
unchecked subtraction which underflows when total_nodes < 4; update the code
around total_nodes and np (the variables computed from
cluster_nodes/cluster_ports and np) to first clamp total_nodes to a minimum of 4
(e.g., compute min_nodes = std::max<size_t>(total_nodes, 4)) and then use
min_nodes in the np formula so the term (total_nodes-4) cannot underflow; also
ensure any later code that indexes conns[0..3] (the core-node loops) uses the
clamped value or otherwise guards accesses when total_nodes < 4.
---
Outside diff comments:
In `@test/tap/tests/pgsql-connection_parameters_test-t.cpp`:
- Around line 543-554: The loop reads a declared field_len and immediately
slices data with row.emplace_back(data + pos, field_len); add a guard after
field_len = ntohl(field_len) to validate that field_len >= 0 and that pos +
field_len does not exceed the remaining payload length (the total buffer length
for data/payload used in this test). If the check fails, return a clear test
failure/assertion (or push a sentinel value) instead of doing the out-of-bounds
slice; update the code around the variables field_len, pos, data and the
row.emplace_back(...) call to perform this bounds check before advancing pos or
constructing the string.
- Around line 408-458: The frame bounds check and empty-buffer handling are
wrong: change the bounds check that currently tests offset + messageLength >
bytes_received to account for the 1-byte type prefix (use offset + messageLength
+ 1 > bytes_received) when validating a full PostgreSQL frame in the recv
parsing loop, and guard against reading msg_list.back() after an EAGAIN that
yielded no messages — either accumulate bytes across successive recv() calls
until at least one full PgSQLResponse is parsed into msg_list, or check
!msg_list.empty() before using msg_list.back() (and return/continue with an
appropriate error) so parse_error(payload) and the subsequent type checks never
read from an empty vector.
In `@test/tap/tests/pgsql-copy_to_test-t.cpp`:
- Around line 48-67: fnResultType currently uses a non-standard VLA (char
buf[qtlen]) and copies qtlen-1 bytes which drops the last character for
single-token queries; replace that logic with a std::string: compute token
length as (fs? fs - query : strlen(query)), construct std::string token(query,
token_len), and pass token.c_str() (and the usual comparison length) to
strncasecmp to check "SELECT" and "COPY"; update references in
executeQueries/fnResultType to use this std::string-based approach so the code
is C++17-compliant and preserves the full token.
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp`:
- Around line 304-326: The read_srv_handshake() routine incorrectly uses
sizeof(buf_pos) (a pointer) and checks only the last read() return value; fix it
by introducing a bytes_received variable that tracks total bytes written into
net_buf, replace all sizeof(buf_pos) with the actual remaining space calculation
(sizeof(net_buf) - bytes_received) when calling read(), change the first loop
condition to while (bytes_received > 0 && bytes_received < NET_HEADER_SIZE) and
after copying mysql_hdr change the second loop condition to while
(bytes_received > 0 && bytes_received < NET_HEADER_SIZE + myhdr.pkt_length) so
you read only the remaining payload bytes (use remaining = NET_HEADER_SIZE +
myhdr.pkt_length - bytes_received for read size) and update bytes_received += r
after each successful read; ensure error handling still returns on r == -1.
In `@test/tap/tests/test_digest_umap_aux-t.cpp`:
- Around line 148-154: The test can call run_stats_digest_query() before
run_dummy_queries() has any samples, causing execution_times to be empty and
std::max_element() UB; fix by synchronizing startup or guarding the empty case:
add a start barrier or handshake between run_stats_digest_query and
run_dummy_queries (e.g., notify when the first dummy query is recorded) or, at
minimum, check execution_times.empty() in run_dummy_queries and fail the test
cleanly (assert or return an explicit error) before calling std::max_element;
reference the functions run_stats_digest_query, run_dummy_queries and the
variables execution_times and num_queries when implementing the guard.
---
Duplicate comments:
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp`:
- Around line 211-215: The temp-cert write currently ignores errors from
fprintf/fflush/fclose (writing inv_cert to tmp_file); update the block that
calls fprintf(tmp_file, "%s", inv_cert), fflush(tmp_file) and fclose(tmp_file)
to check each call's return value, and on any failure remove the temp file
(cleanup the temp path) and fail fast (return an error or abort the helper path)
so the test cannot silently continue on a write/close error; reference the
tmp_file, inv_cert and the fprintf/fflush/fclose calls when locating the change.
---
Nitpick comments:
In `@test/tap/tests/genai_async-t.cpp`:
- Line 852: Part 11 stops adding the return from test_concurrent_connections to
the accumulating test_count, breaking the local bookkeeping pattern; change the
call to add its return to test_count (e.g., replace the standalone call to
test_concurrent_connections(cl.host, cl.username, cl.password, cl.port) with an
additive update to test_count) so it matches the accumulation used in other
parts and keep the bookkeeping consistent.
In `@test/tap/tests/genai_embedding_rerank-t.cpp`:
- Line 713: main currently mixes two conventions: some helpers return an int
added into test_count while the call to test_genai_syntax(client) on line 713
ignores its return value; update the call to follow the single counting
convention by using the helper's return and adding it to test_count (e.g.,
replace the bare call to test_genai_syntax(client) with an expression that
accumulates its int return into test_count) so main consistently uses test_count
for aggregate results.
In `@test/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpp`:
- Around line 150-172: Replace the manual admin cleanup and backup flag with
RAII: wrap the MYSQL* admin in a std::unique_ptr<MYSQL, decltype(&mysql_close)>
(use the same pattern as test_cluster1-t.cpp) so mysql_close is called
automatically, remove the run_cleanup_and_exit lambda and the backup_created
boolean, and move the conditional backup-restore SQL into a small RAII guard or
a separate cleanup function that runs only when a backup exists; keep references
to the original symbols admin, run_cleanup_and_exit, backup_created, mysql_close
and the SQL statements so you can replace their usages with the unique_ptr and
the new guard/function.
In `@test/tap/tests/mysql-test_malformed_packet-t.cpp`:
- Around line 86-87: Remove the needless [[maybe_unused]] int sock_unused =
sock; declaration that comes immediately after close(sock); simply delete the
sock_unused variable (it reads the closed descriptor and is never used); leave
the existing close(sock) call and keep all earlier error paths that reference
REPORT_ERROR_AND_EXIT unchanged.
In `@test/tap/tests/test_binlog_reader-t.cpp`:
- Line 70: The variable rnd_a is computed but never used (declared as
[[maybe_unused]] int rnd_a = rand() % 1000;) in two places; remove these dead
variable initializations to clean up unused code—locate the rnd_a declarations
in the two test functions around the occurrences shown (lines near 70 and 87)
and delete those declarations so the tests no longer compute an unused random
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90e9b583-f9f6-411e-b5f2-d97cd05008b7
📒 Files selected for processing (51)
.gitignoreMakefileinclude/proxysql_debug.htest/deps/Makefiletest/tap/tap/Makefiletest/tap/tests/Makefiletest/tap/tests/admin-listen_on_unix-t.cpptest/tap/tests/admin_show_create_table-t.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/admin_various_commands-t.cpptest/tap/tests/admin_various_commands2-t.cpptest/tap/tests/admin_various_commands3-t.cpptest/tap/tests/ai_llm_retry_scenarios-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/anomaly_detector_unit-t.cpptest/tap/tests/firewall_commands1-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/kill_connection3-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/mcp_mysql_concurrency_stress-t.cpptest/tap/tests/multiple_prepared_statements-t.cpptest/tap/tests/mysql-fast_forward-t.cpptest/tap/tests/mysql-mirror1-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/mysql-show_ssl_version-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/mysql-test_ssl_CA-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/mysql_stmt_send_long_data_large-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/pg_lite_client.htest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/pgsql-copy_from_test-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/pgsql-extended_query_protocol_query_rules_test-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/reg_test_mariadb_metadata_check-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_cluster1-t.cpptest/tap/tests/test_default_conn_collation-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/test_warnings-t.cpp
💤 Files with no reviewable changes (1)
- test/tap/tests/mysql-test_ssl_CA-t.cpp
✅ Files skipped from review due to trivial changes (12)
- test/tap/tests/admin-listen_on_unix-t.cpp
- .gitignore
- test/tap/tests/mysql_stmt_send_long_data_large-t.cpp
- test/tap/tests/mysql-show_ssl_version-t.cpp
- test/tap/tests/mysql-mirror1-t.cpp
- test/tap/tests/pgsql-extended_query_protocol_query_rules_test-t.cpp
- test/tap/tests/anomaly_detector_unit-t.cpp
- test/tap/tests/pg_lite_client.h
- test/tap/tests/multiple_prepared_statements-t.cpp
- test/tap/tests/firewall_commands1-t.cpp
- test/tap/tests/mcp_mysql_concurrency_stress-t.cpp
- test/tap/tests/admin_various_commands2-t.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- test/tap/tests/reg_test_mariadb_metadata_check-t.cpp
- test/tap/tests/test_default_conn_collation-t.cpp
- test/tap/tests/kill_connection3-t.cpp
- test/tap/tests/mysql-fast_forward-t.cpp
- test/tap/tests/test_warnings-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/admin_various_commands3-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_create_table-t.cpptest/tap/tests/admin_various_commands-t.cpptest/tap/tests/ai_llm_retry_scenarios-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/pgsql-copy_from_test-t.cppinclude/proxysql_debug.htest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/admin_various_commands3-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_create_table-t.cpptest/tap/tests/admin_various_commands-t.cpptest/tap/tests/ai_llm_retry_scenarios-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/pgsql-copy_from_test-t.cpptest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards use the pattern
#ifndef __CLASS_*_H(e.g.,#ifndef __ADMIN_H__,#ifndef __PROXYSQL_PROTOCOL_H__)
Files:
include/proxysql_debug.h
🧠 Learnings (14)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Applied to files:
test/tap/tests/admin_various_commands3-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_create_table-t.cpptest/tap/tests/admin_various_commands-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tap/Makefiletest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/Makefile
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/admin_various_commands3-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_create_table-t.cpptest/tap/tests/admin_various_commands-t.cpptest/tap/tests/ai_llm_retry_scenarios-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/pgsql-copy_from_test-t.cpptest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/deps/Makefiletest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/Makefiletest/tap/tests/genai_async-t.cppinclude/proxysql_debug.htest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/deps/Makefiletest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-copy_to_test-t.cpptest/tap/tests/genai_module-t.cpptest/tap/tests/Makefiletest/tap/tests/genai_async-t.cppinclude/proxysql_debug.htest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/admin_show_create_table-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tap/Makefiletest/tap/tests/genai_module-t.cpptest/tap/tests/Makefiletest/tap/tests/mcp_module-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/admin_show_create_table-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/nl2sql_internal-t.cpptest/tap/tests/genai_embedding_rerank-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tap/Makefiletest/tap/tests/genai_module-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/Makefiletest/tap/tests/mcp_module-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/admin_show_create_table-t.cpptest/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/mysql-test_malformed_packet-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/mysql_hostgroup_attributes-servers_defaults-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
Applied to files:
test/tap/tests/admin_show_create_table-t.cpptest/tap/tests/test_digest_umap_aux-t.cpptest/tap/tests/kill_connection2-t.cpptest/tap/tests/test_binlog_reader-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/mysql_query_logging_memory-t.cpptest/tap/tests/mcp_module-t.cpptest/tap/tests/mysql-protocol_compression_level-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Class names use PascalCase with protocol prefixes: `MySQL_`, `PgSQL_`, or `ProxySQL_` (e.g., `MySQL_Protocol`, `PgSQL_Session`, `ProxySQL_Admin`)
Applied to files:
test/tap/tests/mysql-reg_test_4716_single_semicolon-t.cpptest/tap/tests/anomaly_detection-t.cpptest/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/test_cluster1-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: ASAN builds require `NOJEMALLOC=1 WITHASAN=1 make build_deps_debug && make debug && make build_tap_test_debug`
Applied to files:
test/tap/tests/Makefile
📚 Learning: 2026-02-13T09:29:39.713Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.
Applied to files:
test/tap/tests/mcp_module-t.cpp
📚 Learning: 2026-01-14T11:36:39.934Z
Learnt from: rahim-kanji
Repo: sysown/proxysql PR: 5299
File: lib/PgSQL_Session.cpp:5173-5233
Timestamp: 2026-01-14T11:36:39.934Z
Learning: The function `PgSQL_Session::send_parameter_error_response()` in `lib/PgSQL_Session.cpp` has a default parameter for its second argument (PGSQL_ERROR_CODES code) with the default value of PGSQL_ERROR_CODES::ERRCODE_INVALID_TEXT_REPRESENTATION, as declared in include/PgSQL_Session.h. Single-argument calls to this function are valid.
Applied to files:
test/tap/tests/pgsql-connection_parameters_test-t.cpptest/tap/tests/pgsql-extended_query_protocol_test-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
[error] 21-21: 'cerrno' file not found
(clang-diagnostic-error)
test/tap/tests/test_cluster1-t.cpp
[error] 1-1: 'memory' file not found
(clang-diagnostic-error)
Comment on lines
+145
to
+147
| # Rebuild archives when the Makefile itself changes (order-only). | ||
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | ||
|
|
||
| $(LIBTAP_ARCHIVES): preclean-libtap-archives | Makefile | ||
|
|
||
| .PHONY: preclean-libtap-archives | ||
| preclean-libtap-archives: | ||
| rm -f $(LIBTAP_ARCHIVES) | ||
| $(LIBTAP_ARCHIVES): | Makefile |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GNU Make, does target: | Makefileforcetargetto rebuild whenMakefile has a newer timestamp?
💡 Result:
No, in GNU Make, target: | Makefile declares Makefile as an order-only prerequisite. This ensures Makefile is built (if needed) before target, but does not force target to rebuild if Makefile has a newer timestamp than target. Order-only prerequisites impose ordering without triggering rebuilds based on timestamps, unlike normal prerequisites.
Citations:
- 1: https://www.gnu.org/s/make/manual/html_node/Prerequisite-Types.html
- 2: https://stackoverflow.com/questions/58039810/makefiles-what-is-an-order-only-prerequisite
- 3: https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
Change | Makefile to Makefile to trigger rebuilds when Makefile is modified.
The order-only prerequisite | Makefile at line 147 enforces build order only and does not trigger a rebuild when Makefile's timestamp changes. This contradicts the intent stated in the comment (line 145) and can leave stale archives in place.
Fix
-# Rebuild archives when the Makefile itself changes (order-only).
+# Rebuild archives when the Makefile itself changes.
LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a
-$(LIBTAP_ARCHIVES): | Makefile
+$(LIBTAP_ARCHIVES): Makefile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Rebuild archives when the Makefile itself changes (order-only). | |
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | |
| $(LIBTAP_ARCHIVES): preclean-libtap-archives | Makefile | |
| .PHONY: preclean-libtap-archives | |
| preclean-libtap-archives: | |
| rm -f $(LIBTAP_ARCHIVES) | |
| $(LIBTAP_ARCHIVES): | Makefile | |
| # Rebuild archives when the Makefile itself changes. | |
| LIBTAP_ARCHIVES := libtap_mariadb.a libtap_mysql57.a libtap_mysql8.a | |
| $(LIBTAP_ARCHIVES): Makefile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tap/Makefile` around lines 145 - 147, The rule for rebuilding
archives uses an order-only prerequisite ("$(LIBTAP_ARCHIVES): | Makefile")
which prevents a rebuild when Makefile's timestamp changes; update the target
declaration for LIBTAP_ARCHIVES to use a normal prerequisite
("$(LIBTAP_ARCHIVES): Makefile") so changes to the Makefile will trigger
rebuilding the libtap_mariadb.a, libtap_mysql57.a, and libtap_mysql8.a archives.
Comment on lines
+138
to
141
| int num_rows = static_cast<int>(mysql_num_rows(res)); | ||
| ok(num_rows == 2, | ||
| "SHOW VARIABLES LIKE 'genai-%%' returns 2 rows, got %d", num_rows); | ||
| mysql_free_result(res); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can mysql_store_result() return NULL after a successful mysql_query(), and what is the recommended guard pattern before mysql_num_rows()?
💡 Result:
Yes, mysql_store_result can return NULL after a successful mysql_query. This occurs in three cases: malloc failure (e.g., result set too large), error reading data from the connection, or the query returned no data (e.g., INSERT, UPDATE, DELETE). The recommended guard pattern before calling mysql_num_rows is to check if the MYSQL_RES* result from mysql_store_result is not NULL. mysql_num_rows requires a valid result pointer and is only for use after mysql_store_result succeeds in returning a non-NULL result. Example pattern from official docs: if (mysql_query(&mysql, query_string)) { // handle query error } else { MYSQL_RES *result = mysql_store_result(&mysql); if (result) { // there are rows (or empty result set) unsigned int num_rows = mysql_num_rows(result); // process rows mysql_free_result(result); } else { // NULL result: check reason if (mysql_field_count(&mysql) == 0) { // no result set expected (e.g., INSERT) unsigned int num_rows = mysql_affected_rows(&mysql); } else { // error occurred fprintf(stderr, "Error: %s\n", mysql_error(&mysql)); } } }
Citations:
- 1: https://dev.mysql.com/doc/c-api/8.3/en/null-mysql-store-result.html
- 2: https://dev.mysql.com/doc/c-api/8.4/en/null-mysql-store-result.html
- 3: https://docs.oracle.com/cd/E17952_01/c-api-9.6-en/mysql-store-result.html
- 4: https://dev.mysql.com/doc/c-api/8.0/en/mysql-store-result.html
- 5: https://dev.mysql.com/doc/c-api/8.0/en/mysql-field-count.html
🏁 Script executed:
# First, check if the file exists and get line count
wc -l test/tap/tests/genai_module-t.cppRepository: sysown/proxysql
Length of output: 96
🏁 Script executed:
# Examine the specific lines mentioned (138, 223, 235, 247, 259)
sed -n '135,145p' test/tap/tests/genai_module-t.cpp
sed -n '220,230p' test/tap/tests/genai_module-t.cpp
sed -n '232,242p' test/tap/tests/genai_module-t.cpp
sed -n '244,254p' test/tap/tests/genai_module-t.cpp
sed -n '256,266p' test/tap/tests/genai_module-t.cppRepository: sysown/proxysql
Length of output: 1995
🏁 Script executed:
# Search for mysql_store_result patterns in test files to understand project conventions
rg 'mysql_store_result' test/tap/tests/ -A 3 -B 1 | head -100Repository: sysown/proxysql
Length of output: 7068
Guard mysql_store_result() before calling mysql_num_rows().
Line 138 is completely unguarded—if mysql_store_result() returns NULL, mysql_num_rows(res) will segfault. Lines 223, 235, 247, 259 are guarded by query return codes (if (rc1 == 0), etc.), but that validates only the query execution, not the result storage. According to official MySQL C API documentation, mysql_store_result() can legitimately return NULL after a successful mysql_query() and must be checked before calling mysql_num_rows().
Defensive pattern for all affected blocks
MYSQL_RES* res = mysql_store_result(admin);
- int num_rows = static_cast<int>(mysql_num_rows(res));
- ok(num_rows == 2, "...");
- mysql_free_result(res);
+ if (res) {
+ int num_rows = static_cast<int>(mysql_num_rows(res));
+ ok(num_rows == 2, "...");
+ mysql_free_result(res);
+ } else {
+ skip(1, "mysql_store_result() returned NULL");
+ }Also applies to: 223–225, 234–237, 246–249, 258–261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/genai_module-t.cpp` around lines 138 - 141, The test calls
mysql_num_rows(res) without checking that mysql_store_result() returned a
non-NULL MYSQL_RES* (res), which can segfault; update each affected block that
uses res (e.g., the sequence where res is obtained via mysql_store_result() and
then mysql_num_rows(res) is called) to first check if (res != NULL) before
calling mysql_num_rows() and only call mysql_free_result(res) when res is
non-NULL, otherwise handle the NULL case (fail the test or log the error) so
every use of MYSQL_RES* from mysql_store_result() is guarded.
Comment on lines
159
to
+160
| MYSQL_RES* res = mysql_store_result(admin); | ||
| int num_rows = mysql_num_rows(res); | ||
| int num_rows = static_cast<int>(mysql_num_rows(res)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect MYSQL_QUERY macro behavior (to confirm it doesn't guarantee non-null MYSQL_RES*)
rg -n -C3 '#define\s+MYSQL_QUERY\b' --type=cpp --type=h
# 2) Locate all mysql_store_result/mysql_num_rows pairs lacking local null checks
rg -n -C3 'mysql_store_result\(|mysql_num_rows\(' --type=cppRepository: sysown/proxysql
Length of output: 50372
🏁 Script executed:
# Get the exact context around the flagged lines in mcp_module-t.cpp
cat -n test/tap/tests/mcp_module-t.cpp | sed -n '155,165p'
cat -n test/tap/tests/mcp_module-t.cpp | sed -n '275,285p'
cat -n test/tap/tests/mcp_module-t.cpp | sed -n '291,301p'
cat -n test/tap/tests/mcp_module-t.cpp | sed -n '307,317p'
cat -n test/tap/tests/mcp_module-t.cpp | sed -n '323,333p'Repository: sysown/proxysql
Length of output: 2857
🏁 Script executed:
# Check test file for how other code handles mysql_store_result results
rg -n 'mysql_store_result.*\n.*mysql_num_rows' test/tap/tests/mcp_module-t.cpp -A2 -B2
# Check if there's documentation or comments about NULL safety
rg -n -B5 -A5 'mysql_store_result.*NULL|NULL.*mysql_store_result' test/tap/tests/mcp_module-t.cpp
# Look for pattern of mysql_store_result followed by immediate use
rg -n 'mysql_store_result' test/tap/tests/mcp_module-t.cpp -A1Repository: sysown/proxysql
Length of output: 1014
🏁 Script executed:
# Search for MariaDB documentation or comments about NULL handling in client library
rg -i 'mariadb|mysql.*null.*safe|null.*pointer' test/tap/ --type=cpp --type=h | head -30
# Check the MariaDB client library wrapper in test framework
cat test/tap/tap/utils.h | head -100Repository: sysown/proxysql
Length of output: 8406
🏁 Script executed:
# Check if mysql_store_result can return NULL in MariaDB docs or comments
rg -i 'mysql_store_result.*fail|store_result.*null|result.*null' test/tap/tests/ --type=cpp -B2 -A2 | head -60
# Check what other test files do with mysql_store_result
rg -n 'mysql_store_result' test/tap/tests/admin-listen_on_unix-t.cpp -A2 -B2Repository: sysown/proxysql
Length of output: 5326
Add null-checks after mysql_store_result() before calling mysql_num_rows().
All flagged lines call mysql_num_rows(res) immediately after mysql_store_result() without verifying res != nullptr. Even though mysql_query() succeeds (verified by the rc == 0 guards), mysql_store_result() can independently fail and return nullptr. The codebase pattern at line 103–105 and throughout utils.cpp shows this is required.
Suggested fix pattern
MYSQL_RES* res = mysql_store_result(admin);
+if (res == nullptr) {
+ diag("mysql_store_result failed: %s", mysql_error(admin));
+ skip(1, "Skipping row count check because result set is null");
+} else {
int num_rows = static_cast<int>(mysql_num_rows(res));
diag("Test X: Result has %d row(s)", num_rows);
ok(num_rows == 1, "...");
mysql_free_result(res);
+}Applies to lines: 159–160, 278–279, 294–295, 310–311, 326–327, 434–435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/mcp_module-t.cpp` around lines 159 - 160, After each call that
assigns MYSQL_RES* res = mysql_store_result(admin) (and similar temporary result
variables), check if res == nullptr before calling mysql_num_rows(res); if res
is nullptr handle the error path (e.g., log/return/fail the test) and only call
mysql_num_rows(res) when res is non-null, then always mysql_free_result(res)
after use; apply this null-check + free pattern to every occurrence using
mysql_store_result/admin and mysql_num_rows in this file (matching the existing
pattern used near lines ~103–105 and in utils.cpp).
| std::string upper_sql = sql; | ||
| for (size_t i = 0; i < upper_sql.length(); i++) { | ||
| upper_sql[i] = toupper(upper_sql[i]); | ||
| upper_sql[i] = static_cast<char>(toupper(upper_sql[i])); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++17, what does the standard require for std::toupper/std::tolower input values, and is casting via static_cast<unsigned char>(c) the correct safe pattern?
💡 Result:
In C++17, the standard requires that for std::toupper(int ch) and std::tolower(int ch) (declared in ), the input value ch must be representable as an unsigned char or equal to EOF; otherwise, the behavior is undefined. This inherits the requirement from the C standard's <ctype.h> (ISO C11 7.4.2.2 for toupper, similarly for tolower), as C++17 incorporates these headers with the same semantics also in [c.strings] / [cctype.syn] of draft N4659. Yes, casting via static_cast(c) is the correct safe pattern when c is of type char (plain, signed, or unsigned) or similar byte types from strings/arrays, ensuring the value is in [0, UCHAR_MAX] regardless of signedness. The result should then be cast back to char if needed, e.g.: char my_toupper(char ch) { return static_cast(std::toupper(static_cast(ch))); } This avoids UB if plain char is signed and holds a value with high bit set (e.g., accented characters in extended ASCII). For use with algorithms like std::transform on std::string, use a lambda: [](unsigned char c){ return std::toupper(c); }.
Citations:
- 1: https://en.cppreference.com/w/cpp/string/byte/toupper.html
- 2: https://in.cppreference.com/w/cpp/string/byte/toupper.html
- 3: http://cs.cppreference.com/w/cpp/string/byte/toupper.html
- 4: https://stackoverflow.com/questions/21805674/do-i-need-to-cast-to-unsigned-char-before-calling-toupper-tolower-et-al
- 5: https://timsong-cpp.github.io/cppwp/n4659/c.strings
- 6: https://en.cppreference.com/w/cpp/header/cctype.html
🏁 Script executed:
# First, let's check if the file exists and read the relevant sections around lines 66 and 294
wc -l test/tap/tests/nl2sql_internal-t.cppRepository: sysown/proxysql
Length of output: 99
🏁 Script executed:
# Read the area around line 66
sed -n '60,75p' test/tap/tests/nl2sql_internal-t.cppRepository: sysown/proxysql
Length of output: 476
🏁 Script executed:
# Read the area around line 294
sed -n '288,300p' test/tap/tests/nl2sql_internal-t.cppRepository: sysown/proxysql
Length of output: 452
🏁 Script executed:
# Search for all toupper/tolower calls in the file to see if there are other instances
rg 'toupper|tolower' test/tap/tests/nl2sql_internal-t.cpp -nRepository: sysown/proxysql
Length of output: 204
Use safe unsigned char conversion before toupper/tolower.
At Lines 66 and 294, passing char directly to toupper/tolower is undefined behavior in C++17 when the char value is negative. Cast the input to unsigned char first, then cast back to char for the assignment.
Suggested fixes
- upper_sql[i] = static_cast<char>(toupper(upper_sql[i]));
+ upper_sql[i] = static_cast<char>(std::toupper(static_cast<unsigned char>(upper_sql[i])));- if (hex_chars.find(static_cast<char>(tolower(id[i]))) == std::string::npos) {
+ if (hex_chars.find(static_cast<char>(std::tolower(static_cast<unsigned char>(id[i])))) == std::string::npos) {Also applies to: 294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/nl2sql_internal-t.cpp` at line 66, The code passes a plain
char to toupper/tolower which is UB for negative values; update the two
occurrences (the assignment using toupper on upper_sql[i] and the similar
tolower use at the other location) to call toupper/tolower with
static_cast<unsigned char>(upper_sql[i]) (or the corresponding buffer element)
and then static_cast<char> the result for the assignment so the call is made
with unsigned char and the stored value remains char.
| write_int16(out, weight); // weight | ||
| out += sizeof(int16_t); | ||
| write_int16(out, htons(sign)); // sign (converted to network byte order) | ||
| write_int16(out, static_cast<int16_t>(htons(sign))); // sign (converted to network byte order) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import socket
def write_int16(v: int) -> bytes:
v &= 0xFFFF
return bytes([(v >> 8) & 0xFF, v & 0xFF])
sign = 0x4000 # PostgreSQL NUMERIC negative sign word
wrong = write_int16(socket.htons(sign)) # current Line 190 behavior
right = write_int16(sign) # expected
print("wrong:", wrong.hex(), "expected:", right.hex())
assert wrong != right, "Expected mismatch proving double-conversion bug"
PYRepository: sysown/proxysql
Length of output: 85
🏁 Script executed:
cat -n test/tap/tests/pgsql-copy_from_test-t.cpp | sed -n '185,195p'Repository: sysown/proxysql
Length of output: 610
🏁 Script executed:
rg -A 10 'write_int16' test/tap/tests/pgsql-copy_from_test-t.cpp | head -30Repository: sysown/proxysql
Length of output: 1064
🏁 Script executed:
rg 'htons' test/tap/tests/pgsql-copy_from_test-t.cppRepository: sysown/proxysql
Length of output: 295
Fix double byte-order conversion for NUMERIC sign encoding.
Line 190 applies htons(sign) and then passes the result to write_int16, which already writes network-order bytes. This flips the sign word twice on little-endian systems and mis-encodes negative NUMERIC values.
Proposed fix
- write_int16(out, static_cast<int16_t>(htons(sign))); // sign (converted to network byte order)
+ write_int16(out, sign); // sign (write_int16 already serializes to network byte order)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| write_int16(out, static_cast<int16_t>(htons(sign))); // sign (converted to network byte order) | |
| write_int16(out, sign); // sign (write_int16 already serializes to network byte order) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-copy_from_test-t.cpp` at line 190, The NUMERIC sign
encoding double-converts the 16-bit sign by calling htons(sign) and then
write_int16 (which already emits network-order bytes); remove the htons call and
pass the raw int16_t sign to write_int16 (e.g., write_int16(out,
static_cast<int16_t>(sign))) so the sign is only converted once by write_int16
and negative NUMERIC values are encoded correctly.
Comment on lines
463
to
+468
| if (!mysql_real_connect(myconn, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | ||
| diag( | ||
| "Failed to connect to ProxySQL frontend addr=%s port=%d user=%s pass=%s err=%s", | ||
| cl.host, cl.port, cl.username, cl.password, mysql_error(myconn) | ||
| ); | ||
| return { EXIT_FAILURE, {} }; | ||
| return cleanup_and_fail(myconn, EXIT_FAILURE); |
There was a problem hiding this comment.
Do not log the frontend password on connect failures.
This path currently prints cl.password. If warmup fails in CI, the test will leak credentials into logs and artifacts. Redact it entirely.
Proposed fix
diag(
- "Failed to connect to ProxySQL frontend addr=%s port=%d user=%s pass=%s err=%s",
- cl.host, cl.port, cl.username, cl.password, mysql_error(myconn)
+ "Failed to connect to ProxySQL frontend addr=%s port=%d user=%s err=%s",
+ cl.host, cl.port, cl.username, mysql_error(myconn)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!mysql_real_connect(myconn, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| diag( | |
| "Failed to connect to ProxySQL frontend addr=%s port=%d user=%s pass=%s err=%s", | |
| cl.host, cl.port, cl.username, cl.password, mysql_error(myconn) | |
| ); | |
| return { EXIT_FAILURE, {} }; | |
| return cleanup_and_fail(myconn, EXIT_FAILURE); | |
| if (!mysql_real_connect(myconn, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| diag( | |
| "Failed to connect to ProxySQL frontend addr=%s port=%d user=%s err=%s", | |
| cl.host, cl.port, cl.username, mysql_error(myconn) | |
| ); | |
| return cleanup_and_fail(myconn, EXIT_FAILURE); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp` around lines 463 - 468,
The diagnostic log in the mysql_real_connect failure path currently includes the
plaintext frontend password (cl.password); update the diag call in the block
that checks mysql_real_connect (the same block that calls cleanup_and_fail on
error) to stop logging cl.password — either remove that parameter from the
format string or replace it with a redaction token like "[REDACTED]" so the
message retains context (host, port, user, error via mysql_error(myconn)) but
never prints cl.password.
Comment on lines
+165
to
167
| int r_row = rand() % static_cast<int>(NUM_ROWS); | ||
| if (r_row == 0) { r_row = 1; } | ||
|
|
There was a problem hiding this comment.
Fix off-by-one and skew in random row selection.
This logic never selects id == NUM_ROWS and hits id == 1 with higher probability. That biases coverage in both SELECT paths.
Proposed fix
- int r_row = rand() % static_cast<int>(NUM_ROWS);
- if (r_row == 0) { r_row = 1; }
+ int r_row = 1 + (rand() % static_cast<int>(NUM_ROWS));Also applies to: 457-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_binlog_reader-t.cpp` around lines 165 - 167, The random
row selection uses "int r_row = rand() % static_cast<int>(NUM_ROWS); if (r_row
== 0) { r_row = 1; }" which never yields id == NUM_ROWS and overweights id==1;
replace with a uniform 1..NUM_ROWS selection (e.g. compute r_row = (rand() %
static_cast<int>(NUM_ROWS)) + 1) in the test code (the same change should be
applied to the other occurrence referenced at lines ~457-458) so both SELECT
paths get unbiased coverage; update the r_row assignment in
test_binlog_reader-t.cpp accordingly.
Comment on lines
234
to
+236
| size_t total_nodes = cluster_nodes.empty() ? cluster_ports.size() : cluster_nodes.size(); | ||
| int np = 8; | ||
| np += 4*5*(4+(total_nodes-4)); | ||
| np += static_cast<int>(static_cast<size_t>(4)*5*(4+(total_nodes-4))); |
There was a problem hiding this comment.
Validate the 4-node assumption before this size_t math.
total_nodes - 4 underflows when the env override provides fewer than four nodes. That makes np bogus here, and the hard-coded core-node loops at Lines 312-330 will then index conns[0..3] out of bounds. Guard the minimum upfront and compute from total_nodes directly.
🔧 Proposed fix
size_t total_nodes = cluster_nodes.empty() ? cluster_ports.size() : cluster_nodes.size();
+if (total_nodes < 4) {
+ diag("Expected at least 4 nodes, got %zu", total_nodes);
+ return -1;
+}
int np = 8;
- np += static_cast<int>(static_cast<size_t>(4)*5*(4+(total_nodes-4)));
+ np += static_cast<int>(static_cast<size_t>(4) * 5 * total_nodes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/test_cluster1-t.cpp` around lines 234 - 236, The calculation
assumes at least 4 nodes and does unchecked subtraction which underflows when
total_nodes < 4; update the code around total_nodes and np (the variables
computed from cluster_nodes/cluster_ports and np) to first clamp total_nodes to
a minimum of 4 (e.g., compute min_nodes = std::max<size_t>(total_nodes, 4)) and
then use min_nodes in the np formula so the term (total_nodes-4) cannot
underflow; also ensure any later code that indexes conns[0..3] (the core-node
loops) uses the clamped value or otherwise guards accesses when total_nodes < 4.
…r_queue tempnam is dangerous (no atomic create, predictable names). Use mkstemp instead which creates the file atomically with O_EXCL.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/tap/tests/mysql-reg_test_4867_query_rules-t.cpp (1)
395-395:⚠️ Potential issue | 🟡 MinorValidate parse + range before narrowing
atoll()toint.At Line 395,
static_cast<int>(atoll(row[0]))can silently truncate out-of-range values. Please parse defensively and bounds-check before assignment.Suggested fix
- rule_ids[i] = static_cast<int>(atoll(row[0])); + char* endptr = nullptr; + errno = 0; + long long rid = strtoll(row[0], &endptr, 10); + if (endptr == row[0] || *endptr != '\0' || errno == ERANGE || + rid < 0 || rid > std::numeric_limits<int>::max()) { + diag("Invalid or out-of-range rule_id: %s", row[0] ? row[0] : "NULL"); + goto cleanup; + } + rule_ids[i] = static_cast<int>(rid);#!/bin/bash set -euo pipefail # Verify schema/type expectations for mysql_query_rules.rule_id in repo sources. rg -n -C3 --iglob '*.{sql,cpp,h,hpp,cc,c,cmake,txt}' \ 'CREATE TABLE.*mysql_query_rules|`?rule_id`?\s+' # Verify other parsing/casting sites for rule_id to compare handling patterns. rg -n -C2 --iglob '*.{cpp,h,hpp,cc,c}' \ 'atoll\(row\[0\]\)|strtoll\(row\[0\]|static_cast<int>\(atoll'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/mysql-reg_test_4867_query_rules-t.cpp` at line 395, The assignment using static_cast<int>(atoll(row[0])) (in the loop that fills rule_ids[i]) must be made defensively: parse row[0] with a proper integer parser (e.g., strtoll) and check for null/empty, conversion errors (errno or endptr), then verify the parsed long long is within INT_MIN..INT_MAX before narrowing to int; if out-of-range or invalid, handle the error path (return/fail/log) instead of silently truncating—update the code that fills rule_ids[i] and any related uses of atoll(row[0]) to follow this validated-parse-and-range-check pattern.
🧹 Nitpick comments (1)
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp (1)
316-318: Pre-existing issue:sizeof(buf_pos)returns pointer size, not buffer capacity.The casts added here are correct, but
sizeof(buf_pos)evaluates to the pointer size (8 bytes on 64-bit), not the remaining buffer space. This limits subsequent reads to 8 bytes regardless of available buffer capacity.♻️ Suggested improvement
while (r > 0 && r < NET_HEADER_SIZE) { - r = static_cast<int>(read(fd, buf_pos + r, sizeof(buf_pos))); + r = static_cast<int>(read(fd, buf_pos, sizeof(net_buf) - static_cast<size_t>(buf_pos - net_buf))); buf_pos += r;The same pattern applies to line 330.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp` around lines 316 - 318, The loop is using sizeof(buf_pos) (pointer size) for read length, which limits reads to pointer width; change the read call in the while loop that references buf_pos and r to read the remaining header bytes (e.g., use NET_HEADER_SIZE - r or the remaining buffer capacity) instead of sizeof(buf_pos), and apply the same fix to the similar read at the later occurrence (line ~330); ensure you still handle and propagate negative return values from read and update buf_pos and r correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/mysql-reg_test_4867_query_rules-t.cpp`:
- Line 395: The assignment using static_cast<int>(atoll(row[0])) (in the loop
that fills rule_ids[i]) must be made defensively: parse row[0] with a proper
integer parser (e.g., strtoll) and check for null/empty, conversion errors
(errno or endptr), then verify the parsed long long is within INT_MIN..INT_MAX
before narrowing to int; if out-of-range or invalid, handle the error path
(return/fail/log) instead of silently truncating—update the code that fills
rule_ids[i] and any related uses of atoll(row[0]) to follow this
validated-parse-and-range-check pattern.
---
Nitpick comments:
In `@test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp`:
- Around line 316-318: The loop is using sizeof(buf_pos) (pointer size) for read
length, which limits reads to pointer width; change the read call in the while
loop that references buf_pos and r to read the remaining header bytes (e.g., use
NET_HEADER_SIZE - r or the remaining buffer capacity) instead of
sizeof(buf_pos), and apply the same fix to the similar read at the later
occurrence (line ~330); ensure you still handle and propagate negative return
values from read and update buf_pos and r correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b948878f-51bf-46c9-8985-d129854d01d5
📒 Files selected for processing (16)
test/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/max_connections_ff-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/pgsql-admin_metacmds-t.cpptest/tap/tests/pgsql-extended_query_protocol_query_rules_test-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/pgsql-reg_test_4867_query_rules-t.cpptest/tap/tests/reg_test_3223-restapi_return_codes-t.cpptest/tap/tests/reg_test_3504-change_user-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpptest/tap/tests/sqlite_autocommit-t.cpptest/tap/tests/test_cluster_sync_pgsql-t.cpptest/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/test_sqlite3_server-t.cpptest/tap/tests/test_unsupported_queries-t.cpp
✅ Files skipped from review due to trivial changes (7)
- test/tap/tests/reg_test_3504-change_user-t.cpp
- test/tap/tests/sqlite_autocommit-t.cpp
- test/tap/tests/reg_test_3223-restapi_return_codes-t.cpp
- test/tap/tests/test_sqlite3_server-t.cpp
- test/tap/tests/test_cluster_sync_pgsql-t.cpp
- test/tap/tests/pgsql-extended_query_protocol_query_rules_test-t.cpp
- test/tap/tests/pgsql-admin_metacmds-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/tests/max_connections_ff-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers controlled by flags:PROXYSQL31=1for v3.1.x (FFTO, TSDB),PROXYSQLGENAI=1for v4.0.x (GenAI, MCP, Anomaly Detection).PROXYSQLGENAI=1impliesPROXYSQL31=1which impliesPROXYSQLFFTO=1andPROXYSQLTSDB=1
Conditional compilation uses#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSEto gate feature-specific code
Class names use PascalCase with protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session,ProxySQL_Admin)
Member variables use snake_case naming convention
Constants and macros use UPPER_SNAKE_CASE naming convention
Require C++17 standard for all source code
Use RAII (Resource Acquisition Is Initialization) pattern for resource management in C++ code
Use jemalloc for memory allocation in C++ code
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/pgsql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
test/tap/tests/**/{test_*.cpp,*-t.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppin test/tap/tests/
Files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/pgsql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/**/{test_*.cpp,*-t.cpp} : Test files follow naming pattern `test_*.cpp` or `*-t.cpp` in test/tap/tests/
Applied to files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` header files and link against `libproxysql.a` via custom test harness
Applied to files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_unsupported_queries-t.cpptest/tap/tests/pgsql-reg_test_4867_query_rules-t.cpptest/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/pgsql-multiplex_status_test-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/mysql-reg_test_4867_query_rules-t.cpptest/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Admin Interface implementation resides in `ProxySQL_Admin.cpp` and `Admin_Handler.cpp`, with schema versions tracked in `ProxySQL_Admin_Tables_Definitions.h`. Supports runtime config changes without restart via SQL-based configuration with SQLite3 backend
Applied to files:
test/tap/tests/test_sqlite3_pass_exts-t.cpptest/tap/tests/fast_forward_switch_replication_deprecate_eof.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Feature tiers controlled by flags: `PROXYSQL31=1` for v3.1.x (FFTO, TSDB), `PROXYSQLGENAI=1` for v4.0.x (GenAI, MCP, Anomaly Detection). `PROXYSQLGENAI=1` implies `PROXYSQL31=1` which implies `PROXYSQLFFTO=1` and `PROXYSQLTSDB=1`
Applied to files:
test/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-04-12T11:27:41.142Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-12T11:27:41.142Z
Learning: Applies to **/*.{cpp,h,hpp} : Conditional compilation uses `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, `#ifdef PROXYSQLFFTO`, `#ifdef PROXYSQLTSDB`, and `#ifdef PROXYSQLCLICKHOUSE` to gate feature-specific code
Applied to files:
test/tap/tests/genai_async-t.cpptest/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
📚 Learning: 2026-02-13T09:29:39.713Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.
Applied to files:
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp
[error] 21-21: 'cerrno' file not found
(clang-diagnostic-error)
🔇 Additional comments (26)
test/tap/tests/test_sqlite3_pass_exts-t.cpp (1)
426-426: Format-string normalization looks good.Using
diag("%s", "")on Line 426, Line 432, Line 440, and Line 451 preserves the empty-line output while aligning with printf-style lint expectations.Also applies to: 432-432, 440-440, 451-451
test/tap/tests/fast_forward_switch_replication_deprecate_eof.cpp (1)
120-124: Good cleanup of TAP diagnostics/assertions.The switch to printf-style
ok()messages and%zufor the loop index is correct and safer for type formatting.Also applies to: 131-131
test/tap/tests/pgsql-multiplex_status_test-t.cpp (1)
216-216: Formatting refactor looks good.These
ok()message updates are clean and keep the assertions unchanged while improving TAP output consistency.Also applies to: 220-220, 224-224
test/tap/tests/pgsql-reg_test_4867_query_rules-t.cpp (1)
130-133: Type/format normalization inunique_ipis solid.Using
unsigned intwith%uand a larger buffer is a clean fix for format-safety in this path.test/tap/tests/mysql-reg_test_4867_query_rules-t.cpp (2)
83-86:unique_iptype/format normalization looks correct.The
%uchange now matches theunsigned intoctet type, and the larger buffer removes prior tight sizing concerns.
258-285: Macro argument parenthesization fix is good.Parenthesizing
expected_val/expected_strmakes these macros safer for expression arguments and avoids precedence-related miscomparisons.test/tap/tests/test_unsupported_queries-t.cpp (1)
527-527: Good normalization of blank diagnostics.Switching to
diag("%s", "")is consistent and avoids raw empty-format invocations.Also applies to: 532-532, 558-558
test/tap/tests/genai_async-t.cpp (4)
59-59: Type-normalization changes look correct.The explicit
static_cast<int>(...)on MySQL row/field counts is consistent with the linting cleanup and keeps comparisons/logging typed predictably.Also applies to: 90-90, 129-129
476-476: Explicit char narrowing is clear and safe.
static_cast<char>('A' + i)makes the intent explicit and avoids implicit-conversion warnings.
852-852: Main-flow call is fine without test-count accumulation.Calling
test_concurrent_connections(...)for TAP side effects is valid;ok()/plan()remain the authoritative counters.
864-864: Consistent blank-line diag formatting.Using
diag("%s", "")here matches the rest of the TAP lint cleanup.Also applies to: 869-869
test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp (15)
216-220: Previous review feedback still applies.The past review comment about checking
fprintf/fflush/fclosereturn values remains valid and unaddressed.
467-472: Previous review feedback still applies.The past review comment about not logging the password in the failure diagnostic remains valid and unaddressed.
21-21: LGTM -<cerrno>is a valid C++ standard header.The static analysis hint reporting this file as not found is a false positive.
<cerrno>is defined in the C++ standard library and is required here forstrerror(errno)usage in the error handling paths below.
67-76: LGTM - Improved type safety and error handling.The explicit casts clarify intent, and changing from
res == 0tores <= 0correctly routes both poll timeout and poll error cases through the sameMYSQL_WAIT_TIMEOUTreturn path.
88-100: LGTM - Cleaner naming convention.Renaming from double-underscore to single-underscore is a stylistic improvement. The context snippet shows the parallel file
reg_test__ssl_client_busy_wait-t.cppstill uses the old naming, but each test file maintains its own local struct definitions, so this inconsistency is acceptable.
150-176: LGTM - Secure temp file handling and proper cleanup.Good improvements:
mysql_initnull check prevents potential null-pointer dereference- Using
mkstempinstead oftempnameliminates TOCTOU race conditions- Proper cleanup with
mysql_closeandunlinkon both success and failure paths
194-214: LGTM - Robust temp file creation with fdopen.The
mkstemp+fdopenpattern is the correct approach for writing to a secure temporary file. Error handling formkstempandfdopenfailures is comprehensive with proper cleanup of all acquired resources.
368-368: LGTM - Explicit cast for send() return value.
382-406: LGTM - Added null check and proper cleanup on failure.Good defensive coding: checking
mysql_initreturn and callingmysql_closewhenmysql_real_connectfails ensures consistent resource handling.
416-420: LGTM - Proper cleanup on query failure.
449-455: LGTM - Good cleanup helper for error paths.The
cleanup_and_faillambda ensures both the in-flightMYSQL*handle and all previously pooled connections are properly closed on any error path, preventing resource leaks during warmup failures.
533-565: LGTM - Added mysql_free_result calls to prevent result-set leaks.Good practice to free result sets after use, even in test code.
607-609: LGTM - Updated to use renamed struct types.
878-878: LGTM - Explicit cast for plan() argument.
891-891: LGTM - Type-safe conversion for idle_sess_ms parameter.
|
OpenSSL 3.0 deprecated MD5_Init/MD5_Update/MD5_Final. Use EVP_MD_CTX family instead, following the same pattern as MySQL_encode.cpp.
- Fix sign-compare warnings in ProxySQL_RESTAPI_Server (size_t vs int) - Remove duplicate itostr macro in Base_HostGroups_Manager, MySQL_HostGroups_Manager, PgSQL_HostGroups_Manager (already in proxysql_macros.h) - Mark unused 'thr' variable in Base_Thread::run_SetAllSession_ToProcess0 - Remove redundant query_no_space_alloc_size in Admin_Handler (use query_length directly) - Fix string literal to char* assignment in MySQL_HostGroups_Manager
…rnings - Remove duplicate MCP_QUERY_RULES/RUNTIME_MCP_QUERY_RULES macros from inside #ifdef PROXYSQLGENAI block (newer schema outside wins) - Fix member initialization order in MySQL_Tool_Handler and ProxySQL_MCP_Server constructors to match declaration order - Use %ld with (long) cast in LLM_Clients log macros to handle both int (literal 0) and long (http_code) arguments
- Fix snprintf truncation in MySQL_Monitor ConsumerThread (%.15s for 16-byte buffer) - Mark unused safe_update_peer_info in ProxySQL_Cluster with __attribute__((unused)) - Remove unused start_time in GenAI_Thread::generate_embeddings - Remove unused now_iso() in Discovery_Schema
…-analysis # Conflicts: # test/tap/tests/set_testing-240-t.cpp
Previously tests listed in groups.json but not found on disk were only printed as notes. Now they cause a non-zero exit code.
This test was removed in 06b6343 and replaced by gtid_set_unit-t and gtid_trxid_interval_unit-t. The merge with v3.0 accidentally restored the old entry.
Two recent "cleanup" commits on this branch silently changed test behavior and caused the tests to fail under myrun2. 1) kill_connection2-t.cpp (regressed in 3f0cd6a): The "cleanup - replace NULL with nullptr, convert loops to range-based" commit flipped one assertion from ok(rc == 0, ...) to ok(rc != 0, ...) on the first DO 1 batch after a 12s sleep. With mysql-max_transaction_time=17000 (17s), the connection is still alive at 12s by design — ProxySQL only kills it at ~18s. The original test correctly expected rc == 0 there; the second DO 1 batch (after +12s = 24s total) continues to correctly expect rc != 0. 2) max_connections_ff-t.cpp (regressed in da65509): The "static analysis narrowing conversions" commit introduced an integer overflow by wrapping a chrono::nanoseconds count in static_cast<int>(). For the 8000ms test the elapsed time is ~8e9 ns, which overflows int32 (max ~2.147e9) and wraps negative, producing "Waited: -0.589502" and a failed assertion. The 2000ms test happened to fit under 2.147e9 ns and kept passing, hiding the bug. Use static_cast<double> for the nanoseconds count, and drop the unnecessary int casts on connect_timeout / poll_timeout — those are already long, so long / 1000.0 is a valid double.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lint-testsflow and shared clang-tidy normalization for TAP tests.NULL->nullptr, braces, a few narrowings, and some helper simplification).Test Plan
python3 scripts/lint/test_clang_tidy_utils.pypython3 scripts/lint/test_run_tap_tests.pySummary by CodeRabbit
New Features
Bug Fixes
Quality Improvements