fix: attach rate limit details to gRPC error status for SDK retry

[btipling]

Summary

• Attaches RateLimitDescription to gRPC error status details (via st.WithDetails()) so the SDK's retry logic uses proper backoff
• WrapError now sets rate limit info on both the error (for retry) and annotations (for SyncOpResults)
• WrapErrorWithRateLimitOverride re-wraps errors with 60s on both the gRPC status and annotations
• Adds test verifying gRPC status details are populated

Context

Resolves CXH-1301

The SDK retry logic (retry.go:87-111) extracts RateLimitDescription from status.FromError(err).Details(), not from SyncOpResults annotations. Our previous fix only set annotations — the SDK ignored them and fell back to linear backoff (1s, 2s, 3s...), confirmed by Datadog logs showing "wait": 7, "wait": 8 instead of 60.

Test plan

• Unit tests pass: go test ./pkg/connector/client/ -v
• Deploy and verify Datadog logs show "wait": 60 on retry after 429
• Verify sync eventually completes for large workspaces

🤖 Generated with Claude Code

[linear]

CXH-1301 Slack connector never works for Verkada tenant

[github-actions]

Claude finished @btipling's task in 1m 16s —— View job

---

PR Review: Rate Limit gRPC Status Details

• Read PR diff and changed files
• Review code quality, correctness, and patterns
• Post review feedback

---

Overall: This looks correct and well-motivated. The core insight — that the SDK reads RateLimitDescription from gRPC status details, not from SyncOpResults annotations — explains the observed linear backoff behavior. The fix properly attaches rate limit info to both channels.

Specific Feedback

1. errors.Join ordering matters for double-wrapping (subtle but correct)

In user.go:247-251 and user_group.go:168-172, the error gets wrapped twice when the rate limit override kicks in:

wrappedErr := client.WrapError(err, ..., &annos)           // gRPC status with original retry-after
wrappedErr = client.WrapErrorWithRateLimitOverride(wrappedErr, &annos)  // gRPC status with 60s

wrapErrorWithRateLimitDetails uses errors.Join(st.Err(), err), placing the new status first. Since errors.As traverses joined errors in order, status.FromError will find the 60s status before the original one. This is correct, but it's a subtle ordering dependency. A brief comment on line 232 would help future readers understand why the new status is the first argument:

// New status is first so errors.As finds the override before the original.
return errors.Join(st.Err(), err)

2. Silently discarding WithDetails error

helpers.go:230:

st, _ = st.WithDetails(rlDesc)

WithDetails can only fail if proto marshaling fails, which won't happen for RateLimitDescription. This is fine in practice but consider logging the error at debug level if you want to be defensive.

3. Test is solid but could cover the override path too

The new test validates that WrapError attaches gRPC status details for a RateLimitedError. Consider adding a test that verifies WrapErrorWithRateLimitOverride produces a 60s status detail (not just the annotation — TestRateLimitOverrideReplacesExisting only checks annotations). This would catch regressions in the double-wrap path.

4. Minor: WrapErrorWithRateLimitOverride always uses codes.Unavailable

This is fine since it's only called after IsRateLimited confirms the error is rate-limit related, but the hardcoded "rate limited" message loses the original context message. The original wrappedErr is preserved as a joined error, so callers can still unwrap it — just noting this for awareness.

Nits

• The SlackErrorResponse path (helpers.go:87-93) now returns early with wrapErrorWithRateLimitDetails before reaching line 94's uhttp.WrapErrors. This is correct and cleaner than the old flow.

Verdict: Approve. The approach is sound, the fix addresses the root cause, and the test validates the key behavior. The suggestions above are minor improvements, not blockers.