Conversation
|
Pushed bdb2199 addressing two problems found in the retry loop: 1. Gmail 429/5xx were classified as non-retryable. With the pinned google-gmail1 7.0.0 / google-apis-common 8.0.0, 2. The retry budget was lifetime, not consecutive. Also: added unit tests covering the Verified: |
The retries counter increment was commented out, so the retry guard could never fire and any persistent error spun the loop forever with zero delay, restarting the full mailbox scan from page one each time. - Re-enable the retry counter and return an anyhow::Error after MAX_RETRIES attempts instead of panicking, for a clean nonzero exit. - Classify errors before retrying: only transient failures are retried (SQLite SQLITE_BUSY/SQLITE_LOCKED including extended codes, sqlx pool timeouts, and Gmail API 429/5xx responses); auth failures, other 4xx, IO errors, etc. fail fast on first occurrence. - Space retries with exponential backoff (1s doubling, capped at 32s) plus up to 1s of jitter via tokio::time::sleep. - Have work() resume from the last fully processed page's next_page_token on retry instead of restarting from page one. Closes #3
Two review findings on the retry loop: 1. Gmail 429/5xx responses were classified as non-retryable. With google-gmail1 7.0.0 / google-apis-common 8.0.0, the generated doit() code parses any non-success HTTP response body as JSON first and returns Error::BadRequest(value) when that succeeds. Gmail's 429 rate-limit and 5xx errors carry Google's standard JSON error envelope, so they never reached the Error::Failure arm and fell into is_transient's catch-all `false`, aborting the run with "giving up: error is not retryable". is_transient now also matches Error::BadRequest and reads the numeric `error.code` field from the envelope (429 or 5xx => transient); the Failure arm remains for non-JSON responses such as an HTML 502 from an intermediary. 2. The retry counter was never reset, so MAX_RETRIES bounded *lifetime* transient errors instead of consecutive ones: on a long scan, the 4th intermittent SQLITE_BUSY or Gmail 5xx would kill the run even if thousands of pages succeeded between failures. The counter now resets whenever the failing attempt advanced the resume token (i.e. made progress) since the previous attempt. Also adds unit tests for is_transient's Gmail error classification and removes the unused `tokio::task` import that failed `cargo clippy --all-targets -- -D warnings`.
|
Resolved the merge conflict with main by rebasing Conflict: No code changes beyond the conflict resolution — both commits were replayed as-is. Verification (post-rebase):
Force-pushed with |
Gmail delivers per-user rate limiting as HTTP 403 as well as 429: the API error guide documents 403 rateLimitExceeded and 403 userRateLimitExceeded (usageLimits domain) with the instruction to retry with exponential backoff. is_transient() only accepted 429/5xx from the JSON envelope, so a burst of messages_get calls that tripped the 250-quota-units/sec per-user limit surfaced a 403 BadRequest, classified it as fatal, and aborted the whole mailbox scan -- exactly the condition the retry machinery was built to survive. Now the BadRequest arm also treats 403 as transient when error.errors[*].reason is rateLimitExceeded/userRateLimitExceeded or error.status is RESOURCE_EXHAUSTED, while other 403s (e.g. insufficientPermissions, dailyLimitExceeded) still fail fast. Adds unit tests for both the retryable and fatal 403 variants.
|
Reviewed the retry/backoff work on this branch and found one remaining gap in the transient-error classifier, fixed in 0fbaf82. Found: Fixed: the Verified (nothing else needed changing): |
…d tests Rebasing onto main replayed the concurrency rework over three PRs that also changed main.rs; this restores their improvements on top of the concurrent architecture: - get_sender: single borrow-only case-insensitive pass over a header priority list (from, return-path), no clones, no panic on valueless headers (PR #6). - Error classification: unify the per-call is_retryable_error with the outer loop's is_transient. is_transient walks the anyhow chain and recognizes SQLite busy/locked and PoolTimedOut; is_transient_gmail keeps the union of both sides' Gmail coverage (JSON envelope 429/5xx, 403 rate-limit reasons and RESOURCE_EXHAUSTED, non-JSON Failure fallback, network-level errors) and remains the per-call retry predicate (PR #7). - Outer retry loop: fail fast on non-transient errors (consulting the recovered writer error, since a dead writer surfaces to work() only as a channel-closed send error), resume from the last fully processed page via a resume token instead of re-listing from page one, and use capped exponential backoff with jitter (PR #7). The writer quiesce, fresh-writer respawn, and progress-based retry reset are unchanged. - Tests: classification, cleanup_sender, get_sender, and SQLite counting suites (PRs #7, #13), with the DB tests adapted to the new signatures (mark_seen takes an id and reports newly-inserted, increment_sender_mails takes the cleaned sender) and run against the production init_schema DDL including the seen_mails unique index.
1 participant
Closes #3
What changed
The
retries += 1increment inmain()'s retry loop was commented out, so theretries > 3guard could never fire — any persistent error (stuck SQLite lock, revoked credentials, hard API failure) spun the loop forever with zero delay, restarting the full mailbox scan from page one each time.MAX_RETRIES(3) transient failures,main()returns ananyhow::Errorwith context (nonzero exit) instead ofpanic!("Too many retries").is_transient()): walks theanyhowerror chain and retries only transient failures —sqlx::Error::Databasewith SQLITE_BUSY (5) / SQLITE_LOCKED (6) primary result codes (extended codes likeSQLITE_BUSY_SNAPSHOTkeep the primary code in the low byte),sqlx::Error::PoolTimedOut, andgoogle_gmail1::Error::Failurewith HTTP 429 or 5xx. Everything else — auth failures, other 4xx, IO/network errors — fails fast on the first occurrence.backoff_delay()): 1s doubling per attempt, capped at 32s, plus up to 1s of jitter (derived from the system clock's subsecond millis, avoiding a new dependency), applied viatokio::time::sleepbefore each retry.work()now takes a&mut Option<String>resume token that only advances after a page is fully processed, so a retry re-lists from the last successfulnext_page_tokeninstead of page one.retriesdead code and the TODO/comments at the oldsrc/main.rs:69-76.Deliberately scoped to the outer retry loop, independent of the concurrency work in #5 (the pre-existing
unused import: tokio::taskwarning belongs to that issue's commented-out code and is left untouched).Verification
cargo buildsucceeds.cargo clippyreports no new warnings (only the pre-existingunused import: tokio::task).credentials.json/tokencache.json) which are not available in this environment, so the retry/backoff behavior was verified by inspection and compilation only.