Skip to content

Retry GetBuildInfo to absorb Artifactory index propagation lag - #3682

Open
agrasth wants to merge 2 commits into
masterfrom
fix/pnpm-test-buildinfo-race
Open

Retry GetBuildInfo to absorb Artifactory index propagation lag#3682
agrasth wants to merge 2 commits into
masterfrom
fix/pnpm-test-buildinfo-race

Conversation

@agrasth

@agrasth agrasth commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • pm-version-monitor's pnpm compatibility test runs showed a sharp, date-correlated pass-rate collapse (100% → ~25%) on days with heavy concurrent CI load — spread evenly across every pnpm line including the officially-supported 10.x baseline, so it wasn't a pnpm-version issue at all.
  • Every failure shared the same signature: Error: Should be true / Messages: Build info was not found, immediately after a successful jfrog rt bp publish.
  • Root cause: tests.GetBuildInfo queried Artifactory right after the publish call returned success, with no retry. BuildInfoService.GetBuildInfo's own doc comment confirms a 404 surfaces as (found=false, err=nil) — exactly the shape of "published, but the search index hasn't caught up yet," which gets more likely the more concurrent CI load hits the same Artifactory instance.
  • Add a short bounded retry (4 attempts, 500ms exponential backoff, ~7.5s worst case) inside GetBuildInfo itself, so all ~20 call sites across pnpm_test.go, npm_test.go, maven_test.go, etc. benefit without touching each one individually. Only the found=false, err=nil case retries — any real error still returns immediately, unchanged from today's behavior.

Test plan

  • go build ./... — succeeds
  • go vet ./utils/tests/... — clean
  • Traced the exact failing runs from pm-version-monitor's pnpm dashboard (August 20 mass-retest window) back to this call site and confirmed the signature matches on every one
  • Confirmed via BuildInfoService.GetBuildInfo's doc comment in jfrog-client-go that found=false, err=nil is the documented 404 signal, so retrying only that case is safe and won't mask real failures

pm-version-monitor's pnpm compatibility runs showed a sharp,
date-correlated pass-rate collapse (100% -> ~25%) on days with heavy
concurrent CI load, spread across every pnpm line including the
officially-supported 10.x baseline -- not correlated with pnpm version
at all. Root cause traced to every failure sharing the same signature:

    Error: Should be true
    Messages: Build info was not found

tests.GetBuildInfo queried Artifactory immediately after
artifactoryCli.Exec("bp", ...) reported success, with no retry.
BuildInfoService.GetBuildInfo's own doc comment says a 404 surfaces as
(found=false, err=nil) -- exactly the shape of a build that was
published but whose search index entry hasn't propagated yet, which
happens more often under concurrent load hitting the same instance.

Add a short bounded retry (4 attempts, 500ms exponential backoff,
~7.5s worst case) inside GetBuildInfo itself so every one of its ~20
call sites across pnpm_test.go, npm_test.go, maven_test.go, etc.
benefits without individual changes. Only the found=false/err=nil
case retries; any real error still returns immediately, unchanged
from before.
golangci-lint (staticcheck SA1019) started failing on this PR's CI run
because master recently bumped to Go 1.26, which deprecates
httputil.ReverseProxy.Director in favor of Rewrite (available since
Go 1.20). Unrelated to the npm fixture change in this PR, but it
blocks the CI gate regardless, so fixing it here.

Direct translation: Rewrite receives a *httputil.ProxyRequest whose
.Out field is a pre-cloned copy of the incoming request, so the
closure only needs to set the same three fields Director set
directly on the request (Host, URL.Host, URL.Scheme). Defining a
custom Rewrite func means none of the automatic default behavior
(X-Forwarded-For, etc.) kicks in, matching Director's original
no-defaults behavior exactly -- this is a mechanical migration with
no functional change.

Verified with the exact CI lint invocation (golangci-lint 2.13.1,
same flag set) against both the changed package and the full repo:
0 issues.
@agrasth
agrasth deployed to build-gate August 25, 2026 09:27 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

🚨 Frogbot scanned this pull request and found the below:

View full scan results in JFrog Platform

📗 Scan Summary

  • Frogbot scanned for vulnerabilities and found 1 issues
Scan Category Status Security Issues
Software Composition Analysis ✅ Done Not Found
Contextual Analysis ✅ Done -
Static Application Security Testing (SAST) ✅ Done
1 Issues Found 1 High
Secrets ✅ Done Not Found
Infrastructure as Code (IaC) ✅ Done Not Found

pr.Out.URL.Scheme = target.Scheme
}
tr := &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Static Application Security Testing (SAST) Vulnerability

Severity Finding
high
High
TLS settings are configured insecurely, exposing communications to risks
Full description

Vulnerability Details

Rule ID: go-insecure-tls

Overview

Insecure TLS Configuration is a type of vulnerability that occurs when an
application uses weak or outdated cryptographic protocols, ciphers, or
configurations for secure communication over the network.

Vulnerable example

package main

import (
    "crypto/tls"
)

func main() {}

func insecureMinMaxTlsVersion() {
    {
        config := &tls.Config{}
        config.MinVersion = 0
    }
    {
        config := &tls.Config{}
        config.MinVersion = tls.VersionSSL30
    }
    {
        config := &tls.Config{}
        config.MaxVersion = tls.VersionSSL30
    }
    {
        config := &tls.Config{}
    }
}

func insecureCipherSuites() {
    config := &tls.Config{
        CipherSuites: []uint16{
            tls.TLS_RSA_WITH_RC4_128_SHA,
        },
    }
    _ = config
}

In this example, the MinVersion field is set to tls.VersionSSL30, which
uses the outdated SSL 3.0 protocol, making the application vulnerable to
attacks such as POODLE.

Remediation

package main

import (
    "crypto/tls"
)

func main() {}

func insecureMinMaxTlsVersion() {
    {
        config := &tls.Config{}
-       config.MinVersion = 0
+       config.MinVersion = tls.VersionTLS12
    }
    {
        config := &tls.Config{}
-       config.MinVersion = tls.VersionSSL30
+       config.MinVersion = tls.VersionTLS12
    }
    {
        config := &tls.Config{}
-       config.MaxVersion = tls.VersionSSL30
    }
    {
-       config := &tls.Config{}
+       config := &tls.Config{MinVersion: tls.VersionTLS12}
    }
}

func insecureCipherSuites() {
    config := &tls.Config{
        CipherSuites: []uint16{
-           tls.TLS_RSA_WITH_RC4_128_SHA,
+           tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
        },
+       MinVersion: tls.VersionTLS12,
    }
    _ = config
}

By using safe TLS versions (e.g., tls.VersionTLS12) and secure cipher suites we can
mitigate the risk of insecure TLS configurations and improve the security of the
application.



Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant