Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions src/impl/Event/router_v1.py
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
Expand All @@ -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

Expand Down Expand Up @@ -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,
Copy link

Copilot AI Nov 19, 2025

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 delay parameter. Negative values could bypass the throttling sleep, and extremely large values could cause the background task to hang indefinitely. Consider validating that delay is within a reasonable range (e.g., 0 <= delay <= 60).

Copilot uses AI. Check for mistakes.
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
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

There's a race condition between checking is_sending() (line 398) and starting the background task (line 402). If two requests arrive simultaneously, both could pass the is_sending() check before either starts the job, resulting in duplicate jobs. Consider using a lock or atomic check-and-set operation, or move the job tracking initialization into the router before scheduling the background task.

Copilot uses AI. Check for mistakes.
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,
Copy link

Copilot AI Nov 19, 2025

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 delay parameter. Negative values could cause unexpected behavior, and extremely large values could cause the background task to hang indefinitely. Consider validating that delay is within a reasonable range (e.g., 0 <= delay <= 60).

Copilot uses AI. Check for mistakes.
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")
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The send_reminder_mails endpoint is missing a guard to prevent concurrent sending jobs for the same event (similar to the guard at line 398 in send_slack_mail). This could result in duplicate reminder emails being sent if the endpoint is called multiple times.

Suggested change
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")

Copilot uses AI. Check for mistakes.

# schedule background task to avoid request timeout
background_tasks.add_task(event_service.send_reminder_mails_background, event_id, delay)
return {"success": True, "scheduled": True}


# @router.post("/{event_id}/send_remember")
Expand Down
211 changes: 206 additions & 5 deletions src/impl/Event/service.py
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
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

Import of 'logging' is not used.

Suggested change
import logging

Copilot uses AI. Check for mistakes.
import threading
from src.configuration.Settings import settings
from generated_src.lleida_hack_mail_api_client.models.mail_create import MailCreate
from collections import Counter

Expand Down Expand Up @@ -40,6 +45,65 @@ class EventService(BaseService):
hacker_service: HackerService = None
company_service: CompanyService = None
mail_client: MailClient = None
# background sending job tracking: event_id -> job info
_sending_jobs = {}
_sending_jobs_lock = threading.Lock()

Comment on lines +49 to +51
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
_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 uses AI. Check for mistakes.
def _start_job(self, event_id: int, job_type: str, total: int):
with self._sending_jobs_lock:
self._sending_jobs[event_id] = {
"type": job_type,
"start": datetime.now(),
"finish": None,
"total": int(total) if total is not None else 0,
"sent": 0,
"status": "running",
}
Comment on lines +52 to +61
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.

def _increment_sent(self, event_id: int):
with self._sending_jobs_lock:
job = self._sending_jobs.get(event_id)
if job and job.get("status") == "running":
job["sent"] = job.get("sent", 0) + 1

def _finish_job(self, event_id: int):
with self._sending_jobs_lock:
job = self._sending_jobs.get(event_id)
if job:
job["status"] = "finished"
job["finish"] = datetime.now()

def is_sending(self, event_id: int) -> bool:
with self._sending_jobs_lock:
job = self._sending_jobs.get(event_id)
return bool(job and job.get("status") == "running")

def get_send_progress(self, event_id: int):
with self._sending_jobs_lock:
job = self._sending_jobs.get(event_id)
if not job:
return {"running": False}
# copy minimal fields to compute without lock
start = job.get("start")
sent = job.get("sent", 0)
total = job.get("total", 0)
status = job.get("status")
finish = job.get("finish")
elapsed = (datetime.now() - start).total_seconds() if start else 0
estimated_remaining = None
if sent > 0 and total > sent:
avg = elapsed / sent
estimated_remaining = int(max(0, avg * (total - sent)))

return {
"running": status == "running",
"sent": int(sent),
"total": int(total),
"elapsed_seconds": int(elapsed),
"estimated_remaining_seconds": estimated_remaining,
"started_at": start.isoformat() if start else None,
"finished_at": finish.isoformat() if finish else None,
}

def get_all(self):
return db.session.query(Event).filter(Event.archived.is_(False)).all()
Expand Down Expand Up @@ -996,10 +1060,66 @@ def send_slack_mail(self, event_id: int, slackUrl: str, data: BaseToken):
)
)
# send the created mail
self.mail_client.send_mail_by_id(mail.id)
resp = self.mail_client.send_mail_by_id(mail.id)
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
resp = self.mail_client.send_mail_by_id(mail.id)
self.mail_client.send_mail_by_id(mail.id)

Copilot uses AI. Check for mistakes.

db.session.commit()

@BaseService.needs_service(MailClient)
def send_slack_mail_background(self, event_id: int, slackUrl: str, delay: float = 0.2):
"""
Background-safe sender: creates its own DB session and sends slack invite mails
to accepted hackers with an optional `delay` between sends to avoid throttling.
"""
engine = create_engine(settings.database.url)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)
Comment on lines +1073 to +1074
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.
session = SessionLocal()
try:
event = session.query(Event).filter(Event.id == event_id).first()
if event is None or event.archived:
return

hackers = event.accepted_hackers
total = len(hackers) if hackers is not None else 0
# start tracking job
try:
self._start_job(event_id, "slack", total)
except Exception:
# best-effort tracking, don't fail send if tracking setup fails
pass
Comment on lines +1077 to +1088
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.

