Skip to content

Conversation

@hiroTamada
Copy link
Contributor

@hiroTamada hiroTamada commented Sep 24, 2025

TL;DR

Restores the logic to delete browsers when an invocation is cancelled.

Why we made these changes

This fixes a regression where cancelling an invocation would leave orphaned browser instances running, consuming unnecessary resources. This change ensures proper resource cleanup.

What changed?

  • In cmd/invoke.go, the invocation cancellation handler now calls client.Invocations.DeleteBrowsers with a 30-second timeout to ensure associated browsers are properly terminated.

Description generated by Mesa. Update settings

Copy link

@mesa-dot-dev mesa-dot-dev bot left a comment

Choose a reason for hiding this comment

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

Performed full review of fa40463...888cb19

Analysis

  1. The use of context.Background() instead of the cancellation context could potentially lead to unbound cleanup operations if the original request timeout was triggered for a valid reason.
  2. The change addresses browser cleanup during cancellation, but there's no mention of verifying if this creates any race conditions or potential edge cases in the cleanup process.
  3. While following existing patterns in the codebase is generally good, it's worth examining if the current timeout values are appropriate for this specific cleanup scenario.

Tip

⚡ Quick Actions

This review was generated by Mesa.

Actions:

Slash Commands:

  • /review - Request a full code review
  • /review latest - Review only changes since the last review
  • /describe - Generate PR description. This will update the PR body or issue comment depending on your configuration
  • /help - Get help with Mesa commands and configuration options

1 files reviewed | 0 comments | Review on Mesa | Edit Reviewer Settings

Copy link
Contributor

@rgarcia rgarcia left a comment

Choose a reason for hiding this comment

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

I think the order should be reversed since if we destroy browsers first it might dump error logs into the invocation that are red herrings

@hiroTamada hiroTamada merged commit ff52a92 into main Sep 24, 2025
1 check passed
@hiroTamada hiroTamada deleted the hiro/bring_back_browser_deletion branch September 24, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants