[CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplification - #68
[CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplification#68FeliLucero1 wants to merge 17 commits into
Conversation
Pylon #11445 (The Trade Desk): initial full sync fails non-recoverably once an account exceeds DocuSign's hourly API-call budget. DocuSign signals this via a JSON error-body errorCode (HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400 — DocuSign is mid-migration to 429 and documents parsing errorCode instead of relying on status code. uhttp.GrpcCodeFromHTTPStatus maps 400 to codes.InvalidArgument, which the SDK's sync-retry loop (pkg/sync's Retryer, wired to SyncResourcesOp/SyncGrantsOp) treats as fatal — it only waits and retries on Unavailable/DeadlineExceeded — so an otherwise-recoverable rate limit surfaced as a permanent sync failure with no checkpoint resume. doRequestCommon now recognizes this errorCode independent of HTTP status (so it keeps working once DocuSign flips to 429) and reclassifies it as codes.Unavailable with a RateLimitDescription attached via status details, so the SDK's existing retry loop picks it up and the sync pauses/resumes instead of failing outright.
…#11445) userBuilder.Grants() called GetUserDetails once per user, every sync — real N+1 amplification contributing to accounts hitting DocuSign's hourly call budget. The list response (GetUsers) already carries the user's permission profile name and status, both already captured on the resource's profile during List(). Grants now resolves the profile ID via GetPermissionProfiles (one account-wide call, already served from uhttp's default GET cache on repeat calls within a sync) instead, gated on status == Active — the same distinction GetUserDetails.PermissionProfileID-empty already relies on, not a new assumption. Falls back to the original per-user GetUserDetails call unchanged whenever the user isn't Active, the profile field is absent, the cached name no longer resolves (renamed/deleted since listing), or the GetPermissionProfiles call fails. An earlier version of this fix cached GetPermissionProfiles for the builder's lifetime via sync.Once — dropped after review found it silently diverged from the old code for non-active users (no status check at all) and duplicated caching uhttp's GET client already provides by default.
FeliLucero1
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: [CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplificationBlocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit addresses the prior finding about unbounded retries of a persistently failing Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
…-non-recoverable # Conflicts: # pkg/connector/users.go # pkg/connector/users_test.go
| profiles, _, err := b.client.GetPermissionProfiles(ctx) | ||
| if err != nil { | ||
| if isReclassifiedRateLimitError(err) { | ||
| return nil, nil, err, true | ||
| } | ||
| ctxzap.Extract(ctx).Debug("baton-docusign: GetPermissionProfiles failed, falling back to per-user GetUserDetails for this Grants call", | ||
| zap.String("user_id", userID.Resource), zap.Error(err)) | ||
| return nil, nil, nil, false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: uhttp only writes to its GET cache on StatusCode == http.StatusOK (vendor pkg/uhttp/wrapper.go), so a persistent non-rate-limit failure here — e.g. a 403 because the service account can't read permission profiles, which the new permissionProfilesForbidden test exercises — is never cached and is re-attempted for every Active user, then still followed by GetUserDetails. That doubles per-user calls against the same hourly budget this PR is trying to protect. Memoizing the resolved name→ID map and a terminal lookup failure on userBuilder (e.g. sync.Once/mutex-guarded field) would make the fast path independent of the uhttp cache and remove that doubling.
| // docusignRateLimitDefaultResetWindow is the fixed wait this connector asks the SDK's | ||
| // retry loop to use for docusignHourlyRateLimitErrorCode — applied unconditionally, not | ||
| // just as a fallback (see reclassifyHourlyRateLimitError's doc for why response headers | ||
| // are deliberately never consulted for this error). The limit this error names is | ||
| // hourly, so an hour is the sane, safe choice. | ||
| const docusignRateLimitDefaultResetWindow = time.Hour |
There was a problem hiding this comment.
🟡 Suggestion: the hour never actually becomes the wait. retry.Retryer.ShouldWaitAndRetry computes wait from ResetAt and then clamps it with if wait > maxDelay { wait = maxDelay } (vendor pkg/retry/retry.go:121), and the syncer builds its retryer with MaxDelay: 0 (pkg/sync/parallel_syncer.go:151), which NewRetryer normalizes to 60s. So the effective behavior is a 60s retry with unlimited attempts — roughly 60 more failing calls per hour while the account is over budget, not one hour of quiet backoff. The reclassification to Unavailable is still the right fix; the comment just overstates what the window buys, so it's worth documenting the clamp so a future reader doesn't assume the connector sleeps an hour.
There was a problem hiding this comment.
Confirmed and fixed — reworded the comment to state the 60s clamp explicitly, citing retry.go's clamp and parallel_syncer.go's MaxDelay: 0. Behavior is unchanged (the SDK-level gap isn't fixable from this connector), just the doc now matches reality.
…ment - tryFastPathGrant's name-lookup branch relied on uhttp's GET cache to make repeat GetPermissionProfiles calls cheap, but that cache never stores a non-2xx response. A persistent non-rate-limit failure (e.g. a service user lacking permission_profiles read access) re-hit the real API on every Active user instead of once per sync, doubling that user's calls (the failed lookup, then the GetUserDetails fallback) against the same hourly budget this fix exists to protect. Memoizes the call (success or failure) on userBuilder via sync.Once, since Grants() runs concurrently across users sharing one builder per sync. Added a regression test confirming exactly one real call across two Active users during a persistent failure. - docusignRateLimitDefaultResetWindow's doc overstated what the SDK's retry loop actually does with the 1-hour ResetAt: pkg/retry clamps the computed wait to its 60-second MaxDelay default before sleeping, so the net effect is a 60-second retry with unlimited attempts, not an hour of backoff. Corrected the comment; behavior is unchanged (this is an SDK-level gap, not something fixable from this connector). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { | ||
| b.permissionProfilesOnce.Do(func() { | ||
| b.permissionProfiles, _, b.permissionProfilesErr = b.client.GetPermissionProfiles(ctx) | ||
| }) | ||
| return b.permissionProfiles, b.permissionProfilesErr | ||
| } |
There was a problem hiding this comment.
🟠 Bug: (confidence: high) memoizing the rate-limit error makes the sync unrecoverable — the exact case this PR exists to fix. If the first getPermissionProfiles call is the one that hits HOURLY_APIINVOCATION_LIMIT_EXCEEDED, permissionProfilesErr holds that reclassified codes.Unavailable for the builder's lifetime, and tryFastPathGrant returns handled=true, err for it (never falls back). The builder outlives retries: syncOneAction loops f(ctx, action) → retryer.ShouldWaitAndRetry (vendor/.../pkg/sync/parallel_syncer.go:766-780) with MaxAttempts: 0 = unlimited, so every retry replays the memoized error without issuing a single API call — a 60s-per-iteration infinite loop that can never clear, instead of resuming once the hourly window rolls over.
The struct doc's justification ("the fallback path still resolves the grant correctly either way") only holds for the non-rate-limit branch; the rate-limit branch propagates. Only memoize on success and on failures that fall back — e.g. skip caching when isReclassifiedRateLimitError(err) (reset/replace the sync.Once with a mutex + "cached" flag so a later attempt can re-issue the call) and keep memoizing the persistent-403 case the new test covers. Worth a test that calls Grants twice with the rate-limit mock and asserts the second call re-hits permission_profiles.
There was a problem hiding this comment.
Confirmed — this was a real regression I introduced with the memoization fix. Replaced the sticky-on-any-error caching with an allowlist (isCacheablePermissionProfilesError): only PermissionDenied/Unauthenticated/NotFound get cached now, mirroring isOptInFeatureUnavailableError's persistent-failure classification. A reclassified rate-limit error is never cached, so a retry of this same action re-issues a real request and can actually notice the hourly window resetting. Added TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure asserting 2 real calls across 2 retries.
| // the memoization fields' doc on the struct above for why. | ||
| func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { | ||
| b.permissionProfilesOnce.Do(func() { | ||
| b.permissionProfiles, _, b.permissionProfilesErr = b.client.GetPermissionProfiles(ctx) |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) the shared call is bound to whichever user's ctx happens to win the sync.Once race. If that one worker's context is cancelled or deadlines out mid-request, context.Canceled/DeadlineExceeded is memoized (no RateLimitDescription, so isReclassifiedRateLimitError is false) and every remaining Active user in the sync silently drops to per-user GetUserDetails — the 1:1 amplification this PR removes, restored for the rest of the run by one unlucky cancellation. Treating context errors like the rate-limit case (don't cache, let a later caller retry) fixes this alongside the memoization change above.
There was a problem hiding this comment.
Fixed, though not via an explicit context.Canceled/DeadlineExceeded check — switched to an allowlist (isCacheablePermissionProfilesError) that only caches PermissionDenied/Unauthenticated/NotFound. A context error doesn't get a gRPC status code from GrpcCodeFromHTTPStatus (it never got an HTTP response), so status.Code(err) on it falls through to codes.Unknown, which isn't in the allowlist — it's correctly never cached, by construction, without needing a dedicated check. Added TestUserBuilder_Grants_DoesNotMemoizeContextError to lock this in.
Caching a reclassified rate-limit error alongside genuine persistent failures was a blocking regression: the SDK's per-action retry loop reuses this same userBuilder across every retry, so once the first call hit the hourly limit, every subsequent retry replayed the cached Unavailable error without ever issuing a real request again — spinning at the retryer's ~60s interval forever instead of recovering once the account's hourly window resets, defeating the entire point of this PR's error reclassification. Only a genuinely persistent failure (e.g. the existing 403 case) is cached now; a reclassified rate-limit error or a context cancellation/deadline error is left uncached so the next caller re-attempts against the real API. Added a regression test asserting GetPermissionProfiles is called on every retry during a rate-limited window, not just once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if err != nil && (isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) { | ||
| return nil, err | ||
| } | ||
|
|
||
| b.permissionProfilesCached = true | ||
| b.permissionProfiles = profiles | ||
| b.permissionProfilesErr = err | ||
| return profiles, err |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: high) the carve-out is narrower than its own rationale. The reason a rate-limit error must not be cached — the builder outlives the SDK's per-action retry loop, so a cached error replays forever without a fresh request — applies equally to the other transient failures that reach here: a 503 or a network blip (uhttp.wrapTransientNetworkError → codes.Unavailable, no RateLimitDescription, so isReclassifiedRateLimitError is false) gets cached permanently and disables the fast path for the whole sync even after the endpoint recovers. Consequence is milder than the rate-limit case (per-user GetUserDetails fallback, i.e. pre-PR call volume, not a hang), but it's the same shape of staleness. Consider caching only genuinely persistent classes (401/403/404) or attaching a short TTL to the cached failure.
There was a problem hiding this comment.
Confirmed and fixed — same allowlist change as the sibling finding on this thread. isCacheablePermissionProfilesError now only caches PermissionDenied/Unauthenticated/NotFound; a plain transient 503 (codes.Unavailable, no RateLimitDescription) is no longer cached either. Added TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure asserting 2 real calls across 2 retries against a 503 mock.
| } | ||
|
|
||
| if got := atomic.LoadInt32(&permissionProfilesCalls); got != 2 { | ||
| t.Errorf("expected GetPermissionProfiles to be called on every retry (2 calls), got %d — the rate-limit error must not be memoized", got) |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) the rate-limit half of the new carve-out is covered here, but the context-error half (errors.Is(err, context.Canceled) / DeadlineExceeded) has no test. That branch only fires because uhttp.WrapErrors joins the original error (errors.Join(st.Err(), errs...)) and the cancelled-request path returns the *url.Error chain intact — if either changed to a bare status.Error, errors.Is would silently stop matching and context errors would be memoized again, which is exactly the prior review finding this commit set out to fix. A case that cancels the context mid-GetPermissionProfiles, then asserts a second Grants() on the same builder still issues a real call, would pin that down.
There was a problem hiding this comment.
Added — TestUserBuilder_Grants_DoesNotMemoizeContextError: cancels the context before the first Grants() call, then asserts a second call with a fresh context still reaches the real GetPermissionProfiles endpoint (call-counter based, same pattern as the other memoization regression tests).
| b.permissionProfilesMu.Lock() | ||
| defer b.permissionProfilesMu.Unlock() |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) the mutex is now held across the HTTP round trip, and because the rate-limit error is deliberately not cached, every Active user's Grants() during a rate-limited window queues behind a full failing request instead of returning from cache — so worst-case latency for the last waiter grows linearly with the SDK's grants concurrency, and sync.Mutex.Lock can't observe ctx cancellation while waiting. Not incorrect, but if this matters, releasing the lock around the call (double-checked) or gating on a chan struct{}/golang.org/x/sync/singleflight would let cancelled workers bail out.
There was a problem hiding this comment.
Leaving this one as-is. It's correct that the mutex serializes waiters through the full round trip during a rate-limited/unavailable window, but I don't think it's worth the added complexity right now:
- It doesn't reintroduce call amplification — each serialized waiter still only issues one real request each (same total call count as before, just sequential instead of parallel), so it doesn't undo the fix this PR is about.
- The ctx-cancellation-while-waiting-on-Lock gap is real but low-impact: the SDK doesn't aggressively cancel individual Grants() actions in normal operation, so this would only matter under fairly unusual concurrent-cancellation conditions, and even then it's a latency hit, not an incorrect result.
- Fixing it properly (double-checked locking with a released mutex around the call, or singleflight) is real complexity to add correctly, for a benefit that's mostly about worst-case latency during an already-degraded window, not correctness.
Happy to revisit if this shows up as an actual problem in practice.
isCacheablePermissionProfilesError replaces the previous isReclassifiedRateLimitError-specific check: a plain transient 5xx or network blip also arrives as codes.Unavailable but carries no RateLimitDescription, so it fell outside the old carve-out and was being cached permanently — same bug class as the rate-limit case just fixed, just for ordinary transient failures. Now only PermissionDenied/Unauthenticated/NotFound (mirroring isOptInFeatureUnavailableError's persistent-failure classification) are cacheable; everything else, including context errors, is not. Added regression tests for both the newly-widened case (a plain 503) and the previously-untested context-cancellation path (which now works correctly by construction, since Unknown/Canceled codes were never in the cacheable allowlist). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| } | ||
|
|
||
| profiles, _, err := b.client.GetPermissionProfiles(ctx) | ||
| if err != nil && !isCacheablePermissionProfilesError(err) { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): widening the non-cacheable set to "everything except PermissionDenied/Unauthenticated/NotFound" fixes the stale-retry problem, but it also means a persistently failing non-4xx endpoint is now re-requested once per Active user. Under a sustained 500/503 (or a codes.Unknown from a malformed error body), tryFastPathGrant falls back rather than propagating, so every Active user pays two calls — the failed permission_profiles lookup plus GetUserDetails — i.e. 2N calls against the hourly budget vs. N before this PR, the opposite of the amplification reduction this change is for.
Consider a middle ground that keeps the retry-freshness property without unbounded re-requests: cache a non-cacheable error with a short expiry (permissionProfilesErrAt time.Time, re-attempt after e.g. 30–60s), or cap consecutive transient failures before falling back to GetUserDetails for the remainder of the pass. Either preserves the "a reset hourly window / cleared 503 gets noticed" behavior the tests assert while bounding the worst case.
There was a problem hiding this comment.
Confirmed and fixed — added `permissionProfilesTransientFailureThreshold` (3): after that many consecutive transient failures, the builder treats it as a sustained outage rather than a blip and caches it, falling back to the pre-fast-path 1-call-per-user cost for the rest of the sync instead of paying 2 calls per user for its entire duration. A genuine blip still gets retried fresh well within the threshold, so the existing back-to-back-call tests (rate-limit, 503, context) are unaffected. Added `TestUserBuilder_Grants_BoundsTransientFailureRetries` asserting exactly permissionProfilesTransientFailureThreshold real calls occur across more `Grants()` calls than that.
Leaving every transient failure uncached fixed the stale-retry bug, but introduced a new worst case: a *sustained* outage (not just a blip) would cost every Active user two calls (the failed lookup plus the GetUserDetails fallback) for the whole sync — 2N vs. the N calls this fast path exists to avoid, worse than not having it at all. Adds permissionProfilesTransientFailureThreshold (3): after that many consecutive transient failures, the builder treats it as a sustained outage rather than a blip and caches it, falling back to the pre-fast-path 1-call-per-user cost for the remainder of the sync. Genuine blips still get retried fresh (existing tests asserting a second back-to-back call still pass, since they stay under the threshold). Added a regression test asserting exactly permissionProfilesTransientFailureThreshold real calls occur across more Grants() calls than that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if err != nil && !isCacheablePermissionProfilesError(err) { | ||
| b.permissionProfilesTransientFails++ | ||
| if b.permissionProfilesTransientFails < permissionProfilesTransientFailureThreshold { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| b.permissionProfilesCached = true | ||
| b.permissionProfiles = profiles | ||
| b.permissionProfilesErr = err | ||
| return profiles, err |
There was a problem hiding this comment.
🟠 Bug: (confidence: high) the threshold re-introduces the memoized-rate-limit regression this PR already fixed once. isCacheablePermissionProfilesError returns false for the reclassified hourly error (codes.Unavailable + RateLimitDescription), so on the 3rd consecutive occurrence the counter hits the threshold, the return nil, err is skipped, and permissionProfilesErr is cached with that rate-limit error. Every later getPermissionProfiles then returns it from cache, tryFastPathGrant sees isReclassifiedRateLimitError and propagates handled=true, err, and the SDK's per-action retryer (unlimited attempts, reused builder) spins at its clamped 60s interval forever without ever issuing a fresh request — the sync can no longer notice the hourly window resetting. TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure only loops twice, below the threshold, so it still passes.
Suggested fix: exempt the rate-limit error from threshold-caching (e.g. if err != nil && !isCacheablePermissionProfilesError(err) && !isReclassifiedRateLimitError(err) { ... }), or cache a "fast path disabled" sentinel that makes tryFastPathGrant fall back rather than propagate. Worth a test that drives a rate-limited mock past permissionProfilesTransientFailureThreshold and asserts real calls keep being made.
There was a problem hiding this comment.
Confirmed and fixed — the threshold's error path now checks isReclassifiedRateLimitError(err) first and returns uncached unconditionally when true, regardless of the counter. Extended TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure to loop past the threshold (was only 2 calls before, which happened to stay under it) to prove the exemption holds no matter how many times it recurs.
|
|
||
| profiles, _, err := b.client.GetPermissionProfiles(ctx) | ||
| if err != nil && !isCacheablePermissionProfilesError(err) { | ||
| b.permissionProfilesTransientFails++ |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) the counter is documented as counting consecutive transient failures, but it is never reset — and a context error counts toward it. Three cancelled/deadline-exceeded contexts across the sync (each meaning only that one worker's ctx was already done, per isCacheablePermissionProfilesError's own doc) permanently caches that error and drops every remaining Active user to the per-user GetUserDetails fallback, defeating the fast path for an account that is perfectly healthy. Consider excluding context.Canceled/DeadlineExceeded from the counter, since those aren't evidence of a sustained upstream outage.
There was a problem hiding this comment.
Confirmed and fixed — context cancellation/deadline errors are now excluded from incrementing the counter at all (checked alongside the rate-limit exemption, both return uncached before the increment). Extended TestUserBuilder_Grants_DoesNotMemoizeContextError to cancel past the threshold and confirm a later fresh-context call still reaches the real endpoint.
…threshold The threshold added in the previous commit applied uniformly to every non-cacheable error, including the reclassified rate-limit error — so after permissionProfilesTransientFailureThreshold consecutive rate-limit hits, it got cached anyway, reintroducing the exact unrecoverable-sync regression already fixed twice earlier in this PR: the SDK's per-action retry loop would replay the cached codes.Unavailable forever, never re-checking whether the hourly window reset. A rate-limit error now always returns uncached regardless of the counter, no matter how many times it recurs. Also exempts context cancellation/deadline errors from incrementing the counter at all (not just from being cached) — per isCacheablePermissionProfilesError's own reasoning, an unlucky run of cancellations doesn't mean the endpoint is degraded, so it shouldn't accumulate toward disabling the fast path on an otherwise-healthy account. Extended TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure and TestUserBuilder_Grants_DoesNotMemoizeContextError to loop past the threshold, proving both exemptions hold no matter how many times they recur (the previous 2-call versions passed by coincidence, since 2 is below the threshold of 3). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- users.go: extracted newPermissionProfileGrant, replacing 3 identical inline permission_profile-grant constructions (Grants' fallback, tryFastPathGrant's direct-ID branch, and its name-resolved branch). - users_test.go: extracted newCountingPermissionProfilesClient, replacing 5 near-identical ~35-line mock-server setups (differing only in what the permission_profiles endpoint returns) across the memoization regression tests. No behavior change — same assertions, same fixtures, just without the repeated boilerplate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6 participants
Description
Pylon #11445 (The Trade Desk, Odenis Peralta): initial full sync fails non-recoverably once a DocuSign account exceeds its hourly API-call budget (3000/hour). DocuSign returns
HOURLY_APIINVOCATION_LIMIT_EXCEEDEDand the connector surfaces it as a fatalInvalidArgumenterror rather than backing off.Root cause and fix (code-validated, the core of this PR).
uhttp.GrpcCodeFromHTTPStatusmaps this error's HTTP 400 tocodes.InvalidArgument— a code the SDK's sync-retry loop (pkg/sync'sRetryer, wired toSyncResourcesOp/SyncGrantsOp) treats as fatal; it only waits and retries onUnavailable/DeadlineExceeded.doRequestCommonnow recognizes the errorCode independent of HTTP status (DocuSign is mid-migration from 400 to 429 for this condition and documents parsing errorCode, not status) and reclassifies it ascodes.Unavailablewith aRateLimitDescriptionattached via status details, so the SDK's existing retry loop picks it up and the sync pauses/resumes instead of failing outright.Reduced call amplification where feasible.
userBuilder.Grants()calledGetUserDetailsonce per user, every sync — real N+1 volume contributing to hitting the budget in the first place. The list response already carries the user's permission-profile name and status (captured on the resource's profile duringList()), soGrants()now resolves the profile ID viaGetPermissionProfiles(one account-wide call, already served from uhttp's default GET cache on repeat calls within a sync) for Active users, instead of a per-user call. Falls back to the originalGetUserDetailscall, unchanged, for every case that isn't confidently safe (non-active user, missing profile field, unresolvable/renamed profile name, or a failedGetPermissionProfilescall) — an earlier version of this cachedGetPermissionProfilesfor the builder's lifetime and skipped the active-user check entirely; dropped after a review found it could diverge from the old code for non-active users and duplicated caching uhttp's client already provides.Two other findings from the same ticket (the
GetUserByEmail-per-signing-group-member lookup insinging_groups.go, and thepermission_profiles.go/singing_groups.goGrant/Revoke call sites, which the ticket miscited as sync-time amplification but are actually low-frequency provisioning actions) were investigated but left alone — not blocking this ticket's core "non-recoverable sync" complaint, and lower-confidence to change safely without a live tenant.Useful links: