Make Database thread-safe via serialized connection access (#9)#28
Conversation
The library funnels all access through a single shared sqlite3* connection
behind a process-wide singleton, with no synchronization. Concurrent use
raced: each operation's BEGIN/COMMIT interleaved on the shared connection
("cannot start a transaction within a transaction"), the singleton lifecycle
was unguarded, and query error paths closed the shared connection out from
under other callers.
This serializes access (approach chosen for #9):
- Open the connection in serialized mode (sqlite3_open_v2 + SQLITE_OPEN_FULLMUTEX).
- Guard every operation that touches db_ with a per-instance mutex, so the
per-operation transaction is never interleaved. The lock in the
auto-increment path spans the insert and the last_insert_rowid read, so the
returned id always corresponds to this insert.
- Guard the singleton lifecycle (Initialize/Finalize/Instance) with a static
mutex, and make Instance() throw a clear error instead of dereferencing null
when the database is not initialized.
- Stop closing the shared connection from FetchRecordsQuery/FetchMaxIdQuery
error paths (this left a dangling handle and could double-close on Finalize).
Adds concurrency tests that hammer the shared instance from multiple threads
and assert all operations succeed, every row persists, and ids stay unique.
MSVC in C++11 mode rejected the lambdas (C3493) because the constexpr loop-bound locals were used without being captured; C++17+ does not require capturing them, so only the C++11 jobs failed. Capture by reference so the tests build under every standard in the matrix.
![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: 306f486291
ℹ️ 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".
| std::lock_guard<std::mutex> lock(instance_mutex_); | ||
| if (instance_ != nullptr) { | ||
| sqlite3_close(instance_->db_); |
There was a problem hiding this comment.
If one thread calls Finalize() while another is still using a Database reference returned by Instance(), this only takes instance_mutex_; the CRUD methods take db_mutex_ instead. That allows sqlite3_close(instance_->db_) and delete instance_ to run while an operation is inside ExecutionQuery::Execute(), leaving the worker with a freed sqlite3*/destroyed mutex. This is easy to hit when shutdown races a worker that has not joined yet, so Finalize() needs to coordinate with the same operation lock or otherwise wait for active users before closing/deleting the singleton.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 7c9c547. You're right that Finalize() only held instance_mutex_ while operations hold db_mutex_, so it could close/delete the singleton out from under an in-flight operation.
Rather than try to coordinate the two locks (the db_mutex_ lives inside the object being deleted, so "lock then delete" is itself UB, and a held reference couldn't be protected anyway), I moved the singleton to shared ownership:
instance_is now astd::shared_ptr<Database>, andInstance()returnsstd::shared_ptr<const Database>, so a caller holds a strong reference for the duration of its work.Finalize()just drops the singleton's own reference; the connection is closed by a new~Database(RAII) only once the last outstanding handle is released. An operation in progress on another thread keeps the database alive until it finishes.
Added a regression test, ConnectionOutlivesFinalizeWhileAHandleIsHeld, which holds a handle, calls Finalize(), and then keeps using the handle — this would have been a use-after-free under the old raw-pointer lifecycle. Call sites (tests, README) now use the handle via ->, and the README documents the threading contract. Full suite is 57/57 green locally.
Generated by Claude Code
…ht DB (#9) Previously Finalize() closed the connection and deleted the singleton while holding only instance_mutex_, so it could tear the Database down while another thread was mid-operation (operations hold db_mutex_, not instance_mutex_), leaving a freed sqlite3*/destroyed mutex — a use-after-free flagged in review. Move the singleton to shared ownership: - instance_ is now a std::shared_ptr<Database>, and Instance() returns std::shared_ptr<const Database>. Callers hold a strong reference for the duration of their work. - Finalize() just drops the singleton's own reference; the connection is closed by a new ~Database (RAII) once the last outstanding handle is released, so an in-flight operation keeps the database alive until it completes. - Update all call sites (tests, README) to use the handle via ->, and document the threading contract and handle semantics in the README. Adds a regression test (ConnectionOutlivesFinalizeWhileAHandleIsHeld) proving a held handle stays usable after a concurrent Finalize().
Make the shared-ownership contract concrete: explain that Instance() returns a shared_ptr handle that keeps the database alive, that Finalize() only drops the singleton's own handle (the connection closes when the last handle is released), and add a worker-thread example showing a handle captured by value. Also correct the Finalize() comment in the open/close section, which implied the connection is closed immediately.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ffbdf8fe3
ℹ️ 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".
| // Drop the singleton's own reference. The connection is closed by ~Database once the | ||
| // last outstanding Instance() handle is released, so an in-flight operation on another | ||
| // thread keeps the database alive until it finishes. | ||
| instance_.reset(); |
There was a problem hiding this comment.
With the new shared-handle lifecycle, this reset makes instance_ null even when older Instance() handles still keep the previous connection alive. A caller can then call Initialize() again before those handles are released, producing two live Database objects: with Initialize("") the new singleton is an empty in-memory DB while the old handle still has its rows, and with a file path the two connections bypass the single db_mutex_ and can write concurrently. Keep lifecycle state, such as a retired weak_ptr, or reject/retry initialization until outstanding handles have expired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — and it's a hole the shared-handle lifecycle introduced. Fixed in 119b0b8.
Finalize() now records the retiring instance in a static std::weak_ptr<Database> retired_, and Initialize() throws if that weak_ptr is still alive — i.e. while any outstanding Instance() handle keeps the previous database/connection alive. That prevents two live Database objects (the empty-in-memory-vs-old-rows surprise, and two file connections racing outside the shared db_mutex_). Once the last handle is released the previous instance is destroyed, the weak_ptr expires, and re-initialization succeeds again. Normal Initialize/Finalize cycles with no outstanding handles (e.g. the test fixtures) are unaffected.
Added a regression test ReinitializeWhileAHandleIsHeldIsRejected that holds a handle, finalizes, asserts Initialize() throws, then releases the handle and confirms re-initialization works. Full suite 58/58 green locally.
Generated by Claude Code
…ive (#9) With the shared-handle lifecycle, Finalize() nulls instance_ but an outstanding Instance() handle can keep the previous Database (and its connection) alive. A subsequent Initialize() would then create a second live Database whose operations use a different db_mutex_ — two connections to the same file racing, or a fresh empty in-memory DB shadowing the old one. Track the retiring instance with a weak_ptr and make Initialize() throw if it is still alive, so reinitialization is only allowed once all handles to the previous database have been released. Normal init/finalize cycles (no outstanding handles, e.g. the test fixtures) are unaffected. Adds a regression test covering reject-then-allow once the handle is released.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 119b0b845d
ℹ️ 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".
| // last outstanding Instance() handle is released, so an in-flight operation on another | ||
| // thread keeps the database alive until it finishes. Track the retiring instance so a | ||
| // subsequent Initialize() is rejected until those handles are gone. | ||
| retired_ = instance_; |
There was a problem hiding this comment.
When Finalize() is called a second time while a caller still holds an Instance() handle from the first finalized database, instance_ is already null, so this assignment overwrites the non-expired retired_ weak pointer with an empty one. A subsequent Initialize() will then pass the retired_.expired() check and create a second live database/connection while the old handle can still perform operations, reintroducing the split-lock/two-database state this guard is meant to prevent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, that was a real bug in the guard — fixed in b7a3fa9. Finalize() now returns early when instance_ is already null instead of doing retired_ = instance_, so a repeated Finalize() no longer clobbers a still-valid retired_ with an empty weak_ptr. The guard set by the first Finalize() stays in effect until the outstanding handles are released.
Added a regression test RepeatedFinalizeDoesNotClearTheReinitializationGuard (hold a handle, Finalize() twice, assert Initialize() still throws, then release and confirm re-init works). Full suite 59/59 green locally.
Generated by Claude Code
Finalize() unconditionally did `retired_ = instance_; instance_.reset();`. On a second Finalize() (instance_ already null) this overwrote a still-valid retired_ weak_ptr with an empty one, so a later Initialize() would pass the expired() check and create a second live database while outstanding handles from the first were still usable — reopening the split-lock/two-database hole the guard prevents. Make Finalize() a no-op when there is no instance to retire, preserving the existing retired_ tracking. Adds a regression test for repeated Finalize while a handle is held.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
Fixes #9. The library funnels all access through a single shared
sqlite3*connection behind a process-wide singleton with no synchronization, so concurrent use is undefined behavior. This serializes access to make concurrent use correct (approach 1 from the issue discussion).What changed
sqlite3_open_v2(..., SQLITE_OPEN_FULLMUTEX)instead ofsqlite3_open.db_mutex_guards every operation that touchesdb_(Fetch,Save,SaveAutoIncrement,Update,Delete,UnsafeSql,GetMaxId), so the per-operationBEGIN/COMMITcan no longer interleave on the shared connection (the "cannot start a transaction within a transaction" race). In the auto-increment path the lock spans both the insert and thesqlite3_last_insert_rowid()read, so the returned id always corresponds to this insert (this also closes the race deferred in Fix SaveAutoIncrement to write back a database-assigned, non-reused id (#6) #8).instance_mutex_guards the singleton lifecycle (Initialize/Finalize/Instance), andInstance()now throws a clear error instead of dereferencing null when the database is not initialized.sqlite3_close(db_)from theFetchRecordsQuery/FetchMaxIdQueryerror paths, which previously closed the shared connection on a prepare failure (dangling handle + potential double-close onFinalize).Tests (proving the fix)
Adds
tests/concurrency_test.cc— two tests that hammer the shared singleton from 8 threads:ConcurrentAutoIncrementInsertsAllSucceedWithUniqueIds— asserts no operation fails, all 800 rows persist, and all ids are unique.ConcurrentReadsAndWritesDoNotThrow— mixed readers/writers, asserts no crash/throw and the correct final count.These were verified to fail without the fix: with the connection lock removed, the first test produced only 143 of 800 unique ids (lost/colliding inserts from the nested-transaction race). With the fix, the full suite is 56/56 green.
Scope / follow-up
This serializes writes (and reads), which is correct but not concurrent for writers. True write parallelism (connection-per-thread / pool, WAL) is intentionally out of scope here and tracked as follow-up on #9.
https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
Generated by Claude Code