You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ApnsClient and FcmClient are ~95%-identical copy-pastes that have already diverged, and the send path collapses every outcome into Result<bool>, destroying the information downstream consumers need.
Duplicated auth machinery: identical CachedToken { token: Zeroizing<String>, expires_at } behind Arc<RwLock<Option<CachedToken>>> with the same double-checked read/write-lock pattern in src/push/apns.rs:206-389 and src/push/fcm.rs:214-455. The copies have already drifted (transport-retry budgets, log levels, FCM-only aud/endpoint mismatch) — a fix in one provider does not reach the other.
Lossy outcome type:SendAttemptResult::Success(bool) conflates "provider says the token is dead" with "retries exhausted" (with_retry returns Ok(false) on exhaustion, src/push/retry.rs:197-204) and "unknown status" (APNs catch-all _ => Success(false), src/push/apns.rs:495-502). The dispatcher then labels every Ok(false) as reason="invalid_token" (src/push/dispatcher.rs:258-293) — the one metric whose downstream use (dead-token pruning) is destructive.
Every member issue below is a symptom of one of these two defects. Fixing them individually re-patches the same duplicated code paths; fixing the architecture closes them all.
Refactor design
New src/push/auth.rs — shared credential machinery:
Wave 1; parallel with the config, lifecycle, secret-hygiene, NIP-59, and health trackers (disjoint files except dispatcher.rs, where the admission tracker touches different functions).
The retry-policy tracker (Retry-After cap, live-task bound) is hard-blocked behind this one — same retry.rs machinery.
Root cause
ApnsClientandFcmClientare ~95%-identical copy-pastes that have already diverged, and the send path collapses every outcome intoResult<bool>, destroying the information downstream consumers need.CachedToken { token: Zeroizing<String>, expires_at }behindArc<RwLock<Option<CachedToken>>>with the same double-checked read/write-lock pattern insrc/push/apns.rs:206-389andsrc/push/fcm.rs:214-455. The copies have already drifted (transport-retry budgets, log levels, FCM-onlyaud/endpoint mismatch) — a fix in one provider does not reach the other.SendAttemptResult::Success(bool)conflates "provider says the token is dead" with "retries exhausted" (with_retryreturnsOk(false)on exhaustion,src/push/retry.rs:197-204) and "unknown status" (APNs catch-all_ => Success(false),src/push/apns.rs:495-502). The dispatcher then labels everyOk(false)asreason="invalid_token"(src/push/dispatcher.rs:258-293) — the one metric whose downstream use (dead-token pruning) is destructive.Every member issue below is a symptom of one of these two defects. Fixing them individually re-patches the same duplicated code paths; fixing the architecture closes them all.
Refactor design
New
src/push/auth.rs— shared credential machinery:trait AuthTokenGenerator { async fn mint(&self) -> Result<MintedToken>; }withMintedToken { token: Zeroizing<String>, expires_at: SystemTime }.expires_ininstead of the hardcoded 50-min TTL ([MEDIUM] FCM caches OAuth access token for a hardcoded 50 min, ignoring the server-providedexpires_in#88); POSTs tosa.token_uriso the request target matches the signedaud([LOW] FCM OAuth JWTaudis signed withservice_account.token_uribut the request is always POSTed to the hardcodedOAUTH_TOKEN_URL— a non-standardtoken_uricauses a silent total FCM outage #153); reads the token response body with a byte cap ([LOW] FCM OAuth token success response body is read with unboundedresponse.json()#154).TokenCache<G: AuthTokenGenerator>: read-lock fast path; refresh is single-flight and runs outside the write lock so concurrent senders keep using the still-valid token during a refresh ([LOW] FCM access-token refresh holds the cache write lock across the OAuth HTTP round-trip, stalling all concurrent senders during refresh #86);invalidate_if(reason)gates eviction on parsed auth-reject reasons (ExpiredProviderToken/InvalidProviderToken), not blanket HTTP 403 — ends the per-push ES256 re-sign stampede underBadCertificateEnvironment-class 403s ([MEDIUM] APNs evicts the cached JWT on every HTTP 403 reason, causing a per-notification ES256 re-sign stampede under non-JWT 403s (e.g. BadCertificateEnvironment) #145).Rich outcome enum, replacing
Result<bool>/Success(bool)end-to-end:Failed(UnexpectedStatus), never token-invalid ([LOW] APNs unexpected-status catch-all (_ => Success(false)) misclassifies server/config errors (404/405/413/…) as a dead device token #169); FCM/APNs asymmetry removed.retries_exhausted,unexpected_status, …) instead of blanketinvalid_token([LOW] Dispatcher labels everyOk(false)asreason="invalid_token", conflating dead tokens with retries-exhausted and unknown-status failures (and dropping retriable 408s) #90).handle_response; per-logical-push duration/status recorded once insend()after retries resolve, so histogram counts equal push volume ([LOW] push_duration and push_response_status are recorded once per HTTP attempt, so retried pushes over-count samples (attempts ≠ logical pushes) #168).reqwest::Error::without_url()(or an equivalent redacting wrapper) before reaching any log sink, so the APNs device-token-bearing URL can never leak at WARN/DEBUG ([MEDIUM] APNs device token leaks into WARN/DEBUG logs via thereqwest::ErrorURL on transport failures #172 — acceptance test owned by the secret-hygiene tracker).FcmRequest/token-string rebuild ([LOW] FCM rebuilds the entire request (HashMap +FcmRequest+device_token.to_string()) on every transport-retry attempt; APNs hoists it out of the retry closure — divergence + extra non-zeroized token copy #198) and the extra non-zeroized token copy; where feasible assemble the body via a zeroizing buffer (the [LOW] FCM/APNs copy the decrypted device token into non-zeroized String/JSON body inside the request builder, defeating the upstream Zeroizing wrapper #126 remainder — also tracked by the secret-hygiene tracker).is_valid_device_tokenbranches (or demote todebug_assert!) and documentTokenPayload::from_decrypted+ the dispatcher's UTF-8 gate as the single source of truth for token well-formedness ([LOW]is_valid_device_tokenodd-length / non-hex / empty branches are unreachable on the production dispatch path — dead defensive code that reads as meaningful validation #199).Members
expires_in#88 — honor OAuthexpires_ininstead of hardcoded 50 minaudis signed withservice_account.token_uribut the request is always POSTed to the hardcodedOAUTH_TOKEN_URL— a non-standardtoken_uricauses a silent total FCM outage #153 — POST tosa.token_urisoaudand endpoint agreeresponse.json()#154 — bounded read of the OAuth success bodyFcmRequest+device_token.to_string()) on every transport-retry attempt; APNs hoists it out of the retry closure — divergence + extra non-zeroized token copy #198 — build FCM request once, outside the transport-retry closure_ => Success(false)) misclassifies server/config errors (404/405/413/…) as a dead device token #169 — APNs catch-all must not classify server faults as dead tokensOk(false)asreason="invalid_token", conflating dead tokens with retries-exhausted and unknown-status failures (and dropping retriable 408s) #90 — dispatcher label taxonomy from exhaustivePushOutcomematchis_valid_device_tokenodd-length / non-hex / empty branches are unreachable on the production dispatch path — dead defensive code that reads as meaningful validation #199 — remove dead token-validation branches; single source of truthreqwest::ErrorURL on transport failures #172 — (rider, tracked by secret-hygiene tracker) redact provider errors before loggingIn-flight PRs
refresh_token_inner, which lifts wholesale into the FCMAuthTokenGenerator.PushOutcome+TokenCache's retry-once policy expresses. Its tests are spec material for this tracker; [MEDIUM] APNs 403 / FCM 401 invalidates the cached auth token but returns Permanent for the current request — drops one notification per device instead of retrying with a fresh token #85 stays open as a member here.Sequencing
dispatcher.rs, where the admission tracker touches different functions).Retry-Aftercap, live-task bound) is hard-blocked behind this one — sameretry.rsmachinery.