Skip to content

Conversation

@giancarloromeo
Copy link
Contributor

@giancarloromeo giancarloromeo commented Sep 10, 2025

What do these changes do?

This PR fixes the Celery task cancellation mechanism by removing the dependency on AbortableTask and implementing a new cancellation approach based on task existence checking.

Key Changes:

  • Replaced AbortableTask with standard Celery Task across the codebase
  • Implemented task cancellation through task removal rather than abortion
  • Added exists_task method to check task existence for cancellation monitoring

Note

We get rid of AbortableTask, due to inconsistencies when used in combination of task_track_started=True option.

See: celery/celery#2335

Related issue/s

How to test

cd packages/celery-library
make tests

Dev-ops

No changes

@giancarloromeo giancarloromeo added this to the Cheops milestone Sep 10, 2025
@giancarloromeo giancarloromeo self-assigned this Sep 10, 2025
@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 80.85106% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.66%. Comparing base (763be96) to head (5d0bca6).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8354      +/-   ##
==========================================
- Coverage   87.81%   87.66%   -0.15%     
==========================================
  Files        1945     1529     -416     
  Lines       75557    63323   -12234     
  Branches     1313      686     -627     
==========================================
- Hits        66348    55512   -10836     
+ Misses       8813     7569    -1244     
+ Partials      396      242     -154     
Flag Coverage Δ
integrationtests 64.02% <ø> (+0.04%) ⬆️
unittests 86.06% <80.85%> (-0.42%) ⬇️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library 87.34% <85.00%> (-0.03%) ⬇️
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.11% <0.00%> (+<0.01%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.09% <ø> (+0.05%) ⬆️
agent 93.53% <ø> (ø)
api_server 91.91% <100.00%> (+<0.01%) ⬆️
autoscaling 95.77% <ø> (ø)
catalog 92.36% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.81% <ø> (-0.57%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 90.97% <ø> (+0.04%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.46% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.08% <ø> (-0.06%) ⬇️
storage 86.49% <ø> (+0.20%) ⬆️
webclient ∅ <ø> (∅)
webserver 87.96% <ø> (+0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 763be96...5d0bca6. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the Celery task cancellation mechanism by removing the dependency on AbortableTask and implementing a new cancellation approach based on task existence checking.

Key Changes:

  • Replaced AbortableTask with standard Celery Task across the codebase
  • Implemented task cancellation through task removal rather than abortion
  • Added exists_task method to check task existence for cancellation monitoring

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/service-library/src/servicelib/celery/task_manager.py Added exists_task method to the TaskManager protocol
packages/service-library/src/servicelib/celery/models.py Moved TASK_FINAL_STATES constant to module level and updated references
packages/celery-library/tests/unit/test_tasks.py Updated test function signature to use standard Task instead of AbortableTask
packages/celery-library/src/celery_library/task_manager.py Refactored cancellation logic to use task removal and added exists_task implementation
packages/celery-library/src/celery_library/task.py Replaced AbortableTask usage with standard Task and updated cancellation monitoring

@mergify
Copy link
Contributor

mergify bot commented Sep 10, 2025

🧪 CI Insights

Here's what we observed from your CI run for 5d0bca6.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI system-tests You had a 38% chance of failing… lucky you! 🎲 Flaky Configure an automatic retry View View
unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@giancarloromeo giancarloromeo added the a:apiserver api-server service label Sep 11, 2025
@giancarloromeo giancarloromeo changed the title 🐛 Fix Celery task cancellation mechanism 🐛 Fix inconsistent Celery task cancellation behavior Sep 11, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@giancarloromeo giancarloromeo changed the title 🐛 Fix inconsistent Celery task cancellation behavior 🐛 Ensure consistent Celery task cancellation Sep 11, 2025
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx. please check the exception handlers issue.

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx!

@sonarqubecloud
Copy link

@giancarloromeo giancarloromeo merged commit d0d210d into ITISFoundation:master Sep 12, 2025
94 of 95 checks passed
@giancarloromeo giancarloromeo deleted the is8334/fix-celery-task-abort-mechanism branch September 12, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Celery tasks abort monitor is not reliable

4 participants