Skip to content

fix: close Chrome before engine shutdown returns - #122

Merged
LeyckerS merged 1 commit into
LeyckerS:mainfrom
Guflly:fix/chrome-exit-cleanup
Aug 2, 2026
Merged

fix: close Chrome before engine shutdown returns#122
LeyckerS merged 1 commit into
LeyckerS:mainfrom
Guflly:fix/chrome-exit-cleanup

Conversation

@Guflly

@Guflly Guflly commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Description

Makes Engine.stop() close the active BrowserGate and wait briefly for its worker, so Chrome is gone before shutdown returns. BrowserGate also rejects late opens after closing, with a regression test for mid-run shutdown.

Closes #89.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would change existing behavior)
  • Documentation update
  • Refactor / code cleanup
  • Other:

Checklist

  • I have tested my changes locally
  • If this affects shared logic (extraction, download engine), I also applied the equivalent change to moon_cli.py
  • I have kept the single-file architecture (no package split)
  • I have not added new dependencies without justification in the PR description

The CLI already awaits its own browser cleanup; the shared BrowserGate change applies to both paths without a separate CLI change.

Tests: 17 unit tests, Ruff, integration_http.py, integration_web.py, and render_gui.py.

Screenshots / logs (if applicable)

N/A

@LeyckerS LeyckerS left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Merging. This is the fix #89 needed and the test is the kind that actually proves something.

I traced it to the process rather than trusting the description, because the interesting case here is a CDP-attached Chrome — closing a Playwright connection is not the same as ending a process. It holds: BrowserGate.aclose() branches on shared and calls shutdown_chrome(), which does terminate() on _CHROME_PROC. So the real Chrome dies, not just the handle.

Three things done right:

  • The browser_started nonlocal. gate.opened reads _browser is not None, and aclose() nulls it — so once stop() closes the gate, the old if gate.opened: would have skipped the counter decrement and left _browsers stuck at 1 forever. That was not in #89 and you had to find it yourself.
  • _closed on the gate. Without it a worker calling get() after teardown would cheerfully relaunch Chrome and orphan a second one. Checked before and inside the lock, which is the correct shape.
  • The test asserts what matters: shutdown_chrome was called before stop() returned, and the thread was already dead. A weaker test would have checked the counter afterwards and passed even if the close happened later.

Your note about the CLI checkbox is right — moon_cli.py:239 already does await gate.aclose() in its own finally, and the BrowserGate change is shared. No CLI edit needed.

Two things worth knowing, neither blocking:

1. The 1.5s budget can be shorter than the work it waits for. shutdown_chrome() does _CHROME_PROC.wait(timeout=8) internally. If Chrome takes longer than the remaining deadline, future.result() times out, the except Exception: pass swallows it, stop() returns — and on the exit path the interpreter then tears the daemon thread down mid-teardown, which is the original bug again. Rare, since terminate is usually sub-second, but the two numbers should not disagree by that much.

The deeper point is that stop() has two callers with different tolerances. The GUI's Stop button should not hang for five seconds. moon_bridge.py's finally: engine.stop() on app exit absolutely can — the window is already closing and nobody is waiting. It calls with the default today. Passing a longer timeout there would close the remaining gap; worth its own small change rather than folding it in here.

2. Stop now ends in-flight extractions, and the log line does not say so. Closing the gate makes any worker waiting on get() raise, so pending datanodes links fail rather than continue. That is defensible for a stop — arguably it is what stop should have always meant — but the message still reads "stop requested — finishing the downloads in flight...", which is now only true of the downloads. #65 is open on that wording and this makes it slightly more wrong; worth a line there.

Closes #89. Thanks, and welcome.

@LeyckerS
LeyckerS merged commit 3399a86 into LeyckerS:main Aug 2, 2026
5 checks passed
LeyckerS added a commit that referenced this pull request Aug 2, 2026
Thirteenth outside contributor.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

bug: closing the app mid-run leaves the Chrome process alive

2 participants