🧪 test_calc_job: fix daemon-restart flake - #7452
Open
GeigerJ2 wants to merge 2 commits into
Open
Conversation
`test_restart_after_daemon_reset` hand-rolled a polling loop with a 10-second budget covering the entire stop, restart, state reload, resume and finish cycle. That bound is too tight on a contended runner. Measured locally, the post-restart wait is ~3.2s on an idle machine but degrades smoothly under CPU contention, reaching ~9s at moderate load, near enough the bound to trip intermittently. The process always completed, so the failure is a timeout on a slow job rather than a lost or stuck one. Replace the loop with `submit_and_await`, the fixture the test already uses to await the `WAITING` state a few lines above. It polls identically but allows a longer budget and, on expiry, embeds the daemon log in the error rather than reporting a bare `current state: ProcessState.WAITING`, which carried no diagnostic. Measure the fixture's own timeout with `time.monotonic` instead of `time.time`. The latter is wall-clock and can jump under NTP correction, which is not what an elapsed-time bound wants. `brokers/zeromq/broker.py` already uses `monotonic` for its readiness deadline. This addresses the ZMQ timeout symptom observed for this test in issue separate race at task-publish time and is untouched here.
📝 WalkthroughWalkthroughThe process-awaiting fixtures now use monotonic clocks, floating-point timeouts, and explicit terminal-state handling. The daemon reset regression test replaces manual polling with ChangesProcess waiting and restart regression
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant RestartTest
participant SubmitAndAwait
participant ProcessNode
RestartTest->>SubmitAndAwait: await restarted process with timeout=30
SubmitAndAwait->>ProcessNode: poll process_state
ProcessNode-->>SubmitAndAwait: return current state
SubmitAndAwait-->>RestartTest: return on FINISHED or raise on failure/timeout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7452 +/- ##
==========================================
- Coverage 80.61% 80.06% -0.54%
==========================================
Files 580 580
Lines 46722 46730 +8
==========================================
- Hits 37658 37408 -250
- Misses 9064 9322 +258 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`test_restart_after_daemon_reset` hand-rolled a polling loop with a 10-second budget covering the entire stop, restart, state reload, resume and finish cycle. That bound is too tight on a contended runner. Measured locally, the post-restart wait is ~3.2s on an idle machine but degrades smoothly under CPU contention, reaching ~9s at moderate load, near enough the bound to trip intermittently. The process always completed, so the failure is a timeout on a slow job rather than a lost or stuck one. Replace the loop with `submit_and_await`, the fixture the test already uses to await the `WAITING` state a few lines above. It polls identically but allows a longer budget and, on expiry, embeds the daemon log in the error rather than reporting a bare `current state: ProcessState.WAITING`, which carried no diagnostic. The fixture has accepted a process node since well before this test was written, so the duplicated loop was never a workaround for a missing capability. Measure the fixture's own timeout with `time.monotonic` instead of `time.time`. The latter is wall-clock and can jump under NTP correction, which is not what an elapsed-time bound wants. `brokers/zeromq/broker.py` already uses `monotonic` for its readiness deadline. This addresses the ZMQ timeout symptom seen for this test. The RabbitMQ `UnroutableError` reported for it previously is a separate race at task-publish time and is untouched here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/tools/pytest_fixtures/daemon.py`:
- Line 120: Use the Sphinx-style :return: field consistently in both factory
docstrings: in src/aiida/tools/pytest_fixtures/daemon.py lines 120-120, rename
:returns: to :return:, and in src/aiida/manage/tests/pytest_fixtures.py lines
733-741, add the factory’s return description.
- Around line 159-162: Assign the excepted-process error text to msg before
raising RuntimeError in the daemon.py process-state wait logic, and apply the
same change in pytest_fixtures.py at the specified sibling site; preserve the
existing message content and non-excepted state handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2122316c-46ed-4601-ae98-7d5c8847b583
📒 Files selected for processing (3)
src/aiida/manage/tests/pytest_fixtures.pysrc/aiida/tools/pytest_fixtures/daemon.pytests/engine/processes/calcjobs/test_calc_job.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/engine/processes/calcjobs/test_calc_job.py
| :raises RuntimeError: If the process fails to achieve the specified state before the timeout expires. | ||
| :raises RuntimeError: If the process terminates in a state other than the one specified, or if it fails to | ||
| achieve the specified state before the timeout expires. | ||
| :returns `~aiida.orm.nodes.process.process.ProcessNode`: The process node. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use :return: consistently in both factory docstrings.
src/aiida/tools/pytest_fixtures/daemon.py#L120-L120: replace:returns:with:return:.src/aiida/manage/tests/pytest_fixtures.py#L733-L741: add the factory’s:return:field.
As per coding guidelines, use Sphinx-style docstrings with :param:, :return:, and :raises:.
📍 Affects 2 files
src/aiida/tools/pytest_fixtures/daemon.py#L120-L120(this comment)src/aiida/manage/tests/pytest_fixtures.py#L733-L741
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/tools/pytest_fixtures/daemon.py` at line 120, Use the Sphinx-style
:return: field consistently in both factory docstrings: in
src/aiida/tools/pytest_fixtures/daemon.py lines 120-120, rename :returns: to
:return:, and in src/aiida/manage/tests/pytest_fixtures.py lines 733-741, add
the factory’s return description.
Source: Coding guidelines
Comment on lines
+159
to
+162
| if current_state is ProcessState.EXCEPTED: | ||
| raise RuntimeError(f'The process excepted: {node.exception}') | ||
| msg = f'The process terminated in state `{current_state}` while waiting for state `{state}`.' | ||
| raise RuntimeError(msg) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assign the exception message before raising in both fixtures.
src/aiida/tools/pytest_fixtures/daemon.py#L159-L162: assign the excepted-process message tomsgbeforeraise RuntimeError(msg).src/aiida/manage/tests/pytest_fixtures.py#L766-L769: apply the same pattern.
As per coding guidelines, assign exception messages to a variable before raising.
📍 Affects 2 files
src/aiida/tools/pytest_fixtures/daemon.py#L159-L162(this comment)src/aiida/manage/tests/pytest_fixtures.py#L766-L769
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/tools/pytest_fixtures/daemon.py` around lines 159 - 162, Assign the
excepted-process error text to msg before raising RuntimeError in the daemon.py
process-state wait logic, and apply the same change in pytest_fixtures.py at the
specified sibling site; preserve the existing message content and non-excepted
state handling.
Source: Coding guidelines
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
2 participants
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 by CodeRabbit