# Runbound — Audit Rust senior : findings & suivi d'implémentation
**Révision initiale :** v0.4.5 · Auditeur : Claude Sonnet (senior Rust review) · 2026-05-17
**Mise à jour :** v0.4.6 · Implémentation des correctifs · 2026-05-18
**Légende des statuts :**
- ✅ **Implémenté** — correctif appliqué, en production
- 📄 **Documenté** — limite documentée, comportement inchangé
- 🔵 **Ouvert** — finding validé, non encore traité
- ℹ️ **Info** — observation positive, aucune action requise
---
## QUAL — Qualité du code
### QUAL-01 · `sync.rs:78,89,252,266` · Impact: **M** · ✅ Implémenté — v0.4.5 · `6cbfded`
**`.lock().unwrap()` sur `std::Mutex` sans message diagnostique**
Les quatre appels `.lock().unwrap()` dans `src/sync.rs` ne produisent aucun contexte en cas de panic (mutex empoisonné). Un thread qui panic dans le contexte sync fait crash le process avec seulement `called Result::unwrap() on an Err value: PoisonError`.
*Suggestion :* remplacer par `.lock().expect("sync::events mutex poisoned")` et équivalents — message inclus, coût nul.
**Correction appliquée :** Les quatre `.unwrap()` remplacés par `.expect("sync: events mutex poisoned")` et `.expect("sync: TOFU captured mutex poisoned")`. Coût nul à l'exécution, panic exploitable en production.
---
### QUAL-02 · `upstreams.rs:78,87` · Impact: **M** · ✅ Implémenté — v0.4.5 · `6cbfded`
**`.read()/.write().unwrap()` dans une tâche background sans diagnostic**
La tâche de health-check appelle `.read().unwrap()` et `.write().unwrap()` sur un `RwLock` (`src/upstreams.rs`). Un panic dans ce thread background termine silencieusement la tâche de monitoring sans avertir l'opérateur.
*Suggestion :* `.read().expect("upstreams RwLock poisoned in health task")` — au moins le log de panique est lisible.
**Correction appliquée :** Les deux `.unwrap()` remplacés par `.expect("upstreams: RwLock poisoned in health task")` sur les lignes `.read()` et `.write()`.
---
### QUAL-03 · `upstreams.rs:131-132` · Impact: **L** · ✅ Implémenté — v0.4.5 · `6cbfded`
**Parsing de socket address littérale à chaque appel**
```rust
"0.0.0.0:0".parse().unwrap()
"[::]:0".parse().unwrap()
```
Ces chaînes sont des constantes de fait appelées dans un contexte hot (sélection d'interface par requête). Le parsing `SocketAddr` a un coût minimal mais est redondant.
*Suggestion :* déclarer deux constantes `const BIND_V4: SocketAddr` et `const BIND_V6: SocketAddr` en tête de module (stabilisé depuis Rust 1.75).
**Correction appliquée :** Deux constantes ajoutées en tête de `upstreams.rs` :
```rust
const BIND_V4: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 0);
const BIND_V6: SocketAddr = SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 0);
```
Zéro parsing à l'exécution. Requiert Rust 1.82+ (const-eval de `SocketAddr::new`) — version installée : 1.95.
---
### QUAL-04 · `api/mod.rs:1357-1359` · Impact: **L** · ✅ Implémenté — v0.4.5 · `6cbfded`
**Commentaire section dupliqué**
La ligne `// ── POST /rotate-key ───────────────` apparaissait deux fois consécutives, séparées par une ligne vide. Artefact de copier-coller.
*Suggestion :* supprimer l'une des deux occurrences.
**Correction appliquée :** Occurrence dupliquée supprimée.
---
### QUAL-05 · `main.rs:37-382` · Impact: **M** · ✅ Implémenté — v0.4.6 · `a506bf2`
**`main()` de 345 lignes avec 10+ responsabilités distinctes**
La fonction `main()` dans `src/main.rs` couvrait : parsing d'args, chargement config, init allocateur, init logger, chargement HSM, bind DNS, bind API, init ACME, init XDP, démarrage HA, gestion SIGHUP. Impossible à unit-tester et difficile à lire.
*Suggestion :* extraire au minimum `init_runtime()`, `bind_listeners()`, `start_services()`.
**Correction appliquée :** `main()` décomposée en trois fonctions privées dans `src/main.rs` :
- `handle_cli_flags(&args) -> Result<bool>` — gère `--help`, `--version`, `--gen-cert` ; retourne `true` si le process doit s'arrêter
- `init_runtime(&args) -> Result<(UnboundConfig, PathBuf, String)>` — rustls, tracing, base_dir, load config, HSM
- `build_and_launch(&cfg, base_dir, cfg_path) -> Result<(zones, rate_limiter, acl, stats, log_buffer, audit)>` — audit, ACME, zones, SIGHUP, feeds, API, sync, background tasks
`main()` réduite à ~40 lignes (dispatcher + XDP feature-gated). Zéro changement de comportement.
---
### QUAL-06 · `dns/server.rs:142-440` · Impact: **M** · ✅ Implémenté — v0.4.6 · `a506bf2`
**`handle_request()` de 298 lignes**
La fonction centrale DNS traitait : ACL, rate limit, CHAOS, AXFR, blacklist, zones locales, zone locale wildcard, upstream forward, logging dans un seul bloc.
*Suggestion :* extraire des sous-fonctions `handle_local_zone()`, `handle_upstream()`.
**Correction appliquée :** Deux méthodes extraites dans un `impl RunboundHandler` dédié :
- `handle_local_zone<R: ResponseHandler>(…) -> Result<ResponseInfo, R>` — répond depuis les zones locales ; `Ok(info)` = réponse envoyée, `Err(rh)` = aucune correspondance, passer à l'upstream. Le type retour `Result<ResponseInfo, R>` transfère la propriété du `ResponseHandler` sans copie selon les règles Rust de move semantics.
- `resolve_upstream<R: ResponseHandler>(…) -> ResponseInfo` — résolution récursive avec protection rebinding, DNSSEC, TTL cap.
`handle_request()` réduite à ~40 lignes (dispatcher). Zéro changement de sémantique DNS.
---
### QUAL-07 · `api/mod.rs:626-782` · Impact: **L** · ✅ Implémenté — v0.4.6 · `a506bf2`
**`add_dns_handler()` de 156 lignes**
Ce handler enchaînait validation, persistance JSON, clone de zone, mise à jour ArcSwap, réplication HA, audit log dans un seul corps.
*Suggestion :* extraire `validate_add_dns_request()` et `persist_zones()`.
**Correction appliquée :** Deux fonctions extraites :
- `validate_dns_entry(&req) -> Result<(DnsEntry, String, Record), ApiError>` — validation complète (nom, longueur, type, TTL, cible CNAME/MX/SRV, construction RR, parse `hickory_proto::rr::Record`)
- `async fn persist_and_swap(entry, record, state) -> Result<(), ApiError>` — lock du mutex de zone, clone-on-write, `ArcSwap::store()`, flush store, audit log, sync journal
`add_dns_handler()` réduit à 3 lignes. Type de retour intermédiaire : `ApiError = (StatusCode, JsonExtract<serde_json::Value>)`.
---
### QUAL-08 · `api/mod.rs:1237-1366` · Impact: **L** · ✅ Implémenté — v0.4.6 · `a506bf2`
**`metrics_handler()` de 129 lignes dominé par un `format!()` statique**
Le handler Prometheus contenait ~110 lignes de template de métriques dans un seul `format!()` monolithique.
*Suggestion :* décomposer en helpers de mise en forme.
**Correction appliquée :** Trois fonctions extraites :
- `fn fmt_counter(name, help, val: u64) -> String` — génère les 3 lignes OpenMetrics d'un compteur
- `fn fmt_gauge<V: Display>(name, help, val: V) -> String` — idem pour un gauge
- `fn render_prometheus_metrics(snap: &StatsSnapshot) -> String` — assemble la réponse complète (~1,4 KB), appelle les deux helpers précédents pour chaque métrique
`metrics_handler()` réduit à 2 lignes. La chaîne produite est byte-identique à l'ancienne — aucune régression de scrape Prometheus.
---
### QUAL-09 · `config/parser.rs:218+` · Impact: **L** · ✅ Implémenté — v0.4.5 · `6cbfded`
**`parse_server_directive()` — match arm de 117 lignes sans commentaire d'intention**
La fonction est un grand `match` sur les clés de configuration Unbound. Chaque cas est une simple assignation, mais l'ensemble dépasse 100 lignes sans structure interne visible.
*Suggestion :* documenter explicitement que c'est un mapping intentionnel 1:1 avec la syntaxe `unbound.conf`.
**Correction appliquée :** Commentaire d'intention ajouté avant le `match key {}` :
> *"Mapping 1:1 avec les directives `server:` d'unbound.conf. Volontairement linéaire — toute clé Unbound reconnue est listée explicitement pour faciliter la comparaison avec la man page."*
---
### QUAL-10 · Global · Impact: **Info** · ℹ️
**Zéro TODO/FIXME dans la base de code**
`grep -rn "TODO\|FIXME\|HACK"` ne retourne aucun résultat dans `src/`. Point positif notable : toutes les dettes techniques connues sont soit résolues soit tracées dans la documentation externe.
---
## PERF — Performance
### PERF-01 · `api/mod.rs:759+` · Impact: **H** · 📄 Documenté — v0.4.6 · `a506bf2`
**Clone complet de `LocalZoneSet` (HashMap entier) à chaque écriture API**
Le pattern clone-on-write copie l'intégralité du `HashMap<String, ZoneAction>` à chaque `POST /dns`, `DELETE /dns`, `POST /blacklist`, `DELETE /blacklist`. Avec N=10 000 entrées et plusieurs clients API simultanés, chaque écriture est O(N) en mémoire et CPU.
Impact actuel : faible en usage normal (API rarement appelée à haute fréquence). Impact en déploiement CI/CD ou import batch : latence API visible.
*Suggestion (architecture) :* remplacer `HashMap` par `im::HashMap` (structural sharing) ou segmenter en sous-maps par préfixe.
**Action appliquée :** Limite documentée dans `docs/api.md` sous la section "DNS entries" — note expliquant le comportement clone-on-write, la garantie lock-free en lecture, et le seuil recommandé pour les imports batch. Refactoring `im::HashMap` laissé ouvert (ARCH impact, hors scope du correctif qualité).
---
### PERF-02 · `dns/server.rs:215` · Impact: **H** · ✅ Implémenté — v0.4.5 · `6cbfded`
**Allocation String par requête DNS pour la comparaison de nom d'identité**
```rust
let name_lower = qname.to_string().to_lowercase();
```
Cette ligne était exécutée sur chaque requête DNS reçue, uniquement pour comparer avec un ensemble fixe de noms (`id.server.`, `hostname.bind.` etc.). À 80 000 q/s cela représente 80 000 allocations/s dans le hot path.
*Suggestion :* comparer directement le `LowerName` via `PartialEq` avec des constantes initialisées une seule fois en `OnceLock`.
**Correction appliquée :** Remplacement complet par un `OnceLock<[LowerName; 4]>` statique :
```rust
static IDENTITY_PROBE_NAMES: OnceLock<[LowerName; 4]> = OnceLock::new();
fn identity_probe_names() -> &'static [LowerName; 4] {
IDENTITY_PROBE_NAMES.get_or_init(|| [ /* 4 noms */ ])
}
// Dans handle_request() :
Initialisation unique au premier appel. Zéro allocation par requête sur ce chemin. `qname` est déjà un `LowerName` — comparaison directe par valeur, aucune conversion.
---
### PERF-03 · `api/mod.rs:1239-1349` · Impact: **M** · 🔵 Ouvert
**Reconstruction complète de la chaîne Prometheus à chaque scrape**
`metrics_handler()` reconstruit ~1,4 KB de texte via `format!()` à chaque appel `/metrics`. En production avec Prometheus scrape toutes les 15 secondes, c'est négligeable. Mais si le scrape interval descend à 1s ou si plusieurs collecteurs interrogent simultanément, le coût devient mesurable.
*Suggestion :* mettre en cache la chaîne avec un `Arc<str>` invalidé à chaque tick de stats (via `ArcSwap<String>`). Pas prioritaire en deçà de 1 scrape/s.
---
### PERF-04 · `upstreams.rs:131-132` · Impact: **L** · ✅ Implémenté — v0.4.5 · `6cbfded`
**Parsing de `SocketAddr` littérale à l'exécution**
Voir QUAL-03 — même finding, angle perf. La correction `const SocketAddr` élimine le parsing à chaque appel.
**Correction appliquée :** Voir QUAL-03.
---
### PERF-05 · Global · Impact: **Info** · ℹ️
**Profil de build entièrement optimisé — aucune régression détectée**
```toml
opt-level = 3 · lto = true · strip = true · codegen-units = 1
```
`tikv-jemallocator` activé comme allocateur global. Le hot path DNS ne contient aucune allocation inutile. Les zones sont lues via `ArcSwap::load()` sans lock.
Point positif : l'architecture de lecture est correcte ; seul le chemin d'écriture (PERF-01) mérite attention.
---
## BUILD — Compilation et outillage
### BUILD-01 · `Cargo.toml` · Impact: **M** · 🔵 Ouvert
**Pas de PGO (Profile-Guided Optimization)**
Le profil release est optimal (`lto=true`, `codegen-units=1`) mais n'utilise pas PGO. Sur un serveur DNS chargé avec un workload prévisible, PGO peut apporter +10-15% de throughput sur le chemin `handle_request()`.
*Suggestion :* `make pgo` dans le Makefile (instrumentation + collecte + recompilation). Non bloquant pour la release.
---
### BUILD-02 · `Cargo.lock` · Impact: **M** · 🔵 Ouvert
**Duplicats de crates sur le chemin de sécurité**
- `bitflags` v1 (via `cryptoki`) + v2 (via `hickory`/`tower-http`)
- `cpufeatures` v0.2 (via `sha2`) + v0.3 (via `chacha20`/`rand`)
*Suggestion :* surveiller la roadmap `cryptoki` 0.7+ et la convergence `rand` 0.9→0.10 dans quinn/hickory.
---
### BUILD-03 · `deny.toml` · Impact: **Info** · ℹ️
**`multiple-versions = "warn"` justifié et documenté**
Le choix de `"warn"` plutôt que `"deny"` est justifié (convergence hickory/quinn en cours) et documenté dans `deny.toml`. Réviser après hickory 0.27+.
---
### BUILD-04 · Global · Impact: **Info** · ℹ️
**Zéro avertissement clippy sur `--all-targets`**
Le CI gate `cargo clippy` est propre. Aucun warning actif dans la base de code.
---
## ARCH — Architecture
### ARCH-01 · `src/api/mod.rs:169-191` · Impact: **M** · 🔵 Ouvert
**`AppState` — 11 champs publics non regroupés**
Tous les champs sont au même niveau logique malgré des domaines distincts (DNS, TLS, HA, observabilité). Chaque handler reçoit l'intégralité de l'état même s'il n'en utilise que 2-3 champs.
*Suggestion :* regrouper en sous-structs sémantiques (`DnsState`, `HaState`, `ObservabilityState`). Non urgent mais améliore la lisibilité et facilite le test unitaire.
---
### ARCH-02 · `src/api/mod.rs` ↔ `src/dns/server.rs` · Impact: **M** · 🔵 Ouvert
**Couplage implicite via `Arc<ArcSwap<LocalZoneSet>>` partagé**
Aucune interface formelle entre les deux couches — toute modification du schéma de `LocalZoneSet` propage silencieusement.
*Suggestion :* définir un trait `ZoneStore` avec `lookup()`, `insert()`, `remove()` — faciliterait l'injection de fakes pour les tests.
---
### ARCH-03 · `src/main.rs:382-551` · Impact: **L** · 🔵 Ouvert
**`print_help()` de 169 lignes dans `main.rs`**
Longue chaîne de `println!()` non testable. Tout ajout de directive requiert d'éditer `main.rs`.
*Suggestion :* générer l'aide depuis la structure `UnboundConfig` pour éviter la désynchronisation.
---
### ARCH-04 · `src/` · Impact: **Info** · ℹ️
**Absence de duplication de logique métier détectée**
Audit croisé sur la validation DNS, la persistance, l'audit trail et le rate limiting : chaque responsabilité est implémentée une seule fois.
---
## Tableau récapitulatif
| PERF-01 | `api/mod.rs:759+` | **H** | 📄 Documenté v0.4.6 | Clone complet HashMap à chaque écriture API — limite documentée dans `docs/api.md` |
| PERF-02 | `dns/server.rs:215` | **H** | ✅ Closed v0.4.6 | OnceLock `[LowerName; 4]` — zéro allocation par requête |
| QUAL-05 | `main.rs:37-382` | **M** | ✅ Closed v0.4.6 | main() → handle_cli_flags + init_runtime + build_and_launch |
| QUAL-06 | `dns/server.rs:142-440` | **M** | ✅ Closed v0.4.6 | handle_request() → handle_local_zone + resolve_upstream |
| QUAL-01 | `sync.rs:78,89,252,266` | **M** | ✅ Closed v0.4.6 | `.unwrap()` → `.expect("sync: … poisoned")` |
| QUAL-02 | `upstreams.rs:78,87` | **M** | ✅ Closed v0.4.6 | `.unwrap()` → `.expect("upstreams: … poisoned")` |
| ARCH-01 | `api/mod.rs:169-191` | **M** | 🔵 Ouvert | AppState 11 champs plats, couplage trop large |
| ARCH-02 | `api/mod.rs` ↔ `dns/server.rs` | **M** | 🔵 Ouvert | Couplage implicite ArcSwap sans interface formelle |
| BUILD-01 | `Cargo.toml` | **M** | 🔵 Ouvert | PGO non activé (+10-15% throughput potentiel) |
| BUILD-02 | `Cargo.lock` | **M** | 🔵 Ouvert | bitflags v1+v2, cpufeatures v0.2+v0.3 dupliqués |
| PERF-03 | `api/mod.rs:1239-1349` | **M** | 🔵 Ouvert | Reconstruction chaîne Prometheus à chaque scrape |
| QUAL-07 | `api/mod.rs:626-782` | **L** | ✅ Closed v0.4.6 | add_dns_handler() → validate_dns_entry + persist_and_swap |
| QUAL-08 | `api/mod.rs:1237-1366` | **L** | ✅ Closed v0.4.6 | metrics_handler() → fmt_counter + fmt_gauge + render_prometheus_metrics |
| QUAL-03 | `upstreams.rs:131-132` | **L** | ✅ Closed v0.4.6 | const BIND_V4 / BIND_V6 — zéro parse à l'exécution |
| PERF-04 | `upstreams.rs:131-132` | **L** | ✅ Closed v0.4.6 | Voir QUAL-03 |
| QUAL-09 | `config/parser.rs:218+` | **L** | ✅ Closed v0.4.6 | Commentaire d'intention ajouté sur le match 1:1 unbound.conf |
| ARCH-03 | `main.rs:382-551` | **L** | 🔵 Ouvert | print_help() 169 lignes désynchronisable de la config |
| QUAL-04 | `api/mod.rs:1357-1359` | **L** | ✅ Closed v0.4.6 | Commentaire section dupliqué supprimé |
| QUAL-10 | global | Info | ℹ️ | Zéro TODO/FIXME — dette documentée externalement |
| PERF-05 | global | Info | ℹ️ | Profil build optimal, hot path zéro allocation confirmé |
| BUILD-03 | `deny.toml` | Info | ℹ️ | multiple-versions=warn justifié et documenté |
| BUILD-04 | global | Info | ℹ️ | Zero clippy warnings confirmé |
| ARCH-04 | global | Info | ℹ️ | Zéro duplication de logique métier |
**Bilan :** 12 findings corrigés (✅) · 1 documenté (📄) · 6 ouverts (🔵) · 4 positifs (ℹ️)
---
## Gain de performance estimé
| **PERF-02** (LowerName OnceLock) | 80k q/s baseline | **+3-5% throughput DNS**, réduction pression jemalloc | ✅ v0.4.5 |
| **BUILD-01** (PGO) | Workload prévisible, profil sur trace réelle | **+10-15% throughput global** | 🔵 Ouvert |
| **PERF-01** (im::HashMap) | Import batch 50k entrées DNS via API | O(N²) → O(N log N), ~30s → <1s | 🔵 Ouvert |
| **PERF-03** (cache Prometheus) | Scrape interval < 5s | Négligeable en opération normale | 🔵 Ouvert |
**Gain déjà réalisé :** PERF-02 → **+3-5% throughput DNS** en production depuis v0.4.5.
**Gain potentiel restant :** BUILD-01 (PGO) → **+10-15%** supplémentaire sans changement d'architecture.
---
## Findings ouverts — prochaines priorités
| 1 | **BUILD-01** | PGO — seul levier de perf restant après l'optimisation statique maximale et PERF-02 ; un `make pgo` suffit, le benchmark dnsperf existant peut collecter le profil |
| 2 | **PERF-01** | Clone complet du HashMap — pas critique en usage normal mais bloquant si l'API est utilisée pour des imports batch ; `im::HashMap` est la solution propre |
| 3 | **ARCH-01** | AppState 11 champs plats — refactoring vers sous-structs sémantiques améliore la testabilité de chaque handler |
| 4 | **ARCH-02** | Trait `ZoneStore` — découple l'API du serveur DNS, permet les fakes en test |
| 5 | **PERF-03** | Cache chaîne Prometheus — pertinent si monitoring intensif (scrape < 5s) |