retry dpop

[kans]

No description provided.

[github-actions]

🟡 Suggestion: The retry loop shares one 30s budget with each attempt's HTTP request, and pkg/lambda/grpc/config/config.go:149 sets lambdaHTTPClientTimeout = 30 * time.Second on the client passed via WithHTTPClient. So for the hung/timed-out token POST — the exact class tryToken now marks transient — the first attempt consumes the whole budget, sleepBeforeRetry returns false immediately, and zero retries happen. Retries only help for fast 5xx/429. Consider lowering the lambda per-request timeout (e.g. ~8-10s) or passing WithRetryConfig so a retry cycle actually fits.

[github-actions]

🟡 Suggestion: 429 is classified retryable but the backoff ignores the Retry-After response header, so a throttled client retries after ~0.5s/1s regardless of what the authorization server asked for — which can extend the throttle. Consider plumbing resp.Header.Get("Retry-After") from tryToken into the delay computation (clamped to the remaining Token() budget), or excluding 429 from the retryable set until that's honored.

[github-actions]

🟡 Suggestion: GetRequestMetadata still calls d.tokenSource.Token() without the RPC ctx, and Token() uses its own baseCtx (context.Background() in pkg/lambda/grpc/config/config.go). With retries now enabled by default, a per-RPC creds call can block through the full backoff cycle instead of failing fast, and because the SDK wraps this in oauth2.ReuseTokenSource (config.go:100) every concurrent RPC serializes behind that one retrying call. Worth confirming this latency change is acceptable for RPCs with short deadlines.

[github-actions]

General PR Review: retry dpop

Blocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 1e33787f0262.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness: it bumps conductorone/dpop 0.2.6→0.3.0 (dpop_grpc and dpop_oauth2 to 0.3.0) and vendors the corresponding source, adding transient-failure classification plus capped exponential backoff with jitter to dpop_oauth2.Token(), a unique jti on each client assertion, self-contained use_dpop_nonce carry-through, and a codes.Unavailable/codes.Unauthenticated mapping at the gRPC per-RPC credentials boundary. I verified the gRPC status mapping actually survives grpc-go (Unavailable is not in IsRestrictedControlPlaneCode, so it passes through getTrAuthData intact) and that it lines up with the SDK's existing retry policy in pkg/retry/retry.go:61; the go.mod/go.sum/vendor/modules.txt changes are consistent, and the new github.com/google/uuid import in dpop_oauth2 is already a direct, vendored dependency. No blocking issues found — the suggestions are about whether the retry actually fires on the SDK's own call path and about the latency it introduces.

Risk triage (per docs/BUG_CATCHING.md §2): Silence — partial: a wrongly-classified failure shows up as a retry storm or a slow RPC, not a wrong value. Durability — no: nothing persisted, no proto/wire/serialized-state change. Uncontrolled dimensions — yes: correctness of the classification depends on network faults, timeouts, and retry scheduling. Consumer distance — yes: every downstream connector on the lambda transport. Consequence — rung 1 (redeploy). Verdict: MEDIUM. The review-blind class here is error-path/fault-injection; the instrument that would give real coverage is a fake authorization server exercising the permutation table (503 / 429 / non-JSON 5xx body / use_dpop_nonce challenge / transport error / caller cancel / deadline expiry mid-backoff) plus assertions on attempt count and total elapsed time. This PR contains no such test — the changed code is all vendored, so the tests would live in conductorone/dpop; please confirm they exist there before merge.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/lambda/grpc/config/config.go:149 — lambdaHTTPClientTimeout (30s) equals Token()'s total 30s budget, so a hung token POST leaves zero budget for retries; the timeout class the change explicitly targets never actually retries on this path.
• vendor/github.com/conductorone/dpop/integrations/dpop_grpc/client_credential.go:60 — GetRequestMetadata ignores the RPC context and now blocks through the full backoff cycle; combined with oauth2.ReuseTokenSource (pkg/lambda/grpc/config/config.go:100) concurrent RPCs serialize behind one retrying Token() call.
• vendor/github.com/conductorone/dpop/integrations/dpop_oauth2/retry.go:113 — 429 is retried without honoring the Retry-After header.
• pkg/sdk/version.go:3 — this is a default-behavior change for every downstream connector (retries on by default, new Unavailable mapping, new jti claim) with no version signal or rollout note in the PR description; per the repo criteria a default-behavior change should not ride a silent patch bump.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/lambda/grpc/config/config.go`:
- Around line 149: `lambdaHTTPClientTimeout` is 30s, and `dpop_oauth2.Token()` wraps its
  entire retry loop in a 30s context deadline. Because the per-request HTTP timeout equals
  the total token-fetch budget, a single hung or slow token POST consumes the whole budget;
  `sleepBeforeRetry` then sees an already-expired context and returns false, so no retry is
  ever attempted. The new transient classification therefore only helps for fast 5xx/429
  responses, not for the timeout case it names. Fix by either lowering the per-request
  timeout for the token client to roughly 8-10s so three attempts plus backoff fit inside
  30s, or by passing `dpop_oauth.WithRetryConfig(...)` with attempt counts and delays sized
  to the actual per-request timeout. Note the token source uses its own HTTP client, so the
  timeout can be tuned independently of other lambda-config requests if needed.

In `vendor/github.com/conductorone/dpop/integrations/dpop_grpc/client_credential.go`:
- Around line 60: `GetRequestMetadata(ctx, ...)` calls `d.tokenSource.Token()` without
  passing `ctx`, and `Token()` derives its deadline from its own `baseCtx`, which is
  `context.Background()` in `pkg/lambda/grpc/config/config.go`. With retries now on by
  default, this per-RPC credentials call can block for the full backoff cycle rather than
  failing fast, and it will not observe the RPC's own deadline or cancellation. Because
  `pkg/lambda/grpc/config/config.go:100` wraps the token source in
  `oauth2.ReuseTokenSource`, which holds a mutex for the duration of the fetch, every
  concurrent RPC queues behind one retrying call during an authorization-server outage.
  Either confirm this latency profile is acceptable for short-deadline RPCs, or plumb the
  RPC context into the token fetch (upstream change in the dpop repo) so the retry loop
  honors the caller's deadline.

In `vendor/github.com/conductorone/dpop/integrations/dpop_oauth2/retry.go`:
- Around line 113: `isRetryableStatus` treats 429 as retryable, but the backoff in
  `retryDelay` ignores the `Retry-After` header the authorization server sends with a 429.
  A throttled client will retry after roughly 0.5s and 1s regardless of the requested
  wait, which can prolong the throttle. Fix upstream by returning the `Retry-After` value
  from `tryToken` alongside the error and using it as the delay for the next attempt
  (clamped to the remaining `Token()` budget), or drop 429 from the retryable set until
  the header is honored.

In `pkg/sdk/version.go`:
- Around line 3: the version stays at v0.26.0 while this change alters SDK-observable
  defaults for every downstream connector: token fetches now retry by default, transient
  token failures surface as `codes.Unavailable` instead of `codes.Unauthenticated`, and
  client assertions now carry a `jti` claim. Per the repo review criteria, default-behavior
  changes should carry a version signal (a 0.x minor bump) and a short migration or rollout
  note in the PR description rather than shipping silently.

[github-actions]

No blocking issues found.