Stop losing the Spotify refresh token an hour into every session - #12
Merged
Conversation
Reported as repeated "Spotify failed for X; recording it untagged" warnings with no reason given. Two separate defects, and the second is why the first was invisible. The SDK's PKCEAuthenticator stores exactly one token — the instance it was constructed with — and renews by writing the response's fields onto it in place. Spotify's PKCE renewal is not obliged to return a new refresh_token, and when it omits one that null lands on the good value. The first renewal still succeeds, so nothing looks wrong; the next one throws ArgumentException from inside the SDK and every lookup for the rest of the session fails identically. That is why tagging worked for exactly one hour and then stopped: the log shows a client built at 15:47:56 failing from 16:49:00, and one built at 18:35:33 failing from 19:37:49. ResilientPkceAuthenticator remembers the last refresh token Spotify actually sent and puts it back before each request. A refresh token stays valid until revoked or replaced, so the remembered one is still good. Renewal is left to the SDK; only the field it mishandles is guarded. Composition rather than a subclass because Apply implements its interface without being virtual — wrapping works because the token instance is shared, so repairing it from outside repairs what the SDK is about to read. Separately, InMemoryLogSink rendered the message template and discarded LogEvent.Exception, so every logged cause was dropped on the way to the Record page. The answer to this bug sat in the log file for hours while the pane said only that something failed. It now appends the exception's type and message — not the stack trace, which would swamp a one-line-per- entry list; the file keeps the full detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The report
Two defects. The second is why the first went undiagnosed.
The refresh token was being thrown away every hour
PKCEAuthenticatorstores exactly one token — the instance handed to its constructor, exposed asInitialToken— and renews by writing the response's fields onto it in place. Spotify's PKCE renewal is not obliged to return a newrefresh_token, and when it omits one, that null is copied over the good value.The first renewal still succeeds, so nothing looks wrong. The next one throws:
…and every lookup for the rest of the session fails the same way. Hence the signature symptom: tagging works for exactly one hour — the access token's lifetime — then stops until the app restarts. The log confirms it twice, a client built at 15:47:56 first failing at 16:49:00, and one built at 18:35:33 first failing at 19:37:49.
ResilientPkceAuthenticatorremembers the last refresh token Spotify actually sent and restores it before each request. A refresh token stays valid until revoked or replaced, so the remembered one is still good, and an omittedrefresh_tokenbecomes a no-op instead of a session-ending loss. Renewal stays the SDK's — expiry checks, the token request andTokenRefreshedare untouched; only the mishandled field is guarded.Composition rather than a subclass:
Applyimplements its interface without being virtual, so there is nothing to override. Wrapping works because the token instance is shared — repairing it from outside repairs the object the SDK is about to read.The reason never reached the screen
InMemoryLogSink.EmitcalledRenderMessage()and discardedLogEvent.Exception. SoLog.Warning(ex, "…")displayed the template and dropped the only part saying why. The answer to this bug sat in the log file for hours while the Record page reported that something had failed and nothing more.It now appends the exception's type and message. Not the stack trace — this is a one-line-per-entry list beside a running recording, and the file sink already keeps full detail. The type name earns its place:
ArgumentExceptionversusHttpRequestExceptionseparates a bug in Offstream from the network being down.This affects every logged exception in the app, not just this one.
749 core + 168 UI tests green. Eleven new: the renewal-blanks-the-token sequence including the repeat case that was the actual failure, rotation still winning over a superseded token, nothing fabricated when no token was ever stored, and the sink carrying causes without carrying traces.
🤖 Generated with Claude Code