fix: Fix race conditions in schedule_task API#46
Merged
Conversation
Add force parameter to schedule_task/register_task to prevent race conditions and schedule starvation when updating dynamic task instances. Changes: - Add force parameter (default False) to register_task across all backends - When force=False: fail if task is locked, preserve sooner schedule (MIN logic) - When force=True: override locks and use new schedule - Update task_spec while preserving execution timing (unless forced) This prevents two issues: 1. Race condition: Calling schedule_task while task is executing would clear locks, allowing duplicate execution 2. Schedule starvation: Repeated schedule_task calls would push execution time into the future indefinitely Add comprehensive integration tests covering all scenarios across all backends.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #46 +/- ##
==========================================
- Coverage 98.20% 97.98% -0.23%
==========================================
Files 26 26
Lines 2905 3124 +219
Branches 159 180 +21
==========================================
+ Hits 2853 3061 +208
- Misses 29 34 +5
- Partials 23 29 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Document the new force parameter and safe mode behavior for updating dynamic tasks, including race condition prevention and schedule starvation protection.
- Introduce TaskLockedException for clearer error handling when tasks are locked - Update all backends to use TaskLockedException instead of generic PyncetteException - Simplify documentation to match existing style and remove overstated language - Mention MIN logic briefly as preventing postponement rather than emphasizing starvation
There was a problem hiding this comment.
Pull Request Overview
Adds a force flag to scheduling APIs to eliminate race conditions and schedule starvation when updating dynamic task instances. Key changes:
- Introduce force parameter to schedule_task and repository.register_task across all backends.
- Implement lock-aware updates: when force=False, fail if locked and preserve the earlier execute_after; when force=True, override locks and use the new schedule.
- Add integration tests and docs; update CI and dev setup.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/fakerepository.py | Updates test fake repository interface to accept force. |
| tests/test_pyncette_integration.py | Adds comprehensive tests for lock handling, force updates, and schedule preservation. |
| tests/conftest.py | Isolates SQLite DB per pytest-xdist worker to avoid cross-test interference. |
| src/pyncette/sqlite.py | Implements lock-aware register_task with MIN schedule logic and optional force override. |
| src/pyncette/repository.py | Updates abstract interface and docs for register_task(force). |
| src/pyncette/redis/manage.lua | Adds REGISTER handling for force and locked cases; preserves earlier schedule when not forced. |
| src/pyncette/redis/init.py | Passes force and utc_now to Lua script; raises TaskLockedException on lock. |
| src/pyncette/pyncette.py | Exposes force in schedule_task and forwards to repository. |
| src/pyncette/prometheus.py | Plumbs force through the metrics wrapper. |
| src/pyncette/postgres.py | Implements lock-aware updates; removes upsert in favor of checked update/insert. |
| src/pyncette/mysql.py | Implements lock-aware updates; splits update vs. insert paths. |
| src/pyncette/errors.py | Introduces TaskLockedException with informative messages. |
| src/pyncette/dynamodb.py | Adds lock-aware register_task with CAS; extends _update_item options. |
| pyproject.toml | Moves dev deps from extras to dependency-groups. |
| docs/usage.md | Documents updating dynamic tasks, force behavior, and starvation prevention. |
| docs/contributing.md | Updates dev instructions; adds parallel test execution note. |
| README.md | Updates dev setup instructions. |
| .github/workflows/python-package.yml | Adjusts uv sync commands. |
| .github/workflows/docs.yml | Adjusts uv sync commands. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| force: If False, fails if task is locked. If True, overwrites everything including locks. | ||
|
|
||
| Raises: | ||
| PyncetteException: If force=False and task is currently locked |
There was a problem hiding this comment.
The raised exception is specifically TaskLockedException (a PyncetteException subclass). Update the docstring to name TaskLockedException for clarity and discoverability.
Suggested change
| PyncetteException: If force=False and task is currently locked | |
| TaskLockedException: If force=False and task is currently locked |
Prevent race conditions by locking the row during the check-then-update sequence in register_task. This ensures atomicity between checking the lock state and performing the update.
Simplify register_task by using INSERT ON CONFLICT/ON DUPLICATE KEY UPDATE for force=True, eliminating the need to SELECT first. This makes force mode truly unconditional and clarifies the two different code paths.
Reorganize REGISTER logic to have clearer separation: 1. Create if not exists 2. Force mode: unconditional overwrite 3. Safe mode: check locks, compute MIN, preserve locks This makes the three cases explicit and easier to understand.
Use READ COMMITTED instead of REPEATABLE READ (MySQL default) to eliminate gap lock deadlocks when using SELECT FOR UPDATE on non-existent rows. Benefits: - Eliminates gap lock-related deadlocks from concurrent inserts - Better concurrency and throughput for dynamic task registration - Consistent with Postgres behavior (READ COMMITTED default) - No phantom read prevention needed (each operation is independent) Set via init_command on connection pool to avoid per-transaction overhead.
Document force parameter, race condition fixes, performance improvements, and new TaskLockedException in unreleased section.
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.
Add force parameter to schedule_task/register_task to prevent race conditions and schedule starvation when updating dynamic task instances.
Changes:
This prevents two issues: