Skip to content

ci: cache the full-tree lint and give its DB tests a database - #241

Closed
randrini wants to merge 2 commits into
mainfrom
fix/ci-lint-cache
Closed

randrini wants to merge 2 commits into
mainfrom
fix/ci-lint-cache

Conversation

@randrini

@randrini randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

Related issue: N/A
Validation tasks: none

The Go lint (router recovery, full tree) job is the long pole of the CI run at about 19.6 minutes, while every other job finishes in 3–9 minutes. Two things keep it there.

It never restores golangci-lint's analysis cache, so make lint-router-recovery type-checks and analyzes the whole tree from scratch on every run — about 14 minutes. The go-lint job runs the identical command over a restored cache in 23–59 seconds.

It also runs three Postgres tests with no database service and no SILO_TEST_DATABASE_URL, so all three skip while still paying their compile time on the critical path.

This change restores the cache, warms the build cache, and gives those tests a real database.

Approach

lint-router-recovery now restores ~/.cache/golangci-lint under a job-specific key with restore-keys that also pick up the cache go-lint writes, and uses the repo's composite setup-go action so it gets the same per-commit build cache as the other Go jobs. The explicit libvips and embed-stub steps are gone because the composite action does both.

The three 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. It runs in parallel with the rest of the gate.

Validation

  • actionlint .github/workflows/ci.yml: clean.
  • Ran the new job's 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).
  • No Go or web code changed, so no other gate applies.

Risks

CI scheduling only. The first run after this lands starts cold for the new build cache; later runs reuse it. No migration, compatibility, security, or operational impact.

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

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.
@drondeseries

Copy link
Copy Markdown
Collaborator

Review — APPROVE (gate: full green)

Single-file CI change (+76/-28, .github/workflows/ci.yml only). No Go/web code touched. Verified YAML parses (base + head), composite action, migrate path, and test selectors.

What's correct

  • Analysis cache wiring is right. New key golangci-lint-cache-router-<os>-<ver>-<sha> falls through to the shared golangci-lint-cache-<os>-<ver>- prefix that go-lint saves under (ci.yml:116). Path ~/.cache/golangci-lint is the default cache dir. actions/cache@v4 with a sha-key saves at post-step, so the next run restores.
  • Composite swap is complete, not lossy. Old steps (apt libvips, bare setup-go, embed-stub) are all inside .github/actions/setup-go (libvips with pkg-config short-circuit, cache: false + per-commit build cache, stub). cache-name: lint-router-recovery keeps its own lane per the action's contract. This also drops the stale go.sum-keyed cache the bare setup-go restored.
  • Integration job is sound. pgvector/pgvector:pg18 matches docker-compose.yml and the go-test service (migrations + taste tests need the vector extension). DB name silo_test contains "test" per the testdb rule. Migrate step works: LoadBootstrap only requires DATABASE_URL + SECRET_KEY (redis optional, .env load ignored), --migrate-only exists (cmd/silo/main.go:731,826), openssl is stock on ubuntu-latest. cmd/silo test compile needs the embed stub — the composite provides it, which is why the composite (not bare setup-go) matters here too.
  • Selectors match real tests. TestUpsertTasteClusters.*Postgres$ → 3 tests, jellycompat → 1, adminjob pattern → 10 (broader than "three", all DB-gated via lifecycleRepo, harmless). Sequential steps in one job, each test cleans up after itself.
  • Evidence is real. Router-recovery job on this PR ran ~7.4 min vs the ~19.6 min baseline — the warm cache is demonstrably working.

Lows (non-blocking)

  1. Header comment stale (ci.yml:32-38): "split across three parallel jobs" predates the fourth Go lane. One-line refresh.
  2. Cache write volume doubles for the analysis-cache class (per-sha saves from both go-lint and router job). Fine unless quota bites — watch eviction.
  3. Newly-enabled tests are newly-failing-capable. They skipped unconditionally before; now they gate. Author's local run (522/522 migrations, all passing) is good evidence, but watch the first few CI runs for -race flakiness.
  4. CI not green yet — Go integration, Go test, Go lint, contract checks still IN_PROGRESS. Merge only on full green.

No security surface (ephemeral postgres password + random SECRET_KEY for migrate), no docs impact, one concern.

@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the stale header comment in dbd7cfa; it now names the integration lane alongside the changed-lines and full-tree lints. The other three lows need no change: cache-write volume and first-run -race flakiness are watch-items, and the gate is held for full green on the new run.

@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #242. That branch is stacked on this one, so it carries these commits plus the libvips deb cache and the cache-quota fix, and one PR avoids re-running the flaky changed-lines job here. Closing.

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