fix: prevent SQLITE_BUSY errors from concurrent flush loops#102
fix: prevent SQLITE_BUSY errors from concurrent flush loops#102SantiagoDePolonia merged 2 commits intomainfrom
Conversation
Serialize all SQLite access through a single connection (MaxOpenConns=1) so database/sql queues concurrent callers in Go instead of having audit log and usage tracking flush loops compete for SQLite's write lock. Also set busy_timeout pragma explicitly as defense-in-depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes modify SQLite connection pooling to use a single-connection model (max open/idle connections = 1) and add a 5-second busy timeout pragma to mitigate SQLITE_BUSY contention. A new concurrent write safety test validates data integrity under parallel goroutine inserts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/sqlite.go (1)
17-19:⚠️ Potential issue | 🟡 MinorUpdate the
NewSQLitedoc comment to document the single-connection serialization behavior.The doc comment still only describes WAL mode and omits the behavioral change introduced by this PR. Per coding guidelines, doc comments must be updated when changing function behavior.
📝 Proposed doc comment update
// NewSQLite creates a new SQLite storage connection. -// It enables WAL mode for better concurrent read/write performance. +// It enables WAL mode and serializes all access through a single connection +// (MaxOpenConns=1) so that database/sql queues concurrent callers in Go, +// eliminating SQLITE_BUSY contention between concurrent flush loops. func NewSQLite(cfg SQLiteConfig) (Storage, error) {As per coding guidelines: "Maintain Go doc comments on all exported types, functions, and interfaces; update when changing public APIs or function signatures/behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/sqlite.go` around lines 17 - 19, Update the doc comment for NewSQLite to describe both that it enables WAL mode and that the returned Storage uses a single serialized connection (i.e., operations are serialized over one DB connection) so callers understand the changed concurrency semantics; edit the comment above func NewSQLite to mention the single-connection serialization behavior and any implications for concurrent callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/storage/sqlite_test.go`:
- Line 12: Replace the hard-coded path construction in the test by using
filepath.Join: when creating the SQLiteConfig for NewSQLite (the expression
currently using t.TempDir() + "/test.db"), build the path with
filepath.Join(t.TempDir(), "test.db") and import the "path/filepath" package so
the test is cross-platform; update the NewSQLite call to use this joined path.
- Line 34: The buffered error channel errs is oversized (errs := make(chan
error, goroutines*insertsPerGoroutine*2) — max possible errors is
goroutines*insertsPerGoroutine), so either remove the unnecessary *2 to make it
errs := make(chan error, goroutines*insertsPerGoroutine) or keep the *2 but add
a short comment next to the errs declaration explaining why the extra buffer is
required; update the line where errs is created and/or add the explanatory
comment referencing errs, goroutines and insertsPerGoroutine.
In `@internal/storage/sqlite.go`:
- Around line 50-55: Replace the per-connection PRAGMA application with a
connection hook: call sqlite.RegisterConnectionHook before sql.Open (e.g., in an
init function) and register a callback that executes "PRAGMA busy_timeout =
5000" via conn.ExecContext so every new pooled connection gets the setting; then
you can remove or keep the existing db.Exec("PRAGMA busy_timeout = 5000") after
Ping in Open/Connect routines (it becomes redundant), but ensure you import the
sqlite package so RegisterConnectionHook is available.
---
Outside diff comments:
In `@internal/storage/sqlite.go`:
- Around line 17-19: Update the doc comment for NewSQLite to describe both that
it enables WAL mode and that the returned Storage uses a single serialized
connection (i.e., operations are serialized over one DB connection) so callers
understand the changed concurrency semantics; edit the comment above func
NewSQLite to mention the single-connection serialization behavior and any
implications for concurrent callers.
| const insertsPerGoroutine = 50 | ||
|
|
||
| var wg sync.WaitGroup | ||
| errs := make(chan error, goroutines*insertsPerGoroutine*2) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Error channel buffer is 2× the maximum possible error count.
goroutines*insertsPerGoroutine*2 = 1000, but the maximum number of errors is goroutines*insertsPerGoroutine = 500. The extra factor of 2 is harmless but unexplained; a brief comment or dropping the *2 would improve clarity.
♻️ Proposed fix
- errs := make(chan error, goroutines*insertsPerGoroutine*2)
+ errs := make(chan error, goroutines*insertsPerGoroutine)📝 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.
| errs := make(chan error, goroutines*insertsPerGoroutine*2) | |
| errs := make(chan error, goroutines*insertsPerGoroutine) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/sqlite_test.go` at line 34, The buffered error channel errs
is oversized (errs := make(chan error, goroutines*insertsPerGoroutine*2) — max
possible errors is goroutines*insertsPerGoroutine), so either remove the
unnecessary *2 to make it errs := make(chan error,
goroutines*insertsPerGoroutine) or keep the *2 but add a short comment next to
the errs declaration explaining why the extra buffer is required; update the
line where errs is created and/or add the explanatory comment referencing errs,
goroutines and insertsPerGoroutine.
| // Set busy_timeout explicitly as defense-in-depth. The DSN parameter may not be | ||
| // honored reliably by the pure-Go driver across pooled connections. | ||
| if _, err := db.Exec("PRAGMA busy_timeout = 5000"); err != nil { | ||
| db.Close() | ||
| return nil, fmt.Errorf("failed to set busy_timeout pragma: %w", err) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, examine the full context of the sqlite.go file around lines 50-55
cd /repo && wc -l internal/storage/sqlite.go 2>/dev/null || echo "File not found, searching for sqlite.go"Repository: ENTERPILOT/GOModel
Length of output: 174
🏁 Script executed:
# Search for the sqlite.go file
find . -name "sqlite.go" -type f 2>/dev/null | head -5Repository: ENTERPILOT/GOModel
Length of output: 90
🏁 Script executed:
# Once found, read the file context
cat -n internal/storage/sqlite.go | head -100Repository: ENTERPILOT/GOModel
Length of output: 2720
🏁 Script executed:
# Search for MaxOpenConns and connection configuration settings
rg "MaxOpenConns|ConnMaxLifetime|ConnMaxIdleTime" internal/storage/ -n -A 2 -B 2Repository: ENTERPILOT/GOModel
Length of output: 427
🏁 Script executed:
# Search for RegisterConnectionHook in the codebase to see if it's used
rg "RegisterConnectionHook" . -n -A 3 -B 1Repository: ENTERPILOT/GOModel
Length of output: 44
🌐 Web query:
modernc.org/sqlite RegisterConnectionHook API documentation and usage
💡 Result:
modernc.org/sqlite — RegisterConnectionHook API
Signature
- Hook type:
type ConnectionHookFn func(conn ExecQuerierContext, dsn string) error(pkg.go.dev) - Register globally:
func RegisterConnectionHook(fn ConnectionHookFn)(pkg.go.dev) - Register on a specific driver instance:
func (d *Driver) RegisterConnectionHook(fn ConnectionHookFn)(pkg.go.dev)
When it runs
The hook is called after each connection is opened and fully set up (i.e., per physical connection in database/sql’s pool, not per sql.Open). (pkg.go.dev)
What you can do in the hook
conn is an ExecQuerierContext (it supports driver.ExecerContext and driver.QueryerContext), so you can run per-connection initialization SQL (PRAGMAs, temp tables, etc.). (pkg.go.dev)
dsn is the data source name used to open that connection. (pkg.go.dev)
Usage
A) Global registration (affects the default "sqlite" driver)
Call this before opening your DB.
package main
import (
"context"
"database/sql"
"modernc.org/sqlite"
_ "modernc.org/sqlite" // registers driver name "sqlite"
)
func init() {
sqlite.RegisterConnectionHook(func(conn sqlite.ExecQuerierContext, dsn string) error {
// Example: enforce foreign keys on every pooled connection
_, err := conn.ExecContext(context.Background(), "PRAGMA foreign_keys=ON", nil)
return err
})
}
func main() {
db, _ := sql.Open("sqlite", "file:test.db")
_ = db
}B) Driver-instance registration (lets you choose a separate driver name)
d := &sqlite.Driver{}
d.RegisterConnectionHook(myHook)
sql.Register("sqlite+hook", d)
db, err := sql.Open("sqlite+hook", "file:test.db")
_ = db
_ = err(Behavior and signatures as documented on pkg.go.dev.) (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/modernc.org/sqlite
- 2: https://pkg.go.dev/modernc.org/sqlite
- 3: https://pkg.go.dev/modernc.org/sqlite
- 4: https://pkg.go.dev/modernc.org/sqlite
- 5: https://pkg.go.dev/modernc.org/sqlite
- 6: https://pkg.go.dev/modernc.org/sqlite
- 7: https://pkg.go.dev/modernc.org/sqlite
Use sqlite.RegisterConnectionHook for consistent PRAGMA application across connection lifecycle.
The PRAGMA busy_timeout is per-connection. While the current code applies it explicitly after Ping and the DSN includes _busy_timeout=5000 as a fallback, this covers only the initial connection. If database/sql recycles the underlying connection (e.g., after a driver error), neither the explicit PRAGMA nor the DSN parameter will be re-applied to the new connection.
The idiomatic solution for modernc.org/sqlite is sqlite.RegisterConnectionHook, which runs a callback on every new pooled connection:
func init() {
sqlite.RegisterConnectionHook(func(conn sqlite.ExecQuerierContext, _ string) error {
_, err := conn.ExecContext(context.Background(), "PRAGMA busy_timeout = 5000", nil)
return err
})
}Call this before sql.Open() to ensure the PRAGMA is applied consistently, making the explicit db.Exec after Ping redundant but harmless.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/storage/sqlite.go` around lines 50 - 55, Replace the per-connection
PRAGMA application with a connection hook: call sqlite.RegisterConnectionHook
before sql.Open (e.g., in an init function) and register a callback that
executes "PRAGMA busy_timeout = 5000" via conn.ExecContext so every new pooled
connection gets the setting; then you can remove or keep the existing
db.Exec("PRAGMA busy_timeout = 5000") after Ping in Open/Connect routines (it
becomes redundant), but ensure you import the sqlite package so
RegisterConnectionHook is available.
… test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Serialize all SQLite access through a single connection (MaxOpenConns=1) so database/sql queues concurrent callers in Go instead of having audit log and usage tracking flush loops compete for SQLite's write lock. Also set busy_timeout pragma explicitly as defense-in-depth.
Summary by CodeRabbit
Bug Fixes
Tests