From 1088c7e1fc3084de092e250a8c3928ed3dadda59 Mon Sep 17 00:00:00 2001 From: Jiro Amato Date: Wed, 11 Nov 2026 23:11:37 -0700 Subject: [PATCH] fix(management): retry transient Devin OAuth callback read errors waitDevinOAuthCallback ended the login with "Failed to read OAuth callback" on any read error other than not-exist. On Windows a read that lands while the callback handler renames the file into place fails with a sharing violation, so a login could fail although the callback arrived. Retry on the next tick, as the other OAuth flows already do. The context deadline and session state still bound the wait. This was the cause of the intermittent "token exchange did not start" in TestDevinRemoteOAuthFlow. The test now reports the session status on that failure, and its 5s failure bounds are 15s. Backport of upstream router-for-me/CLIProxyAPI#7482 (a50f54eb), with the Devin test bound change from 97b8a034 that the regression test depends on. --- .../management/auth_files_devin_oauth.go | 12 +++-- .../management/auth_files_devin_oauth_test.go | 44 +++++++++++++++++-- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/internal/api/handlers/management/auth_files_devin_oauth.go b/internal/api/handlers/management/auth_files_devin_oauth.go index 12cfeeef2ee..f87d304ac80 100644 --- a/internal/api/handlers/management/auth_files_devin_oauth.go +++ b/internal/api/handlers/management/auth_files_devin_oauth.go @@ -136,6 +136,9 @@ func (h *Handler) completeDevinOAuth(ctx context.Context, authDir, state, codeVe log.Info("Devin authentication successful") } +// readDevinOAuthCallbackFile is a variable so tests can simulate read errors. +var readDevinOAuthCallbackFile = os.ReadFile + func waitDevinOAuthCallback(ctx context.Context, path, state string) (*oauthCallbackFilePayload, error) { ticker := time.NewTicker(500 * time.Millisecond) defer ticker.Stop() @@ -146,7 +149,7 @@ func waitDevinOAuthCallback(ctx context.Context, path, state string) (*oauthCall if ctx.Err() != nil { return nil, errors.New("Timeout waiting for OAuth callback") } - data, errRead := os.ReadFile(path) + data, errRead := readDevinOAuthCallbackFile(path) if errRead == nil { var result oauthCallbackFilePayload if errDecode := json.Unmarshal(data, &result); errDecode != nil { @@ -154,9 +157,10 @@ func waitDevinOAuthCallback(ctx context.Context, path, state string) (*oauthCall } return &result, nil } - if !errors.Is(errRead, os.ErrNotExist) { - return nil, errors.New("Failed to read OAuth callback") - } + // Retry on any read error, like the other OAuth flows do. The file may not + // exist yet, or it may be briefly unreadable: on Windows a read that lands + // while the callback handler renames the file into place fails with a + // sharing violation. The context deadline and session state bound the wait. select { case <-ctx.Done(): return nil, errors.New("Timeout waiting for OAuth callback") diff --git a/internal/api/handlers/management/auth_files_devin_oauth_test.go b/internal/api/handlers/management/auth_files_devin_oauth_test.go index 7bf92c8747c..86b0142a1ff 100644 --- a/internal/api/handlers/management/auth_files_devin_oauth_test.go +++ b/internal/api/handlers/management/auth_files_devin_oauth_test.go @@ -21,6 +21,11 @@ import ( coreauth "github.com/router-for-me/CLIProxyAPI/v8/sdk/cliproxy/auth" ) +// devinOAuthTestWait is a failure bound, not an expected latency: the flow +// checks for its callback file every 500ms and saves the credential to disk, +// and both stall for seconds on a loaded host. +const devinOAuthTestWait = 15 * time.Second + type fakeDevinOAuthService struct { exchange func(context.Context, string, string) (string, error) create func(context.Context, string) (*coreauth.Auth, error) @@ -122,11 +127,14 @@ func TestDevinRemoteOAuthFlow(t *testing.T) { if base64.RawURLEncoding.EncodeToString(digest[:]) != query.Get("code_challenge") { t.Fatal("PKCE challenge does not match verifier") } - case <-time.After(5 * time.Second): - t.Fatal("token exchange did not start") + case <-time.After(devinOAuthTestWait): + // Report the session status: it carries the reason when the flow gave up. + w = httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/get-auth-status?state="+start.State, nil)) + t.Fatalf("token exchange did not start, session status: %s", w.Body.String()) } - deadline := time.NewTimer(5 * time.Second) + deadline := time.NewTimer(devinOAuthTestWait) defer deadline.Stop() ticker := time.NewTicker(10 * time.Millisecond) defer ticker.Stop() @@ -254,3 +262,33 @@ func TestWaitDevinOAuthCallbackExpiredContext(t *testing.T) { t.Fatalf("error = %v", errWait) } } + +func TestWaitDevinOAuthCallbackRetriesTransientReadError(t *testing.T) { + const state = "devin-transient-read-error" + RegisterOAuthSession(state, "devin") + defer CompleteOAuthSession(state) + + // On Windows a read that lands while the callback handler renames the file + // into place fails with a sharing violation. The next read succeeds. + reads := 0 + originalRead := readDevinOAuthCallbackFile + readDevinOAuthCallbackFile = func(string) ([]byte, error) { + reads++ + if reads == 1 { + return nil, errors.New("the process cannot access the file because it is being used by another process") + } + return []byte(`{"state":"` + state + `","code":"code"}`), nil + } + t.Cleanup(func() { readDevinOAuthCallbackFile = originalRead }) + + result, errWait := waitDevinOAuthCallback(context.Background(), filepath.Join(t.TempDir(), "callback.oauth"), state) + if errWait != nil { + t.Fatalf("transient read error ended the login: %v", errWait) + } + if result.Code != "code" || result.State != state { + t.Fatalf("unexpected callback: %+v", result) + } + if reads != 2 { + t.Fatalf("reads = %d, want 2", reads) + } +}