From b5c557c3f6787c460ed0c5367882a5199f15bc23 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sun, 6 Sep 2026 10:20:23 +0200 Subject: [PATCH] =?UTF-8?q?ci(test):=20remplace=20le=20verrou=20docker=20p?= =?UTF-8?q?ar=20une=20attente=20des=20jobs=20test=20ant=C3=A9rieurs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Les deux approches précédentes de cette branche ne tenaient pas sur ce runner : - Nom de conteneur Postgres par run + `$GITHUB_ENV` (721a1ec) : laissait tourner le conteneur `postgres` nu de l'ancien ci.yml du run précédent → deux Postgres sur un runner à court de RAM → I/O DB ~5× plus lent → `resetDatabase()` dépasse le timeout Mocha de 10 s, cascade. Run 125 a échoué ainsi *en solo*. - Verrou `docker network create ci-test-lock` (785d97b) : ce runner (forgejo-runner v3.5.0) n'exécute PAS l'étape de libération `if: always()` après l'échec d'une étape → le verrou fuit à chaque run rouge. Retour au ci.yml de `main` (qui passe 614/614) + une seule étape en plus : « Wait for any earlier test job » boucle tant qu'un conteneur `GITEA-ACTIONS-TASK-_..._JOB-test` de task id inférieur au sien tourne. Le runner retire toujours le conteneur de job (succès comme échec), donc rien à libérer ; task id le plus bas d'abord → pas d'interblocage. `DATABASE_URL` et l'étape « Start Postgres » redeviennent identiques à `main`. `timeout-minutes: 45` conservé. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 203 ++++++++++++--------------------------- 1 file changed, 61 insertions(+), 142 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91f68eb..efdd8d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,18 +12,13 @@ on: push: env: - # Only the `test` job ever opens a database connection, and it *overwrites* - # this value at runtime (see its "Start Postgres" step) with a per-run - # hostname. This entry stays for one reason: `build`'s `pnpm install` runs - # `prisma generate` (apps/api `postinstall`), which wants DATABASE_URL to - # be present and syntactically valid — it never dials the host, so the - # literal `postgres` here need not resolve anywhere. - # - # Why the `test` job runs Postgres as a plain `docker run` and not a - # `services:` block: this repo's Forgejo `act` runner does not keep a - # `services:` container as the *same* instance across a job's steps, so - # `migrate deploy` would land the schema in one Postgres and `pnpm --filter - # api test`'s `beforeAll` would then hit an empty one ("table … does not + # Hostname `postgres` = the container the `test` job starts itself (see + # that job's "Start Postgres" step) — a plain `docker run`, not a + # `services:` block, for the same reason the intent-service below can't be + # one: this repo's Forgejo `act` runner does not keep a `services:` + # container as the *same* instance across a job's steps, so `migrate + # deploy` would land the schema in one Postgres and `pnpm --filter api + # test`'s `beforeAll` would then hit an empty one ("table … does not # exist" right after "migrations successfully applied"). One long-lived # container in the job, shared by every step, removes that failure mode. DATABASE_URL: "postgresql://ci:ci@postgres:5432/batchcooking_ci?schema=public" @@ -66,140 +61,86 @@ jobs: test: runs-on: ubuntu-latest - # The active job is ~16-19 min (runs 117-121); the rest of the budget is - # for time spent queued on the serialization lock below (a realistic - # pile-up is two, sometimes three, runs). Also a runaway ceiling: Mocha - # runs with no `--exit` (`apps/api/.mocharc.json`), so one leaked handle - # would otherwise hang the job until the runner's global timeout — a - # `test` job zombied ~1h once before being cancelled by hand. If a hung - # holder ever hits this ceiling its `if: always()` "Release the test-job - # lock" step still frees the queue (and the stale-lock sweep frees it - # sooner, at ~35 min). - timeout-minutes: 60 + # ~16-19 min of actual work (runs 117-124); the extra budget covers time + # spent queued in "Wait for any earlier test job" below. Also a runaway + # ceiling — Mocha has no `--exit` (`apps/api/.mocharc.json`), so one + # leaked handle would otherwise hang the job until the runner's global + # timeout (a `test` job zombied ~1h once). If a hung job hits this + # ceiling the runner removes its container, which unblocks the queue. + timeout-minutes: 45 steps: - name: Install curl and the docker CLI run: | apt-get update && apt-get install -y curl docker.io - # ── Serialize the `test` job across every concurrent run ───────────── - # GitHub's `concurrency:` is the native "only one `test` job at a time, - # across all branches", but this Forgejo (9.0.3) predates Actions - # `concurrency` entirely — it landed in Forgejo v14 / Gitea 1.26, and - # the key is silently ignored on anything older. The one resource every - # job on this runner shares is the docker daemon, so a uniquely-named - # docker object is the mutex: `docker network create` fails if the name - # is taken, so the first run to create `ci-test-lock` holds it and the - # others spin until it is freed — queue, don't cancel (the task's - # preferred `cancel-in-progress: false`: a half-finished `main` run - # shouldn't be killed by a feature push). + # Serialize `test` across every concurrent run. Two `test` jobs must + # never overlap: they share one `--name postgres` container on the + # runner's shared docker network (next step), so their `prisma migrate + # deploy` + `seedReferenceData` (root Mocha `beforeAll`) race — + # `Unique constraint failed on (key)` in `seedReferenceData`, "No + # pending migrations" when there should be some, `tech_step`/`diet` id + # shifts breaking unrelated assertions; and even past that they starve + # each other's RAM/CPU (each also trains a spaCy model) until + # `resetDatabase()` overruns Mocha's 10s hook timeout and cascades. + # Hit repeatedly (PRs #47, #53, #54), each needing an empty-commit + # re-trigger once the runner was idle. # - # Why serialize and not just give each run a private database (it gets - # one too, next step): two `test` jobs on this single runner also - # contend for CPU — each trains a spaCy model for its own - # intent-service *and* runs the full Mocha DB suite — and under that - # load a `resetDatabase()` hook overran Mocha's 10s timeout; Mocha then - # started the next file's hook while the timed-out one's TRUNCATE + - # reseed was still in flight, and the two collided inside one run's own - # database (`deadlock detected`, `Unique constraint failed on (key)` in - # `seedReferenceData`, intent-service starved to F1 0.000). Run 122 - # failed exactly that way while run 123's `test` job overlapped it. - - name: Acquire the test-job lock + # GitHub's `concurrency:` is the native fix but this Forgejo (9.0.3) + # predates Actions `concurrency` — it arrived in Forgejo v14 / Gitea + # 1.26 and is silently ignored here. A lock object (network, container, + # file) would leak: this runner (forgejo-runner v3.5.0) does NOT run a + # trailing `if: always()` step after an earlier step fails, so there is + # no reliable in-job release. Instead, key off the one thing the runner + # *does* always clean up — the job container itself (removed on pass, + # fail, and cancel alike): wait while any lower-numbered `test` job's + # container is still running. Lower task id goes first, so two jobs + # that start together can't deadlock; nothing to release. + - name: Wait for any earlier test job run: | - # Force-release a lock older than ~35 min (well past a normal - # ~19-min run): its holder was killed outright before the release - # step could run (SIGKILL skips `if: always()`), or is wedged. - # `timeout-minutes` on this job is the backstop for a holder that - # is merely slow. Fail safe: if the age can't be read, leave the - # lock alone rather than risk nuking a live holder's lock. - created="$(docker network inspect -f '{{.Created}}' ci-test-lock 2>/dev/null || true)" - if [ -n "$created" ]; then - # `.Created` renders as RFC3339 on newer daemons and as Go's - # "2006-01-02 15:04:05 -0700 MST" on older ones — strip a trailing - # timezone name so GNU `date -d` accepts either. - created="$(printf '%s' "$created" | sed 's/ [A-Za-z][A-Za-z]*$//')" - if created_epoch="$(date -d "$created" +%s 2>/dev/null)"; then - age=$(( $(date +%s) - created_epoch )) - if [ "$age" -gt 2100 ]; then - echo "test-job lock is ${age}s old — assuming abandoned, releasing it" - docker network rm ci-test-lock >/dev/null 2>&1 || true - fi - else - echo "could not read the test-job lock's age ('$created') — leaving it in place" - fi - fi - # Up to ~40 min of waiting (80 * 30s); past that the stale-release - # above would have fired for any real hang, so give up and let the - # re-run get through. - for i in $(seq 1 80); do - if docker network create ci-test-lock >/dev/null 2>&1; then - echo "test-job lock acquired" - # Marker checked by the release step so a job that gave up - # waiting (and never held the lock) can't delete another run's. - # /tmp persists across steps (same job container) and, unlike - # the workspace, isn't wiped by actions/checkout. - touch /tmp/ci-test-lock-held + my_id="$(printf '%s\n' "${JOB_CONTAINER_NAME:-}" | sed -n 's/.*TASK-\([0-9]\{1,\}\).*/\1/p')" + echo "this test job is task ${my_id:-}" + # ~25 min cap (75 * 20s): comfortably covers one earlier run + # (the realistic overlap is two runs); a rarer 3-way pile-up whose + # wait exceeds this just fails and gets an empty-commit re-run. + for attempt in $(seq 1 75); do + earlier="$(docker ps --format '{{.Names}}' 2>/dev/null \ + | sed -n 's/.*TASK-\([0-9]\{1,\}\)_.*_JOB-test$/\1/p' \ + | awk -v me="${my_id:-0}" 'NF && ($1 + 0) < (me + 0)')" + if [ -z "$earlier" ]; then + [ "$attempt" -gt 1 ] && echo "earlier test job(s) finished — proceeding" exit 0 fi - echo "waiting for the test-job lock, held by another run… attempt ${i}" - sleep 30 + echo "waiting for earlier test job(s): $(printf '%s ' $earlier)(attempt ${attempt})" + sleep 20 done - echo "could not acquire the test-job lock within ~40 min" >&2 + echo "still blocked by an earlier test job after ~25 min — giving up; a re-run should get through" >&2 exit 1 # Postgres as a plain background container instead of a `services:` # block — see this workflow's top-level `env.DATABASE_URL` comment for - # why. The runner mounts the docker socket and runs every job on one - # shared user-defined network, so a `--name` sibling resolves by name; - # the network is discovered from the job container itself rather than - # hardcoded. - # - # The container name still carries the run id (`postgres-${GITHUB_RUN_ID}`) - # even though the lock above already keeps `test` jobs from overlapping: - # defence in depth for the window where another branch is still on the - # pre-lock version of this file (its `test` job takes no lock and would - # collide on a bare `--name postgres`), and for a mis-fired stale-lock - # release. The `if: always()` teardown plus the age-gated reap below - # keep these per-run containers from piling up on the runner host. + # why. Reachable as `postgres` from this job (the runner mounts the + # docker socket and runs jobs on a shared user-defined network, so a + # `--name postgres` sibling resolves by name); the network is + # discovered from the job container itself rather than hardcoded. The + # `docker rm -f postgres` below also clears a container left behind by + # the previous run (this runner has no reliable post-job cleanup), and + # the serialization step above means no other run is using it now. - name: Start Postgres in the background run: | - pg_host="postgres-${GITHUB_RUN_ID}" - - # Best-effort reap of per-run Postgres containers left behind by a - # `test` job that was force-killed before its teardown step could - # run (SIGKILL skips `if: always()` steps). Age-gated well above - # this job's `timeout-minutes`, so a slower *concurrent* `test` - # job's container is never in range. - reap_before=$(( $(date +%s) - 7200 )) - for c in $(docker ps -aq --filter "name=postgres-"); do - started="$(docker inspect -f '{{.State.StartedAt}}' "$c" 2>/dev/null)" || continue - started_epoch="$(date -d "$started" +%s 2>/dev/null)" || continue - if [ "$started_epoch" -lt "$reap_before" ]; then - echo "reaping abandoned Postgres container $c (started $started)" - docker rm -f "$c" >/dev/null 2>&1 || true - fi - done - net="$(docker inspect "$(hostname)" \ --format '{{range $k, $_ := .NetworkSettings.Networks}}{{$k}} {{end}}' \ | awk '{print $1}')" echo "job network: ${net:?could not determine the job container network}" - docker rm -f "$pg_host" >/dev/null 2>&1 || true - docker run -d --name "$pg_host" --network "$net" \ + docker rm -f postgres >/dev/null 2>&1 || true + docker run -d --name postgres --network "$net" \ -e POSTGRES_USER=ci -e POSTGRES_PASSWORD=ci -e POSTGRES_DB=batchcooking_ci \ postgres:16-alpine - - # Hand this run's hostname to every later step (prisma migrate - # deploy, pnpm --filter api test) — this line overrides the - # workflow-level env.DATABASE_URL, whose `postgres` host no longer - # resolves. - echo "DATABASE_URL=postgresql://ci:ci@${pg_host}:5432/batchcooking_ci?schema=public" >> "$GITHUB_ENV" - for _ in $(seq 1 60); do - docker exec "$pg_host" pg_isready -U ci -d batchcooking_ci && exit 0 + docker exec postgres pg_isready -U ci -d batchcooking_ci && exit 0 sleep 2 done echo "Postgres did not become ready in time" >&2 - docker logs "$pg_host" || true + docker logs postgres || true exit 1 - uses: https://github.com/actions/checkout@v4 @@ -246,28 +187,6 @@ jobs: - run: pnpm --filter api exec prisma migrate deploy - run: pnpm --filter api test - # Release the serialization lock as early as possible so the next - # queued `test` job can start — `always()` so a failed or timed-out job - # frees it too (the stale-lock sweep in "Acquire the test-job lock" - # only covers a job killed outright). Only if this job actually holds - # it: a job that gave up waiting must not remove the holder's lock. - - name: Release the test-job lock - if: always() - run: | - if [ -f /tmp/ci-test-lock-held ]; then - docker network rm ci-test-lock >/dev/null 2>&1 || true - fi - - # Remove this run's Postgres even when a step above failed or the job - # hit its `timeout-minutes` — with a per-run name nothing else ever - # reuses or cleans it, and a bare `docker run` sibling outlives the job - # container. `|| true`: teardown must never itself fail the job. A job - # killed outright (SIGKILL, no `always()` step) is caught by the - # age-gated reap in "Start Postgres" on a later run. - - name: Stop this run's Postgres - if: always() - run: docker rm -f "postgres-${GITHUB_RUN_ID}" >/dev/null 2>&1 || true - intent-service-test: runs-on: ubuntu-latest steps: