From e008a8beb45421edc49d144ec85378f856aa98b5 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sat, 9 May 2026 18:26:08 +0800 Subject: [PATCH] harden obiskio error handling with explicit variants and bounds checking Extend `SKError` with `BadMagic`, `Truncated`, and `InvalidData` variants to replace `expect()` calls and unsafe slice indexing. Implement proper error chaining via `source()` and update `Display` formatting. Improve magic byte validation and serde error mapping for clearer debugging. Documents a broader roadmap for further crate hardening, including LRU eviction, concurrency, and benchmarking. --- .kilo/plans/1778319532254-eager-panda.md | 87 ++++++++++++++++++++++++ src/obiskio/src/error.rs | 14 ++++ src/obiskio/src/meta.rs | 12 +++- src/obiskio/src/unitig_index.rs | 24 ++++--- 4 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 .kilo/plans/1778319532254-eager-panda.md diff --git a/.kilo/plans/1778319532254-eager-panda.md b/.kilo/plans/1778319532254-eager-panda.md new file mode 100644 index 0000000..b6c6513 --- /dev/null +++ b/.kilo/plans/1778319532254-eager-panda.md @@ -0,0 +1,87 @@ +# Plan d'amélioration technique - obiskio + +## 1. Contexte et objectifs +- **Objectif** : Renforcer la robustesse, la maintenabilité et les performances de la crate `obiskio`. +- **Priorités** : + 1. Gestion des erreurs + 2. Optimisation de la mémoire du pool + 3. Robustesse concurrente + 4. Couverture de tests + 5. Documentation + +--- + +## 2. Axes d'amélioration détaillés + +### 2.1 Gestion des erreurs +- **Problème** : `SKError` ne couvre pas tous les cas (format invalide, taille maximale, CRC) +- **Actions** : + - Ajouter variante `ParseError(String)` dans `src/error.rs` + - Valider les tailles de SuperKmer avant parsing + - Remplacer `expect()` par `unwrap_or_else` avec messages explicites + - Documenter chaque variante d’erreur dans le README + +### 2.2 Optimisation du pool de fichiers +- **Problème** : `SKFilePool` utilise un `Vec` non contraint et n’effectue pas de nettoyage en cas d’erreur +- **Actions** : + - Implémenter un `LimitedVec` avec limite stricte à `MAX_POOL_SIZE` + - Créer `clear_memory()` qui supprime les entrées orphelines + - Ajouter `evict_lru_threshold()` pour éviction proactive + - Introduire un `RwLock` pour les opérations de lecture massives + +### 2.3 Robustesse concurrente +- **Problème** : Risque de deadlocks dans `SKFileWriter::write_batch()` et `SKFileReader::reopen_and_seek()` +- **Actions** : + - Remplacer `Mutex` par `RwLock` pour les accès en lecture + - Ajouter un compteur de blocage et logs de timeout + - Utiliser `std::thread::park_timeout` pour débloquer + - Insérer `debug_assert!` sur les états invariants + +### 2.4 Couverture de tests +- **Problème** : Absence de benchmarks, de tests de migration, de résilience de fichiers corrompus +- **Actions** : + - Benchmarks I/O sur 10k+ SuperKmer avec `criterion` + - Tests de migration de version de fichier `.meta` → `.v2.meta` + - Tests de corruption volontaire (truncature, inversion de bits) + - Tests de stress sur pool saturation (100 threads) + +### 2.5 Documentation & exemples +- **Actions** : + - Ajouter des examples dans chaque module (`# Examples`) + - Documenter la logique LRU avec diagrammes Mermaid + - Créer un guide « How to recover from eviction » + - Mettre à jour le `README.md` avec tableau des variantes d’erreur + +--- + +## 3. Plan d'exécution (Roadmap) + +| Sprint | Durée | Livrables clés | +|--------|-------|----------------| +| **S1** | 2 jours | Refactorisation `SKError`, ajout de tests unitaires | +| **S2** | 3 jours | Implémentation `clear_memory()` + `LimitedVec` | +| **S3** | 2 jours | Passage à `RwLock`, ajout de compteurs de blocage | +| **S4** | 2 jours | Benchmarks + tests de migration | +| **S5** | 1 jour | Documentation finale & mise à jour du README | + +--- + +## 4. Dépendances externes +- Mettre à jour `niffler` vers la version 2.0 (performance compression) +- Évaluer `bincode` vs `serde_json` pour les métas (I/O) +- Ajouter dépendance `criterion` (dev‑dependencies) + +--- + +## 5. KPI de suivi +- **Couverture de tests** : ≥85 % des chemins critiques +- **Latence moyenne d’écriture** : ↓15 % après optimisation du pool +- **Taux d’erreurs résolues** : 100 % des nouvelles variantes couvertes +- **Temps de build CI** : ≤5 min pour l’ensemble des benchmarks + +--- + +## 6. Validation finale +- Revue de code avec `cargo clippy -- -D warnings` +- Analyse de toxicité avec `cargo deny open-source-licenses` +- Vérification de la conformité aux standards de naming du projet diff --git a/src/obiskio/src/error.rs b/src/obiskio/src/error.rs index 969ec3d..135cba2 100644 --- a/src/obiskio/src/error.rs +++ b/src/obiskio/src/error.rs @@ -5,6 +5,12 @@ use std::io; pub enum SKError { Io(io::Error), Compression(niffler::Error), + /// Binary file has unrecognized magic bytes. + BadMagic { expected: &'static str, got: [u8; 4] }, + /// File is too short to contain a required field. + Truncated { context: &'static str }, + /// A field value is outside its valid range or cannot be parsed. + InvalidData { context: &'static str, detail: String }, } pub type SKResult = Result; @@ -14,6 +20,13 @@ impl fmt::Display for SKError { match self { SKError::Io(e) => write!(f, "I/O error: {e}"), SKError::Compression(e) => write!(f, "compression error: {e}"), + SKError::BadMagic { expected, got } => { + write!(f, "bad magic in {expected}: expected {:?}, got {:?}", expected.as_bytes(), got) + } + SKError::Truncated { context } => write!(f, "truncated file: {context}"), + SKError::InvalidData { context, detail } => { + write!(f, "invalid data in {context}: {detail}") + } } } } @@ -23,6 +36,7 @@ impl std::error::Error for SKError { match self { SKError::Io(e) => Some(e), SKError::Compression(e) => Some(e), + _ => None, } } } diff --git a/src/obiskio/src/meta.rs b/src/obiskio/src/meta.rs index a4887d7..95ca422 100644 --- a/src/obiskio/src/meta.rs +++ b/src/obiskio/src/meta.rs @@ -1,6 +1,6 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; -use crate::error::SKResult; +use crate::error::{SKError, SKResult}; /// Statistics sidecar written alongside each SuperKmer file on close. /// @@ -31,7 +31,10 @@ impl SKFileMeta { pub fn read(sk_path: &Path) -> SKResult> { let path = Self::sidecar_path(sk_path); match std::fs::File::open(&path) { - Ok(f) => Ok(Some(serde_json::from_reader(f).map_err(std::io::Error::other)?)), + Ok(f) => Ok(Some(serde_json::from_reader(f).map_err(|e| SKError::InvalidData { + context: "SKFileMeta sidecar", + detail: e.to_string(), + })?)), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), Err(e) => Err(e.into()), } @@ -40,7 +43,10 @@ impl SKFileMeta { pub(crate) fn write(&self, sk_path: &Path) -> SKResult<()> { let path = Self::sidecar_path(sk_path); let f = std::fs::File::create(&path)?; - serde_json::to_writer_pretty(f, self).map_err(std::io::Error::other)?; + serde_json::to_writer_pretty(f, self).map_err(|e| SKError::InvalidData { + context: "SKFileMeta sidecar", + detail: e.to_string(), + })?; Ok(()) } } diff --git a/src/obiskio/src/unitig_index.rs b/src/obiskio/src/unitig_index.rs index dc674fd..7bb7b5c 100644 --- a/src/obiskio/src/unitig_index.rs +++ b/src/obiskio/src/unitig_index.rs @@ -218,23 +218,31 @@ fn read_idx(path: &Path) -> SKResult<(Vec, Vec)> { let data = std::fs::read(path).map_err(SKError::Io)?; let mut pos = 0; - if &data[pos..pos + 4] != &MAGIC { - return Err(SKError::Io(std::io::Error::new( - std::io::ErrorKind::InvalidData, - "unitig index: bad magic", - ))); + let magic_bytes = data.get(pos..pos + 4) + .ok_or(SKError::Truncated { context: "unitig index: magic" })?; + if magic_bytes != &MAGIC { + return Err(SKError::BadMagic { + expected: "UIDX", + got: magic_bytes.try_into().unwrap(), + }); } pos += 4; - let n = u32::from_le_bytes(data[pos..pos + 4].try_into().unwrap()) as usize; + let n_bytes = data.get(pos..pos + 4) + .ok_or(SKError::Truncated { context: "unitig index: n_unitigs" })?; + let n = u32::from_le_bytes(n_bytes.try_into().unwrap()) as usize; pos += 4; - let seqls = data[pos..pos + n].to_vec(); + let seqls = data.get(pos..pos + n) + .ok_or(SKError::Truncated { context: "unitig index: seqls" })? + .to_vec(); pos += n; let mut packed_offsets = Vec::with_capacity(n + 1); for _ in 0..=n { - packed_offsets.push(u32::from_le_bytes(data[pos..pos + 4].try_into().unwrap())); + let off_bytes = data.get(pos..pos + 4) + .ok_or(SKError::Truncated { context: "unitig index: packed_offsets" })?; + packed_offsets.push(u32::from_le_bytes(off_bytes.try_into().unwrap())); pos += 4; }