ci: speed up integration tests#3235
Conversation
PR wall time for integration tests is ~21 min; on a recent successful run the zstd1 shard spent 2m26s building packages, ~4m on host init/template/ services, and 14m07s running tests. Several compounding causes: - The full suite (incl. the whole build+services pipeline) ran 3x per PR in the compression matrix, though most tests don't depend on compression/dedup/memfd settings. PRs now run only the production-like zstd1 config via a new full-matrix workflow input; push to main keeps all three configs. - go test ran with -parallel=4 while the harness allows 100 concurrent sandboxes; api/sandboxes alone is ~1250s of test time squeezed through 4 slots. Parallelism is now tunable via TEST_PARALLELISM (4 locally, 12 in CI). - All debug builds used -race -gcflags=all="-N -l"; disabling optimizations slows compilation and every test at runtime (envd runs inside every sandbox). -N -l is now opt-out via DEBUG_GCFLAGS, empty in CI, and set at the job level so the rebuilds inside run/run-debug targets become Go build cache no-ops. - postgres/clickhouse/redis started serially, each with an unbounded sleep-2 health loop, on unpinned images. They now start concurrently (overlapping the migrator image build), with bounded health waits and pinned tags. - 7 template tests built full Firecracker templates (~60-160s each) only to exercise metadata/authz paths (visibility updates, cross-team 403/ 404, default-tag deletion guard) that don't depend on build state. They now use a new RequestTemplateWithoutBuild helper (single API call): the env row, waiting build, alias, and default tag assignment all exist at request time, and PATCH/list/delete don't filter on build status. - envd process tests slept a fixed 10s/5s waiting for processes to start; they now poll the process list with EventuallyWithT.
PR SummaryLow Risk Overview In Reviewed by Cursor Bugbot for commit 3715e06. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| echo "TESTS_SANDBOX_USER_ID=${TESTS_SANDBOX_USER_ID}" >> .env.test | ||
| set -x | ||
| make migrate | ||
| make -C tests/integration seed |
There was a problem hiding this comment.
ClickHouse killed between CI steps
High Severity
The ClickHouse server is started with a shell background job in the Start Database Containers step, but that step returns as soon as docker run for Postgres and Redis finishes. When the step’s shell exits, the background make -C packages/clickhouse run process is typically torn down, unlike the detached Postgres and Redis containers, so the later wait_healthy clickhouse step can time out or race against a missing container.
Reviewed by Cursor Bugbot for commit ea47dab. Configure here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes CI and integration test performance by starting database containers concurrently, allowing compiler optimization overrides for debug builds, parameterizing test parallelism, replacing static sleeps with dynamic polling in process tests, and introducing a fast template creation helper that bypasses slow VM builds. The review feedback highlights two issues in the GitHub Actions workflow: a background Clickhouse process that will be prematurely terminated when its step ends, and a typo in the Clickhouse connection string where $localhost is used instead of localhost.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| set -x | ||
| make migrate | ||
| make -C tests/integration seed | ||
| make -C packages/clickhouse run & |
There was a problem hiding this comment.
In GitHub Actions, the runner automatically terminates all background processes spawned during a step once that step completes (via its process tree killer). Since make -C packages/clickhouse run is executed in the background (&) in the Start Database Containers step, it will be killed before the Wait for Databases and Migrate step runs.
If the Makefile target runs Clickhouse in the foreground, it will be terminated immediately at the end of this step. If it runs Clickhouse in the background (e.g., via docker run -d), the & is redundant.
Recommendation:
To ensure Clickhouse persists across steps, run the container directly using docker run -d (similar to Postgres and Redis), or keep the startup command in the same step where it is waited on and migrated.
| echo "CLICKHOUSE_PORT=${CLICKHOUSE_PORT}" >> .env.test | ||
| echo "CLICKHOUSE_PASSWORD=${CLICKHOUSE_PASSWORD}" >> .env.test | ||
| echo "CLICKHOUSE_DATABASE=${CLICKHOUSE_DATABASE}" >> .env.test | ||
| echo "CLICKHOUSE_CONNECTION_STRING=clickhouse://${CLICKHOUSE_USERNAME}:${CLICKHOUSE_PASSWORD}@$localhost:${CLICKHOUSE_PORT}/${CLICKHOUSE_DATABASE}" >> .env.test |
There was a problem hiding this comment.
The connection string contains @$localhost instead of @localhost. Since localhost is not a defined environment variable in this context, $localhost will expand to an empty string, resulting in an invalid or malformed connection string (clickhouse://e2b:clickity-clicky-click@:9000/default).
Please remove the $ prefix.
echo "CLICKHOUSE_CONNECTION_STRING=clickhouse://${CLICKHOUSE_USERNAME}:${CLICKHOUSE_PASSWORD}@localhost:${CLICKHOUSE_PORT}/${CLICKHOUSE_DATABASE}" >> .env.test
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea47dab30b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| reqEditors = append(reqEditors, setup.WithTestsUserAgent()) | ||
|
|
||
| return requestTemplateBuild(tb, name, nil, nil, nil, reqEditors...) |
There was a problem hiding this comment.
When these tests call RequestTemplateWithoutBuild, requestTemplateBuild still goes through RegisterBuild, which inserts an active_template_builds row for a pending build; because this helper never starts, finishes, cancels, or deletes that build, the row remains until the 40-minute sync timeout and is counted by GetInProgressTemplateBuildsByTeam. In integration runs that create real templates afterward—now with TEST_PARALLELISM=12—these leaked pending builds reduce the team build-concurrency headroom and can make later template requests fail with the max-concurrent-builds 429 even though no actual build is running.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| echo "TESTS_SANDBOX_USER_ID=${TESTS_SANDBOX_USER_ID}" >> .env.test | ||
| echo "REDIS_URL=localhost:6379" >> .env.test | ||
| echo "CLICKHOUSE_MIGRATOR_IMAGE=clickhouse-migrator" >> .env.test | ||
| echo "CLICKHOUSE_USERNAME=${CLICKHOUSE_USERNAME}" >> .env.test |
There was a problem hiding this comment.
🟣 Pre-existing typo on line 118: @$localhost: references an unset shell variable that expands to empty, so .env.test gets CLICKHOUSE_CONNECTION_STRING=clickhouse://e2b:...@:9000/default (empty host). This PR only relocates the line — since it's already being touched, dropping the $ while it's being moved would be a cheap drive-by fix (compare the neighboring REDIS_URL=localhost:6379 line, which has no dollar sign).
Extended reasoning...
What the bug is
Line 118 of .github/actions/start-services/action.yml writes:
echo "CLICKHOUSE_CONNECTION_STRING=clickhouse://${CLICKHOUSE_USERNAME}:${CLICKHOUSE_PASSWORD}@$localhost:${CLICKHOUSE_PORT}/${CLICKHOUSE_DATABASE}" >> .env.testThe fragment @$localhost: contains a bare $localhost — a bash parameter expansion referencing a variable that is never assigned anywhere in the composite action, the calling workflow, or any sourced file. Inside double quotes without set -u, bash silently expands an undefined variable to the empty string. Compare the neighboring REDIS_URL=localhost:6379 line, which is a literal localhost (no $) — that spelling is clearly the intent here as well.
Step-by-step proof
- Step env sets
CLICKHOUSE_USERNAME=e2b,CLICKHOUSE_PASSWORD=clickity-clicky-click,CLICKHOUSE_PORT=9000,CLICKHOUSE_DATABASE=default.localhostis not in that env block, nor exported by any earlier step. - Bash performs parameter expansion inside the double-quoted string.
${CLICKHOUSE_USERNAME}→e2b,${CLICKHOUSE_PASSWORD}→clickity-clicky-click,$localhost→ `` (empty),${CLICKHOUSE_PORT}→ `9000`, `${CLICKHOUSE_DATABASE}` → `default`.- The literal line appended to
.env.testis:CLICKHOUSE_CONNECTION_STRING=clickhouse://e2b:clickity-clicky-click@:9000/default— note the@:with no host between them.
Why existing code doesn't prevent it\n\nThe surrounding run: block doesn't use set -u, so undefined-variable references are non-fatal. Tests still pass because clickhouse-go/v2's DSN parser tolerates an empty host and defaults it to localhost — so runtime impact is masked. A stricter URL parser (or a different ClickHouse client) would reject the URL outright.\n\nImpact\n\nMinimal today, since integration tests are green with this typo. The risk is latent: swapping the ClickHouse driver, adding a URL-validation step, or reusing this env file in another tool would surface the broken host silently. Marked as pre_existing because the typo lives in commit 8f83959 (before this PR); this PR only relocates the line verbatim from the removed Run Clickhouse step to the new Wait for Databases and Migrate step without altering it.\n\nHow to fix\n\nDrop the $ on line 118:\n\nbash\necho "CLICKHOUSE_CONNECTION_STRING=clickhouse://${CLICKHOUSE_USERNAME}:${CLICKHOUSE_PASSWORD}@localhost:${CLICKHOUSE_PORT}/${CLICKHOUSE_DATABASE}" >> .env.test\n\n\nSingle-character edit, no behavior change (empty host already resolves to localhost in the client), and this PR is already touching the line — a natural drive-by cleanup.
At -parallel=12 the zstd1 shard failed 87 tests: concurrent Firecracker template builds (each with 8 zstd frame-encode workers) starved envd inits inside freshly booted sandboxes, surfacing as 'failed to init envd: syncing took too long' (44 occurrences in the orchestrator log, no OOM/data races).
Measured on the zstd1 shard: at -parallel=8 per-test times inflated 70-140% (api/templates 3232s vs 1898s, api/sandboxes 2994s vs 1248s of summed test time) for a net wall-clock win of ~1 min, with 10 template build flakes rescued by --rerun-fails; at 12, envd inits starved into 'syncing took too long' across 87 tests. The host saturates server-side (FC boots + zstd compression), so client concurrency beyond 4 only adds contention. The TEST_PARALLELISM knob stays for local runs and future tuning.
|
Parallelism tuning results from this PR's CI runs (zstd1 shard):
The host saturates server-side (FC VM boots + zstd compression with 8 workers per operation), so raising client-side This strengthens the follow-up note: the compression path is the dominant cost (uncompressed shard runs the same suite in 4m51s vs 14m07s for zstd1). |
Each CI host saturates at go test -parallel=4 (Firecracker boots + zstd compression), so client-side concurrency can't cut wall time - more hosts can. PRs now split the zstd1 config into three package shards on parallel runners: api/templates (~1900s of summed test time) and api/sandboxes (~1250s) each get a dedicated runner, everything else shares the third. Shard resolution lives in tests/integration/Makefile (TEST_SHARD=templates|sandboxes|rest|all); the rest shard is computed via go list so new packages are picked up automatically. Push to main still runs the full suite per compression config. Also start the postgres/clickhouse/redis containers in the background right after checkout (new start-databases action), so their image pulls and boots overlap with the ~3 min package build instead of serializing in front of the migrations.
The previous long pole (zstd1-templates, 14.7 min) was ~7 min of setup plus ~7 min of tests. Both halves shrink: - Split api/templates across two runners by test-name prefix (^TestTemplateBuild = 417s vs the rest = 460s of measured test time), since each host saturates at -parallel=4. - Build packages concurrently instead of serially (2m48s -> bounded by the slowest binary) and give the integration jobs their own salted Go cache: setup-go keys purely on dependency files, so it kept restoring stale -N -l artifacts and forced full recompiles every run. - Stop building the clickhouse migrator image (1m54s per job even with gha layer caching): its only job is running goose against migrations that get volume-mounted anyway, so run goose directly on the host via a new migrate-host target (pinned by go.mod tool directive). - Run the sandbox template build (~40s) in the background overlapping service startup; tests gate on a wait step that surfaces the log and exit status.
The background overlap wedged every job: create-build manages its own sandbox network state (slot pools, namespaces) and conflicts with the orchestrator starting up concurrently, so the provisioning VM never came ready and hit its 5-minute deadline. The overlap only saved ~40s; run it synchronously before Start Services as before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8311d9280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --health-interval=5s \ | ||
| --health-timeout=2s \ | ||
| --health-retries=5 \ | ||
| postgres:18 & |
There was a problem hiding this comment.
In the integration job running on the self-hosted infra-tests runner, this docker run is now backgrounded and its exit status is never observed. If a cancelled or failed prior job leaves a healthy container named postgres (similarly redis below), the new launch fails with a name conflict but the step still succeeds; the later wait_healthy postgres only checks by name and can accept the stale database, so migrations/seeds/tests run against leftover state instead of a fresh container. Before this change the foreground docker run failure stopped the job immediately.
Useful? React with 👍 / 👎.
The templates-builds shard was 11.6 min: its 18 tests are all real FC builds (~1400s of summed test+subtest time) that barely parallelize on one host. Split them in two balanced halves by name prefix (^TestTemplateBuild(ENV|COPY|RUN|W) = ~680s vs the rest = ~740s) and fold the non-build template tests into the rest shard (separate packages run concurrently within one gotestsum invocation; the -skip only affects the templates package since no other package has TestTemplateBuild* tests). Still four runners total.
The rest shard came in at 9.9 min - too close to the 10-minute budget. Move proxies, api/metrics, and api/volumes from rest (261s of tests) to the sandboxes runner (160s) to even the two out at roughly 210s each.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3715e06. Configure here.
|
|
||
| wait_healthy postgres 120 | ||
| wait_healthy clickhouse 120 | ||
| wait_healthy redis 60 |
There was a problem hiding this comment.
Health wait ignores timeout failure
Medium Severity
The new wait_healthy helper returns a non-zero status when a database container does not become healthy in time, but the step never enables set -e or checks those return codes. After a timeout the job still runs migrations and seeds against containers that may not be running.
Reviewed by Cursor Bugbot for commit 3715e06. Configure here.
|
Final result after the sharding + setup work: PR wall time 21.4 min → 7.2 min, under the 10-minute budget with ~3 min of headroom.
Coverage verified from the shard junits: all 219 top-level tests run exactly once across the four shards (none missing, none duplicated), 0 failures, no rerun-rescues. Steady-state should improve slightly further once push-main runs keep the salted Go cache warm for PR branches. |
1 participant
Why
PR wall time for integration tests was ~21.5 min, gated by the slowest job of a 3-config compression matrix where every job rebuilds everything and runs the full suite. Target: under 10 minutes.
Baseline (
28977209000, zstd1 = long pole): Build Packages 2m26s, host init + template + services ~4m20s, tests 14m07s, total 21m24s.Parallelism experiments on this PR showed each host saturates server-side (Firecracker boots + zstd compression) right at
go test -parallel=4: at 8, per-test times inflated 70–140% for ~1 min net gain and 10 template builds flaked; at 12, envd inits starved (syncing took too long) and 87 tests failed. Client-side concurrency can't cut wall time — more hosts can.What
Sharding for wall time (the main change)
TEST_SHARDintests/integration/Makefile):templates-builds-1/templates-builds-2: the^TestTemplateBuild*tests (real FC builds, ~1400s of summed test time that barely parallelizes on one host), split into balanced halves by name prefix — new tests fall into shard 2 automatically.sandboxes:api/sandboxes+ the light packages (proxies, metrics, volumes).rest: everything else viago list(auto-picks-up new packages), including the non-build template tests.full-matrixinput).Setup cut from ~7 min to ~4.5 min per job
-N -lartifacts and recompiling everything; after merge, push-main runs will keep the salted cache warm for PRs).migrate-host, pinned via go.mod tool directive) — building the migrator image cost ~2 min/job even with gha layer caching, for a container whose migrations are volume-mounted anyway.start-databasesaction), overlapping the Go build;start-serviceswaits with bounded health loops (previously infinite) on pinned images.-gcflags=all=\"-N -l\"is now opt-out (DEBUG_GCFLAGS, empty in CI): race detection stays on, optimizations stay on.Less test work
RequestTemplateWithoutBuildhelper: 7 tests that only exercise template metadata/authz no longer build full FC templates (~60–160s each). Verified none of these API paths filter on build status.EventuallyWithTpolling.Learned the hard way
create-buildmanages its own sandbox network state and the two conflict, wedging the provisioning VM (reverted after one red run).Measured result (run
29041851323)PR wall time: 21.4 min → 7.2 min (−66%), runner usage ~50 → ~26 min. Verified across the shard junits: all 219 top-level tests run exactly once (none missing, none duplicated), 0 failures.
Not changed (checked and rejected)
-parallel(measurements above; knob kept for local/future tuning).MAX_PARALLEL_MEMFILE_SNAPSHOTTING: dead config, nothing reads it.POST /templates/tagsrequires a ready build.Follow-up worth investigating
The compression path is the dominant remaining cost: the same suite runs 3x faster uncompressed than under zstd1 with
compress_workers=8, and the host saturates at 4 concurrent FC operations. Likely production-relevant contention, worth profiling independently of CI.