fix(xmtp_db): make TestInstrumentation database-lock panic opt-in

[xmtp-coder-agent]

Resolves #3765

Problem

TestInstrumentation (crates/xmtp_db/src/encrypted_store/database/instrumentation.rs) called panic!("database locked") on any query that failed with database is locked. This instrumentation is installed on every SQLite connection whenever the test cfg or the test-utils feature is active — and the node-sdk Vitest suite (bindings_node) enables test-utils.

Under concurrent test load (CI node-sdk shard 2), Vitest fork workers transiently contend on SQLite locks. Each database is locked error panicked on an r2d2-worker/tokio-rt-worker thread, aborting the worker process and surfacing as flaky Worker exited unexpectedly failures with a non-zero Vitest exit code.

database is locked is not fatal in this codebase: SQLite is configured with PRAGMA busy_timeout = 5000 and the error is explicitly classified as retryable (PlatformStorageError::DatabaseLocked => true; ClientError retries on "database is locked"). The panic short-circuited that designed retry/backoff machinery, turning a recoverable transient error into a hard crash.

Fix

Gate the panic behind an opt-in XMTP_PANIC_ON_DB_LOCK environment variable (accepts 1/true, matching the existing SQLCIPHER_LOG convention):

• Default (unset): log the query error at error level and return normally — the existing busy_timeout/retry machinery recovers. No worker crashes.
• XMTP_PANIC_ON_DB_LOCK=1: preserve the legacy panic for developers explicitly debugging lock contention.

The env read is split from a pure decision helper (panic_on_db_lock_from_env) so the opt-in logic is unit-tested without mutating the process environment (Rust 2024 set_var is unsafe/racy).

Unchanged behavior

• Query-error logging (message/table/column/hint) is preserved.
• DatabaseLocked remains classified as retryable.
• Begin/Commit/Rollback transaction tracing is unchanged.

Testing

• New unit tests in instrumentation.rs cover: panic disabled by default (unset env), opt-in values (1/true), and that other values (0, false, empty, TRUE, etc.) do not enable it.
• cargo test -p xmtp_db --lib instrumentation → 3 passed.
• cargo clippy -p xmtp_db --all-features --tests → clean.
• cargo fmt -p xmtp_db --check → clean.

🤖 Generated with Claude Code

Note

Make TestInstrumentation database-lock panic opt-in via XMTP_PANIC_ON_DB_LOCK env var

• Panicking on SQLite "database is locked" errors in TestInstrumentation now requires XMTP_PANIC_ON_DB_LOCK=1 or XMTP_PANIC_ON_DB_LOCK=true; previously any occurrence would panic unconditionally.
• Adds panic_on_db_lock_enabled and panic_on_db_lock_from_env helpers in instrumentation.rs to read and evaluate the env var.
• Sets XMTP_PANIC_ON_DB_LOCK=true in the justfile test recipes and the nextest.nix package so CI and local test runs still opt in.
• Fixes a flaky test in stream_messages.rs by filtering the message stream to application messages only, avoiding races with membership-change commits.
• Behavioral Change: the default behavior no longer panics on lock contention; environments outside the configured test runners must explicitly set the env var to preserve the old behavior.

^{Macroscope summarized 36e833a.}

[xmtp-coder-agent]

CI fix: deflaked test_stream_messages

The Rust Workspace CI run surfaced a failure in subscriptions::stream_messages::tests::test_stream_messages — pre-existing flakiness unrelated to the instrumentation change (this PR's only other diff is in xmtp_db).

Root cause: the test opened a message stream after add_members(bob) and asserted the first streamed message equaled "hello". The group_updated membership-change commit can race into the stream ahead of the application message, so the assertion intermittently saw the commit message (left: group_updated…, right: "hello"). The test even had a commented-out assert_msg_exists! acknowledging this commit message.

Fix: filter the stream to GroupMessageKind::Application messages so the assertions are deterministic regardless of whether the commit is delivered through the stream (errors pass through unchanged).

Verification: test_stream_messages passed 10/10 consecutive runs against a live node; cargo check --tests, clippy --tests, and fmt --check all clean for xmtp_mls.

This is in-scope with the PR's flaky-CI theme.

[codecov]

Codecov Report

❌ Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.47%. Comparing base (578abea) to head (36e833a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...db/src/encrypted_store/database/instrumentation.rs	84.21%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3767      +/-   ##
==========================================
+ Coverage   84.44%   84.47%   +0.02%     
==========================================
  Files         408      408              
  Lines       59807    59830      +23     
==========================================
+ Hits        50506    50540      +34     
+ Misses       9301     9290      -11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[insipx]

this makes it default off, so libxmtp workspace test and others which used to have panic on lock now have it off by default. i still want those to panic. can you turn it on for CI please? @xmtp-coder-agent it can stay off for nodesdk tests ONLY

[xmtp-coder-agent]

Done — turned the panic back on for the Rust workspace tests while keeping it off for node-sdk only.

Since both the workspace tests and node-sdk compile xmtp_db with test-utils, the split is made at the test-runner environment rather than at compile time:

• CI workspace tests: set XMTP_PANIC_ON_DB_LOCK=true in nix/package/nextest.nix (covers the v3 + d14n test-workspace job). Verified with nix eval .#nextest.x86_64-linux.v3.XMTP_PANIC_ON_DB_LOCK → true, so it reaches the derivation build env.
• Local just test: set it in the _test-v3 / _test-d14n recipes (the ci / ci-d14n nextest profiles), so local workspace runs panic too.
• node-sdk / node-bindings: run via just node test* (Vitest) and never set the var → stay off by default, so no more crashed fork workers.

Commit: 36e833a. Let me know if you'd also like it on for any other CI job (e.g. test-node).

[insipx]

@xmtp-coder-agent maybe instead of this env var, node sdk should use an in memory database for tests wherever possible? is the node sdk currently reusing the same db for each of its tests rather than creating a new one? then we can keep test using the panic. the panic is very useful for debuggin race conditions and should be default on.