Skip to content

ci(backend): stop merge queue ejecting healthy PRs on the 20m test timeout - #13780

Merged
ntindle merged 1 commit into
devfrom
ci/backend-test-timeout-merge-queue-ejections
Aug 6, 2026
Merged

ci(backend): stop merge queue ejecting healthy PRs on the 20m test timeout#13780
ntindle merged 1 commit into
devfrom
ci/backend-test-timeout-merge-queue-ejections

Conversation

@ntindle

@ntindle ntindle commented Aug 5, 2026

Copy link
Copy Markdown
Member

Why / What / How

Why. Roughly half of all dev merge-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:

PR enqueued ejected elapsed PR's own checks actual cause
#13434 02:55 03:12 ~17 min all green real test failures (credit suite)
#13434 04:07 (2nd) 04:29 ~22 min all green 20m job timeout
#13743 18:33 18:54 ~21 min all green 20m job timeout
#13575 19:05 19:26 ~21 min all green 20m job timeout

(A fifth ejection, #13764 at 03:21→03:42, was also the 20m timeout.)

What. The test job in platform-backend-ci.yml had timeout-minutes: 20, which sits below the job's real p95 runtime. GitHub reports a timeout-minutes kill as conclusion cancelled, not failure — which is why this was invisible when reading the merge-queue runs. .github/workflows/scripts/check_actions_status.py treats any conclusion outside success/skipped/neutral as a failure, so a timed-out test leg makes Check PR Status fail, 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):

failure | The job has exceeded the maximum execution time of 20m0s

Every timed-out leg died at 1217-1222s — exactly the 20m0s cap:

run merge group leg job total pytest step
31037832829 pr-13575 test (3.12) 1222s 674s (killed)
31035323338 pr-13743 test (3.12) 1219s killed
30974258262 pr-13434 test (3.12) 1217s 1049s (killed)
30972001003 pr-13764 test (3.11) 1218s 1054s (killed)

These were healthy runs killed mid-suite, not hangs — the pytest step was still actively emitting PASSED lines when the runner pulled the plug.

Sibling matrix legs in the same runs passed comfortably, which is what makes this look like a "flake":

  • run 31037832829: test (3.11) 956s ✅, test (3.13) 975s ✅, test (3.12) 1222s ❌
  • run 30972001003: test (3.12) 949s ✅, test (3.13) 898s ✅, test (3.11) 1218s ❌

Two independent variance sources push a leg over the line:

  1. The suite's own runtime. ~10.6k tests run seriallypytest-xdist is not a dependency, and the pytest invocation has no -n. Measured across 126 test legs: 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.
  2. Checkout. The test job is the only job using fetch-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):

event n p50 p90 max killed at 20m
pull_request 78 922s 978s 1218s 2 (2.6%)
merge_group 27 950s 1056s 1222s 2 (7.4%)
push 21 928s 954s 976s 0

merge_group carries the heaviest tail. It is also the most damaging place to fail: merge_group has no paths: 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 changed platform-backend-ci.yml and TESTING.md.

Before / after

The meaningful rate for a timeout-minutes change is the share of legs the cap kills, not a test pass rate:

legs exceeding the cap per-leg per enqueue (3-leg matrix)
Before (20m) 4 / 126 3.2% ~9.2%; on merge_group legs alone 7.4% → ~20.6%
After (35m) 0 / 126 0% 0%

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:

  • First failure: credit_concurrency_test.py::test_concurrent_spends_insufficient_balanceExpected 5 failures, got 4. One of 10 concurrent spend_credits coroutines raised something that was neither a success nor InsufficientBalanceError.
  • Then test_race_condition_exact_balanceValueError: User not found with ID: exact-balance-… for a user that had just been created successfully.
  • Then everything cascaded: ~8 minutes of 25P02 current transaction is aborted, commands ignored until end of transaction block across credit_concurrency_test.py, credit_integration_test.py, credit_metadata_test.py and credit_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.py opens no Prisma interactive transaction anywhere — _add_transaction runs a single autocommit query_raw CTE with SELECT … 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, test job:
    • timeout-minutes: 2035, with a comment recording the measured runtime distribution so it doesn't get tightened back into the failure zone.
    • Added filter: blob:none to the fetch-depth: 0 checkout. This is a blobless partial clone: every ref stays reachable (so the base-branch poetry.lock lookup in "Install Poetry" is unchanged) while the blobs for all other branches are never downloaded. If the lazy fetch ever fails, the existing ; true fallback 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:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • python3 -c "yaml.safe_load(...)" parses the workflow; jobs.test.timeout-minutes == 35 and the checkout with: block resolves to {fetch-depth: 0, filter: blob:none, submodules: true}
    • actionlint on the changed workflow reports 5 shellcheck findings — byte-identical to the count on dev, so no new lint issues are introduced (all 5 are pre-existing, on lines this PR does not touch)
    • Confirmed test is the only job referencing BASE_REF, so fetch-depth: 0 is load-bearing there and nowhere else — it is preserved, only made blobless
    • All pre-commit hooks pass on the commit
    • End-to-end confirmation that a merge_group run completes inside 35m and that the blobless checkout still resolves git show "origin/$BASE_BRANCH":./poetry.lock — this can only be observed on CI, and this PR's own merge_group run is the test

