Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 45 additions & 8 deletions .github/actions/build-packages/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,52 @@ description: "Compiles and builds Go services."
runs:
using: "composite"
steps:
- name: Setup Go
uses: ./.github/actions/go-setup-cache
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version-file: go.work
# Caching is handled explicitly below. setup-go keys its cache purely
# on the dependency files, so it could never store a build cache for
# the optimized (DEBUG_GCFLAGS="") binaries this job builds - every
# run would restore stale artifacts and recompile everything.
cache: false

- name: Restore Go caches
uses: actions/cache@v5
with:
path: |
~/go/pkg/mod
~/.cache/go-build
# The "opt" salt separates this cache from setup-go caches populated
# by other jobs with debugger-friendly build flags.
key: integration-go-opt-v1-${{ runner.os }}-${{ hashFiles('go.work', 'packages/*/go.mod', 'packages/*/go.sum', 'tests/integration/go.mod', 'tests/integration/go.sum') }}
restore-keys: |
integration-go-opt-v1-${{ runner.os }}-

- name: Build Packages
run: |
make -C tests/integration build-debug
make -C packages/db build-debug
make -C packages/orchestrator build-debug
make -C packages/api build-debug
make -C packages/envd build-debug
make -C packages/client-proxy build-debug
# The builds are independent, so run them concurrently: the Go build
# cache and module cache are safe for concurrent use and shared
# dependencies compile only once.
pkgs="tests/integration packages/db packages/orchestrator packages/api packages/envd packages/client-proxy"

pids=""
for pkg in $pkgs; do
log="/tmp/build-$(basename "$pkg").log"
make -C "$pkg" build-debug > "$log" 2>&1 &
pids="$pids $!:$pkg"
done

status=0
for entry in $pids; do
pid="${entry%%:*}"
pkg="${entry#*:}"
if ! wait "$pid"; then
echo "::error::build failed for $pkg"
status=1
fi
echo "===== $pkg ====="
cat "/tmp/build-$(basename "$pkg").log"
done
exit $status
shell: bash
3 changes: 3 additions & 0 deletions .github/actions/build-sandbox-template/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ inputs:
runs:
using: "composite"
steps:
# Note: this cannot run concurrently with a running orchestrator -
# create-build manages its own sandbox network state (slot pools,
# namespaces) and the two processes conflict, wedging the provisioning VM.
- name: Build Sandbox Template
env:
TEMPLATE_ID: "2j6ly824owf4awgai1xo"
Expand Down
38 changes: 38 additions & 0 deletions .github/actions/start-databases/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: "Start Databases"
description: "Kicks off the PostgreSQL, ClickHouse, and Redis containers in the background. Run this as early as possible: pulls and boots then overlap with the Go builds, and the start-services action later waits for health before migrating."

runs:
using: "composite"
steps:
- name: Start Database Containers
env:
CLICKHOUSE_USERNAME: "e2b"
CLICKHOUSE_PASSWORD: "clickity-clicky-click"
CLICKHOUSE_PORT: "9000"
CLICKHOUSE_DATABASE: "default"
run: |
# Everything is backgrounded so the image pulls and container boots
# overlap with the (minutes-long) package builds that follow. The
# start-services action waits for container health with a bounded
# timeout, which also surfaces any startup failure from here.
docker run -d --name postgres \
-e POSTGRES_USER=postgres \
-e POSTGRES_PASSWORD=local \
-e POSTGRES_DB=mydatabase \
-p 5432:5432 \
--health-cmd="pg_isready -U postgres" \
--health-interval=5s \
--health-timeout=2s \
--health-retries=5 \
postgres:18 &

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait for backgrounded database launches

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 👍 / 👎.


docker run -d --name redis \
-p 6379:6379 \
--health-cmd="redis-cli ping" \
--health-interval=5s \
--health-timeout=2s \
--health-retries=5 \
redis:8 &

