Repository navigation
fix: give each thread its own SQLite connection - #54
Conversation
The plugin shared one sqlite3.Connection between all callers. A connection owns the transaction state, so when two threads use it at the same time one thread's COMMIT ends the other thread's transaction and resets its in-flight statements. Reads were not guarded at all, and search_by_value wrapped its SELECT in `with self.conn:`, which commits on exit - a read committing somebody else's write. Under the Tornado network backend everything ran on one IOLoop thread, so this never showed. Under a threaded backend it shows immediately: a 50-client load test logged 98 x "bad parameter or other API misuse" from search_by_value and 11 x "NOT NULL constraint failed: clients.api_key" from add_item, and valid clients were refused because their api_key lookup returned nothing. Each thread now opens its own connection on first use. WAL already allows many readers beside one writer, and _write_lock still serialises writers in-process, so the connections do not fight over the file lock; busy_timeout covers any that do. The SELECT no longer opens a transaction. The new concurrency test fails on the previous code and passes on this one.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSQLiteDB now uses lazy, thread-local SQLite or SQLCipher connections with WAL mode and a configurable busy timeout. It validates encryption passwords, supports per-thread connection injection and closing, avoids implicit read commits, and adds concurrent read/write coverage. ChangesSQLite concurrency support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReaderThreads
participant WriterThreads
participant SQLiteDB
participant SQLiteDatabase
ReaderThreads->>SQLiteDB: Request thread-local connection
SQLiteDB->>SQLiteDatabase: Open connection with WAL and busy timeout
SQLiteDB-->>ReaderThreads: Return connection
WriterThreads->>SQLiteDB: Upsert API-key data
SQLiteDB->>SQLiteDatabase: Write data
ReaderThreads->>SQLiteDB: Search by value
SQLiteDB->>SQLiteDatabase: Execute read query
SQLiteDatabase-->>ReaderThreads: Return results
Possibly related PRs
✨ 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 |
Look what I found! The automated check results are in. 🔍I've aggregated the results of the automated checks for this PR below. 📊 CoverageA bird's eye view of your test coverage landscape. 🦅 ✅ 84.8% total coverage Per-file coverage (2 files)
Full report: download the 🔒 Security (pip-audit)Ensuring our security logs are being captured. 🪵 ✅ No known vulnerabilities found (70 packages scanned). ⚖️ License CheckEnsuring our CLA requirements are met. 🖋️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📋 Repo HealthScanning for any signs of 'copy-paste' obesity. 🍕 ✅ All required files present. Latest Version: ✅ 🔨 Build TestsEnsuring no loose screws in the assembly. 🔩 ✅ All versions pass
Delivered by the OVOS Automated Messenger 🕊️ |
The plugin shared one
sqlite3.Connectionbetween all callers. A connection owns the transaction state, so when two threads use it at the same time one threadsCOMMITends the other threads transaction and resets its in-flight statements. Reads were not guarded at all, andsearch_by_valuewrapped itsSELECTinwith self.conn:, which commits on exit — a read committing somebody elses write.How it was found
Under the Tornado network backend every admission and every inbound frame ran on one IOLoop thread, so this never showed. A threaded backend (webrockets, JarbasHiveMind/hivemind-websocket-protocol#43) dispatches admission and receive from several Rust worker threads at once. Measured on a 50-client load test against the previous code:
search_by_value:bad parameter or other API misuseadd_item:NOT NULL constraint failed: clients.api_keyClient provided an invalid api key(valid keys)Same rig, same N, Tornado backend: 0 of each. Client names in the rejection logs came out mangled (
lt93::94::::), which is a corrupted row read, not a client-side problem.The fix
Each thread opens its own connection on first use (
threading.local). WAL already allows many readers beside one writer, and_write_lockstill serialises writers in-process so the connections do not fight over the file lock;busy_timeoutcovers any that do.search_by_valueno longer opens a transaction around itsSELECT.connbecomes a property. The setter adopts a connection for the calling thread only, which keeps the existing test helpers working.Tests
TestSQLiteDBThreadSafety.test_concurrent_read_write_is_cleanruns 16 readers and 8 writers against a file-backed DB. It fails on the previous code (IndexError: tuple index out of rangeand friends) and passes on this one. Full suite: 63 passed, 3 skipped.Verification in the load rig
With this branch on
PYTHONPATH, the webrockets rig at N=50 logs 0 SQLite errors, down from 98/100 across two pre-fix repetitions.Summary by CodeRabbit