Skip to content

Conversation

@JohnThomson
Copy link
Contributor

@JohnThomson JohnThomson commented Dec 3, 2025

This change is Reviewable

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch portFindingAndZombies

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a robust shutdown mechanism to prevent zombie Bloom processes that block new instances from starting. The solution introduces a forced shutdown timeout after 20 seconds, replaces all Application.Exit() calls with a custom Program.Exit() method, and refactors server cleanup logic.

Key Changes

  • Added Program.Exit() method with a 20-second timeout mechanism that forcefully terminates Bloom if normal shutdown hangs
  • Systematically replaced all Application.Exit() calls with Program.Exit() across 7 files
  • Extracted CloseListener() method from BloomServer's Dispose for reusability in forced shutdown
  • Simplified HandleExceptionOpeningPort() to use console logging instead of NonFatalProblem reporting, enabling quiet retry of multiple ports

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/BloomExe/Program.cs Added Exit() with forced shutdown timer, ReleaseBloomToken() extraction, and _gotUniqueToken field
src/BloomExe/web/BloomServer.cs Extracted CloseListener() method and simplified port retry error handling
src/BloomExe/web/BloomWebSocketServer.cs Updated to use Program.Exit() instead of Application.Exit()
src/BloomExe/ProjectContext.cs Updated to use Program.Exit() instead of Application.Exit()
src/BloomExe/MiscUI/BrowserProgressDialog.cs Updated to use Program.Exit() instead of Application.Exit()
src/BloomExe/InstallerSupport.cs Updated to use Program.Exit() instead of Application.Exit()
src/BloomExe/FatalExceptionHandler.cs Updated to use Program.Exit() instead of Application.Exit()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
Program.ReleaseBloomToken();
}
catch (Exception) { }
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Poor error handling: empty catch block.

Suggested change
catch (Exception) { }
catch (Exception ex)
{
// If we can't log it, at least write to Console.Error.
Console.Error.WriteLine("Exception during ReleaseBloomToken: " + ex);
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@StephenMcConnel StephenMcConnel left a comment

Choose a reason for hiding this comment

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

@StephenMcConnel reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

@andrew-polk
Copy link
Contributor

src/BloomExe/Program.cs line 1304 at r1 (raw file):

        {
            EnsureBloomReallyQuits();
            Application.Exit();

Is it worth trying to future-proof things such that we can't call Application.Exit() other than from here?
(We have other c#-based checks we run for this kind of thing...)

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 7 of 7 files at r1, all commit messages.
@andrew-polk dismissed @copilot-pull-request-reviewer[bot] from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

Copy link

Copilot AI commented Dec 4, 2025

@andrew-polk I've opened a new pull request, #7533, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk dismissed @copilot-pull-request-reviewer[bot] from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

Copy link

Copilot AI commented Dec 4, 2025

@andrew-polk I've opened a new pull request, #7534, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk dismissed @copilot-pull-request-reviewer[bot] from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JohnThomson)

@andrew-polk andrew-polk merged commit a1b26ce into master Dec 4, 2025
2 checks passed
@andrew-polk andrew-polk deleted the portFindingAndZombies branch December 4, 2025 17:42
Copy link

Copilot AI commented Dec 4, 2025

@andrew-polk I've opened a new pull request, #7535, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor Author

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


src/BloomExe/Program.cs line 1304 at r1 (raw file):

Previously, andrew-polk wrote…

Is it worth trying to future-proof things such that we can't call Application.Exit() other than from here?
(We have other c#-based checks we run for this kind of thing...)

Done.

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.

4 participants