Repository navigation
fix: retry transient OIDC refresh failures instead of disabling the controller - #129
Conversation
…ontroller A network error, timeout, or token endpoint outage while refreshing the OIDC access token returned out of remote::run, and the daemon then marked enrollment "failed" and never restarted the controller task. The device stayed disconnected until the daemon process was restarted; "Try again" in the desktop UI could not recover it. - Treat refresh failures that are not an explicit 400/401/403 rejection as transient: keep the stored identity and refresh token, back off, and retry. Logout requests are still honoured during the backoff. - Supervise the controller session inside remote::run so any remaining error (for example a local identity store failure) restarts the session with backoff instead of permanently disabling the integration. Fixes agentdesktop-dev#113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@howardjohn can you review this |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The refresh retry branch also swallows local identity-store failures that should reach the supervisor.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds resilient controller-session supervision and retry behavior for transient OIDC refresh failures.
Changes:
- Retries transient token refresh failures with exponential backoff.
- Supervises failed controller sessions and handles logout between retries.
- Simplifies daemon task spawning.
| File | Description |
|---|---|
crates/agent/src/remote.rs |
Adds refresh retries, session supervision, and tests. |
crates/agent/src/daemon.rs |
Delegates supervision directly to remote::run. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Err(error) => { | ||
| // Network failures and token endpoint outages are transient: the | ||
| // stored refresh token is still valid, so keep the identity and retry | ||
| // instead of tearing down the controller integration. | ||
| warn!( | ||
| controller = %controller.address, | ||
| retry_in_seconds = delay.as_secs(), | ||
| error = %format!("{error:#}"), | ||
| "OIDC access token refresh failed; retaining current identity and retrying" | ||
| ); | ||
| if wait_before_retry(delay, logout, &identity_path, &identity, enrollment).await | ||
| { | ||
| break; | ||
| } | ||
| delay = next_retry_delay(delay); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Fixed in e23c123. The in-place retry arm now only matches errors whose chain contains a reqwest::Error (is_transient_refresh_failure). Anything else, such as identity::load/identity::save failures or an unsupported token type, is returned from run_session, so the supervisor sets enrollment to failed and applies its backoff. I added tests covering both cases (connect/timeout/503 are transient; an IO error from persisting the identity is not).
peterj
left a comment
peterj
left a comment
There was a problem hiding this comment.
thanks for the PR! Can you address the comments?
The in-place retry arm caught every error from refresh_oauth_if_needed, including identity::load/save failures, so a broken local identity store left enrollment reported as "enrolled" and looped forever. Retry only when the error chain contains a reqwest::Error and propagate everything else to the supervisor, which reports "failed". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Done, fixed the comments |
Fixes #113
Problem
When the daemon refreshed its OIDC access token and hit a transient error (connection reset, DNS failure, timeout, 5xx from the token endpoint),
remote::runreturnedErr. The spawn site indaemon.rsthen set enrollment to"failed", loggedcontroller integration disabledand exited the task, so nothing ever reconnected. The only recovery was restarting the daemon process. "Try again" in the desktop UI couldn't help, because the task it depends on was already gone.Only 400/401/403 responses (for example
invalid_grant) mean the refresh token is actually unusable, and those were already handled by re-enrolling.Change
Transient refresh failures are retried. A refresh error that
is_oauth_refresh_rejecteddoesn't classify as a rejection now:The enrollment state stays
enrolledthroughout.The controller session is supervised.
remote::runnow wraps the session loop (run_session), so it no longer returns. Any other error, such as a local identity store failure, is logged, surfaced asfailed, and followed by a backoff restart instead of disabling the integration permanently.daemon.rsjust spawnsremote::run.Tests
network_failures_during_refresh_are_transienttest. It checks that real reqwest connect-refused and timeout errors, wrapped in the same contextoidc::refreshadds, are not treated as token rejections.distinguishes_rejected_refresh_tokens_from_transient_errorstest still covers 400/401 (rejected) vs 500 (transient).cargo fmt --all,make lintandcargo test --workspace --lib --binsall pass, andmake generate-schemaproduces no diff.🤖 Generated with Claude Code