make -C packages/clickhouse run &
shell: bash
95 changes: 35 additions & 60 deletions .github/actions/start-services/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,42 @@ inputs:
runs:
using: "composite"
steps:
- name: Run PostgreSQL Database
# The postgres/clickhouse/redis containers were already kicked off in the
# background by the start-databases action (early in the job, so their
# pulls and boots overlapped with the package builds). Here we wait for
# database health and migrate. ClickHouse migrations run goose directly
# on the host (migrate-host): building the migrator image took ~2 min per
# job even with layer caching, for a container whose only purpose is to
# run goose against migrations that get volume-mounted anyway.
- name: Wait for Databases and Migrate
env:
TESTS_E2B_API_KEY: "e2b_5ec17bd3933af21f80dc10bba686691c4fcd7057"
TESTS_E2B_ACCESS_TOKEN: "sk_e2b_17bd3933af21f80dc10bba686691c4fcd7057123"
TESTS_SANDBOX_TEAM_ID: "834777bd-9956-45ca-b088-9bac9290e2ac"
TESTS_SANDBOX_USER_ID: "2a5a9fc5-db8d-4af7-ac9e-d0f9272463bc"
CLICKHOUSE_USERNAME: "e2b"
CLICKHOUSE_PASSWORD: "clickity-clicky-click"
CLICKHOUSE_PORT: "9000"
CLICKHOUSE_DATABASE: "default"
run: |
docker run -d --name postgres \
-e POSTGRES_USER=postgres \
-e POSTGRES_PASSWORD=local \
-e POSTGRES_DB=mydatabase \
-p 5432:5432 \
--health-cmd="pg_isready -U postgres" \
--health-interval=5s \
--health-timeout=2s \
--health-retries=5 \
postgres:latest
while [ "$(docker inspect -f '{{.State.Health.Status}}' postgres 2>/dev/null)" != "healthy" ]; do echo "Waiting for PostgreSQL to be healthy..."; sleep 2; done
echo "PostgreSQL is healthy!"
wait_healthy() {
local name="$1" timeout="${2:-120}" i
for ((i = 0; i < timeout; i += 2)); do
if [ "$(docker inspect -f '{{.State.Health.Status}}' "$name" 2>/dev/null)" = "healthy" ]; then
echo "$name is healthy!"
return 0
fi
echo "Waiting for $name to be healthy..."
sleep 2
done
echo "::error::$name did not become healthy within ${timeout}s"
docker logs "$name" 2>&1 | tail -50 || true
return 1
}

wait_healthy postgres 120
wait_healthy clickhouse 120
wait_healthy redis 60

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3715e06. Configure here.


# Install extensions
docker exec postgres psql -U postgres -d mydatabase -c "CREATE SCHEMA extensions; CREATE EXTENSION IF NOT EXISTS pgcrypto SCHEMA extensions;"
Expand All @@ -58,60 +75,18 @@ runs:
echo "TESTS_E2B_ACCESS_TOKEN=${TESTS_E2B_ACCESS_TOKEN}" >> .env.test
echo "TESTS_SANDBOX_TEAM_ID=${TESTS_SANDBOX_TEAM_ID}" >> .env.test
echo "TESTS_SANDBOX_USER_ID=${TESTS_SANDBOX_USER_ID}" >> .env.test
set -x
make migrate
make -C tests/integration seed

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ea47dab. Configure here.

shell: bash

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v4

- name: Build codegen image with caching
uses: docker/build-push-action@v7
with:
context: packages/clickhouse
file: packages/clickhouse/Dockerfile
tags: clickhouse-migrator:latest
load: true # makes the image available for `docker run`
cache-from: type=gha
cache-to: type=gha,mode=max

- name: Run Clickhouse
env:
CLICKHOUSE_USERNAME: "e2b"
CLICKHOUSE_PASSWORD: "clickity-clicky-click"
CLICKHOUSE_PORT: "9000"
CLICKHOUSE_DATABASE: "default"
run: |
echo "CLICKHOUSE_MIGRATOR_IMAGE=clickhouse-migrator" >> .env.test
echo "REDIS_URL=localhost:6379" >> .env.test
echo "CLICKHOUSE_USERNAME=${CLICKHOUSE_USERNAME}" >> .env.test

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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.test

The 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

  1. Step env sets CLICKHOUSE_USERNAME=e2b, CLICKHOUSE_PASSWORD=clickity-clicky-click, CLICKHOUSE_PORT=9000, CLICKHOUSE_DATABASE=default. localhost is not in that env block, nor exported by any earlier step.
  2. Bash performs parameter expansion inside the double-quoted string.
  3. ${CLICKHOUSE_USERNAME}e2b, ${CLICKHOUSE_PASSWORD}clickity-clicky-click, $localhost → `` (empty), ${CLICKHOUSE_PORT} → `9000`, `${CLICKHOUSE_DATABASE}` → `default`.
  4. The literal line appended to .env.test is: 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.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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


