-
Notifications
You must be signed in to change notification settings - Fork 2
Background mail sender #334
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
Changes from all commits
8f493f6
8aa8df6
0856de9
30e7afd
ae00f33
dd86059
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,7 +1,7 @@ | ||||||||||||
| from datetime import datetime | ||||||||||||
| from typing import List, Union | ||||||||||||
|
|
||||||||||||
| from fastapi import APIRouter, Depends | ||||||||||||
| from fastapi import APIRouter, Depends, BackgroundTasks, HTTPException | ||||||||||||
|
|
||||||||||||
| from src.configuration.Settings import settings | ||||||||||||
| from src.error.AuthenticationException import AuthenticationException | ||||||||||||
|
|
@@ -20,6 +20,7 @@ | |||||||||||
| from src.utils.JWTBearer import JWTBearer | ||||||||||||
| from src.utils.service_utils import subtract_lists | ||||||||||||
| from src.utils.Token import AssistenceToken, BaseToken | ||||||||||||
| from src.utils.UserType import UserType | ||||||||||||
|
|
||||||||||||
| # from src.error.NotFoundException import NotFoundException | ||||||||||||
|
|
||||||||||||
|
|
@@ -380,20 +381,53 @@ def resend_accept_mail( | |||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @router.post("/{event_id}/send_slack_mail/") | ||||||||||||
| def send_slack_mail(event_id: int, slackUrl: str, token: BaseToken = Depends(JWTBearer())): | ||||||||||||
| event_service.send_slack_mail(event_id, slackUrl, token) | ||||||||||||
| return {"success": True} | ||||||||||||
| def send_slack_mail( | ||||||||||||
| event_id: int, | ||||||||||||
| slackUrl: str, | ||||||||||||
| background_tasks: BackgroundTasks, | ||||||||||||
| delay: float = 0.5, | ||||||||||||
| token: BaseToken = Depends(JWTBearer()), | ||||||||||||
| ): | ||||||||||||
| """ | ||||||||||||
| Schedule sending slack invite mails in background. The endpoint validates | ||||||||||||
| permissions and immediately returns while the work continues in background. | ||||||||||||
| """ | ||||||||||||
| if not token.check([UserType.LLEIDAHACKER]): | ||||||||||||
| raise AuthenticationException("Not authorized") | ||||||||||||
| # guard: don't schedule if there's already a sending job for this event | ||||||||||||
| if event_service.is_sending(event_id): | ||||||||||||
| raise HTTPException(status_code=409, detail="Sending already in progress for this event") | ||||||||||||
|
|
||||||||||||
| # schedule background task to avoid request timeout | ||||||||||||
| background_tasks.add_task(event_service.send_slack_mail_background, event_id, slackUrl, delay) | ||||||||||||
|
Comment on lines
+398
to
+402
|
||||||||||||
| return {"success": True, "scheduled": True} | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @router.get("/{event_id}/send_progress") | ||||||||||||
| def get_send_progress(event_id: int, token: BaseToken = Depends(JWTBearer())): | ||||||||||||
| """Return send progress for background jobs for an event.""" | ||||||||||||
| if not token.check([UserType.LLEIDAHACKER]): | ||||||||||||
| raise AuthenticationException("Not authorized") | ||||||||||||
| return event_service.get_send_progress(event_id) | ||||||||||||
|
|
||||||||||||
| @router.post("/{event_id}/send_reminder_mails/") | ||||||||||||
| def send_reminder_mails( | ||||||||||||
| event_id: int, | ||||||||||||
| background_tasks: BackgroundTasks, | ||||||||||||
| delay: float = 0.0, | ||||||||||||
|
||||||||||||
| token: BaseToken = Depends(JWTBearer()), | ||||||||||||
| ): | ||||||||||||
| """ | ||||||||||||
| Send reminder mails to all accepted hackers of an event | ||||||||||||
| Schedule sending reminder mails to all accepted hackers of an event. | ||||||||||||
| The endpoint validates permissions and immediately returns while the work | ||||||||||||
| continues in background. | ||||||||||||
| """ | ||||||||||||
| event_service.send_reminder_mails(event_id, token) | ||||||||||||
| return {"success": True} | ||||||||||||
| if not token.check([UserType.LLEIDAHACKER]): | ||||||||||||
| raise AuthenticationException("Not authorized") | ||||||||||||
|
||||||||||||
| raise AuthenticationException("Not authorized") | |
| raise AuthenticationException("Not authorized") | |
| # guard: don't schedule if there's already a sending job for this event | |
| if event_service.is_sending(event_id): | |
| raise HTTPException(status_code=409, detail="Sending already in progress for this event") |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,11 @@ | ||||||||||||||||
| from fastapi_sqlalchemy import db | ||||||||||||||||
| from sqlalchemy import desc | ||||||||||||||||
| from sqlalchemy import desc, create_engine | ||||||||||||||||
| from sqlalchemy.orm import sessionmaker | ||||||||||||||||
| from datetime import datetime | ||||||||||||||||
| import time | ||||||||||||||||
| import logging | ||||||||||||||||
|
||||||||||||||||
| import logging |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class-level mutable default values (_sending_jobs = {} and _sending_jobs_lock = threading.Lock()) are shared across all instances of EventService. This can lead to unexpected behavior if multiple instances are created, as they will all share the same job tracking state. Consider initializing these in __init__ method instead.
| _sending_jobs = {} | |
| _sending_jobs_lock = threading.Lock() | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| self._sending_jobs = {} | |
| self._sending_jobs_lock = threading.Lock() |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _sending_jobs dictionary grows unbounded as jobs are added but never removed. This can lead to a memory leak over time as finished jobs accumulate. Consider removing finished jobs after a certain period or implementing a maximum size limit with eviction of old entries.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a new database engine for every background mail send operation is inefficient and can lead to connection pool exhaustion. The engine should be created once (typically at application startup) and reused. Consider using dependency injection or accessing a shared engine instance from settings or a connection pool.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the event query fails (line 1077) or the event is archived/not found (line 1078-1079), the function returns early without calling _finish_job(). This leaves the job in a perpetually "running" state, blocking future sends for this event. Consider calling _finish_job() or removing the job entry in the early return paths, or defer job creation until after validation.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently catching all exceptions without logging can make debugging difficult. Consider logging the exception details (e.g., using logger.exception()) to track when and why mail creation or sending fails, while still allowing the process to continue for other recipients.
| except Exception: | |
| except Exception: | |
| logging.exception("Exception occurred while creating or sending slack invite mail to hacker (id: %s, email: %s)", getattr(hacker, 'id', 'unknown'), getattr(hacker, 'email', 'unknown')) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database session is closed in the finally block, but if an exception occurs during session.close(), the engine connections may not be properly disposed. Consider adding engine.dispose() in the finally block or using a context manager pattern to ensure proper cleanup of database connections.
| session.close() | |
| try: | |
| session.close() | |
| finally: | |
| engine.dispose() |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a new database engine for every background mail send operation is inefficient and can lead to connection pool exhaustion. The engine should be created once (typically at application startup) and reused. Consider using dependency injection or accessing a shared engine instance from settings or a connection pool.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the event query fails (line 1197) or the event is archived/not found (line 1198-1199), the function returns early without calling _finish_job(). This leaves the job in a perpetually "running" state, blocking future sends for this event. Consider calling _finish_job() or removing the job entry in the early return paths, or defer job creation until after validation.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| logging.exception("Failed to start reminder job for event_id %s", event_id) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently catching all exceptions without logging can make debugging difficult. Consider logging the exception details (e.g., using logger.exception()) to track when and why mail creation or sending fails, while still allowing the process to continue for other recipients.
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database session is closed in the finally block, but if an exception occurs during session.close(), the engine connections may not be properly disposed. Consider adding engine.dispose() in the finally block or using a context manager pattern to ensure proper cleanup of database connections.
| session.close() | |
| try: | |
| session.close() | |
| except Exception: | |
| pass | |
| engine.dispose() |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||
| from http import HTTPStatus | ||||||||||||
| from typing import Any | ||||||||||||
| import logging | ||||||||||||
| from generated_src.lleida_hack_mail_api_client.api.health import health_check | ||||||||||||
| from generated_src.lleida_hack_mail_api_client.api.mail import ( | ||||||||||||
| mail_create, | ||||||||||||
|
|
@@ -12,6 +13,8 @@ | |||||||||||
| from src.utils.Base.BaseClient import BaseClient | ||||||||||||
| from src.configuration.Settings import settings | ||||||||||||
|
|
||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def initialized(func): | ||||||||||||
| def wrapper(*args, **kwargs): | ||||||||||||
|
|
@@ -40,7 +43,7 @@ def __init__(self) -> Any: | |||||||||||
| self._initialized = True | ||||||||||||
| except Exception: | ||||||||||||
| self._initialized = False | ||||||||||||
| print("MailClient not initialized") | ||||||||||||
| logger.warning("MailClient not initialized") | ||||||||||||
| # raise MailClientException('MailClient is not available') | ||||||||||||
|
|
||||||||||||
| def check_health(self): | ||||||||||||
|
|
@@ -59,12 +62,22 @@ def create_mail(self, mail: MailCreate): | |||||||||||
| r = mail_create.sync(client=self.client, body=mail) | ||||||||||||
| if r is None: | ||||||||||||
| raise Exception(f"error creating {mail}") | ||||||||||||
| try: | ||||||||||||
| mail_id = getattr(r, "id", None) | ||||||||||||
| logger.info("Mail created id=%s receiver=%s subject=%s", mail_id, mail.receiver_mail, mail.subject) | ||||||||||||
| except Exception: | ||||||||||||
| logger.debug("Mail created (unable to log details)") | ||||||||||||
| return r | ||||||||||||
|
|
||||||||||||
| @initialized | ||||||||||||
| def send_mail_by_id(self, id: int): | ||||||||||||
| print('estem arrivant al send nanu') | ||||||||||||
| logger.info("Sending mail id=%s", id) | ||||||||||||
| r = mail_send_by_id.sync_detailed(id, client=self.client) | ||||||||||||
| status = getattr(r, "status_code", None) | ||||||||||||
| try: | ||||||||||||
| logger.info("Mail send result id=%s status=%s", id, status) | ||||||||||||
| except Exception: | ||||||||||||
| logger.debug("Mail send completed id=%s", id) | ||||||||||||
|
Comment on lines
+77
to
+80
|
||||||||||||
| try: | |
| logger.info("Mail send result id=%s status=%s", id, status) | |
| except Exception: | |
| logger.debug("Mail send completed id=%s", id) | |
| logger.info("Mail send result id=%s status=%s", id, status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation for the
delayparameter. Negative values could bypass the throttling sleep, and extremely large values could cause the background task to hang indefinitely. Consider validating thatdelayis within a reasonable range (e.g.,0 <= delay <= 60).