fix: relaunch Chromium on disconnect and fail health check when it is gone - #17
Open
HarshMN2345 wants to merge 9 commits into
Open
fix: relaunch Chromium on disconnect and fail health check when it is gone#17HarshMN2345 wants to merge 9 commits into
HarshMN2345 wants to merge 9 commits into
Conversation
… gone Chromium runs as a module-level singleton with no reconnect. When it dies the Bun server keeps listening, so every subsequent request fails with "Target page, context or browser has been closed" until something restarts the container, and /v1/health reports the failure with a 200 so no probe can act on it. Resolve the browser through getBrowser(), which relaunches when the instance is disconnected and shares one relaunch between concurrent callers, and return 503 from /v1/health so an httpGet liveness probe can restart the pod.
Docker Image Stats
Screenshot benchmark: Average of 3 runs on https://appwrite.io |
Greptile SummaryThis PR adds on-demand Chromium recovery and makes browser disconnection visible through the health endpoint.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains. Important Files Changed
Reviews (9): Last reviewed commit: "test: poll for the disconnect instead of..." | Re-trigger Greptile |
Kills Chromium inside the running container and asserts the service reports 503 while it is gone, recovers on the next capture and reports 200 again. Runs as its own suite and CI step because killing the browser mid-flight fails whatever else is using it, and skips when no container is resolvable so it stays runnable outside CI.
Meldiron
reviewed
Aug 24, 2026
Meldiron
reviewed
Aug 24, 2026
…uard Export the browser instance as a live binding instead of wrapping it in isBrowserConnected(); health.ts reads it directly, same as it did before this was a mutable reference. Replace the nullish-coalescing-assignment promise chain with a plain if guard around launch(). Same single-flight behaviour — concurrent callers see launching non-null and await the same promise — but reads as an ordinary guarded assignment instead of an unfamiliar idiom.
Killing headless-shell by walking /proc simulated the symptom (the process disappears) but not a real trigger, and depended on docker exec and a running compose container. Send the CDP Browser.crash command instead, the same one Chromium itself exposes for testing crash recovery, through a small test-support endpoint alongside the existing /v1/test. The recovery test now runs over plain HTTP against any running instance, no docker exec or container name resolution needed.
/v1/test/crash destroys the shared browser instance, unlike /v1/test which is read-only, so it stays unregistered unless ENABLE_TEST_ROUTES=1 is set. The deployed chart won't set it, so the route doesn't exist in any reachable environment; docker-compose.yml sets it for local and CI runs. Confirmed 404 without the flag and 200 with it, both against the same image.
The crash endpoint shipped test-only surface in the product image and needed an env gate to keep it unreachable. It was also less faithful than what it replaced: in production Chromium dies from the outside, OOM-killed, not from a CDP client asking it to crash. Send SIGKILL to headless-shell via pidof instead, which is exactly the production failure, and cover the relaunch logic the e2e cannot reach, shared in-flight relaunch and retry after a failed launch, with unit tests that mock playwright-core.
Kill Chromium, fire five concurrent captures, and assert every one succeeds while the container logs show a single relaunch. A broken guard launches one Chromium per request, which the log count catches, so the mocked unit tests and their fake browser are no longer needed. Verified the assertion has teeth by removing the guard: five concurrent launches wedged the service past the test timeout.
A fixed sleep is slower than needed when Playwright notices the dead browser in milliseconds and fails the run on a machine where it takes longer. Poll health until it reports 503, bounded at 30s. Health only reads isConnected, so polling cannot itself trigger the relaunch the second test counts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Chromium is launched once at module load and never re-checked:
When it dies, the Bun server keeps listening. Every request after that fails with
Target page, context or browser has been closed, and it stays that way until something unrelated restarts the container./v1/healthalready reports the right thing —browser.isConnected()— but returns it with HTTP 200, so no probe can act on it.Observed in production: sustained failures at roughly this rate for days, each failing in well under a second, with zero pod restarts. It stopped only when a rollout happened to restart the pods.
Change
getBrowser()resolves the browser and relaunches it when the instance is disconnected. Concurrent callers share a single in-flight relaunch, and the guard is cleared on failure so a failed relaunch is retried on the next request./v1/healthreturns 503 when disconnected, so anhttpGetliveness probe can restart the pod.screenshots,reportsandtestresolve throughgetBrowser()instead of importing the singleton.Verification
Ran the service and killed Chromium out from under it.
passpassfailfail"context: Target page, context or browser has been closed"fail, stays brokenpass, self-healedThe "before" column reproduces the production error string exactly.
bun run type-checkandbun run lintpass.Tests
tests/recovery/browser.test.ts, running against the real image as its own CI step:/v1/healthreturns 503, a capture succeeds (relaunch), and health returns 200 again.No mocks, no test-only surface in the image. The suite runs separately from the other e2e tests because killing the browser mid-flight fails whatever else is using it, and skips when no container is resolvable.
Note for deployment
The 503 only has an effect once the liveness probe actually calls
/v1/health. The chart currently usestcpSocket, which only checks that Bun is listening — that needs a separate change tohttpGeton this path, plus a readiness probe so a pod with a dead browser leaves the Service.