diff --git a/pkg/accessreview/drivers/name_resolver.go b/pkg/accessreview/drivers/name_resolver.go index 3f3b9a4ac..9d1553dfd 100644 --- a/pkg/accessreview/drivers/name_resolver.go +++ b/pkg/accessreview/drivers/name_resolver.go @@ -41,14 +41,10 @@ type NameResolver interface { ResolveInstanceName(ctx context.Context) (string, error) } -// ErrTerminalNameResolution marks a name-resolution failure as permanent: -// an auth or bad-request response that retrying cannot fix. The -// source-name worker treats it as terminal — it keeps the generic source -// name and marks the source synced instead of re-claiming it every poll. -// Transient failures (5xx, network errors) are returned as plain errors so -// they keep retrying. Name resolution is best-effort display metadata, so a -// permanent failure must never wedge the worker in a retry loop (a single -// unauthorized source otherwise produced millions of error logs in prod). +// ErrTerminalNameResolution marks a permanent name-resolution failure +// (auth/bad-request) that retrying cannot fix: the source-name worker keeps +// the generic name and marks the source synced instead of re-claiming it. +// Transient failures (5xx, network) stay plain errors so they keep retrying. var ErrTerminalNameResolution = errors.New("terminal name resolution failure") // nameStatusError classifies a non-2xx response from a name-resolution diff --git a/pkg/accessreview/drivers/tailscale.go b/pkg/accessreview/drivers/tailscale.go index 1a72d6ae2..b5b79036c 100644 --- a/pkg/accessreview/drivers/tailscale.go +++ b/pkg/accessreview/drivers/tailscale.go @@ -151,9 +151,9 @@ func (d *TailscaleDriver) fetchUsers(ctx context.Context) ([]tailscaleUser, erro }() // Classify the status so the source-name worker (which reuses this via - // tailscaleNameResolver) treats a 4xx as terminal instead of hot-looping. - // The sentinel is inert on the ListAccounts sync path, which does not - // inspect it. + // tailscaleNameResolver) treats an auth/not-found 4xx as terminal instead + // of hot-looping. The sentinel is inert on the ListAccounts sync path, + // which does not inspect it. if httpResp.StatusCode < 200 || httpResp.StatusCode >= 300 { return nil, nameStatusError("tailscale users", httpResp.StatusCode) } diff --git a/pkg/accessreview/source_name_worker.go b/pkg/accessreview/source_name_worker.go index 8b10dc929..bf63e55c6 100644 --- a/pkg/accessreview/source_name_worker.go +++ b/pkg/accessreview/source_name_worker.go @@ -146,15 +146,10 @@ func (h *sourceNameHandler) Process(ctx context.Context, source coredata.AccessR }, ) if err != nil { - // Setting up the resolver failed: the connector is gone, its - // credentials cannot be decrypted, or an eager token refresh failed - // on a revoked OAuth refresh token. Returning nil without marking the - // source synced leaves name_synced_at NULL, so the worker re-claims - // the same row every drain cycle with no delay — a single dead - // connector then hot-loops the vendor token endpoint (millions of - // error logs in prod). Treat it as terminal: keep the generic name - // and mark the source synced so it stops re-claiming. A - // reconnect/reconfigure clears name_synced_at to try again. + // Resolver setup failed (missing connector, undecryptable credential, + // or an eager refresh on a revoked token). Mark the source synced + // rather than returning nil: an unsynced row is re-claimed every poll + // with no backoff and hot-loops the vendor. A reconnect clears it. h.logger.WarnCtx( ctx, "cannot set up name resolver, keeping generic name", diff --git a/pkg/accessreview/source_service.go b/pkg/accessreview/source_service.go index aa41056c1..d3667e869 100644 --- a/pkg/accessreview/source_service.go +++ b/pkg/accessreview/source_service.go @@ -582,18 +582,14 @@ func (s *Service) SourceNeedsConfiguration( return cfg.SelectedSlug(dbConnector) == "", nil } -// AutoSelectDefaultOrganization picks the first workspace/org the connector -// can see for a freshly linked picker-provider source that has none selected -// yet. Without it a connected source stays "needs configuration" until the -// user completes the picker; if they skip it, the first campaign silently -// resolves no users (the driver requires an org). Defaulting to the first -// available makes the source immediately usable; the picker stays available -// to switch when several are listed. +// AutoSelectDefaultOrganization picks the first workspace/org a freshly linked +// picker-provider source can see when none is selected yet, so the source is +// usable immediately instead of failing its first campaign fetch. The picker +// stays available to switch when several are listed. // -// Best-effort: any failure (provider unreachable, nothing listed) leaves the -// source in its existing "needs configuration" state, where the picker is the -// fallback. It never returns an error and must not fail the create/update -// that triggered it. +// Best-effort: any failure leaves the source in its "needs configuration" +// state (the picker is the fallback); it never errors and must not fail the +// create/update that triggered it. func (s *Service) AutoSelectDefaultOrganization( ctx context.Context, scope coredata.Scoper, diff --git a/pkg/coredata/access_review_source.go b/pkg/coredata/access_review_source.go index b3ec71d15..0f4a50ea6 100644 --- a/pkg/coredata/access_review_source.go +++ b/pkg/coredata/access_review_source.go @@ -360,11 +360,8 @@ WHERE } // ClearNameSyncedAtByConnectorID resets name_synced_at to NULL for every -// access source backed by connectorID so the source-name worker re-resolves -// the display name. A reconnect (possibly with a new scope/org) or a manual -// org (re)configuration can change the resolvable instance name; without this -// a source that was terminal-marked keeps its generic name forever. It is a -// no-op when no source references the connector. +// access source backed by connectorID, so the source-name worker re-resolves +// their display name. No-op when no source references the connector. func (sources *AccessReviewSources) ClearNameSyncedAtByConnectorID( ctx context.Context, conn pg.Tx, diff --git a/pkg/coredata/connector_provider.go b/pkg/coredata/connector_provider.go index 25f7e0d0c..7f15240a8 100644 --- a/pkg/coredata/connector_provider.go +++ b/pkg/coredata/connector_provider.go @@ -58,14 +58,8 @@ const ( ConnectorProviderAsana ConnectorProvider = "ASANA" ConnectorProviderNetlify ConnectorProvider = "NETLIFY" ConnectorProviderClickUp ConnectorProvider = "CLICKUP" - // ConnectorProviderClerk is retained for existing connectors but is - // no longer a registerable access-review provider: Clerk's Backend API - // (secret key) only exposes the customer's application end-users, not - // the Clerk workspace/dashboard team who administer the platform, so a - // campaign reviews the wrong population. Kept in IsValid and the - // GraphQL enum so stored CLERK rows still validate and serialize; - // dropped from ConnectorProviders and unregistered from the builtin - // registry so it cannot be added or fetched. + // ConnectorProviderClerk is disabled (unregistered) but kept in IsValid + // and the GraphQL enum so existing stored CLERK rows still validate. ConnectorProviderClerk ConnectorProvider = "CLERK" ConnectorProviderVercel ConnectorProvider = "VERCEL" ConnectorProviderMonday ConnectorProvider = "MONDAY" diff --git a/pkg/server/api/console/v1/resolver.go b/pkg/server/api/console/v1/resolver.go index 79c99eb25..e1c755dbe 100644 --- a/pkg/server/api/console/v1/resolver.go +++ b/pkg/server/api/console/v1/resolver.go @@ -366,11 +366,9 @@ func handleConnectorComplete( } } - // Vercel surfaces the customer's team as the `teamId` OAuth - // callback query parameter (camelCase, not in the token response - // body). When the install targets a personal account no teamId is - // sent — fall back to /v2/user.id as a synthetic TeamID; the v3 - // members endpoint accepts personal-account UIDs. + // Personal-account installs send no teamId; fall back to + // /v2/user.id as a synthetic TeamID (the v3 members endpoint + // accepts personal-account UIDs). if connectorProvider == coredata.ConnectorProviderVercel { teamID := vercelCallbackTeamID(query) if teamID == "" { @@ -476,10 +474,9 @@ func handleConnectorOAuth2Error( safeRedirect.Redirect(w, r, parsedURL.String(), "/", http.StatusSeeOther) } -// vercelCallbackTeamID returns the team identifier Vercel surfaces on the -// OAuth callback. Vercel uses the camelCase `teamId` query parameter (not the -// snake_case `team_id` most other params use); the name is pinned by a test so -// it cannot silently regress and leave every Vercel source without a team. +// vercelCallbackTeamID returns the team identifier from Vercel's OAuth +// callback. Vercel uses the camelCase `teamId` query param (not snake_case +// `team_id`); the name is pinned by a test so it cannot silently regress. func vercelCallbackTeamID(query url.Values) string { return query.Get("teamId") }