Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 53 additions & 3 deletions crates/xmtp_db/src/encrypted_store/database/instrumentation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,33 @@ use diesel::connection::InstrumentationEvent;
use diesel::result::Error as DieselError;
use std::fmt::Write;

// Test instrumentatiomn
// Test instrumentation
// prints out query on error
// panics on database lock
// panics on database lock ONLY when explicitly opted in via the
// `XMTP_PANIC_ON_DB_LOCK` env var (accepts "1" or "true").
//
// `database is locked` is a transient, retryable condition in this codebase
// (SQLite is configured with `PRAGMA busy_timeout` and the error is classified
// as retryable). Panicking on it by default aborts the worker thread and crashes
// downstream test harnesses (e.g. the node-sdk Vitest fork workers), so the panic
// is opt-in for developers actively debugging lock contention.
#[allow(unused)]
pub struct TestInstrumentation;

/// Returns true when the caller has explicitly opted in to panicking on
/// `database is locked` via the `XMTP_PANIC_ON_DB_LOCK` env var.
fn panic_on_db_lock_enabled() -> bool {
panic_on_db_lock_from_env(std::env::var("XMTP_PANIC_ON_DB_LOCK"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_LOCKtrue, 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).

}

/// Pure decision helper: the opt-in is enabled only when the env var resolves to
/// exactly `"1"` or `"true"`. Anything else (unset, empty, other values) keeps the
/// default non-panicking behavior. Kept separate from the env read so it can be
/// unit-tested without mutating the process environment.
fn panic_on_db_lock_from_env(var: Result<String, std::env::VarError>) -> bool {
matches!(var, Ok(s) if s == "1" || s == "true")
}

impl Instrumentation for TestInstrumentation {
fn on_connection_event(&mut self, event: InstrumentationEvent<'_>) {
use InstrumentationEvent::*;
Expand All @@ -32,7 +53,7 @@ impl Instrumentation for TestInstrumentation {
tracing::error!("details: {},", details);
}
tracing::error!("{}", s);
if s.contains("database is locked") {
if s.contains("database is locked") && panic_on_db_lock_enabled() {
panic!("database locked");
}
}
Expand All @@ -52,3 +73,32 @@ impl Instrumentation for TestInstrumentation {
}
}
}

#[cfg(test)]
mod tests {
use super::panic_on_db_lock_from_env;
use std::env::VarError;

#[test]
fn db_lock_panic_disabled_by_default() {
// Unset env var (the default in CI / node-sdk Vitest workers) must NOT
// opt in to panicking, so a `database is locked` error stays recoverable.
assert!(!panic_on_db_lock_from_env(Err(VarError::NotPresent)));
}

#[test]
fn db_lock_panic_opt_in_values() {
assert!(panic_on_db_lock_from_env(Ok("1".to_string())));
assert!(panic_on_db_lock_from_env(Ok("true".to_string())));
}

#[test]
fn db_lock_panic_ignores_other_values() {
for v in ["0", "false", "", "yes", "TRUE", "True"] {
assert!(
!panic_on_db_lock_from_env(Ok(v.to_string())),
"value {v:?} should not enable the panic"
);
}
}
}
16 changes: 13 additions & 3 deletions crates/xmtp_mls/src/subscriptions/stream_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ pub mod tests {
use crate::tester;
use futures::stream::StreamExt;
use rstest::*;
use xmtp_db::group_message::GroupMessageKind;

#[xmtp_common::timeout(std::time::Duration::from_secs(30))]
#[rstest]
Expand All @@ -613,15 +614,24 @@ pub mod tests {
let bob_group = bob_groups.first().unwrap();
alice_group.sync().await.unwrap();

let stream = alice_group.stream().await.unwrap();
// The `group_updated` membership-change commit from `add_members` above can
// race into the stream ahead of the application messages, which made this
// test flaky (see xmtp/libxmtp#3765). Filter to application messages so the
// assertions below are deterministic regardless of whether the commit
// message is delivered through the stream.
let stream = alice_group.stream().await.unwrap().filter(|msg| {
futures::future::ready(
msg.as_ref()
.map(|m| m.kind == GroupMessageKind::Application)
.unwrap_or(true),
)
});
futures::pin_mut!(stream);
bob_group
.send_message(b"hello", SendMessageOpts::default())
.await
.unwrap();

// group updated msg/bob is added
// assert_msg_exists!(stream);
assert_msg!(stream, "hello");

bob_group
Expand Down
Loading