Repository navigation
Run ci-ok on a GitHub-hosted runner - #1747
Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
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
Collaborator
Author
|
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
Mikola Lysenko (mikolalysenko)
marked this pull request as ready for review
October 27, 2026 22:14
Collaborator
Author
|
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.
Collaborator
Author
|
Burn-down agent: labeled Ready for review at head
Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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