fix: cancel in progress has incorrect ordering with new in-memory index#4442
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
github_actions
abelanger5
changed the title
Contributor
|
|
Contributor
Benchmark resultsCompared against |
github-actions
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a bug in the V1 in-memory concurrency index where CANCEL_IN_PROGRESS behaved like CANCEL_NEWEST due to using the default (oldest-first) comparator. The PR introduces a strategy-specific comparator selection so CANCEL_IN_PROGRESS ranks newer equal-priority slots ahead of older ones, and adds both Go and Python test coverage to prevent regressions—especially when SERVER_CONCURRENCY_IN_MEMORY_INDEX_ENABLED=true.
Changes:
- Select a comparator based on concurrency strategy, using a newest-first tie-break for
CANCEL_IN_PROGRESS. - Add Go unit/integration tests that pin the
CANCEL_IN_PROGRESSrecency behavior under the in-memory index. - Add Python task-level concurrency examples + tests and run them in CI with the in-memory index toggled on/off.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sdks/python/examples/worker.py | Registers the new task-level concurrency example workflows so they can be exercised in the examples/test harness. |
| sdks/python/examples/concurrency_cancel_newest_task_level/worker.py | Adds a task-level CANCEL_NEWEST example to exercise the in-memory index path. |
| sdks/python/examples/concurrency_cancel_newest_task_level/test_concurrency_cancel_newest_task_level.py | Adds an async test asserting task-level CANCEL_NEWEST keeps the oldest run and cancels newer arrivals. |
| sdks/python/examples/concurrency_cancel_in_progress_task_level/worker.py | Adds a task-level CANCEL_IN_PROGRESS example to exercise the in-memory index path. |
| sdks/python/examples/concurrency_cancel_in_progress_task_level/test_concurrency_cancel_in_progress_task_level.py | Adds an async test asserting task-level CANCEL_IN_PROGRESS keeps the newest run and cancels older (including running) ones. |
| pkg/scheduling/v1/concurrency/strategy.go | Switches from a fixed comparator to comparatorForStrategy, aligning heap ordering with strategy semantics. |
| pkg/scheduling/v1/concurrency/strategy_test.go | Updates test strategy construction to use the same strategy-specific comparator selection. |
| pkg/scheduling/v1/concurrency/index.go | Refines priorityCompare docs and introduces cancelInProgressCompare (newest-first tie-break) to match legacy semantics. |
| pkg/scheduling/v1/concurrency/cancel_in_progress_test.go | Adds targeted unit tests to pin newest-wins behavior and preemption direction for equal-priority slots. |
| pkg/scheduling/v1/concurrency_integration_test.go | Updates the in-memory CANCEL_IN_PROGRESS integration test description to reflect newest-first behavior and point to unit tests for ordering. |
| .github/workflows/sdk-python.yml | Extends the SDK Python CI matrix to run with SERVER_CONCURRENCY_IN_MEMORY_INDEX_ENABLED both true/false and disambiguates uploaded artifact names. |
mrkaye97
approved these changes
Jul 16, 2026
| ## wait for the olap repo to catch up | ||
| await asyncio.sleep(5) | ||
|
|
||
| successful_run = hatchet.runs.get(to_run.workflow_run_id) |
Contributor
There was a problem hiding this comment.
it'll be less flaky to use await hatchet.runs.aio_get_details() instead - have been trying to do that everywhere (then don't need the sleep above this either)
| await asyncio.sleep(5) | ||
|
|
||
| runs = sorted( | ||
| hatchet.runs.list(additional_metadata={"test_run_id": test_run_id}).rows, |
Contributor
There was a problem hiding this comment.
Suggested change
| hatchet.runs.list(additional_metadata={"test_run_id": test_run_id}).rows, | |
| await hatchet.runs.aio_list(additional_metadata={"test_run_id": test_run_id}).rows, |
Contributor
|
|
Contributor
|
|
Contributor
|
|
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.
Description
When
SERVER_CONCURRENCY_IN_MEMORY_INDEX_ENABLED=trueis set (enabling the new experimental in-memory index), the cancel-in-progress concurrency strategy incorrectly acts like cancel-newest. This is due to the comparator method for the internal heap using the defaultpriorityCompareinstead of ranking newer slots as greater values in the heap, so they take precedence when popping out of the minHeap.Also adds a set of Python tests which use task-level concurrency for cancel-in-progress and cancel-newest, which correctly failed when setting
SERVER_CONCURRENCY_IN_MEMORY_INDEX_ENABLED.Type of change
What's Changed
CancelInProgressChecklist
Changes have been:
🤖 AI Disclosure