perf(workflow): check embedded action pins before gh-api network call in ActionResolver#39707
Conversation
… in ActionResolver The ActionResolver.ResolveSHA method previously fell through to a gh-api subprocess call whenever the on-disk ActionCache had a miss. For builtin actions like actions/github-script@v9, this caused a ~1.2s subprocess invocation even though the exact SHA is already embedded in action_pins.json as v9.0.0 (semver-compatible with the requested v9 tag). This fix adds an embedded-pin check between the disk-cache lookup and the network call. For any action whose requested version is semver-compatible with an embedded pin, the resolver now returns the embedded SHA immediately without spawning a gh-api process. Impact: - TestCompileWorkflow_PerformanceRegression/small_workflow: 1.4s → 50ms (first compilation in a test run), then 2ms (subsequent runs with disk-cache hit) - TestCompileWorkflow_PerformanceRegression/medium_workflow: 1.2s → 2ms - Full pkg/workflow test suite: significantly faster for all tests that compile workflows with builtin actions
There was a problem hiding this comment.
Pull request overview
This PR improves pkg/workflow action SHA resolution performance by avoiding an expensive gh api subprocess call when the requested action version can be satisfied from the embedded action pin set.
Changes:
- Adds an embedded action-pin lookup on
ActionResolver.ResolveSHAcache misses, before invokinggh api. - Caches embedded-pin hits in the on-disk
ActionCacheto avoid repeated lookups/subprocess calls.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/action_resolver.go | Adds embedded pin short-circuit in ResolveSHA before falling back to gh api resolution. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The previous change called r.cache.Set() when returning an embedded-pin hit, marking the on-disk ActionCache as dirty. This caused actions-lock.json to be written into mounted volumes when compiling inside Docker containers (e.g. the Alpine CI test). Since Docker containers run as root, the file was owned by root:root, preventing the host runner's cleanup step from deleting it. The embedded pins are always available in memory (action_pins.json is embedded at compile time), so there is no value in persisting them to the on-disk cache. Remove the r.cache.Set() call from the embedded-pin fast path.
|
✅ smoke-ci: safeoutputs CLI comment + comment-memory run (27657200043)
|
|
Hey @dsyme 👋 — great catch on the resolver ordering issue! The before/after numbers in the impact table speak for themselves — collapsing a 1.4 s test run down to 50 ms for the first run and 2 ms cached is a meaningful win for the whole One thing that would strengthen this before merge:
If you'd like a hand writing those, here's a ready-to-use agent prompt:
|
|
Please add focused unit tests for the embedded-pin ResolveSHA path (exact match, loose semver, fallback).
|
|
\n@copilot add focused unit tests for exact-match, loose-semver, and fallback ResolveSHA paths.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot please refresh the branch and summarize the remaining blockers.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Adds an in-memory embedded-pins fast path to
ActionResolver.ResolveSHAso that builtin actions (e.g.actions/github-script@v9) never trigger agh-apisubprocess when their SHA is already compiled into the binary viaaction_pins.json.Previously, a disk-cache miss immediately fell through to a ~1.2 s
gh-apisubprocess. The resolver now checks the embedded pin table first, and only falls back to the network when no compatible embedded pin exists.Changes
pkg/workflow/action_resolver.goNew fast path between disk-cache and network (
94a4ff761,6aa755b08)After a disk-cache miss,
ResolveSHAnow:semverutil.EnsureVPrefix.v9.0.0), performs an exact string comparison against each embedded pin for that repo.semverutil.IsCompatibleto find the highest semver-compatible pin.gh-apipath.No on-disk cache write for embedded hits (
ae230f0d8,5548abd9c)The original fast-path implementation called
r.cache.Set()on an embedded hit. This was removed because:actions-lock.jsonas root prevented the host runner's cleanup step from deleting the file.The
// Note:comment explaining this decision was subsequently re-indented bygofmt(5548abd9c).Performance
All
pkg/workflowtests that compile workflows containing builtin actions benefit from the fast path.Affected File
pkg/workflow/action_resolver.goNew imports:
pkg/actionpins,pkg/semverutilCommit Log
94a4ff7616aa755b08ae230f0d85548abd9cNotes for Reviewers
requestedIsPrecise) uses plain string equality (pinVersion != requested) rather than a semver API call — intentional and safe because both sides are normalised byEnsureVPrefixbefore comparison.r.cache.Set()was deliberately removed from the fast path (not just moved). If a caller later requests the same action via the network path it will still be written to disk at that point.6aa755b08has the vague message "Potential fix for pull request finding" but its diff is purely a refinement of the semver-matching logic added in94a4ff761.