Skip to content

ci: cache the Go gate's lint and libvips, and bound PR cache churn - #242

Merged
drondeseries merged 9 commits into
mainfrom
fix/ci-libvips-cache
Oct 7, 2026
Merged

drondeseries merged 9 commits into
mainfrom
fix/ci-libvips-cache

Conversation

@randrini

@randrini randrini commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Related issue: N/A
Validation tasks: none

This supersedes #241, which is closed: this branch is stacked on it, so its lint-cache and DB-job changes are included here.

The Go gate had three cache and setup problems.

The full-tree router-recovery lint never restored golangci-lint's analysis cache, so make lint-router-recovery re-analyzed the whole tree from scratch every run (~14 min) — the job was the CI critical path at ~19.6 min. It also ran three Postgres tests with no database service and no SILO_TEST_DATABASE_URL, so all three skipped while still paying their compile time on the critical path.

Every GitHub-hosted Go job installs libvips-dev from apt each run: an apt-get update plus a ~108 MB dependency tree, about three minutes, and it occasionally stalls on a slow mirror (one run sat on it for over twenty minutes).

The composite action then saved a per-PR copy of its build and module cache: 0.6–1.4 GB per lane, per run. Two commits of one PR left ~10 GB of PR-scoped caches, filling GitHub's 10 GB budget. GitHub evicted the small caches first, including the analysis cache, so the lint lanes ran cold even with a warm build cache.

Approach

The three Postgres tests move to a new go-integration job: a pgvector/pgvector:pg18 service, SILO_TEST_DATABASE_URL, a go run ./cmd/silo/ --migrate-only step, then the tests, in parallel with the rest of the gate. lint-router-recovery keeps the full-tree lint, restores the golangci-lint analysis cache, and uses the composite setup action.

The composite action caches the apt .deb files per runner image and installs them offline with apt-get --no-download on a warm cache. On a cold cache it tries the install before apt-get update — the runner image ships package lists — and bounds the update with Acquire timeouts and retries, because a stalled mirror once held that step for 22 minutes. The changed-lines lint job drops its hand-rolled apt install and uses the composite.

PR runs now restore the build and module cache without saving it; only a push to main saves. The golangci-lint analysis caches follow the same rule, so every PR restores one shared cache instead of saving a copy no other PR can read. A new prune-go-caches job on main keeps only the newest cache per lane.

Validation

  • actionlint on ci.yml: clean; both edited YAML files parse.
  • Ran the new go-integration steps locally in the repo's test image (Go 1.26.8 + libvips) against a fresh pgvector/pgvector:pg18: migrations applied 522/522, and all three tests passed (internal/adminjob, internal/recommendations, cmd/silo).
  • Offline libvips install verified in a clean ubuntu:24.04 container: populated the cache with the real apt command (285 packages, 108 MB), then installed from it in a fresh container with --network none and --no-download; headers present.
  • Prune selection logic verified against a sample cache list: deletes all but the newest entry per lane and ignores other refs.
  • No Go or web code changed.

Risks

CI scheduling and cache policy only. The first run after this lands is cold and populates the main-scoped caches; later PR runs restore them. If GitHub bumps the runner image, the libvips key changes and the packages reinstall once.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: OpenCode
  • Tool(s): OpenCode agent runtime (oh-my-opencode-slim); verification used the gh CLI and Docker
  • Model(s): opencode-go/deepseek-v4.1-flash
  • Involvement: Fully AI-generated, human verified
  • Adversarial review: n/a — CI scheduling change, no runtime code touched

randrini added 3 commits October 7, 2026 22:04
The "Go lint (router recovery, full tree)" job was the CI critical path at
~19.6 min. Three changes cut it down.

It never restored golangci-lint's analysis cache, so
`make lint-router-recovery` re-analyzed the whole tree from scratch every run
(~14 min). The go-lint job runs the identical command over a restored cache
in under a minute, so this job now restores that cache too, including the
entries go-lint writes.

It also carried three Postgres tests with no database service and no
SILO_TEST_DATABASE_URL, so every one skipped while still paying its compile
time on the critical path. They move to a new parallel go-integration job
with a pgvector service and a migration step, so they actually run.

