Repository navigation
fix(health): stop a transient 5xx paging as a dead credential, and make an unpersisted refresh impossible - #56
Merged
Conversation
…troys credentials Two bugs from the same incident: a 504 from SoundCloud's token endpoint was reported as a dead credential (any refresh failure mapped to DEAD, so a transient gateway blip paged as "needs re-authorisation"), and diagnosing that false alarm with a persister-less SoundCloudUploader silently rotated and discarded the real refresh token, turning the false alarm into a real outage. - platform_health/soundcloud_uploader/youtube_uploader: classify token- endpoint failures at the point the HTTP status is known. DEAD only for invalid_grant/unauthorized_client/invalid_client or a 401/403 on the credential itself; 5xx/timeouts/network errors and unrecognized rejections now raise SoundCloudTransientError/YouTubeTransientError and report UNKNOWN (non-paging). YouTube reuses google-auth's own RefreshError.retryable signal instead of re-deriving it. - platform_health: require 3 consecutive DEAD probes (~30 min) before the credential_dead notification fires; /api/health still flips to dead on the first probe. A recovery resets the streak. - soundcloud_uploader: SoundCloudUploader refuses to exercise the refresh_token grant when no on_tokens_refreshed persister is attached, raising SoundCloudUnpersistedRefreshError instead of silently rotating and losing the credential. allow_unpersisted_refresh=True opts back in for tests/throwaway credentials. Updated two existing soundcloud_uploader tests that encoded the old behavior: test_refresh_without_callback_still_works asserted the exact destructive path this fixes (replaced with a raises-by-default test plus an explicit-opt-in test), and test_password_grant_rejection_is_recorded used unsupported_grant_type (a protocol/config mismatch, not a revoked grant) as its fatal fixture -- swapped for invalid_client so it still tests what it always meant to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both fixes come from one incident this morning, and the second is the more important of the two.
What happened
SoundCloud's token endpoint returned a 504 Gateway Time-out.
_probe_soundcloudtreats any refresh failure asDEAD, so thecredential_deadalert — added in #51 — emailed the owner "soundcloud credentials need re-authorisation". The credential was perfectly healthy.Then, while diagnosing that false alarm, a
SoundCloudUploaderwas constructed without a persister and asked for a token. SoundCloud rotates the refresh token on every use and invalidates the old one, so the check consumed the rotation and discarded the replacement. It reported success — and in succeeding, destroyed the credential. A false alarm became a real outage requiring re-authorisation.platform_health.pyalready carried a comment warning about exactly this. The comment was read, and then the thing it warned about was done anyway. A hazard that depends on every future caller remembering a comment is not handled, it is deferred.The fixes
1. Transient vs fatal. Only genuine authorisation failures (
invalid_grant,unauthorized_client,invalid_client, 401/403 on the credential) map toDEAD. 5xx, gateway timeouts and network errors map toUNKNOWN, which already existed and already doesn't page.Classified structurally, not by matching prose — on SoundCloud from the token endpoint's status code, and on YouTube from
google.auth.exceptions.RefreshError.retryable, which google-auth sets itself.YouTube had the identical flaw and is fixed the same way. Mixcloud's probe was already correct.
2. Confirmation window. Three consecutive
DEADprobes (~30 min) before the notification fires./api/healthstill flips toDEADimmediately — an operator looking at the dashboard sees the truth; only the page waits. A recovery resets the counter, and the existing page-once-on-transition behaviour is preserved.3. The footgun is now structurally impossible.
_refresh_access_token()raisesSoundCloudUnpersistedRefreshErrorbefore any network call when there is no persister and no explicit opt-in (allow_unpersisted_refresh=True, keyword-only). The guard sits at the dangerous call itself, not at the entry point this incident happened to use, so every future path is covered. Callers that just want to check a credential have a safe, obvious route.Two existing tests encoded the bug
Flagged rather than quietly edited:
test_refresh_without_callback_still_worksasserted the credential-destroying behaviour as correct. The bug was pinned by the suite.test_password_grant_rejection_is_recordedusedunsupported_grant_typeas its fatal fixture — that's a protocol mismatch, not a revoked grant. Swapped toinvalid_client.Tests
397 passing across 12 related files;
ruffclean. New coverage: a 504 →UNKNOWNand no page; a timeout →UNKNOWN;invalid_grant→DEAD; one probe doesn't page while three consecutive do, exactly once, with recovery resetting; a persister-less rotating refresh raises; a persister-ful uploader still refreshes and persists.All three mutation checks run individually — reverting the classification, the threshold, or the guard each fails its own named test.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M