Skip to content

fix(db): KF-002-H-002 deadlock-retry helper + apply to finalize (closes #43) - #65

Merged
guycorbaz merged 2 commits into
mainfrom
fix/kf-002-h-002-deadlock-retry
May 2, 2026
Merged

fix(db): KF-002-H-002 deadlock-retry helper + apply to finalize (closes #43)#65
guycorbaz merged 2 commits into
mainfrom
fix/kf-002-h-002-deadlock-retry

Conversation

@guycorbaz

@guycorbaz guycorbaz commented May 2, 2026

Copy link
Copy Markdown
Owner

Summary

Item #3 du prep sprint Epic 8. Adresse le risque user-visible 500 sur deadlock cross-table documenté dans Pattern 5.

Avant : MariaDB ne détecte pas les deadlocks multi-table avant innodb_lock_wait_timeout (50s) → 500 brut côté user. Mitigation v0.1 : doc-only.

Après : helper retry_with générique catche ER_LOCK_DEADLOCK (1213) et rejoue la closure avec backoff exponentiel (50→100ms ; max 3 tentatives, ≈ 150ms latence ajoutée pire cas). Appliqué à finalize() qui était l'endpoint at-risk identifié.

Architecture (option B validée Guy)

Helper repo-level retry_with(max_attempts, predicate, closure) dans crates/kesh-db/src/retry.rs :

  • retry_on_deadlock : ergo wrapper sur DbError, max 3 tentatives.
  • retry_with : générique sur tout type d'erreur E + prédicat. Utilisé par kesh-api pour wrap AppError.
  • is_deadlock_error(&DbError) : prédicat exposé pour les callers.

Backoff capé à MAX_BACKOFF_MS = 30s + checked_shl pour éviter overflow sur max_attempts exotiques (review M-002).

Application à finalize()

Refactoring : extraction du body en finalize_inner() (idempotent — INSERT IGNORE, create_if_absent_in_tx, court-circuit step_completed == 8). Wrap dans retry_with avec prédicat AppError ciblant uniquement AppError::Database(DbError::Sqlx(deadlock)).

Business errors (OnboardingStepAlreadyCompleted, Validation) ne sont pas retryables — pass-through immédiat.

Code review (1 passe Sonnet adversariale)

# Severity Finding Action
H-001 HIGH Test panic sans DATABASE_URL ⏸️ Defer (convention projet — 152 tests font pareil)
M-001 MEDIUM Doc latence 350 → 150 ms ✅ Patché
M-002 MEDIUM u64 overflow à max_attempts ≥ 65 ✅ Patché (saturating cap 30s)
M-003 MEDIUM Commentaire tokio dep ambigu ✅ Patché (reword)
L-001 LOW Tracing sans request context ⏸️ Defer (caller responsibility)
L-002 LOW Mirror commit/no-commit doc ⏸️ Defer (pré-existant)
L-003 LOW Integration test non-determinist ⏸️ Defer (invariant suffit)
L-004 LOW SQLSTATE 40001 mention ✅ Patché
L-005 LOW 1205 lock_wait_timeout non retried ✅ Patché (Pattern 5 doc)

Verdict reviewer : GO.

Doc updates

docs/MULTI-TENANT-SCOPING-PATTERNS.md Pattern 5 :

  • KF-002-H-002 marqué resolved 2026-05-03.
  • Section deny-list endpoints divergents (vide).
  • 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).

Test plan

  • 5 tests unit/integration retry (success path, non-deadlock passthrough, max_attempts edge cases, real MariaDB deadlock recovery)
  • 152/152 kesh-db lib tests verts (release)
  • 18/18 onboarding e2e tests verts (incluant full_path_b_flow qui exercise finalize)
  • cargo clippy -D warnings clean
  • cargo fmt clean
  • 1 passe code review adversariale Sonnet → GO post-remediation
  • CI verts (Backend / Frontend / Docker)
  • Issue [KF-002-H-002] Deadlock-retry middleware for finalize multi-locks #43 fermée auto par closes #43

Suite — prep sprint Epic 8

Items restants après ce PR :

  1. ✅ Créer epic-8.md (PR docs(epic-8): create planning artifact + open CR-009/CR-010 #63)
  2. ✅ KF-020 [KF-020] invoices::update : passer en SELECT FOR UPDATE pour fermer la race no-op KF-004 résiduelle #49 SELECT FOR UPDATE (PR fix(db): KF-020 SELECT FOR UPDATE in invoices::update (closes #49) #64)
  3. ⏳ KF-002-H-002 [KF-002-H-002] Deadlock-retry middleware for finalize multi-locks #43 deadlock-retry → ce PR
  4. ⏳ Spike kesh-import crate design

🤖 Generated with Claude Code

guycorbaz and others added 2 commits May 3, 2026 01:07
@guycorbaz @claude
fix(db): KF-002-H-002 deadlock-retry helper + apply to finalize (closes
78ec5a2 
#43)

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) <noreply@anthropic.com>
@guycorbaz @claude
fix(db): KF-002-H-002 review remediation pass 1 (Sonnet)
8390fcb 
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) <noreply@anthropic.com>
@guycorbaz
guycorbaz merged commit 5893515 into main May 2, 2026
3 checks passed
@guycorbaz
guycorbaz deleted the fix/kf-002-h-002-deadlock-retry branch May 2, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@guycorbaz