50 lines
1.8 KiB
Markdown
50 lines
1.8 KiB
Markdown
# Code Review: FRE-328 - Error Handling
|
|
|
|
## Verdict: APPROVED with suggestions
|
|
|
|
Reviewed all 3 files in `src/errors/`:
|
|
- `__init__.py` (33 lines)
|
|
- `pipeline_errors.py` (269 lines)
|
|
- `error_recovery.py` (376 lines)
|
|
|
|
## Strengths
|
|
✅ Well-designed exception hierarchy with context and recovery hints
|
|
✅ Comprehensive retry strategy with exponential backoff and jitter
|
|
✅ Graceful degradation for non-critical failures
|
|
✅ Central ErrorRecoveryManager for coordination
|
|
✅ Good use of TypeVar for generic decorators
|
|
|
|
## Suggestions (non-blocking)
|
|
|
|
### 1. pipeline_errors.py:134 - Operator precedence bug
|
|
```python
|
|
if not default_hint and "OOM" in message or "GPU" in message:
|
|
```
|
|
This evaluates as `if (not default_hint and "OOM" in message) or ("GPU" in message)` due to operator precedence. Should be:
|
|
```python
|
|
if not default_hint and ("OOM" in message or "GPU" in message):
|
|
```
|
|
|
|
### 2. error_recovery.py:56 - Import inside method
|
|
```python
|
|
def calculate_delay(self, attempt: int) -> float:
|
|
import random # Should be at module level
|
|
```
|
|
|
|
### 3. error_recovery.py:138 - Off-by-one in retry loop
|
|
```python
|
|
for attempt in range(max_retries + 1): # Runs max_retries + 1 times
|
|
```
|
|
The `should_retry` method uses 0-indexed attempts, which may cause confusion. Consider aligning with the max_retries count.
|
|
|
|
### 4. error_recovery.py:187-197 - Potential logic issue
|
|
```python
|
|
if is_critical and not self.strict_mode:
|
|
self.warnings.append(...) # Adds warning but still skips!
|
|
return True # Always returns True regardless of is_critical
|
|
```
|
|
When `is_critical=True` and `strict_mode=False`, a warning is added but the segment is still skipped. This may not be the intended behavior.
|
|
|
|
## Overall Assessment
|
|
Solid error handling implementation with comprehensive recovery strategies. The issues identified are minor.
|