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) + } +}