Files

Michael Freno 96b63ebf20 FRE-5256: Review silent active run for Senior Engineer - false positive

- 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>

2026-05-13 17:00:12 -04:00

3.3 KiB

Raw Blame History

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)

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
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)

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
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
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)

Thread-unsafe rand jitter (internal/api/client.go:523)
- Uses math/rand without locking — concurrent calls may produce identical jitter
Missing error code constants
- SessionExpired (10005), TokenExpired (10006), AccountSuspended (10050), QuotaExceeded (10011)
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.

3.3 KiB Raw Blame History