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.
This commit is contained in:
@@ -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<WriteEntry>` 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
|
||||||
@@ -5,6 +5,12 @@ use std::io;
|
|||||||
pub enum SKError {
|
pub enum SKError {
|
||||||
Io(io::Error),
|
Io(io::Error),
|
||||||
Compression(niffler::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<T> = Result<T, SKError>;
|
pub type SKResult<T> = Result<T, SKError>;
|
||||||
@@ -14,6 +20,13 @@ impl fmt::Display for SKError {
|
|||||||
match self {
|
match self {
|
||||||
SKError::Io(e) => write!(f, "I/O error: {e}"),
|
SKError::Io(e) => write!(f, "I/O error: {e}"),
|
||||||
SKError::Compression(e) => write!(f, "compression 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 {
|
match self {
|
||||||
SKError::Io(e) => Some(e),
|
SKError::Io(e) => Some(e),
|
||||||
SKError::Compression(e) => Some(e),
|
SKError::Compression(e) => Some(e),
|
||||||
|
_ => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
use crate::error::SKResult;
|
use crate::error::{SKError, SKResult};
|
||||||
|
|
||||||
/// Statistics sidecar written alongside each SuperKmer file on close.
|
/// Statistics sidecar written alongside each SuperKmer file on close.
|
||||||
///
|
///
|
||||||
@@ -31,7 +31,10 @@ impl SKFileMeta {
|
|||||||
pub fn read(sk_path: &Path) -> SKResult<Option<Self>> {
|
pub fn read(sk_path: &Path) -> SKResult<Option<Self>> {
|
||||||
let path = Self::sidecar_path(sk_path);
|
let path = Self::sidecar_path(sk_path);
|
||||||
match std::fs::File::open(&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) if e.kind() == std::io::ErrorKind::NotFound => Ok(None),
|
||||||
Err(e) => Err(e.into()),
|
Err(e) => Err(e.into()),
|
||||||
}
|
}
|
||||||
@@ -40,7 +43,10 @@ impl SKFileMeta {
|
|||||||
pub(crate) fn write(&self, sk_path: &Path) -> SKResult<()> {
|
pub(crate) fn write(&self, sk_path: &Path) -> SKResult<()> {
|
||||||
let path = Self::sidecar_path(sk_path);
|
let path = Self::sidecar_path(sk_path);
|
||||||
let f = std::fs::File::create(&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(())
|
Ok(())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -218,23 +218,31 @@ fn read_idx(path: &Path) -> SKResult<(Vec<u8>, Vec<u32>)> {
|
|||||||
let data = std::fs::read(path).map_err(SKError::Io)?;
|
let data = std::fs::read(path).map_err(SKError::Io)?;
|
||||||
let mut pos = 0;
|
let mut pos = 0;
|
||||||
|
|
||||||
if &data[pos..pos + 4] != &MAGIC {
|
let magic_bytes = data.get(pos..pos + 4)
|
||||||
return Err(SKError::Io(std::io::Error::new(
|
.ok_or(SKError::Truncated { context: "unitig index: magic" })?;
|
||||||
std::io::ErrorKind::InvalidData,
|
if magic_bytes != &MAGIC {
|
||||||
"unitig index: bad magic",
|
return Err(SKError::BadMagic {
|
||||||
)));
|
expected: "UIDX",
|
||||||
|
got: magic_bytes.try_into().unwrap(),
|
||||||
|
});
|
||||||
}
|
}
|
||||||
pos += 4;
|
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;
|
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;
|
pos += n;
|
||||||
|
|
||||||
let mut packed_offsets = Vec::with_capacity(n + 1);
|
let mut packed_offsets = Vec::with_capacity(n + 1);
|
||||||
for _ in 0..=n {
|
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;
|
pos += 4;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user