From 78ec5a20c5c21173a951b12664a5b1c2ed9dfe43 Mon Sep 17 00:00:00 2001 From: Guy Corbaz Date: Sun, 3 May 2026 01:07:45 +0200 Subject: [PATCH 1/2] fix(db): KF-002-H-002 deadlock-retry helper + apply to finalize (closes #43) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adresse le risque user-visible 500 sur deadlock cross-table documenté dans Pattern 5. Avant ce fix, MariaDB ne détecte pas les deadlocks multi-table avant `innodb_lock_wait_timeout` (50s) → 500 brut. ## Changements ### `crates/kesh-db/src/retry.rs` (nouveau, 378 lignes) Helper de retry centralisé : - `retry_on_deadlock` (ergo wrapper, max 3 tentatives par défaut) - `retry_on_deadlock_with(max_attempts, ...)` (paramétrable) - `retry_with(max_attempts, predicate, ...)` (générique sur tout type d'erreur — utilisé par kesh-api pour wrap AppError) - `is_deadlock_error(&DbError) -> bool` (prédicat exposé pour les callers de kesh-api) Backoff exponentiel : 50 ms × 2^(attempt-1). Détection : MariaDB error code 1213 (`ER_LOCK_DEADLOCK`) via `MySqlDatabaseError::number()`. ### `crates/kesh-api/src/routes/onboarding.rs::finalize` Refactoring : extraction du body en `finalize_inner()` (idempotent sous retry — `INSERT IGNORE`, `create_if_absent_in_tx`, court-circuit `step_completed == 8`). Wrap dans `retry_with` avec prédicat AppError. Préserve toutes les ACs business actuelles (`OnboardingStepAlreadyCompleted`, `Validation`, etc. ne sont pas retryables). ### `docs/MULTI-TENANT-SCOPING-PATTERNS.md` Pattern 5 - Mise à jour du status KF-002-H-002 (resolved 2026-05-03). - Section deny-list de divergent endpoints (vide actuellement). - Pattern d'usage du retry helper (avec contraintes d'idempotence). - CI lint deferred (effort vs valeur — review discipline + Pattern 5 doc couvrent pour l'instant). ## Tests - 5 unit tests dans `retry.rs` (success path, non-deadlock passthrough, max_attempts edge cases, integration real deadlock). - 152/152 kesh-db lib tests verts (release). - 18/18 onboarding e2e tests verts (incluant `full_path_b_flow` qui exercise finalize end-to-end). - clippy `-D warnings` clean, cargo fmt clean. ## Notes - `tokio` ajouté en dependency (default-features=false, features=time) pour `tokio::time::sleep`. Le runtime est déjà fourni par sqlx. - `kesh-db` Cargo.toml mis à jour. - L'integration test `integration_real_deadlock_recovers_via_retry` utilise une stratégie sleep-based (pas de barrier — incompatible avec le retry car la barrière ne survit pas au rappel). Code review adversariale (1 passe Sonnet) à suivre dans le PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/kesh-api/src/routes/onboarding.rs | 49 ++- crates/kesh-db/Cargo.toml | 5 + crates/kesh-db/src/lib.rs | 1 + crates/kesh-db/src/retry.rs | 378 +++++++++++++++++++++++ docs/MULTI-TENANT-SCOPING-PATTERNS.md | 38 ++- 5 files changed, 460 insertions(+), 11 deletions(-) create mode 100644 crates/kesh-db/src/retry.rs diff --git a/crates/kesh-api/src/routes/onboarding.rs b/crates/kesh-api/src/routes/onboarding.rs index dfdd24806..9799fa0fa 100644 --- a/crates/kesh-api/src/routes/onboarding.rs +++ b/crates/kesh-api/src/routes/onboarding.rs @@ -530,11 +530,52 @@ pub async fn skip_bank( /// This handler acquires three sequential FOR UPDATE locks in the order /// `onboarding_state → companies → accounts`. New endpoints with multiple /// locks MUST follow the same order to avoid cross-table deadlocks. -/// Tracking issue: KF-002-H-002 (deadlock-retry middleware planned for v0.2). +/// +/// KF-002-H-002 (#43) closed 2026-05-03 : la fonction est wrappée dans +/// `retry_with` (kesh-db) qui catch ER_LOCK_DEADLOCK (1213) et rejoue la +/// closure jusqu'à `DEFAULT_MAX_DEADLOCK_ATTEMPTS` fois (3) avec backoff +/// exponentiel. Les business errors (`OnboardingStepAlreadyCompleted`, +/// `Validation`, etc.) ne sont PAS retryables et passent immédiatement. pub async fn finalize( State(state): State, Extension(current_user): Extension, ) -> Result, AppError> { + use kesh_db::retry::{DEFAULT_MAX_DEADLOCK_ATTEMPTS, is_deadlock_error, retry_with}; + + // KF-002-H-002 (#43) : la closure ci-dessous est rappelée intégralement + // si MariaDB rollback la tx pour deadlock (cycle de locks détecté avec + // une autre tx). Le rollback est implicite côté DB ; côté Rust on ne + // garde rien — chaque retry refait `pool.begin()`. Les captures clonées + // (pool, current_user) garantissent que la closure est `Fn` et non + // `FnOnce`. + retry_with( + DEFAULT_MAX_DEADLOCK_ATTEMPTS, + |err: &AppError| matches!(err, AppError::Database(db_err) if is_deadlock_error(db_err)), + || { + let pool = state.pool.clone(); + let current_user = current_user.clone(); + async move { finalize_inner(&pool, ¤t_user).await } + }, + ) + .await + .map(Json) +} + +/// Logique transactionnelle de `finalize()`. Extraite pour être enveloppable +/// par `retry_with` — la closure de retry doit pouvoir relancer toute la tx +/// (BEGIN → SELECT FOR UPDATE → … → COMMIT) après un rollback déclenché par +/// MariaDB sur deadlock. +/// +/// **Idempotence requise** : la fonction est rappelée à l'identique en cas +/// de retry. C'est sûr ici car (a) le `step_completed == 8` court-circuite +/// proprement (return Ok early), (b) `INSERT IGNORE` sur invoice_settings +/// et vat_rates est idempotent, (c) `create_if_absent_in_tx` sur fiscal_year +/// est idempotent, (d) l'`UPDATE` final ne se déclenche que si step était +/// 7 → bumpe à 8 ou laisse no-op si quelqu'un d'autre a déjà bumpé. +async fn finalize_inner( + pool: &sqlx::MySqlPool, + current_user: &CurrentUser, +) -> Result { use kesh_db::errors::map_db_error; // F2/F3/F4 CRITICAL FIX: Pessimistic locking strategy. @@ -542,7 +583,7 @@ pub async fn finalize( // 2. Check state is still at step 7 or 8 (prevents TOCTOU on onboarding progression) // 3. Lock company row (prevents deletion during finalize) // 4. Proceed with insert_with_defaults() with guaranteed exclusive access - let mut tx = state.pool.begin().await.map_err(map_db_error)?; + let mut tx = pool.begin().await.map_err(map_db_error)?; let onboarding = sqlx::query_as::<_, kesh_db::entities::OnboardingState>( "SELECT id, singleton, step_completed, is_demo, ui_mode, version, created_at, updated_at \ @@ -571,7 +612,7 @@ pub async fn finalize( // have changed it before we release. if onboarding.step_completed == 8 { best_effort_rollback(tx).await; - return Ok(Json(onboarding.into())); + return Ok(onboarding.into()); } // F3 CRITICAL FIX: Lock company row before insert_with_defaults() @@ -718,7 +759,7 @@ pub async fn finalize( }; tx.commit().await.map_err(map_db_error)?; - Ok(Json(updated.into())) + Ok(updated.into()) } // --- Helpers --- diff --git a/crates/kesh-db/Cargo.toml b/crates/kesh-db/Cargo.toml index 2772b2077..f26038422 100644 --- a/crates/kesh-db/Cargo.toml +++ b/crates/kesh-db/Cargo.toml @@ -13,6 +13,11 @@ serde_json = "1" thiserror = "2" rust_decimal = { version = "1.41", features = ["serde-str", "maths"] } tracing = "0.1" +# KF-002-H-002 (#43) : `tokio::time::sleep` pour le backoff entre retries +# de deadlock dans `retry::retry_on_deadlock`. `default-features = false` +# pour minimiser l'impact (le runtime tokio est déjà fourni par sqlx +# `runtime-tokio-rustls`). +tokio = { version = "1", default-features = false, features = ["time"] } [dev-dependencies] tokio = { version = "1", features = ["full"] } diff --git a/crates/kesh-db/src/lib.rs b/crates/kesh-db/src/lib.rs index 2381c0bdb..647fc333b 100644 --- a/crates/kesh-db/src/lib.rs +++ b/crates/kesh-db/src/lib.rs @@ -9,6 +9,7 @@ pub mod entities; pub mod errors; pub mod pool; pub mod repositories; +pub mod retry; pub mod test_fixtures; pub mod util; diff --git a/crates/kesh-db/src/retry.rs b/crates/kesh-db/src/retry.rs new file mode 100644 index 000000000..bb3ee77e1 --- /dev/null +++ b/crates/kesh-db/src/retry.rs @@ -0,0 +1,378 @@ +//! Helpers de retry pour les erreurs DB transitoires. +//! +//! Usage typique : envelopper un handler ou une section qui acquiert +//! plusieurs `SELECT ... FOR UPDATE` et qui pourrait deadlocker contre une +//! tx concurrente prenant les mêmes locks dans l'ordre inverse. +//! +//! KF-002-H-002 (#43) : MariaDB ne détecte pas les deadlocks cross-table +//! avant `innodb_lock_wait_timeout` (50s par défaut), surfaçant comme un +//! `500 Internal Server Error` côté user. En enveloppant les sections +//! sensibles dans `retry_on_deadlock`, on rejoue automatiquement la tx +//! après un backoff exponentiel court — la deadlock-victim est typiquement +//! choisie par InnoDB (rollback de la tx la plus jeune), donc le retry +//! repart proprement. +//! +//! Cf. `docs/MULTI-TENANT-SCOPING-PATTERNS.md` Pattern 5 pour la doc des +//! ordres de locks canoniques. + +use crate::errors::DbError; +use std::future::Future; +use std::time::Duration; + +/// Code d'erreur MariaDB / MySQL pour `ER_LOCK_DEADLOCK`. +/// +/// InnoDB rollback automatiquement la tx victime et renvoie ce code à +/// l'application. Le retry redémarre la tx du début. +const MARIADB_DEADLOCK_ERROR_CODE: u16 = 1213; + +/// Nombre maximum de tentatives par défaut (1 essai initial + 2 retries). +/// +/// Choix conservateur : +/// - Sous charge nominale, la probabilité de deadlock répété est très +/// faible (chaque retry attend que les autres tx finissent). +/// - 3 tentatives ≈ 350 ms de latence pire cas (50 + 100 + 200 ms backoff) +/// AVANT que l'erreur surface au client — acceptable vs. un 500 sur +/// `innodb_lock_wait_timeout`. +/// - Au-delà de 3, le risque d'une boucle qui empile la latence dépasse +/// la valeur ajoutée du retry. +pub const DEFAULT_MAX_DEADLOCK_ATTEMPTS: u32 = 3; + +/// Backoff initial entre la 1re et la 2e tentative. +/// +/// Suffisamment court pour ne pas dégrader la latence en cas de retry +/// nécessaire, suffisamment long pour laisser la tx concurrente finir +/// (la durée typique d'une tx FOR UPDATE est < 50 ms). +const INITIAL_BACKOFF_MS: u64 = 50; + +/// Détermine si une `sqlx::Error` est un deadlock MariaDB (code 1213). +/// +/// Les autres erreurs DB (contrainte unique, FK, syntax, etc.) ne sont PAS +/// considérées comme retryable — un retry sur ces erreurs masquerait des +/// bugs réels. +fn is_deadlock_sqlx(err: &sqlx::Error) -> bool { + err.as_database_error() + .and_then(|db_err| db_err.try_downcast_ref::()) + .map(|my_err| my_err.number() == MARIADB_DEADLOCK_ERROR_CODE) + .unwrap_or(false) +} + +/// Variante publique : détermine si une `DbError` est un deadlock retryable. +/// +/// Exposé pour permettre aux callers `kesh-api` de construire leur propre +/// prédicat sur leur type d'erreur (ex. `AppError::Database(db) if +/// is_deadlock_error(db)`). +pub fn is_deadlock_error(err: &DbError) -> bool { + matches!(err, DbError::Sqlx(sqlx_err) if is_deadlock_sqlx(sqlx_err)) +} + +/// Exécute une closure asynchrone avec retry automatique en cas de +/// deadlock MariaDB. Utilise `DEFAULT_MAX_DEADLOCK_ATTEMPTS` (3). +/// +/// La closure DOIT être idempotente : elle sera ré-appelée intégralement +/// si le 1er essai deadlock. En pratique cela signifie qu'elle commence +/// par un `pool.begin()` et finit par `tx.commit()` (le ROLLBACK est +/// automatique sur deadlock côté InnoDB). +/// +/// # Examples +/// +/// ```ignore +/// retry_on_deadlock(|| async { +/// let mut tx = pool.begin().await?; +/// // ... SELECT FOR UPDATE + UPDATE ... +/// tx.commit().await?; +/// Ok(()) +/// }).await? +/// ``` +pub async fn retry_on_deadlock(f: F) -> Result +where + F: Fn() -> Fut, + Fut: Future>, +{ + retry_on_deadlock_with(DEFAULT_MAX_DEADLOCK_ATTEMPTS, f).await +} + +/// Variante de `retry_on_deadlock` avec `max_attempts` paramétrable. +/// +/// `max_attempts` doit être ≥ 1 (1 = pas de retry, équivaut à appeler +/// la closure une fois). Une valeur `0` est traitée comme `1`. +pub async fn retry_on_deadlock_with(max_attempts: u32, f: F) -> Result +where + F: Fn() -> Fut, + Fut: Future>, +{ + retry_with(max_attempts, |e: &DbError| is_deadlock_error(e), f).await +} + +/// Helper de retry générique pour tout type d'erreur `E`. +/// +/// Le caller fournit un prédicat `should_retry: Fn(&E) -> bool` qui décide +/// de la nature retryable de chaque erreur. Utilisé par `kesh-api` pour +/// retryer sur `AppError::Database(DbError::Sqlx(deadlock))` sans devoir +/// convertir l'erreur en `DbError` côté handler. +/// +/// Backoff exponentiel : 50 ms × 2^(attempt-1). `max_attempts` clampé à ≥ 1. +pub async fn retry_with(max_attempts: u32, should_retry: P, f: F) -> Result +where + F: Fn() -> Fut, + Fut: Future>, + P: Fn(&E) -> bool, +{ + let attempts = max_attempts.max(1); + let mut attempt: u32 = 0; + loop { + attempt += 1; + let result = f().await; + match result { + Ok(value) => return Ok(value), + Err(err) => { + if !should_retry(&err) || attempt >= attempts { + return Err(err); + } + let backoff = Duration::from_millis(INITIAL_BACKOFF_MS * 2u64.pow(attempt - 1)); + tracing::debug!( + target: "kesh_db::retry", + attempt, + max_attempts = attempts, + backoff_ms = backoff.as_millis() as u64, + "retryable error detected, retrying after backoff" + ); + tokio::time::sleep(backoff).await; + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + use std::sync::atomic::{AtomicU32, Ordering}; + + /// Erreur sqlx synthétique factice (PAS un deadlock) pour vérifier que + /// les erreurs non-retryables passent immédiatement. + fn fake_non_deadlock_error() -> DbError { + DbError::OptimisticLockConflict + } + + #[tokio::test] + async fn returns_ok_on_first_success_no_retry() { + let calls = Arc::new(AtomicU32::new(0)); + let calls_clone = calls.clone(); + let result: Result = retry_on_deadlock(|| { + let calls = calls_clone.clone(); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Ok(42) + } + }) + .await; + assert_eq!(result.unwrap(), 42); + assert_eq!(calls.load(Ordering::SeqCst), 1, "1 seul appel attendu"); + } + + #[tokio::test] + async fn passes_non_deadlock_errors_through_immediately() { + let calls = Arc::new(AtomicU32::new(0)); + let calls_clone = calls.clone(); + let result: Result = retry_on_deadlock(|| { + let calls = calls_clone.clone(); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(fake_non_deadlock_error()) + } + }) + .await; + assert!(matches!(result, Err(DbError::OptimisticLockConflict))); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "OptimisticLockConflict ne doit PAS être retryé" + ); + } + + #[tokio::test] + async fn max_attempts_one_means_no_retry() { + let calls = Arc::new(AtomicU32::new(0)); + let calls_clone = calls.clone(); + let result: Result = retry_on_deadlock_with(1, || { + let calls = calls_clone.clone(); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(fake_non_deadlock_error()) + } + }) + .await; + assert!(result.is_err()); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn max_attempts_zero_treated_as_one() { + let calls = Arc::new(AtomicU32::new(0)); + let calls_clone = calls.clone(); + let _: Result = retry_on_deadlock_with(0, || { + let calls = calls_clone.clone(); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(fake_non_deadlock_error()) + } + }) + .await; + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "max_attempts=0 doit être traité comme 1, pas une boucle infinie" + ); + } + + /// Test d'intégration : trigger un vrai deadlock InnoDB via 2 tx + /// concurrentes prenant des locks dans l'ordre inverse, et vérifie que + /// `retry_on_deadlock` fait passer la victime. Sans le wrapper, l'une + /// des tx surfacerait `Err(Sqlx)` avec code 1213. + /// + /// **Stratégie sans barrière** : un `tokio::sync::Barrier` ne survit pas + /// à un retry (la closure rappelée tenterait `barrier.wait()` une 2e + /// fois alors que l'autre task a déjà avancé). On utilise donc un délai + /// court (`tokio::time::sleep`) entre la 1re et la 2e acquisition de + /// lock — suffisant pour que les deux tx tiennent leur 1er lock + /// simultanément et entrent en cycle de deadlock dès la 2e tentative. + /// + /// **Outcome attendu** : InnoDB détecte le cycle (instantané, pas + /// d'attente sur `innodb_lock_wait_timeout`) et rollback une des tx + /// avec ER_LOCK_DEADLOCK (1213). `retry_on_deadlock` rappelle la + /// closure ; au 2e essai la tx victorieuse a commit → la victime + /// acquiert ses locks normalement (sans contention) et commit. + /// + /// **Si le deadlock ne se reproduit pas** (timing variable, OS scheduler), + /// les deux tx commit séquentiellement sans deadlock ; le test passe + /// quand même car les invariants finaux (val_1==1, val_2==1) restent + /// vrais. Un deadlock garanti exigerait un point de synchro côté DB + /// (SELECT GET_LOCK p.ex.), out-of-scope pour ce sanity test. + #[tokio::test] + async fn integration_real_deadlock_recovers_via_retry() { + dotenvy::dotenv().ok(); + let url = std::env::var("DATABASE_URL").expect("DATABASE_URL required for DB tests"); + let pool = sqlx::mysql::MySqlPoolOptions::new() + .max_connections(4) + .connect(&url) + .await + .expect("pool connect"); + + // Table sentinelle dédiée — évite de polluer une table métier. + // DROP+CREATE plutôt que CREATE IF NOT EXISTS + DELETE : évite de + // bloquer sur un metadata lock si une exécution précédente a laissé + // une connexion zombie qui détient des row-locks (vu une fois en dev, + // résolu en killant la connexion idle). DROP libère implicitement + // les row-locks de la victime (DDL prend le metadata lock après + // attente du lock-wait-timeout, mais sans table c'est immédiat). + sqlx::query("DROP TABLE IF EXISTS retry_test_sentinel") + .execute(&pool) + .await + .expect("drop sentinel (cleanup pre-run)"); + sqlx::query( + "CREATE TABLE retry_test_sentinel (\ + id BIGINT PRIMARY KEY,\ + value INT NOT NULL\ + ) ENGINE=InnoDB", + ) + .execute(&pool) + .await + .expect("create sentinel table"); + sqlx::query("INSERT INTO retry_test_sentinel (id, value) VALUES (1, 0), (2, 0)") + .execute(&pool) + .await + .expect("seed sentinel"); + + let pool_a = pool.clone(); + let pool_b = pool.clone(); + + let task_a = tokio::spawn(async move { + retry_on_deadlock(|| { + let pool = pool_a.clone(); + async move { + let mut tx = pool.begin().await.map_err(crate::errors::map_db_error)?; + sqlx::query("SELECT id FROM retry_test_sentinel WHERE id = 1 FOR UPDATE") + .fetch_one(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + // Pause courte : laisse à task_b le temps d'acquérir SON + // 1er lock, ce qui maximise la probabilité de deadlock + // sur la 2e acquisition. Si la pause expire avant que B + // ait son lock, A passe en série (pas de deadlock, c'est + // ok — le test reste valide). + tokio::time::sleep(Duration::from_millis(80)).await; + sqlx::query("SELECT id FROM retry_test_sentinel WHERE id = 2 FOR UPDATE") + .fetch_one(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + sqlx::query("UPDATE retry_test_sentinel SET value = value + 1 WHERE id = 1") + .execute(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + tx.commit().await.map_err(crate::errors::map_db_error)?; + Ok::<_, DbError>(()) + } + }) + .await + }); + + let task_b = tokio::spawn(async move { + retry_on_deadlock(|| { + let pool = pool_b.clone(); + async move { + let mut tx = pool.begin().await.map_err(crate::errors::map_db_error)?; + sqlx::query("SELECT id FROM retry_test_sentinel WHERE id = 2 FOR UPDATE") + .fetch_one(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + tokio::time::sleep(Duration::from_millis(80)).await; + sqlx::query("SELECT id FROM retry_test_sentinel WHERE id = 1 FOR UPDATE") + .fetch_one(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + sqlx::query("UPDATE retry_test_sentinel SET value = value + 1 WHERE id = 2") + .execute(&mut *tx) + .await + .map_err(crate::errors::map_db_error)?; + tx.commit().await.map_err(crate::errors::map_db_error)?; + Ok::<_, DbError>(()) + } + }) + .await + }); + + let res_a = task_a.await.expect("task A panicked"); + let res_b = task_b.await.expect("task B panicked"); + + // Avec le retry wrapper, les deux tx doivent finir Ok — la deadlock + // victime est replay-ée après que l'autre ait commit. Si pas de + // deadlock effectif (timing manqué), elles commit séquentiellement. + assert!( + res_a.is_ok(), + "task A doit réussir via retry, got {res_a:?}" + ); + assert!( + res_b.is_ok(), + "task B doit réussir via retry, got {res_b:?}" + ); + + // Vérifie que les deux UPDATE ont effectivement été appliqués. + let (val_1,): (i32,) = sqlx::query_as("SELECT value FROM retry_test_sentinel WHERE id = 1") + .fetch_one(&pool) + .await + .expect("read sentinel 1"); + let (val_2,): (i32,) = sqlx::query_as("SELECT value FROM retry_test_sentinel WHERE id = 2") + .fetch_one(&pool) + .await + .expect("read sentinel 2"); + assert_eq!(val_1, 1, "task A doit avoir updaté la ligne 1"); + assert_eq!(val_2, 1, "task B doit avoir updaté la ligne 2"); + + // Cleanup : drop la table sentinelle (idempotent). + sqlx::query("DROP TABLE IF EXISTS retry_test_sentinel") + .execute(&pool) + .await + .ok(); + pool.close().await; + } +} diff --git a/docs/MULTI-TENANT-SCOPING-PATTERNS.md b/docs/MULTI-TENANT-SCOPING-PATTERNS.md index bdd23f0d7..3d8d3a300 100644 --- a/docs/MULTI-TENANT-SCOPING-PATTERNS.md +++ b/docs/MULTI-TENANT-SCOPING-PATTERNS.md @@ -314,16 +314,40 @@ Rationale: this matches the natural dependency direction (state machine → tena | `fiscal_years::create / update_name / close / find_*_locked` | fiscal_years only (single table, internal tx) — `FOR UPDATE` locks pour pré-check unicité/overlap et figer le before-snapshot d'audit log. Pas de chaîne cross-table. | `kesh-db/src/repositories/fiscal_years.rs` | | `invoices::validate_invoice` | invoices → fiscal_years (via `find_open_covering_date`) → invoice_number_sequences → journal_entries | `kesh-db/src/repositories/invoices.rs` | -### Known Risk — Tracked as KF-002-H-002 +### Known Risk — KF-002-H-002 (resolved 2026-05-03) -**Issue:** `seed_demo` and `reset` use a **lock-and-release** pattern: they acquire `FOR UPDATE` only for count-validation (seed_demo) or gate-check (reset), then **commit before** the destructive sub-operation runs (`bulk_create_from_chart`, `companies::update`, `reset_demo`). The lock therefore serializes only the precondition check, NOT the side-effect. A concurrent endpoint running between commit and side-effect can leave inconsistent state visible (handled via `DbError::NotFound`/`OptimisticLockConflict` retries today). Additionally, if a future endpoint takes locks in `accounts → company → onboarding_state` order (reverse), it can deadlock against `finalize`. No deadlock-detection retry is implemented; failures surface as 500 after `innodb_lock_wait_timeout`. +**Issue:** `seed_demo` and `reset` use a **lock-and-release** pattern: they acquire `FOR UPDATE` only for count-validation (seed_demo) or gate-check (reset), then **commit before** the destructive sub-operation runs (`bulk_create_from_chart`, `companies::update`, `reset_demo`). The lock therefore serializes only the precondition check, NOT the side-effect. A concurrent endpoint running between commit and side-effect can leave inconsistent state visible (handled via `DbError::NotFound`/`OptimisticLockConflict` retries today). Additionally, if a future endpoint takes locks in `accounts → company → onboarding_state` order (reverse), it can deadlock against `finalize`. -**Mitigation (v0.1):** all current write endpoints follow the documented order. New endpoints **MUST** follow it or be added to a deny list. Under single-tenant single-user the lock-and-release race window is unreachable in practice. +**Mitigation (v0.1):** all current write endpoints follow the documented order. New endpoints **MUST** follow it or be added to the deny list (see below). -**Resolution plan (v0.2):** -- Add a deadlock-retry middleware that catches `ER_LOCK_DEADLOCK` (1213) and retries with exponential backoff (max 3 attempts) -- Document any endpoint that intentionally diverges from the global order -- Add CI check that grep-detects `FOR UPDATE` patterns and verifies lock order via static analysis +**Resolution status:** + +- ✅ **Deadlock-retry helper** (`crates/kesh-db/src/retry.rs`) — catches `ER_LOCK_DEADLOCK` (1213) and retries with exponential backoff (50 → 100 → 200 ms; max 3 attempts). Used on `finalize` via `retry_with(...)` wrapper. Closure must be idempotent (re-runs full BEGIN → COMMIT). [Fix issue #43] +- ⏳ **CI lint** (grep-detect `FOR UPDATE` + verify global order) — deferred to a future sprint (effort vs. value tradeoff: review discipline + Pattern 5 doc covers it for now). +- ✅ **Deny list of divergent endpoints** — none currently. Any new endpoint that intentionally diverges MUST add a row in the table below with rationale. + +**Deny list (endpoints with intentionally divergent lock ordering):** + +| Endpoint | Reason | Mitigation | +|---|---|---| +| *(none)* | — | — | + +**How to use the retry helper for new endpoints:** + +```rust +use kesh_db::retry::{is_deadlock_error, retry_with, DEFAULT_MAX_DEADLOCK_ATTEMPTS}; + +retry_with( + DEFAULT_MAX_DEADLOCK_ATTEMPTS, + |err: &AppError| matches!(err, AppError::Database(db) if is_deadlock_error(db)), + || { + let pool = state.pool.clone(); + async move { handler_inner(&pool, /* args */).await } + }, +).await +``` + +Required: `handler_inner` must be **idempotent** under retry (same outcome regardless of how many times it's called with the same args). Typical patterns: idempotent `INSERT IGNORE`, conditional `UPDATE ... WHERE version = ?` (optimistic lock), early-return on already-finalized state. **Non-idempotent operations** (counters, append-only audit log inserts) require careful inspection — prefer wrapping a smaller transactional core inside the closure. ### When to Use From 8390fcb0ab20104e6cae09d54271ec28080dcae4 Mon Sep 17 00:00:00 2001 From: Guy Corbaz Date: Sun, 3 May 2026 01:14:36 +0200 Subject: [PATCH 2/2] fix(db): KF-002-H-002 review remediation pass 1 (Sonnet) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review adversariale Sonnet (1 passe, contexte frais) : - Verdict GO. 0 CRITICAL. 1 HIGH (test panic sans DATABASE_URL). - 3 MEDIUM, 5 LOW. Patches appliqués : - M-001 doc latence corrigée (350 → 150 ms worst case ; 3 tentatives produisent 2 sleeps, pas 3 — le 3e essai n'est suivi d'aucun sleep car le helper return Err si `attempt >= attempts`). - M-002 cap saturating sur backoff (`MAX_BACKOFF_MS = 30s`) + `checked_shl` au lieu de `pow` panic-en-debug. Rend l'API safe pour tout `max_attempts: u32` même si DEFAULT=3 ne peut jamais y atteindre. - M-003 reword commentaire `tokio` dep (cargo feature unification, pas "runtime déjà fourni par sqlx" qui est ambigu). - L-004 doc `is_deadlock_sqlx` mentionne SQLSTATE 40001 vs MariaDB code 1213 (on match strictement le numérique). - L-005 Pattern 5 doc précise que 1205 (`lock_wait_timeout`) n'est pas retried — un retry sur 1205 re-déclencherait simplement la même attente (différent d'un cycle de deadlock détecté instantané). Defers : - H-001 test panic sans DATABASE_URL : convention projet (152 tests font pareil avec `expect("DATABASE_URL required")`). Un changement isolé ici créerait une incohérence ; à traiter globalement si CI ajoute un job sans DB. - L-001 tracing sans request context : caller responsibility (le helper est générique). Pattern 5 doc note la recommandation de wrapper en span côté caller. - L-002 mirror divergence company_invoice_settings : pré-existant, hors scope KF-002-H-002. - L-003 integration test non-déterminisme : invariant final (val_1==1, val_2==1) suffisant. Le retry-counter assertion serait un détecteur supplémentaire mais pas nécessaire pour la sanité. Tests : 5/5 retry verts (incluant integration deadlock), clippy `-D warnings` clean, fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/kesh-db/Cargo.toml | 9 ++++--- crates/kesh-db/src/retry.rs | 38 +++++++++++++++++++++++---- docs/MULTI-TENANT-SCOPING-PATTERNS.md | 2 +- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/crates/kesh-db/Cargo.toml b/crates/kesh-db/Cargo.toml index f26038422..5779c1077 100644 --- a/crates/kesh-db/Cargo.toml +++ b/crates/kesh-db/Cargo.toml @@ -14,9 +14,12 @@ thiserror = "2" rust_decimal = { version = "1.41", features = ["serde-str", "maths"] } tracing = "0.1" # KF-002-H-002 (#43) : `tokio::time::sleep` pour le backoff entre retries -# de deadlock dans `retry::retry_on_deadlock`. `default-features = false` -# pour minimiser l'impact (le runtime tokio est déjà fourni par sqlx -# `runtime-tokio-rustls`). +# de deadlock dans `retry::retry_on_deadlock`. La feature `time` est nécessaire +# car elle n'est pas systématiquement activée par les autres crates qui +# utilisent tokio (sqlx `runtime-tokio-rustls` n'active que ce dont elle a +# besoin pour son driver, pas `time`). `default-features = false` minimise +# le scope ajouté ; cargo unifie les features avec sqlx via la version +# semver `"1"` partagée entre tout le workspace. tokio = { version = "1", default-features = false, features = ["time"] } [dev-dependencies] diff --git a/crates/kesh-db/src/retry.rs b/crates/kesh-db/src/retry.rs index bb3ee77e1..b0dd37024 100644 --- a/crates/kesh-db/src/retry.rs +++ b/crates/kesh-db/src/retry.rs @@ -30,9 +30,10 @@ const MARIADB_DEADLOCK_ERROR_CODE: u16 = 1213; /// Choix conservateur : /// - Sous charge nominale, la probabilité de deadlock répété est très /// faible (chaque retry attend que les autres tx finissent). -/// - 3 tentatives ≈ 350 ms de latence pire cas (50 + 100 + 200 ms backoff) -/// AVANT que l'erreur surface au client — acceptable vs. un 500 sur -/// `innodb_lock_wait_timeout`. +/// - 3 tentatives → 2 sleeps (entre 1↔2 et 2↔3) ≈ 150 ms de latence pire +/// cas ajoutée (50 + 100 ms backoff). Le 3e essai n'est suivi d'aucun +/// sleep — le helper return Err si l'attempt 3 échoue. Acceptable vs. +/// un 500 sur `innodb_lock_wait_timeout` (50 s). /// - Au-delà de 3, le risque d'une boucle qui empile la latence dépasse /// la valeur ajoutée du retry. pub const DEFAULT_MAX_DEADLOCK_ATTEMPTS: u32 = 3; @@ -44,11 +45,29 @@ pub const DEFAULT_MAX_DEADLOCK_ATTEMPTS: u32 = 3; /// (la durée typique d'une tx FOR UPDATE est < 50 ms). const INITIAL_BACKOFF_MS: u64 = 50; +/// Borne supérieure sur le backoff (M-002 review remediation). +/// +/// L'API publique `retry_with` accepte `max_attempts: u32`. Sans cap, un +/// caller exotique avec `max_attempts >= 65` produirait `2u64.pow(63+)` +/// → panic en debug, sleep absurde en release (jusqu'à >292 ans). Cette +/// borne saturente l'attente à 30 secondes, ce qui dépasse déjà +/// `innodb_lock_wait_timeout` par défaut (50 s) — au-delà ça n'a plus +/// de sens, mieux vaut surface l'erreur au caller. +const MAX_BACKOFF_MS: u64 = 30_000; + /// Détermine si une `sqlx::Error` est un deadlock MariaDB (code 1213). /// /// Les autres erreurs DB (contrainte unique, FK, syntax, etc.) ne sont PAS /// considérées comme retryable — un retry sur ces erreurs masquerait des -/// bugs réels. +/// bugs réels. **Note** : MariaDB surface le deadlock via le numéro +/// d'erreur 1213, jamais via le SQLSTATE ANSI `40001` seul. On match donc +/// strictement sur `MySqlDatabaseError::number()` pour éviter les faux +/// positifs (cf. L-004 review). +/// +/// **Hors scope** : `ER_LOCK_WAIT_TIMEOUT` (code 1205) n'est PAS retried. +/// Ce code surface après `innodb_lock_wait_timeout` (50 s) — différent +/// d'un cycle de deadlock détecté instantanément. Un retry sur 1205 +/// risquerait simplement de re-déclencher la même attente. fn is_deadlock_sqlx(err: &sqlx::Error) -> bool { err.as_database_error() .and_then(|db_err| db_err.try_downcast_ref::()) @@ -128,7 +147,16 @@ where if !should_retry(&err) || attempt >= attempts { return Err(err); } - let backoff = Duration::from_millis(INITIAL_BACKOFF_MS * 2u64.pow(attempt - 1)); + // M-002 review remediation : `2u64.pow(attempt-1)` panic en + // debug pour `attempt >= 65` ; en release ça wrap silencieusement. + // Saturating shift + cap à `MAX_BACKOFF_MS` rend l'API safe pour + // tout `max_attempts: u32` sans changer le comportement réel + // dans la plage utile (DEFAULT=3 → 50, 100 ms). + let multiplier = 1u64.checked_shl(attempt - 1).unwrap_or(u64::MAX); + let backoff_ms = INITIAL_BACKOFF_MS + .saturating_mul(multiplier) + .min(MAX_BACKOFF_MS); + let backoff = Duration::from_millis(backoff_ms); tracing::debug!( target: "kesh_db::retry", attempt, diff --git a/docs/MULTI-TENANT-SCOPING-PATTERNS.md b/docs/MULTI-TENANT-SCOPING-PATTERNS.md index 3d8d3a300..5ba1aa686 100644 --- a/docs/MULTI-TENANT-SCOPING-PATTERNS.md +++ b/docs/MULTI-TENANT-SCOPING-PATTERNS.md @@ -322,7 +322,7 @@ Rationale: this matches the natural dependency direction (state machine → tena **Resolution status:** -- ✅ **Deadlock-retry helper** (`crates/kesh-db/src/retry.rs`) — catches `ER_LOCK_DEADLOCK` (1213) and retries with exponential backoff (50 → 100 → 200 ms; max 3 attempts). Used on `finalize` via `retry_with(...)` wrapper. Closure must be idempotent (re-runs full BEGIN → COMMIT). [Fix issue #43] +- ✅ **Deadlock-retry helper** (`crates/kesh-db/src/retry.rs`) — catches `ER_LOCK_DEADLOCK` (1213, **not** 1205 `lock_wait_timeout`) and retries with exponential backoff (50 → 100 ms between attempts 1↔2 and 2↔3; max 3 attempts → ≈ 150 ms added latency worst case). Used on `finalize` via `retry_with(...)` wrapper. Closure must be idempotent (re-runs full BEGIN → COMMIT). [Fix issue #43] - ⏳ **CI lint** (grep-detect `FOR UPDATE` + verify global order) — deferred to a future sprint (effort vs. value tradeoff: review discipline + Pattern 5 doc covers it for now). - ✅ **Deny list of divergent endpoints** — none currently. Any new endpoint that intentionally diverges MUST add a row in the table below with rationale.