ci(integration): isolate live-DB stacks instead of serialising them#731
Conversation
Integration jobs were being CANCELLED, not run — four times in ~80 minutes on 2026-07-20, most recently the `db=supabase` leg of #712, which is the exact variant that PR exists to fix. Cause: the three integration workflows share one job-level concurrency group, `integration-live-db-<db>`, deliberately not scoped by ref (d02bf63) because two PRs contend for a fixed host port as much as two pushes to one PR. But GitHub Actions allows only ONE in-progress plus ONE pending run per group — a third contender is cancelled outright. With three workflows funnelling into two keys, that fires under any load. The failure mode is worse than a flake: a cancelled job never runs, so its check goes ABSENT rather than red. The signal is silently lost, and `main` is not branch-protected, so nothing blocks the merge. Remove the contention rather than serialising it. A new `integration-db` composite action gives every job: - its own compose project name (run id + attempt + job + variant), so container names cannot collide; and - EPHEMERAL host ports — `CS_PG_PORTS` / `CS_PGRST_PORTS` carry a bare container port, so compose publishes with no fixed host side and the assigned port is read back with `docker compose port`. Nothing is shared between jobs, so nothing needs to queue, and the concurrency groups are gone from all three workflows. The compose files keep their fixed 55432 / 55433 / 55430 mappings when the vars are unset, so local dev and the help text in `test-kit/src/env.ts` are unchanged. Dropped the pre-`up` `docker compose down` too. With unique project names it is a no-op, and blanket-pruning is now actively unsafe: without the concurrency group, another job's stack may be live on the same runner. Also, since this adds a composite action: `.github/actions/` had no CODEOWNERS rule, though such actions run arbitrary steps in the same job with the same secrets as the workflow calling them. Added the rule and the matching assertion in `supply-chain.e2e.test.ts` (verified it fails without it). Fixed a stale claim in the Drizzle workflow header that `PGRST_URL` is left unset — the matrix had been setting it. Verified: compose interpolation resolves to the fixed mapping by default and to an ephemeral publish under the CI vars (`docker compose config`); YAML parses; the action's shell body is syntax-clean with no GitHub templating interpolated into it; 18/18 supply-chain e2e tests pass.
|
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntegration CI now uses a shared composite action to launch isolated Postgres or Supabase Compose stacks with ephemeral ports, exposes runtime connection URLs to tests, and performs project-scoped teardown. Workflow triggers and CODEOWNERS coverage were updated accordingly. ChangesIntegration database CI
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Verified on the runnerThe PR body flagged that the live Ephemeral ports were genuinely assigned (not a silent fallback to the fixed mappings):
The contention is gone. Five integration jobs ran concurrently — three of them the
Cancelled checks: 0. Note the two legs that both landed on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/integration-db/action.yml:
- Around line 83-99: Validate the PostgREST port in the supabase branch before
constructing or exporting pgrest_url. Capture the result of port_of postgrest
3000, fail with a clear error and docker compose status output when it is empty,
and preserve the existing pg_port validation and URL behavior for valid ports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01f103ca-e389-48b8-bd71-2398acb17312
📒 Files selected for processing (8)
.github/CODEOWNERS.github/actions/integration-db/action.yml.github/workflows/integration-drizzle.yml.github/workflows/integration-prisma-next.yml.github/workflows/integration-supabase.ymle2e/tests/supply-chain.e2e.test.tslocal/docker-compose.postgres.ymllocal/docker-compose.supabase.yml
CodeRabbit caught a real asymmetry in the new action: `pg_port` was checked for emptiness but the supabase branch built `pgrest_url` straight from `$(port_of postgrest 3000)`. `docker compose port` exits 0 with EMPTY stdout when a port is not bound, so an unbound PostgREST would have silently yielded `http://localhost:` and handed every `PGRST_URL` consumer (the Drizzle-supabase leg, both Supabase-workflow steps) a malformed URL instead of the immediate error the Postgres path already gave. Move the check into `port_of` itself so it applies by construction and the asymmetry cannot come back, rather than adding a second call-site guard. Each port is now captured in its own bare assignment so `set -e` fails the step unambiguously on a non-zero `port_of`. Verified with a stubbed `docker` that returns a port for `db` and nothing for `postgrest`: the Postgres read succeeds, the guard fires for postgrest, and the script exits 1 before exporting anything.
|
Good catch @coderabbitai — fixed in dccc322. Real asymmetry, and my bug. Rather than add a second call-site guard, I moved the emptiness check into Verified with a stubbed The script stops before exporting anything, instead of emitting |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Overview
Fixes a genuinely nasty CI failure mode: the three integration workflows funnelled into two ref-unscoped concurrency groups (integration-live-db-<db>), and GitHub Actions allows only one in-progress + one pending per group — so a third contender is cancelled, not queued. A cancelled job's check goes absent rather than red, and since main isn't branch-protected, the lost signal blocks nothing (it bit the db=supabase leg of #712 — the exact variant that PR exists to fix). This removes the contention instead of serialising it: a new integration-db composite action gives each job a unique compose project name + ephemeral host ports, so nothing is shared and nothing needs to queue.
Well-reasoned and cleanly executed. LGTM / approvable — details below; no blocking issues.
What I verified
- Isolation mechanism is correct. The compose files gain
ports: "${CS_PG_PORTS:-55432:5432}"— unset keeps the fixed local-dev mapping; set to a bare container port (CS_PG_PORTS=5432) makes Compose publish to an ephemeral host port. The action reads the assigned port back viadocker compose port, and the project name (cs-it-<run>-<attempt>-<job>-<variant>, lowercased/sanitised to[a-z0-9_-], always starting withc) is unique per job and attempt, so a re-run can't adopt a prior attempt's containers. - Fail-closed port readback.
port_oftreats empty stdout (whatdocker compose portprints, at exit 0, for an unbound port) as the failure signal — errors, dumpsps, returns 1 — and each port is a barex="$(port_of …)"assignment soset -euo pipefailfails the step unambiguously (correctly avoiding thelocal x=$(…)exit-status mask). The comment notes this closes a real review-found asymmetry where an unbound PostgREST would have silently producedhttp://localhost:. - Teardown is correctly guarded.
CS_COMPOSE_PROJECT/CS_COMPOSE_FILEare written to$GITHUB_ENVbeforedocker compose up, so even a failedup(e.g.--waittimeout) still tears down the right project; andif: always() && env.CS_COMPOSE_PROJECT != ''skips teardown cleanly when the stack never came up (no confusing-p ""error masking the real failure). Removing the pre-upblanketdown -vis right — with no concurrency group, another job's stack may be live on the runner, so a blanket prune would now be actively unsafe. - Script-injection-safe.
inputs.dbandgithub.jobreach the shell viaenv:(DB_VARIANT/JOB_ID), never templated into therunbody;matrix.dbis a static[postgres, supabase]. No user-controlled value interpolates into a shell script. - Governance gate is real.
/.github/actions/added to CODEOWNERS (composite actions run arbitrary steps with the workflow's secrets — same blast radius as/.github/workflows/) and asserted insupply-chain.e2e.test.ts; the PR states the assertion was verified to fail without the rule (non-vacuous). No changeset is correct — repo tooling only, no published surface. - The runner-only path is proven. The
docker compose portreadback can only run on a real runner, and all five of this PR's own integration jobs pass (Drizzle postgres+supabase, Supabase, prisma-next postgres+supabase) — so the added action executed end-to-end and worked.
Notes — non-blocking
- Increased concurrent load on the shared ZeroKMS workspace. The old Supabase workflow comment cited "a live-DB job that shares one ZeroKMS workspace should finish, not be interrupted" as part of the queueing rationale. With the groups gone, all three workflows × both variants can now run fully concurrently against the same
CS_WORKSPACE_CRN. This looks safe — the per-job DB is where suite state lives and it's now isolated, and I grepped the integration suites for workspace-global assertions (listClients/listKeys/key counts) and found none — but worth a moment's confirmation that no suite depends on workspace-exclusive ZeroKMS state, since that's the one shared resource the isolation doesn't cover. - No reaper for hard-killed leaks. A SIGKILL'd job skips even the
always()teardown, and the pre-upprune is (correctly) gone, so on a reused runner a leaked stack has nothing to clean it up. Harmless if the Blacksmith runners are ephemeral (fresh VM per job) — worth a one-line confirmation, else a periodic prune ofcs-it-*projects older than N hours would backstop it.
Verdict
A real, well-diagnosed fix for a silent-signal-loss failure mode, implemented with genuinely careful shell (fail-closed readback, guarded teardown, injection-safe env passing) and validated by its own green integration run. Both notes are non-blocking confirmations. I'd approve this — say the word and I'll flip it to a formal approval.
3 participants
Integration jobs have been getting cancelled rather than run — four times in ~80 minutes on 2026-07-20, most recently the
db=supabaseleg of #712, which is the exact variant that PR exists to fix. That job had never actually run; re-running it manually passed.Root cause
The three integration workflows share one job-level concurrency group,
integration-live-db-<db>, deliberately not scoped by ref (d02bf63) because two PRs contend for a fixed host port as much as two pushes to one PR.But GitHub Actions allows only one in-progress plus one pending run per concurrency group. A third contender is cancelled outright, not queued. With three workflows (
integration-drizzle,integration-supabase,integration-prisma-next) funnelling into two keys, that fires under any load:The failure mode is worse than a flake. A cancelled job never runs, so its check goes absent rather than red — the signal is silently lost, and
mainisn't branch-protected, so nothing blocks the merge.Fix — remove the contention instead of serialising it
A new
integration-dbcomposite action gives every job:CS_PG_PORTS/CS_PGRST_PORTScarry a bare container port, so compose publishes with no fixed host side, and the assigned port is read back withdocker compose port.Nothing is shared between jobs, so nothing needs to queue. The concurrency groups are gone from all three workflows, and the three duplicated
up/down/URL blocks collapse into one action.Local dev is unchanged. The compose files keep their fixed
55432/55433/55430mappings when the vars are unset, so the help text intest-kit/src/env.tsandlive-gate.tsstays accurate.Dropped the pre-
updocker compose downas well: with unique project names it's a no-op, and blanket-pruning is now actively unsafe — without the concurrency group, another job's stack may be live on the same runner.Two things found along the way
.github/actions/had no CODEOWNERS rule, though composite actions run arbitrary steps in the same job with the same secrets as the workflow calling them — same blast radius as.github/workflows/, which is covered. Added the rule plus the matching assertion insupply-chain.e2e.test.ts, and verified the assertion fails without it (not a vacuous gate).PGRST_URLis left unset; the matrix had been setting it. Corrected.Validation
docker compose configresolves to the fixed mapping by default and to an ephemeral publish under the CI vars — both variants, all three ports.env, avoiding the script-injection seam).The live
docker compose portreadback can only be exercised on a runner, so this PR's own integration jobs are the real test — they exercise every path the action adds.No changeset: repo tooling only, no published package surface affected.
Summary by CodeRabbit
New Features
Bug Fixes
Chores