Take activity tests off one-second clocks - #1842
Draft
DABH wants to merge 4 commits into
Draft
Conversation
The test ran its three cancellation phases with a 1s workflow task timeout, a 1s heartbeat timeout on the regular activity and a 5s schedule-to-close on the local activity. Core only completes the workflow task holding a local activity at 80% of the task timeout, so the completion had 200ms of slack; every failing CI log starts with "Error reporting WFT to server" evictions from missing that. The eviction then either orphaned the local activity (shutdown hang, fixed separately), replayed the task into a stale local activity resolution (Core nondeterminism, queries rejected with "Workflow Task in failed state"), or simply left the phase without its expected query result for 10s. Widen the task timeout to 5s, give the local activity a timeout it never needs to hit, keep cancel delivery fast for the regular activity through the worker heartbeat throttle instead of a 1s heartbeat timeout, and make the shared completion event checks depend only on this phase's activity, with bounded waits so a missing completion fails instead of hanging.
test_manual_heartbeat and its siblings start a standalone activity with a
5s start-to-close timeout, then wait for the activity to report that it
started through an EventWorkflow before completing, cancelling, failing or
heartbeating it by ID. That handshake includes the first workflow task of a
cold worker, which on a loaded macOS runner took over 5s; the attempt timed
out and the heartbeat failed with RPCError NOT_FOUND ("activity not found
for ID"). Nothing in these tests depends on the attempt timing out, so give
the attempt a minute.
DABH
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The focused test-only changes consistently address the documented timing failures without introducing correctness issues.
Pull request overview
Improves activity-test reliability by replacing timing-sensitive short deadlines with safer bounds.
Changes:
- Extends workflow, activity, and heartbeat timeouts.
- Adds bounded cancellation waits and faster heartbeat delivery.
File summaries
| File | Description |
|---|---|
tests/worker/test_workflow.py |
Stabilizes activity-cancellation timing and waits. |
tests/test_activity.py |
Extends four manual activity deadlines. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
What was changed
test_workflow_cancel_activity: task timeout 1s to 5s (Core completes the task holding a local activity at 80% of it), local activity schedule-to-close 5s to 1 minute, heartbeat timeout 1s to 5s with a 300ms throttle, and one boundedwait_cancel_complete()helper per phase.tests/test_activity.py: the four manual activity tests use a 1-minute start-to-close timeout instead of 5s.Why
Nothing in these tests depends on the short bounds firing, and loaded runners regularly miss them. The 1s task timeout produced the
Error reporting WFT to serverevictions behind the most frequent macOS failure of the last two weeks, including the shutdown hang fixed in #1837. The manual tests' 5s bound closed the attempt before the start handshake finished.Testing
Cancel test 40/40 per parameter under load, 15/15 after the helper; manual tests 40/40 each. Lint clean.