ci(backend): stop merge queue ejecting healthy PRs on the 20m test timeout - #13780
Conversation
…t timeout The `test` job's `timeout-minutes: 20` sat below the job's real p95 runtime, so healthy, still-progressing runs were killed mid-suite. GitHub reports a `timeout-minutes` kill as conclusion `cancelled`, and `.github/workflows/scripts/check_actions_status.py` treats any non success/skipped/neutral conclusion as a failure. On `merge_group` that makes `Check PR Status` fail, which ejects the PR from the merge queue. Measured on the backend suite: ~10.6k tests run serially (no pytest-xdist), pytest step p50 ~787s / max ~1093s, plus 2-7min of container init, checkout, poetry install, prisma generate and migrations. Four observed ejections all died at 1217-1222s, i.e. exactly the 20m0s cap, with the annotation "The job has exceeded the maximum execution time of 20m0s". Raise the cap to 35m so it guards against a genuinely hung job rather than acting as a performance budget, and add `filter: blob:none` to the `test` job's `fetch-depth: 0` checkout - that checkout was measured at 429s vs 27s for a sibling matrix leg in the same run, and the blobless partial clone keeps every ref reachable for the base-branch poetry.lock lookup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
WalkthroughThe backend CI test job timeout increases from 20 to 35 minutes. Its checkout retains full history and uses ChangesBackend CI
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #13780 +/- ##
==========================================
- Coverage 77.53% 77.53% -0.01%
==========================================
Files 2843 2843
Lines 215190 215190
Branches 20559 20559
==========================================
- Hits 166841 166838 -3
- Misses 43824 43825 +1
- Partials 4525 4527 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
📋 Automated Review — PR #13780
PR #13780 — ci(backend): stop merge queue ejecting healthy PRs on the 20m test timeout
Author: ntindle | Files: 1
🎯 Verdict: APPROVE
PR Description Quality
✅ Has Why + What + How — the description quantifies the before/after (p50/max runtimes, 0/126 legs over the new cap), traces the root cause (20m cap below p95 → GitHub reports the kill as cancelled → merge queue ejects the PR), and includes an explicit "not fixed here" section that scopes out the serial-suite slowness and the credit-suite flake without scope creep. The one unchecked box (end-to-end merge_group confirmation) is structurally unresolvable before merge, not neglected.
What This PR Does
The backend test CI job had a 20-minute timeout-minutes cap that sat below the suite's real p95 runtime, so healthy green PRs were being killed mid-suite and reported as cancelled — which the merge queue treats as a failure and ejects them. This PR raises the cap to 35 minutes (a hang-guard well above p99, ~40% headroom over the worst realistic run) and adds filter: blob:none to the fetch-depth: 0 checkout to remove a large, high-variance blob-download cost (measured 429s vs 27s on a sibling leg) while keeping all refs reachable so the base-branch poetry.lock lookup still resolves.
Specialist Findings
🛡️ Security ✅ — Confirmed clean. The test job keeps permissions: contents: read (line 170–171), adds no new trigger, no untrusted-ref checkout, and no new ${{ }} interpolation. filter: blob:none is provenance-neutral (integrity still verified via the promisor remote); the longer timeout only extends the window before a hung job is force-killed. Blast radius: none introduced.
🏗️ Architecture ✅ — Correct semantics: timeout used as a hang-guard, not a perf budget; blobless partial clone is the right tool and preserves reachability with a graceful ; true fallback. Blast radius contained (test is the only fetch-depth: 0/BASE_REF consumer). Two comment-durability polish nits.
🔵 .github/workflows/platform-backend-ci.yml:175 and :262 — comments narrate the change (old 20m cap behavior / a single-run 429s-vs-27s measurement) rather than the standing invariant; will read as stale once merged.
⚡ Performance ✅ — The blobless checkout is a genuine, endorsed efficiency win. The timeout raise is the correct immediate mitigation. Root cause is deferred and out of scope: the ~10.6k-test suite runs serially with branch-coverage instrumentation and -s -vv.
🟡 Serial suite (no pytest-xdist/-n), --cov-branch on the merge-queue path, and -s -vv verbosity are the structural drivers of the tail — follow-ups, not blockers (line ~452).
🧪 Testing ✅ — Config-only change; no application or test code touched, so no coverage regression. The author's test plan (YAML assertions + actionlint parity + load-bearing-dependency check) is the correct validation surface. Flags the genuinely flaky credit-concurrency suite (test_concurrent_spends_insufficient_balance, transaction-poisoning cascade) as a real defect that should be tracked so the 35m cap doesn't hide it — correctly scoped out of this PR.
📖 Quality ✅ — Readability grade A. Minimal two-edit diff; the timeout-minutes: 35 magic value is justified by an inline comment recording the measured distribution and rationale. No dead code, no churn.
📦 Product ✅ — DX surface (contributors/maintainers), no end-user impact. Fixes the stated problem cleanly; the tradeoff (a genuinely hung job now burns 35m before failing) is deliberate and documented.
📬 Discussion ✅ — No open threads, no unaddressed feedback, MERGEABLE, no conflicts. Bot sweep clean (CodeRabbit 2× LGTM, Bugbot/Seer/CodeQL/Snyk ✅). A live test (3.12) leg passed in 12m31s — real-world confirmation the suite completes comfortably under the new cap. No human reviewer has engaged yet (REVIEW_REQUIRED).
🔎 QA ✅ — Independently reproduced every mechanical claim on commit 2814a3a: timeout-minutes(test) == 35; checkout with == {fetch-depth:0, filter:blob:none, submodules:true}; actionlint exit 0; actionlint+shellcheck yields exactly 5 findings, all on unchanged lines (337/346/412), disjoint from changed lines (179, 255–267); fetch-depth: 0 occurs once and BASE_REF is used only in the test job; ; true fallback intact. No runtime/UI surface to exercise — browser/API testing genuinely N/A.
🟡 Nice to Have
- Parallelize the backend suite (
platform-backend-ci.yml:~452) — adoptpytest-xdistwith-n auto(using--dist loadgroupto keep the DB-stateful credit/concurrency tests isolated) to cut wall-clock 3-6× and eliminate the tail, letting the cap return to a tighter value. Out of scope here. (performance, testing, architect — 3 specialists) - Trim merge-queue-path overhead (
:~452) — drop--cov-branch/coverage onmerge_group(artifact isn't consumed there) or setCOVERAGE_CORE=sysmonon 3.12 legs, and use-qinstead of-s -vvon the non-debug path. (performance, testing — 2 specialists) - Gate
merge_groupon changed paths (:~452) — a lightweightchangesfilter to short-circuit the full backend suite for PRs that can't touch the backend (GitHub lackspaths:onmerge_group). (performance, product) - Track the flaky credit-concurrency suite — the
25P02transaction-poisoning cascade is a real test-isolation defect; file a tracking issue so the raised cap doesn't mask it. (testing, discussion)
🔵 Nits
- Comment durability — timeout block (
.github/workflows/platform-backend-ci.yml:175) — narrates the old 20m cap's bug rather than the standing invariant; keep the distribution + "guard not budget, keep above p99" and drop the historical account. (architect, quality) - Comment durability — checkout block (
:262) — replace the single-run "429s vs 27s" measurement with the durable rationale (blobless avoids unbounded high-variance blob fetches while keeping refs reachable). Optionally date-stamp the runtime figures (as of 2026-08). (architect, quality)
Human Review Needed
NO — This is an isolated CI-config change (one timeout value + a checkout filter) with no touch to authentication, authorization, secret handling, or trust boundaries between services. It does not cross the security boundary, and every mechanical claim was independently verified. A human with CI-ownership context reviewing is welcome given the merge-queue blast radius, but it is not required by the security-boundary criterion.
Risk Assessment
Merge risk: LOW | Rollback: EASY (revert a single-file, two-line change)
CI Status
Local harness: ✅ 5/5 checks pass (frontend lint, backend lint, frontend typecheck, frontend unit tests, frontend build).
GitHub CI: per the discussion specialist, 19/21 resolved checks green with test (3.12) passing in 12m31s; test (3.11), test (3.13), and Check PR Status still running (not failing) at review time — final GitHub status should be confirmed green before merge.
Why / What / How
Why. Roughly half of all
devmerge-queue enqueues were ejecting PRs whose own checks were fully green, and the time-to-ejection clustered hard around 17-22 minutes. Observed live on 2026-08-04/05:(A fifth ejection, #13764 at 03:21→03:42, was also the 20m timeout.)
What. The
testjob inplatform-backend-ci.ymlhadtimeout-minutes: 20, which sits below the job's real p95 runtime. GitHub reports atimeout-minuteskill as conclusioncancelled, notfailure— which is why this was invisible when reading the merge-queue runs..github/workflows/scripts/check_actions_status.pytreats any conclusion outsidesuccess/skipped/neutralas a failure, so a timed-outtestleg makesCheck PR Statusfail, and GitHub ejects the PR from the merge queue.How. Raise the cap so it guards against a genuinely hung job instead of acting as a performance budget, and remove the single largest source of setup variance from the job.
Root-cause evidence
The three ~21-22 min ejections are all the same mechanism. GitHub's own annotation on the cancelled job (
check-runs/92414409798/annotations):Every timed-out leg died at 1217-1222s — exactly the 20m0s cap:
test (3.12)test (3.12)test (3.12)test (3.11)These were healthy runs killed mid-suite, not hangs — the pytest step was still actively emitting
PASSEDlines when the runner pulled the plug.Sibling matrix legs in the same runs passed comfortably, which is what makes this look like a "flake":
test (3.11)956s ✅,test (3.13)975s ✅,test (3.12)1222s ❌test (3.12)949s ✅,test (3.13)898s ✅,test (3.11)1218s ❌Two independent variance sources push a leg over the line:
pytest-xdistis not a dependency, and the pytest invocation has no-n. Measured across 126testlegs: pytest step p50 787s, max 1093s. Two of the four kills had entirely normal setup and were killed purely because pytest itself was still running at 1049s/1054s.testjob is the only job usingfetch-depth: 0(it needs base-branch refs for the poetry.lock version comparison in "Install Poetry"). On run 31037832829 that checkout took 429s on the leg that died, versus 27s and 49s on the two legs that passed — same commit, same run.Measured
test-leg duration distribution (126 legs):pull_requestmerge_grouppushmerge_groupcarries the heaviest tail. It is also the most damaging place to fail:merge_grouphas nopaths:filter (GitHub doesn't support one), so every merge group runs the full backend suite even for PRs that cannot touch the backend — #13434 only changedplatform-backend-ci.ymlandTESTING.md.Before / after
The meaningful rate for a
timeout-minuteschange is the share of legs the cap kills, not a test pass rate:20m)merge_grouplegs alone 7.4% → ~20.6%35m)No leg in the sample has ever come within 14 minutes of the new cap. The longest completed leg observed is 1218s (20.3m); the killed legs were truncated, but extrapolating from their pytest progress they would have landed at roughly 21-25m — still comfortably inside 35m, which retains hang detection while leaving ~40% headroom over the worst realistic run.
Not fixed here (separate issue)
The #13434 02:55 ejection was a genuinely different failure mode and is not addressed by this PR.
test (3.11)(job 92194348225) failed with 12 failures + 7 errors, all in the credit suite:credit_concurrency_test.py::test_concurrent_spends_insufficient_balance—Expected 5 failures, got 4. One of 10 concurrentspend_creditscoroutines raised something that was neither a success norInsufficientBalanceError.test_race_condition_exact_balance—ValueError: User not found with ID: exact-balance-…for a user that had just been created successfully.25P02 current transaction is aborted, commands ignored until end of transaction blockacrosscredit_concurrency_test.py,credit_integration_test.py,credit_metadata_test.pyandcredit_refund_test.py.I deliberately have not shipped a speculative fix for this. My initial hypothesis (a leaked interactive transaction in the spend path) was disproven:
credit.pyopens no Prisma interactive transaction anywhere —_add_transactionruns a single autocommitquery_rawCTE withSELECT … FOR UPDATE, so it structurally cannot leave a connection in an aborted state. The real poisoning vector is still open, and reproducing it needs the full stack (Postgres + 3-shard Redis cluster + RabbitMQ + ClamAV + FalkorDB), which I could not stand up in this environment. Fixing it on a guess risks introducing a new merge-queue failure mode, which is exactly the problem this PR exists to remove.Changes 🏗️
.github/workflows/platform-backend-ci.yml,testjob:timeout-minutes: 20→35, with a comment recording the measured runtime distribution so it doesn't get tightened back into the failure zone.filter: blob:noneto thefetch-depth: 0checkout. This is a blobless partial clone: every ref stays reachable (so the base-branchpoetry.locklookup in "Install Poetry" is unchanged) while the blobs for all other branches are never downloaded. If the lazy fetch ever fails, the existing; truefallback already degrades to the HEAD poetry version, so the worst case is benign.No configuration, service, port, secret or env changes. Behaviour of the tests themselves is unchanged.
Checklist 📋
For code changes:
python3 -c "yaml.safe_load(...)"parses the workflow;jobs.test.timeout-minutes == 35and the checkoutwith:block resolves to{fetch-depth: 0, filter: blob:none, submodules: true}actionlinton the changed workflow reports 5 shellcheck findings — byte-identical to the count ondev, so no new lint issues are introduced (all 5 are pre-existing, on lines this PR does not touch)testis the only job referencingBASE_REF, sofetch-depth: 0is load-bearing there and nowhere else — it is preserved, only made bloblesspre-commithooks pass on the commitmerge_grouprun completes inside 35m and that the blobless checkout still resolvesgit show "origin/$BASE_BRANCH":./poetry.lock— this can only be observed on CI, and this PR's ownmerge_grouprun is the testNote
Low Risk
Workflow-only timing and checkout tuning; no application code, secrets, or test behavior changes.
Overview
Raises the backend CI
testjob cap from 20m to 35m and documents why: serial ~10.6k-test runs often exceed 20m, GitHub marks timeouts ascancelled, and merge-queueCheck PR Statustreats that as failure—ejecting otherwise green PRs.Adds
filter: blob:noneon the existingfetch-depth: 0checkout so base-branchpoetry.lockresolution for Install Poetry stays the same while avoiding full blob downloads that sometimes stretched checkout to hundreds of seconds on one matrix leg.Reviewed by Cursor Bugbot for commit 2814a3a. Bugbot is set up for automated code reviews on this repo. Configure here.
CI verification (this PR's own run 31045429279)
All three legs green, and both changes behave as intended:
test (3.11)test (3.12)test (3.13)Checkout: 9s / 9s / 14s, against 27s / 49s / 429s on the pre-change baseline (run 31037832829) — the 429s outlier that blew the budget is gone.
The one real risk in the checkout change was whether a blobless clone could still resolve the base branch's
poetry.lock. Confirmed from theInstall Poetrystep log:The lazy blob fetch resolves correctly and the base-branch comparison is unchanged.