[CXH-2245] fix: classify DocuSign hourly-limit error as retryable, reduce per-user call amplification

.github/workflows/ci.yaml

@@ -69,6 +69,12 @@ jobs:
           baton-entitlement: "${{ vars.TEST_GROUP_ENTITLEMENT }}"
           baton-principal: "${{ vars.TEST_GROUP_PRINCIPAL }}"
           baton-principal-type: "${{ vars.TEST_GROUP_PRINCIPAL_TYPE }}"
+          # DocuSign's backend doesn't apply a write instantly — the sync immediately
+          # after a grant/revoke can still read the pre-write state. sync-test's sleep
+          # input exists exactly for this; confirmed needed by a real failure (a
+          # verification query returning no grants right after a grant that itself
+          # succeeded) on test-permission-profiles, which has no built-in retry.
+          sleep: "3"
   test-signing-groups:
     needs: [test-groups]
     runs-on: ubuntu-latest
@@ -98,6 +104,9 @@ jobs:
           baton-entitlement: "${{ vars.TEST_SIGNING_GROUP_ENTITLEMENT }}"
           baton-principal: "${{ vars.TEST_SIGNING_GROUP_PRINCIPAL }}"
           baton-principal-type: "${{ vars.TEST_SIGNING_GROUP_PRINCIPAL_TYPE }}"
+          # See test-groups' identical sleep above — same shared demo account, same
+          # DocuSign write-propagation delay risk.
+          sleep: "3"
   test-permission-profiles:
     needs: [test-signing-groups]
     runs-on: ubuntu-latest
@@ -127,3 +136,7 @@ jobs:
           baton-entitlement: "${{ vars.TEST_PERMISSION_PROFILE_ENTITLEMENT }}"
           baton-principal: "${{ vars.TEST_PERMISSION_PROFILE_PRINCIPAL }}"
           baton-principal-type: "${{ vars.TEST_PERMISSION_PROFILE_PRINCIPAL_TYPE }}"
+          # See test-groups' identical sleep above — same shared demo account, same
+          # DocuSign write-propagation delay risk (the one confirmed to actually fire
+          # here).
+          sleep: "3"

pkg/client/client.go

@@ -645,13 +645,51 @@ func (c *Client) GetUserByEmail(ctx context.Context, userEmail string) (*User, a
 	return &user, annos, nil
 }
 
-// GetPermissionProfiles fetches all permission profiles from the DocuSign account.
+// GetPermissionProfiles fetches all permission profiles from the DocuSign account. The
+// response may be served from the shared HTTP GET cache — fine for List/Revoke, neither
+// of which memoizes across syncs the way userBuilder does (see GetPermissionProfilesFresh).
+//
+// Cache-split skew (accepted tradeoff, not an oversight): this cache is uhttp's own
+// shared GET cache, which is cross-sync for a long-lived connector process (default TTL
+// 1h, overridable via BATON_HTTP_CACHE_TTL) — not just cross-caller within one sync. A
+// permission profile created or deleted between two syncs less than that TTL apart can
+// be visible to userBuilder's always-fresh GetPermissionProfilesFresh grants pass before
+// it is visible here, or vice versa, producing a bounded resource-vs-grant skew window
+// (up to the cache TTL) between what List/Revoke see and what the grants pass sees.
+// Pre-split, both readers shared this same cached view, so there was no skew between
+// them — this split introduced the asymmetry, in exchange for keeping List/Revoke on
+// uhttp's cheap shared cache path instead of paying for a fresh call on every read. That
+// tradeoff is intentional: bypassing the cache here too would undo the call-volume
+// reduction this split exists for.
 //
 // Pagination: This endpoint does NOT support pagination. It returns all permission profiles in a single request.
 // Typically, DocuSign accounts have a limited number of permission profiles (< 50), so this is acceptable.
 //
 // Returns: all permission profiles, annotations, error.
 func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) {
+	return c.getPermissionProfiles(ctx, false)
+}
+
+// GetPermissionProfilesFresh is identical to GetPermissionProfiles but bypasses the
+// shared HTTP GET cache. userBuilder calls this at most once per sync (see its
+// memoization fields' doc in pkg/connector/users.go), so by the time this request
+// fires, that caller has already decided a fresh call is needed — a cached response
+// here could only ever serve a stale snapshot left over from a previous sync on the
+// same long-lived connector process, never save a real call. List and Revoke don't
+// share userBuilder's process-lifetime memoization risk (neither memoizes this call
+// across syncs), and sharing uhttp's cache with them when both fire in the same sync
+// saves a real network call — so only this path opts out of the cache, not
+// GetPermissionProfiles itself.
+//
+// This does NOT mean List/Revoke are free of cross-sync staleness risk of their own:
+// see GetPermissionProfiles' doc for the cache-split skew this asymmetry introduces
+// between this method's always-fresh view and List/Revoke's cached one — an accepted,
+// bounded tradeoff, not a gap specific to this method.
+func (c *Client) GetPermissionProfilesFresh(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) {
+	return c.getPermissionProfiles(ctx, true)
+}
+
+func (c *Client) getPermissionProfiles(ctx context.Context, noCache bool) ([]PermissionProfile, annotations.Annotations, error) {
 	if err := c.ensureInitialized(ctx); err != nil {
 		return nil, nil, err
 	}
@@ -665,7 +703,11 @@ func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile
 
 	permissionProfilesURL = baseURL.ResolveReference(permissionProfilesURL)
 
-	_, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse)
+	var extraOpts []uhttp.RequestOption
+	if noCache {
+		extraOpts = append(extraOpts, uhttp.WithNoCache())
+	}
+	_, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse, extraOpts...)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -801,6 +843,7 @@ func (c *Client) doRequest(
 	url *url.URL,
 	body any,
 	response any,
+	extraOpts ...uhttp.RequestOption,
 ) (http.Header, annotations.Annotations, error) {
 	token, err := c.tokenSource.Token()
 	if err != nil {
@@ -816,6 +859,7 @@ func (c *Client) doRequest(
 	if body != nil {
 		requestOptions = append(requestOptions, uhttp.WithJSONBody(body))
 	}
+	requestOptions = append(requestOptions, extraOpts...)
 
 	request, err := c.wrapper.NewRequest(ctx, method, url, requestOptions...)
 	if err != nil {

pkg/client/client_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"strings"
+	"sync/atomic"
 	"testing"
 
 	"github.com/conductorone/baton-sdk/pkg/uhttp"
@@ -273,3 +274,77 @@ func TestMultiAccountResourceIsolation(t *testing.T) {
 		}
 	}
 }
+
+// newCountingPermissionProfilesClient wires a Client to a mock server that counts real
+// GET /permission_profiles hits, to distinguish a real network call from one served by
+// uhttp's shared GET cache. The counter is incremented inside the httptest server's
+// handler goroutine and read from the test's main goroutine, so it must be an atomic
+// (see pkg/connector/users_test.go's newCountingPermissionProfilesClient, which
+// established this pattern with atomic.Int32) — a plain int here would trip `go test -race`.
+func newCountingPermissionProfilesClient(t *testing.T) (*Client, *atomic.Int32) {
+	t.Helper()
+	var calls atomic.Int32
+
+	mockServer := httptest.NewServer(nil)
+	t.Cleanup(mockServer.Close)
+	mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		switch {
+		case r.URL.Path == "/oauth/userinfo":
+			_ = json.NewEncoder(w).Encode(UserInfoResponse{
+				Sub: "service-account-user-id",
+				Accounts: []AccountInfo{
+					{AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true},
+				},
+			})
+		case strings.HasSuffix(r.URL.Path, "/permission_profiles"):
+			calls.Add(1)
+			_ = json.NewEncoder(w).Encode(PermissionProfilesResponse{
+				PermissionProfiles: []PermissionProfile{{PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}},
+			})
+		default:
+			http.NotFound(w, r)
+		}
+	})
+
+	mockServerURL, _ := url.Parse(mockServer.URL)
+	transport := &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}
+	wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: transport})
+	tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"})
+	return NewClient(context.Background(), false, tokenSource, "", "", wrapper), &calls
+}
+
+// TestGetPermissionProfiles_CachingSplitByCaller is a regression test for a review
+// finding: GetPermissionProfiles used to unconditionally bypass uhttp's GET cache for
+// every caller, but List and Revoke (unlike userBuilder) don't memoize this call across
+// syncs — sharing the cache between them when both fire in the same sync saves a real
+// network call, and unconditional WithNoCache() silently turned that 1 call into 2.
+// GetPermissionProfiles must still be cacheable; only GetPermissionProfilesFresh (the
+// dedicated variant for userBuilder's cross-sync-safe memoization) bypasses the cache.
+func TestGetPermissionProfiles_CachingSplitByCaller(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("GetPermissionProfiles is cacheable: two calls, one real request", func(t *testing.T) {
+		c, calls := newCountingPermissionProfilesClient(t)
+		for i := 0; i < 2; i++ {
+			if _, _, err := c.GetPermissionProfiles(ctx); err != nil {
+				t.Fatalf("call %d: %v", i, err)
+			}
+		}
+		if got := calls.Load(); got != 1 {
+			t.Errorf("expected 1 real request across 2 GetPermissionProfiles calls (cache should serve the second), got %d", got)
+		}
+	})
+
+	t.Run("GetPermissionProfilesFresh always issues a real request", func(t *testing.T) {
+		c, calls := newCountingPermissionProfilesClient(t)
+		for i := 0; i < 2; i++ {
+			if _, _, err := c.GetPermissionProfilesFresh(ctx); err != nil {
+				t.Fatalf("call %d: %v", i, err)
+			}
+		}
+		if got := calls.Load(); got != 2 {
+			t.Errorf("expected 2 real requests across 2 GetPermissionProfilesFresh calls (no caching), got %d", got)
+		}
+	})
+}