fix: add retry with exponential backoff to BricksApiAdapter and improve logging

[Kaiohz]

Summary

• Add 3 retries with exponential backoff (2s, 4s, 8s) for connection/SSL/timeout errors on _get() and _post() to handle transient SSL handshake timeouts from Cloudflare
• Replace logger.debug → logger.info throughout adapter for better observability in container logs
• Replace logger.error → logger.exception in except blocks to capture stack traces

Context

SSL handshake timeouts from the K3s cluster to Railway (behind Cloudflare) were causing transient failures. Retry logic with backoff gives the connection a chance to recover on retry.

Test plan

• All 33 unit tests pass with updated mock side_effects for retry behavior
• time.sleep is mocked in tests to avoid delays
• Deploy and monitor container logs for retry messages during SSL timeouts

[Kaiohz]

🔍 Code Review — PR #42

Score : 7/10

Bonne PR dans l'ensemble : le problème est réel (SSL handshake timeout Cloudflare), la solution est pragmatique, et les tests sont mis à jour. Quelques points à améliorer ci-dessous.

---

✅ Ce qui est bien

• Le problème est bien ciblé : les timeouts SSL Cloudflare sont des erreurs transitoires classiques, le retry avec backoff est la bonne réponse.
• Constantes extraites (_MAX_RETRIES, _RETRY_BACKOFF) : propre et configurable.
• logger.exception() au lieu de logger.error() : capture les stack traces, c'est un vrai gain d'observabilité.
• Tests mis à jour : les side_effect changés en listes de _MAX_RETRIES éléments + mock de time.sleep → pas de delays dans les tests.
• Backoff exponentiel (2s, 4s, 8s) : raisonnable pour des erreurs transitoires réseau.

---

⚠️ Points à améliorer

1. _is_retryable() est définie… mais jamais utilisée 🐛

@staticmethod
def _is_retryable(exc: Exception) -> bool:
    ...

Cette méthode est clean et bien pensée, mais elle n'est appelée nulle part. Dans _get() et _post(), la logique de retry est inline :

• Pour HTTPError : e.code >= 500 ✅ cohérent
• Pour Exception (catch-all) : toutes les exceptions sont retryées, y compris des erreurs qui ne devraient pas l'être (ex: ValueError, KeyError, TypeError)

Suggestion : Utiliser _is_retryable() dans les except blocks, ou la supprimer si le comportement actuel est intentionnel. Le catch-all Exception qui retry tout est risqué.

2. time.sleep() dans un contexte synchrone appelé via asyncio.to_thread()

_get() et _post() sont des méthodes synchrones appelées via asyncio.to_thread(). Le time.sleep() bloque un thread du ThreadPool. Ça fonctionne, mais :

• En production sous load, ça consomme des threads du pool
• L'approche idéale serait un async adapter avec asyncio.sleep(), mais c'est un refactor plus large

Pour l'instant c'est acceptable, mais à garder en tête pour une v2 async.

3. La dernière erreur est avalée dans certains cas

À la fin de la boucle for, si attempt == _MAX_RETRIES :

except Exception as e:
    if attempt < _MAX_RETRIES:  # False → on skip le continue
        ...
    logger.exception(...)  # ✅ logged
    raise                     # ✅ raised

Mais pour HTTPError avec e.code >= 500 :

if e.code >= 500 and attempt < _MAX_RETRIES:  # False → on skip
    last_exc = e
    time.sleep(...)
    continue
logger.exception(...)  # ✅ logged
raise                   # ✅ raised

Ça marche, mais last_exc n'est jamais levé dans le cas nominal (la boucle se termine via return avant). Le raise last_exc à la fin n'est atteint que si la boucle se termine sans raise intermédiaire — ce qui ne peut pas arriver avec la logique actuelle. C'est du code mort inoffensif mais à clarifier.

4. logger.info() pour le debug de filename extraction ⚠️

logger.info("Filename from Content-Disposition (quoted): %s", filename)

Ces logs _extract_filename() étaient en debug, les passer en info va spammer les logs en production. C'est du debug de parsing, pas de l'observabilité métier.

Suggestion : Remettre ces 3 logs en logger.debug().

5. Pas de jitter dans le backoff

En cas de burst de failures simultanées, tous les retries partent en même temps (thundering herd). Un petit jitter randomisé éviterait ça.

import random
time.sleep(_RETRY_BACKOFF ** attempt + random.uniform(0, 1))

6. Les 4xx autres que 401/403/404 ne sont pas loggués en warning

Dans les méthodes async callers (list_project_documents, etc.), seul 401/403/404 sont gérés. Un 429 (rate limit) ou 400 (bad request) tombe dans le RuntimeError générique sans retry. Pas critique ici car le retry est dans _get/__post, mais ça mériterait un logger.warning pour 429.

---

📋 Résumé des actions suggérées

#	Action	Priorité
1	Utiliser _is_retryable() ou la supprimer	🔴 Haute
2	Remettre _extract_filename logs en debug	🟡 Moyenne
3	Ajouter du jitter dans le backoff	🟡 Moyenne
4	Retirer le raise last_exc mort à la fin des méthodes	🟢 Basse
5	Logger les 429 en warning	🟢 Basse
6	Refactor async à terme	🔵 Future

---

Bonne base, le point 1 est le seul vrai bug (méthode inutilisée). Le reste est de l'amélioration progressive. 👍

[Kaiohz]

🔍 Code Review — PR #42

Score : 7.5/10

Bonne PR qui adresse un vrai problème (SSL handshake timeouts Cloudflare). La logique de retry est solide dans les grandes lignes, mais il y a quelques problèmes de cohérence et des améliorations notables à faire.

---

✅ Ce qui est bien

• Problème bien ciblé : les SSL handshake timeouts Cloudflare sont des erreurs transitoires classiques, le retry avec backoff est la bonne réponse
• logger.exception() au lieu de logger.error() : capture les stack traces, vrai gain d'observabilité
• _is_retryable() est bien pensée : distingue erreurs retryable (5xx, URLError, ConnectionError, TimeoutError, OSError) vs non-retryable
• Constantes extraites (_MAX_RETRIES, _RETRY_BACKOFF) : propre et facilement configurable
• Jitter aléatoire (random.uniform(0, 1)) dans le backoff : évite le thundering herd
• 429 logging : bon ajout pour la visibilité du rate limiting
• Tests mis à jour : side_effect en listes, mock de time.sleep et random.uniform
• Test test_does_not_retry_non_retryable_exception : vérifie que les erreurs non-retryable ne sont pas retryées, bien vu

---

🔴 Problème : 429 est loggué en warning mais pas retryé

Dans les méthodes list_project_documents, download_document, et publish_section_version, un 429 est loggué en warning puis re-raise en RuntimeError. Mais dans _get() / _post(), un 429 (e.code == 429 < 500) n'est pas retryable selon _is_retryable() car 429 < 500.

C'est incohérent : on log le 429 comme un problème transitoire (ce qu'il est) mais on ne le retry pas.

Suggestion : Ajouter 429 comme code retryable dans _is_retryable() :

@staticmethod
def _is_retryable(exc: Exception) -> bool:
    if isinstance(exc, urllib.error.HTTPError):
        return exc.code >= 500 or exc.code == 429
    if isinstance(exc, (urllib.error.URLError, ConnectionError, TimeoutError, OSError)):
        return True
    return False

Et du coup, retirer les if e.code == 429: logger.warning(...) redondants dans les méthodes appelantes, car le retry dans _get()/_post() s'en chargera.

🟡 logger.info() pour les appels HTTP — trop verbeux en production

Le passage de logger.debug → logger.info pour les logs de requêtes/réponses HTTP va spammer les logs en production :

• GET https://api.bricks.example.com/... à chaque appel
• GET ... -> 1234 bytes (status=200) à chaque réponse

En haute fréquence (indexation de documents), ça peut générer des volumes de logs importants.

Suggestion : Garder logger.info pour les retries/warnings/erreurs, mais remettre les logs de succès en logger.debug :

logger.debug("GET %s", url)  # succès
logger.info("GET %s -> retry attempt %d/%d", ...)  # retry
logger.exception("GET %s -> HTTP %d: %s", ...)  # erreur finale

🟡 Duplication de code entre _get() et _post()

La logique de retry est identique dans les deux méthodes, à quelques détails près (construction de la requête). C'est ~30 lignes dupliquées.

Suggestion : Extraire une méthode _request_with_retry(method, url, ...) pour éviter la duplication et faciliter les évolutions futures (ajout de headers de retry, circuit breaker, etc.).

🟢 Le raise RuntimeError final est du code défensif inaccessible

À la fin de _get() et _post(), après la boucle :

raise RuntimeError(f"GET {url} failed after {_MAX_RETRIES} retries")

Ce code est en pratique inaccessible : si on arrive à la fin de la boucle sans return, ça veut dire que le dernier attempt a levé une exception, qui sera déjà raise dans le except block. Le RuntimeError ne sera jamais atteint.

C'est un safety net raisonnable (défensif), mais un commentaire serait plus clair sur le fait que c'est du code défensif.

🟢 Suppression des docstrings de test

Les docstrings des tests ("""Should call GET...""") ont été supprimées. C'est un choix de style acceptable (les noms de tests sont assez descriptifs), mais ça réduit la lisibilité pour quelqu'un qui découvre le code. Pas bloquant.

---

📋 Résumé des actions

#	Action	Priorité
1	Ajouter 429 comme retryable dans _is_retryable()	🔴 Haute
2	Extraire _request_with_retry() pour éliminer la duplication	🟡 Moyenne
3	Remettre les logs de succès en debug, garder info pour retries/erreurs	🟡 Moyenne
4	Clarifier le raise RuntimeError final (code défensif)	🟢 Basse
5	Considérer un refactor async pour éviter time.sleep() bloquant	🔵 Future

Le point 1 est le seul vrai bug fonctionnel (429 non retryé alors qu'il devrait l'être). Le reste est de l'amélioration progressive.

Bonne PR dans l'ensemble 👍