Repository navigation
Midstream merge: rebase on top of konflux-ci/devlake - #48
Merged
Merged
Conversation
Signed-off-by: Francisco Herrera <fjglira@gmail.com>
Signed-off-by: Francisco Herrera <fjglira@gmail.com>
Migration 20260615 dropped the auto_increment id but AutoMigrateTables failed to create the composite PK (connection_id, repo_id, commit_sha). Without a PK, CreateOrUpdate always inserts, causing ~32x row bloat. This led to CollectCommitCoverage iterating all duplicate rows × flags, turning a ~45min sync into ~25h and breaking per-flag coverage collection for 12+ repos after the raw table dedup migration (20260622). Changes: - New migration: dedup _tool_codecov_commits + add composite PK - Collector: GROUP BY to deduplicate commits at query time (defensive) Assisted-by: Cursor
deterministic dedup, ONLY_FULL_GROUP_BY, retry safety Assisted-by: Cursor
- Add target column list to INSERT INTO dedup (Qodo konflux-ci#1) - Move _old/_dedup cleanup before PK check for crash recovery (Qodo konflux-ci#3) - Fix staticcheck ST1006: omit unused receiver name Assisted-by: Cursor
* 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 apache#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 apache#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>
Document the cherry-pick of upstream PR apache#8791 (commit 61d66a1) in docs/upstream-diffs.md per fork conventions. Assisted-by: Cursor
Signed-off-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Signed-off-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Midstream merge: rebase
RH-DevProd/devlakedownstream changes on top ofkonflux-ci/devlake(upstreammain@141eb90b7).Upstream updates incorporated
fix: graphql client token refresh (#162)/ upstream Fix/graphql client token refresh apache/devlake#8791): the GitHub token refresher is now applied both to the REST (githubplugin) and GraphQL (github_graphqlplugin) clients, with a shared round-tripper for auth-token rotation.fix: codecov: missing primary key on commits table (#161)): adds migration20261007000000_fix_commits_primary_keyplus collector updates and registers the migration script.Downstream changes preserved
All downstream build/Tekton/Konflux configurations are preserved on top of the upstream rebase:
Merge status
Confirmed clean merge/rebase without conflicts:
dprod-rebasecontains all ofkonflux-ci/devlake:main(merge-base check passes) plus the 16 downstream commits. Files diverging fromdprod/mainare limited to the upstream fixes above plus build configuration (15 files, +479/−92).Validation
git merge-base --is-ancestor upstream/main dprod-rebase→ passes (fully rebased on upstream)main