Skip to content

refactor(backend): Clear out Notification Service code blockage #9915

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

Merged
merged 8 commits into from
May 9, 2025

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented May 7, 2025

Some of the code paths in the notification & scheduler service were synchronous HTTP calls that execute a long-running job that blocks. This makes the service threads busy waiting.

Changes πŸ—οΈ

  • Remove queue_notification API
  • Remove DTO
  • Move heavy tasks intothe executor

Checklist πŸ“‹

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • Manually executing notification service jobs through the scheduler API

@majdyz majdyz requested a review from ntindle May 7, 2025 11:17
@majdyz majdyz requested a review from a team as a code owner May 7, 2025 11:17
@majdyz majdyz requested review from aarushik93 and removed request for a team May 7, 2025 11:17
@github-project-automation github-project-automation bot moved this to πŸ†• Needs initial review in AutoGPT development kanban May 7, 2025
Copy link

netlify bot commented May 7, 2025

βœ… Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
πŸ”¨ Latest commit e4afd3a
πŸ” Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/681d9cc523282000081cf33a

@majdyz majdyz marked this pull request as draft May 7, 2025 11:17
@github-actions github-actions bot added the platform/backend AutoGPT Platform - Back end label May 7, 2025
@qodo-merge-pro qodo-merge-pro bot added Review effort 3/5 and removed platform/backend AutoGPT Platform - Back end labels May 7, 2025
Copy link

qodo-merge-pro bot commented May 7, 2025

PR Reviewer Guide πŸ”

(Review updated until commit 4e6b66f)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 πŸ”΅πŸ”΅πŸ”΅βšͺβšͺ
πŸ§ͺΒ No relevant tests
πŸ”’Β No security concerns identified
⚑ Recommended focus areas for review

Error Handling

The new queue_notification and queue_notification_async functions have similar error handling but don't follow the same pattern as the service methods they replace. Consider standardizing error handling across these functions.

def queue_notification(event: NotificationEventModel) -> NotificationResult:
    """Queue a notification - exposed method for other services to call"""
    try:
        logger.debug(f"Received Request to queue {event=}")

        exchange = "notifications"
        routing_key = get_routing_key(event.type)

        queue = get_notification_queue()
        queue.publish_message(
            routing_key=routing_key,
            message=event.model_dump_json(),
            exchange=next(ex for ex in EXCHANGES if ex.name == exchange),
        )

        return NotificationResult(
            success=True,
            message=f"Notification queued with routing key: {routing_key}",
        )

    except Exception as e:
        logger.exception(f"Error queueing notification: {e}")
        return NotificationResult(success=False, message=str(e))


async def queue_notification_async(event: NotificationEventModel) -> NotificationResult:
    """Queue a notification - exposed method for other services to call"""
    try:
        logger.debug(f"Received Request to queue {event=}")

        exchange = "notifications"
        routing_key = get_routing_key(event.type)

        queue = await get_async_notification_queue()
        await queue.publish_message(
            routing_key=routing_key,
            message=event.model_dump_json(),
            exchange=next(ex for ex in EXCHANGES if ex.name == exchange),
        )

        return NotificationResult(
            success=True,
            message=f"Notification queued with routing key: {routing_key}",
        )

    except Exception as e:
        logger.exception(f"Error queueing notification: {e}")
        return NotificationResult(success=False, message=str(e))
Thread Safety

The background_executor is shared globally but there's no clear lifecycle management. Consider adding proper initialization and shutdown for the ThreadPoolExecutor to prevent resource leaks.

background_executor = ThreadPoolExecutor(max_workers=2)
Code Duplication

The queue_notification and queue_notification_async functions share almost identical code. Consider refactoring to reduce duplication by extracting common logic into a shared helper function.

def queue_notification(event: NotificationEventModel) -> NotificationResult:
    """Queue a notification - exposed method for other services to call"""
    try:
        logger.debug(f"Received Request to queue {event=}")

        exchange = "notifications"
        routing_key = get_routing_key(event.type)

        queue = get_notification_queue()
        queue.publish_message(
            routing_key=routing_key,
            message=event.model_dump_json(),
            exchange=next(ex for ex in EXCHANGES if ex.name == exchange),
        )

        return NotificationResult(
            success=True,
            message=f"Notification queued with routing key: {routing_key}",
        )

    except Exception as e:
        logger.exception(f"Error queueing notification: {e}")
        return NotificationResult(success=False, message=str(e))


async def queue_notification_async(event: NotificationEventModel) -> NotificationResult:
    """Queue a notification - exposed method for other services to call"""
    try:
        logger.debug(f"Received Request to queue {event=}")

        exchange = "notifications"
        routing_key = get_routing_key(event.type)

        queue = await get_async_notification_queue()
        await queue.publish_message(
            routing_key=routing_key,
            message=event.model_dump_json(),
            exchange=next(ex for ex in EXCHANGES if ex.name == exchange),
        )

        return NotificationResult(
            success=True,
            message=f"Notification queued with routing key: {routing_key}",
        )

    except Exception as e:
        logger.exception(f"Error queueing notification: {e}")
        return NotificationResult(success=False, message=str(e))

@github-actions github-actions bot added the size/l label May 7, 2025
Copy link

deepsource-io bot commented May 7, 2025

Here's the code health analysis summary for commits 089e7aa..e4afd3a. View details on DeepSourceΒ β†—.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScriptβœ…Β SuccessView CheckΒ β†—
DeepSource Python LogoPythonβœ…Β Success
❗ 104 occurences introduced
🎯 106 occurences resolved
View CheckΒ β†—

πŸ’‘ If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link

netlify bot commented May 7, 2025

βœ… Deploy Preview for auto-gpt-docs canceled.

Name Link
πŸ”¨ Latest commit e4afd3a
πŸ” Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/681d9cc5b6471400084da577

@github-actions github-actions bot added the platform/backend AutoGPT Platform - Back end label May 7, 2025
@majdyz majdyz changed the title WIP: Clear out Notification Service code blockage refactor(backend): Clear out Notification Service code blockage May 8, 2025
@majdyz majdyz marked this pull request as ready for review May 8, 2025 09:24
@majdyz majdyz enabled auto-merge May 9, 2025 06:12
@github-project-automation github-project-automation bot moved this from πŸ†• Needs initial review to πŸ‘πŸΌ Mergeable in AutoGPT development kanban May 9, 2025
@majdyz majdyz added this pull request to the merge queue May 9, 2025
Merged via the queue into dev with commit 82cf0bc May 9, 2025
23 checks passed
@majdyz majdyz deleted the zamilmajdy/notification-service-cleanup branch May 9, 2025 06:52
@github-project-automation github-project-automation bot moved this from πŸ‘πŸΌ Mergeable to βœ… Done in AutoGPT development kanban May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: βœ… Done
Development

Successfully merging this pull request may close these issues.

2 participants