Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive background job system for sending Slack invite and reminder emails to event participants, improving scalability by preventing request timeouts. The implementation includes thread-safe job tracking, progress reporting endpoints, and refactored mail sending logic with proper session management.
Key Changes:
- Background job scheduling for Slack invite and reminder mail sending with progress tracking
- Thread-safe job tracking infrastructure using locks and status management
- New progress endpoint to monitor background mail sending operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| src/impl/Mail/client.py | Enhanced logging with structured log statements replacing print statements for better observability |
| src/impl/Event/service.py | Implemented thread-safe job tracking, background mail sending methods with independent DB sessions, and progress reporting functionality |
| src/impl/Event/router_v1.py | Added background task endpoints with authorization checks, progress tracking endpoint, and refactored existing mail endpoints to use background processing |
Comments suppressed due to low confidence (4)
src/impl/Event/service.py:1107
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
src/impl/Event/service.py:1118
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
src/impl/Event/service.py:1252
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
src/impl/Event/service.py:1262
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| engine = create_engine(settings.database.url) | ||
| SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) |
There was a problem hiding this comment.
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.
| engine = create_engine(settings.database.url) | ||
| SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) |
There was a problem hiding this comment.
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.
| event_service.send_reminder_mails(event_id, token) | ||
| return {"success": True} | ||
| if not token.check([UserType.LLEIDAHACKER]): | ||
| raise AuthenticationException("Not authorized") |
There was a problem hiding this comment.
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.
| 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") |
| session.commit() | ||
| if delay and delay > 0: | ||
| time.sleep(delay) | ||
| except Exception: |
There was a problem hiding this comment.
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')) |
| except Exception: | ||
| pass | ||
| finally: | ||
| session.close() |
There was a problem hiding this comment.
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() |
| fields=slackUrl, | ||
| ) | ||
| ) | ||
| resp = self.mail_client.send_mail_by_id(mail.id) |
There was a problem hiding this comment.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
| ) | ||
| # send the created mail | ||
| self.mail_client.send_mail_by_id(mail.id) | ||
| resp = self.mail_client.send_mail_by_id(mail.id) |
There was a problem hiding this comment.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
| ) | ||
| ) | ||
|
|
||
| resp = self.mail_client.send_mail_by_id(mail.id) |
There was a problem hiding this comment.
Variable resp is not used.
| resp = self.mail_client.send_mail_by_id(mail.id) | |
| self.mail_client.send_mail_by_id(mail.id) |
| from sqlalchemy.orm import sessionmaker | ||
| from datetime import datetime | ||
| import time | ||
| import logging |
There was a problem hiding this comment.
Import of 'logging' is not used.
| import logging |
| try: | ||
| self._start_job(event_id, "reminder", total) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'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) |
This pull request introduces background job support for sending Slack invite and reminder emails to event participants, improving scalability and user experience by preventing request timeouts and enabling progress tracking. It also adds job tracking and progress endpoints, refactors mail sending to be background-safe, and improves logging in the mail client.
Background job support and API enhancements:
UserType.LLEIDAHACKER) can trigger these actions. [1] [2]/send_progress) to report the status of background mail jobs for a given event, including sent count, total, elapsed time, and estimated time remaining. [1] [2]Service layer and job tracking:
EventServiceto monitor background mail jobs per event, with methods to start, increment, finish, and report job progress.send_slack_mail_background,send_reminder_mails_background) to run in background-safe mode with their own DB sessions, optional throttling delay, and accurate progress tracking. [1] [2]Logging improvements:
MailClientto record mail creation and sending activities, replacing print statements with structured logging for better observability. [1] [2] [3] [4]Minor fixes and refactoring: