[CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplification - #68
Open
FeliLucero1 wants to merge 30 commits into
Open
[CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplification#68FeliLucero1 wants to merge 30 commits into
FeliLucero1 wants to merge 30 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
requested review from
a team,
Bencheng21,
JavierCarnelli-ConductorOne,
agustin-conductor,
al-conductorone,
btipling,
ggreer,
johnallers,
luisina-santos,
manuel-ts-14,
mateoHernandez123,
mateovespConductor and
sergiocorral-conductorone
and removed request for
mateoHernandez123
August 11, 2026 19:57
FeliLucero1
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Connector PR Review: [CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplificationBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Note: this run could not write the machine-readable |
…ifetime userBuilder is registered once via ResourceSyncers and reused for the connector process's lifetime, not reconstructed per sync — in service/ hosted mode that's many syncs sharing one builder instance. The cache only checked "have I fetched this at all", so the first sync's snapshot (or its cached failure) would silently serve every later sync forever. Threads SyncOpAttrs.SyncID through Grants() -> tryFastPathGrant -> getPermissionProfiles and resets the cache (and the transient-failure counter) whenever the SyncID changes.
Surfaced by testing the SyncID-keyed memoization fix: even after that fix correctly decided a new sync needed a fresh call, uhttp's GET cache was still serving the previous sync's cached HTTP response underneath it. userBuilder already calls this at most once per sync, so a cached response here can only ever be a stale snapshot from a prior sync on the same long-lived connector process — never a real saved call.
…ead Pebble Same root cause already fixed on PR #63/#64: GitHub Actions tests this PR's merge-preview against main, and main is already on baton-sdk v0.25.0 (Pebble-by-default) even though this branch's own go.mod is still on v0.24.2 — the merge cleanly picks up main's newer line since this PR never touches it. The downloaded baton CLI (v0.4.5, built against SDK v0.8.24) can't read a Pebble-format file.
…ion-forwarding comment GetPermissionProfiles unconditionally bypassing the shared HTTP GET cache saved userBuilder from cross-sync staleness, but List and Revoke share this same call and don't have that risk — sharing uhttp's cache between them when both fire in one sync used to cost 1 real request, unconditional WithNoCache() silently made it 2. Added GetPermissionProfilesFresh as the dedicated no-cache variant for userBuilder's memoization; GetPermissionProfiles (List/Revoke) stays cacheable. Also corrected tryFastPathGrant's doc comment: it still attributed "exactly one real call per sync" to uhttp's GET cache, which no longer applies to the fresh variant — getPermissionProfiles' own memoization is what guarantees that now.
…ntly test-permission-profiles failed: the final grant succeeded, but the sync run immediately after still read the pre-grant state, so the verification query found no grants. sync-test@v3 already has a sleep input built for exactly this (DocuSign write-propagation delay) — no job here was using it. Added to all three jobs; only test-permission-profiles has actually hit the race so far, but all three share the same account and write pattern.
Resolves 4 conflicted files, all from independent, non-overlapping additions on each side landing at the same location in the file: - .github/workflows/ci.yaml: both sides independently added a BATON_STORAGE_ENGINE: sqlite pin with slightly different wording. Kept main's wording (evergreen) plus main's new `concurrency` block and BATON_SYNC_RESOURCE_TYPES allowlist var, dropped this branch's now-stale comment (it referenced a go.mod version mismatch that no longer exists post-merge). This branch's own `sleep: "3"` additions on the three sync-test steps were untouched by the conflict and are preserved. - pkg/client/helper.go: this branch added the hourly-rate-limit reclassification helpers (reclassifyHourlyRateLimitError, isHourlyAPIInvocationLimitError) and a `time` import; main added IDFromHref. Kept both. - pkg/connector/helper.go: this branch added permissionProfileIDByName and profileFieldPermission/Status/PermissionID; main added profileFieldHref, clmHrefWithID, clmPreferredHref, and clmSampleHrefsFrom, and replaced clmIDFromHref's literal implementation with one that delegates to the new client.IDFromHref. Kept both sides' additions. main's removal of isOptInFeatureUnavailableError (superseded by the Validate()/ EnsureClmReady upfront CLM-readiness gate added in #63/#64) was applied automatically by git's 3-way merge, since this branch never touched that function — verified it has no remaining call sites. - pkg/connector/helper_test.go: test-side companion to the above; kept TestPermissionProfileIDByName (this branch) alongside TestClmPreferredHref/TestClmSampleHrefsFrom (main), and merged imports. Dropped the now-orphaned codes/status imports along with the isOptInFeatureUnavailableError test they supported. Verified with: go build ./..., go vet ./..., go test ./... -count=1 (all pass), golangci-lint run ./... (1 pre-existing goconst finding in pkg/client/clmtest/handlers.go, present on origin/main already, unrelated to this merge). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iew gaps Removes the unverified permission-profile-ID fast path from Grant resolution (no live tenant to confirm it matches GetUserDetails' effective profile — kept only the well-tested name-based path). Restores Revoke's pre-PR first-match behavior for ambiguous default-profile names, now logged instead of silent. Adds a 5s TTL-bounded short-circuit so concurrent Grants() workers stop re-hitting an already-exhausted rate limit. Makes the hourly rate-limit predicate case-insensitive and extends it to DocuSign's burst-limit variant. Fixes a dropped-error-chain bug in reclassifyRateLimitError via errors.Join. Also fixes issues a deep-code-review pass found in that work: a Warn-level log that violated this repo's no-Warn convention (now Debug, matching tryFastPathGrant's own fallback-log pattern); dead annotation-forwarding plumbing on the rate-limit-error path (Grants() already discards annotations whenever it returns an error, so the SDK never saw them either way); a write-only, never-read struct field; and duplicated hourly/burst matching logic factored into one shared helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sion-profile cache Two sergiocorral findings on PR68 that didn't make it into the prior fix batch. tryFastPathGrant's propagated rate-limit error was returned bare, unlike every other error return in this file — now wrapped with %w (verified this still preserves codes.Unavailable/RateLimitDescription through errors.As, same property reclassifyRateLimitError's errors.Join already relies on). getPermissionProfiles now checks ctx.Err() before taking permissionProfilesMu, since the lock is held across a real HTTP round-trip and an already-cancelled caller shouldn't queue behind it for nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Last of the sergiocorral findings on PR68. attrs.SyncID is only populated when baton-sdk's own version check passes; when it isn't, every Grants() call arrives with syncID == "", which equals permissionProfilesSyncID's zero value — so the cache's mismatch check would never fire again after the first write, silently reverting to the exact process-lifetime memoization bug the SyncID keying exists to prevent. Now an empty syncID disables the cache entirely (every call is real, matching pre-fast-path cost) and logs once so the condition is observable instead of a silent regression. Updated three existing tests that were relying on the empty-SyncID zero value as a stand-in for "one consistent sync" to use a real SyncID instead, so they still exercise the caching behavior they're named for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Empty commit to re-run stuck/cancelled GitHub Actions workflows on PR #68. Co-authored-by: Cursor <cursoragent@cursor.com>
Fix SA5011 in users_test, race-safe updateCalls recording, align ambiguous-profile docs with Debug logging, use first-match in permissionProfileIDByName, and fix stale reclassifyRateLimitError doc. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename UnambiguousDefaultNameNoWarning to NoAmbiguityLog so the test
name reflects FilterMessageSnippet("ambiguous") rather than Warn level.
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Operational note (from review): the SDK's retry loop clamps its wait to 60s regardless of the hour-long
ResetAtthis connector reports (pkg/retry'sMaxDelaynormalization). So an account that trips the hourly limit early in the hour retries roughly once a minute for the rest of that hour — ~60 calls against an already-exhausted budget, holding a sync slot open the whole time. Real improvement over a fatal failure, but not a single hour-long pause; the 60s cap lives in baton-sdk, not this connector.Known limitation: the burst-limit variant (
BURST_APIINVOCATION_LIMIT_EXCEEDED, DocuSign's other 30s rate limiter) is now also reclassified as retryable alongside the hourly one, but its exact errorCode/message string is a best-effort reconstruction — not confirmed against a live account or DocuSign's docs (flagged in code comments on the relevant constants). Worth verifying before this ships if anyone gets access to a real burst-limit response.Useful links: