fix(fcm): retry transient OAuth2 failures behind a single-flight refresh - #41
Conversation
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b98410db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Single-flight only serialised the cohort waiting on the refresh lock. With Google still down, each waiter in turn found the cache empty and ran its own full retry sequence, so 50 queued dispatches became 150 requests aimed at a service already failing, and the last caller waited roughly 50 retry budgets while holding an /api/notify permit. Saturating that pool drops dispatches silently, which is the outcome the retry exists to avoid. The lock now guards the last failed refresh and every caller arriving within a 10 s window fails with the same cause. The window is longer than the worst-case retry budget, so the cohort cannot immediately re-probe, and negligible against the ~1 h token lifetime, so recovery is not delayed.
A connection that times out or resets after the response headers arrive fails at the body read, not at send(). Both cases were classified alongside malformed JSON and failed without a retry, dropping a notification a second attempt could still have delivered. Reading the body and parsing it are now separate steps. A transport failure is retryable like any other network error, while a body that arrived in full and does not parse stays terminal, since repeating the request cannot repair it. The new test drives a raw socket that closes mid-body and asserts the exchange takes exactly two connections.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved
Re-reviewed the current head and the prior feedback. The refresh now single-flights both successful and failed OAuth exchanges: waiters reuse a fresh cached token or the short failure cooldown, preventing an outage from serially multiplying retry budgets while notify permits are held. Network failures while reading a successful response body are also retried, while complete malformed bodies and non-429 4xx responses fail fast.
Verified locally on this head: cargo fmt --all -- --check, cargo test --locked (61 passed), and cargo clippy --locked --all-targets --all-features -- -D warnings. I also merged the current live main tip locally without conflicts and ran its combined suite successfully (98 passed). Current GitHub checks are green.
1 participant
A failed OAuth2 token exchange returned
Errimmediately and the dispatch was discarded, so a transient 503 from Google silently dropped a notification. FCM is the only backend enabled in production, which makes this the live failure mode of the push path.Retrying alone would have made the outage worse
Two things surfaced while scoping this, neither in the original issue.
The retry runs on a borrowed semaphore permit. The sequence executes while the caller holds one of the 50
/api/notifypermits, andnotify_tokendrops the dispatch silently when the pool saturates (src/api/notify.rs:106-110) — it still returns202, because the privacy contract forbids distinguishing outcomes. A failed exchange currently costs ~5 s, the shared client's timeout. Three attempts at that timeout would cost ~15 s, cutting dispatch throughput from ~10/s to ~3/s during a Google outage and roughly tripling the silent-drop rate. The retry would have made the incident it exists to mitigate measurably worse, with nothing user-visible to say so.There was no single-flight. Every concurrent dispatch that missed the cache minted its own JWT and called Google independently. Adding retries on top would multiply an outage by the number of in-flight dispatches: N dispatches × 3 attempts = 3N requests aimed at a service already failing.
Single-flight is therefore not a separate optimisation to defer. Without it the retry is a load amplifier pointed at a struggling dependency.
What changed
Mutexaround the refresh, with a cache re-check after acquiring it, so one exchange serves every waiter. Worst case goes from ~15 s per task to ~6 s shared.429is the exception, being explicitly a retry signal.build_jwtsplit fromexchange_with_retry, so the transport policy is testable without RSA key material. Credential handling and transport behaviour fail for unrelated reasons and now read that way.Tests
Eight new, with exact hit-count assertions so an over- or under-eager retry both fail:
OAUTH_MAX_ATTEMPTSThe one worth reading is
worst_case_retry_budget_stays_close_to_the_pre_retry_cost. It asserts a property rather than a function: it composes the constants and fails if a later change raises the attempt count or the per-attempt timeout past the point where an outage starts costing more notifications than no retry at all. That regression would otherwise be invisible — the symptom is dropped pushes and awarn!with no pubkey in it.Not covered by tests
Single-flight itself is verified structurally, not by test. Driving the full refresh path needs RSA key material, and both ways to get it are worse than the gap: committing a private key to a repo whose history already includes a leaked
SERVER_PRIVATE_KEY, or moving JWT signing outside the lock so every concurrent caller signs one even when a single exchange runs — which is part of what the lock avoids. The guarded region is three lines and the policy it protects is covered. Noted on the issue rather than left implicit.docs/architecture.mdnow describes both behaviours and why they are bounded.Closes #9