You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by "Ke-vin-S (via GitHub)" <gi...@apache.org> on 2026/03/22 05:11:46 UTC
[PR] Fix/graphql client token refresh [incubator-devlake]
Ke-vin-S opened a new pull request, #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791
### Summary
This PR fixes two issues in the GitHub GraphQL client related to stability and authentication during long-running pipelines:
1. **Prevent process crash on rate-limit polling errors**
- Replaces `panic` in `GraphqlAsyncClient` background goroutine with graceful error handling.
- On failure, errors are logged and the system retries in the next polling cycle while retaining the last known rate limit.
2. **Enable automatic token refresh for GraphQL requests**
- Replaces `oauth2.StaticTokenSource`-based HTTP client with the underlying `http.Client` from `ApiAsyncClient`.
- This allows GraphQL requests to reuse the `RefreshRoundTripper`, enabling automatic token refresh for GitHub App installation tokens.
---
### Does this close any open issues?
Closes #8788
---
### Screenshots
N/A
---
### Other Information
#### Design decisions
- **Avoid panic in background goroutines**
Rate-limit polling is non-critical and should not crash the entire pipeline. Errors are now handled gracefully.
- **Separate handling of initial vs runtime failures**
- Initial rate-limit fetch failure → fallback to default (`5000`) to prevent deadlock.
- Runtime failure → retain last known `rateRemaining` to preserve correctness.
- **Use eventual consistency for rate limiting**
Rate-limit polling is best-effort; failures should not block or terminate execution.
- **Reuse transport layer for consistency**
GraphQL now uses `apiClient.GetClient()` to share authentication, proxy, and retry behavior with REST.
- **Avoid `StaticTokenSource`**
Static tokens break long-running pipelines due to expiration. Using `RefreshRoundTripper` ensures seamless token rotation.
---
This change ensures:
- No crashes due to transient GraphQL rate-limit polling errors
- Stable execution of long-running pipelines
- Proper support for GitHub App authentication in GraphQL workflows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4215953088
Hi, @danielemoraschi Would you like to take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3049220188
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -218,3 +246,29 @@ func (apiClient *GraphqlAsyncClient) Wait() {
func (apiClient *GraphqlAsyncClient) Release() {
apiClient.cancel()
}
+
+// WithFallbackRateLimit sets the initial/fallback rate limit used when
+// rate limit information cannot be fetched dynamically.
+// This value may be overridden later by getRateRemaining.
+func WithFallbackRateLimit(limit int) GraphqlClientOption {
+ return func(c *GraphqlAsyncClient) {
+ if limit > 0 {
+ c.rateRemaining = limit
+ }
+ }
+}
+
+// resolveRateLimit determines the rate limit for GraphQL requests using task configuration -> else default constant.
+func resolveRateLimit(taskCtx plugin.TaskContext, logger log.Logger) int {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r2975746476
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -47,6 +47,8 @@ type GraphqlAsyncClient struct {
getRateCost func(q interface{}) int
}
+const defaultRateLimit = 5000
Review Comment:
Currently it falls back to 5000 (GitHub’s standard limit) when rate info isn’t available. I agree this shouldn’t be hardcoded—I'll make it configurable so it can adapt to different scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #8791:
URL: https://github.com/apache/devlake/pull/8791#issuecomment-4719348680
> Apologies, I was not that available the last few weeks 🙏 Glad to see it landed!
Don't be. Your input was important, and you are not obligated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r2975746476
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -47,6 +47,8 @@ type GraphqlAsyncClient struct {
getRateCost func(q interface{}) int
}
+const defaultRateLimit = 5000
Review Comment:
Currently it falls back to 5000 (GitHub’s standard limit) when rate info isn’t available. I agree this shouldn’t be hardcoded—I'll make it configurable so it can adapt to different auth modes or environments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4203944666
Hey @danielemoraschi , addressed your feedback. Would appreciate a second look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4169623953
> @Ke-vin-S Great job! Would you like to take a look at @danielemoraschi’s comments?
Yes, currently working on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4531185694
@klesh, @danielemoraschi
Apologies for the delay on this PR. I've now implemented requested changes and would love to get anymore changes if any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4581737854
@Ke-vin-S Thank you for following up. Great job!
@danielemoraschi Would you like to take another look? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3049223521
##########
backend/plugins/github_graphql/impl/impl.go:
##########
@@ -230,8 +189,8 @@ func (p GithubGraphql) PrepareTaskData(taskCtx plugin.TaskContext, options map[s
return 0, nil, errors.Default.Wrap(dataErrors[0], `query rate limit fail`)
}
if query.RateLimit == nil {
- logger.Info(`github graphql rate limit are disabled, fallback to 5000req/hour`)
- return 5000, nil, nil
+ logger.Info(`github graphql rate limit unavailable, using fallback rate limit`)
+ return 0, nil, errors.Default.New("rate limit unavailable")
Review Comment:
Changed the Warn logs as Info logs. but kept the error here so the handling is centralized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4169588485
@Ke-vin-S Great job! Would you like to take a look at @danielemoraschi’s comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3022908608
##########
backend/plugins/github_graphql/tasks/graphql_client.go:
##########
@@ -0,0 +1,71 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "context"
+ "fmt"
+ "github.com/apache/incubator-devlake/core/log"
+ "net/url"
+ "time"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ githubTasks "github.com/apache/incubator-devlake/plugins/github/tasks"
+ "github.com/merico-ai/graphql"
+)
+
+func CreateGraphqlClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ getRateRemaining func(context.Context, *graphql.Client, log.Logger) (rateRemaining int, resetAt *time.Time, err errors.Error),
+) (*helper.GraphqlAsyncClient, errors.Error) {
+
+ // inject the shared auth layer
+ httpClient, err := githubTasks.CreateAuthenticatedHttpClient(taskCtx, connection, nil)
Review Comment:
Yes that actually happens in this code, I fixed it by passing the already configured 'http.Client' to this, it already has proxy, auth configurations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4203954999
> Hi — just wanted to share that we're actively hitting both issues this PR addresses in production, and this fix would be hugely impactful for us.
> ## Our setup
>
> * **DevLake v1.0.3-beta9** on GKE (Helm chart)
>
> * **GitHub App auth** across 4 connections (408 repos total)
>
> * Largest repo: ~20K PRs, ~1,200 currently OPEN
>
>
> ## What we're experiencing
> ### 1. The `updateRateRemaining` panic (the primary fix in this PR)
>
> Our lake pod is at **42 restarts** — almost entirely from this panic. The sequence is:
>
> 1. The `github_graphql` "Collect Pull Requests" subtask enters Phase 2 (refetching all OPEN PRs)
>
> 2. Phase 2 sends heavy compound GraphQL queries (100 PRs per batch) that trigger **502 Bad Gateway** from GitHub
>
> 3. The retry loop runs for 10+ retries × 120s each, easily exceeding 1 hour
>
> 4. Our GitHub App token expires (~1hr lifetime)
>
> 5. The background `updateRateRemaining` goroutine calls `/rate_limit` with the expired token → **401 Unauthorized**
>
> 6. `panic()` at `graphql_async_client.go:129` → pod crash
>
>
> ```
> panic: non-200 OK status code: 401 Unauthorized body: "Bad credentials"
>
> goroutine 3660 [running]:
> github.com/apache/incubator-devlake/helpers/pluginhelper/api.(*GraphqlAsyncClient).updateRateRemaining.func1()
> /app/helpers/pluginhelper/api/graphql_async_client.go:129 +0x166
> ```
>
> ### 2. GraphQL client token refresh (the second fix in this PR)
>
> Since the GraphQL client uses a `StaticTokenSource` that freezes the token at task start, any task running longer than ~1 hour with GitHub App auth is guaranteed to eventually fail. For us, the most important repo's `github_graphql` task takes 2-3 hours even when it succeeds, so we hit this on every single pipeline run.
>
> The REST client was already fixed for this via #8746, but without this PR the GraphQL client remains vulnerable.
> ## Impact on us
>
> * **Largest Repo has failed on 4 of the last 6 daily pipeline runs** due to this combination
>
> * Each failure wastes 15+ hours of pipeline time (retries + pod restarts + re-queuing)
>
> * The pod crashes affect **all** running tasks, not just the one that triggered the panic
>
>
> ## Summary
>
> This PR addresses exactly the two bugs compounding against us:
>
> 1. The panic → replaced with graceful error handling ✅
>
> 2. The static token → wired up to `RefreshRoundTripper` for automatic renewal ✅
>
>
> Would love to see this merged. Happy to help test if that would be useful — we have a production environment that reliably reproduces both issues on every nightly sync.
If you get a chance to test against your setup before it's merged, that'd be great — but no pressure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [devlake]
Posted by "danielemoraschi (via GitHub)" <gi...@apache.org>.
danielemoraschi commented on PR #8791:
URL: https://github.com/apache/devlake/pull/8791#issuecomment-4706015378
Apologies, I was not that available the last few weeks 🙏
Glad to see it landed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4169907538
Thanks for your contribution! Could you please take a look at the failed checks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S closed pull request #8791: Fix/graphql client token refresh
URL: https://github.com/apache/incubator-devlake/pull/8791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4203484107
Fixed commit message formatting - changed underscores to hyphens in scopes to pass the lint check. Force-pushed to update all commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "danielemoraschi (via GitHub)" <gi...@apache.org>.
danielemoraschi commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3006064300
##########
backend/plugins/github/tasks/http_client.go:
##########
@@ -0,0 +1,89 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "net/http"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ "github.com/apache/incubator-devlake/plugins/github/token"
+)
+
+func CreateAuthenticatedHttpClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ baseClient *http.Client,
+) (*http.Client, errors.Error) {
+
+ logger := taskCtx.GetLogger()
+ db := taskCtx.GetDal()
+ encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr)
+
+ if baseClient == nil {
+ baseClient = &http.Client{}
+ }
+
+ // Inject TokenProvider for OAuth refresh or GitHub App installation tokens.
+ var tp *token.TokenProvider
+ if connection.RefreshToken != "" {
+ tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 {
+ tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ }
+
+ baseTransport := baseClient.Transport
+ if baseTransport == nil {
+ baseTransport = http.DefaultTransport
+ }
+
+ if tp != nil {
+ baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp)
+ logger.Info(
+ "Installed token refresh round tripper for connection %d (authMethod=%s)",
+ connection.ID,
+ connection.AuthMethod,
+ )
+
+ } else if connection.Token != "" {
Review Comment:
`connection.Token` can contain multiple comma-separated tokens (e.g. tok1,tok2,tok3). The old code in `impl.go` split on comma and used only the first token (tokens[0]) via oauth2.StaticTokenSource. This passes `connection.Token` directly to `StaticRoundTripper`, which injects the entire unsplit string as `Authorization: Bearer tok1,tok2,tok3`. On the REST path, this also overwrites the correctly-rotated single token set by `SetupAuthentication` at the request level.
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -218,3 +246,29 @@ func (apiClient *GraphqlAsyncClient) Wait() {
func (apiClient *GraphqlAsyncClient) Release() {
apiClient.cancel()
}
+
+// WithFallbackRateLimit sets the initial/fallback rate limit used when
+// rate limit information cannot be fetched dynamically.
+// This value may be overridden later by getRateRemaining.
+func WithFallbackRateLimit(limit int) GraphqlClientOption {
+ return func(c *GraphqlAsyncClient) {
+ if limit > 0 {
+ c.rateRemaining = limit
+ }
+ }
+}
+
+// resolveRateLimit determines the rate limit for GraphQL requests using task configuration -> else default constant.
+func resolveRateLimit(taskCtx plugin.TaskContext, logger log.Logger) int {
Review Comment:
`resolveRateLimit` reads env config at line 69, then opts (including WithFallbackRateLimit(5000)) are applied at lines 82-84, unconditionally overwriting the env value. Wouldn't a user setting `GRAPHQL_RATE_LIMIT=2000` be silently overridden by the code-level "fallback" of 5000 from graphql_client.go:69?
##########
backend/plugins/github_graphql/tasks/graphql_client.go:
##########
@@ -0,0 +1,71 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "context"
+ "fmt"
+ "github.com/apache/incubator-devlake/core/log"
+ "net/url"
+ "time"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ githubTasks "github.com/apache/incubator-devlake/plugins/github/tasks"
+ "github.com/merico-ai/graphql"
+)
+
+func CreateGraphqlClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ getRateRemaining func(context.Context, *graphql.Client, log.Logger) (rateRemaining int, resetAt *time.Time, err errors.Error),
+) (*helper.GraphqlAsyncClient, errors.Error) {
+
+ // inject the shared auth layer
+ httpClient, err := githubTasks.CreateAuthenticatedHttpClient(taskCtx, connection, nil)
Review Comment:
Calling this with `nil` as the base client I believe will default to an empty `http.Client`. This ignores the proxy configuration (`connection.GetProxy()`) and abandons the HTTP/SOCKS5 Proxy logic associated with the connection. It might break enterprise proxy setups as the proxy configuration is never attached, while the previous implementation explicitly injected a custom proxy `http.Transport` in `backend/plugins/github_graphql/impl/impl.go` into the GraphQL HTTP client. This refactoring drops that setup for corporate proxy users.
##########
backend/plugins/github_graphql/impl/impl.go:
##########
@@ -230,8 +189,8 @@ func (p GithubGraphql) PrepareTaskData(taskCtx plugin.TaskContext, options map[s
return 0, nil, errors.Default.Wrap(dataErrors[0], `query rate limit fail`)
}
if query.RateLimit == nil {
- logger.Info(`github graphql rate limit are disabled, fallback to 5000req/hour`)
- return 5000, nil, nil
+ logger.Info(`github graphql rate limit unavailable, using fallback rate limit`)
+ return 0, nil, errors.Default.New("rate limit unavailable")
Review Comment:
The old code returned `5000, nil, nil` for GitHub Enterprise with rate limiting disabled. This now returns an error instead of the previous 5000. The error propagates so the functionality is preserved, but every task run against such servers now produces a warning for what was previously a silently handled normal case.
##########
backend/plugins/github/tasks/http_client.go:
##########
@@ -0,0 +1,89 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "net/http"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ "github.com/apache/incubator-devlake/plugins/github/token"
+)
+
+func CreateAuthenticatedHttpClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ baseClient *http.Client,
+) (*http.Client, errors.Error) {
+
+ logger := taskCtx.GetLogger()
+ db := taskCtx.GetDal()
+ encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr)
+
+ if baseClient == nil {
+ baseClient = &http.Client{}
+ }
+
+ // Inject TokenProvider for OAuth refresh or GitHub App installation tokens.
+ var tp *token.TokenProvider
+ if connection.RefreshToken != "" {
+ tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 {
+ tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ }
+
+ baseTransport := baseClient.Transport
+ if baseTransport == nil {
+ baseTransport = http.DefaultTransport
+ }
+
+ if tp != nil {
+ baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp)
+ logger.Info(
+ "Installed token refresh round tripper for connection %d (authMethod=%s)",
+ connection.ID,
+ connection.AuthMethod,
+ )
+
+ } else if connection.Token != "" {
+ baseClient.Transport = token.NewStaticRoundTripper(
+ baseTransport,
+ connection.Token,
+ )
+ logger.Info(
+ "Installed static token round tripper for connection %d",
+ connection.ID,
+ )
+ }
+
+ // Persist the freshly minted token so the DB has a correctly encrypted value.
+ // PrepareApiClient (called by NewApiClientFromConnection) mints the token
+ // in-memory but does not persist it; without this, the DB may contain a stale
+ // or corrupted token that breaks GET /connections.
+ if connection.AuthMethod == models.AppKey && connection.Token != "" {
+ if err := token.PersistEncryptedTokenColumns(db, connection, encryptionSecret, logger, false); err != nil {
+ logger.Warn(err, "Failed to persist initial token for connection %d", connection.ID)
+ } else {
+ logger.Info("Persisted initial token for connection %d", connection.ID)
+ }
+ }
+ println("http client HIT3")
Review Comment:
Leftover development debug print statement deployed in production code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "danielemoraschi (via GitHub)" <gi...@apache.org>.
danielemoraschi commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3063141767
##########
backend/plugins/github/token/round_tripper.go:
##########
@@ -93,3 +95,36 @@ func (rt *RefreshRoundTripper) roundTripWithRetry(req *http.Request, refreshAtte
return resp, nil
}
+
+// StaticRoundTripper is an HTTP transport that injects a fixed bearer token.
+// Unlike RefreshRoundTripper, it does NOT attempt refresh or retries.
+type StaticRoundTripper struct {
+ base http.RoundTripper
+ tokens []string
+ idx atomic.Uint64
+}
+
+func NewStaticRoundTripper(base http.RoundTripper, rawToken string) *StaticRoundTripper {
+ if base == nil {
+ base = http.DefaultTransport
+ }
+ parts := strings.Split(rawToken, ",")
+ tokens := make([]string, 0, len(parts))
+ for _, t := range parts {
+ if t = strings.TrimSpace(t); t != "" {
+ tokens = append(tokens, t)
+ }
+ }
+ if len(tokens) == 0 {
+ tokens = []string{rawToken}
+ }
+ return &StaticRoundTripper{base: base, tokens: tokens}
+}
+
+func (rt *StaticRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+ // always overrides headers put by SetupAuthentication, to make sure the token is always injected
+ tok := rt.tokens[rt.idx.Add(1)%uint64(len(rt.tokens))]
Review Comment:
Uint64 starts at 0, `Add(1)` returns 1, so the first request uses `tokens[1]`, not `tokens[0]`. For single-token lists this is harmless (1%1==0), but multi-PAT connections skip the first token on first request. The old SetupAuthentication started at index 0.
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -126,7 +154,9 @@ func (apiClient *GraphqlAsyncClient) updateRateRemaining(rateRemaining int, rese
case <-time.After(nextDuring):
newRateRemaining, newResetAt, err := apiClient.getRateRemaining(apiClient.ctx, apiClient.client, apiClient.logger)
if err != nil {
- panic(err)
+ apiClient.logger.Info("failed to update graphql rate limit, will retry next cycle: %v", err)
+ apiClient.updateRateRemaining(apiClient.rateRemaining, nil)
Review Comment:
Potential deadlock on GitHub Enterprise? The Signal() at L139 only fires when `rateRemaining > 0`, so all goroutines waiting in Query (L183: for rateRemaining <= 0 { Wait() }) block forever. Since Github returns this error every cycle rate limit never recovers.
Check if use `max(apiClient.rateRemaining, defaultRateLimitConst)` instead of `apiClient.rateRemaining` works, to ensure the fallback is always positive and Signal() fires.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh merged PR #8791:
URL: https://github.com/apache/devlake/pull/8791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4112464851
@klesh I’ve pushed an update for this—could you please take a look? Happy to make changes if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r2975345458
##########
backend/helpers/pluginhelper/api/graphql_async_client.go:
##########
@@ -47,6 +47,8 @@ type GraphqlAsyncClient struct {
getRateCost func(q interface{}) int
}
+const defaultRateLimit = 5000
Review Comment:
Where does this constant come from? Could you add a source link for reference, or should we make it configurable if it varies?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3049219410
##########
backend/plugins/github/tasks/http_client.go:
##########
@@ -0,0 +1,89 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "net/http"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ "github.com/apache/incubator-devlake/plugins/github/token"
+)
+
+func CreateAuthenticatedHttpClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ baseClient *http.Client,
+) (*http.Client, errors.Error) {
+
+ logger := taskCtx.GetLogger()
+ db := taskCtx.GetDal()
+ encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr)
+
+ if baseClient == nil {
+ baseClient = &http.Client{}
+ }
+
+ // Inject TokenProvider for OAuth refresh or GitHub App installation tokens.
+ var tp *token.TokenProvider
+ if connection.RefreshToken != "" {
+ tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 {
+ tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ }
+
+ baseTransport := baseClient.Transport
+ if baseTransport == nil {
+ baseTransport = http.DefaultTransport
+ }
+
+ if tp != nil {
+ baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp)
+ logger.Info(
+ "Installed token refresh round tripper for connection %d (authMethod=%s)",
+ connection.ID,
+ connection.AuthMethod,
+ )
+
+ } else if connection.Token != "" {
+ baseClient.Transport = token.NewStaticRoundTripper(
+ baseTransport,
+ connection.Token,
+ )
+ logger.Info(
+ "Installed static token round tripper for connection %d",
+ connection.ID,
+ )
+ }
+
+ // Persist the freshly minted token so the DB has a correctly encrypted value.
+ // PrepareApiClient (called by NewApiClientFromConnection) mints the token
+ // in-memory but does not persist it; without this, the DB may contain a stale
+ // or corrupted token that breaks GET /connections.
+ if connection.AuthMethod == models.AppKey && connection.Token != "" {
+ if err := token.PersistEncryptedTokenColumns(db, connection, encryptionSecret, logger, false); err != nil {
+ logger.Warn(err, "Failed to persist initial token for connection %d", connection.ID)
+ } else {
+ logger.Info("Persisted initial token for connection %d", connection.ID)
+ }
+ }
+ println("http client HIT3")
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on code in PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3049218884
##########
backend/plugins/github/tasks/http_client.go:
##########
@@ -0,0 +1,89 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tasks
+
+import (
+ "net/http"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/plugin"
+ "github.com/apache/incubator-devlake/plugins/github/models"
+ "github.com/apache/incubator-devlake/plugins/github/token"
+)
+
+func CreateAuthenticatedHttpClient(
+ taskCtx plugin.TaskContext,
+ connection *models.GithubConnection,
+ baseClient *http.Client,
+) (*http.Client, errors.Error) {
+
+ logger := taskCtx.GetLogger()
+ db := taskCtx.GetDal()
+ encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr)
+
+ if baseClient == nil {
+ baseClient = &http.Client{}
+ }
+
+ // Inject TokenProvider for OAuth refresh or GitHub App installation tokens.
+ var tp *token.TokenProvider
+ if connection.RefreshToken != "" {
+ tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 {
+ tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret)
+ }
+
+ baseTransport := baseClient.Transport
+ if baseTransport == nil {
+ baseTransport = http.DefaultTransport
+ }
+
+ if tp != nil {
+ baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp)
+ logger.Info(
+ "Installed token refresh round tripper for connection %d (authMethod=%s)",
+ connection.ID,
+ connection.AuthMethod,
+ )
+
+ } else if connection.Token != "" {
Review Comment:
Implemented that in static round tripper
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "zaiddialpad (via GitHub)" <gi...@apache.org>.
zaiddialpad commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4200909836
Hi — just wanted to share that we're actively hitting both issues this PR addresses in production, and this fix would be hugely impactful for us.
## Our setup
- **DevLake v1.0.3-beta9** on GKE (Helm chart)
- **GitHub App auth** across 4 connections (408 repos total)
- Largest repo: ~20K PRs, ~1,200 currently OPEN
## What we're experiencing
### 1. The `updateRateRemaining` panic (the primary fix in this PR)
Our lake pod is at **42 restarts** — almost entirely from this panic. The sequence is:
1. The `github_graphql` "Collect Pull Requests" subtask enters Phase 2 (refetching all OPEN PRs)
2. Phase 2 sends heavy compound GraphQL queries (100 PRs per batch) that trigger **502 Bad Gateway** from GitHub
3. The retry loop runs for 10+ retries × 120s each, easily exceeding 1 hour
4. Our GitHub App token expires (~1hr lifetime)
5. The background `updateRateRemaining` goroutine calls `/rate_limit` with the expired token → **401 Unauthorized**
6. `panic()` at `graphql_async_client.go:129` → pod crash
```
panic: non-200 OK status code: 401 Unauthorized body: "Bad credentials"
goroutine 3660 [running]:
github.com/apache/incubator-devlake/helpers/pluginhelper/api.(*GraphqlAsyncClient).updateRateRemaining.func1()
/app/helpers/pluginhelper/api/graphql_async_client.go:129 +0x166
```
### 2. GraphQL client token refresh (the second fix in this PR)
Since the GraphQL client uses a `StaticTokenSource` that freezes the token at task start, any task running longer than ~1 hour with GitHub App auth is guaranteed to eventually fail. For us, the most important repo's `github_graphql` task takes 2-3 hours even when it succeeds, so we hit this on every single pipeline run.
The REST client was already fixed for this via #8746, but without this PR the GraphQL client remains vulnerable.
## Impact on us
- **Largest Repo has failed on 4 of the last 6 daily pipeline runs** due to this combination
- Each failure wastes 15+ hours of pipeline time (retries + pod restarts + re-queuing)
- The pod crashes affect **all** running tasks, not just the one that triggered the panic
## Summary
This PR addresses exactly the two bugs compounding against us:
1. The panic → replaced with graceful error handling ✅
2. The static token → wired up to `RefreshRoundTripper` for automatic renewal ✅
Would love to see this merged. Happy to help test if that would be useful — we have a production environment that reliably reproduces both issues on every nightly sync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/graphql client token refresh [incubator-devlake]
Posted by "Ke-vin-S (via GitHub)" <gi...@apache.org>.
Ke-vin-S commented on PR #8791:
URL: https://github.com/apache/incubator-devlake/pull/8791#issuecomment-4582419309
Closed and reopened to reset the PR base SHA. The lint failure is a false positive caused by stale CI metadata comparing against an older main head, flagging commits already merged upstream
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org