9488 Commits (f7ee5cd028ff34efea42a41dc19cb24be121b65e)

 

All Branches   

Search

Author	SHA1	Message	Date
René Cannaò	f7ee5cd028	Merge pull request #5614 from sysown/v3.0-pgsql-monitor-reschedule-on-interval-change ... fix(pgsql-monitor): clamp next_*_at on LOAD PGSQL VARIABLES TO RUNTIME	1 month ago
Rene Cannao	4bd2f95e5b	fix(pgsql-monitor): add proxy_debug logging and clean up test variable ... - Add proxy_debug(PROXY_DEBUG_MONITOR, 5) calls when clamping fires, so the fix is observable in debug logs. Follows existing pattern from nearby scheduling debug output. - Rename original_monitor_username to monitor_username and clarify it is for diagnostic logging only, since the variable is no longer used for restore after the credential change was removed.	1 month ago
René Cannaò	c678ae40d7	Merge pull request #5616 from sysown/v3.0-remove-sqlite-rembed ... build: remove sqlite-rembed and the Rust toolchain dependency	1 month ago
Rene Cannao	e0a96d380e	chore: fix stale rembed reference and simplify test Makefile ... - Remove stale comment reference to rembed in AI_Features_Manager.cpp - Remove redundant ifeq/else/endif in anomaly_detection-t target (both branches identical after libsqlite_rembed.a removal)	1 month ago
René Cannaò	da0e67b587	Merge pull request #5618 from sysown/ci-add-unit-tests-asan-coverage ... ci: add CI-unit-tests-asan-coverage workflow	1 month ago
Rene Cannao	92949a9404	ci: add CI-unit-tests-asan-coverage workflow ... 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.	1 month ago
Rene Cannao	6cfcb2b40a	docs: drop Rust toolchain claim from CLAUDE.md and top-level Makefile ... The AI/MCP tier (PROXYSQLGENAI=1) no longer requires rustc/cargo after the sqlite-rembed removal. Follow-up to the preceding cleanup commits to keep the user-facing tier documentation honest.	1 month ago
Rene Cannao	57c3e3fa28	docs: excise sqlite-rembed sections from GENAI.md and SQLite3-Server.md ... - GENAI.md: Remove two rembed-related links from Related Documentation section - SQLite3-Server.md: Delete Embedding Generation section (was lines 72-103) - SQLite3-Server.md: Remove rembed-dependent use cases (embedding generation, AI pipelines)	1 month ago
Rene Cannao	f821ebcdc7	docs: remove sqlite-rembed guides, examples, and embeddings script	1 month ago
Rene Cannao	c3266dd4f3	chore: remove vendored sqlite-rembed-0.0.1-alpha.9 tarball	1 month ago
Rene Cannao	3832c41626	build(deps): remove sqlite-rembed build rule and Rust toolchain check ... libsqlite_rembed.a is no longer linked into any ProxySQL binary or test as of the preceding commits, so the build rule, the Rust toolchain detection block, the SQLITE3_* environment exports, the sqlite-rembed phony target, and the rembed clean lines can go. The sqlite-vec extension stays (it is C, not Rust). After this commit, PROXYSQLGENAI=1 builds no longer require rustc or cargo on PATH.	1 month ago
Rene Cannao	44a6c9d583	build(test): drop libsqlite_rembed.a from TAP and rag test link order	1 month ago
Rene Cannao	3bcc1dd43f	build: drop libsqlite_rembed.a from src/lib link order	1 month ago
Rene Cannao	886f09c211	chore(sqlite): remove dead-code sqlite-rembed hook pointer ... proxy_sqlite3_rembed_init was declared unconditionally as NULL with a TODO comment ("Fix sqlite-rembed header inclusion and assign the function pointer properly") and never reassigned anywhere in the tree. The sole caller in Admin_Bootstrap::__bootstrap was null-guarded, so the sqlite-rembed auto-extension was never registered at runtime in any build tier, stable or GENAI. This commit removes the pointer, its extern declaration, and the null-guarded call. No runtime behavior changes; subsequent commits remove the build-system wiring and the Rust toolchain dependency that existed solely to produce libsqlite_rembed.a.	1 month ago
Rene Cannao	27daf2c21d	docs(plan): add plan for removing sqlite-rembed and Rust dep	1 month ago
Rene Cannao	c7dff77a83	chore: gitignore .worktrees/ for local worktree workflow	1 month ago
René Cannaò	fd01e0f6f6	Merge pull request #5613 from sysown/v3.0-fix-read-only-actions-hostgroup ... test(read_only_offline_hard): set default_hostgroup to WHG, not hardcoded 0	1 month ago
René Cannaò	793a9c3765	Merge pull request #5612 from sysown/v3.0-fix-pgsql-ssl-keylog-path ... test(pgsql-ssl_keylog): fix container-local path + NSS label regex	1 month ago
René Cannaò	caa2129eee	Merge pull request #5611 from sysown/v3.0-fix-flake-test-flush-logs ... test(flush_logs): harden timing race — poll instead of fixed sleep	1 month ago
Rene Cannao	30be016be5	fix(pgsql-monitor): clamp next_*_at on LOAD PGSQL VARIABLES TO RUNTIME ... Closes the pgsql-servers_ssl_params-t portion of sysown/proxysql#5610. This is the final of four tests from the flake tracking issue. ## Problem 1 — the scheduler caching bug (lib/PgSQL_Monitor.cpp) `pgsql-servers_ssl_params-t` subtests 32 ("Monitor SSL counter increased with use_ssl=1 and no per-server row") and 34 ("Monitor per-server: SSL OK counter resumes advancing after removing the per-server row") were failing deterministically on v3.0. Both expected `PgSQL_Monitor_ssl_connections_OK` to increase within a few seconds of: SET pgsql-monitor_connect_interval=2000; LOAD PGSQL VARIABLES TO RUNTIME; UPDATE pgsql_servers SET use_ssl=1; LOAD PGSQL SERVERS TO RUNTIME; but the counter stayed at 0. Root cause is state caching in the pgsql monitor scheduler loop (`PgSQL_monitor_scheduler_thread()`): T=0 proxysql starts with default pgsql-monitor_connect_interval =120000. The first scheduler tick schedules an initial connect check; compute_next_intvs() sets next_connect_at = T + 120000 ms = T + 120 seconds. T+<30s> test does SET connect_interval=2000 + LOAD PGSQL VARIABLES TO RUNTIME. fetch_updated_conf() starts returning the new 2000 value, but next_connect_at still points at T+120000 because compute_next_intvs() only updates next_<type>_at when the corresponding task type has fired. T+<35s> test reads ssl_connections_OK at the end of its 5-second wait. Counter still 0 because the next scheduled connect check is ~85 seconds in the future. The scheduler is working correctly; what's missing is a bridge between "runtime variables were just refreshed" and "next_<type>_at should reflect the refreshed (shorter) intervals". Fix: in the scheduler loop, track whether the variable version bumped this iteration, and if so, clamp each next_<type>_at down to cur_intv_start + new_interval whenever the refreshed interval would schedule the next check sooner than the currently-cached value. The clamp is one-way (we never push next_<type>_at FURTHER into the future, because growing the interval should not delay an already- imminent check), idempotent, and safe against interval=0 (disabled) which is left to compute_next_intvs() to set to ULONG_MAX. Applied to all four monitor task types: ping, connect, readonly, repl_lag. Same class of bug affected all of them; fixing only connect would leave analogous latent issues for any test that changes the other intervals at runtime. Verified with the raw experiment before committing: configure monitor/monitor + interval=2000 + use_ssl=1 + LOAD, observe counter ticking at the 2-second cadence within 1-2 seconds of LOAD. Before fix: counter stuck at 0 for ~2 minutes (until the cached 120 s interval naturally elapses). ## Problem 2 — test's hardcoded wrong credentials (pgsql-servers_ssl_params-t.cpp) With the scheduler fix alone, the test was still failing because its main() was doing: SET pgsql-monitor_username='postgres'; SET pgsql-monitor_password='postgres'; on the assumption that the backend had a postgres user with password "postgres". But the actual test infra (test/infra/docker-pgsql16-single) RANDOMIZES `POSTGRES_PASSWORD` on every container startup — e.g. `POSTGRES_PASSWORD=05e792e51d`. Hardcoded 'postgres' never matched, so every monitor connect failed with: FATAL: password authentication failed for user "postgres" which increments connect_check_ERR instead of ssl_connections_OK. After the scheduler fix, these auth failures fired every 2 seconds instead of every 120 seconds — but they were still failures, so the counter never advanced. Fix: remove the username/password switch entirely. The default monitor/monitor user is already configured in the infra's pg_hba.conf and authenticates successfully (I verified this manually via `docker exec psql 'host=... user=monitor password=monitor sslmode=require'` from both inside and outside the proxysql container). The corresponding "restore original values" block is also removed since there's nothing to restore. ## Local verification 3 consecutive runs of the full pgsql-servers_ssl_params-t test in legacy-g4 infra with the patched proxysql + patched test binary: attempt 1: PASS attempt 2: PASS attempt 3: PASS === pgsql-servers_ssl_params-t: 3/3 pass === Subtest-level confirmation from the final attempt's TAP log: # Original monitor: user=monitor interval=120000 ms # Initial PgSQL_Monitor_ssl_connections_OK: 33 # After PgSQL_Monitor_ssl_connections_OK: 36 <- +3 in 5 s ok 32 - Monitor SSL counter increased with use_ssl=1 and no per-server row # With TLSv1 per-server pin, ssl OK before wait: 39 # With TLSv1 per-server pin, ssl OK after wait: 39 (delta=0) ok 33 - Monitor per-server: SSL OK counter does NOT advance when per-server row pins ssl_protocol_version_range to TLSv1 # After cleanup, ssl OK recovered from 41 to 44 <- +3 in 5 s ok 34 - Monitor per-server: SSL OK counter resumes advancing after removing the per-server row All three monitor-SSL subtests now exercise the real code path (SSL handshake happening, counter incrementing, per-server pin blocking SSL as designed) instead of observing a no-op. ## Side effect on pgsql-ssl_keylog-t subtest 7 Subtest 7 of pgsql-ssl_keylog-t was marked as SKIP in PR #5612 because it tripped on the same "pgsql monitor isn't making SSL connections" symptom. With this fix merged, the skip's runtime condition (`lines_before_monitor == lines_after_monitor`) will evaluate to false once the monitor is actually producing SSL handshakes, and the test will fall into the `ok(...)` branch automatically. No separate change to that test is needed — the skip was defensive and is dead code after this fix. ## Why mysql monitor is not touched here This fix is scoped to the pgsql monitor scheduler only. The mysql monitor is a different file (lib/MySQL_Monitor.cpp) with a different scheduling architecture (per-thread timers, not a centralized scheduler). If the same class of bug exists there, it would need a separate patch - out of scope for this PR.	1 month ago
Rene Cannao	5d35a213ce	test(read_only_offline_hard): set default_hostgroup to WHG, not hardcoded 0 ... Part of sysown/proxysql#5610 — third of four tests from the flake tracking issue. Like pgsql-ssl_keylog-t, this turned out to be a deterministic failure miscategorized as a flake. ## Background: the incomplete hostgroup migration Two commits on v3.0 main set up the stage: 1. 26c5a2572 ("Fix read_only test assuming default_hostgroup=0"): fixed the ORIGINAL bug where the test's writer hostgroup was 0 but the CI infra set user.default_hostgroup to 1300. The fix set default_hostgroup=0 at the start of each scenario so the user routing matched the test's servers. 2. 4f9ab49e7 ("Fix hardcoded hostgroups in TAP tests for mysql84"): introduced TAP_MYSQL8_BACKEND_HG env var so the test's writer/ reader hostgroups could be overridden for mysql84 (WHG=2900, RHG= 2901 in the mysql84-g4 group). init_hostgroups() at file scope reads this env var into the static `WHG` and `RHG` symbols. The second commit updated the `INSERT INTO mysql_servers` calls to use WHG/RHG, but missed the `UPDATE mysql_users SET default_hostgroup=0` added by the first commit. So on mysql84-g4: - insert_mysql_servers_records() puts servers in hostgroup 2900 (WHG) - UPDATE mysql_users SET default_hostgroup=0 points the user at hostgroup 0, which has no servers - the test opens a proxy connection as that user, runs BEGIN, and ProxySQL routes BEGIN to hostgroup 0 — which is empty - 10 s later: "Max connect timeout reached while reaching hostgroup 0" - test aborts at subtest 4 of 18, not a flake Verified by querying the running proxysql during a failing run: SELECT username, default_hostgroup FROM mysql_users; -> root | 0 user | 2900 testuser | 2900 ... `root` is the user the test connects as (cl.root_username). Still at 0 after the setup step tried to make it match. In mysql84-g4 that's wrong. ## Fix Change the two `string_format("UPDATE mysql_users SET default_hostgroup=0 ...")` calls in test_scenario_1 and test_scenario_2 to use the `WHG` static symbol (already populated from TAP_MYSQL8_BACKEND_HG by init_hostgroups() earlier in the same file), so the user's default_hostgroup always matches whatever writer hostgroup the test configured its servers into. Zero behavior change on the legacy infra (where WHG defaults to 0 so the literal value is unchanged); correct behavior on the mysql84 infra (where WHG=2900). Same two-line pattern in both scenario functions. ## Local verification (dogfood of test/README.md §"Debugging a flaky test") 3 consecutive local iterations in mysql84-g4 + TEST_PY_TAP_INCL=test_read_only_actions_offline_hard_servers-t: attempt 1: PASS attempt 2: PASS attempt 3: PASS === read_only_actions: 3/3 === Pre-fix: 0/3 (all hit the "Max connect timeout reached while reaching hostgroup 0 after 10000ms" error and aborted at subtest 4 of 18). Post-fix: 18/18 subtests run and pass on every iteration. ## Not touched here - `pgsql-servers_ssl_params-t` — still stuck on the pgsql monitor not making SSL connections under `use_ssl=1`. Same open question as subtest 7 of pgsql-ssl_keylog-t (skipped in PR #5612). Needs pgsql monitor source analysis, not a test-side fix.	1 month ago
Rene Cannao	0f0c3b6609	test(pgsql-ssl_keylog): fix container-local path + NSS label regex ... Part of sysown/proxysql#5610. Addresses the CI-legacy-g4 'pgsql-ssl_keylog-t' failure that had been miscategorized as a flake but was actually two deterministic bugs. ## Bug 1 — container-local keylog path The test used `/tmp/pgsql_ssl_keylog_test.log` as the shared keylog path between ProxySQL (writer) and the test runner (reader). In the isolated CI infra each of those processes runs in its own docker container, and each container has its own /tmp/, so writes from ProxySQL inside its container are invisible to the test runner in a different container. Every subtest that called count_file_lines() got -1 because it was reading a file that never existed in its own container's filesystem. Verified locally by `docker exec`-ing into the proxysql container and confirming the file existed there at 33 KB of real TLS secrets while the test runner saw it as missing. Fix: change KEYLOG_PATH to `/var/lib/proxysql/pgsql_ssl_keylog_test.log`. That directory IS bind-mounted from the host into BOTH containers by test/infra/control/run-tests-isolated.bash (see the `-v ${PROXY_DATA_DIR_HOST}:/var/lib/proxysql` mounts), so writes on one side are immediately readable on the other. After this fix alone, 6 of 9 subtests went from FAIL to PASS (tests 1, 2, 3, 5, 6, 8 which all relied on reading the keylog file back). ## Bug 2 — NSS label regex excluded digits With the path fixed, test 4 still failed. The "Concurrent SSL connections" test iterates every line in the keylog file and validates each against the nss_re regex, failing the whole test if any line doesn't match. The regex was: ^[A-Z_]+ [0-9a-fA-F]{64} [0-9a-fA-F]+$ i.e. the label was restricted to uppercase letters and underscores. But TLS 1.3 traffic-secret labels contain digits: CLIENT_TRAFFIC_SECRET_0, SERVER_TRAFFIC_SECRET_0 (and in principle _1, _2, ... for rekeying). Every one of those valid NSS lines was flagged as "corrupt" by test 4 because [A-Z_]+ doesn't match the '0'. The concurrent-connections scenario generated many TLS 1.3 handshakes back-to-back, producing many traffic-secret lines, so the test failed reliably. Test 2 passed despite the same regex because file_has_regex_match returns true as soon as SOME line matches — non-digit labels like CLIENT_HANDSHAKE_TRAFFIC_SECRET were still in the file, so test 2 was satisfied even though the regex was too narrow. Fix: change the character class to [A-Z0-9_]+ which matches all valid NSS label formats including the _<N> suffix. ## Test 7 — deferred via SKIP marker With the two above fixes, 8 of 9 subtests pass. Test 7 ('Monitor SSL connections write keylog entries') still fails for an UNRELATED underlying reason: after UPDATE pgsql_servers SET use_ssl=1 + LOAD TO RUNTIME, the pgsql monitor does not make SSL connections. It continues making non-SSL connections (PgSQL_Monitor_non_ssl_connections_OK increases, PgSQL_Monitor_ssl_connections_OK stays at 0). No SSL handshake = no keylog entries. This matches exactly the behavior observed in pgsql-servers_ssl_params-t subtests 32 and 34, which also expect the monitor to make SSL connections under the same config. The question "is the test's use_ssl=1 assumption correct, or does the pgsql monitor have a regression" is an OPEN item in #5610 that needs proxysql source-level analysis and is out of scope for this commit (which is test-only). To keep the test green on the two legitimate bugs I DID fix while that third question is resolved, test 7 now emits a libtap skip(1, ...) rather than ok(false, ...) when the monitor keylog file shows no new lines during the wait window. This produces a proper "ok 7 # SKIP ..." TAP line with a reason string referencing #5610, so a future maintainer reading the test output will see the subtest is being skipped and why, and can remove the skip once the monitor behavior is understood. ## Local verification (dogfood of test/README.md §"Debugging a flaky test") 5 consecutive local runs in legacy-g4 + TEST_PY_TAP_INCL=pgsql-ssl_keylog-t: attempt 1: PASS attempt 2: PASS attempt 3: PASS attempt 4: PASS attempt 5: PASS === pgsql-ssl_keylog-t: 5/5 pass === TAP output confirms: ok 1..6 — all using the new /var/lib/proxysql/ path, reading back real TLS secrets ok 7 — # SKIP pgsql monitor did not produce SSL connections ... ok 8..9 — passing Test took ~12 seconds per iteration (no timing change; this is not a race fix, it's a bug fix).	1 month ago
Rene Cannao	5444125bd2	test(flush_logs): harden timing race — poll instead of fixed-sleep ... Part of sysown/proxysql#5610 (flaky TAP tests revealed by cascade fix). Addresses the CI-legacy-g3 'test_flush_logs-t' flake that failed on some PR #5596 commits but passed on others despite identical code. ## Root cause The test measures "does command X cause proxysql.log to gain one new '[INFO] ProxySQL version' marker line". The old flow used fixed sleeps: BASELINE=$(fn_get_rotations) # (includes sleep 1) fn_padmin "PROXYSQL FLUSH LOGS;" RES=$(fn_get_rotations) # (sleep 1, then count) fn_check_res # strict equality check For the SIGUSR1 subtest the flow was worse because the signal had to travel through a "Scheduler Hack" (fn_signal inserts a scheduler row that runs `kill -USR1 $(pidof proxysql)` every 2000 ms, waits 4 s, then deletes the row). Total wall-clock budget from signal scheduling to log read: 5 s including the inner fn_get_rotations sleep. On a slow / contended CI runner any one of three asynchronous stages can slip past that 5 s window: 1. The scheduler's first interval fires slightly late. 2. The SIGUSR1 handler in proxysql queues the log rotation on a worker thread; the actual rotation + re-open + banner write happens after the signal is delivered, not synchronously. 3. The host-side read of proxysql.log via the docker volume bind-mount can lag the in-container write by up to a second on Azure-hosted runners under load. When any of those raced past 5 s, fn_get_rotations returned the pre-rotation count, fn_check_res saw `RES != BASELINE+1`, the test flagged a failure, and CI went red. Same code ran fine on the next retrigger because the race was environmental, not logical. A second, subtler issue: the strict `-ne` equality check in fn_check_res would ALSO fail if the scheduler fired twice before the script got around to deleting its row (interval_ms was 2000, sleep was 4 s - one extra fire was a coin flip on fast runners). One signal is the contract; two rotations from one signal is still the correct observable answer for "did this trigger at least one rotation". ## Fix Two changes, both in the .sh (the -t file is a gitignored copy regenerated by `make sh-test_flush_logs-t.sh`, which itself just `cp`s the .sh — same rule applies to every other `*-t.sh` test): 1. Replace fixed sleeps with bounded polling: - fn_get_rotations no longer has an internal `sleep 1` — it is now a pure "read the current count" function. - New fn_wait_for_rotation_at_least <target> <max_seconds> polls fn_get_rotations every 500 ms until the count reaches the target or the timeout expires. On success it prints the observed count and returns 0; on timeout it prints whatever it last saw and returns 1, so fn_check_res still reports a useful "BASELINE X expected Y got Z" error message. - Max budget raised to 30 s (was 5 s). On a fast machine the poll still terminates within one 500 ms cycle, so total test time on the local dev workstation dropped from ~9 s to ~1 s. 2. Accept "at least BASELINE+1" instead of "exactly BASELINE+1": - fn_check_res uses `-lt` instead of `-ne`. - Test semantics: "did the command trigger at least one rotation". Two rotations from one signal is still a pass (happens only in the scheduler-hack corner, when the scheduler fires twice before fn_signal_cleanup tears it down; harmless to proxysql behavior). - Also drops the scheduler interval from 2000 ms to 500 ms so the first fire happens quickly under normal conditions. 3. Split fn_signal into fn_signal (install only) and fn_signal_cleanup (tear down the scheduler row). The caller now: fn_signal SIGUSR1 RES=$(fn_wait_for_rotation_at_least $((BASELINE+1)) 30) fn_signal_cleanup fn_check_res ... which is race-free: the cleanup runs AFTER the rotation has been observed, so the scheduler row exists only for the bounded window between install and observation. ## Local verification Reproduced the original PR #5596 failure context (legacy-g3 group with only test_flush_logs-t enabled via TEST_PY_TAP_INCL), ran 10 back-to-back iterations against a single infra lifecycle following the recipe in test/README.md §"Debugging a flaky test": for i in 1..10: ./test/infra/control/run-tests-isolated.bash Results: - 10/10 pass - Per-attempt wall time dropped from ~9 s to ~1 s - Confirmed new code path active via the new "got N (>= BASELINE+1)" message format in the captured TAP output ## What's still TODO in #5610 The other three flaky tests from the issue are C++ and each needs its own targeted investigation. They are NOT touched here: - pgsql-servers_ssl_params-t - pgsql-ssl_keylog-t - test_read_only_actions_offline_hard_servers-t Deliberately keeping this PR small - one test per PR so each fix can be reviewed and validated independently.	1 month ago
René Cannaò	04466b35a5	Merge pull request #5607 from sysown/v3.0-doc-gh-actions-vocabulary ... doc(GH-Actions): add detailed vocabulary section with diagrams	1 month ago
René Cannaò	ca1a7a4deb	Merge pull request #5609 from sysown/v3.0-doc-test-readme ... doc(test): document per-test log paths and flaky-test debugging recipe	1 month ago
Rene Cannao	06176265f3	doc(test): address gemini + coderabbit review nits on README flake section ... Three minor corrections from the PR #5609 review: 1. (coderabbit, MD040) The directory-tree fenced block at L74 had no language tag. Added `text` so markdownlint and GitHub's renderer treat it as plain text rather than guessing a language that would then syntax-highlight the tree characters incorrectly. 2. (gemini, L142) The flake-debug loop's "which attempts failed" grep was too narrow: `grep 'FAIL [1-9]'` only matches the proxysql-tester.py summary line "SUMMARY: 'tests' PASS N/M : FAIL K/M" when K is non-zero - but misses tests that fail at the TAP level via "not ok" without incrementing the FAIL counter (e.g. when the binary crashes or times out). Broadened to `grep -lE 'not ok|FAIL [1-9]'` so both paths are caught, matching the idiom already used in the "Where logs actually live" section above. 3. (gemini, L159) The Troubleshooting bullet suggested `docker network prune` as the cleanup idiom for dangling networks. `network prune` is a GLOBAL operation - it would also wipe networks belonging to unrelated Docker projects on the same host, which is user-hostile if a maintainer has other test infras running in parallel. Changed to prefer the targeted form `docker network rm "${INFRA_ID}_backend"` and explicitly call out why `prune` is the wrong default. No change to the "Where logs actually live" section, the "Debugging a flaky test" loop itself, or any of the other content. Doc-only update on top of the earlier commit.	1 month ago
Rene Cannao	81b406a428	doc(GH-Actions): address gemini review nits on §12 vocabulary section ... Four minor corrections from the PR #5607 review: 1. §12 intro: "from issue" → "from the issue" (missing article) 2. §12.1.1: comma splice "(... — ProxySQL does exactly this, see #7 below)" → semicolon for grammatical correctness 3. §12.1.1 YAML example: replaced the `← ← ←` arrow annotations with `# <- ...` comments so the snippet is valid YAML if copy-pasted 4. §12.1.1: "has runs on 10 commits" → "has run on 10 commits" (past participle) No change to meaning or structure. The `←` arrows that remain in the ASCII box diagrams at §12.2 / §12.3 / §12.4 are untouched - those diagrams are illustrative text inside un-typed fenced blocks, not copy-pasteable code, and the gemini review only flagged the yaml-typed block. Coderabbit's "Critical" finding about anchor links using `--` instead of `-` is a false alarm: the em-dash in the §12 heading is stripped by GitHub's slugger, leaving two adjacent spaces which become two adjacent hyphens. I verified this by asking GitHub's own `/markdown` API to render the heading and inspecting the generated `<a id="...">`, which contains exactly `...vocabulary--read-this-first-if-confused` (double hyphen). No anchor fix is needed.	1 month ago
Rene Cannao	f4313be6d9	doc(test): document per-test log paths and flaky-test debugging recipe ... Two gaps in test/README.md surfaced while investigating flaky TAP test failures on PR #5596: 1. The README said "Check logs in ci_infra_logs/${INFRA_ID}/tests/" but did not document the actual layout below that — per-test TAP output lives three directories deep at: ci_infra_logs/${INFRA_ID}/tests/proxysql-tester.py/tests/<test>-t.log.gz The files are gzipped, which makes `cat` and most editors do the wrong thing (binary garbage). A maintainer opening one with vim without knowing to use zless burns several minutes before figuring it out. 2. There was no guidance on how to reproduce a flaky test locally for stress-testing. The typical question "this test passed twice locally and fails on CI - is it really flaky, or is it my environment?" has no canonical answer anywhere in the repo. The natural recipe (reuse one infra, loop N runs, snapshot per-run logs, diff) is described in enough detail for a maintainer to copy/paste it without thinking. This commit adds: - A new "Where logs actually live after a run" section showing the full ci_infra_logs/ directory tree with per-backend, per-ProxySQL, and per-test subdirs. - Concrete zless/zcat/zgrep/zdiff one-liners for reading the gzipped per-test logs (including the cross-test grep idiom). - A new "Debugging a flaky test" section with a copy-pasteable loop that runs the same test 20 times against one infra, snapshotting per-attempt per-test logs for later diffing. - An expanded Troubleshooting bullet list: how to recover from lost INFRA_ID (via `docker network ls`), where each flavor of log lives, and how to clean up stale docker state. No code or workflow changes - doc-only update.	1 month ago
Rene Cannao	90c103a345	doc(GH-Actions): add §12.6 terminal flow — how to actually see what ran ... Follow-up to the vocabulary section added in ca4da932e. A maintainer hit GitHub's longstanding check-run UX papercut: clicking a row in the PR "Checks" tab lands on /runs/<check_id>, which shows only a status card and whose "View more details on GitHub Actions" link often points back at the same page because details_url on auto-created check runs is self-referential. The same dead-end is reached via the PR checks table's "Details" button (an earlier draft of this doc claimed otherwise — the claim was wrong and is corrected here). This commit inserts §12.6 "Seeing what actually ran — the terminal flow" between §12.5 (Common confusions) and what is now §12.7 (Sanity check, formerly §12.6). The new section walks the exact four-command terminal flow needed to go from a single row of `gh pr checks` output to the log lines of the failing step, using the real row from PR 5596 as the walked example: ✓ CI-trigger/CI-legacy-g1 / tests (mysql57) → gh run list -R sysown/proxysql --workflow CI-legacy-g1 → gh run view 24281031512 -R sysown/proxysql → gh run view --log-failed --job=70902846188 -R sysown/proxysql The section covers: - A character-by-character breakdown of the check row, identifying which fields are useful (workflow name) and which are dead ends (the /runs/<id> URL). - Why `gh run list`'s `branch:` column lies ("v3.0" rather than the PR's actual branch) because workflow_run-triggered runs inherit the default branch's metadata, not the triggering PR's. The documented gotcha in §10.2 cross-referenced. - How the run's display-title is the ONLY place where the PR head SHA appears, and the idiom to grep / `jq .displayTitle | contains(...)` against it. - How job name and check-run name differ ("run / tests (mysql57)" vs "CI-legacy-g1 / tests (mysql57)") because the prefixes come from different namespaces (caller job key vs workflow name). - Three flavors of log retrieval (--log-failed, --log | less, full-run --log) with concrete awk / grep idioms for the tab-separated log format. - A copy-pasteable condensed cheat sheet parameterised over PR number and workflow name. - A "why the web UI cannot do this" explanation stacking the three separate GitHub design issues: check-runs vs job-runs are different objects with no explicit link, details_url is self-referential, and workflow_run runs are tagged with the default branch's SHA not the PR's. Also: - §12.7 Sanity-check renumbered from §12.6, internal cross-refs in its "if you didn't get this, re-read X" pointer updated. - §12.7 gains a sixth self-test question covering the terminal flow. - No TOC change needed: the TOC lists only top-level sections (§12 as a whole), subsections are reached via the heading anchors. Doc-only change. 1547 → 1818 lines. Code fences balanced at 100.	1 month ago
Rene Cannao	ca4da932ef	doc(GH-Actions): add detailed GitHub Actions vocabulary section with diagrams ... The existing CI architecture doc assumes the reader already knows the GitHub Actions terminology and can parse check-run labels like 'CI-maketest / builds (testgalera)'. A maintainer asked what that label means and where to find it by grep, and the only answer was "re-read the whole doc and work it out yourself, most of the pieces are in different sections". That's not good enough. This commit adds a new §12 'Understanding GitHub Actions vocabulary' that: - Walks through the seven core terms (workflow, workflow run, job, matrix cell, step, check run, caller/reusable split) bottom-up, using ci-maketest.yml as a single concrete example throughout. - Includes an ASCII nesting diagram that shows how workflow -> run -> job -> matrix cell -> steps slot into each other, with check runs as separate objects attached to the commit SHA. - Includes a second ASCII diagram showing the ProxySQL two-branch caller/reusable split: one logical trigger -> two workflow runs, one on v3.0 and one on GH-Actions, sharing the same workflow name. - Includes a third diagram tracing exactly how the check-run label 'CI-maketest / builds (testgalera)' is assembled at runtime from github.workflow + github.job + env.MATRIX, explaining why the literal string exists nowhere on disk and grepping for it or for 'galera' will fail. - Answers five common confusion questions directly (why click-throughs land on GH-Actions, why two check rows per cell, where 'make testgalera' is defined, which file produced a given check row, CI-legacy-g2-genai vs CI-legacy-g2). - Ends with a five-question self-test so the reader can check they have the model right before closing the section. Also: - Added a 'New to GitHub Actions terminology?' pointer at the top of the doc so a confused reader jumps directly to the new section instead of wading through the two-branch architecture explanation first. - Renamed the existing compact Glossary table to 'Glossary (quick reference)' to make clear it is the lookup-by-term reference and the new narrative section is the learn-by-walkthrough explanation. - Both sections are kept: §12 for people who need the model, §13 for people who just need a word defined quickly. No source-code or workflow changes. Doc only.	1 month ago
René Cannaò	e3e769f372	Merge pull request #5604 from sysown/ci/disable-unittests-caller ... ci: disable CI-unittests caller temporarily	1 month ago
Rene Cannao	818481a62d	ci: disable CI-unittests caller temporarily (remove workflow_run trigger) ... CI-unittests cannot run in the normal CI cascade right now because the companion PR on GH-Actions (#5603) strips unit test binaries from the _test cache payload to keep it under GitHub's 10 GB per-repo quota. The unit tests are still COMPILED in ci-builds.yml (a sanity check enforces UNIT_COUNT > 0 before stripping, so compile errors in unit tests still fail the build) but their binaries don't ship in the cache, so CI-unittests would fail on "Cache restore test" or on the safety net in proxysql-tester.py that detects zero discovered test binaries. Rather than leave CI-unittests producing confusing red failures on every cascade, remove the workflow_run trigger so it stops firing automatically. The workflow_dispatch trigger is kept so it can still be invoked manually from the Actions tab (e.g. to test a re-enablement branch). This is intentionally the minimum surgical change: - No file deletion (preserves the caller for easy re-enable). - No change to the reusable on GH-Actions (ci-unittests.yml is dormant when no caller invokes it). - An inline comment block at the top of the caller explains the situation and lists the options for re-enabling (docker rebuild, expanded _src cache with incremental make, or artifact plumbing). Re-enabling CI-unittests is a separate follow-up effort. The goal for now is to unblock CI-legacy-g* and CI-mysql84-g* by getting the _test cache under quota; unit tests will come back later with a proper rebuild strategy. Companion PR: sysown/proxysql#5603	1 month ago
René Cannaò	ecd8b72dac	Merge pull request #5602 from sysown/ci/proxysql-tester-zero-test-safety-net ... test: fail proxysql-tester.py TAP runs when zero binaries are discovered (safety net)	2 months ago
Rene Cannao	8c2275178a	test: fail proxysql-tester.py TAP runs when zero binaries are discovered ... Defence-in-depth against the silent false-green failure mode we just tracked down. Previously, if CI-builds produced no TAP test binaries on the host (for any reason -- wrong make target, broken volume mount, typo in a docker-compose override, misconfigured groups.json), the tester would walk three workdirs, find zero `*-t` binaries in each, report: SUMMARY: 'tests' PASS 0/0 : FAIL 0/0 : SKIP 0/0 SUMMARY: 'deprecate_eof_support' PASS 0/0 : FAIL 0/0 : SKIP 0/0 SUMMARY: 'unit' PASS 0/0 : FAIL 0/0 : SKIP 0/0 SUMMARY: ret_rc = [0] ...and exit 0. Zero failures out of zero tests is technically "success" by the old logic, so the workflow would turn green. Combined with a build-target regression on the GH-Actions branch, this masked the fact that the entire TAP test suite (CI-legacy-g*, CI-mysql84-g*, CI-basictests, CI-taptests-pgsql-cluster, CI-legacy-clickhouse-g1, CI-legacy-g2-genai) had been running zero tests per commit for several weeks while still reporting green. Safety net ---------- At the end of `run_tap_tests`, check whether `glob(*-t)` found ANY binaries across ALL workdirs. The summary tuple (cmd, rc) is populated for every discovered binary, including ones that end up skipped by version filter or INCL/EXCL regexes -- those get `rc=None` rather than being absent. So `len(ret_summary) == 0` is a strict "nothing was even on disk" signal. When that happens, check groups.json for how many tests TAP_GROUP is expected to run. If N > 0, log a SAFETY NET FAIL line and bump ret_rc to 1. Version-filter skips and INCL/EXCL skips are not affected -- they still produce summary entries, so ret_summary is non-empty in those cases. Only the actual "test binaries were never built" case trips the guard. If TAP_GROUP is unset (running the full TAP suite), any zero-discovery result is a failure regardless -- there's no legitimate configuration where proxysql-tester should run zero tests. This complements the ci-builds.yml fix on the GH-Actions branch that restores the TAP build target. With both in place, a future regression in either the build or the test discovery chain surfaces loudly instead of silently.	2 months ago
René Cannaò	e0fded870d	Merge pull request #5598 from sysown/ci/rewire-group-callers-and-docs ... ci: rewire legacy-g{1,3,5} and mysql84-g{1..5} callers to dedicated reusables, add CI-unittests, document CI architecture	2 months ago
Rene Cannao	22e0ec2c91	docs(ci): document matrix.infradb gotcha; tag fenced blocks with text ... Addresses review feedback on #5598: 1. Adds `matrix.infradb` to the "What a sibling differs in" checklist. Gemini-code-assist flagged it -- and it was also a real bug in the merged ci-mysql84-g{1..5}.yml files, fixed in the separate #5600. The doc now explains that `infradb` is cosmetic for routing but displayed in the PR UI via env.MATRIX, so leaving it at the template default creates a misleading "CI-mysql84-g3 / tests (mysql57)" label. 2. Updates the sed example in "Adding a new test group" to substitute the infradb string alongside the group name, so copy-pasting from the doc produces a correct file on the first try. 3. Adds `text` language tag to seven previously untyped fenced code blocks (ASCII diagrams, directory listings, plain-text output) to satisfy markdownlint MD040. CodeRabbit flagged these. The repo doesn't enforce markdownlint, but the tag is harmless and silences the bot for future contributors editing the doc.	2 months ago
Rene Cannao	5aad576125	ci: rewire group callers to dedicated reusables; add CI-unittests; document CI architecture ... Rewiring -------- Following the landing of sysown/proxysql#5597 on the GH-Actions branch (which adds the nine dedicated reusable workflows), point each of the eight existing group callers at its dedicated reusable instead of the broken generic ci-taptests-groups.yml: CI-legacy-g1.yml → ci-legacy-g1.yml@GH-Actions CI-legacy-g3.yml → ci-legacy-g3.yml@GH-Actions CI-legacy-g5.yml → ci-legacy-g5.yml@GH-Actions CI-mysql84-g1.yml → ci-mysql84-g1.yml@GH-Actions CI-mysql84-g2.yml → ci-mysql84-g2.yml@GH-Actions CI-mysql84-g3.yml → ci-mysql84-g3.yml@GH-Actions CI-mysql84-g4.yml → ci-mysql84-g4.yml@GH-Actions CI-mysql84-g5.yml → ci-mysql84-g5.yml@GH-Actions Each edit also drops the now-useless `testgroup: '["<group>"]'` input that the new dedicated reusables do not accept (and the old generic reusable silently ignored). This is the other half of the fix started in #5597. Before this change, the eight group workflows were routed to ci-taptests-groups.yml, whose tests job strategy evaluates from a cached proxysql/tap-matrix.json that CI-builds has been populating as "[ ]". With an empty matrix vector GitHub Actions rejects the strategy with "Matrix vector 'testgroup' does not contain any values" and the tests job never instantiates -- visible for example in run 24240099115. With this PR merged (after #5597), these eight workflows will route to their own dedicated reusables and start running real tests on the next push. CI-unittests caller ------------------- Adds a new v3.0 caller CI-unittests.yml targeting ci-unittests.yml@GH-Actions (also added in #5597). This runs the 42 unit-tests-g1 binaries listed in test/tap/groups/groups.json directly on the runner, via the SKIP_PROXYSQL=1 host-only branch in run-tests-isolated.bash -- no Docker, no backends. Documentation ------------- Replaces doc/GH-Actions/README.md with an expanded, current CI architecture reference (~800 lines). Covers: * The two-branch caller/reusable split and the workflow_run default-branch rule that forces it * The CI-trigger / CI-builds babysitter pattern and why test workflows chain on CI-trigger completion (and not on CI-builds directly -- see 78b8f5ac6 for the incident that taught us why) * The dedicated-reusable pattern, with ci-legacy-g4.yml walked through step by step as the canonical template * Build cache layout (_bin / _src / _test / _matrix) * The TAP groups system: groups.json, BASE_GROUP stripping, infras.lst, env.sh, SKIP_PROXYSQL mode for unit tests * Full workflow catalogue as of 2026-04-11 with status per file * End-to-end walkthrough for adding a new test group * Common pitfalls: merge ordering, workflow_run head_sha propagation, the empty-matrix wedge, the LouisBrunner/checks-action false-positive permissions hotspot * Debugging guide: how to trace a failing run through CI-trigger → CI-builds → test workflow, and how to reproduce locally * Glossary Adds a Mermaid sequence diagram for the execution flow and inline ASCII diagrams throughout. Also updates CLAUDE.md to point agents at doc/GH-Actions/README.md before they touch anything under .github/workflows/.	2 months ago
René Cannaò	904807cf90	Merge pull request #5595 from sysown/fix/ci-workflow-run-chain-pr-sha ... ci: revert test workflows to listen on CI-trigger completion	2 months ago
René Cannaò	669ab91494	Merge pull request #5594 from sysown/v3.0-lint ... lint + static analysis: clang-tidy and cppcheck fixes	2 months ago
Rene Cannao	f1a78f81af	chore: remove superpowers specs/plans from PR	2 months ago
Rene Cannao	78b8f5ac62	ci: revert test workflows to listen on CI-trigger completion ... Commit 9671a414a3 changed 32 test workflows to trigger on CI-builds completion instead of CI-trigger completion. This broke PR CI runs: workflow_run-triggered workflows receive the head_sha of the workflow that triggered them, and CI-builds (itself workflow_run-triggered) runs on the default branch with head_sha pointing at v3.0 HEAD rather than the PR commit. As a result, all chained test workflows were checking out and testing v3.0 code instead of PR code. Revert to the original design where test workflows listen on CI-trigger (which is pull_request-triggered and carries the PR head_sha). CI-trigger already contains a babysitter step (gh run watch on CI-builds) that blocks until the build artifacts are ready, so gating on CI-trigger completion still waits for builds. Affects 32 workflow files — only the workflow_run.workflows entry is changed, no other logic touched.	2 months ago
Rene Cannao	76251a78dc	fix: resolve invalidFunctionArg in get_current_query - use size_t cast	2 months ago
Rene Cannao	24e56d4dd1	fix: SonarCloud issues - explicit ctors, sprintf->snprintf	2 months ago
Rene Cannao	a82224f1ef	fix: PR review feedback - double-free, end comments, dead code ... - Fix double-free bug in free_account_details() (lib/MySQL_Authentication.cpp) - Fix trailing #endif comments in 5 files to match guard names - Rename __SIZE -> SIZE_ in LDAP_USER_FIELD_IDX enum - Remove dead code: Query_Processor_Output ctor now calls init()	2 months ago
Rene Cannao	d5ffcba695	fix: cppcheck deeper analysis - invalidLifetime and arrayIndexOutOfBounds ... - Fix invalidLifetime: look up pointer from map after insertion, not before - Fix arrayIndexOutOfBounds: add bounds check before array access	2 months ago
Rene Cannao	bf9030ecb7	fix: remaining cppcheck printf and null pointer issues ... - Fix wrongPrintfScanfArgNum: remove extra args in 4 sprintf calls - Fix nullPointer: add GloMTH check, add null checks for strdup args - All high-risk cppcheck warnings now resolved	2 months ago
Rene Cannao	5ccb715e57	fix: cppcheck fixes - printf formats, null checks, copy operators ... - Fix invalidPrintfArgType: %d->%u for hid, %u->%d for use_ssl - Fix nullPointerRedundantCheck: replace assert with continue - Add deleted copy ctor/operator= to KillArgs and MySQL_Session - Fix invalidFunctionArg: add query_len > 3 check before memcpy	2 months ago
Rene Cannao	a51fe19e3f	fix: delete copy operations in 4 classes (Group_Replication_Info, Galera_Info, AWS_Aurora_Info, MySQL_Errors_stats)	2 months ago
Rene Cannao	18f83e9100	plan: add more cppcheck fix tasks (noCopyConstructor, printf, null pointer)	2 months ago
Rene Cannao	521fcf5ce7	fix: initialize all members in stmt_execute_metadata_t constructor	2 months ago