Repository navigation
fix: make account creation idempotent and surface Linear API errors - #62
russellhaering wants to merge 3 commits into
Conversation
CreateAccount always sent a workspace invite, which Linear rejects when the user is already a member or already has a pending invite. Retries of a create that had already succeeded therefore failed with an opaque "success=false" error. - Look up the user by email first and return AlreadyExists for active users; fail with a clear message for suspended users. - Return ActionRequired when an unexpired invite is already pending. - Include Linear's GraphQL error message when an invite fails. - Only treat a 400 as rate limiting when Linear says so (RATELIMITED error code or an exhausted rate-limit header), so invalid requests are no longer retried as if they were rate limited.
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return nil, err | ||
| } | ||
|
|
||
| if len(res.Data.Users.Nodes) == 0 { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): GraphQLUsersResponse has no errors field. If Linear answers this lookup with HTTP 200 plus GraphQL errors (for example a resolver or permission error), nodes comes back empty. The user is then reported as not found, and CreateAccount sends a duplicate invite, which skips the idempotency check this PR adds. GetPendingOrganizationInvite has the same gap. Consider embedding GraphQLError in both lookup responses and returning an error when HasErrors() is true.
There was a problem hiding this comment.
Fixed in 675b981: both lookups now embed GraphQLError and return an error when HasErrors() is true, so a failed lookup no longer looks like "not found". Added tests for each.
| if !res.Data.OrganizationInvites.PageInfo.HasNextPage { | ||
| return nil, nil | ||
| } | ||
| after = res.Data.OrganizationInvites.PageInfo.EndCursor |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): if Linear ever returns hasNextPage: true with an empty endCursor, after resets to "" and the loop refetches page 1 until the context is cancelled. Guard with if after == "" { return nil, error }. Also note that every CreateAccount now pages through the workspace's entire invite history, accepted invites included. That cost grows over time, so a server-side filter would help if the API supports one.
There was a problem hiding this comment.
Fixed in 675b981: pagination now returns an error if endCursor is empty or unchanged. On cost: Linear's organizationInvites query takes no filter argument, so a client-side scan is the only option. It runs only on account creation, not during sync.
| return resp, rlData, uhttp.WrapErrorsWithRateLimitInfo(codes.Unavailable, resp, err) | ||
| } | ||
|
|
||
| if gqlErr.HasErrors() { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): uhttp.WithErrorResponse (vendor pkg/uhttp/wrapper.go:531-563) already returns "Request failed with status %d: " + resource.Message(), and Do carries that into err. Appending gqlErr.Message() here prints the message twice, e.g. ...status 400: Argument Validation Error: Argument Validation Error. Consider dropping this append; non-rate-limit HTTP errors already included Linear's message before this PR.
There was a problem hiding this comment.
Fixed in 675b981: removed the extra wrap, since WithErrorResponse already includes the message. The existing TestDoRequestBadRequest still checks that Linear's message appears in the error.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…ation Return an error when the user or invite lookup carries GraphQL errors instead of treating it as not found, stop invite pagination if the cursor does not advance, and drop the duplicated error message in doRequest.
Superseded — see the current review report for commit
|
| } | ||
| if existing != nil { | ||
| if !existing.Active { | ||
| return nil, nil, nil, fmt.Errorf("baton-linear: %s already has a suspended Linear account, which must be reactivated rather than invited", email) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this provisioning error has no gRPC status code, so it comes through as Unknown (criteria P4/E3). Retrying won't fix it until someone reactivates the account, so status.Errorf(codes.FailedPrecondition, ...) would show the failure correctly in C1.
There was a problem hiding this comment.
Fixed in 1fe4c2a: now returns codes.FailedPrecondition, and TestUserCreateAccount_SuspendedUser asserts the code.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Connector PR Review: fix: make account creation idempotent and surface Linear API errorsBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryBefore inviting, The new commit ( I rescanned the whole PR diff for security and correctness. In I applied the repo criteria:
The incremental diff was complete ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
| } | ||
| if existing != nil { | ||
| if !existing.Active { | ||
| return nil, nil, nil, status.Errorf(codes.FailedPrecondition, "baton-linear: %s already has a suspended Linear account, which must be reactivated rather than invited", email) |
There was a problem hiding this comment.
A suspended account is still a duplicate, and this branch makes a deprovision/reprovision cycle unrecoverable.
Delete on this same type calls userSuspend (user.go:229) — Linear has no hard delete — so "suspended" is exactly the state C1's own deprovision leaves behind. The next CreateAccount for that email finds it here and fails, and the failure is terminal: pkg/tasks/c1api/create_account.go:53 joins ErrTaskNonRetryable to whatever CreateAccount returns without reading the gRPC code, and pkg/connectorbuilder/accounts.go builds no retryer (contrast resource_provisioner.go:83/:129 for Grant/Revoke). So the FailedPrecondition we added in 1fe4c2a improves how the error reads but doesn't make it retryable. There's no in-connector recovery either — we have SuspendUser but no unsuspend, and no enable_user action — so someone has to reactivate in Linear's UI.
Suggestion
Drop the status gate so both cases return the existing account:
if existing != nil {
ur, err := userResource(ctx, existing, &v2.ResourceId{...})
if err != nil {
return nil, nil, nil, err
}
return &v2.CreateAccountResponse_AlreadyExistsResult{Resource: ur}, nil, nil, nil
}Why
GetUsersalready syncs disabled users (includeDisabled: true, client.go:172) anduserResourcestamps themRESOURCE_STATUS_DISABLED(user.go:75-78), so the resource we'd return is one C1 has already seen, with accurate status. Nothing is misreported.- baton-okta is the only shipped connector that gates this on status, and only for
DEPROVISIONED, justified in the comment as "a login that can never be provisioned". Okta's reversibleSUSPENDEDfalls through toAlreadyExistsResult. Linear has no deprovisioned state —userSuspendis reversible — so every collision here is the Okta-SUSPENDEDcase. - Re-enabling is a separate lifecycle concern. If we want the account usable again, the thing to add is
userUnsuspendas anenable_useraction, not a failure on create.
Separately: the message tells the operator the account "must be reactivated", but we don't expose a way to do that and docs/connector.mdx doesn't describe one. Worth rewording whatever happens to the branch.
See also
- Sibling: baton-okta
user.goL479-497 — the DEPROVISIONED carve-out and theSUSPENDEDfall-through - Sibling: baton-github
invitation.goL270-275 —AlreadyExistsResultwith no status check at all
| } `json:"organizationInvites"` | ||
| } `json:"data"` | ||
| } | ||
| resp, _, err := c.doRequest(ctx, b, &res) |
There was a problem hiding this comment.
This drops the rate-limit annotation, and this is the call where it matters most.
doRequest already builds the descriptor (uhttp.WithRatelimitData) and sets STATUS_OVERLIMIT on the throttle path, but resp, _, err := discards it here and at line 614 in GetUserByEmail, and neither signature returns annotations.Annotations — so CreateAccount hands C1 nil on every path. The error path is the one that hurts: a 429 is an error, so the throttled response carrying the budget the platform needs for backoff reports nothing.
It compounds with the scan: since organizationInvites takes no filter, every CreateAccount pages the whole invite history. On a workspace with a few thousand historical invites that's dozens of requests per provision, all silent.
Suggestion
Return annotations from both new methods and thread them through CreateAccount's third return value. In the page loop, keep the newest descriptor rather than accumulating:
func (c *Client) GetPendingOrganizationInvite(ctx context.Context, email string) (*OrganizationInvite, annotations.Annotations, error) {
var annos annotations.Annotations
for {
resp, rlData, err := c.doRequest(ctx, b, &res)
annos.WithRateLimiting(rlData) // Update semantics: freshest wins
…Why
review-client-layerRule 4 flags a method that calls the shared helper and discards its annotations.- Linear does publish the headers (
X-RateLimit-Requests-*,X-RateLimit-Complexity-*), so there's real data being thrown away. - baton-zoom and baton-docusign both use a plain variable assigned per call rather than an accumulator — worth copying that shape rather than inventing a fold.
See also
- API: Linear rate limiting — the headers
doRequestis already parsing
| if err == nil || resp == nil { | ||
| return resp, rlData, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we have a captured 400 that motivated this? Linear's rate-limiting reference says rate-limited requests return 429, with extensions.code: RATELIMITED and a Retry-After header — it documents no 400 for rate limiting.
Narrowing the old blanket "every 400 is a rate limit" is clearly the right direction. What I'd like to pin down is the header fallback in isRateLimited: a bare Remaining == "0" test is a second, weaker signal for a condition Linear already signals two documented ways, both of which the first branch and gqlErr.IsRateLimited() already cover — and it's applied to a status code the docs say never carries a rate limit.
If the 400 is real, could the comment name the condition that produces it? If it isn't reproducible any more, I think the header loop can go and the RATELIMITED code carries the whole rule.
Summary
Account provisioning always sent a workspace invite. Linear rejects an invite when the user is already a member or already has a pending invite, so a retried or repeated create failed with
organizationInviteCreate returned success=falseand no detail.Changes
CreateAccountlooks the user up by email (case-insensitive, including suspended users) before inviting. An active user is returned asAlreadyExistsResult. A suspended user returns an error explaining that the account must be reactivated.ActionRequiredResultinstead of sending a duplicate.RATELIMITEDerror code, or an exhaustedX-RateLimit-*-Remainingheader). Previously every 400 was converted into a retryable rate-limit error, so invalid requests were retried until timeout.Testing
go test ./pkg/...passes. New tests cover the existing-user, suspended-user, pending-invite and invite-error cases, plus rate-limit vs. invalid-request 400s and the email lookup.usersfiltered by email,organizationInvites) against the live Linear GraphQL schema with a read-only query.golangci-lintlocally; my installed binary is older than this module's Go version, so CI covers lint.