Commit 61d66a1
Fix/graphql client token refresh (#8791)
* fix(github-graphql): prevent panic in graphql rate limit polling goroutine
Replace panic in GraphqlAsyncClient rate-limit polling goroutine with graceful error handling.
Previously, any error while fetching rate limit (e.g., transient network issues or 401 responses) would trigger a panic inside a background goroutine, crashing the entire DevLake process.
Now, errors are logged and the client retries in the next cycle while retaining the last known rate limit.
Design decisions:
- Avoid panic in background goroutines: rate-limit polling is non-critical and should not bring down the entire pipeline.
- Use last known rateRemaining on runtime failures instead of resetting or blocking, ensuring continued progress with eventual consistency.
- Retry via existing polling mechanism instead of immediate retry to prevent tight retry loops and unnecessary API pressure.
- Introduce a default fallback (5000) only for initial rate-limit fetch failures, since no prior state exists at startup.
- Separate handling of initial vs runtime failures:
- Initial failure → fallback to default (5000)
- Runtime failure → retain previous value
Fixes #8788 (bug 1)
* fix(github-graphql): reuse ApiClient transport for GraphQL to enable token refresh
Replace oauth2.StaticTokenSource-based HTTP client with the underlying http.Client from ApiAsyncClient.
Previously, the GraphQL client constructed its own HTTP client using StaticTokenSource, which froze the access token at task start time. This caused GitHub App installation tokens (which expire after ~1 hour) to become invalid during long-running pipelines, leading to persistent 401 errors.
Now, the GraphQL client reuses apiClient.GetClient(), which is already configured with RefreshRoundTripper and TokenProvider. This enables automatic token refresh on 401 responses, aligning GraphQL behavior with the REST client.
Design decisions:
- Reuse transport layer instead of duplicating authentication logic to ensure consistency across REST and GraphQL clients.
- Avoid StaticTokenSource, as it prevents token refresh and breaks long-running pipelines.
- Leverage existing RefreshRoundTripper for transparent token rotation without modifying GraphQL query logic.
- Keep protocol-specific logic (GraphQL vs REST) separate while sharing the underlying HTTP transport.
This ensures GraphQL pipelines using GitHub App authentication can run beyond token expiry without failure.
Fixes #8788 (bug 2)
* refactor(github): extract shared authenticated http client from api client
- moved token provider and refresh round tripper setup into a reusable helper
- introduced CreateAuthenticatedHttpClient to centralize auth + transport logic
- updated CreateApiClient to use shared http client instead of inline setup
Rationale:
- decouples authentication (transport layer) from REST-specific client logic
- enables reuse for GraphQL client without duplicating token refresh logic
- aligns architecture with separation of concerns (http transport vs api clients)
* feat(github-graphql): introduce graphql client with shared auth and integrate into task flow
- added CreateGraphqlClient to encapsulate graphql client construction
- reused CreateAuthenticatedHttpClient from github/tasks to inject token refresh via RoundTripper
- replaced manual graphql client setup in PrepareTaskData with new factory function
- preserved existing rate limit handling via getRateRemaining callback
- preserved query cost calculation using SetGetRateCost
Technical details:
- graphql client now uses http transport with TokenProvider and RefreshRoundTripper
- removes dependency on oauth2 client and avoids token expiration issues
- decouples graphql client from REST ApiClient by avoiding reuse of apiClient.GetClient()
- maintains compatibility with github.com and enterprise graphql endpoints
Note:
- shared auth logic remains in github/tasks and is imported with alias to avoid package name collision
- introduces cross-plugin dependency (github_graphql → github/tasks) as a pragmatic tradeoff to avoid duplication
* feat(github): support static token transport for GraphQL and REST clients
add StaticRoundTripper for PAT authentication and use it in the shared http client.
since the same client is used by both REST and GraphQL, auth handling must distinguish
between refreshable tokens and static tokens. avoid applying refresh/retry logic to PAT.
ensures correct behavior across clients and prevents unnecessary retries for static auth.
* feat(github-graphql): introduce hierarchical fallback for GraphQL rate limit
Implement a layered fallback mechanism for GraphQL rate limiting:
1. Dynamic rate limit from provider (getRateRemaining)
2. Per-client override (WithFallbackRateLimit)
3. Config override (GRAPHQL_RATE_LIMIT)
4. Default fallback (1000)
Also moved GitHub-specific fallback (5000) via WithFallbackRateLimit
to the Graphql client.
* feat(github-graphql): Add graphql rate limit to .env example
* fix(github): Fix leaked debug statement
* fix(github-graphql): reuse http.Client proxy, auth configurations
Reused `http.Client` inside the apiClient returned by `CreateApiClient` method, so keeping the proxy and auth configurations the same.That also keep the centralized management of logic.
* fix(helpers): fix the priority order of fallback rate limit
Priority order fixed for fallback rate limit, priority order is:
1.Env variable
2.Value set with `WithFallbackRateLimit`
3.default value in the code
This all works only when the `getRateRemaining` fails: hence the fallback
* fix(github): StaticRoundTripper now owns token splitting and rotation for AccessToken connections
Previously, connection.Token (comma-separated PATs) was injected as-is
into the Authorization header, sending "Bearer tok1,tok2,tok3" instead
of a single rotated token.
StaticRoundTripper now splits the raw token string on comma and rotates
through tokens round-robin using an atomic counter.
For REST: StaticRoundTripper operates at transport level and always
overwrites the Authorization header set by SetupAuthentication.
SetupAuthentication is retained because conn.tokens is still required
by GetTokensCount() for rate limit calculation — but its header write
is superseded by StaticRoundTripper on every request.
For GraphQL: SetupAuthentication is never called by the graphql client,
so StaticRoundTripper is the only auth mechanism on this path — without
this fix, GraphQL requests were sent with the full unsplit token string.
* refactor(github-graphql): Downgrade fetch failure logs from Warn to Info
* fix(helper): use inline func type for GraphqlClientOption to avoid mock cycle
Replace exported GraphqlClientOption type with inline func(*GraphqlAsyncClient)
in CreateAsyncGraphqlClient signature. The named type caused mockery to generate
a mock file (GraphqlClientOption.go) that created an import cycle in tests.
* style(github): fix linting
* fix(github): token rotation start from index
* fix(helper): prevent graphql deadlock when rate limit fetch keeps failing
---------
Co-authored-by: Klesh Wong <klesh@qq.com>1 parent 144c2b7 commit 61d66a1
7 files changed
Lines changed: 265 additions & 87 deletions
File tree
- backend
- helpers/pluginhelper/api
- plugins
- github_graphql
- impl
- tasks
- github
- tasks
- token
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
23 | 27 | | |
24 | 28 | | |
25 | 29 | | |
26 | 30 | | |
27 | | - | |
28 | | - | |
29 | 31 | | |
30 | 32 | | |
31 | 33 | | |
| |||
47 | 49 | | |
48 | 50 | | |
49 | 51 | | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
50 | 57 | | |
51 | 58 | | |
52 | 59 | | |
53 | 60 | | |
54 | 61 | | |
55 | 62 | | |
| 63 | + | |
56 | 64 | | |
57 | 65 | | |
| 66 | + | |
58 | 67 | | |
59 | 68 | | |
60 | 69 | | |
61 | 70 | | |
62 | 71 | | |
63 | 72 | | |
64 | | - | |
| 73 | + | |
65 | 74 | | |
66 | 75 | | |
67 | 76 | | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
68 | 88 | | |
69 | 89 | | |
70 | 90 | | |
71 | | - | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
72 | 95 | | |
73 | | - | |
| 96 | + | |
| 97 | + | |
74 | 98 | | |
75 | 99 | | |
76 | 100 | | |
| |||
115 | 139 | | |
116 | 140 | | |
117 | 141 | | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
118 | 146 | | |
119 | 147 | | |
120 | 148 | | |
| |||
126 | 154 | | |
127 | 155 | | |
128 | 156 | | |
129 | | - | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
130 | 166 | | |
131 | 167 | | |
132 | 168 | | |
| |||
218 | 254 | | |
219 | 255 | | |
220 | 256 | | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
30 | 29 | | |
31 | 30 | | |
32 | 31 | | |
| |||
35 | 34 | | |
36 | 35 | | |
37 | 36 | | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
72 | 41 | | |
73 | 42 | | |
74 | 43 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
| 23 | + | |
22 | 24 | | |
23 | 25 | | |
24 | 26 | | |
| |||
93 | 95 | | |
94 | 96 | | |
95 | 97 | | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
23 | | - | |
24 | | - | |
25 | 23 | | |
26 | | - | |
27 | 24 | | |
28 | 25 | | |
29 | 26 | | |
| |||
39 | 36 | | |
40 | 37 | | |
41 | 38 | | |
42 | | - | |
43 | 39 | | |
44 | 40 | | |
45 | 41 | | |
| |||
180 | 176 | | |
181 | 177 | | |
182 | 178 | | |
183 | | - | |
184 | | - | |
185 | | - | |
186 | | - | |
187 | | - | |
188 | | - | |
189 | | - | |
190 | | - | |
191 | | - | |
192 | | - | |
193 | | - | |
194 | | - | |
195 | | - | |
196 | | - | |
197 | | - | |
198 | | - | |
199 | | - | |
200 | | - | |
201 | | - | |
202 | | - | |
203 | | - | |
204 | | - | |
205 | | - | |
206 | | - | |
207 | | - | |
208 | | - | |
209 | | - | |
210 | | - | |
211 | | - | |
212 | | - | |
213 | | - | |
214 | | - | |
215 | | - | |
216 | | - | |
217 | | - | |
218 | | - | |
219 | | - | |
220 | | - | |
221 | | - | |
222 | | - | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
223 | 183 | | |
224 | 184 | | |
225 | 185 | | |
| |||
230 | 190 | | |
231 | 191 | | |
232 | 192 | | |
233 | | - | |
234 | | - | |
| 193 | + | |
235 | 194 | | |
236 | 195 | | |
237 | 196 | | |
| |||
0 commit comments