Correct the e2e contention diagnosis for task 260 (no fix shipped yet) #205
Loading…
Reference in a new issue
No description provided.
Delete branch "serialise-e2e-ci"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR no longer ships a fix. The reviewer's
Verdict: changes neededwas right on both counts, verified independently rather than accepted from either side — see the throwaway probes on (now-deleted) branchconcurrency-probe/ PR #208 for the raw evidence, summarised here and on task 260.1. Job-level
concurrencyis a no-op on this Forgejo instance. Confirmed empirically, not just from the docs: two throwaway jobs sharing an identical job-level group ran fully concurrently for over a minute (probe-a/probe-b, bothrunningcontinuously from 06:20:04 to past 06:21:14). The originalconcurrency:block undere2ehas been removed — shipping it would have looked like a fix while doing nothing, which is worse than shipping nothing. As a direct consequence,container-images's own job-levelconcurrencygroup (its CalVer-race guard, ADR-0148) has also never worked — recorded on task 260, not fixed here, since that's a separate concern.I also verified workflow-level
concurrencyis honoured here (two runs of a throwaway single-job workflow sharing a workflow-level group: the second satwaitingthrough the first's entire run phase and only started the instant the first reportedsuccess, while unrelated jobs from both runs ran concurrently in the same window — so it wasn't just runner-slot scarcity). But applying that toci.ymlas a whole would serialisebuildandcontainer-imagesacross every PR too, which this repo's routine 3-PR-parallel usage makes a materially bigger cost than task 260 asked to accept.2. The original "fixed ports" diagnosis was wrong. Two jobs on this runner are network- and filesystem-isolated from each other — proven directly: a job that bound an HTTP server to its own loopback on port 5169 (self-checking the bind itself succeeded) stayed unreachable from a concurrently running sibling job retrying for a full minute; a marker file one job wrote was never visible to a sibling job retrying for the same. A literal bind collision between two
e2ejobs is therefore not possible on this runner. The observed failures (#840/#841: Playwright timeouts, not bind errors; ~32min against a normal ~8) are far better explained by CPU/memory contention across the real subprocesses and Chromium eache2ejob runs — the same shape of contention ADR-0091/ADR-0092 already measured and sequencedbuild/e2ewithin one run to avoid, just not yet fixed across separate runs.ci.yml's own comment on thee2ejob is corrected accordingly, and ephemeral ports is now doubly rejected — not just costly to build, but pointless, since it doesn't touch the actual contended resource.What this PR now does: corrects the diagnosis in
ci.yml's own comment, drops the inertconcurrency:block, and replacesE2eJobConcurrencyTestswithE2eJobHasNoInertConcurrencyBlockTests— a regression guard against re-adding a job-level block that looks like protection and isn't (watched to redden by re-adding the exact block that was removed).What's left, and why it isn't here: the properly-scoped fix is a separate workflow file for
e2e, with its own workflow-levelconcurrencygroup, triggered byci.yml's completion (workflow_run) rather thanneeds—needsdoesn't cross workflow files, and this repo'se2ejob already re-clones and rebuilds independently ofbuild(no artefact-passing to redesign), but it's still a real change to how this pipeline is wired and untested on this runner's own quirks. That's larger than an "S" ticket. Recorded on task 260 with a request to either re-scope this ticket to the diagnosis correction (done here) or open a new ticket and allocate an ADR number for the workflow split — not self-allocated, per this repo's own convention.This states as fact that two
e2ejobs "contend for the same sockets." Butcontainer-images's own comment later in this file ("Discover this job's own container network") establishes that jobs on this runner already execute inside a per-job Docker container on a Forgejo Actions-managed network — i.e. each job already has its own network namespace, so a literal bind-collision onlocalhost:5169between two job containers shouldn't be possible. The observed symptom (#840/#841) was five Playwright timeouts, not a bind/EADDRINUSEerror — equally consistent with CPU contention across three concurrentdotnet runsubprocess sets slowing the app under test past the 30s timeout. Serialising the job would likely mask either cause, but if the real mechanism is resource contention rather than a literal port collision, that's worth stating correctly here — a future reader deciding whether ephemeral ports would fix this needs the right diagnosis, not just a fix that happens to work.This explains why a superseded commit of the same PR isn't cancelled, but doesn't state the larger cost of a single shared group: with three PRs routinely in flight (per task 260's own comment log), PR C's
e2ejob now waits behind PR A's and B's in strict FIFO order before it even starts — not just "doesn't get cancelled," but "doesn't start for 15-20+ minutes." Task 260 explicitly asked to prefer keepinge2econcurrent where workable, given this repo's routine 3-PR-parallel usage. Worth a sentence acknowledging the queueing cost across unrelated PRs, not only the narrower same-PR pre-emption cost.Forgejo's own docs (reference page,
concurrencysection) state plainly: "Multiple jobs within a workflow are not affected by theconcurrencysetting" —concurrencyis a workflow-level key only. This block is nested undere2e:, i.e. job-level, so Forgejo almost certainly ignores it entirely (the same way it already ignores job-levelpermissions:, percontainer-images's own comment further down this file). If so, this is a complete no-op: the YAML still parses, run #851 is genuinely green at job level, and nothing is actually serialised. The PR body's claim that Forgejo "follows GitHub's own concurrency semantics" for this, "confirmed against this file's owncontainer-imagesgroup," isn't independent confirmation —container-images's group is at the same (job) level and, on this reading, is equally likely to be a pre-existing no-op that has just never been tested under real contention. Before merging, either moveconcurrencyto the workflow's top level (accepting that it then also gatesbuild/container-images, which is a materially different design), or produce direct evidence — e.g. Forgejo source, or an actual three-way overlap that this PR's own evidence trail says hasn't been reproduced yet — that job-levelconcurrencyis honoured on this instance.@ -0,0 +12,4 @@/// the same pull request from an unrelated one — see the job's own comment in <c>ci.yml</c> for/// the full reasoning. Nothing compiles this rule; only reading <c>ci.yml</c> as text can./// </summary>public sealed class E2eJobConcurrencyTestsRe-ran these two regexes by hand against the real e2e block plus four mutations (cancel-in-progress reverted to true, group scoped per-PR, block commented out, block deleted) — all four correctly redden, and the
\s*(not.*) betweenconcurrency:/group:/cancel-in-progress:correctly refuses to match across a#comment prefix, so a comment merely mentioning the marker doesn't fool it. The extraction and regex design are solid. But given the job-level-concurrencyquestion raised on ci.yml: this proves the text says the right thing, not that Forgejo acts on it. Worth being explicit in the class doc that this is a text-shape guard only and cannot detect "technically present, semantically inert" — which is exactly the risk here.Verdict: changes needed
The fix places
concurrency:under thee2ejob (job-level). Forgejo's own docs are explicit: "Multiple jobs within a workflow are not affected by theconcurrencysetting" — it's a workflow-level key only. This block almost certainly parses and is silently ignored, exactly the failure mode task 260 worries about: CI stays green, nothing is actually serialised. The PR's claim that this is "confirmed against this file's owncontainer-imagesgroup" doesn't hold up — that group is at the same (job) level and is equally unverified; citing it as precedent isn't independent evidence. See inline comment on theconcurrency:block.Separately, worth resolving before this ships regardless of the above:
container-images's own comments establish jobs already run in per-job Docker containers with their own network namespace, which undercuts the literal "contend for the same sockets" diagnosis — the observed failures were timeouts, not bind errors, and are equally consistent with CPU contention. And the "cost accepted" reasoning forcancel-in-progress: falseonly covers same-PR pre-emption, not the larger cost that a shared group now queues every PR'se2ejob behind every other's — the opposite of what task 260 asked to be weighed, given three PRs routinely in flight.The tripwire itself (
E2eJobConcurrencyTests) is well-built for what it checks — re-derived by hand against four mutations (cancel-in-progress reverted, group scoped per-PR, block commented out, block deleted) and all four correctly redden. But it only proves the YAML text says the right thing; it can't tell a live setting from an inert one, which is the actual open question here.CI on this PR (#851, SHA
49cd013) is genuinely green at job level (build/e2e/container-images), independently re-checked — but that's expected whether or not the concurrency block does anything, since a solo run isn't contended either way. Nothing here disputes the honesty of the PR's own "does not prove a real overlap" disclosure; that's the right call. The blocker is the job-level placement itself.Serialise the e2e CI job to stop it contending for fixed portsto Correct the e2e contention diagnosis for task 260 (no fix shipped yet)Verdict: mergeable
Re-derived independently, not accepted on the author's say-so:
E2eJobHasNoInertConcurrencyBlockTests' regex against the actualci.ymland against a copy with the exact round-1 job-levelconcurrency:block (group: e2e-fixed-ports,cancel-in-progress: false) reinserted undere2e. Passes clean, reddens on the mutation. The extraction sanity check also correctly findsneeds: build/localhost:5169in both, so it isn't passing on an empty match.list_workflow_runs: cancelledpushruns onmainfrom a superseded merge are a routine, longstanding pattern here (8 instances back to 2026-08-11, long before anyconcurrency:block existed in this file), andci.ymlcarries no workflow-levelconcurrencykey at all. So that cancellation is some other, pre-existing Forgejo/act_runner behaviour, not evidence about job-levelconcurrency— it doesn't touch the mechanism the PR and the new test actually make claims about. The PR's wording throughout stays scoped to "an explicit job-levelconcurrency:key does nothing here," never to "nothing on this runner is ever superseded," so there's no overreach to correct.85c7b40,push/pull_requestfiltered byhead_sha— success, 13m12s, matching the SHA directly rather than by number.One non-blocking thought for whoever picks this up in task 271: the assertion's regex (
^ concurrency:\s*$) requires the line to end right after the colon, so a reintroduction with a trailing inline comment (concurrency: # scoped to this job) would slip past it. Given this project's own history with narrow source-text tripwires (ADR-0066), worth either loosening the regex or adding the same "best-effort, not exhaustive" disclaimerFieldErrorsValidatesOnSubmitOnlyTestscarries — not blocking, since the test still catches the one mistake it was written against and CLAUDE.md's own standard for these guards is "watched to redden," which this passes.No remedy shipping here is the right call, and it's stated plainly — title, PR body and the
ci.ymlcomment all agree nothing is serialised yet and point at task 271 for the real fix.