Note

Low Risk
Workflow-only timing and checkout tuning; no application code, secrets, or test behavior changes.

Overview
Raises the backend CI test job cap from 20m to 35m and documents why: serial ~10.6k-test runs often exceed 20m, GitHub marks timeouts as cancelled, and merge-queue Check PR Status treats that as failure—ejecting otherwise green PRs.

Adds filter: blob:none on the existing fetch-depth: 0 checkout so base-branch poetry.lock resolution 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:

leg result job total headroom to 35m checkout pytest
test (3.11) ✅ success 935s (15.6m) 19.4m 9s 795s
test (3.12) ✅ success 751s (12.5m) 22.5m 9s 624s
test (3.13) ✅ success 925s (15.4m) 19.6m 14s 773s

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 the Install Poetry step log:

Found Poetry version 2.2.1 in backend/poetry.lock
Found Poetry version 2.2.1 in backend/poetry.lock on dev
Using Poetry version 2.2.1

The lazy blob fetch resolves correctly and the base-branch comparison is unchanged.

…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>
@ntindle
ntindle requested a review from a team as a code owner August 5, 2026 20:43
@github-project-automation github-project-automation Bot moved this to 🆕 Needs initial review in AutoGPT development kanban Aug 5, 2026
@github-actions github-actions Bot added the size/m label Aug 5, 2026
@ntindle

ntindle commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

/review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The backend CI test job timeout increases from 20 to 35 minutes. Its checkout retains full history and uses blob:none filtering.

Changes

Backend CI

Layer / File(s) Summary
Backend test job configuration
.github/workflows/platform-backend-ci.yml
The test timeout increases to 35 minutes. Checkout retains full history and defers blob downloads with blob:none.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: pwuts

Poem

A rabbit checks the workflow stream,
Thirty-five minutes for the test job’s dream.
Full history stays close at hand,
Blobs arrive when commands demand.
CI hops onward, quick and bright.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the backend CI timeout change and its effect on merge-queue ejections.
Description check ✅ Passed The description directly explains the timeout increase, blobless checkout, evidence, validation, and scope of the changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/backend-test-timeout-merge-queue-ejections

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@autogpt-pr-reviewer

Copy link
Copy Markdown

Queued a review for PR #13780 at 2814a3a.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.53%. Comparing base (43e7631) to head (2814a3a).

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     
Flag Coverage Δ
platform-backend 83.61% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Platform Backend 83.61% <ø> (-0.01%) ⬇️
Platform Frontend 54.94% <ø> (ø)
AutoGPT Libs ∅ <ø> (∅)
Classic AutoGPT 28.43% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@autogpt-pr-reviewer autogpt-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📋 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

  1. Parallelize the backend suite (platform-backend-ci.yml:~452) — adopt pytest-xdist with -n auto (using --dist loadgroup to 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)
  2. Trim merge-queue-path overhead (:~452) — drop --cov-branch/coverage on merge_group (artifact isn't consumed there) or set COVERAGE_CORE=sysmon on 3.12 legs, and use -q instead of -s -vv on the non-debug path. (performance, testing — 2 specialists)
  3. Gate merge_group on changed paths (:~452) — a lightweight changes filter to short-circuit the full backend suite for PRs that can't touch the backend (GitHub lacks paths: on merge_group). (performance, product)
  4. Track the flaky credit-concurrency suite — the 25P02 transaction-poisoning cascade is a real test-isolation defect; file a tracking issue so the raised cap doesn't mask it. (testing, discussion)

🔵 Nits

  1. 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)
  2. 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.


UI Testing — Variant Results

✅ local: CI-only change (test-job timeout 20→35 + blobless checkout) independently verified via YAML parse, actionlint+shellcheck parity, and dependency checks — all claims hold; no runtime surface to exercise.

✅ hosted: CI-only workflow change (test-job timeout 20→35m + blobless checkout) verified line-by-line against the file; all claims match, safe fallback present, no defects found — final under-35m timing is inherently CI-only and author-acknowledged.

@github-project-automation github-project-automation Bot moved this from 🆕 Needs initial review to 👍🏼 Mergeable in AutoGPT development kanban Aug 5, 2026
@ntindle
ntindle added this pull request to the merge queue Aug 6, 2026
Merged via the queue into dev with commit 0b37533 Aug 6, 2026
38 checks passed
@ntindle
ntindle deleted the ci/backend-test-timeout-merge-queue-ejections branch August 6, 2026 03:51
@github-project-automation github-project-automation Bot moved this from 👍🏼 Mergeable to ✅ Done in AutoGPT development kanban Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

1 participant