The job now uses the repo's composite setup-go action, which restores a
per-commit build cache, instead of a bare setup-go that restored only its own
stale go.sum-keyed cache.
Every GitHub-hosted Go job installs libvips-dev from apt: an apt-get update
plus a ~108 MB dependency tree, about three minutes. It also occasionally
stalls on a slow mirror; one run sat on that step for over twenty minutes.

The composite setup-go action now caches the downloaded .deb files per runner
image. A warm cache installs them offline with --no-download, so there is no
apt-get update and no downloads. The key is the runner image, not the commit,
and the cache is saved only on a push to main, where PRs can restore it. The
private runner image still short-circuits on pkg-config.

The changed-lines lint job stops hand-rolling its own apt install and uses the
composite action, so it gets the same cached packages and build cache.
@randrini
randrini force-pushed the fix/ci-libvips-cache branch from fcee0da to a4d221e Compare October 7, 2026 21:16
@randrini
randrini changed the base branch from main to fix/ci-lint-cache October 7, 2026 21:16
The composite action restored the build and module cache and then saved a
per-PR copy: 0.6-1.4 GB per lane, per run. Two commits of one PR left about
10 GB of PR-scoped caches, filling GitHub's 10 GB budget. GitHub evicted the
small caches first, including the golangci-lint analysis cache, so the lint
lanes ran cold: router-recovery took about 9 minutes in both jobs even with
go-lint's build cache warm.

PR runs now restore the build and module cache without saving it; only a push
to main saves. The golangci-lint analysis caches follow the same rule, so every
PR restores one shared cache instead of saving a copy no other PR can read. A
new prune job on main keeps only the newest cache per lane, bounding main's own
per-commit saves.
@randrini randrini changed the title ci: cache libvips packages so the Go jobs skip apt on warm runners ci: cache libvips packages and stop PR runs filling the cache quota Oct 7, 2026
@randrini
randrini changed the base branch from fix/ci-lint-cache to main October 7, 2026 21:42
@randrini randrini changed the title ci: cache libvips packages and stop PR runs filling the cache quota ci: cache the Go gate's lint and libvips, and bound PR cache churn Oct 7, 2026
The composite's libvips install ran apt-get update and then install. On one run
the update stalled fetching package indexes from the mirrors for 22 minutes
(azure.archive.ubuntu.com was ignored and archive.ubuntu.com was slow), wedging
the job.

The runner image ships package lists, so try the install first and only run the
update if it cannot resolve. Add Acquire timeouts and retries so a stalled
mirror fails fast instead of holding the job open, and skip the translation
indexes.
@drondeseries

Copy link
Copy Markdown
Collaborator

Review — COMMENT (approve after 1 real fix)

Good work on a real quota failure. All 10 checks green (incl. new Go integration), MERGEABLE/CLEAN, no Go/web code. One defect in the prune job must be fixed, plus two small cleanups.

Must-fix

1. High — prune script crashes on any cache outside its lanes. The jq filter uses capture(...) without ?, so any non-matching key (golangci-lint-binary-*, setup-go-*, docker caches, etc.) aborts the whole map — verified against sample keys. One foreign cache means no lane gets pruned that run, silently defeating the job (the script then succeeds vacuously with empty cache_ids). Fix: capture(...) // empty or a select(.key | test("^...")) before capture. Add a foreign-key row to the sample-list verification.

Should-fix (non-blocking but worth it)

2. Stale comment re-introduced at ci.yml:78. Commit 2/5 fixed the header ("split across parallel jobs… and the DB-backed integration tests") but left "The Go gate is three jobs…" two paragraphs down — now wrong in both count and membership. Update to four lanes.

3. gh cache delete "$cache_id" syntax is uncertain. Bare numeric IDs have flip-flopped between key and ID parsing across gh versions. Run the exact command once against a real cache ID, or switch to the unambiguous gh api -X DELETE repos/{owner}/{repo}/actions/caches/{id}. Don't discover this on the first main push.

