fix: worker thread can silently write to the real client db when a test only overrides .conn on the main thread - #56
Conversation
…real client db The conn.setter only ever bound the calling thread's connection; any other thread that later touched .conn opened its own connection against self._db_path, which defaulted to the real xdg client database. A test that injects an in-memory db and then exercises a worker thread had that thread silently fall back to writing the real file, no error raised. Add a db_path constructor field that overrides the computed xdg path for every thread, present and future, so tests (and any other caller wanting an isolated database) have one target every thread agrees on. Keep the .conn setter for existing single-threaded call sites, but document the hazard explicitly and point at db_path instead of presenting the setter as the sanctioned way to inject a test db.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Beep boop! Data processing finished. ⚙️I've aggregated the results of the automated checks for this PR below. 📊 CoverageThe coverage audit is ready for your inspection. 📋 ✅ 85.6% total coverage Per-file coverage (2 files)
Full report: download the 🔍 LintProcessing complete! Details follow. 📬 ❌ ruff: issues found — see job log 📋 Repo HealthChecking if the repo is following its diet. 🥗 ✅ All required files present. Latest Version: ✅ 🏷️ Release PreviewEnsuring the version bump is correctly calculated. 🔢 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)Our digital defenses have been updated. 🛡️ ✅ No known vulnerabilities found (70 packages scanned). 🔨 Build TestsFrom source to binary, let's see how it holds up. 🧱 ✅ All versions pass
⚖️ License CheckChecking if we're following open-source best practices. 📏 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. Every line of code matters. Thanks for contributing! 💖 |
…hreads A plain ":memory:" database belongs to the connection that opened it, so every thread opened a second, empty one. Schema creation runs once, on the thread that built the object, so a worker thread found no tables and counted zero clients. The test could not see it: "SELECT 1" answers just as well against an empty database. Map ":memory:" to a named shared cache database, one name per instance so two in-memory databases in the same process stay independent, and hold one connection open for the lifetime of the object because a shared in-memory database disappears with its last connection. Also create the parent directory for an explicit db_path again. It only happened on the xdg branch, so SQLiteDB(db_path="/var/lib/newdir/x.db") failed with "unable to open database file". The thread test now writes a client on one thread and reads it on another.
1 participant
Summary
conn.setter(hivemind_sqlite_database/__init__.py) only binds the calling thread's connection — the docstring said as much. A test that doesdb.conn = sqlite3.connect(":memory:")and then exercises a worker thread had that thread's.conngetter fall through to_connect(), which opensself._db_path— the real xdg client database (xdg_data_home()/hivemind-core/<name>.db) if the instance was constructed normally. No error, no warning, just a real file silently written to.Fix
Added a
db_pathconstructor field. When set (e.g.db_path=":memory:"), it overrides the computed xdg path entirely, so every thread — including ones that haven't opened a connection yet — agrees on the same, safe target. This closes the gap for future threads, which a setter-side guard could not: a guard at assignment time can only check threads that already hold a connection, not ones that connect later.Kept the
.connsetter (many existing single-threaded tests rely on it, e.g. viaobject.__new__(SQLiteDB)bypassing__post_init__entirely, where_db_pathis never even set), but rewrote its docstring to state the hazard plainly and point callers atdb_pathinstead of presenting the setter as the sanctioned way to inject a test database.Test plan
pytest tests -q -p no:ovoscope -p no:recording --timeout=600— 66 passed, 3 skippedtest_db_path_override_keeps_worker_threads_off_disk: injectsdb_path=":memory:", exercises a second thread, asserts the real db file under a mocked xdg home is never created