CXP-857 Handle invite account provisioning duplicates and pending state - #23
Conversation
CreateAccount now returns ActionRequiredResult for fresh invites (Segment never assigns a UID until the invite is accepted) and disambiguates duplicate-invite errors by scanning pending invites for a match, since Segment returns the identical error for "already invited" and "already a member" and exposes no email-filtered lookup for either. Confirmed against the live API that DELETE /invites is itself idempotent.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXP-857 Handle invite account provisioning duplicates and pending stateBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff (5 files, +153/-18) for security and correctness: the invite Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
- Return ActionRequiredResult instead of AlreadyExistsResult when the pending-invite lookup itself fails, rather than assuming the duplicate is a full member. - Propagate the pending-invite scan's rate limit info to CreateAccount's output annotations. - Make the mock server's DELETE /invites idempotent, matching the confirmed live behavior and the connector's Delete. - Normalize the mock's invite/user email comparisons to be case-insensitive, matching FindPendingInviteByEmail.
luisina-santos
changed the title
| } | ||
|
|
||
| existingEmail, found, scanRateLimit, lookupErr := b.client.FindPendingInviteByEmail(ctx, email) | ||
| outputAnnotations.WithRateLimiting(scanRateLimit) |
There was a problem hiding this comment.
🟡 Suggestion: annotations.Update replaces an existing annotation of the same type, so this call overwrites the CreateInvites rate-limit descriptor set on line 132. That is fine when the scan produced real data, but doRequest returns &ratelimitData unconditionally (a zero-valued struct when the transport call fails before a response), so FindPendingInviteByEmail's if rl != nil guard at client.go:329 cannot filter that out — on the lookupErr path the good create-call descriptor gets replaced with STATUS_UNSPECIFIED / limit 0 / remaining 0. A nil scanRateLimit also clobbers, since a typed-nil *v2.RateLimitDescription is not caught by Update's msg == nil check. Suggest guarding on content, e.g. if scanRateLimit.GetStatus() != v2.RateLimitDescription_STATUS_UNSPECIFIED || scanRateLimit.GetLimit() > 0 { outputAnnotations.WithRateLimiting(scanRateLimit) }. (medium confidence on impact, high on mechanism)
There was a problem hiding this comment.
Fixed — added a hasRateLimitData guard before attaching the scan's rate limit, so a zero-valued *v2.RateLimitDescription from a failed/short-circuited page (which doRequest returns non-nil even on transport failure) can no longer overwrite the real one from CreateInvites.
| if lookupErr != nil { | ||
| l.Debug("failed to scan pending invites for duplicate lookup", zap.String("email", email), zap.Error(lookupErr)) | ||
| return &v2.CreateAccountResponse_ActionRequiredResult{ | ||
| Message: fmt.Sprintf("Segment reported %s as a duplicate, but the pending-invite lookup used to confirm whether it's already a full member failed. Retry account creation.", email), | ||
| }, nil, outputAnnotations, nil |
There was a problem hiding this comment.
🟡 Suggestion: the mis-attribution from the previous review is fixed, but lookupErr is still discarded — logged at Debug (below the default level) and returned as ActionRequiredResult with a nil error. A permanent failure such as 401/403 (token missing invite-read scope) or a 5xx becomes a benign "Retry account creation", so retries loop with no operator signal; context.Canceled mid-scan likewise reports action-required instead of cancellation. Suggest logging at Warn per the repo's log-level rules and returning the wrapped error (or at minimum propagating cancellation) rather than swallowing it into a non-error result.
There was a problem hiding this comment.
Kept as ActionRequiredResult (not propagating the error) — this was investigated further, not overlooked. Checked the C1 platform repo (ductone/c1): ActionRequiredResult and a resource-less AlreadyExistsResult are handled identically by AccountManagementController (create_account.go) — both park the ticket for a sync-driven resume, or in the automation/FSM path (no ticket to park) become a non-retryable workflow error with the connector's message surfaced directly (workflow_execution_process.go). There's an explicit comment in that file noting a real Go error from CreateAccount was tried for a related case and caused 'retry-until-timeout instead of fail-fast' in that same automation path. So propagating this lookup error as a hard error risks exactly that blind-retry behavior, for no benefit — the parking/non-retryable-failure semantics we get from ActionRequiredResult already surface the failure without it. Log level is intentionally Debug, not Warn — the message itself already reaches the tenant through the platform's action-required surface, so a louder connector-side log isn't adding new visibility.
| if ts.invites[strings.ToLower(invite.Email)] { | ||
| // Verified against the live Segment API. | ||
| logf("❌ POST /invites - already invited: %s", invite.Email) | ||
| http.Error(w, `{"errors":[{"type":"bad-request","message":"One or more email address was already invited to join workspace."}]}`, http.StatusBadRequest) |
There was a problem hiding this comment.
🟡 Suggestion: http.Error forces Content-Type: text/plain; charset=utf-8, so this JSON body never reaches uhttp's JSON error path — WithErrorResponse bails at the IsJSONContentType check and returns the "unexpected content type for JSON error response … body: %s" error instead. IsAlreadyExistsError still matches only because the raw body is interpolated into that message, so CI never exercises the ErrorResponse.Message() decode that production depends on. Write the header explicitly (w.Header().Set("Content-Type", "application/json"); w.WriteHeader(http.StatusBadRequest); w.Write(...)) to make the mock match the real error shape the PR describes.
There was a problem hiding this comment.
Fixed — added a sendJSONError helper that sets Content-Type: application/vnd.segment.v1+json and writes the body directly, instead of http.Error (which forces text/plain). This now exercises the real ErrorResponse.Message() JSON-decode path on the client instead of only matching via the generic 'unexpected content type' fallback message.
- Guard against a zero-valued rate limit from a failed pending-invite scan overwriting the real one from CreateInvites. - Fix the mock's mismatched Content-Type on duplicate-invite errors so it exercises the client's real JSON error decode path. - Drop the dead codes.AlreadyExists branch (Segment returns 400, not 409, for this endpoint). - Preserve original email casing in the mock's invite store so CreateAccount and the next List() sync mint the same resource ID for a mixed-case email. - Simplify a redundant presence check on the profile map. Confirmed against the C1 platform (ductone/c1) that a resource-less AlreadyExistsResult and ActionRequiredResult are handled identically by AccountManagementController - both park the ticket for a sync-driven resume, or fail non-retryably in automation/workflow paths. A genuine error from CreateAccount was found to cause retry-until-timeout in that same automation path instead, so the pending-invite lookup failure keeps returning ActionRequiredResult rather than propagating the error.
- CreateAccount's fresh-invite success path now builds the resource from CreateInvites' own echoed email instead of the caller's raw input, matching how the duplicate branch already trusts the API's returned value over guessing at casing normalization. - hasRateLimitData now also treats a non-zero ResetAt as real data and explicitly guards a nil input, so a legitimate response with only a reset header isn't mistaken for the zero-valued struct doRequest returns on transport failure.
| if rl.GetStatus() != v2.RateLimitDescription_STATUS_UNSPECIFIED || rl.GetLimit() > 0 { | ||
| return true | ||
| } | ||
| return !rl.GetResetAt().AsTime().IsZero() |
There was a problem hiding this comment.
🟠 Bug: this new line makes hasRateLimitData return true unconditionally for any non-nil rl, defeating the guard in exactly the case its doc comment names. (*timestamppb.Timestamp)(nil).AsTime() is time.Unix(0,0).UTC() = 1970-01-01, and time.Time.IsZero() is only true for Jan 1 year 1 — so a nil ResetAt yields !IsZero() == true. doRequest returns &ratelimitData (a zero-valued struct with ResetAt == nil) on transport failure, so pkg/connector/invites.go:139 now always overwrites the real CreateInvites rate-limit annotation with the all-zero one from a failed scan.
Note the real-response case works correctly for the opposite reason: ratelimit.ExtractRateLimitData always sets ResetAt: timestamppb.New(resetAt), and a missing reset header gives the Go zero time, which does satisfy IsZero(). So the nil-vs-set distinction is what needs checking:
| return !rl.GetResetAt().AsTime().IsZero() | |
| return rl.HasResetAt() && !rl.GetResetAt().AsTime().IsZero() |
There was a problem hiding this comment.
Confirmed real bug, fixed — added the HasResetAt() guard as suggested. Verified with a standalone check against all four cases (doRequest's fake zero-valued struct, a real response with no reset header, a real response with a reset time, and nil): only the two real-reset-time cases now return true.
A nil ResetAt (doRequest's zero-valued struct on transport failure) converts via AsTime() to the Unix epoch, not Go's zero time, so !IsZero() was true for it - the opposite of the intended guard. Add HasResetAt() so the IsZero() check only runs when ResetAt was actually populated, which is also true for ExtractRateLimitData's real no-reset-header case (it always sets ResetAt, just to Go's zero time).
| for { | ||
| response, rl, err := c.ListInvites(ctx, cursor, DefaultPageSize) | ||
| if rl != nil { | ||
| lastRateLimit = rl | ||
| } | ||
| if err != nil { | ||
| return "", false, lastRateLimit, fmt.Errorf("find pending invite by email: %w", err) | ||
| } | ||
| for _, inviteEmail := range response.Data.Invites { | ||
| if strings.EqualFold(inviteEmail, email) { | ||
| return inviteEmail, true, lastRateLimit, nil | ||
| } | ||
| } | ||
| cursor = response.Data.Pagination.Next | ||
| if cursor == "" { | ||
| return "", false, lastRateLimit, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this loop has no upper bound and no cursor-progress check, so it relies entirely on Segment eventually returning an empty pagination.next. If the API ever returns a cursor that doesn't advance (a stale or repeated cursor), this spins forever issuing GET /invites inside a single CreateAccount call, only stopping when the caller's context deadline fires. Consider capping the scan at N pages and bailing if the new cursor equals the previous one, returning found=false in that case.
| outputAnnotations.WithRateLimiting(scanRateLimit) | ||
| } | ||
| if lookupErr != nil { | ||
| l.Debug("failed to scan pending invites for duplicate lookup", zap.String("email", email), zap.Error(lookupErr)) |
There was a problem hiding this comment.
🟡 Suggestion: lookupErr here can be a real upstream failure (401, 5xx, transport error) but it's logged at Debug and then discarded — the SDK sees a nil error, so it won't retry, and at the default log level an operator gets the "retry account creation" message with no trace of the cause. Skip-and-continue is the right call, but log it at Warn with zap.Error(lookupErr) so the underlying failure is visible.
|
|
||
| const alreadyInvitedMessage = "was already invited to join workspace" | ||
|
|
||
| func IsAlreadyExistsError(err error) bool { |
There was a problem hiding this comment.
🟡 Suggestion: IsAlreadyExistsError and hasRateLimitData are both pure, dependency-free functions that gate the entire duplicate-handling path, and neither gets a unit test in this PR. hasRateLimitData already shipped a logic bug in this branch (the nil-ResetAt case fixed in 2487f5a) that a three-case table test would have caught. A small helpers_test.go covering the real Segment message, a non-matching error, and nil — plus zero/nil/populated RateLimitDescription — would lock in both.
3 participants
Summary
CreateAccounton the invite resource type now returnsActionRequiredResultfor a fresh invite instead ofSuccessResult, since Segment never assigns a stable UID until the invitee accepts.client.FindPendingInviteByEmail, unbounded pagination) for a match: found →ActionRequiredResultwith the pending invite resource attached; not found →AlreadyExistsResult{}(Segment returns the identical error message whether the email is a pending invite or an existing full member, and exposes no email-filtered lookup for either, so a full member can't have a resource attached without an unjustified full scan of all users).pkg/connector/client/helpers.go:IsAlreadyExistsErrormatches Segment's confirmed live error text ("was already invited to join workspace").Deleteno longer has a not-found branch — confirmed against the live API thatDELETE /invitesis itself idempotent and always returns success.cmd/test-server/handlers.go: mock now returns the real confirmed error shape (400,type: "bad-request") for duplicate invites so CI actually exercises these paths.Test plan
go build ./...,go vet ./...,go test ./...GET /invitesscan); duplicate existing member → AlreadyExists with no resource; delete → success.400,bad-request, "was already invited to join workspace") confirmed against the live Segment API for both the pending-invite and already-a-member cases.Deleteidempotency confirmed live by the requester; not independently re-verified in this PR.