Fix false 'login expired' crash loop from stale auth-error text in the TUI pane - #39
Open
aroc wants to merge 9 commits into
Open
Fix false 'login expired' crash loop from stale auth-error text in the TUI pane#39aroc wants to merge 9 commits into
aroc wants to merge 9 commits into
Conversation
The TUI health check greps the last 20 rendered pane lines for auth-error strings. That text is transcript history, not current state: after an auth incident resolves (token refreshed), the old 401/"Please run /login" output stays on screen indefinitely, so GetHealth kept reporting a non-recoverable "login expired" — crash-looping the daemon via runtime validation even though credentials were valid the whole time. Now, when auth-error text matches, GetHealth and GetSessionState consult the on-disk OAuth token ($CLAUDE_CONFIG_DIR/.credentials.json expiresAt). An unexpired token refutes the pane text: the failure is downgraded to recoverable so callers restart the session — clearing the stale pane while --resume preserves the conversation — instead of demanding a manual /login. Missing, unreadable, or expired credentials (macOS Keychain, API-key auth) leave behavior exactly as before.
Review found two gaps in the credentials-file cross-check: a token can be revoked server-side while still unexpired on disk, in which case (a) the recoverable downgrade caused indefinite restart churn — a freshly restarted pane is clean until traffic hits it, so ensureHealthySession always declared recovery after one restart and the admin escalation never fired — and (b) GetSessionState fell through to dispatch, losing user messages silently since the retry detector only matches 5xx errors. Auth-error text plus an unexpired token now triggers a definitive check: a minimal headless claude -p request (same invocation conventions as internal/subagent). Probe success proves the pane text is stale — report healthy and let normal traffic scroll it away; no restart needed. Probe auth failure restores the original non-recoverable escalation. Verdicts are cached (5 min positive, 1 min negative, inconclusive never) and invalidated on session restart; GetSessionState only ever reads the cache, since it runs in 2-second polling loops.
Review demonstrated by mutation that the credentials and probe wiring was untested: hardcoding credentialsUnknown at GetHealth's call site — an exact revert of the incident fix — passed the entire suite, as did inverting the BlockedAuth gate in GetSessionState. Extract healthFromSnapshot and classifySessionState so both classifications are testable without a live tmux server, driving the real on-disk credentials lookup via CLAUDE_CONFIG_DIR. Both mutations now fail the new tests.
Round-2 review found the probe could wedge the whole daemon: claude spawns children that inherit the output pipes, so after the 25s context kill of the direct process, CombinedOutput blocked indefinitely on pipe EOF — and probeMu was held across the probe, so GetHealth, GetSessionState polling, RestartSession, and ResetConversation all queued behind the hang with no escape. The probe now runs in its own process group, kills the group on cancellation, and caps pipe drain with WaitDelay; probe execution is serialized on a dedicated probeRunMu while the verdict cache keeps its own mutex that is never held across a probe, keeping cachedAuthState and invalidateAuthProbe wait-free (covered by a liveness test). Also raise the positive verdict TTL to 15 minutes: at 5 it exactly matched the gateway's post-send polling window, so a verdict could expire mid-wait of a long-running task with stale auth text still on screen, flipping classifySessionState to a false BlockedAuth mid-dispatch. New mutation-verified tests pin the two remaining wiring contracts: both session-recycle paths must invalidate the cache, and GetHealth's probe must land in the same cache GetSessionState reads.
Round-3 review showed the TTL raise alone did not establish the no-expiry-mid-dispatch invariant: verdict lifetime runs from probe time and cache hits never extend it, so a dispatch admitted at minute 11 of a 15-min verdict could still watch it lapse inside the gateway's 5-min post-send polling window — reproducing the false BlockedAuth this branch exists to prevent. GetHealth's probing path now treats a positive verdict with under 8 minutes of life as a miss and re-probes, so any admitted dispatch holds a verdict that outlives the polling window plus dispatch overheads. Expiry is never slid on reads — a verdict stays at most 15 minutes old, keeping the revoked-token window bounded. Negative verdicts renew naturally via their 1-minute TTL. A fake-clock seam (nowFn) pins all three timing rules; mutations shrinking the positive TTL, dropping negative-verdict caching, or removing the renewal all fail the new test.
Round-4 review showed the renewal window only covers one gateway polling window, but sendWithRetry chains up to three 5-minute windows after a single GetHealth admission — and the worst case exceeds the whole verdict TTL, so no renewal constant can guarantee a verdict outlives a retried dispatch. Close it at the consumption point instead: when WaitForAwaitingInput sees BlockedAuth, it re-checks via confirmBlockedAuth — unexpired on-disk token plus a passing probe (cached or fresh) refutes the block and re-arms the cache, so the next poll classifies normally; a failed or inconclusive verification lets the block stand. This bounds the false-escalation window regardless of dispatch length while keeping classifySessionState itself probe-free.
Round-5 review found the safety branch untested by mutation: weakening the comparison so an inconclusive probe refutes the block survived the suite — under which a genuine outage whose re-verification probe times out would be swallowed instead of surfacing login-expired. New subtests cover the inconclusive-probe and unknown-credentials cases; the mutation now fails.
This was referenced Aug 23, 2026
Open
Contributor
dorkitude
added a commit
to alan-botts/goated
that referenced
this pull request
Aug 23, 2026
dorkitude
added a commit
to alan-botts/goated
that referenced
this pull request
Aug 23, 2026
Contributor
|
Follow-ups pushed in 51770a2 and c8674a1: the live auth probe is now authoritative across OAuth refresh, Keychain, and API-key auth; local credential-file state is diagnostic only; and only a confirmed auth-probe failure may surface BlockedAuth. Passing probes refute stale pane text, while inconclusive probes resolve to an ambiguous non-auth-blocking state. Full tests, claudetui race tests, vet, and build pass. |
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
3 participants
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.
Problem
The Claude TUI health check greps the last 20 rendered pane lines for auth-error strings (
API Error: 401,authentication_error,OAuth token has expired,Please run /login). That text is transcript history, not current state: after an auth incident resolves (token refreshed), the old error output stays on screen indefinitely.This caused a real production outage:
GetHealthkept reporting a non-recoverable "login expired" from stale pane text,runtime.Validatetreats that as fatal, and the daemon crash-looped for hours — while the on-disk OAuth token was valid the whole time and the operator's/loginhad long since succeeded. The gateway also kept replying "login expired; run /login" to users for the same reason.Fix
When auth-error text matches, the pane is no longer trusted on its own:
$CLAUDE_CONFIG_DIR/.credentials.json, default~/.claude/.credentials.json,claudeAiOauth.expiresAt). Missing, unreadable, or expired credentials (macOS Keychain setups, API-key auth) keep the exact pre-PR behavior — the check can only ever tighten the conditions for declaring login expired, never loosen them elsewhere.claude --dangerously-skip-permissions -prequest (same invocation conventions asinternal/subagent). A token can be revoked server-side while still looking valid on disk, so the expiry timestamp alone can't be trusted either:The probe is hang-proof by construction: it runs claude in its own process group, kills the whole group on cancellation (claude's children inherit the output pipes — killing only the direct process would leave
CombinedOutputblocked on pipe EOF indefinitely), and caps post-kill pipe drain withcmd.WaitDelay. Probe execution is serialized on its own mutex; the verdict cache has a separate mutex that is never held across a probe, so cache readers stay wait-free even mid-probe (pinned by a liveness test).Probe verdicts are cached (15 min positive, 1 min negative, inconclusive never) and invalidated on
RestartSession/ResetConversation.GetSessionStateruns in 2-second polling loops, so it never probes — it only reads the cached verdict, and suppressesBlockedAuthsolely when the token is unexpired and a recent probe passed. Two mechanisms keep that safe for long dispatches without ever sliding expiry on reads (a verdict is at most 15 minutes old, keeping revoked-token exposure bounded): GetHealth renews a positive verdict when under 8 minutes of life remain, andWaitForAwaitingInputre-verifies an apparent auth block (confirmBlockedAuth) before surfacing it — so even a dispatch that chains multiple retry windows past the whole TTL re-arms the cache instead of emitting a false "login expired".Known trade-off: the positive cache means a token revoked within 15 minutes of a passing probe, while auth text is still on-screen, could swallow messages until the cache expires or the session restarts.
Tests
internal/claudetuipreviously had one test (context-output parsing). This PR adds coverage for:CLAUDE_CONFIG_DIRresolutionhealthFromSnapshot/classifySessionStateseams, driving the real credentials lookup throughCLAUDE_CONFIG_DIR. These were added after adversarial review demonstrated by mutation that hardcoding the credentials state at theGetHealthcall site — an exact revert of the incident fix — passed the entire suite. All seven review-surfaced mutations (hardcodedcredentialsUnknown; invertedBlockedAuthgate; deleted cache-invalidation calls;GetHealthbypassing the shared cache; shrunken positive TTL; dropped negative-verdict caching; removed renewal) now fail the tests.gofmtclean,go build ./...,go vet ./...,go test -race ./internal/claudetui/,go test ./...all pass.Scope / follow-ups
claudetuionly.internal/claude(headless) greps fresh stderr from the last run — a different mechanism without the staleness bug.codextuihas its own auth detection against codex's auth store; giving it the same treatment is a possible follow-up.Review process
Reviewed pre-submission by four rounds of a multi-agent panel (4 review lenses × 3 adversarial judges per finding). Round 1 confirmed the revoked-token dispatch regression and two mutation-proven test gaps; round 2 confirmed the probe-hang/mutex-wedge deadlock and the TTL/polling-window collision; round 3 confirmed the TTL fix was incomplete (no renewal on cache hits). Each round's findings were fixed in a dedicated commit before the next round ran against the full updated diff; the final round ran against the complete branch.