diff --git a/src/db.rs b/src/db.rs index 2e987fe..f91249c 100644 --- a/src/db.rs +++ b/src/db.rs @@ -27,3 +27,10 @@ pub async fn run_migrations(pool: &PgPool) -> Result<()> { tracing::info!("Migrations complete"); Ok(()) } + +/// True if `error` is a PostgreSQL unique/primary-key constraint violation. +/// Used across routes and services to gracefully handle insert races as `409 Conflict` (#55). +pub fn is_unique_violation(error: &sqlx::Error) -> bool { + matches!(error, sqlx::Error::Database(db_err) if db_err.is_unique_violation()) +} + diff --git a/src/routes/auth.rs b/src/routes/auth.rs index e7cd18f..da0e6d1 100644 --- a/src/routes/auth.rs +++ b/src/routes/auth.rs @@ -1,4 +1,5 @@ use crate::{ + db::is_unique_violation, error::{AppError, AppResult}, middleware::auth::issue_jwt, models::user::{AuthResponse, CreateUserRequest, LoginRequest, User, UserRow}, @@ -31,7 +32,7 @@ pub async fn register( return Err(AppError::Validation("full_name is required".into())); } - // Check uniqueness. + // Fast-path uniqueness check to avoid unnecessary bcrypt hashing cost. let exists: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM users WHERE email = $1)") .bind(&email) .fetch_one(&state.pool) @@ -47,7 +48,7 @@ pub async fn register( AppError::Internal(anyhow::anyhow!("bcrypt error: {e}")) })?; - // Insert. + // Insert with unique-violation protection against concurrent registration races (#55). let id = Uuid::new_v4(); let row = sqlx::query_as::<_, UserRow>( r#" @@ -62,7 +63,14 @@ pub async fn register( .bind(req.full_name.trim()) .bind(&req.stellar_address) .fetch_one(&state.pool) - .await?; + .await + .map_err(|e| { + if is_unique_violation(&e) { + AppError::Conflict("Email".into()) + } else { + e.into() + } + })?; let user = User::from(row); @@ -122,3 +130,109 @@ pub async fn login( "data": AuthResponse { token, user } }))) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn is_unique_violation_helper_identifies_non_db_errors_as_false() { + let err = sqlx::Error::RowNotFound; + assert!(!is_unique_violation(&err)); + } +} + +/// End-to-end database tests for registration unique-constraint race condition (#55). +/// Run with `cargo test -- --ignored` against a real database instance. +#[cfg(test)] +mod db_tests { + use super::*; + use crate::config::Config; + use sqlx::postgres::PgPoolOptions; + + async fn test_pool() -> PgPool { + let database_url = std::env::var("DATABASE_URL") + .expect("DATABASE_URL must be set to run this integration test"); + let pool = PgPoolOptions::new() + .max_connections(5) + .connect(&database_url) + .await + .expect("failed to connect to DATABASE_URL"); + sqlx::migrate!("./migrations") + .run(&pool) + .await + .expect("failed to run migrations"); + pool + } + + async fn cleanup_user_by_email(pool: &PgPool, email: &str) { + let _ = sqlx::query("DELETE FROM users WHERE email = $1") + .bind(email) + .execute(pool) + .await; + } + + #[tokio::test] + #[ignore] + async fn concurrent_register_calls_for_same_email_returns_one_success_and_one_conflict() { + let pool = test_pool().await; + let email = format!("test-race-{}@example.test", Uuid::new_v4()); + + let config = Config::from_env().unwrap_or_else(|_| Config { + app_env: "test".into(), + host: "127.0.0.1".into(), + port: 8080, + database_url: std::env::var("DATABASE_URL").unwrap(), + database_max_connections: 5, + database_min_connections: 1, + database_connect_timeout_secs: 5, + jwt_secret: "test_secret_for_registration_tests_12345".into(), + jwt_expiry_hours: 24, + horizon_url: "https://horizon-testnet.stellar.org".into(), + stellar_network_passphrase: "Test SDF Network ; September 2015".into(), + soroban_rpc_url: "https://soroban-testnet.stellar.org".into(), + allowed_origins: vec!["*".into()], + rate_cache_ttl_secs: 30, + keeper_enabled: false, + keeper_poll_interval_secs: 60, + keeper_secret_key: None, + escrow_contract_id: None, + subscription_contract_id: None, + reconciliation_poll_interval_secs: 60, + reconciliation_stale_after_secs: 300, + }); + + let state = Arc::new(AppState { + pool: pool.clone(), + config, + loop_health: crate::BackgroundLoopHealth::default(), + }); + + let req1 = CreateUserRequest { + email: email.clone(), + password: "password123".into(), + full_name: "User One".into(), + stellar_address: None, + }; + let req2 = CreateUserRequest { + email: email.clone(), + password: "password123".into(), + full_name: "User Two".into(), + stellar_address: None, + }; + + let (res1, res2) = tokio::join!( + register(State(state.clone()), Json(req1)), + register(State(state.clone()), Json(req2)) + ); + + let one_ok = res1.is_ok() || res2.is_ok(); + let one_conflict = matches!(&res1, Err(AppError::Conflict(_))) || matches!(&res2, Err(AppError::Conflict(_))); + + assert!(one_ok, "Exactly one concurrent registration should succeed"); + assert!(one_conflict, "The racing registration loser must receive Conflict (409), not a 500 Database error"); + + cleanup_user_by_email(&pool, &email).await; + } +} + diff --git a/src/services/batch.rs b/src/services/batch.rs index 943d493..7f0ea12 100644 --- a/src/services/batch.rs +++ b/src/services/batch.rs @@ -1,4 +1,5 @@ use crate::{ + db::is_unique_violation, error::{AppError, AppResult}, models::{ batch_payment::{BatchPaymentResult, SendBatchPaymentRequest}, @@ -17,11 +18,6 @@ pub struct BatchPaymentService { pool: PgPool, } -/// True if `error` is a Postgres unique/primary-key violation — used to -/// detect losing the race to claim a `batch_submissions` row. -fn is_unique_violation(error: &sqlx::Error) -> bool { - matches!(error, sqlx::Error::Database(db_err) if db_err.is_unique_violation()) -} /// Decides what a failed submission attempt means for transaction status /// (#30). The distinction is "did we get a definitive answer from Horizon diff --git a/src/services/escrow.rs b/src/services/escrow.rs index 679dd48..a44ba45 100644 --- a/src/services/escrow.rs +++ b/src/services/escrow.rs @@ -1,5 +1,6 @@ use crate::{ config::Config, + db::is_unique_violation, error::{AppError, AppResult}, models::{ escrow::{CreateEscrowRequest, Escrow, EscrowActionRequest, EscrowActor, EscrowRow, EscrowStatus}, @@ -32,17 +33,11 @@ impl EscrowAction { } } -/// True if `error` is a Postgres unique/primary-key violation — used to -/// detect a duplicate `onchain_escrow_id` (#56), mirroring the same pattern -/// in `services::batch::is_unique_violation`. -fn is_unique_violation(error: &sqlx::Error) -> bool { - matches!(error, sqlx::Error::Database(db_err) if db_err.is_unique_violation()) -} - pub struct EscrowService { pool: PgPool, } + impl EscrowService { pub fn new(pool: PgPool) -> Self { Self { pool } diff --git a/src/services/subscription.rs b/src/services/subscription.rs index 465a519..4af1999 100644 --- a/src/services/subscription.rs +++ b/src/services/subscription.rs @@ -1,5 +1,6 @@ use crate::{ config::Config, + db::is_unique_violation, error::{AppError, AppResult}, models::{ subscription::{ @@ -34,17 +35,11 @@ pub struct KeeperRunSummary { pub considered: usize, } -/// True if `error` is a Postgres unique/primary-key violation — used to -/// detect a duplicate `onchain_subscription_id` (#56), mirroring the same -/// pattern in `services::batch::is_unique_violation`. -fn is_unique_violation(error: &sqlx::Error) -> bool { - matches!(error, sqlx::Error::Database(db_err) if db_err.is_unique_violation()) -} - pub struct SubscriptionService { pool: PgPool, } + impl SubscriptionService { pub fn new(pool: PgPool) -> Self { Self { pool }