Verified correct

  • libvips offline install: key on ${ImageOS}-${ImageVersion}-${RUNNER_ARCH} (not commit) is right; Dir::Cache::archives redirect + --no-download is the correct offline mechanism; install-before-update with bounded update fallback is sound; Acquire timeouts/retries/Languages=none/ForceIPv4 address the observed 22-min stall. Private-runner pkg-config short-circuit preserved. The --network none container test is the right proof.
  • PR restore-only / main-save policy: composite restore on PR vs cache on non-PR is correct scoping; per-sha key + prefix restore-keys converge PRs on main's newest entry. libvips save gated on cache-hit != 'true' + non-PR avoids rewrite churn.
  • Prune scoping: ref == $GITHUB_REF + default-branch + non-PR confines deletes to main; actions: write is minimal and job-scoped (top-level stays contents: read); keep-newest-per-prefix retention shape is right; pagination handled.
  • Integration job + router cache (from the ci: cache the full-tree lint and give its DB tests a database #241 review, still valid): pgvector image, silo_test name, migrate-then-test ordering, selector coverage, ~7.4 min observed vs 19.6 baseline.
  • No gate erosion: docker.yml trigger note preserved, workflow name/semantics unchanged, fetch-depth: 0 retained where needed, concurrency unchanged.

Body is complete. No security surface beyond the standard ephemeral test password.

Gate: fix the prune capture, confirm the delete syntax, touch up the stale comment — then merge.

The prune jq used capture() without a guard, so a cache whose key matched no
lane (golangci-lint-binary-, setup-go-, node-cache-, docker) aborted the whole
map and the job pruned nothing. Skip non-lane keys before the capture.

Delete through the REST API instead of `gh cache delete`, whose bare-number
argument has parsed as a key on some gh versions. Refresh the stale "three
jobs" comment now that the gate has four lanes.
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in 817d67a.

  1. The prune filter now skips non-lane keys before the capture (select(.key | test($re))), so a foreign key can't abort the map. Re-ran the sample-list check with golangci-lint-binary-, setup-go-, node-cache-, and a PR-scoped key included: it deletes only the older entries for the real lanes and ignores the rest.
  2. Stale "three jobs" comment updated to four lanes.
  3. Deletes now go through gh api -X DELETE repos/$GH_REPO/actions/caches/$id instead of gh cache delete.

@drondeseries

Copy link
Copy Markdown
Collaborator

Final: APPROVE — ready to merge

Verified 817d67a6: prune select(.key | test($re)) guard independently re-ran clean (foreign + wrong-ref keys ignored, only the older lane entry selected); delete via REST API; four-lane comment fixed. Fresh CI 9/9 green on this head, MERGEABLE/CLEAN. Merging.

Two cold-path stalls hit the libvips install: apt-get update sat on a slow
mirror for 22 minutes, and an install-first attempt against a mirror returning
502s spent 16 minutes retrying per package (Acquire::Retries=3 multiplied the
per-package retries). Drop the install-first shortcut, let the update refresh
the runner's mirror selection, and bound the fetches with Retries=1 and 20s
timeouts.

The .deb cache used a restore-only step on PRs, so a PR could never warm it and
every run paid the flaky apt path. Use a single actions/cache step: a run that
restores the entry does not re-save, so it only writes when the shared entry is
missing, which lets a PR warm it before main ever runs.
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

One more change from watching the run. The cold apt path stalled twice: apt-get update sat on a slow mirror for 22 minutes, and the install-first shortcut against a mirror returning 502s spent 16 minutes retrying per package because Acquire::Retries=3 multiplied the per-package retries. Dropped the shortcut, let update refresh the runner's mirror selection, and set Retries=1 + 20s timeouts.

Also switched the deb cache to a single actions/cache step so a PR can warm it before main runs — a run that restores the entry doesn't re-save, so it only writes when the shared entry is missing, which keeps the churn self-limiting.

The deb cache never saved, so every run started cold and paid the flaky apt
path. sudo apt-get creates lock/ and partial/ in the archive dir as root, and
the cache step runs as the runner user, so tar failed with "Cannot open:
Permission denied" and the save was skipped.

chown the archive dir back to the runner user on exit, including the early
cache-hit exit, so the save succeeds and later runs install offline.
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Found why the deb cache never helped: it never saved. sudo apt-get creates lock/ and partial/ in the archive dir as root, and the cache step runs as the runner user, so tar failed with Cannot open: Permission denied and the save was skipped — every run started cold. The archive dir is now chowned back to the runner user on exit (including the early cache-hit exit).

@drondeseries

Copy link
Copy Markdown
Collaborator

Re-review of 740c9d37: NOT READY — do not merge

Two new commits landed after the 817d67a6 APPROVE, and CI is still running (all jobs IN_PROGRESS, UNSTABLE), so the merge gate can't clear regardless. The new commits also need a fresh read — they change reviewed behavior.

What changed since the APPROVE

1. e446239e — reverses the reviewed cold-path + cache policy. Drops the install-before-update shortcut (always update first now, on fresh 16-min-retry-storm evidence) with Retries=1/20s timeouts — and switches the .deb cache from restore-only-on-PR to a single actions/cache step any run can save on miss. Both respond to real field data, but neither is what was verified, and the single-step cache re-opens the quota-churn question this PR exists to close (concurrent cold-key PRs can race to each save).

2. 740c9d37 — fixes the .deb save that never worked. Root cause is credible (sudo apt-get leaves root-owned lock//partial/ in the archive dir, cache step runs as runner user → tar permission-denied → save skipped), and the trap ... chown ... EXIT covers both the cache-hit early exit and the download path. But the || true swallows a failure that would silently re-empty the cache, so one green run needs to prove the save actually lands (check Actions → Caches for a libvips-dev-* entry).

To verify when green

  • The update-first strategy bets the 22-min stall was stale mirror selection, not a slow mirror per se — plausible, but only a few green runs prove Retries=1 + 20s bounds both observed stalls without flaking healthy-but-slow mirrors.
  • Stale composite header ("installs them offline: no apt-get update and no downloads") now describes only the warm path — the cold path does update + download. Minor doc touch-up.
  • The capture($re) / REST-delete / four-lane comment fixes from 817d67a6 are untouched — still good.

YAML parses (both files). No Go/web code touched.

Gate: full green on 740c9d37 + visible libvips-dev-* cache write, then a focused re-review of the two new commits. The prior APPROVE covers 817d67a6, not this head.

Address the re-review:

- The .deb cache goes back to restore-only on PRs and save-on-main, so a cold
  key cannot be raced into a duplicate save by every job in a PR run. Now that
  the save bug is fixed, main warms it and PRs restore it.
- The composite header now says the warm path skips apt while the cold path
  runs the bounded update and install.
- The chown trap emits a warning instead of swallowing a failure, so a broken
  save is visible rather than silently re-emptying the cache.
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in 2afd820:

  • .deb cache back to restore-only on PRs / save-on-main, so a cold key can't be raced into a duplicate save. Now that the save works, main warms it and PRs restore it.
  • Composite header updated to say the warm path skips apt while the cold path runs the bounded update and install.
  • The chown trap emits a warning instead of swallowing a failure.

Gate evidence:

  • Full green on 740c9d3: run 37701483534, all 10 checks pass, 4.55 min total.
  • Cache write visible: libvips-dev-ubuntu24-20260927.320.1-X64 and libvips-dev-ubuntu24-20261004.327.1-X64, 44 MB each, at refs/pull/242/merge.
  • Re-ran the changed-lines job: Cache hit for: libvips-dev-ubuntu24-20261004.327.1-X64 → Cache restored successfully, setup 49s vs ~3.5 min cold.

@drondeseries

Copy link
Copy Markdown
Collaborator

Final re-review of 2afd8200: APPROVE — ready to merge

All 10 checks green on this head, MERGEABLE/CLEAN. Verified the final composite end-to-end:

  • Cache policy restored correctly. Restore uses actions/cache/restore@v4 with id: libvips-cache, which emits cache-primary-key — the save step's key: ${{ steps.libvips-cache.outputs.cache-primary-key }} resolves. Save gated on non-PR + cache-hit != 'true': PRs restore-only (no quota race on a cold key), main warms once per runner image. The quota concern from the hold note is closed.
  • chown trap is visible on failure. Warns via ::warning:: instead of swallowing — a broken save surfaces in the step log rather than silently re-emptying the cache.
  • Header accurate. Warm path (offline, no update/downloads) vs cold path (bounded update + install) now distinguished.
  • Always-update + Retries=1/20s accepted. Trades a small flake risk on slow-but-healthy mirrors for hard bounds on both observed stalls (22-min update hang, 16-min 502 retry storm). Failure mode is a retried workflow, not a production incident — appropriate for CI infra with this field data.
  • Earlier fixes intact. Prune select(test($re)) guard, REST-API delete, four-lane comment all untouched by the later commits.

Observed evidence: router-recovery lint 57s (from 19.6-min baseline), integration lane 1m42s, libvips-dev-* entries (~46MB) writing. Merging.

@drondeseries
drondeseries merged commit 00589f2 into main Oct 7, 2026
10 checks passed
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.

2 participants