fix(auth): client-side auth timeout + reconnect recovery#61
Merged
Conversation
PR #60 left the in-TUI Spotify auth flow vulnerable to two stuck-state bugs: if the daemon crashed mid-flow the `auth_in_progress` flag survived indefinitely, and a control-socket reconnect would leave the client convinced a flow was still running against a daemon that had no memory of it. Fix both: - `PickerState::tick_auth_deadline` trips after 5m30s — longer than the daemon's own 5m ceiling so the daemon's `AuthFailed { "timeout" }` normally wins with its specific reason, and this is the last-resort unstick when the daemon is simply gone. Called every render tick. - `ReconnectingSession` fires a reconnect notice via a sender the TUI installs on startup. The render loop drains the channel each tick, calls `handle_daemon_reconnected` (which clears any pending auth flag with a benign "daemon reconnected — retry if needed" banner), and re-issues `Verb::ReadConfig` so the Settings tab resyncs with the new daemon's actual auth state. Fix 1 from the original scope — capturing the OAuth authorize URL so the TUI can render it for SSH/headless users — turned out to be structurally impossible from outside librespot-oauth. Both the CSRF state and the PKCE challenge are generated inside `OAuthClient::set_auth_url` with no public hook. Any URL we'd build ourselves would pair a different state + challenge with the verifier librespot's listener is waiting to exchange — the user's browser redirect would then fail PKCE validation. The task's stop-and-report clause applies. Added a TODO in auth.rs pointing at the upstream fix (expose the URL through the public API) and softened the pending-row message to point headless users at the sibling `clitunes auth` CLI, which runs in their terminal and can print the URL directly. Tests: - tick_auth_deadline: noop when idle, noop before timeout, trips after timeout with a client-side reason, and doesn't double-fail. - handle_daemon_reconnected: clears pending flag + surfaces benign note; noop when not in progress. - set_auth_completed/failed clear the deadline clock. - `a` press populates `auth_started_at`. - paint: pending state renders the new "run clitunes auth" fallback. Local-CI triple clean: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace --all-features`. Closes clitunes-lsn / CLI-93
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Summary
PickerState::tick_auth_deadlinetrips the in-TUI Spotify auth flow after 5m30s when the daemon dies mid-flow and never emitsAuthCompleted/AuthFailed. 30s longer than the daemon's own 5m ceiling so the daemon's specificAuthFailed { reason: "timeout" }wins the happy-path-with-delay race.ReconnectingSessionnow fires a reconnect notice into a channel the render loop drains each tick. On reconnect the client clears any in-flight auth state with a benign "daemon reconnected — retry if needed" banner and re-issuesVerb::ReadConfigso the Settings tab resyncs with the new daemon process.clitunes authuntil the upstream URL-capture is available.Why
PR #60 landed in-TUI Spotify sign-in with two known gaps flagged during pride-gate review (CLI-93):
auth_in_progressflag survived forever; if the control socket reconnected into a fresh daemon, the client remained convinced a flow was still running.Event::AuthStarted { url }was alwaysNonebecause librespot-oauth 0.8 doesn't expose the authorize URL through its public API.Gap 1 is this PR. Gap 2 turned out to be structurally impossible to shadow from outside the crate —
OAuthClient::set_auth_urlgenerates both the CSRFstateand the PKCEcode_challengewith private randomness, so any URL we'd rebuild ourselves would pair a different state + challenge with thepkce_verifierlibrespot's listener is waiting to exchange. The user's browser redirect would fail PKCE validation. The task's stop-and-report clause applied. Added aTODO(librespot-oauth)pointing at the upstream dev branch and routed SSH/headless users to the siblingclitunes authCLI in the meantime, where the URL can be printed directly to the user's terminal.Test plan
cargo fmt --checkcleancargo clippy --workspace --all-targets -- -D warningscleancargo test --workspace --all-features— full suite passes (509 + 4 new picker-state tests, 1 new paint test, existing auth-event tests untouched).tick_auth_deadline_trips_after_timeout— deterministic time-driven unstick verified withAUTH_CLIENT_TIMEOUT + 1s.daemon_reconnect_clears_pending_auth_with_benign_note—auth_in_progressflips off with the expected reason;auth_started_atresets.clitunesdebug binary successfully; rebinding the render-loop config compiles through the main TUI and pane entrypoints.