fix: use time.monotonic() in ResourceGovernor to prevent Python 3.12 CI hang#336
Conversation
….time mocks from fixtures Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
🤖 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSwitch ResourceGovernor timeout accounting to use time.monotonic() and remove global time.time mocking from integration fixtures so timeouts and safety mechanisms work reliably in CI, plus update the corresponding unit test. Sequence diagram for ResourceGovernor timeout enforcement with time.monotonicsequenceDiagram
actor Pytest
participant IntegrationTest
participant ResourceGovernor
participant time_time
participant time_monotonic
Pytest->>IntegrationTest: run test_full_pipeline_index_then_search
IntegrationTest->>ResourceGovernor: enter context
activate ResourceGovernor
ResourceGovernor->>time_monotonic: monotonic()
time_monotonic-->>ResourceGovernor: start_time
ResourceGovernor-->ResourceGovernor: _start_time = start_time
loop chunking loop
IntegrationTest->>ResourceGovernor: process next chunk
ResourceGovernor->>ResourceGovernor: _chunk_count++
ResourceGovernor->>ResourceGovernor: check_timeout()
ResourceGovernor->>time_monotonic: monotonic()
time_monotonic-->>ResourceGovernor: now
ResourceGovernor-->ResourceGovernor: elapsed = now - _start_time
ResourceGovernor->>ResourceGovernor: check_chunk_limit()
Note over time_time,Pytest: time.time may be globally mocked<br/>but time.monotonic is not affected
alt elapsed > settings.chunk_timeout_seconds or _chunk_count > settings.max_chunks_per_file
ResourceGovernor-->>IntegrationTest: raise ResourceLimitViolation
deactivate ResourceGovernor
end
end
Pytest-->>IntegrationTest: test completes without hanging
Updated class diagram for ResourceGovernor timeout accountingclassDiagram
class ResourceGovernorSettings {
float chunk_timeout_seconds
int max_chunks_per_file
}
class ResourceGovernor {
- float _start_time
- int _chunk_count
+ ResourceGovernor(ResourceGovernorSettings settings)
+ __enter__() ResourceGovernor
+ __exit__(exception_type, exception_value, traceback) bool
+ check_timeout() void
+ check_chunk_limit() void
}
ResourceGovernor --> ResourceGovernorSettings : uses
class time {
+ monotonic() float
}
ResourceGovernor ..> time : uses_monotonic_for_elapsed_time
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 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 found 1 issue, and left some high level feedback:
- Since
ResourceGovernornow depends ontime.monotonic, consider injecting a clock function (defaulting totime.monotonic) to make timeout behavior easier to unit test and to decouple it from the globaltimemodule. - You’ve removed global
time.timemocking from a few fixtures; it may be worth briefly scanning other integration/conftest helpers for any remaining time-based mocks that could similarly interfere with timeout/circuit-breaker behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `ResourceGovernor` now depends on `time.monotonic`, consider injecting a clock function (defaulting to `time.monotonic`) to make timeout behavior easier to unit test and to decouple it from the global `time` module.
- You’ve removed global `time.time` mocking from a few fixtures; it may be worth briefly scanning other integration/conftest helpers for any remaining time-based mocks that could similarly interfere with timeout/circuit-breaker behavior.
## Individual Comments
### Comment 1
<location path="src/codeweaver/engine/chunker/governance.py" line_range="159-161" />
<code_context>
actual_value=float(self._chunk_count),
extra_context={
- "elapsed_seconds": time.time() - self._start_time if self._start_time else 0
+ "elapsed_seconds": time.monotonic() - self._start_time if self._start_time else 0
},
)
</code_context>
<issue_to_address>
**suggestion:** Use an explicit None check for `_start_time` instead of relying on float truthiness.
Relying on `_start_time`’s truthiness is slightly brittle for a float and could behave unexpectedly if it’s ever `0.0` or another falsy value. Prefer an explicit `None` check instead:
```python
"elapsed_seconds": (
time.monotonic() - self._start_time
if self._start_time is not None
else 0
)
```
```suggestion
extra_context={
"elapsed_seconds": (
time.monotonic() - self._start_time
if self._start_time is not None
else 0
)
},
```
</issue_to_address>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
Updates chunking governance and integration test fixtures to prevent Python 3.12 CI hangs caused by global time.time() mocking interfering with timeout enforcement.
Changes:
- Switch
ResourceGovernorelapsed-time calculations fromtime.time()totime.monotonic(). - Remove global
time.time()patching from integration fixtures to avoid disabling timeouts/circuit breakers. - Update the governance unit test to mock
time.monotonic()instead oftime.time().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/codeweaver/engine/chunker/governance.py |
Use time.monotonic() for timeout/elapsed calculations in ResourceGovernor. |
tests/integration/conftest.py |
Remove global time.time() mocking from provider/indexing-related fixtures. |
tests/unit/engine/chunker/test_governance.py |
Update unit test mock to align with time.monotonic() usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| actual_value=float(self._chunk_count), | ||
| extra_context={ | ||
| "elapsed_seconds": time.time() - self._start_time if self._start_time else 0 | ||
| "elapsed_seconds": time.monotonic() - self._start_time if self._start_time else 0 |
There was a problem hiding this comment.
elapsed_seconds uses a truthiness check on self._start_time. If _start_time is 0.0 (possible in tests when time.monotonic() is mocked, and theoretically possible depending on the clock’s epoch), this will log 0 instead of the real elapsed time. Use an explicit is not None check (or guard earlier) so 0.0 is treated as a valid start time.
| "elapsed_seconds": time.monotonic() - self._start_time if self._start_time else 0 | |
| "elapsed_seconds": ( | |
| time.monotonic() - self._start_time | |
| if self._start_time is not None | |
| else 0 | |
| ) |
Code Review - PR #336SummaryThis PR addresses a critical CI hang issue in Python 3.12 tests by switching from ✅ Strengths
🔍 Observations
🎯 RecommendationsOptional Enhancement (not blocking): Security & Performance
ConclusionThis is a high-quality fix that addresses a real production issue with the correct solution. The use of Recommendation: Approve ✓ The changes are correct, well-tested, and follow Python best practices for time measurement. The CI tests should now complete reliably across all Python versions. |
PR ReviewI've reviewed this PR and the changes look excellent overall. The fix correctly addresses the root cause of Python 3.12 CI hangs by switching from Summary of Changes✅ Correct approach: Using
✅ Root cause fix: Removed harmful global
✅ Test update: Correctly updated the unit test to mock Code QualityThe implementation is clean and follows best practices:
Addressing the Review CommentRegarding the comment on The suggested change is technically correct but has zero practical impact: # Current (potentially problematic if _start_time is 0.0)
"elapsed_seconds": time.monotonic() - self._start_time if self._start_time else 0
# Suggested (explicitly checks for None)
"elapsed_seconds": (
time.monotonic() - self._start_time
if self._start_time is not None
else 0
)Why this edge case is theoretical only:
My recommendation: The suggested change improves code clarity and defensive programming, making the intent explicit ( Overall AssessmentThis PR demonstrates excellent debugging work - identifying that global time mocks were defeating timeout enforcement and fixing it at the root cause level. The switch to Status: Awaiting CI test results (Python 3.12/3.13/3.14 tests are currently running). Once tests pass, I recommend approval with optional consideration of the truthiness check improvement suggested in the review comment. The core fix is sound and will resolve the Python 3.12 CI hangs. 🎯 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Notes on Review Comment (governance.py:160)
Regarding the truthiness check suggestion on elapsed_seconds:
The suggestion is correct and improves defensive programming. While time.monotonic() = 0.0 is extremely unlikely in production (would require system boot at the exact instant __enter__ is called), the explicit is not None check is better practice:
# Current (works but theoretically fragile)
"elapsed_seconds": time.monotonic() - self._start_time if self._start_time else 0
# Better (explicit None check)
"elapsed_seconds": (
time.monotonic() - self._start_time
if self._start_time is not None
else 0
)Benefits of the suggested change:
- Makes intent explicit:
None= inactive, any float (including 0.0) = valid timestamp - Handles edge case in tests where
time.monotonicmight be mocked to return0.0 - More Pythonic - explicit
is not Noneis clearer than truthy check
Recommendation: Apply this suggestion as a defensive programming improvement, even though it's not fixing a practical bug in the current code.
Overall, this PR is excellent work identifying and fixing the root cause of Python 3.12 CI hangs. The core solution (switching to time.monotonic() and removing harmful time mocks) is exactly right. Once the CI tests pass, this should be merged. 🚀
Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Python 3.12 CI consistently hangs at
test_full_pipeline_index_then_search(~19% progress) until the runner is killed. Root cause: three test fixtures globally mocktime.time(), which theResourceGovernoruses for timeout enforcement. The mock returns barely-incrementing values so timeouts never fire. Combined withtimeout_func_only = true(pytest-timeout skips fixture setup), any slow operation during indexing runs forever. Python 3.13/3.14 happen to be fast enough to complete; 3.12 is not.Changes
ResourceGovernor:time.time()→time.monotonic()for all elapsed-time calculations.monotonic()is the correct clock for interval measurement and is unaffected bytime.timemocks.Integration fixtures (
configured_providers,real_providers,indexed_test_project): Remove the globaltime.timemock entirely. It was added speculatively ("for deterministic behavior if needed") but defeated timeout enforcement, circuit breakers, and any othertime.time-based safety mechanism across the entire indexing pipeline.Governance unit test: Mock target updated from
time.timetotime.monotonic.Summary by Sourcery
Switch ResourceGovernor to use a monotonic clock for timeout tracking and remove brittle global time mocking in integration tests to prevent hangs and make timeout behavior reliable.
Bug Fixes:
Enhancements:
Tests: