Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 23, 2025

What do these changes do?

This pull request introduces a new utility function, cancel_and_wait, to simplify and standardize the cancellation and cleanup of asyncio tasks. It also updates existing code to use this function in various services and adds corresponding unit tests to ensure its correctness.

  • Updated _cleanup_ctx_fun in multiple service files to use cancel_and_wait for task cancellation to solve sonarcloud warnings:
    • services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_api_keys.py
    • services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_core.py
    • services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_trash.py
    • services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_users.py
    • services/web/server/src/simcore_service_webserver/payments/_tasks.py
  • Update servicelib.cancel_wait_task

Related issue/s

  • Addresses sonarcloud issues: Ensure that the asyncio.CancelledError exception is re-raised after your cleanup code

How to test

cd packages/common-library
make install-dev
pytest -vv tests/test_async_tools.py

Dev-ops

NOne

@pcrespov pcrespov self-assigned this Jun 23, 2025
@pcrespov pcrespov changed the title ♻️ Improves task cancellation 🎨 ♻️ Improves task cancellation with new cancel_and_wait tool Jun 23, 2025
@pcrespov pcrespov changed the title 🎨 ♻️ Improves task cancellation with new cancel_and_wait tool 🎨 ♻️ Improves task cancellation with new cancel_and_wait utility Jun 23, 2025
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 23, 2025
@pcrespov pcrespov added this to the Engage milestone Jun 23, 2025
@pcrespov pcrespov marked this pull request as ready for review June 23, 2025 16:21
@pcrespov pcrespov enabled auto-merge (squash) June 23, 2025 16:21
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2025

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Jun 23, 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

Introduces a new cancel_and_wait utility to standardize asyncio task cancellation across services and replaces ad-hoc cancel/await logic. Also adds unit tests for the new utility.

  • Added cancel_and_wait implementation in common_library.async_tools
  • Updated multiple cleanup functions in payment and garbage-collector modules to use cancel_and_wait
  • Added tests for cancel_and_wait in test_async_tools.py

Reviewed Changes

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

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/payments/_tasks.py Replaced manual cancel/await with cancel_and_wait
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_users.py Ditto for user GC task
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_trash.py Ditto for trash GC task
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_core.py Ditto for core GC background task
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_api_keys.py Ditto for API-key GC task
packages/common-library/src/common_library/async_tools.py New cancel_and_wait function added
packages/common-library/tests/test_async_tools.py Tests for cancel_and_wait added
Comments suppressed due to low confidence (1)

packages/common-library/tests/test_async_tools.py:6

  • The tests reference asyncio (e.g., asyncio.sleep, asyncio.create_task) but don't import it. Add import asyncio at the top of the file.
from common_library.async_tools import cancel_and_wait, make_async, maybe_await

@codecov
Copy link

codecov bot commented Jun 23, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.62%. Comparing base (2d1134f) to head (764da33).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7956      +/-   ##
==========================================
- Coverage   87.83%   87.62%   -0.21%     
==========================================
  Files        1849     1451     -398     
  Lines       71343    59996   -11347     
  Branches     1250      705     -545     
==========================================
- Hits        62663    52572   -10091     
+ Misses       8318     7189    -1129     
+ Partials      362      235     -127     
Flag Coverage Δ
integrationtests 64.28% <90.90%> (+0.02%) ⬆️
unittests 85.95% <95.83%> (-0.48%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library 81.13% <100.00%> (ø)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.48% <100.00%> (-0.07%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (-0.06%) ⬇️
agent 96.29% <100.00%> (ø)
api_server 92.64% <100.00%> (ø)
autoscaling 96.03% <100.00%> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <100.00%> (ø)
dask_sidecar 92.35% <100.00%> (+0.56%) ⬆️
datcore_adapter 97.94% <ø> (ø)
director 76.73% <100.00%> (ø)
director_v2 91.14% <100.00%> (+0.12%) ⬆️
dynamic_scheduler 96.69% <100.00%> (ø)
dynamic_sidecar 90.09% <100.00%> (ø)
efs_guardian 89.65% <100.00%> (ø)
invitations 93.60% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.00% <100.00%> (-0.11%) ⬇️
storage 86.27% <100.00%> (ø)
webclient ∅ <ø> (∅)
webserver 87.64% <93.33%> (+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 2d1134f...764da33. 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
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.

Please replace the other one called cancel_wait_task thanks!

@pcrespov pcrespov requested a review from sanderegg June 23, 2025 16:33
@pcrespov pcrespov changed the title 🎨 ♻️ Improves task cancellation with new cancel_and_wait utility 🎨 📝♻️ Improves task cancellation with new cancel_and_wait utility Jun 23, 2025
@pcrespov pcrespov requested a review from eofli June 23, 2025 17:23
@pcrespov pcrespov changed the title 🎨 📝♻️ Improves task cancellation with new cancel_and_wait utility 🎨 ♻️ Improves task cancellation with new cancel_and_wait utility Jun 23, 2025
@pcrespov pcrespov removed the request for review from eofli June 23, 2025 17:23
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

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks

@pcrespov pcrespov force-pushed the mai/sonarcloud-task-cancellation branch 2 times, most recently from 70c353e to d502eaa Compare June 24, 2025 11:55
@pcrespov pcrespov force-pushed the mai/sonarcloud-task-cancellation branch from cfe5917 to cc1ffe1 Compare June 24, 2025 14:52
@pcrespov pcrespov force-pushed the mai/sonarcloud-task-cancellation branch from 7400200 to b715ab0 Compare June 24, 2025 16:04
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@sonarqubecloud
Copy link

@pcrespov pcrespov disabled auto-merge June 25, 2025 06:28
@pcrespov pcrespov merged commit 8bc8f3c into ITISFoundation:master Jun 25, 2025
97 checks passed
@pcrespov pcrespov deleted the mai/sonarcloud-task-cancellation branch June 25, 2025 06:28
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants