GeezerVison is showing github.com — perf(provisioner/terraform): install providers from the plugin cache during init by sreya · Pull Request #32070 · coder/coder
Conversation
sreya
commented
Nov 12, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Instead of skipping terraform init, a build that reuses the lock file captured at import first runs terraform init -plugin-dir -lockfile=readonly against the plugin cache, so with a warm cache init never contacts the registry. On my machine that takes init from about 0.29s to 0.12s with the coder and docker providers and from about 1.4s to 0.86s with AWS. Skipping init would save the rest, but this leaves modules, backends and everything else to Terraform, so it's a much smaller change.
If that init fails (a cold cache, say) the build runs init as before and the failed attempt's errors stay out of the build log. Provisioners that set TF_CLI_ARGS or TF_CLI_ARGS_init always run init as before, since a flag like -upgrade needs the registry. Terraform logs providers installed this way as unauthenticated, but it still checks each one against the lock file and won't install anything the lock file doesn't pin.
Stacked on #31761.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…during init
A build that reuses the lock file captured at import now runs
`terraform init -plugin-dir` against the plugin cache first. With the
providers cached, init no longer asks the registry which versions exist,
which takes it from about 0.29s to 0.12s with the coder and docker
providers and from about 1.4s to 0.86s with AWS on Terraform 1.17.1.
If that init fails, for example on a cold cache, the build runs init as
before and the failed attempt stays out of the build log. Provisioners
that set TF_CLI_ARGS or TF_CLI_ARGS_init always run init as before,
because a flag such as -upgrade needs the registry.
This is an alternative to skipping init (#29827).
coder-agents-review[bot] reacted with rocket emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Chat: Review posted | View chat
Requested: 2026-11-12 07:07 UTC by @sreya
Spend: $12.59 / $100.00
Review history
- R1 (2026-11-12): 13 reviewers, 1 Nit, 2 P2, 7 P3, 4 P4, COMMENT. Review
deep-review v0.13.0 | Round 1 | 8ebe865..5f2b9ff
Last posted: Round 1, 14 findings (2 P2, 7 P3, 4 P4, 1 Nit), COMMENT. Review
Finding inventory
Findings
| # |
Sev |
Status |
Location |
Summary |
Round |
Reviewer |
Posted |
| CRF-1 |
P3 |
Open |
provision.go:200 |
TF_CLI_ARGS / TF_CLI_ARGS_init gate in initFromPluginCache has no test |
R1 |
Netero |
Yes |
| CRF-2 |
Note |
Note (body) |
executor.go:288 |
init -plugin-dir leaves .terraform/plugin_path; manual run shows later init, plan, apply unaffected; no test covers plan/apply |
R1 |
Netero, Bisky |
No |
| CRF-3 |
P2 |
Open |
executor.go:288 |
-plugin-dir installs providers absent from the captured lock from the cache with no hash or signature check |
R1 |
Kurapika |
Yes |
| CRF-4 |
P2 |
Open |
provision.go:207 |
Failed -plugin-dir attempt leaves installed providers that the fallback init reuses |
R1 |
Kurapika |
Yes |
| CRF-5 |
P3 |
Open |
provision.go:204 |
Cancel during plugin-cache init loses init output and reports context canceled |
R1 |
Takumi |
Yes |
| CRF-6 |
P3 |
Open |
provision.go:203 |
Buffered plugin-cache init hides module download output; cold cache discards it |
R1 |
Hisoka P3, Mafuuu P3, Takumi P4 |
Yes |
| CRF-7 |
P3 |
Open |
provision.go:116 |
No supported way to stop builds using a provider version withdrawn from a mirror |
R1 |
Pen Botter |
Yes |
| CRF-8 |
P3 |
Open |
speed-up-templates.md:107 |
Delete-from-cache instruction gives no path; linked section is wrong for built-in provisioners |
R1 |
Leorio P3, Pen Botter P3 |
Yes |
| CRF-9 |
P3 |
Open |
provision.go:115 |
Three names for the provider source across comments, docs, and tests |
R1 |
Gon P3, Pen Botter Nit |
Yes |
| CRF-10 |
P3 |
Open |
executor.go:246 |
init doc says pluginDir run doesn't contact a registry; modules and backend still do |
R1 |
Gon P2, Mafuuu P4, Leorio P4 |
Yes |
| CRF-11 |
P4 |
Open |
executor.go:645 |
flush does not empty bufferedLogSink and the doc does not say so |
R1 |
Gon P2 |
Yes |
| CRF-12 |
P4 |
Open |
speed-up-templates.md:100 |
"Otherwise" follows the unauthenticated sentence instead of its condition |
R1 |
Leorio P4, Pen Botter Nit |
Yes |
| CRF-13 |
P4 |
Open |
provision.go:193 |
"a cold cache is no error" is ambiguous and omits where failed output goes |
R1 |
Leorio |
Yes |
| CRF-14 |
P4 |
Open |
executor.go:248 |
pluginDir string parameter separates -plugin-dir from its safety checks; s.cachePath vs e.cachePath |
R1 |
Knov P4, Ryosuke P4, Gon Nit |
Yes |
| CRF-15 |
Nit |
Open |
provision.go:204 |
Variable logs shadows field name in logs.logs |
R1 |
Gon |
Yes |
| CRF-16 |
Nit |
Note (body) |
PR title |
Title and commit scope provisioner/terraform does not contain the docs file |
R1 |
Leorio |
No |
| CRF-17 |
Note |
Note (body) |
executor.go:288 |
Failed attempt's init timings are overwritten by the fallback's spans |
R1 |
Takumi, Netero |
No |
| CRF-18 |
Note |
Note (body) |
speed-up-templates.md:106 |
"30 days" repeats staleTerraformPluginRetention with no link |
R1 |
Gon |
No |
| CRF-19 |
OOS |
Out of scope (body) |
speed-up-templates.md:137 |
Cache directory section names wrong directory for built-in provisioners |
R1 |
Leorio |
No |
| CRF-20 |
OOS |
Out of scope (body) |
safeenv.go:62 |
safeEnvironValue could use strings.CutPrefix |
R1 |
Ging-Go |
No |
Contested and acknowledged
Round log
Round 1
Netero first pass: 1 P3, 1 Note, no P0. Proceeding to full panel (round 1).
Round type: round 1, full panel. Multi-domain (Go, docs, provider verification), target 9-12. Specialists: ging-go, ryosuke (Go), kurapika (provider signature verification bypass via -plugin-dir), takumi (mutex, context cancellation), pen-botter (docs), killua (perf claim on init path). Core: pariston, mafuuu. Pool: bisky (tests), gon, leorio (floor), hisoka (error paths). Wildcard (random): knov. 12 + wildcard.
Cross-check:
- CRF-3/CRF-4 linked: -lockfile=readonly (CRF-3 fix) does not close the path without clearing .terraform/providers (CRF-4). Pariston's premise "captured lock pins every provider" contradicts CRF-3; Kurapika's trigger (re-downloaded oversized module adding a provider) is plausible from modules.go:184-187 and not run end to end. Keep P2: the PR removes a check that the normal init enforced and makes the docs claim false. Considered downgrade: poisoning the cache requires code execution as the provisioner user, which can already replace a Coder-installed terraform binary in the same CachePath (serve.go:109); kept P2 because the fix is one flag and the docs claim must hold.
- CRF-6: Takumi P4 vs Mafuuu/Hisoka P3; higher wins, Hisoka adds the double-timeout consequence.
- CRF-10: Gon P2, Mafuuu P4, Leorio P4. Keep argument for P2: a future caller relies on the comment for network behavior. Settled P3: the only caller is in this PR and behavior is unchanged; the defect is comment wording.
- CRF-11: Gon P2. Keep argument: the name invites a second flush that duplicates build log lines. Downgraded to P4: one caller, one call, unexported type.
- CRF-8: Pen Botter's built-in path ($CODER_CACHE_DIRECTORY/provisioner-/tf) verified against cli/server.go:1298,1741; Leorio's $CODER_CACHE_DIRECTORY/tf omits provisioner-. Line 107 from Leorio is the delete sentence.
- CRF-2: Bisky ran plan/apply after a -plugin-dir init; both exit 0. Converted to Note (body).
- Cleared, not posted: Killua cold-cache cost (~35ms, once per provider version per provisioner), Hisoka corrupt-cache behavior same as before, Knov and Pariston lock-file verification for locked providers, Takumi unlocked read of logs.logs has no race, Ryosuke fallback cost note.
- 2 P2, 7 P3, 4 P4, 2 Nit, 3 Note (body), 2 OOS. Reviewed against 8ebe865..5f2b9ff.
- CRF-16 has no file line (PR title and commit subject), so it is stated in the review body as a process observation.
About deep-review
CRF = Coder Review Finding (P0-P4, Nit, Note)
| Reviewer |
Focus |
| Bisky |
tests |
| Chopper |
ops/errors |
| Churn-guard |
change verification |
| Ging |
language modernization |
| Gon |
naming |
| Hisoka |
edge cases |
| Killua |
perf |
| Kite |
change integrity |
| Knov |
contracts |
| Knuckle |
SQL |
| Komugi |
flake/determinism |
| Kurapika |
security |
| Law |
decomposition |
| Leorio |
docs |
| Luffy |
product |
| Mafu-san |
process |
| Mafuuu |
contracts |
| Melody |
dispatch/pairing |
| Meruem |
structural |
| Nami |
frontend |
| Netero |
mechanical checks |
| Pariston |
premise testing |
| Pen-botter |
product gaps |
| Razor |
verification |
| Robin |
duplication |
| Ryosuke |
Go arch |
| Takumi |
concurrency |
| Zoro |
shape |
🤖 Managed by Coder Agents.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![coder-agents-review[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F3454270%3Fs%3D60%26v%3D4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR runs terraform init -plugin-dir against the plugin cache before the normal init on builds that reuse the captured lock file. 2 P2, 7 P3, 4 P4, 2 Nit.
The PR title and commit subject use the scope provisioner/terraform, which does not contain docs/workspaces/speed-up-templates.md. AGENTS.md requires the scope to contain every changed file, so drop the scope or broaden it.
Out of scope (needs a ticket or explicit acceptance by a human):
docs/workspaces/speed-up-templates.md:137: the Cache directory section says providers are cached in CODER_CACHE_DIRECTORY, but built-in provisioners use $CODER_CACHE_DIRECTORY/provisioner-<n>/tf, so operators who inspect or clear the cache by following it look in the wrong directory.provisioner/terraform/safeenv.go:62: safeEnvironValue pairs strings.HasPrefix with strings.TrimPrefix, which strings.CutPrefix does in one call.
Notes:
terraform init -plugin-dir writes .terraform/plugin_path, which stays in the workdir. A manual run with Terraform 1.17.1 showed the following normal init, plan, and apply are unaffected, but no test runs plan or apply after a -plugin-dir init.- The failed attempt's init timing spans are overwritten by the fallback init's spans with the same keys, as with the existing
initWithTemplateLockFile retry.
- "30 days" at
docs/workspaces/speed-up-templates.md:106 repeats staleTerraformPluginRetention (provisioner/terraform/provision.go:33), and nothing links the two.
🤖 This review was automatically generated with Coder Agents.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The plugin cache init now runs with -lockfile=readonly, so it fails
instead of installing a provider the lock file doesn't pin, which
Terraform could not verify.
Its output is streamed to the build log again. Only its errors are held
back, and they are dropped if a normal init follows. A canceled attempt
no longer starts a normal init, so the build keeps Terraform's output
and exit error.
Tests cover an unpinned provider, operator init flags and canceling
the attempt. The docs name the provider registry or mirror
consistently and say where a provisioner keeps cached providers.
…ugin cache
Terraform records -plugin-dir in .terraform/plugin_path, and plan and
apply then look for provisioner plugins only in that directory. A
template that uses a third-party provisioner plugin failed at plan on
every build. Init now removes the record whether the cache attempt
worked or failed.
Tests now also cover TF_CLI_ARGS, and the cancel case checks that the
held errors reach the build log. The docs say that providers linked
from a filesystem mirror are not removed after 30 days, and describe
the cache layout in the cache directory section.
…uginCache
initFromPluginCache is now an executor method that returns an error, so
Init falls back to a normal init with one error check.
The log sink that held the cache attempt's errors is gone. The attempt
logs straight to the build log, so a cold cache now shows Terraform's
errors before the normal init runs. A build canceled during the attempt
reports the context error, and the test case for the old error is
removed along with its helpers.
This branch has not been deployed
No deployments
Reviewers
Emyrk
Awaiting requested review from Emyrk
1 more reviewer
Reviewers whose approvals may not affect merge requirements
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|