[CXP-527] Mint fresh installation token at start of every gRPC call (experiment) - #155
[CXP-527] Mint fresh installation token at start of every gRPC call (experiment)#155johnallers wants to merge 1 commit into
Conversation
Wrap each ResourceSyncerV2 returned from ResourceSyncers() with a thin shim that flags the next Token() call to refresh, so each entry into a gRPC method on the connector picks up a freshly-minted installation token instead of reusing the cached one. baton-sdk does not expose gRPC interceptors, so the refresh hook is attached at the syncer level. Wrapper variants cover the combinations of optional ResourceSyncer interfaces present today (Provisioner, Deleter, AccountManager, StaticEntitlements) so capability detection via type assertion still works. Experiment to test whether bypassing oauth2.ReuseTokenSource entirely eliminates the mid-sync 401s seen at ~1-5% rate across ~40 GitHub App connectors. Branched off main, not stacked on PRs 151 or 152.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return &rsStaticEnt{rsBase: base, se: se} | ||
| case isDel && isAcct: | ||
| return &rsDeleterAccountMgr{ | ||
| rsDeleter: rsDeleter{rsBase: base, del: del}, | ||
| acct: acct, | ||
| } | ||
| case isDel: | ||
| return &rsDeleter{rsBase: base, del: del} | ||
| default: | ||
| return &base | ||
| } | ||
| } | ||
|
|
||
| func getOrgs(ctx context.Context, client *github.Client, orgs []string) ([]string, error) { | ||
| if len(orgs) != 0 { | ||
| return orgs, nil |
There was a problem hiding this comment.
🟡 Suggestion: The switch silently drops interfaces for combinations not explicitly listed. For example, a syncer implementing both Deleter and StaticEntitlementSyncer would lose the StaticEntitlements capability. This is fine for the current experiment since all existing builder combos are covered, but consider adding a debug log in the default case (or a comment in wrapSyncerForRefresh) that lists the expected set of builders hitting each branch, so a future reader can tell at a glance whether a new builder needs a new variant.
Connector PR Review: [CXP-527] Mint fresh installation token at start of every gRPC call (experiment)Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThis PR adds a Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
|
Tested this change. Does not address issue. |
1 participant
Summary
Experiment branch testing btipling's suggestion from #151: mint a fresh GitHub App installation token at the start of every gRPC method on the connector, instead of relying on
oauth2.ReuseTokenSource. Branched offmain, not stacked on #151 or #152, so it can be tested in isolation.Hypothesis
PR #152 (cap cached token lifetime at
stated_expiry - 10m) deployed asv0.3.2-test.1and did not stop the 401s — three more in the 3 hours after deploy on fanduel. That weakens the clock-skew / cache-aging story and suggests either (a) the existing oauth2 refresh path is broken in a way we can't see, or (b) tokens are being invalidated externally. This PR bypassesoauth2.ReuseTokenSourceentirely so we can tell which.If 401s drop with this change, the existing refresh path has a bug we should fix. If they don't, the cause is external and PR #151's reactive retry is the right answer.
What changed
pkg/connector/connector.go:forceRefreshTokenSource— caches a token likeoauth2.ReuseTokenSourcebut exposesForceRefreshNext()to make the nextToken()call refresh regardless of expiry.newWithGithubAppuses it for the installation token; stored on theGitHubstruct asinstallationTokenSource.ResourceSyncers()wraps each syncer withwrapSyncerForRefresh, which forces a refresh on every method entry.PAT mode is unaffected —
installationTokenSourceis nil andResourceSyncers()skips wrapping.A DEBUG log line fires when a fresh token is actually minted:
Cost
~200ms per gRPC call. For a multi-day fanduel sync that's roughly 16–30 min of cumulative latency. Acceptable for an experiment. JWT-authenticated endpoints on GitHub Apps are limited to 30/min per app — well above per-Lambda-instance rate, but worth watching during the test.
Test plan
go test -race ./pkg/connector/make lintIf 401s persist after this, the cause is external and we should land #151. If they stop, we have a bug to chase in the existing refresh path.