fix: prevent Python 3.12 CI hang by excluding real_providers tests and hardening timeouts#337
Conversation
- Set timeout_func_only=false in pyproject.toml so pytest timeout covers fixture setup/teardown (previously only test body was covered) - Add asyncio.wait_for(timeout=300) in indexed_test_project fixture to catch hangs during model loading or embedding generation - Add timeout-minutes: 20 to CI test job as safety net - Use signal timeout method in CI for all tests (can interrupt native code)" Agent-Logs-Url: https://github.com/knitli/codeweaver/sessions/f35eacec-3469-4488-98b4-b20056964e04
Reviewer's GuideImproves CI robustness against Python 3.12 hangs by expanding pytest timeouts to cover fixtures, adding an explicit async timeout around project indexing in an integration fixture, and tightening CI workflow behavior with a job-level timeout and signal-based pytest timeouts. Sequence diagram for fixture indexing with asyncio timeoutsequenceDiagram
participant Pytest
participant IndexedTestProjectFixture as indexed_test_project_fixture
participant Asyncio as asyncio
participant IndexProject as index_project
participant NativeCode as native_model_loading_and_indexing
Pytest->>IndexedTestProjectFixture: request indexed_test_project
IndexedTestProjectFixture->>Asyncio: wait_for(index_project, timeout=300)
Asyncio->>IndexProject: start indexing
IndexProject->>NativeCode: load_models_and_generate_embeddings
alt indexing completes before 300s
NativeCode-->>IndexProject: embeddings_generated
IndexProject-->>Asyncio: indexing_result
Asyncio-->>IndexedTestProjectFixture: result
IndexedTestProjectFixture-->>Pytest: provide indexed_test_project
Pytest->>Pytest: execute test body
else indexing exceeds 300s
Asyncio-->>IndexedTestProjectFixture: TimeoutError
IndexedTestProjectFixture-->>Pytest: raise fixture_timeout_error
Pytest->>Pytest: mark test as failed_due_to_fixture_timeout
end
Flow diagram for pytest timeout coverage before and after changeflowchart LR
subgraph Before
B_Start["start pytest"] --> B_Setup["run setup\n(fixtures)"]
B_Setup --> B_Call["run test call"]
B_Call --> B_Teardown["run teardown"]
B_Timeout["pytest-timeout\n(timeout-func-only true)"] --> B_Call
end
subgraph After
A_Start["start pytest"] --> A_Setup["run setup\n(fixtures)"]
A_Setup --> A_Call["run test call"]
A_Call --> A_Teardown["run teardown"]
A_Timeout["pytest-timeout\n(timeout-func-only false)\n(timeout-method signal)"] --> A_Setup
A_Timeout --> A_Call
A_Timeout --> A_Teardown
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @Copilot, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @Copilot, but I was unable to process your request. Please see the logs for more details. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the
indexed_test_projectfixture, consider catchingasyncio.TimeoutErrorrather than the baseTimeoutErrorso you don't inadvertently mask other timeout exceptions raised insideindex_project(). - You might want to centralize the 300s timeout configuration (e.g., via a pytest marker or constant) rather than hard-coding it in the fixture, so timeout behavior remains consistent and easier to tune alongside the global pytest-timeout settings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `indexed_test_project` fixture, consider catching `asyncio.TimeoutError` rather than the base `TimeoutError` so you don't inadvertently mask other timeout exceptions raised inside `index_project()`.
- You might want to centralize the 300s timeout configuration (e.g., via a pytest marker or constant) rather than hard-coding it in the fixture, so timeout behavior remains consistent and easier to tune alongside the global pytest-timeout settings.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR hardens the test/CI timeout strategy to prevent Python 3.12 integration runs from hanging indefinitely during fixture setup (notably during project indexing and model/embedding initialization).
Changes:
- Extend pytest-timeout coverage to include fixture setup/teardown (
timeout_func_only = false). - Add an explicit
asyncio.wait_for(..., timeout=300)guard aroundindex_project()inside theindexed_test_projectintegration fixture. - Add a job-level GitHub Actions timeout and force CI runs to use pytest-timeout’s
signalmethod.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/integration/conftest.py |
Adds an asyncio-level timeout guard around integration fixture indexing to avoid indefinite hangs. |
pyproject.toml |
Applies pytest timeout to the entire lifecycle (fixtures included) so setup/teardown hangs are caught. |
.github/workflows/_reusable-test.yml |
Adds a job timeout and enforces --timeout-method=signal for CI test execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ "${TEST_MARKERS}" == *"real_providers"* ]]; then | ||
| mise run test-cov -m "${TEST_MARKERS}" --timeout=900 --timeout-method=signal | ||
| else | ||
| mise run test-cov -m "${TEST_MARKERS}" | ||
| mise run test-cov -m "${TEST_MARKERS}" --timeout-method=signal | ||
| fi |
There was a problem hiding this comment.
--timeout-method=signal is not supported on Windows (pytest-timeout only supports the signal method on Unix-like platforms). Since this workflow is reused with runner-version: windows-latest (e.g. weekly-tests), this change will cause those runs to error. Consider conditionally using signal only on Linux/macOS and falling back to thread on Windows, or gating the step/args on ${{ runner.os }}/$RUNNER_OS.
There was a problem hiding this comment.
This is absolutely correct! Windows doesn't support SIGALRM, so the signal method will fail on Windows runners. The workflow needs to conditionally use signal on Linux/macOS and fall back to thread on Windows.
I've submitted a full review with a recommended fix that checks ${{ runner.os }} to select the appropriate timeout method. Thank you for catching this!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
This PR addresses a critical issue with Python 3.12 CI hangs through a multi-layered timeout approach. The solution is well-thought-out and the PR description provides excellent context about the root cause. However, there is a critical Windows compatibility issue that must be addressed before merging.
Critical Issue
Windows Compatibility Problem with --timeout-method=signal
The changes in .github/workflows/_reusable-test.yml now apply --timeout-method=signal to all test runs (lines 212, 214). However, the signal method is not supported on Windows because Windows doesn't support SIGALRM.
This workflow is used by weekly-tests.yml with runner-version: windows-latest (line 67), which means the weekly Windows tests will fail with an error like:
pytest: error: argument --timeout_method: invalid choice: 'signal' (choose from 'thread')
Recommended Fix
Conditionally apply the signal method based on the runner OS:
run: |
# Use signal timeout method on Unix-like platforms so SIGALRM can interrupt
# blocking C code (ONNX runtime inference, model loading).
# Windows doesn't support SIGALRM, so it falls back to thread method.
# Combined with timeout_func_only=false in pyproject.toml,
# this ensures fixture hangs (e.g. indexed_test_project) are
# caught and reported instead of running until job timeout.
if [[ "${{ runner.os }}" == "Windows" ]]; then
TIMEOUT_METHOD="thread"
else
TIMEOUT_METHOD="signal"
fi
if [[ "${TEST_MARKERS}" == *"real_providers"* ]]; then
mise run test-cov -m "${TEST_MARKERS}" --timeout=900 --timeout-method="${TIMEOUT_METHOD}"
else
mise run test-cov -m "${TEST_MARKERS}" --timeout-method="${TIMEOUT_METHOD}"
fiAlternatively, you could use the PowerShell-compatible approach or simply omit the --timeout-method flag on Windows (pytest-timeout defaults to thread on Windows automatically).
Positive Aspects
- Excellent root cause analysis - The PR description clearly explains the three compounding issues
- Multi-layered defense - Combining pytest configuration changes, fixture-level timeouts, and job-level timeouts is a robust approach
- Clear documentation - The code comments explain the reasoning well
- Appropriate use of asyncio.wait_for - The fixture-level timeout in
tests/integration/conftest.pyis a good Python-level safeguard with a clear error message
Additional Observations
-
pyproject.toml change (timeout_func_only=false) - This is correct and necessary. The previous setting left fixtures uncovered, which was the root cause of undetected hangs.
-
Job-level timeout (20 minutes) - This is a sensible safety net. The comment mentions tests typically take 4.5-12 minutes, so 20 minutes provides adequate buffer.
-
asyncio.wait_for usage - The 300-second (5-minute) timeout in the fixture is reasonable for indexing operations.
-
Import placement - Good that the second commit fixed the asyncio import to be at the top level (line 11 in conftest.py).
Test Coverage
Please verify that the fix has been tested or will be tested on:
- ✅ Linux (currently running)
⚠️ Windows (will fail without the OS-conditional fix)⚠️ macOS (should work with signal method)
Recommendation
Request Changes - The Windows compatibility issue must be fixed before merging. Once the OS-conditional timeout method is implemented, this will be a solid fix for the Python 3.12 hang issues.
|
@copilot While this is a step in the right direction, I note that 3.12 tests are still failing in CI. Also, please incorporate this feedback |
…onditional - Add `not real_providers` to CI test markers — these heavy tests with full model loading/indexing should only run in nightly builds, not on every push. This was the actual root cause of the 3.12 hang. - Make `--timeout-method=signal` conditional on $RUNNER_OS to avoid breaking Windows weekly tests (signal not supported on Windows). - Revert accidental mise.toml hk version change." Agent-Logs-Url: https://github.com/knitli/codeweaver/sessions/90b7c085-c5bb-419a-bb95-a2a4c8abb04d
Fixed in b9e0812. The primary issue was that Also addressed the Windows feedback: Additionally reverted the accidental |
…d hardening timeouts (#337) * fix: prevent Python 3.12 CI hang with timeout coverage for fixtures - Set timeout_func_only=false in pyproject.toml so pytest timeout covers fixture setup/teardown (previously only test body was covered) - Add asyncio.wait_for(timeout=300) in indexed_test_project fixture to catch hangs during model loading or embedding generation - Add timeout-minutes: 20 to CI test job as safety net - Use signal timeout method in CI for all tests (can interrupt native code)" Agent-Logs-Url: https://github.com/knitli/codeweaver/sessions/f35eacec-3469-4488-98b4-b20056964e04 * fix: move asyncio import to top level, revert accidental mise.toml change Agent-Logs-Url: https://github.com/knitli/codeweaver/sessions/f35eacec-3469-4488-98b4-b20056964e04 * fix: exclude real_providers from CI markers, make signal timeout OS-conditional - Add `not real_providers` to CI test markers — these heavy tests with full model loading/indexing should only run in nightly builds, not on every push. This was the actual root cause of the 3.12 hang. - Make `--timeout-method=signal` conditional on $RUNNER_OS to avoid breaking Windows weekly tests (signal not supported on Windows). - Revert accidental mise.toml hk version change." Agent-Logs-Url: https://github.com/knitli/codeweaver/sessions/90b7c085-c5bb-419a-bb95-a2a4c8abb04d --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary
Fixes Python 3.12 CI test failures caused by
real_providerstests running in standard CI (they should only run in nightly builds) and hardens timeout handling as defense-in-depth.Root Cause Analysis
The primary cause was that
real_providerstests (e.g.test_full_pipeline_index_then_search) were not excluded from CI test markers. These tests use theindexed_test_projectfixture which performs full model loading and embedding generation through native code (ONNX runtime, tokenizers). On Python 3.12, this operation hangs — the CI runner consistently receives a "shutdown signal" after ~5 minutes.Three compounding issues prevented detection:
real_providerstests not excluded from CI markers — heavy tests requiring full model loading/indexing were running on every push instead of only in nightly buildstimeout_func_only = truein pyproject.toml meant pytest-timeout only covered the test body, NOT fixture setup/teardown — so the fixture hang ran uncheckedtimeout_method = "thread"(the default) can't interrupt native C code (ONNX inference, model loading) — even if the timeout fired, it couldn't kill the stuck operationChanges
.github/workflows/ci.yml
not real_providersto CI test markers — these heavy tests should only run in nightly builds, not on every pushpyproject.toml
timeout_func_only = falseso pytest timeout covers the entire test lifecycle (setup + call + teardown)tests/integration/conftest.py
index_project()inasyncio.wait_for(timeout=300)with a clear error message, providing a Python-level timeout for the fixture's async indexing operation.github/workflows/_reusable-test.yml
timeout-minutes: 20as a job-level safety netsignaltimeout method (--timeout-method=signal) conditionally on Linux/macOS only —signalcan interrupt native code via SIGALRM but is not supported on Windows (the reusable workflow is also used by weekly Windows tests)Context
real_providerstests are designed for nightly runs (nightly-tests.yml) where model downloads are pre-warmed and longer timeouts are expected