Repository navigation
Run ci-ok on a GitHub-hosted runner - #1747
Conversation
ci-ok is one 2 s python3 check that runs alone after every other job, so any Depot host boot lands on the merge's critical path: 32 s mean, up to 155 s, in the 10-27 sample (#1745). Hosted ubuntu-24.04 jobs in this repo started in 2-3 s at p90 over 10-24..10-27 (n=27). The job's name, needs, if: always() and logic are unchanged, so the required check stays the same. A scheduling test pins the runner. Fixes #1745 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aePipa5qoK8iqhBVEbW
|
bugbot run Generated by Claude Code |
Code review on #1747: with a hard-coded hosted label, a hosted Linux backlog could only be undone by a PR that itself waits on ci-ok in the backed-up pool. vars.CI_OK_RUNNER restores a no-merge escape hatch, like DISABLE_DEPOT_RUNNERS for the other jobs. Use ubuntu-latest to match every other hosted Linux job, move the assertion into its own test, and drop dated measurements from the YAML comment. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeP2GXjchXdv7YRSbGe
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 800ff09. Configure here.
|
Burn-down agent: labeled Ready for review at head
Generated by Claude Code |
Final review briefWhat it does. Moves the required Risk: low. Only the runner of the aggregate job changes. Name, Look here
Verified
Open questions: none. Auto-merge is armed, so approving sends it straight to the merge queue. Generated by Claude Code |
Fixes #1745
Problem
ci-okruns one 2 spython3check overtoJSON(needs), alone, after every other CI job. It ran ondepot-ubuntu-24.04-4, so each run paid a Depot host boot right on the merge's critical path. From #1745 (n=21, 10-27 13:07–20:16 UTC): start gap (job start → first step) mean 32 s, p90 131 s, max 155 s; since 16:17 the mean was 75 s.Re-checked on the latest 4 runs before this change:
Change
.github/workflows/ci.yml, jobci-ok:runs-on: ${{ vars.CI_OK_RUNNER || 'ubuntu-latest' }}(GitHub-hosted by default) instead of the Depot/DISABLE_DEPOT_RUNNERSexpression. Setting the repo variableCI_OK_RUNNER(e.g.depot-ubuntu-24.04-4) moves it back without a merge. Name,if: always(),needs,timeout-minutesand the check are unchanged.scripts/tests/test_ci_scheduling.py: newtest_ci_ok_defaults_to_a_hosted_runnerpins thatruns-online.Hosted queue check (asked for in #1745, because #1372 once moved
ci-okoffubuntu-latestduring a 9–18 min hosted backlog)GitHub-hosted jobs in
merge-queue-fail-fast,merge-queue-janitorandpin-check, up to 3 runs per workflow per day, 10-24 → 10-27 (n=27): queue wait (created → started) p50 2 s, p90 2 s, max 3 s; start gap (started → first step) p50/p90/max 1 s. Well under the 10 s p90 bar. If hosted Linux ever backs up again, setCI_OK_RUNNER; no PR or merge needed.Expected saving
Measured result
This PR's CI run 40578824535 (head
e631e18, draft, so the heavy jobs skip as designed):ci-okjob ran on hostedubuntu-24.04(GitHub Actions 1002782885). Queued 2 s, first step 0 s after start, check took <1 s, whole job 3 s, result success.The merge_group saving shows only after merge (the queue runs the base branch's workflow plus this change). The profiler will verify it against the 10-27 baseline.
Where each test runs
No test moves.
ci-okkeeps its name andneeds, so the required check is the same.Risk
Low. Only the runner of the aggregate check changes. The workflow change takes effect on this PR's head commit; merge_group effects show after merge, and the profiler will verify them.
Validation:
python3 -m unittest discover -s scripts/tests(374 tests OK); YAML parses; zizmor findings onci.ymlidentical to main (59, 0 medium/high); actionlint 1.7.7 reports only pre-existing anchor/matrix findings, none inci-ok.Code review (
/code-review high, before leaving draft)Fixed in 800ff09: (1) a hard-coded hosted label meant a hosted backlog could only be undone by a PR that itself waits on
ci-ok, sovars.CI_OK_RUNNERis the escape hatch; (3)ubuntu-latestinstead ofubuntu-24.04, matching every other hosted Linux job; (4) the assertion now lives in its own test; (6) dated measurements dropped from the YAML comment.Not fixed: (2) hosted concurrency shared with the Windows/macOS legs under merge-queue peaks is a real unknown, but
CI_OK_RUNNERnow covers it and the profiler checks merge_group after merge; (5) moving other short critical-path jobs such asrelease-readinessoff Depot is out of scope for #1745 and would need its own measurement.https://claude.ai/code/session_011f9aePipa5qoK8iqhBVEbW
Note
Low Risk
Only the runner for the final aggregate status job changes; required-check logic and job dependencies are unchanged, with an optional repo variable to revert runner choice without a code change.
Overview
Moves the merge-blocking
ci-okaggregate job off Depot by default so the last step of every CI run no longer pays a Depot runner cold-start on the critical path (#1745).ci-oknow usesruns-on: ${{ vars.CI_OK_RUNNER || 'ubuntu-latest' }}instead of theDISABLE_DEPOT_RUNNERS/depot-ubuntu-24.04-4expression. The job name,needs,if: always(), and the short Python verdict step are unchanged; only the runner label changes. Workflow comments document thatCI_OK_RUNNER(e.g.depot-ubuntu-24.04-4) is the repo-variable escape hatch if hosted Linux queues up.scripts/tests/test_ci_scheduling.pyaddstest_ci_ok_defaults_to_a_hosted_runnerto lock in thatruns-online.Reviewed by Cursor Bugbot for commit 800ff09. Configure here.
Generated by Claude Code