Skip to content

fix(oauth2_common): wrap underlying error in findOrCreateUser terminal fallthrough#234

Merged
veverkap merged 2 commits into
mainfrom
copilot/fix-oidc-user-error-handling
May 11, 2026
Merged

fix(oauth2_common): wrap underlying error in findOrCreateUser terminal fallthrough#234
veverkap merged 2 commits into
mainfrom
copilot/fix-oidc-user-error-handling

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 10, 2026

Bug Fix

What was the bug?

The terminal return in findOrCreateUser() used a bare fmt.Errorf with no %w, discarding the underlying error entirely. Every other error path in the function wraps with %w; this one silently swallowed all diagnostic context, making production log entries useless when the race-retry path exhausted all lookup strategies.

How did you fix it?

Restructured the final FindByEmail block to hoist err out of the if initializer so it remains in scope at the return site, then wrapped it:

// before
return nil, fmt.Errorf("failed to resolve OIDC user")

// after
u, err := users.FindByEmail(ctx, email)
if err == nil {
    linkSubjectBestEffort(ctx, users, u.ID, subject, "race_retry")
    return u, nil
} else if !errors.Is(err, auth.ErrNotFound) {
    return nil, fmt.Errorf("look up user by email after email race: %w", err)
}
return nil, fmt.Errorf("failed to resolve OIDC user after race retry: %w", err)

err here is always auth.ErrNotFound, but wrapping it lets callers errors.Is on the result and gives log entries a complete chain rather than a dead-end string.

Greptile Summary

This PR fixes a bug where the terminal return path in findOrCreateUser() discarded the underlying error entirely, replacing a bare fmt.Errorf with a %w-wrapped version. The fix also hoists the FindByEmail call out of the if initializer so err remains in scope at the return site.

  • Error wrapping restored: The final return now wraps err (always auth.ErrNotFound at that point) with %w, completing the error chain and enabling errors.Is checks by callers.
  • Variable scope fix: Separating u, err := users.FindByEmail(...) from the if condition is a required correctness step to make err available at the return site below — a clean, minimal restructuring.

Confidence Score: 5/5

Safe to merge — the change is a small, targeted fix to a single return statement with no impact on the happy path.

The only changed line is the terminal return in the race-retry exhaustion path. The restructuring to hoist u, err out of the if initializer is mechanically necessary and correct. All existing error paths, the linkSubjectBestEffort call, and all slog usages are untouched and correct.

No files require special attention.

Important Files Changed

Filename Overview
handler/oauth2_common.go Hoists FindByEmail result out of if-initializer so err stays in scope; wraps the previously bare terminal error with %w. Change is minimal and correct — all slog calls already use context variants.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([findOrCreateUser]) --> B{FindByOIDCSubject}
    B -- found --> C([return user, nil])
    B -- ErrNotFound --> D{FindByEmail}
    B -- other err --> E([return nil, err])
    D -- found --> F[linkSubjectBestEffort\nemail_match]
    F --> C
    D -- ErrNotFound --> G{CreateOIDCUser}
    D -- other err --> E
    G -- ok --> C
    G -- other err --> H([return nil, fmt.Errorf wrap])
    G -- ErrEmailExists\nrace --> I{FindByOIDCSubject\nrace retry}
    I -- found --> C
    I -- ErrNotFound --> J{FindByEmail\nrace retry}
    I -- other err --> K([return nil, fmt.Errorf wrap])
    J -- found --> L[linkSubjectBestEffort\nrace_retry]
    L --> C
    J -- other err --> M([return nil, fmt.Errorf wrap])
    J -- ErrNotFound --> N([return nil, fmt.Errorf\nfailed to resolve user after race retry: err\n now wrapped with %w])
    style N fill:#d4edda,stroke:#28a745
Loading

Reviews (2): Last reviewed commit: "Potential fix for pull request finding" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes lost diagnostic context in handler.findOrCreateUser by ensuring the terminal race-retry fallthrough wraps the underlying store error (%w) instead of returning a bare fmt.Errorf without the root cause.

Changes:

  • Hoists err out of the FindByEmail if initializer so it remains in scope for the terminal return.
  • Wraps the final fallthrough error with %w, preserving the underlying ErrNotFound (and error chain) for logging and errors.Is.
Show a summary per file
File Description
handler/oauth2_common.go Preserves underlying error context in the terminal race-retry failure path of findOrCreateUser.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread handler/oauth2_common.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@veverkap veverkap merged commit b2e1f8f into main May 11, 2026
14 checks passed
@veverkap veverkap deleted the copilot/fix-oidc-user-error-handling branch May 11, 2026 15:41
github-actions Bot added a commit that referenced this pull request May 11, 2026
…reateUser

The fix in #234 wraps the terminal ErrNotFound fallthrough in
findOrCreateUser with context ("failed to resolve user after race retry").

Update the OAuth2Handler and OIDCHandler callback-behaviour sections to
document this rare edge case: when the race-retry still cannot locate the
user, the callback returns HTTP 500 and logs the wrapped error under the
existing 'user resolution failed' slog message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
veverkap added a commit that referenced this pull request May 11, 2026
…reateUser (#252)

* docs(handler): document race-retry terminal error wrapping in findOrCreateUser

The fix in #234 wraps the terminal ErrNotFound fallthrough in
findOrCreateUser with context ("failed to resolve user after race retry").

Update the OAuth2Handler and OIDCHandler callback-behaviour sections to
document this rare edge case: when the race-retry still cannot locate the
user, the callback returns HTTP 500 and logs the wrapped error under the
existing 'user resolution failed' slog message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs(handler): fix error string, terminology, and JSON note in race-retry docs

- Use full error string "failed to resolve user after race retry: not found"
  instead of the truncated wrapper-only form in both oauth2.md and oidc.md
- Replace inverted "wrapped cause" phrasing with "error" in oidc.md so
  the outer wrapper and inner cause are not confused
- Rephrase scenario from "account was removed" to "inconsistent-store outcome
  (e.g. concurrent deletion or read-after-write lag)" to avoid over-asserting
  the cause of the ErrNotFound fallthrough
- Change "Callback does not return JSON" to "does not return JSON on success"
  to match oauth2.md and reflect that error paths do return JSON

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Patrick Veverka <veverkap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants