[DX-4553] Binary Caching for loopinstall#2276
Open
kalverra wants to merge 11 commits into
Open
Conversation
product-security-plaid-production
Bot
requested review from
DylanTinianov,
engnke and
harry-anderson
July 23, 2026 19:38
Contributor
|
👋 kalverra, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Contributor
📊 API Diff Results
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
📊 API Diff Results
|
Contributor
📊 API Diff Results
|
Contributor
📊 API Diff Results
|
Contributor
📊 API Diff Results
|
Contributor
📊 API Diff Results
|
Contributor
✅ API Diff Results -
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a binary caching layer to pkg/loop/cmd/loopinstall to avoid rebuilding plugins from source when a cached or prebuilt binary is available, alongside Go/tooling version bumps and GitHub Actions workflow updates.
Changes:
- Introduce a 3-tier install path for
loopinstall(disk cache → GitHub release asset → source build) with configurable cache directory. - Extend plugin configuration model and CLI to support caching (new
cache-dirflag, newbinaryURLfield). - Bump Go version references and refresh CI workflow action versions.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflows/sdk/v2/pb/go.mod | Bumps module Go version directive. |
| pkg/workflows/artifacts/utils.go | Updates documentation comment for toolchain pinning example. |
| pkg/values/go.mod | Bumps module Go version directive. |
| pkg/monitoring/go.mod | Bumps module Go version directive. |
| pkg/loop/cmd/loopinstall/models.go | Adds BinaryURL field to plugin config model. |
| pkg/loop/cmd/loopinstall/main.go | Adds --cache-dir flag and wires it into install flow. |
| pkg/loop/cmd/loopinstall/main_test.go | Updates tests for new install signature and minor cleanup. |
| pkg/loop/cmd/loopinstall/install.go | Routes installs through caching wrapper. |
| pkg/loop/cmd/loopinstall/cache.go | Implements caching + GitHub release download logic. |
| pkg/loop/cmd/loopinstall/cache_test.go | Adds unit tests for cache key/dir and tier behaviors. |
| pkg/chipingress/go.mod | Bumps module Go version directive. |
| observability-lib/go.mod | Bumps module Go version directive. |
| keystore/go.mod | Bumps module Go version directive. |
| .tool-versions | Updates Go tool version. |
| .github/workflows/validate-protos-version.yml | Updates checkout action version. |
| .github/workflows/sonar-scan.yml | Updates checkout action version. |
| .github/workflows/release.yml | Updates checkout action version. |
| .github/workflows/pkg.yml | Updates checkout/upload-artifact action versions. |
| .github/workflows/observability.yml | Updates checkout/setup-go action versions. |
| .github/workflows/keystore.yml | Updates checkout/upload-artifact action versions. |
| .github/workflows/golangci_lint.yml | Updates cron quoting and checkout action version. |
| .github/workflows/go-mod-cache.yml | Updates checkout action version. |
| .github/workflows/dependabump.yml | Updates cron quoting, permissions formatting, and checkout action version. |
| .github/workflows/cre-settings-schema-reminder.yml | Updates checkout action version. |
| .github/workflows/build_external.yml | Updates multiple action versions (checkout/setup-go/github-script/install-nix). |
| .github/workflows/benchmark.yml | Updates runner and normalizes YAML quoting + checkout version. |
| .github/actions/setup-go/action.yml | Updates setup-go and cache action versions. |
Comments suppressed due to low confidence (1)
pkg/loop/cmd/loopinstall/cache.go:188
- GOOS/GOARCH are derived only from the process environment/runtime, but the actual build in downloadAndInstallPlugin can override GOOS/GOARCH via defaults.EnvVars / CL_PLUGIN_ENVVARS / plugin.EnvVars. This can cause cache keys and release asset selection to target the wrong platform when GOOS/GOARCH are set in the plugin env var configuration.
goos := os.Getenv("GOOS")
if goos == "" {
goos = runtime.GOOS
}
goarch := os.Getenv("GOARCH")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jmank88
previously approved these changes
Jul 23, 2026
kalverra
enabled auto-merge
July 23, 2026 19:58
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
pkg/loop/cmd/loopinstall/cache.go:42
computeCacheKeydoes not incorporate the effective build-time overrides fromCL_PLUGIN_GOFLAGS/CL_PLUGIN_ENVVARS. SincedownloadAndInstallPluginhonors these env vars when building, a cache entry produced without overrides can be incorrectly reused when overrides are set (or vice-versa), leading to wrong binaries being installed from cache.
func computeCacheKey(pluginType string, plugin PluginDef, defaults DefaultsConfig, goos, goarch string) string {
h := sha256.New()
_, _ = fmt.Fprintf(h, "%s|%s|%s|%s|%s|%s|%s|%s|%s|%v|%v",
pluginType, plugin.ModuleURI, plugin.GitRef, plugin.InstallPath, plugin.Flags, defaults.GoFlags, plugin.BinaryURL, goos, goarch, plugin.EnvVars, defaults.EnvVars)
base := filepath.Base(filepath.Clean(plugin.ModuleURI))
return fmt.Sprintf("%s-%x", base, h.Sum(nil)[:12])
pkg/loop/cmd/loopinstall/cache.go:250
- The BinaryURL download path unconditionally attaches an Authorization header when
GIT_AUTH_TOKEN/GITHUB_TOKENis set. This can leak credentials to arbitrary third-party hosts ifbinaryURLis configured to a non-GitHub domain, and it also allows insecurehttp://downloads. Consider requiring HTTPS and only sending the token to GitHub-owned hosts.
// Tier 2: Try explicit binary URL override, then GitHub Release Asset fallback
if plugin.BinaryURL != "" {
log.Printf("%s - cache miss, attempting binaryURL download (%s)", pluginKey, plugin.BinaryURL)
client := &http.Client{Timeout: 15 * time.Second}
req, err := http.NewRequest("GET", plugin.BinaryURL, nil)
if err == nil {
if token := getAuthToken(); token != "" {
if strings.Contains(plugin.BinaryURL, "github.com") || strings.Contains(plugin.BinaryURL, "githubusercontent.com") || strings.Contains(plugin.BinaryURL, "amazonaws.com") {
req.Header.Set("Authorization", "Bearer "+token)
pkg/loop/cmd/loopinstall/cache.go:304
downloadAndInstallPluginWithCachecomputesoutputPathwith an explicit.exesuffix whenGOOS=windows, butdownloadAndInstallPlugin(Tier 3 source build) computes its output path without that suffix. Ifgo build -o <name>does not append.exe, Tier 3 will successfully build but the subsequent cache write will be skipped becauseoutputPathdoesn't exist (and future installs may use a different filename). Consider normalizing the built filename before caching.
log.Printf("%s - release asset download not available (%v), falling back to source build", pluginKey, relErr)
}
// Tier 3: Source Build Fallback
if err := downloadAndInstallPlugin(pluginType, pluginIdx, plugin, defaults); err != nil {
return err
}
// Write compiled output to cache dir for future runs
if _, err := os.Stat(outputPath); err == nil {
if err := os.MkdirAll(resolvedCacheDir, 0755); err == nil {
if err := copyFile(outputPath, cachedBinaryPath); err != nil {
log.Printf("%s - warning: failed to write compiled binary to cache: %v", pluginKey, err)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
pkg/loop/cmd/loopinstall/cache.go:42
- computeCacheKey does not include the Go toolchain/version, so upgrading Go (or changing the
gobinary on PATH) can lead to reusing an incompatible/stale cached plugin binary. Include a version identifier (at minimumruntime.Version()) in the hashed inputs so cache entries are invalidated across Go upgrades.
h := sha256.New()
_, _ = fmt.Fprintf(h, "%s|%s|%s|%s|%s|%s|%s|%s|%s|%v|%v",
pluginType, plugin.ModuleURI, plugin.GitRef, plugin.InstallPath, plugin.Flags, defaults.GoFlags, plugin.BinaryURL, goos, goarch, plugin.EnvVars, defaults.EnvVars)
base := filepath.Base(filepath.Clean(plugin.ModuleURI))
return fmt.Sprintf("%s-%x", base, h.Sum(nil)[:12])
pkg/loop/cmd/loopinstall/cache.go:299
- On Windows, Tier 3 (source build) can succeed but still fail to install/cache correctly: downloadAndInstallPlugin writes to
GOBIN/<name>(no.exe), while this wrapper expectsGOBIN/<name>.exeand then looks foroutputPathwhen caching. This leaves users without the expected.exeand prevents caching on Windows.
// Tier 3: Source Build Fallback
if err := downloadAndInstallPlugin(pluginType, pluginIdx, plugin, defaults); err != nil {
return err
}
pkg/loop/cmd/loopinstall/main.go:29
- The --cache-dir flag help text is slightly misleading: resolveCacheDir appends a "loopinstall-cache" subdirectory under os.UserCacheDir() (or uses a temp fallback), so the effective default is not exactly os.UserCacheDir(). Updating the flag description avoids confusion for users trying to locate/cache-bust binaries.
flag.StringVar(&cacheDir, "cache-dir", "", "Directory path for caching compiled plugin binaries (optional; defaults to $CL_LOOPINSTALL_CACHE_DIR, else os.UserCacheDir())")
.github/workflows/pkg.yml:60
- This workflow previously pinned actions/upload-artifact by commit SHA but now uses the moving
@v7tag. If you want to preserve supply-chain hardening, keep actions pinned to a specific commit SHA (and update the comment to the corresponding version).
- name: Upload Go test coverage
if: always()
uses: actions/upload-artifact@v7
with:
jmank88
previously approved these changes
Jul 23, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
pkg/loop/cmd/loopinstall/cache.go:43
- Cache keys currently ignore CL_PLUGIN_GOFLAGS and CL_PLUGIN_ENVVARS, even though the build path (downloadAndInstallPlugin) uses them to affect compilation. Changing these env vars between runs could incorrectly reuse a cached binary built with different flags/env, producing surprising behavior. Including the effective env override values in the cache key keeps cache correctness aligned with the build inputs.
// computeCacheKey calculates a deterministic cache key for a plugin.
func computeCacheKey(pluginType string, plugin PluginDef, defaults DefaultsConfig, goos, goarch string) string {
h := sha256.New()
_, _ = fmt.Fprintf(h, "%s|%s|%s|%s|%s|%s|%s|%s|%s|%v|%v",
pluginType, plugin.ModuleURI, plugin.GitRef, plugin.InstallPath, plugin.Flags, defaults.GoFlags, plugin.BinaryURL, goos, goarch, plugin.EnvVars, defaults.EnvVars)
base := filepath.Base(filepath.Clean(plugin.ModuleURI))
return fmt.Sprintf("%s-%x", base, h.Sum(nil)[:12])
}
pkg/loop/cmd/loopinstall/cache.go:252
- BinaryURL download adds an Authorization header based on substring matching against the full URL. This can leak GIT_AUTH_TOKEN/GITHUB_TOKEN to attacker-controlled hosts like https://github.com.evil.example/... because it still "contains" github.com. Prefer checking the parsed request host (req.URL.Hostname()) and using exact/suffix matches on the hostname before attaching credentials.
if token := getAuthToken(); token != "" {
if strings.Contains(plugin.BinaryURL, "github.com") || strings.Contains(plugin.BinaryURL, "githubusercontent.com") || strings.Contains(plugin.BinaryURL, "amazonaws.com") {
req.Header.Set("Authorization", "Bearer "+token)
}
}
jmank88
previously approved these changes
Jul 23, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (6)
pkg/workflows/wasm/host/module.go:712
ctx.Err()can becontext.Canceledas well ascontext.DeadlineExceeded. Mapping any context error duringlinkWasmtocontext.DeadlineExceededloses that information and can mislead callers. Consider returningctx.Err()when it is non-nil, and only treating the explicit interrupt trap as a deadline exceeded case.
if err != nil {
if ctx.Err() != nil || strings.Contains(err.Error(), "wasm trap: interrupt") {
return o, context.DeadlineExceeded
}
return o, fmt.Errorf("error linking wasm: %w", err)
pkg/workflows/wasm/host/module.go:752
- Similar to the
linkWasmcase above: if the parent context was canceled, returningcontext.DeadlineExceededhere is incorrect. Handlectx.Err()!=nilseparately and return the actual context error; reserveDeadlineExceededfor the epoch timeout / interrupt-trap cases.
// If an error has occurred and the deadline has been reached or exceeded, return a deadline exceeded error.
// Note - there is no other reliable signal on the error that can be used to infer it is due to epoch deadline
// being reached, so if an error is returned after the deadline it is assumed it is due to that and return
// context.DeadlineExceeded.
if err != nil && ((executionDuration >= maxTimeout-m.cfg.TickInterval) || ctx.Err() != nil || strings.Contains(err.Error(), "wasm trap: interrupt")) { // As start could be called just before epoch update 1 tick interval is deducted to account for this
m.cfg.Logger.Errorw("start function returned error after deadline reached, returning deadline exceeded error", "errFromStartFunction", err)
return o, context.DeadlineExceeded
pkg/loop/cmd/loopinstall/cache.go:41
- The cache key is computed from
defaults.GoFlags/plugin.Flagsanddefaults.EnvVars/plugin.EnvVars, but the actual build can be overridden byCL_PLUGIN_GOFLAGSandCL_PLUGIN_ENVVARS(seedetermineGoFlagsand env var merging indownloadAndInstallPlugin). That means the cache can return a binary built with different flags/env than the current run, leading to incorrect installs.
Suggestion: compute the cache key from the effective goflags and envvars after applying CL_PLUGIN_GOFLAGS/CL_PLUGIN_ENVVARS (and any other build-affecting inputs) so the cache is safe to use.
func computeCacheKey(pluginType string, plugin PluginDef, defaults DefaultsConfig, goos, goarch string) string {
h := sha256.New()
_, _ = fmt.Fprintf(h, "%s|%s|%s|%s|%s|%s|%s|%s|%s|%v|%v",
pluginType, plugin.ModuleURI, plugin.GitRef, plugin.InstallPath, plugin.Flags, defaults.GoFlags, plugin.BinaryURL, goos, goarch, plugin.EnvVars, defaults.EnvVars)
base := filepath.Base(filepath.Clean(plugin.ModuleURI))
pkg/loop/cmd/loopinstall/cache.go:257
- This uses
strings.Containson the full URL to decide whether to attach anAuthorizationheader. That can leak tokens to unintended hosts (e.g. a URL containing "github.com" in the query/fragment, or similar tricks). Also, sending GitHub tokens toamazonaws.comis risky and usually unnecessary.
Prefer a strict allowlist check (at minimum strings.HasPrefix for known HTTPS GitHub domains) before adding the header.
if token := getAuthToken(); token != "" {
if strings.Contains(plugin.BinaryURL, "github.com") || strings.Contains(plugin.BinaryURL, "githubusercontent.com") || strings.Contains(plugin.BinaryURL, "amazonaws.com") {
req.Header.Set("Authorization", "Bearer "+token)
}
.github/actions/setup-go/action.yml:56
- These workflow/action updates switch to floating major tags (e.g.
actions/setup-go@v7,actions/cache@v6). For stronger supply-chain guarantees, this repo often pins third-party actions to immutable SHAs (see e.g..github/workflows/dependabump.ymland.github/workflows/validate-protos-version.yml). Consider pinning these actions to a specific commit SHA as well.
- name: Set up Go
uses: actions/setup-go@v7
with:
go-version: ${{ steps.go-version.outputs.version }}
cache: false
pkg/loop/cmd/loopinstall/cache.go:297
- Same non-atomic cache write issue as the BinaryURL path:
os.WriteFile(cachedBinaryPath, ...)can leave partially-written cache entries under concurrent runs, which then get installed on the next Tier 1 hit. Consider the same temp-file + rename approach here as well.
log.Printf("%s - release asset downloaded successfully", pluginKey)
if err := os.MkdirAll(resolvedCacheDir, 0755); err != nil {
return fmt.Errorf("%s - failed to create cache dir: %w", pluginKey, err)
}
if err := os.WriteFile(cachedBinaryPath, assetData, 0755); err != nil {
return fmt.Errorf("%s - failed to write binary to cache: %w", pluginKey, err)
chainchad
requested changes
Jul 23, 2026
Contributor
There was a problem hiding this comment.
Is there enough value to add this over just improving how we cache LOOPPs in the /chainlink core Dockerfile?
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
pkg/workflows/wasm/host/module.go:711
ctx.Err() != nilcan represent eithercontext.DeadlineExceededorcontext.Canceled. Returningcontext.DeadlineExceededunconditionally here can misclassify cancellations as timeouts. Prefer returningctx.Err()when it is set, and only map the Wasmtime interrupt trap toDeadlineExceededwhen the context hasn’t already been canceled/expired.
if ctx.Err() != nil || strings.Contains(err.Error(), "wasm trap: interrupt") {
return o, context.DeadlineExceeded
}
pkg/workflows/wasm/host/module.go:750
- This condition treats any
ctx.Err() != nil(includingcontext.Canceled) as a deadline overrun and forcescontext.DeadlineExceeded. If the caller cancels the context, it’s more accurate to only translate when the error is actuallyDeadlineExceeded(and keep the interrupt-trap mapping as-is).
if err != nil && ((executionDuration >= maxTimeout-m.cfg.TickInterval) || ctx.Err() != nil || strings.Contains(err.Error(), "wasm trap: interrupt")) { // As start could be called just before epoch update 1 tick interval is deducted to account for this
pkg/loop/cmd/loopinstall/cache.go:68
- Allowlisting all
*.amazonaws.comas "trusted" for sending bearer tokens is overly broad and can leakGIT_AUTH_TOKEN/GITHUB_TOKENto arbitrary S3 buckets/hosts if a config pointsbinaryURLthere. GitHub’s asset domains are already covered by*.githubusercontent.com, so this can be removed or narrowed to the specific GitHub-controlled hosts you expect.
return host == "github.com" ||
strings.HasSuffix(host, ".github.com") ||
host == "githubusercontent.com" ||
strings.HasSuffix(host, ".githubusercontent.com") ||
strings.HasSuffix(host, ".amazonaws.com")
}
pkg/loop/cmd/loopinstall/cache.go:214
tryDownloadGitHubReleaseusesio.ReadAllon the asset response body, which can allocate unbounded memory for a large or unexpected response. Adding a max-size limit viaio.LimitReaderhelps prevent OOM and makes failures clearer.
if dlResp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("asset download HTTP %d", dlResp.StatusCode)
}
return io.ReadAll(dlResp.Body)
}
.github/workflows/pkg.yml:59
- This switches
actions/upload-artifactfrom a pinned commit SHA to a floating major tag. To reduce supply-chain risk and improve reproducibility, prefer pinning to the action’s commit SHA (as was done previously) and updating the comment to the corresponding release.
uses: actions/upload-artifact@v7
.github/workflows/keystore.yml:99
- This switches
actions/upload-artifactfrom a pinned commit SHA to a floating major tag. To reduce supply-chain risk and improve reproducibility, prefer pinning to the action’s commit SHA (as was done previously) and updating the comment to the corresponding release.
uses: actions/upload-artifact@v7
Comment on lines
+71
to
+93
| func tryDownloadBinaryURL(pluginKey, binaryURL string) ([]byte, error) { | ||
| client := &http.Client{Timeout: 15 * time.Second} | ||
| req, err := http.NewRequest("GET", binaryURL, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("binaryURL request creation failed: %w", err) | ||
| } | ||
|
|
||
| if token := getAuthToken(); token != "" && isTrustedAuthDomain(binaryURL) { | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("binaryURL request failed: %w", err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("binaryURL returned HTTP %d", resp.StatusCode) | ||
| } | ||
|
|
||
| return io.ReadAll(resp.Body) | ||
| } |
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.
Big Changes
cache-dirflag orCL_LOOPINSTALL_CACHE_DIRenv var. Defaults toos.UserCacheDir()Small Changes
1.26.2->1.26.5