Repository navigation
fix(ci): prevent runner shutdown on concurrent main pushes and restore path filters - #678
Merged
Merged
Conversation
…e path filters Two pushes to main within seconds caused a race condition with cancel-in-progress: true, where the ARC ephemeral runner received a shutdown signal mid-test (KeyboardInterrupt in worker.py:357). The runner was killed 10 minutes into a 20-minute timeout, well before the GitHub Actions timeout. Changes: - Set cancel-in-progress to only apply on pull_request events, not push to main. Push-to-main runs now always complete fully. - Restore detect-changes job with dorny/paths-filter that was removed in 7a8b0e2. Five filter categories: common, chat-completions, agentic, embeddings, go-bindings. - Wire path filters into all GPU E2E and vendor jobs so PRs only run tests relevant to the changed paths. Push to main and workflow_dispatch always run all tests. - Decouple e2e-2gpu-responses from e2e-1gpu-chat dependency so agentic-only changes still trigger responses tests. Job filter mapping: e2e-1gpu-chat: common || chat-completions e2e-1gpu-embeddings: common || embeddings e2e-1gpu-gateway: common || chat-completions e2e-2gpu-responses: common || agentic e2e-2gpu-pd: common || chat-completions e2e-vendor: common || agentic go-bindings-e2e: common || go-bindings benchmarks: always (no filter) Refs: https://github.com/lightseekorg/smg/actions/runs/22840166403 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
requested review from
CatherineSue,
XinyueZhang369 and
key4ng
as code owners
March 9, 2026 16:12
Contributor
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant PR as PR Trigger
participant DC as detect-changes Job
participant Build as build-wheel Job
participant E2E as E2E Test Jobs
PR->>DC: trigger (pull_request)
DC->>DC: analyze changed paths\n(set outputs: common, chat-completions, agentic, embeddings, go-bindings)
DC-->>E2E: expose category outputs
PR->>Build: trigger (always)
Build-->>E2E: build-wheel success
alt Event is PR
E2E->>E2E: check needs.detect-changes.result == "success"\nand any relevant output == 'true'
alt condition met
E2E->>E2E: run tests
else
E2E->>E2E: skip tests
end
else Non-PR event
E2E->>E2E: run tests (no detect-changes gating)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d0d894fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 347-400: The detect-changes paths-filter (id: filter) is missing
shared E2E entrypoints so PRs that edit them may skip relevant jobs; update the
filters block (the "common" filter in the detect-changes step) to include the
patterns '.github/workflows/e2e-gpu-job.yml', 'scripts/ci_install_e2e_deps.sh',
and 'scripts/ci_killall_sglang.sh' so changes to those files trigger the same
gated jobs (edit the filters under the common key in the detect-changes step to
add those three paths).
- Around line 508-516: The workflow's expensive 2-GPU job conditional is missing
the dependabot opt-out; update the job's if expression (the multiline condition
starting with always() && !cancelled() && needs.build-wheel.result == 'success'
...) to also require github.actor != 'dependabot[bot]' so Dependabot PRs don't
trigger this lane—mirror the same guard used by the sibling lanes like
e2e-1gpu-chat/e2e-1gpu-embeddings.
- Around line 529-535: The current if expression removes the implicit success()
guard (because it uses cancelled()), so add an explicit success check for the
upstream job e2e-1gpu-gateway to prevent this job running when that dependency
fails; update the conditional that includes cancelled(),
needs.detect-changes.result and needs.detect-changes.outputs to also require
needs.e2e-1gpu-gateway.result == 'success' (or success()) so the downstream job
only runs when e2e-1gpu-gateway succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96b24d81-d08d-4579-8c94-682ad1bf5baa
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
- Increase benchmark timeout from 20 to 30 minutes - Add missing common filter paths: e2e-gpu-job.yml, ci_install_e2e_deps.sh, ci_killall_sglang.sh so PRs touching shared E2E entrypoints trigger tests - Add dependabot[bot] guard to e2e-2gpu-responses to match sibling jobs - Add always() + explicit needs.e2e-1gpu-gateway.result == 'success' check to e2e-2gpu-pd so it runs on push events (when detect-changes is skipped) but not when gateway fails Refs: #678 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f98af1585
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+365
to
+369
| common: | ||
| - 'model_gateway/**' | ||
| - 'crates/protocols/**' | ||
| - 'bindings/**' | ||
| - 'e2e_test/conftest.py' |
There was a problem hiding this comment.
The restored detect-changes common filter only matches a subset of gateway sources, so PRs that touch core crates like crates/auth/**, crates/kv_index/**, crates/mesh/**, crates/wasm/**, or crates/workflow/** will produce all-false outputs and skip the GPU e2e jobs gated by common || .... This is a regression from always-running coverage: those crates are compiled into the wheel (see the hashFiles(...) key earlier in this workflow) and can change runtime behavior, so these PRs can go green without running the relevant integration suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 347-358: The detect-changes job is not included in the final
aggregation so failures in checkout or paths-filter can be masked; update the
workflow so the finish/aggregate job depends on the detect-changes job (add
"detect-changes" to the finish job's needs) and include its result in the
aggregate decision logic (use needs.detect-changes.result where the aggregate
computes success/failure or include its outputs in the common aggregation),
referencing the job name detect-changes and the finish/aggregate job to ensure
detect-changes failures prevent a green finish.
- Around line 364-402: The workflow's detect-changes filters in
.github/workflows/pr-test-rust.yml currently omit paths that are hashed into the
wheel (e.g., Cargo.toml and the crate/source trees referenced at line 121 such
as crates/auth/src/**, crates/kv_index/src/**, crates/mesh/src/**,
crates/wasm/src/**, crates/workflow/src/**, examples/wasm/**), so PRs touching
those files will rebuild the runtime but then skip gated E2E lanes; update the
filters block (the named groups like common, chat-completions, agentic, etc.) to
include those missing paths (add Cargo.toml and the listed crates/paths into the
appropriate filter group(s) or into the common group) so detect-changes will
trigger the wheel-dependent E2E/vendor lanes when those files change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 151a01b8-5e71-4379-b673-e005ca6aa829
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
Comment on lines
+347
to
+358
| detect-changes: | ||
| runs-on: ubuntu-latest | ||
| if: github.event_name == 'pull_request' | ||
| permissions: | ||
| contents: read | ||
| pull-requests: read | ||
| outputs: | ||
| common: ${{ steps.filter.outputs.common }} | ||
| chat-completions: ${{ steps.filter.outputs.chat-completions }} | ||
| agentic: ${{ steps.filter.outputs.agentic }} | ||
| embeddings: ${{ steps.filter.outputs.embeddings }} | ||
| go-bindings: ${{ steps.filter.outputs.go-bindings }} |
There was a problem hiding this comment.
Make detect-changes part of the aggregate result.
Every gated lane turns needs.detect-changes.result != 'success' into a skip, and finish does not depend on this job. If checkout or dorny/paths-filter fails, the run can still end with a green finish job even though all detect-changes-gated suites were suppressed.
🩹 Suggested patch
- finish:
- needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, unit-tests, benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-vendor, go-unit-tests, go-bindings-e2e]
+ finish:
+ needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, unit-tests, benchmarks, detect-changes, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-vendor, go-unit-tests, go-bindings-e2e]
@@
- "${{ needs.build-wheel.result }}" == "failure" || \
+ "${{ needs.build-wheel.result }}" == "failure" || \
+ "${{ needs.detect-changes.result }}" == "failure" || \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-test-rust.yml around lines 347 - 358, The
detect-changes job is not included in the final aggregation so failures in
checkout or paths-filter can be masked; update the workflow so the
finish/aggregate job depends on the detect-changes job (add "detect-changes" to
the finish job's needs) and include its result in the aggregate decision logic
(use needs.detect-changes.result where the aggregate computes success/failure or
include its outputs in the common aggregation), referencing the job name
detect-changes and the finish/aggregate job to ensure detect-changes failures
prevent a green finish.
Comment on lines
+364
to
+402
| filters: | | ||
| common: | ||
| - 'model_gateway/**' | ||
| - 'crates/protocols/**' | ||
| - 'bindings/**' | ||
| - 'e2e_test/conftest.py' | ||
| - 'e2e_test/infra/**' | ||
| - 'e2e_test/fixtures/**' | ||
| - 'Cargo.lock' | ||
| - '.github/actions/**' | ||
| - '.github/workflows/pr-test-rust.yml' | ||
| - '.github/workflows/e2e-gpu-job.yml' | ||
| - 'scripts/ci_setup_python_venv.sh' | ||
| - 'scripts/ci_install_sglang.sh' | ||
| - 'scripts/ci_install_e2e_deps.sh' | ||
| - 'scripts/ci_killall_sglang.sh' | ||
| - 'scripts/ci_build_wheel.sh' | ||
| - 'crates/tokenizer/**' | ||
| - 'crates/tool_parser/**' | ||
| chat-completions: | ||
| - 'crates/reasoning_parser/**' | ||
| - 'crates/multimodal/**' | ||
| - 'crates/grpc_client/**' | ||
| - 'grpc_servicer/**' | ||
| - 'e2e_test/chat_completions/**' | ||
| - 'e2e_test/router/**' | ||
| - 'scripts/ci_install_vllm.sh' | ||
| - 'scripts/ci_install_trtllm.sh' | ||
| agentic: | ||
| - 'crates/mcp/**' | ||
| - 'crates/data_connector/**' | ||
| - 'e2e_test/responses/**' | ||
| - 'e2e_test/messages/**' | ||
| - 'scripts/ci_agentic_svc_deps.sh' | ||
| - 'scripts/oracle_flyway/**' | ||
| embeddings: | ||
| - 'e2e_test/embeddings/**' | ||
| go-bindings: | ||
| - 'e2e_test/bindings_go/**' |
There was a problem hiding this comment.
Cover the remaining wheel inputs in detect-changes.
Line 121 still hashes Cargo.toml, crates/auth/src/**, crates/kv_index/src/**, crates/mesh/src/**, crates/wasm/src/**, crates/workflow/src/**, and examples/wasm/** into the wheel artifact, but none of the new filters match those paths. A PR that only touches one of them will rebuild the runtime under test and then skip every path-gated E2E/vendor lane.
🩹 Suggested patch
filters: |
common:
+ - 'Cargo.toml'
+ - 'crates/auth/**'
+ - 'crates/kv_index/**'
+ - 'crates/mesh/**'
- 'model_gateway/**'
- 'crates/protocols/**'
+ - 'crates/wasm/**'
+ - 'crates/workflow/**'
- 'bindings/**'
- 'e2e_test/conftest.py'
- 'e2e_test/infra/**'
- 'e2e_test/fixtures/**'
- 'Cargo.lock'
@@
- 'scripts/ci_build_wheel.sh'
- 'crates/tokenizer/**'
- 'crates/tool_parser/**'
+ - 'examples/wasm/**'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| filters: | | |
| common: | |
| - 'model_gateway/**' | |
| - 'crates/protocols/**' | |
| - 'bindings/**' | |
| - 'e2e_test/conftest.py' | |
| - 'e2e_test/infra/**' | |
| - 'e2e_test/fixtures/**' | |
| - 'Cargo.lock' | |
| - '.github/actions/**' | |
| - '.github/workflows/pr-test-rust.yml' | |
| - '.github/workflows/e2e-gpu-job.yml' | |
| - 'scripts/ci_setup_python_venv.sh' | |
| - 'scripts/ci_install_sglang.sh' | |
| - 'scripts/ci_install_e2e_deps.sh' | |
| - 'scripts/ci_killall_sglang.sh' | |
| - 'scripts/ci_build_wheel.sh' | |
| - 'crates/tokenizer/**' | |
| - 'crates/tool_parser/**' | |
| chat-completions: | |
| - 'crates/reasoning_parser/**' | |
| - 'crates/multimodal/**' | |
| - 'crates/grpc_client/**' | |
| - 'grpc_servicer/**' | |
| - 'e2e_test/chat_completions/**' | |
| - 'e2e_test/router/**' | |
| - 'scripts/ci_install_vllm.sh' | |
| - 'scripts/ci_install_trtllm.sh' | |
| agentic: | |
| - 'crates/mcp/**' | |
| - 'crates/data_connector/**' | |
| - 'e2e_test/responses/**' | |
| - 'e2e_test/messages/**' | |
| - 'scripts/ci_agentic_svc_deps.sh' | |
| - 'scripts/oracle_flyway/**' | |
| embeddings: | |
| - 'e2e_test/embeddings/**' | |
| go-bindings: | |
| - 'e2e_test/bindings_go/**' | |
| filters: | | |
| common: | |
| - 'Cargo.toml' | |
| - 'crates/auth/**' | |
| - 'crates/kv_index/**' | |
| - 'crates/mesh/**' | |
| - 'model_gateway/**' | |
| - 'crates/protocols/**' | |
| - 'crates/wasm/**' | |
| - 'crates/workflow/**' | |
| - 'bindings/**' | |
| - 'e2e_test/conftest.py' | |
| - 'e2e_test/infra/**' | |
| - 'e2e_test/fixtures/**' | |
| - 'Cargo.lock' | |
| - '.github/actions/**' | |
| - '.github/workflows/pr-test-rust.yml' | |
| - '.github/workflows/e2e-gpu-job.yml' | |
| - 'scripts/ci_setup_python_venv.sh' | |
| - 'scripts/ci_install_sglang.sh' | |
| - 'scripts/ci_install_e2e_deps.sh' | |
| - 'scripts/ci_killall_sglang.sh' | |
| - 'scripts/ci_build_wheel.sh' | |
| - 'crates/tokenizer/**' | |
| - 'crates/tool_parser/**' | |
| - 'examples/wasm/**' | |
| chat-completions: | |
| - 'crates/reasoning_parser/**' | |
| - 'crates/multimodal/**' | |
| - 'crates/grpc_client/**' | |
| - 'grpc_servicer/**' | |
| - 'e2e_test/chat_completions/**' | |
| - 'e2e_test/router/**' | |
| - 'scripts/ci_install_vllm.sh' | |
| - 'scripts/ci_install_trtllm.sh' | |
| agentic: | |
| - 'crates/mcp/**' | |
| - 'crates/data_connector/**' | |
| - 'e2e_test/responses/**' | |
| - 'e2e_test/messages/**' | |
| - 'scripts/ci_agentic_svc_deps.sh' | |
| - 'scripts/oracle_flyway/**' | |
| embeddings: | |
| - 'e2e_test/embeddings/**' | |
| go-bindings: | |
| - 'e2e_test/bindings_go/**' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-test-rust.yml around lines 364 - 402, The workflow's
detect-changes filters in .github/workflows/pr-test-rust.yml currently omit
paths that are hashed into the wheel (e.g., Cargo.toml and the crate/source
trees referenced at line 121 such as crates/auth/src/**, crates/kv_index/src/**,
crates/mesh/src/**, crates/wasm/src/**, crates/workflow/src/**,
examples/wasm/**), so PRs touching those files will rebuild the runtime but then
skip gated E2E lanes; update the filters block (the named groups like common,
chat-completions, agentic, etc.) to include those missing paths (add Cargo.toml
and the listed crates/paths into the appropriate filter group(s) or into the
common group) so detect-changes will trigger the wheel-dependent E2E/vendor
lanes when those files change.
slin1237
added a commit
that referenced
this pull request
Mar 9, 2026
- Increase benchmarks job timeout from 20 to 30 minutes - Add explicit 480s timeout for PD benchmark test which was hitting the default 240s limit — PD setup with 4 workers (2 prefill + 2 decode) needs more time than regular benchmarks Refs: #678 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
This was referenced Mar 10, 2026
This was referenced Mar 18, 2026
This was referenced Apr 8, 2026
7 tasks
This was referenced May 24, 2026
This was referenced Jun 11, 2026
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
1 participant
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.
Summary
Fixes flaky CI failures where ARC ephemeral runners receive shutdown signals during e2e tests, caused by
cancel-in-progress: trueracing between concurrent push-to-main runs. Also restores thedetect-changespath filter job that was removed in #643.Refs: https://github.com/lightseekorg/smg/actions/runs/22840166403/job/66245282176
What changed
1. Concurrency fix (
.github/workflows/pr-test-rust.yml)Changed
cancel-in-progressfromtrueto${{ github.event_name == 'pull_request' }}:Root cause: Two commits pushed to main 14 seconds apart shared the concurrency group
gateway-tests-refs/heads/main. The second run cancelled the first, but the ARC runner pod assigned to the new run received a late shutdown signal, causing aKeyboardInterruptinworker.py:357only 10 minutes into a 20-minute timeout.2. Restored
detect-changesjobAdded back
dorny/paths-filterwith five filter categories:common: core gateway, protocols, bindings, infra, CI scriptschat-completions: reasoning parser, multimodal, grpc, router tests, engine scriptsagentic: MCP, data connector, responses/messages tests, Oracle scriptsembeddings: embedding test filesgo-bindings: Go binding test files3. Wired path filters into GPU-heavy jobs
e2e-1gpu-chate2e-1gpu-embeddingse2e-1gpu-gatewaye2e-2gpu-responsese2e-2gpu-pde2e-vendorgo-bindings-e2ebenchmarksPush to main and
workflow_dispatchalways run all tests regardless of path filters.4. Decoupled
e2e-2gpu-responsesfrome2e-1gpu-chatChanged dependency from
needs: [e2e-1gpu-chat]toneeds: [build-wheel, detect-changes]so agentic-only changes still trigger responses tests (previously blocked when chat tests were skipped).Why
e2e-1gpu-chat (vllm)job to fail with aKeyboardInterruptduring model loading, despite all 46 tests passingdetect-changesjob was accidentally removed during the e2e test reorganization in refactor(e2e): remove parallel testing infrastructure and simplify architecture #643Test plan
e2e_test/responses/skips chat/embeddings/gateway jobs but runs vendor + responsesmodel_gateway/runs all e2e jobs (common filter matches)Summary by CodeRabbit
Chores