Cache Plugins on Runner between Runs - #841
Draft
RobbieMcKinstry wants to merge 3 commits into
Draft
Conversation
This commit adds an `install` command option which halts execution after the Pulumi CLI is installed. This attempts to replicate the @pulumi/setup-pulumi action. To test this, most logic in main() was moved to a separate file which can be imported in a test context, unlike main.ts, which executes main() during resolution.
edobry
added a commit
to edobry/minsky
that referenced
this pull request
Sep 4, 2026
…ypecheck-infra ## Summary `typecheck-infra` was failing persistently and **gating nothing** — PRs #3603 and #3610 merged over the red check, so `infra/` silently stopped being typechecked. A red X on a job named `typecheck-infra` reads as a type error; it actually means the job died in `pulumi install` before `tsgo` ever ran, so it means NO coverage. **The mechanism, measured rather than inherited.** A cold `pulumi install` takes **~109s** (run `33854882366`, this PR's own cache-miss run: `08:45:10.8 → 08:46:59.6`, *"added 228 packages ... in 2m"*). The bound was **120s**. That is **1.1x headroom**, so roughly half of cold runs cross it. The step's comment claimed the bound was *"~12x the measured healthy duration (10s, run 32287929281 2026-08-19)"* — accurate when written, 16 days stale now. **Nothing broke; the install grew until ordinary variance started crossing a bound sized for a much smaller operation.** That settles what the spec's `## SETTLED` section explicitly left open ("this settles PERSISTENT vs TRANSIENT; it does not settle WHICH persistent cause"), and explains the green-then-a-run-of-failures pattern without needing an outage. **A green run is not evidence the condition cleared.** On run `33851833179` the job PASSED while its first attempt was killed at exactly 120.0s and the retry rescued it. The failure happens *inside* successful jobs. ## Key changes **Decomposed the operation before sizing anything.** `pulumi install` is two operations and only one is slow: plugin download+unpack **~2.4s** (`08:45:08.4 → 08:45:10.8`), then `npm install` of the generated SDK's deps **~109s**. A fix aimed at the whole step, or at the plugin half, treats the wrong 2 seconds. - **Cache `~/.pulumi/plugins`** — vendor-canonical ([Pulumi's GitHub Actions guide](https://www.pulumi.com/docs/iac/guides/continuous-delivery/github-actions/), [pulumi/actions#804](pulumi/actions#841)). **This workflow cached nothing on the Pulumi path at all.** Keyed on `Pulumi.yaml`. Commented so it is not mistaken for the fix. - **Cache `infra/node_modules` + `infra/sdks`** — the ~109s half. Both together because `package.json` declares `"@pulumi/railway": "file:sdks/railway"`, so restoring one without the other leaves a dangling `file:` link. Keyed on the lockfile AND `Pulumi.yaml`. - **Bound re-derived: 120s → 240s** (~2.2x the cold figure). Backstop redone: 3×240 + 2×10 = 740s = 12.3 min, so `timeout-minutes: 13`. - **A hang and an install error are now distinguishable** (criterion 2): `timeout` exits **124** specifically when it killed the command, so the two separate at no cost and call for opposite responses. Previously both collapsed into one `exit 1`. **Criterion 1 — should it be a required check? Yes, and the mechanism is decided on evidence.** Branch protection would be the more general home (`evaluateRequiredChecksStatus` already reads it). It is **not** used because that route is unavailable from agent context — probed: `forge_branch_protection_get main` returns `Resource not accessible by integration` for the App token. So this follows the `bundle-boot-smoke` precedent: a named check in `require-review-before-merge.ts` with an override (`MINSKY_SKIP_TYPECHECK_INFRA`). **The gate is deliberately NARROWER than `bundle-boot-smoke`'s, and tests pin the asymmetry.** It denies on exactly one class — an explicit non-success conclusion. Absent, pending and an unparseable API response all PASS, where the bundle gate denies on all four. What was measured is PRs merging over a RED check; denying on ABSENT would make every webhook miss a merge-stopper, a different failure this task did not measure. Three tests assert the divergence so a later "make it consistent with the bundle gate" refactor cannot silently widen the blast radius. Two incidental corrections: `parseBundleBootSmokeResponse` is parameterized by check name (default unchanged, asserted by a test) so the second gate reuses its match/parse/sort rather than copying ~60 lines; and the `MINSKY_SKIP_BUNDLE_SMOKE` comment pointed at `src/domain/...`, a path gone since the mt#2108 package extraction. ## Testing Execution evidence: ``` $ bun run test:hooks 7042 pass 0 fail 14700 expect() calls (191 files) $ bun scripts/run-related-tests.ts .minsky/hooks/require-review-before-merge.ts \ packages/domain/src/configuration/sources/environment.ts .github/workflows/ci.yml 754 pass 0 fail (42 related test files) $ bun test --preload ./tests/setup.ts ./.minsky/hooks/require-review-before-merge.typecheck-infra.test.ts (pass) evaluateTypecheckInfraConclusion (mt#4950) > denies when the check concluded failure — the mt#4950 condition (pass) evaluateTypecheckInfraConclusion (mt#4950) > denies on other non-success conclusions too (timed_out, cancelled) (pass) evaluateTypecheckInfraConclusion (mt#4950) > passes on success (pass) evaluateTypecheckInfraConclusion (mt#4950) > passes on skipped and neutral — GitHub uses these for jobs that did not run (pass) evaluateTypecheckInfraConclusion (mt#4950) > passes when no typecheck-infra run exists (absent is NOT a denial here) (pass) evaluateTypecheckInfraConclusion (mt#4950) > passes while the check is still in progress (pass) evaluateTypecheckInfraConclusion (mt#4950) > passes when the check-runs response could not be parsed (pass) evaluateTypecheckInfraConclusion (mt#4950) > parseBundleBootSmokeResponse filters to typecheck-infra when given the name (pass) evaluateTypecheckInfraConclusion (mt#4950) > the default check name is unchanged — pre-existing callers still get bundle-boot-smoke ``` **AT1 / SC3 — cold-install measurement, bound derived from it.** Run `33854882366` job `100965760201`, this PR's own run, with BOTH new caches missing and being written: ``` 08:45:08.4 Downloading provider terraform-provider 08:45:10.8 Installing dependencies... 08:46:59.4 added 228 packages, and audited 230 packages in 2m 08:46:59.6 Finished installing dependencies -> 108.8s, FIRST attempt, no retry ... Cache saved with key: infra-deps-Linux-db3e1f50... Cache saved with key: pulumi-plugins-Linux-b3f2aba0... ``` **AT2 / SC4 — a run that pays the cold path, under the new bound.** The same run: `typecheck-infra` concluded **success**, first attempt, on a run where both caches missed. Under the OLD 120s bound this 108.8s install had 11 seconds of margin; under 240s it has 131. **A correction to my own earlier basis, recorded rather than quietly fixed.** The first version of this PR sized 240s as "~2.75x the measured 87s", from run `33851833179` attempt 2. That 87s is *not* a cold install: it ran after attempt 1 was killed at 120s having already installed 225 of the 228 packages, and its own log line says so (*"added 3 packages"*). A correct measurement of the wrong population — the same shape as the stale "12x / 10s" comment this task exists to replace. The bound is unchanged and still sound; its stated basis is now the 109s cold figure. **AT3 / SC2 — negative control on the failure reporting**, below. **AT4 / SC1 — the criterion-1 decision is recorded** in `hook-files.mdc`, the `evaluateTypecheckInfraConclusion` docblock, and the workflow comments. Negative control — the merge gate: reverted `evaluateTypecheckInfraConclusion`'s deny to its pre-fix behaviour (unconditional `{ deny: false }`, behaviourally identical to the function not existing). Both deny tests failed `Expected: true / Received: false`; the seven pass-cases correctly stayed green, since they assert behaviour the fix does not change. ``` (fail) evaluateTypecheckInfraConclusion (mt#4950) > denies when the check concluded failure (fail) evaluateTypecheckInfraConclusion (mt#4950) > denies on other non-success conclusions too 137 pass, 2 fail ``` Negative control — AT3, the install-failure reporting: the new retry block has two production branches and a healthy CI run only exercises one, so both were run under `bash -e` with `timeout 240 pulumi install` stubbed to a controlled exit code (mt#2776 §7a dual-mode rule): ``` --- stub rc=124 (hang) --- pulumi install HUNG - killed by the 240s per-attempt bound (attempt 1/3, mt#4950) [x3] ::error::pulumi install hung on all 3 attempts (240s each). step exit=124 --- stub rc=1 (real install error) --- pulumi install FAILED with exit 1 - this is an install error, not a hang (attempt 1/3, mt#4950) [x3] ::error::pulumi install failed with exit 1 on all 3 attempts. This is NOT a timeout; read the install output above for the real cause. step exit=1 --- stub rc=0 --- (loop completed, step passes) step exit=0 ``` The old code produced `exit 1` with no distinguishing text for both of the first two. Typecheck clean across 8 projects (`infra/` skipped — no local install; CI covers it, and this PR is about making that CI coverage actually run). Lint clean: 0 errors, 0 warnings over 4375 files. One process note, because the run summary hid it: the first version of these tests used `test(` in a file importing only `it`, so the whole block threw `ReferenceError` between tests while the summary still read **"130 pass, 0 fail"** — my tests contributed zero of them. Caught by reading the `1 error` line rather than the pass count. ## Live verification Discharged above — AT1/AT2/AT4 all come from run `33854882366`, a genuine cache-miss run of this PR's own branch: cold install 108.8s on the first attempt, `typecheck-infra` success, both caches written. Nothing about the cold path is deferred. Deploy verification: `isDeploySurfaceFile` returns **true** for `packages/domain/src/configuration/sources/environment.ts` (the env-var registration) and false for the other eleven changed files, so this is a deploy-surface PR — `deployment_wait-for-latest` against `minsky-mcp` with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge SHA, post-merge, per §10. Co-Authored-By: minsky-ai[bot] <minsky-ai[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
1 participant
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.
This PR caches downloaded plugins between runs as a performance optimization.
Closes #804