make -C packages/clickhouse run &
while [ "$(docker inspect -f '{{.State.Health.Status}}' clickhouse 2>/dev/null)" != "healthy" ]; do echo "Waiting for Clickhouse to be healthy..."; sleep 2; done
echo "Clickhouse is healthy!"

# We build the image in separate step to cache it and avoid rebuilding it every time
make -C packages/clickhouse migrate-without-build
shell: bash

- name: Run Redis
run: |
docker run -d --name redis \
-p 6379:6379 \
--health-cmd="redis-cli ping" \
--health-interval=5s \
--health-timeout=2s \
--health-retries=5 \
redis:latest

while [ "$(docker inspect -f '{{.State.Health.Status}}' redis 2>/dev/null)" != "healthy" ]; do echo "Waiting for Redis to be healthy..."; sleep 2; done
echo "Redis is healthy!"
set -x
make migrate
make -C tests/integration seed

echo "REDIS_URL=localhost:6379" >> .env.test
make -C packages/clickhouse migrate-host
shell: bash

- name: Start Services
Expand Down
76 changes: 54 additions & 22 deletions .github/workflows/integration_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ on:
description: "Whether to run integration tests"
required: false
default: true
full-matrix:
type: boolean
description: "When true (main), run the whole suite once per compression config. When false (PRs), run only the production-like zstd1 config, split into package shards on parallel runners to cut wall time."
required: false
default: true
secrets:
CODECOV_TOKEN: { required: false }
jobs:
Expand All @@ -25,42 +30,60 @@ jobs:
strategy:
fail-fast: false
matrix:
include:
- name: uncompressed
compress_enabled: "false"
compress_type: ""
compress_level: ""
compress_workers: ""
dedup_mode: "default"
disable_memfd: "true"
- name: zstd1
compress_enabled: "true"
compress_type: "zstd"
compress_level: "1"
compress_workers: "8"
dedup_mode: "best_effort"
disable_memfd: "false"
- name: lz4
compress_enabled: "true"
compress_type: "lz4"
compress_level: "0"
compress_workers: "8"
dedup_mode: "direct_io"
disable_memfd: "false"
# On push to main, the full 3-config matrix (uncompressed/zstd1/lz4)
# runs the whole suite per config, as before.
#
# On PRs, only the production-like zstd1 config runs (most tests don't
# depend on compression/dedup/memfd settings), but the suite is split
# into shards on parallel runners. Each host saturates at
# go test -parallel=4 (Firecracker boots + zstd compression), so the
# only way to cut wall time is more hosts: the TestTemplateBuild*
# tests (real FC builds) are split in two halves by name prefix,
# api/sandboxes gets its own runner, and everything else shares the
# last one. Shards are resolved by TEST_SHARD in
# tests/integration/Makefile.
include: >-
${{ fromJSON(inputs.full-matrix
&& '[
{"name": "uncompressed", "shard": "all", "compress_enabled": "false", "compress_type": "", "compress_level": "", "compress_workers": "", "dedup_mode": "default", "disable_memfd": "true"},
{"name": "zstd1", "shard": "all", "compress_enabled": "true", "compress_type": "zstd", "compress_level": "1", "compress_workers": "8", "dedup_mode": "best_effort", "disable_memfd": "false"},
{"name": "lz4", "shard": "all", "compress_enabled": "true", "compress_type": "lz4", "compress_level": "0", "compress_workers": "8", "dedup_mode": "direct_io", "disable_memfd": "false"}
]'
|| '[
{"name": "zstd1-templates-1", "shard": "templates-builds-1", "compress_enabled": "true", "compress_type": "zstd", "compress_level": "1", "compress_workers": "8", "dedup_mode": "best_effort", "disable_memfd": "false"},
{"name": "zstd1-templates-2", "shard": "templates-builds-2", "compress_enabled": "true", "compress_type": "zstd", "compress_level": "1", "compress_workers": "8", "dedup_mode": "best_effort", "disable_memfd": "false"},
{"name": "zstd1-sandboxes", "shard": "sandboxes", "compress_enabled": "true", "compress_type": "zstd", "compress_level": "1", "compress_workers": "8", "dedup_mode": "best_effort", "disable_memfd": "false"},
{"name": "zstd1-rest", "shard": "rest", "compress_enabled": "true", "compress_type": "zstd", "compress_level": "1", "compress_workers": "8", "dedup_mode": "best_effort", "disable_memfd": "false"}
]') }}
env:
# Surfaced as env so upload steps can gate on presence (skipped on fork PRs).
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
# Build race-instrumented but optimized binaries: nobody attaches a
# debugger in CI, and -N -l slows both compilation and every test at
# runtime. Set at the job level so the build-packages step and the
# rebuilds inside the run/run-debug targets use identical flags (the
# second build is then a Go build cache no-op).
DEBUG_GCFLAGS: ""

steps:
- name: Checkout Code
uses: actions/checkout@v5

# Kick off the database containers first: their image pulls and boots
# run in the background and overlap with the package builds below.
- name: Start Databases
uses: ./.github/actions/start-databases

- name: Build Packages
uses: ./.github/actions/build-packages

- name: Initialize Host
uses: ./.github/actions/host-init

# Must run before Start Services: create-build manages its own sandbox
# network state (slot pools, namespaces) and conflicts with a running
# orchestrator doing the same - overlapping them wedges the
# provisioning VM until its deadline.
- name: Build Template
uses: ./.github/actions/build-sandbox-template
with:
Expand All @@ -86,6 +109,15 @@ jobs:
TESTS_ORCHESTRATOR_HOST: "localhost:5008"
TESTS_ENVD_PROXY: "http://localhost:3002"
TESTS_CLIENT_PROXY: "http://localhost:3002"
# Keep go test -parallel at 4: the host is the bottleneck, not
# client-side concurrency. Measured on the zstd1 config: at 8 the
# per-test times inflated 70-140% (net wall win of only ~1 min) and
# 10 template builds flaked on timeouts; at 12 concurrent FC builds
# (x8 zstd workers each) starved envd inits into "syncing took too
# long" failures across 87 tests. Wall time is cut by sharding
# across runners instead (see the matrix above).
TEST_PARALLELISM: "4"
TEST_SHARD: ${{ matrix.shard }}
run: |
# Run the integration tests
make test-integration
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/pull-request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ jobs:
- '.github/actions/build-packages/**'
- '.github/actions/build-sandbox-template/**'
- '.github/actions/host-init/**'
- '.github/actions/start-databases/**'
- '.github/actions/start-services/**'
- '.github/actions/go-setup-cache/**'
lint-inputs:
Expand Down Expand Up @@ -164,6 +165,10 @@ jobs:
# Only publish the results for same-repo PRs
publish: ${{ github.event.pull_request.head.repo.full_name == github.repository }}
run-tests: ${{ contains(fromJSON(needs.detect-changes.outputs.changed-scopes), 'integration-inputs') && needs.detect-changes.outputs.docs-only != 'true' }}
# PRs run only the production-like zstd1 config, sharded by package
# across parallel runners for wall time; the full compression matrix
# runs on push to main.
full-matrix: false
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
publish-test-results:
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ test:

.PHONY: test-integration
test-integration:
$(MAKE) -C tests/integration test
$(MAKE) -C tests/integration test-shard

.PHONY: connect-orchestrator
connect-orchestrator:
Expand Down
8 changes: 7 additions & 1 deletion packages/api/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,17 @@ build:
$(eval EXPECTED_MIGRATION_TIMESTAMP ?= $(expectedMigration))
CGO_ENABLED=0 go build -o bin/api -ldflags "-X=main.commitSHA=$(COMMIT_SHA) -X=main.expectedMigrationTimestamp=$(EXPECTED_MIGRATION_TIMESTAMP)" .

# Compiler flags for debug builds. The default disables optimizations and
# inlining so binaries are debugger-friendly. CI overrides this with an empty
# value: race-instrumented but optimized binaries compile faster and speed up
# every integration test at runtime.
DEBUG_GCFLAGS ?= -gcflags=all="-N -l"

.PHONY: build-debug
build-debug:
$(eval COMMIT_SHA ?= $(shell git rev-parse --short HEAD))
$(eval EXPECTED_MIGRATION_TIMESTAMP ?= $(expectedMigration))
CGO_ENABLED=1 go build -race -gcflags=all="-N -l" -o bin/api -ldflags "-X=main.commitSHA=$(COMMIT_SHA) -X=main.expectedMigrationTimestamp=$(EXPECTED_MIGRATION_TIMESTAMP)" .
CGO_ENABLED=1 go build -race $(DEBUG_GCFLAGS) -o bin/api -ldflags "-X=main.commitSHA=$(COMMIT_SHA) -X=main.expectedMigrationTimestamp=$(EXPECTED_MIGRATION_TIMESTAMP)" .

.PHONY: run
run:
Expand Down
Loading
Loading