Michael Freno
96b63ebf20
- Senior Engineer run 8f0979ee on FRE-4807 silent for 1h (suspicious threshold) - Run was automation/system triggered after pending ci.yml security fixes were already completed by CTO at 19:07 UTC - Zero output sequences because run had no actionable scope - FRE-5256 marked done with false positive disposition - FRE-4807 reassigned to Security Reviewer for ci.yml re-review Co-Authored-By: Paperclip <noreply@paperclip.ing>
81 lines
3.3 KiB
Markdown
81 lines
3.3 KiB
Markdown
# Code Review: FRE-4764 — Retry Logic, Rate Limiting, Error Handling
|
|
|
|
**Reviewer**: Code Reviewer (f274248f-c47e-4f79-98ad-45919d951aa0)
|
|
**Date**: 2026-05-13
|
|
**Status**: Changes requested
|
|
|
|
## Files Reviewed
|
|
|
|
- `internal/api/client.go` (553 lines)
|
|
- `internal/mail/client_test.go` (1390 lines)
|
|
|
|
## Implementation Assessment
|
|
|
|
### What Was Done Well
|
|
- Structured error codes match go-proton-api pattern
|
|
- NetError type with proper Unwrap()/Is() for error classification
|
|
- Status/StatusObserver pattern for connection monitoring
|
|
- APIHVDetails struct for human verification error parsing
|
|
- RetryConfig with sensible defaults
|
|
- executeWithRetry with exponential backoff, jitter, and Retry-After header parsing
|
|
- RateLimiter sliding window implementation
|
|
- All 53 test routes correctly mapped to `/mail/v4/messages/*` endpoints
|
|
- HTTP methods corrected (GET for GetMessage, PUT for UpdateDraft/MoveToTrash, DELETE for PermanentlyDelete)
|
|
|
|
### Issues Found
|
|
|
|
#### P1 — Critical (2 issues)
|
|
|
|
1. **Resource leak on retry exhaustion** (`internal/api/client.go:418-440`)
|
|
- When all retries exhausted with `lastErr` set, `lastResp.Body` is never closed
|
|
- Response body leak on network failure paths
|
|
|
|
2. **Context cancellation response leak** (`internal/api/client.go:343-344`)
|
|
- When context cancelled during retry backoff delay, `lastResp.Body` is leaked
|
|
- `return lastResp, ctx.Err()` without closing body
|
|
|
|
#### P2 — High (3 issues)
|
|
|
|
3. **Unreachable code in `shouldRetryError`** (`internal/api/client.go:465-486`)
|
|
- `NetError` check (line 471-473) is unreachable
|
|
- `net.OpError` check (line 476-478) always matches first via `errors.As` unwrapping
|
|
- Dead code that confuses maintainability
|
|
|
|
4. **RateLimiter `Wait()` GC pressure** (`internal/api/client.go:277-298`)
|
|
- Creates new slice on every call instead of in-place filtering
|
|
- High throughput scenarios generate significant GC pressure
|
|
|
|
5. **Race condition on auth refresh retry** (`internal/api/client.go:381-386`)
|
|
- Retry response body not closed when `doSingleRequest` fails after auth refresh
|
|
|
|
#### P3 — Minor (3 issues)
|
|
|
|
6. **Thread-unsafe rand jitter** (`internal/api/client.go:523`)
|
|
- Uses `math/rand` without locking — concurrent calls may produce identical jitter
|
|
|
|
7. **Missing error code constants**
|
|
- SessionExpired (10005), TokenExpired (10006), AccountSuspended (10050), QuotaExceeded (10011)
|
|
|
|
8. **Test route ambiguity** (`internal/mail/client_test.go:72-82`)
|
|
- `POST /mail/v4/messages` matches multiple operations via generic handler
|
|
- Fragile if new routes added without corresponding mux registrations
|
|
|
|
### Test Coverage Gaps (P2)
|
|
- No retry logic tests (backoff, jitter, Retry-After parsing)
|
|
- No connection monitoring tests (StatusUp/StatusDown transitions)
|
|
- No HV handling tests (GetHVDetails, IsHVError)
|
|
- No rate limiter tests
|
|
- No concurrent auth refresh test
|
|
|
|
## Recommendation
|
|
|
|
**P1 issues must be fixed before passing.** Response body leaks are serious resource leaks that will cause connection pool exhaustion under failure conditions.
|
|
|
|
**P2 issues should be addressed in follow-up.** Unreachable code and GC pressure are important but not blocking.
|
|
|
|
**P3 issues can be deferred.** Missing constants and thread safety are low priority.
|
|
|
|
## Disposition
|
|
|
|
**Changes requested** — Reassigned to Senior Engineer for P1 fixes.
|