fix(admin): stop aborting the daemon when the TSDB flush hits a locked DB - #6004
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces fixed SQLite retry delays with capped exponential backoff. TSDB variable writes now use the retry-capable SQLite macro for persistent and runtime statements. ChangesSQLite retry and TSDB flush
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/sqlite3db.cpp (1)
305-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local constant to
UPPER_SNAKE_CASE.
max_backoff_usis a constant. The coding guidelines requireUPPER_SNAKE_CASEfor constants.♻️ Proposed rename
- const unsigned long max_backoff_us = 10000; + const unsigned long MAX_BACKOFF_US = 10000;Update the two uses at Lines 339-340.
As per coding guidelines: "Constants and macros must use
UPPER_SNAKE_CASE."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/sqlite3db.cpp` at line 305, Rename the local constant max_backoff_us to MAX_BACKOFF_US in its declaration and update both uses in the surrounding backoff logic, preserving the existing value and behavior.Source: Coding guidelines
lib/ProxySQL_Admin.cpp (1)
3176-3181: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ASSERT_SQLITE_OKnow aborts while a transaction is open.The transaction starts at Line 3154.
prepare_v2retriesSQLITE_LOCKEDandSQLITE_BUSYwithout a bound, so this path can still hang, andASSERT_SQLITE_OKstill aborts the daemon on any other error. The same applies to theASSERT_SQLITE_OKcalls on the bind and reset calls in the loop. Each abort leaves theBEGINuncommitted.The PR objective is to keep the daemon running when SQLite writes fail. Consider handling a non-
SQLITE_OKprepare result by rolling back and returning, which matches the failure handling used for the step results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ProxySQL_Admin.cpp` around lines 3176 - 3181, Replace the ASSERT_SQLITE_OK calls in the transaction around prepare_v2, bind, and reset operations with explicit return-code handling that rolls back the open transaction and returns on any non-SQLITE_OK result. Apply this to both query_a and the runtime-dependent query_b paths, matching the existing step-result failure handling and keeping the daemon running.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ProxySQL_Admin.cpp`:
- Around line 3154-3155: Update flush_tsdb_variables___runtime_to_database to
check the bool returned by BEGIN and avoid proceeding when transaction startup
fails, preserving any caller-owned transaction. Also capture and validate the
COMMIT result, log commit failures, and execute ROLLBACK when COMMIT fails so no
transaction remains open.
In `@lib/sqlite3db.cpp`:
- Around line 308-342: Update the retry loop in step_retry to stop retrying
SQLITE_BUSY or SQLITE_LOCKED when sqlite3_get_autocommit(db) == 0, matching the
existing guarded retry loops. Preserve the current retry and timeout behavior
for autocommit mode.
---
Nitpick comments:
In `@lib/ProxySQL_Admin.cpp`:
- Around line 3176-3181: Replace the ASSERT_SQLITE_OK calls in the transaction
around prepare_v2, bind, and reset operations with explicit return-code handling
that rolls back the open transaction and returns on any non-SQLITE_OK result.
Apply this to both query_a and the runtime-dependent query_b paths, matching the
existing step-result failure handling and keeping the daemon running.
In `@lib/sqlite3db.cpp`:
- Line 305: Rename the local constant max_backoff_us to MAX_BACKOFF_US in its
declaration and update both uses in the surrounding backoff logic, preserving
the existing value and behavior.
🪄 Autofix
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 Plus
Run ID: b239f4d3-def3-47d0-b355-804456b2c0fa
📒 Files selected for processing (3)
include/sqlite3db.hlib/ProxySQL_Admin.cpplib/sqlite3db.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/sqlite3db.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/sqlite3db.hlib/sqlite3db.cpplib/ProxySQL_Admin.cpp
🔇 Additional comments (2)
include/sqlite3db.h (1)
241-261: LGTM!lib/ProxySQL_Admin.cpp (1)
3183-3187: LGTM!Also applies to: 3190-3249
| db->execute("BEGIN"); | ||
| bool flush_ok = true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Check the result of BEGIN, and of COMMIT at Line 3242.
SQLite3DB::execute() returns bool, and this code discards it. Two failure modes follow:
- If the connection already has an open transaction,
BEGINfails with "cannot start a transaction within a transaction". TheCOMMITat Line 3242 then commits the caller's outer transaction, and theROLLBACKat Line 3247 discards the caller's outer work. That converts a local flush failure into data loss for an unrelated write. - If
COMMITfails withSQLITE_BUSY, the transaction stays open on the connection after the function returns. The next call to this function then hits the nested-BEGINcase above.
Capture the return value of BEGIN, and skip the transaction wrapper or return early when it fails. Log a failed COMMIT and roll back.
#!/bin/bash
# Check whether any caller of the flush already holds an open transaction.
rg -nP -C 8 'flush_tsdb_variables___runtime_to_database\s*\(' --type=cpp
# Inspect how other flush functions in the same file manage BEGIN/COMMIT.
rg -nP -C 3 'execute\("(BEGIN|COMMIT|ROLLBACK)"\)' --type=cpp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ProxySQL_Admin.cpp` around lines 3154 - 3155, Update
flush_tsdb_variables___runtime_to_database to check the bool returned by BEGIN
and avoid proceeding when transaction startup fails, preserving any caller-owned
transaction. Also capture and validate the COMMIT result, log commit failures,
and execute ROLLBACK when COMMIT fails so no transaction remains open.
| while (true) { | ||
| rc = (*proxy_sqlite3_step)(stmt); | ||
| if (rc != SQLITE_LOCKED && rc != SQLITE_BUSY) { | ||
| break; | ||
| } | ||
| if (waited_us >= max_wait_us) { | ||
| proxy_error( | ||
| "SQLite3 database still locked after %lums, giving up rc=%d msg='%s' db='%s'\n", | ||
| waited_us / 1000, rc, (*proxy_sqlite3_errmsg)(db), (url ? url : "(null)")); | ||
| break; | ||
| } | ||
| // Warn once, halfway through the budget. A wait this long is not | ||
| // ordinary contention and should be visible even when the retry | ||
| // ultimately succeeds and the caller reports nothing. | ||
| if (warned == false && waited_us >= max_wait_us / 2) { | ||
| proxy_warning( | ||
| "SQLite3 database locked for %lums, still retrying rc=%d db='%s'\n", | ||
| waited_us / 1000, rc, (url ? url : "(null)")); | ||
| warned = true; | ||
| } | ||
| unsigned long sleep_us = backoff_us; | ||
| if (waited_us + sleep_us > max_wait_us) { | ||
| sleep_us = max_wait_us - waited_us; | ||
| } | ||
| struct timespec ts { | ||
| .tv_sec = static_cast<time_t>(sleep_us / 1000000), | ||
| .tv_nsec = static_cast<long>((sleep_us % 1000000) * 1000) | ||
| }; | ||
| nanosleep(&ts, nullptr); | ||
| waited_us += sleep_us; | ||
| backoff_us *= 2; | ||
| if (backoff_us > max_backoff_us) { | ||
| backoff_us = max_backoff_us; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retrying SQLITE_BUSY inside an explicit transaction can block for the full budget on every statement.
The other step loops in this file check sqlite3_get_autocommit(db) and stop retrying when an explicit transaction is open (Lines 412-418 and Lines 540-546). step_retry omits that check. The only caller runs inside BEGIN/COMMIT in lib/ProxySQL_Admin.cpp. If the write lock is held by another connection, SQLite does not upgrade the lock for a statement inside an open transaction, so the retry cannot succeed. The flush then waits the full max_wait_ms for the first failing statement, and the flush blocks the calling thread for 10 seconds before it rolls back.
Consider breaking out early when sqlite3_get_autocommit(db) == 0, or document that the caller accepts the full budget per statement.
🛠️ Suggested early exit
rc = (*proxy_sqlite3_step)(stmt);
if (rc != SQLITE_LOCKED && rc != SQLITE_BUSY) {
break;
}
+ if ((*proxy_sqlite3_get_autocommit)(db) == 0) {
+ // Inside an explicit transaction the lock cannot be acquired by
+ // retrying the same statement; report the busy status instead.
+ break;
+ }#!/bin/bash
# Find all callers of step_retry and check whether they run inside an explicit transaction.
rg -nP -C 10 '\bstep_retry\s*\(' --type=cpp --type=c
# Show existing autocommit-guarded retry loops for comparison.
rg -nP -C 4 'proxy_sqlite3_get_autocommit' --type=cpp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/sqlite3db.cpp` around lines 308 - 342, Update the retry loop in
step_retry to stop retrying SQLITE_BUSY or SQLITE_LOCKED when
sqlite3_get_autocommit(db) == 0, matching the existing guarded retry loops.
Preserve the current retry and timeout behavior for autocommit mode.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8da0f4298a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // statement -- both cheaper (one fsync rather than one per variable) and a | ||
| // far smaller window in which to collide with another writer. Other | ||
| // modules do read and write these databases concurrently. | ||
| db->execute("BEGIN"); |
There was a problem hiding this comment.
If this call overlaps another admin path that has already opened a transaction on the shared admindb handle (for example, dump_checksums_values_table()), SQLite rejects this nested BEGIN, but the false return from execute() is ignored. The TSDB statements then run inside the pre-existing transaction, and this function subsequently commits or rolls back that other path's work. Hold the database write lock for the entire transaction and stop if BEGIN fails.
Useful? React with 👍 / 👎.
| if (flush_ok) { | ||
| db->execute("COMMIT"); |
There was a problem hiding this comment.
When flushing configdb in SQLite's default rollback-journal mode, an existing reader can allow the writes to complete but make COMMIT return SQLITE_BUSY. This calls SQLite3DB::execute(), which retries BUSY/LOCKED forever, so the newly advertised 10-second bound is bypassed and startup can remain stuck indefinitely. Commit through a bounded operation and handle failure by rolling back.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6004 +/- ##
===========================================
+ Coverage 13.87% 53.07% +39.20%
===========================================
Files 154 478 +324
Lines 82411 143730 +61319
Branches 0 36343 +36343
===========================================
+ Hits 11431 76280 +64849
+ Misses 70980 50546 -20434
- Partials 0 16904 +16904
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…d DB
CI-mysql90-gr-g1 died during startup:
ProxySQL_Admin.cpp:3187:flush_tsdb_variables___runtime_to_database():
[ERROR] SQLite3 error. Shutting down rc=5 msg='database is locked'
proxysql: Assertion `0' failed.
ProxySQL_Admin::flush_tsdb_variables___runtime_to_database()
ProxySQL_Admin::init_tsdb_variables()
ProxySQL_Main_init_phase3___start_all()
rc=5 is SQLITE_BUSY. This is not a crash: ASSERT_SQLITE_OK() treats any
non-SQLITE_OK return as fatal and calls assert(0). A locked database is a
runtime condition -- other modules read and write these databases
concurrently -- not a programming error, so aborting the daemon on it is
wrong.
flush_tsdb_variables___runtime_to_database() was the ONLY variables flush
that stepped its statements raw and asserted on the result. The codebase
idiom is SAFE_SQLITE3_STEP2(), used 59 times across ProxySQL_Admin.cpp and
Admin_FlushVariables.cpp; the structurally identical sibling
flush_pgsql_variables___runtime_to_database() uses it. TSDB is newer code
that missed the convention. Use it here too -- that is the whole fix.
NDEBUG is never defined by any Makefile, so this assert was live in release
builds too: it aborted production daemons, not just CI.
Also gives SAFE_SQLITE3_STEP2() exponential backoff, from the same 100us it
already used up to a 10ms cap. The fixed 100us sleep meant a lock held for
any noticeable time was waited out at ~10k wakeups/sec. This benefits all
59 call sites, not just this one.
Deliberately NOT done, after review: an earlier version of this patch added
a bounded step_retry() helper and wrapped the flush in BEGIN/COMMIT. Both
were dropped. The transaction put the retry inside an explicit transaction,
where SQLite will not upgrade the lock, so retrying could not succeed --
the two halves worked against each other. It also left the BEGIN and COMMIT
return values unchecked, and COMMIT went through execute(), which retries
BUSY forever and so bypassed the very bound the helper advertised. Matching
the existing idiom avoids all of it.
8da0f42 to
0390416
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/sqlite3db.h`:
- Line 25: Update the _backoff_us retry backoff logic so doubling never assigns
a value above the 10,000-microsecond cap; clamp the doubled result to 10,000
before storing it, while preserving the existing growth behavior below the cap.
🪄 Autofix
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 Plus
Run ID: 71c170b7-d3bd-4101-8c33-f2d44ec5fa67
📒 Files selected for processing (2)
include/sqlite3db.hlib/ProxySQL_Admin.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ProxySQL_Admin.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/sqlite3db.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/sqlite3db.h
Review catch. The guard tested the value BEFORE doubling:
if (_backoff_us < 10000) { _backoff_us *= 2; }
so 6400 (which is < 10000) doubled to 12800 and stuck there, giving a real
ceiling of 12.8ms against a comment promising 10ms. Sequence was
100..3200, 6400, 12800, 12800; it is now 100..3200, 6400, 10000, 10000.
Harmless in effect, but the code should do what the comment next to it
says.
|
1 participant
The failure
CI-mysql90-gr-g1died during startup on an unrelated PR (#5998):rc=5isSQLITE_BUSY. This is not a crash —ASSERT_SQLITE_OK()treats anynon-
SQLITE_OKreturn as fatal and callsassert(0). A locked database is aruntime condition (other modules read and write these databases concurrently),
not a programming error, so aborting the daemon on it is wrong by construction.
Intermittent, as expected of a lock race: the previous run of this same workflow
on the same branch passed.
Root cause
flush_tsdb_variables___runtime_to_database()was the only variables flushthat stepped its statements raw and asserted on the result. The codebase idiom is
SAFE_SQLITE3_STEP2()— 59 uses acrossProxySQL_Admin.cppandAdmin_FlushVariables.cpp— and the structurally identical siblingflush_pgsql_variables___runtime_to_database()uses it. TSDB is newer code thatmissed the convention.
It was also the only flush with no
BEGIN/COMMIT, so it took and releasedthe write lock once per variable rather than once for the whole flush. That made
it simultaneously the flush most likely to hit
SQLITE_BUSYand the only oneunable to survive it.
NDEBUGis never defined by any Makefile, so thisassertis live in releasebuilds too — it aborted production daemons, not just CI.
Changes
1. New
SQLite3DB::step_retry()(sqlite3db.{h,cpp})Rather than reuse
SAFE_SQLITE3_STEP2(), which retries forever at a fixed 100uswith no upper bound and no logging — trading a loud abort for a silent spin at
~10k wakeups/sec — this:
rcso the caller decides, instead of hanging or asserting2.
flush_tsdb_variables___runtime_to_database()uses it, and wraps the wholeflush —
DELETEs included — in a single transaction:BEGINis placed before theDELETEs so delete-then-reinsert is atomic anda mid-flush failure cannot leave
tsdb-%rows deleted but not repopulateddaemon keeps running
GloProxyStats == NULLearly return rolls back rather than stranding anopen transaction
error) so an active statement cannot hold up the
ROLLBACKNeither of the other two callers (
GenericRefreshStatistics,flush_tsdb_variables___database_to_runtime) holds an open transaction, so theBEGINdoes not nest.Verification
Built
PROXYSQL31=1(TSDB enabled) and ran the daemon locally. All three callpaths exercised, with no assertion, rollback, or failed flush in the log:
init_tsdb_variables(),runtime=falseglobal_variablesGenericRefreshStatistics(),runtime=trueruntime_global_variablesflush_tsdb_variables___database_to_runtime()LOAD TSDB VARIABLES TO RUNTIME/SAVE TSDB VARIABLES TO DISKThe
runtime_global_variablesrows are the meaningful check for the secondstatement:
init_tsdb_variables()only calls withruntime=false, so those rowscan only come from the
runtime=truepath committing.Scope
Unrelated to the auth work on #5998, where the abort was merely first observed.
A sweep of the remaining
ASSERT_SQLITE_OKcall sites that can see contention isworth doing separately — this PR fixes the one site that had a raw step feeding it.
Summary by CodeRabbit