for hacker in hackers:
try:
mail = self.mail_client.create_mail(
MailCreate(
template_id=self.mail_client.get_internall_template_id(
InternalTemplate.EVENT_SLACK_INVITE
),
subject="HackEPS2025 slack invitation",
receiver_id=str(hacker.id),
receiver_mail=str(hacker.email),
fields=slackUrl,
)
)
resp = self.mail_client.send_mail_by_id(mail.id)
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
resp = self.mail_client.send_mail_by_id(mail.id)
self.mail_client.send_mail_by_id(mail.id)

Copilot uses AI. Check for mistakes.
# increment progress
try:
self._increment_sent(event_id)
except Exception:
pass
session.commit()
if delay and delay > 0:
time.sleep(delay)
except Exception:
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
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 uses AI. Check for mistakes.
session.rollback()
continue
# finish tracking
try:
self._finish_job(event_id)
except Exception:
pass
finally:
session.close()
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
session.close()
try:
session.close()
finally:
engine.dispose()

Copilot uses AI. Check for mistakes.

@BaseService.needs_service(MailClient)
def send_reminder_mails(
self,
Expand Down Expand Up @@ -1041,7 +1161,8 @@ def send_reminder_mails(
except Exception:
days_left = 0

fields = f"{hacker.name},{event.name},{days_left},{reg.confirm_assistance_token}"
# fields order: name, days_left, token, event_name
fields = f"{hacker.name},{days_left},{reg.confirm_assistance_token},{event.name}"

mail = self.mail_client.create_mail(
MailCreate(
Expand All @@ -1055,10 +1176,90 @@ def send_reminder_mails(
)
)
# send the created mail
self.mail_client.send_mail_by_id(mail.id)
resp = self.mail_client.send_mail_by_id(mail.id)
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
resp = self.mail_client.send_mail_by_id(mail.id)
self.mail_client.send_mail_by_id(mail.id)

Copilot uses AI. Check for mistakes.
db.session.commit()
except Exception as e:
db.session.rollback()
# Optionally log the error here, e.g.:
# print(f"Failed to send reminder to hacker {hacker.id}: {e}")
continue
continue

@BaseService.needs_service(MailClient)
def send_reminder_mails_background(self, event_id: int, delay: float = 0.0):
"""
Background-safe sender: creates its own DB session and sends reminder mails
to accepted hackers. Optional `delay` between sends to avoid throttling.
"""
engine = create_engine(settings.database.url)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)
Comment on lines +1193 to +1194
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.
session = SessionLocal()
try:
event = session.query(Event).filter(Event.id == event_id).first()
if event is None or event.archived:
return

# build list of eligible (hacker, reg) to have accurate total
hackers = event.accepted_hackers
eligible = []
for hacker in hackers:
reg = (
session.query(HackerRegistration)
.filter(
HackerRegistration.user_id == hacker.id,
HackerRegistration.event_id == event.id,
)
.first()
)
if reg is None or reg.confirmed_assistance:
continue
eligible.append((hacker, reg))

total = len(eligible)
try:
self._start_job(event_id, "reminder", total)
except Exception:
pass
Comment on lines +1197 to +1221
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
pass
logging.exception("Failed to start reminder job for event_id %s", event_id)

Copilot uses AI. Check for mistakes.

for hacker, reg in eligible:
try:
# ensure there is a confirmation token for this registration
if not reg.confirm_assistance_token:
reg.confirm_assistance_token = AssistenceToken(hacker, event.id).to_token()

try:
delta = event.start_date - datetime.now()
days_left = max(0, int(delta.total_seconds() // 86400))
except Exception:
days_left = 0

fields = f"{hacker.name},{days_left},{reg.confirm_assistance_token},{event.name}"

mail = self.mail_client.create_mail(
MailCreate(
template_id=self.mail_client.get_internall_template_id(
InternalTemplate.EVENT_HACKER_REMINDER
),
subject=f"{event.name} - Recordatori de confirmació d'assistència",
receiver_id=str(hacker.id),
receiver_mail=str(hacker.email),
fields=fields,
)
)

resp = self.mail_client.send_mail_by_id(mail.id)
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
resp = self.mail_client.send_mail_by_id(mail.id)
self.mail_client.send_mail_by_id(mail.id)

Copilot uses AI. Check for mistakes.
try:
self._increment_sent(event_id)
except Exception:
pass
session.commit()
if delay and delay > 0:
time.sleep(delay)
except Exception:
session.rollback()
continue
Comment on lines +1257 to +1259
Copy link

Copilot AI Nov 19, 2025

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 uses AI. Check for mistakes.
try:
self._finish_job(event_id)
except Exception:
pass
finally:
session.close()
Copy link

Copilot AI Nov 19, 2025

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.

Suggested change
session.close()
try:
session.close()
except Exception:
pass
engine.dispose()

Copilot uses AI. Check for mistakes.
17 changes: 15 additions & 2 deletions src/impl/Mail/client.py
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,
Expand All @@ -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):
Expand Down Expand Up @@ -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):
Expand All @@ -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
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The try-except block catches all exceptions during logging, which is overly broad. Since logger.info() is a simple string formatting operation, catching Exception here will mask genuine issues. Consider either removing this try-except (logging failures are typically non-critical) or catching only specific exceptions like AttributeError if concerned about attribute access on the response object.

Suggested change
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)

Copilot uses AI. Check for mistakes.
return r

def get_template_by_name(self, name):
Expand Down
Loading