Skip to content

Keep monitoring side tasks after one returns cleanly - #1605

Open
Sanjays2402 wants to merge 1 commit into
procrastinate-org:mainfrom
Sanjays2402:fix/side-task-monitor-early-exit
Open

Keep monitoring side tasks after one returns cleanly#1605
Sanjays2402 wants to merge 1 commit into
procrastinate-org:mainfrom
Sanjays2402:fix/side-task-monitor-early-exit

Conversation

@Sanjays2402

@Sanjays2402 Sanjays2402 commented Jul 31, 2026

Copy link
Copy Markdown

Closes #1597

_monitor_side_tasks awaited asyncio.wait(..., FIRST_COMPLETED) once and only acted when a completed task had raised, so a side task that returned cleanly silently ended supervision of all the others. That is the normal path rather than an edge case: PeriodicDeferrer.worker() returns immediately when no periodic task is registered, so any app without periodic tasks loses side-task supervision at startup and a later listener/heartbeat failure no longer stops the worker. The monitor now loops over the remaining pending tasks until one fails or all have finished.

This change was prepared with AI assistance; the regression test was run locally and fails without the fix.

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

PR label(s):

Summary by CodeRabbit

  • Bug Fixes

    • Improved background task monitoring so all pending tasks are processed, including tasks that finish at different times.
    • Cancelled tasks no longer interrupt monitoring.
    • Failures continue to trigger worker shutdown and provide error details.
  • Tests

    • Added regression coverage for failures occurring after an earlier task completes.

_monitor_side_tasks awaited asyncio.wait(..., FIRST_COMPLETED) once and
only acted when a completed task had raised. A side task that returned
cleanly therefore ended supervision of every other side task for the rest
of the worker's life.

This is the normal path, not an edge case: PeriodicDeferrer.worker()
returns immediately when no periodic task is registered, so any app
without periodic tasks loses side-task supervision at startup and a later
listener or heartbeat failure no longer stops the worker.

The monitor now loops over the remaining pending tasks until one fails or
all of them have finished, skipping cancelled tasks.

Adds a regression test in tests/unit/test_worker.py asserting the worker
is stopped by a side task that fails after another one returned cleanly.

Closes procrastinate-org#1597
@Sanjays2402
Sanjays2402 requested a review from a team as a code owner July 31, 2026 10:18
@github-actions github-actions Bot added the PR type: bugfix 🕵️ Contains bug fix label Jul 31, 2026
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3dd77a3e-65eb-45a1-b13e-e6e554312b95

📥 Commits

Reviewing files that changed from the base of the PR and between d9cf91d and c0b9ec0.

📒 Files selected for processing (2)
  • procrastinate/worker.py
  • tests/unit/test_worker.py

📝 Walkthrough

Walkthrough

Changes

Side-task supervision

Layer / File(s) Summary
Iterative side-task monitoring and regression coverage
procrastinate/worker.py, tests/unit/test_worker.py
_monitor_side_tasks continues monitoring pending tasks after a task returns cleanly. Cancelled tasks are ignored. Failures still log details and stop the worker. The test verifies a later failure triggers worker shutdown.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: medihack

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: continued monitoring after a side task returns cleanly.
Linked Issues check ✅ Passed The implementation and regression test satisfy issue #1597 by preserving supervision and stopping the worker on a later side-task failure.
Out of Scope Changes check ✅ Passed The changes are limited to side-task monitoring and its regression test, which directly support issue #1597.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR type: bugfix 🕵️ Contains bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Side task supervision silently ends when a side task returns cleanly (which happens on every run with no periodic tasks)

1 participant