Revisión de Código Rust — PR #47

MetricStream · feat: add user-defined alerting thresholds with DB persistence · Autor: Carlos Pérez · 18 Jun 2026

Veredicto
BLOQUEADO
3
Críticos
4
Altos
5
Medios
3
Archivos revisados
Diagnóstico — Herramientas cargo
$ cargo check Checking metricstream v0.5.2 Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.14s $ cargo clippy -- -D warnings error: unused import: `std::collections::HashMap` --> src/alerts.rs:3:5 | 3 | use std::collections::HashMap; | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the whole `use` item = note: `-D unused-imports` implied by `-D warnings` warning: variable does not need to be mutable --> src/ingestion.rs:84:9 error: aborting due to 1 previous error; 1 warning emitted ────────────────────────────────────────────────────── $ cargo fmt --check Diff in src/db.rs at line 67: - ); + ) ; ────────────────────────────────────────────────────── $ cargo test running 12 tests test alerts::tests::test_threshold_validation ... ok test alerts::tests::test_threshold_save ... ok test db::tests::test_alert_query_injection ... FAILED ... 9 tests passed FAILED (1 failed) ────────────────────────────────────────────────────── $ cargo-audit Fetching advisory database... Scanning Cargo.lock for vulnerabilities (184 crate dependencies) warning: 1 advisory found RUSTSEC-2024-0421: `openssl` 0.10.62 — use-after-free in SSL_free_buffers
Hallazgos priorizados
Crítico Inyección SQL por interpolación de strings src/db.rs : 67–72

El metric_name proveniente del usuario se interpola directamente en la query SQL con format!(). Cualquier usuario puede ejecutar SQL arbitrario contra TimescaleDB. La suite de tests tiene un caso específico que falla sobre esto (test_alert_query_injection).

Rust — src/db.rs❌ Problemático
67-    let query = format!(
68-        "SELECT * FROM alerts WHERE metric_name = '{}'",
69-        metric_name  // ← user-controlled input sin sanitizar
70-    );
71-    sqlx::query(&query).fetch_all(&*pool).await.unwrap()
Rust — src/db.rs✅ Corrección
67+    sqlx::query_as::<_, Alert>(
68+        "SELECT * FROM alerts WHERE metric_name = $1"
69+    )
70+    .bind(metric_name)
71+    .fetch_all(&*pool)
72+    .await
73+    .context("failed to fetch alerts for metric")?
✔ Usa consultas parametrizadas con sqlx::query_as y .bind(). Nunca interpolación de strings en SQL.
Crítico unsafe sin comentario SAFETY — operación sobre puntero raw src/alerts.rs : 112–119

Bloque unsafe que dereference un puntero crudo sin ningún comentario // SAFETY: que documente los invariantes. Es imposible auditar si la operación es segura y Clippy lo marca como error con -D warnings.

Rust — src/alerts.rs❌ Problemático
112-    unsafe {
113-        let val = *raw_ptr;
114-        process_threshold(val);
115-    }
Rust — src/alerts.rs✅ Corrección
112+    // SAFETY: `raw_ptr` es no-nulo y apunta a un ThresholdConfig que vive
113+    // al menos durante la duración de esta función (garantizado por el caller
114+    // en AlertManager::process, que mantiene ownership del Config).
115+    let val = unsafe { *raw_ptr };
116+    process_threshold(val);
✔ Añade un comentario // SAFETY: exhaustivo antes de todo bloque unsafe. Si no puedes justificarlo, refactoriza para eliminar el unsafe.
Crítico unwrap() en ruta de producción — panic garantizado en error de DB src/db.rs : 71, src/ingestion.rs : 38

Múltiples llamadas a .unwrap() sobre Result de operaciones de base de datos y de parse. Una pérdida de conexión o un valor malformado derriba el proceso completo.

Rust — src/ingestion.rs❌ Problemático
38-    let threshold: f64 = value.parse().unwrap();
39-    let saved = db::save_alert(&pool, threshold).await.unwrap();
Rust — src/ingestion.rs✅ Corrección
38+    let threshold: f64 = value.parse()
39+        .map_err(|e| AppError::InvalidThreshold(e.to_string()))?;
40+    let saved = db::save_alert(&pool, threshold).await
41+        .context("failed to persist alert threshold")?;
✔ Usa ? con el operador de propagación y .context() de anyhow (o map_err con thiserror) en toda ruta de producción.
Alto Canal mpsc sin bounds — riesgo de consumo ilimitado de memoria src/ingestion.rs : 22

Se usa tokio::sync::mpsc::unbounded_channel() para pasar métricas al procesador. Con un pico de tráfico, la cola crece sin límite y puede agotar la RAM del contenedor.

Rust — src/ingestion.rs❌ Problemático
22-    let (tx, rx) = tokio::sync::mpsc::unbounded_channel::<Metric>();
Rust — src/ingestion.rs✅ Corrección
22+    // Bounded: aplica back-pressure al productor antes de OOM
23+    let (tx, rx) = tokio::sync::mpsc::channel::<Metric>(1_024);
✔ Cambia a canal acotado con capacidad razonable. Documenta la decisión del tamaño del buffer.
Alto Bloqueo en contexto async — std::thread::sleep en handler tokio src/alerts.rs : 89

std::thread::sleep(Duration::from_millis(500)) dentro de una función async bloquea el thread de tokio, impidiendo que otras tareas progresen en ese worker.

Rust — src/alerts.rs❌ Problemático
89-    std::thread::sleep(Duration::from_millis(500)); // "debounce" artesanal
Rust — src/alerts.rs✅ Corrección
89+    tokio::time::sleep(Duration::from_millis(500)).await; // yield al runtime
✔ En código async, usa siempre equivalentes tokio::time::sleep (y tokio::fs, tokio::net, etc.).
Alto String en lugar de &str en firma pública — copia innecesaria src/alerts.rs : 15, 42

Funciones públicas que reciben String obligando al caller a ceder ownership (o a clonar). Aceptar &str o impl AsRef<str> es más idiomático y evita copias.

Rust — src/alerts.rs❌ → ✅
15-pub fn validate_metric_name(name: String) -> bool {
15+pub fn validate_metric_name(name: &str) -> bool {
Alto Función de 78 líneas — demasiado grande para revisar y mantener src/alerts.rs : 55–133

AlertManager::evaluate_and_persist() tiene 78 líneas mezclando validación, lógica de negocio, llamada a BD y notificación. Límite del equipo: 50 líneas.

✔ Extrae: validate_threshold(), persist_alert() y notify_channel() como funciones independientes con responsabilidad única.
Medio Vec::new() sin with_capacity en bucle de ingesta src/ingestion.rs : 61

Se crea un Vec<Alert> por cada batch de métricas sin reservar capacidad, provocando múltiples reallocaciones en producción con batches de 1 000+ métricas.

Rust — src/ingestion.rs❌ → ✅
61-    let mut alerts = Vec::new();
61+    let mut alerts = Vec::with_capacity(metrics.len());
Medio Dependencia vulnerable — openssl 0.10.62 (RUSTSEC-2024-0421) Cargo.lock (transitiva desde reqwest 0.11)

cargo audit detecta un use-after-free en la versión transitiva de OpenSSL. Aunque es una dependencia indirecta, el servicio sirve TLS.

✔ Actualiza reqwest a 0.12+ (usa rustls por defecto) o fija openssl = "0.10.66" en [dependencies] con override hasta que reqwest actualice.
Resumen de hallazgos
Severidad Categoría Descripción Archivo Estado
Crítico Seguridad Inyección SQL por interpolación de strings src/db.rs:67 🚫 BLOQUEA
Crítico Seguridad unsafe sin comentario SAFETY src/alerts.rs:112 🚫 BLOQUEA
Crítico Error Handling unwrap() en rutas de producción (x3) src/db.rs:71, src/ingestion.rs:38 🚫 BLOQUEA
Alto Concurrencia Canal mpsc sin bounds src/ingestion.rs:22 ⚠ BLOQUEA
Alto Concurrencia std::thread::sleep en contexto async src/alerts.rs:89 ⚠ BLOQUEA
Alto Ownership String en lugar de &str en API pública src/alerts.rs:15,42 ⚠ BLOQUEA
Alto Calidad Función de 78 líneas (límite: 50) src/alerts.rs:55 ⚠ BLOQUEA
Medio Rendimiento Vec::new() sin with_capacity en bucle src/ingestion.rs:61 — advertencia
Medio Dependencias openssl vulnerable (RUSTSEC-2024-0421) Cargo.lock — advertencia
Medio Calidad Importación no usada (HashMap) rompe clippy -D warnings src/alerts.rs:3 — advertencia
Medio Formato cargo fmt --check falla en db.rs:67 src/db.rs:67 — advertencia
Medio Docs AlertManager pub sin documentación /// src/alerts.rs:10 — advertencia
Generado por Revisor Experto de Código Rust · CULTIVA IA cargo 1.84.0 · clippy 0.1.84 · cargo-audit 0.21.1 · 18 Jun 2026 14:32 UTC