-
Notifications
You must be signed in to change notification settings - Fork 25
✨(tasks) switch from celery to dramatiq #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,10 @@ | ||
| """API ViewSet for Celery task status.""" | ||
| """API ViewSet for asynchronous task statuses.""" | ||
|
|
||
| import logging | ||
|
|
||
| from django.core.cache import cache | ||
| import dramatiq | ||
| from dramatiq.results import ResultFailure, ResultMissing, Results | ||
|
|
||
| from celery import states as celery_states | ||
| from celery.result import AsyncResult | ||
| from drf_spectacular.utils import ( | ||
| OpenApiExample, | ||
| extend_schema, | ||
|
|
@@ -17,16 +16,12 @@ | |
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
|
|
||
| from messages.celery_app import app as celery_app | ||
| from core.utils import get_task_progress, get_task_tracking | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| TASK_OWNER_CACHE_TTL = 86400 # 24 hours | ||
|
|
||
|
|
||
| def register_task_owner(task_id, user_id): | ||
| """Register the owner of a task for permission checks.""" | ||
| cache.set(f"task_owner:{task_id}", str(user_id), timeout=TASK_OWNER_CACHE_TTL) | ||
| TASK_STATES = ["PENDING", "SUCCESS", "FAILURE", "PROGRESS"] | ||
|
|
||
|
|
||
| @extend_schema( | ||
|
|
@@ -44,11 +39,13 @@ | |
| 200: inline_serializer( | ||
| name="TaskStatusResponse", | ||
| fields={ | ||
| "status": drf_serializers.ChoiceField( | ||
| choices=sorted({*celery_states.ALL_STATES, "PROGRESS"}) | ||
| ), | ||
| "status": drf_serializers.ChoiceField(choices=sorted(TASK_STATES)), | ||
| "result": drf_serializers.JSONField(allow_null=True), | ||
| "error": drf_serializers.CharField(allow_null=True), | ||
| # Present when status == "PROGRESS" | ||
| "progress": drf_serializers.IntegerField(required=False), | ||
| "message": drf_serializers.CharField(required=False, allow_blank=True), | ||
| "timestamp": drf_serializers.FloatField(required=False), | ||
| }, | ||
| ) | ||
| }, | ||
|
|
@@ -69,43 +66,69 @@ | |
| ], | ||
| ) | ||
| class TaskDetailView(APIView): | ||
| """View to retrieve the status of a Celery task.""" | ||
| """View to retrieve the status of a task.""" | ||
|
|
||
| permission_classes = [permissions.IsAuthenticated] | ||
|
|
||
| def get(self, request, task_id): | ||
| """Get the status of a Celery task.""" | ||
| owner_id = cache.get(f"task_owner:{task_id}") | ||
| if owner_id is None: | ||
| """Get the status of a task.""" | ||
| tracking = get_task_tracking(task_id) | ||
| if tracking is None: | ||
| raise PermissionDenied("Task not found or access expired.") | ||
| if str(request.user.id) != owner_id: | ||
| if str(request.user.id) != tracking["owner"]: | ||
| raise PermissionDenied("You do not have access to this task.") | ||
|
|
||
| task_result = AsyncResult(task_id, app=celery_app) | ||
|
|
||
| # By default unknown tasks will be in PENDING. There is no reliable | ||
| # way to check if a task exists or not with Celery. | ||
| # https://github.com/celery/celery/issues/3596#issuecomment-262102185 | ||
|
|
||
| # Prepare the response data | ||
| result_data = { | ||
| "status": task_result.status, | ||
| "result": None, | ||
| "error": None, | ||
| } | ||
|
|
||
| # If the result is a dict with status/result/error, unpack and propagate status | ||
| if isinstance(task_result.result, dict) and set(task_result.result.keys()) >= { | ||
| "status", | ||
| "result", | ||
| "error", | ||
| }: | ||
| result_data["status"] = task_result.result["status"] | ||
| result_data["result"] = task_result.result["result"] | ||
| result_data["error"] = task_result.result["error"] | ||
| else: | ||
| result_data["result"] = task_result.result | ||
| if task_result.state == "PROGRESS" and task_result.info: | ||
| result_data.update(task_result.info) | ||
|
|
||
| return Response(result_data) | ||
| # Try to fetch the result from dramatiq's native result backend | ||
| message = dramatiq.Message( | ||
| queue_name=tracking["queue_name"], | ||
| actor_name=tracking["actor_name"], | ||
| args=(), | ||
| kwargs={}, | ||
| options={}, | ||
| message_id=task_id, | ||
| ) | ||
| try: | ||
| result_data = message.get_result(block=False) | ||
| except ResultMissing: | ||
| result_data = None | ||
| except ResultFailure as exc: | ||
| return Response({ | ||
| "status": "FAILURE", | ||
| "result": None, | ||
| "error": str(exc), | ||
| }) | ||
|
Comment on lines
+90
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find the project's dependency file to check Dramatiq version
fd -e "requirements.txt" -e "pyproject.toml" -e "poetry.lock" -e "Pipfile" | head -20Repository: suitenumerique/messages Length of output: 49 🏁 Script executed: # Check the actual code at the specified location
cat -n src/backend/core/api/viewsets/task.py | sed -n '85,105p'Repository: suitenumerique/messages Length of output: 910 🏁 Script executed: # Search for any existing Sentry imports in the backend
rg "from sentry_sdk import|import sentry_sdk" src/backend/ -A 2Repository: suitenumerique/messages Length of output: 1588 🏁 Script executed: # Check if there's a logger already configured in this file
rg "import logging|logger = " src/backend/core/api/viewsets/task.py -A 2Repository: suitenumerique/messages Length of output: 140 🏁 Script executed: # Find dependency files
find . -name "requirements*.txt" -o -name "pyproject.toml" -o -name "setup.py" | grep -v __pycache__ | head -10Repository: suitenumerique/messages Length of output: 226 🏁 Script executed: # Check git history or setup files for Dramatiq version
rg "dramatiq" . --max-count=5 -A 2 | head -30Repository: suitenumerique/messages Length of output: 2047 🌐 Web query:
💡 Result: In Dramatiq 2.0.1,
So Sources: 🏁 Script executed: # Check for similar exception handling patterns in the codebase
rg "ResultFailure|ResultMissing" src/backend/ -B 3 -A 5Repository: suitenumerique/messages Length of output: 1355 🏁 Script executed: # Look for other places where exceptions are returned to clients in viewsets
rg "return Response.*str\(exc\)" src/backend/ -B 2 -A 2Repository: suitenumerique/messages Length of output: 49 🏁 Script executed: # Check the imports in the task viewset file
cat -n src/backend/core/api/viewsets/task.py | head -30Repository: suitenumerique/messages Length of output: 991 Capture exceptions internally and return a generic error message to clients.
Add the import and update the exception handler: Fiximport dramatiq
+from sentry_sdk import capture_exception except ResultFailure as exc:
+ logger.exception("Task %s failed", task_id)
+ capture_exception(exc)
return Response({
"status": "FAILURE",
"result": None,
- "error": str(exc),
+ "error": "Task execution failed.",
})🧰 Tools🪛 GitHub Check: CodeQL[warning] 95-99: Information exposure through an exception 🤖 Prompt for AI Agents |
||
|
|
||
| if result_data is not None: | ||
| response = {"status": "SUCCESS", "result": result_data, "error": None} | ||
| # If the result follows the {status, result, error} convention, unpack it | ||
| if ( | ||
| isinstance(result_data, dict) | ||
| and {"status", "result", "error"} <= result_data.keys() | ||
| ): | ||
| response["status"] = result_data["status"] | ||
| response["result"] = result_data["result"] | ||
| response["error"] = result_data["error"] | ||
| return Response(response) | ||
|
|
||
| # Check if we have progress data for this task | ||
| progress_data = get_task_progress(task_id) | ||
| if progress_data: | ||
| return Response( | ||
| { | ||
| "status": "PROGRESS", | ||
| "result": None, | ||
| "error": None, | ||
| "progress": progress_data.get("progress"), | ||
| "message": progress_data.get("metadata", {}).get("message"), | ||
| "timestamp": progress_data.get("timestamp"), | ||
| } | ||
|
Comment on lines
+113
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expose progress metadata in PROGRESS responses.
🔧 Proposed fix progress_data = get_task_progress(task_id)
if progress_data:
+ metadata = progress_data.get("metadata", {}) or {}
return Response(
{
"status": "PROGRESS",
- "result": None,
- "error": None,
+ "result": metadata or None,
+ "error": metadata.get("error"),
"progress": progress_data.get("progress"),
- "message": progress_data.get("metadata", {}).get("message"),
+ "message": metadata.get("message")
+ or metadata.get("result", {}).get("message_status"),
"timestamp": progress_data.get("timestamp"),
}
)🤖 Prompt for AI Agents |
||
| ) | ||
|
|
||
| # Default to pending when no result and no progress | ||
| return Response( | ||
| { | ||
| "status": "PENDING", | ||
| "result": None, | ||
| "error": None, | ||
| } | ||
| ) | ||
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Copilot Autofix
AI 3 months ago
In general, the fix is to avoid returning raw exception messages (or stack traces) directly to the client. Instead, log the detailed exception server-side and send the client a generic, non-sensitive error message or code while preserving the existing API contract where possible.
For this specific code, the best minimal fix is: in the
except ResultFailure as exc:block, log the exception (including the stack trace) using the existinglogger, and change the"error"field in the response to a generic message such as"Task failed"or"An internal error has occurred.". This keeps the response structure (status,result,error) and HTTP status code unchanged, so existing clients still receive"status": "FAILURE"and anerrorstring, but without leaking internal exception details.Concretely, in
src/backend/core/api/viewsets/task.py, in theTaskDetailView.getmethod, update theexcept ResultFailure as exc:clause starting at line 94. Add alogger.exception(...)call before returning the response, and change"error": str(exc)to a generic error string. No new imports are needed becauseloggingandloggerare already defined at the top of the file.