Browser fetchers - Report the real status code when Chrome aborts a bodiless error response (#4420)

* Browser fetchers - Report the real status code when Chrome aborts a bodiless error response Chrome 153+ refuses to commit a navigation when a 4xx/5xx arrives with a zero-length body: page.goto() raises net::ERR_HTTP_RESPONSE_CODE_FAILURE instead of returning the response. The response is received fine, we just never get it as a return value, so the raw net:: string landed in last_error instead of "Error - 404". Verified against two browser images, same HTTP server: Chrome 153 empty-body 404 -> raises ERR_HTTP_RESPONSE_CODE_FAILURE Chrome 153 404 with body -> status=404 Chromium 119 empty-body 404 -> status=404 Chromium 119 404 with body -> status=404 The fix keeps the main-frame response from the 'response' event and hands that back when goto raises, so .status / .all_headers() and the existing non-200 branch (which also captures the screenshot) work unchanged. The latest matching response wins, so a redirect chain still reports its final hop. Any other error re-raises as before, and if no response was captured we re-raise too - the status is never invented, which keeps older browsers on exactly their old path. Two independent navigation sites needed it: - browser_steps.py action_goto_url - covers the playwright fetcher, the live Browser Steps UI, the Goto URL / Goto site steps, and the CloakBrowser plugin which imports it. This is the one that broke CI: content_fetchers/__init__.py forces playwright when a watch has browser steps, so test_non_200_errors_report_browsersteps ran the playwright path in the pyppeteer jobs too. - puppeteer.py - its own goto retry loop, used when FAST_PUPPETEER_CHROME_FETCHER is set and the watch has no browser steps. No test covers that path; verified by driving the fetcher directly. Note pyppeteer exposes isNavigationRequest / frame / mainFrame as properties where playwright uses is_navigation_request() as a method. Mixing them up raises 'bool' object is not callable, which gets swallowed as a renderer page error rather than failing loudly. Checked against the pinned pyppeteer-ng==2.0.0rc16. Selenium is unaffected - it hardcodes status_code = 200 because WebDriver cannot see the HTTP status, so it never reaches the non-200 branch. Tested with the full CI browser set (test_content, test_errorhandling, test_fetch_data, test_custom_js_before_content): 10 passed on each of Chrome 153 + playwright, Chrome 153 + pyppeteer, Chromium 119 + playwright, Chromium 119 + pyppeteer, the last two against a canonical Dockerfile.chromium119 build so Chromium is the only variable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * CI - Make a fail-fast test abort say what was skipped rather than looking like a total failure Fail-fast is kept deliberately - the first failure is nearly always the real problem and it keeps the run short - but nothing said so, which made a single failing assertion read as "every browser test is broken". The playwright and pyppeteer jobs each ran four pytest files as four commands in one `run:` block, which GitHub executes under `bash -e`. tests/visualselector/test_fetch_data.py is the third, so when one 404 assertion failed there, test_custom_js_before_content.py never ran, and the later "Headers and requests" and "Restock detection" steps were skipped as a consequence - three test files silently dropped, reported only as dashes in the job list. run_basic_tests.sh has the same shape: 8 independent pytest groups under `set -e`, so a failure in the first parallel group hides the 7 after it. No behaviour change to when we stop - only to what gets reported: - Each browser test file now runs inside its own ::group:: so the log is navigable, and the failing file is named in a ::error:: annotation that states plainly that the remaining files and steps were SKIPPED, not failed. - run_basic_tests.sh gets an ERR trap saying the same thing, with the line number of the group that aborted. Verified the loop stops on the third file, names it and exits 1, that the all-pass path still exits 0, and that the trap reports the failing line while preserving the exit code. YAML parses and run_basic_tests.sh passes bash -n. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix unit test failure - install the navigation-response tracker only on a page that supports events action_goto_url() registered its 'response' listener unconditionally, which broke test_fetch_url_gate.py::TestBrowserStepGotoUrlGate::test_permitted_url_still_navigates: self.page.on("response", _keep_navigation_response) E AttributeError: '_RecordingPage' object has no attribute 'on' The three refusal tests in that class still passed because validate_fetch_url_async() raises before reaching the listener, so only the permitted-URL case (the one that actually navigates) hit it. The listener now lives in track_latest_navigation_response(), which returns None for a page that has no event support instead of raising. That also removes a real inefficiency: registering per navigation meant a page accumulated a listener per goto(), and 'response' fires for every subresource - measured 133 events on getastra.com (54 script, 37 image, 19 fetch, 11 xhr, ...) of which only 2 were navigations. The tracker is installed once per page and shared, verified as one listener remaining after an install plus four navigations. Tested: 494 unit + llm tests pass (was 1 failed / 480 passed), and the full browser set still passes 10/10 on playwright and 10/10 on pyppeteer, so the Chrome 153 "Error - 404" recovery still works through the shared tracker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-26 15:26:13 +00:00 · 2026-09-12 13:15:44 +02:00
parent 5842e7a158
commit 6318fc70f4
4 changed files with 121 additions and 25 deletions
@@ -201,10 +201,20 @@ jobs:

      - name: Playwright - Specific tests in built container
        run: |
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest -vv --capture=tee-sys --showlocals --tb=long --live-server-host=0.0.0.0 --live-server-port=5004 tests/fetchers/test_content.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest -vv --capture=tee-sys --showlocals --tb=long --live-server-host=0.0.0.0 --live-server-port=5004 tests/test_errorhandling.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest -vv --capture=tee-sys --showlocals --tb=long --live-server-host=0.0.0.0 --live-server-port=5004 tests/visualselector/test_fetch_data.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest -vv --capture=tee-sys --showlocals --tb=long --live-server-host=0.0.0.0 --live-server-port=5004 tests/fetchers/test_custom_js_before_content.py'
+          # Deliberately fail fast - the first failure is nearly always the real problem and it
+          # keeps the run short. Each file is wrapped in its own log group and the failing one is
+          # named explicitly, because everything after it is SKIPPED rather than run, and that is
+          # otherwise easy to misread as "the whole browser suite broke".
+          for t in tests/fetchers/test_content.py tests/test_errorhandling.py tests/visualselector/test_fetch_data.py tests/fetchers/test_custom_js_before_content.py; do
+            echo "::group::pytest $t"
+            if ! docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio \
+                 bash -c "cd changedetectionio;pytest -vv --capture=tee-sys --showlocals --tb=long --live-server-host=0.0.0.0 --live-server-port=5004 $t"; then
+              echo "::endgroup::"
+              echo "::error::$t FAILED - stopping here. Any later test file and the remaining steps of this job were SKIPPED, not failed."
+              exit 1
+            fi
+            echo "::endgroup::"
+          done

      - name: Playwright - Headers and requests
        run: |
@@ -242,10 +252,17 @@ jobs:

      - name: Pyppeteer - Specific tests in built container
        run: |
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "FAST_PUPPETEER_CHROME_FETCHER=True" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest --live-server-host=0.0.0.0 --live-server-port=5004 tests/fetchers/test_content.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "FAST_PUPPETEER_CHROME_FETCHER=True" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest --live-server-host=0.0.0.0 --live-server-port=5004 tests/test_errorhandling.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "FAST_PUPPETEER_CHROME_FETCHER=True" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest --live-server-host=0.0.0.0 --live-server-port=5004 tests/visualselector/test_fetch_data.py'
-          docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "FAST_PUPPETEER_CHROME_FETCHER=True" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio bash -c 'cd changedetectionio;pytest --live-server-host=0.0.0.0 --live-server-port=5004 tests/fetchers/test_custom_js_before_content.py'
+          # Fail fast, but name the file that failed - see the note in the playwright job above
+          for t in tests/fetchers/test_content.py tests/test_errorhandling.py tests/visualselector/test_fetch_data.py tests/fetchers/test_custom_js_before_content.py; do
+            echo "::group::pytest $t"
+            if ! docker run --rm -e "FLASK_SERVER_NAME=cdio" -e "FAST_PUPPETEER_CHROME_FETCHER=True" -e "PLAYWRIGHT_DRIVER_URL=ws://sockpuppetbrowser:3000" --network changedet-network --hostname=cdio test-changedetectionio \
+                 bash -c "cd changedetectionio;pytest --live-server-host=0.0.0.0 --live-server-port=5004 $t"; then
+              echo "::endgroup::"
+              echo "::error::$t FAILED - stopping here. Any later test file and the remaining steps of this job were SKIPPED, not failed."
+              exit 1
+            fi
+            echo "::endgroup::"
+          done

      - name: Pyppeteer - Headers and requests checks
        run: |
@@ -9,6 +9,39 @@ from changedetectionio.content_fetchers.base import get_playwright_bypass_csp, m
 from changedetectionio.jinja2_custom import render as jinja_render
 from changedetectionio.validate_url import validate_fetch_url_async

+def track_latest_navigation_response(page):
+    """Record the latest main-frame document response seen on this page, and return the holder.
+
+    Idempotent on purpose - every navigation would otherwise add another 'response' listener, and
+    that event fires once per HTTP response (hundreds on a heavy page), so the callbacks are worth
+    not duplicating. One tracker per page is installed and then shared by the fetcher and by every
+    action_goto_url() call on it.
+
+    Returns a dict that holds {'response': <latest main-frame document response>}, or None if the
+    page does not support event listeners (the unit test stubs, mainly).
+    """
+    if not hasattr(page, 'on'):
+        return None
+
+    existing = getattr(page, '_cdio_latest_navigation_response', None)
+    if existing is not None:
+        return existing
+
+    latest = {}
+
+    def _keep(response):
+        try:
+            if response.frame == page.main_frame and response.request.is_navigation_request():
+                latest['response'] = response
+        except Exception as e:
+            # Never let a bookkeeping listener break a fetch
+            logger.debug(f"Could not record navigation response: {e}")
+
+    page.on("response", _keep)
+    page._cdio_latest_navigation_response = latest
+    return latest
+
+
 def browser_steps_get_valid_steps(browser_steps: list):
    if browser_steps is not None and len(browser_steps):
        valid_steps = list(filter(
@@ -145,8 +178,23 @@ class steppable_browser_interface():
        # and private-IP SSRF possible via a browser step (GHSA-hm22-wg2m-35v4).
        await validate_fetch_url_async(value)

+        # Chrome 153+ refuses to commit a navigation when an error status arrives with a
+        # zero-length body, so page.goto() raises net::ERR_HTTP_RESPONSE_CODE_FAILURE instead of
+        # handing back the response. The response was received fine, we just never get it as a
+        # return value, so fall back to the page's navigation-response tracker and hand that back -
+        # callers then report a real "Error - 404" instead of a raw net:: string.
+        navigation_response = track_latest_navigation_response(self.page)
+
        now = time.time()
-        response = await self.page.goto(value, timeout=0, wait_until='load')
+        try:
+            response = await self.page.goto(value, timeout=0, wait_until='load')
+        except Exception as e:
+            if 'ERR_HTTP_RESPONSE_CODE_FAILURE' not in str(e) or not navigation_response:
+                raise
+            response = navigation_response['response']
+            logger.debug(f"Navigation was aborted by the browser (empty body on an error status), "
+                         f"recovered status {response.status} from the response event")
+
        logger.debug(f"Time to goto URL {time.time()-now:.2f}s")
        return response

@@ -430,25 +430,50 @@ class fetcher(Fetcher):
        # Listen for first response to trigger frame handler setup
        self.page._client.on('Network.responseReceived', setup_frame_handlers_on_first_response)

+        # Chrome 153+ refuses to commit a navigation when an error status arrives with a
+        # zero-length body, so goto() raises net::ERR_HTTP_RESPONSE_CODE_FAILURE instead of handing
+        # back the response. The response was received fine, we just never get it as a return value,
+        # so keep the main-frame response from the 'response' event and use that instead - the
+        # status check below then reports a real "Error - 404" instead of a raw net:: string.
+        # Kept as the latest matching response so a redirect chain reports its final hop.
+        navigation_response = {}
+
+        def _keep_navigation_response(response):
+            # Note pyppeteer exposes these as properties, unlike playwright where they are methods
+            if response.frame == self.page.mainFrame and response.request.isNavigationRequest:
+                navigation_response['response'] = response
+
+        self.page.on('response', _keep_navigation_response)
+
        response = None
        attempt=0
-        while not response:
-            logger.debug(f"Attempting page fetch {url} attempt {attempt}")
-            asyncio.create_task(handle_frame_navigation())
-            response = await self.page.goto(url, timeout=0)
-            await asyncio.sleep(1 + extra_wait)
-            # Check if page still exists before sending command
-            if self.page and hasattr(self.page, '_client'):
-                await self.page._client.send('Page.stopLoading')
+        try:
+            while not response:
+                logger.debug(f"Attempting page fetch {url} attempt {attempt}")
+                asyncio.create_task(handle_frame_navigation())
+                try:
+                    response = await self.page.goto(url, timeout=0)
+                except Exception as e:
+                    if 'ERR_HTTP_RESPONSE_CODE_FAILURE' not in str(e) or not navigation_response:
+                        raise
+                    response = navigation_response['response']
+                    logger.debug(f"Navigation was aborted by the browser (empty body on an error status), "
+                                 f"recovered status {response.status} from the response event")
+                await asyncio.sleep(1 + extra_wait)
+                # Check if page still exists before sending command
+                if self.page and hasattr(self.page, '_client'):
+                    await self.page._client.send('Page.stopLoading')

-            if response:
-                break
-            if not response:
-                logger.warning("Page did not fetch! trying again!")
-            if response is None and attempt>=2:
-                logger.warning(f"Content Fetcher > Response object was none (as in, the response from the browser was empty, not just the content) exiting attempt {attempt}")
-                raise EmptyReply(url=url, status_code=None)
-            attempt+=1
+                if response:
+                    break
+                if not response:
+                    logger.warning("Page did not fetch! trying again!")
+                if response is None and attempt>=2:
+                    logger.warning(f"Content Fetcher > Response object was none (as in, the response from the browser was empty, not just the content) exiting attempt {attempt}")
+                    raise EmptyReply(url=url, status_code=None)
+                attempt+=1
+        finally:
+            self.page.remove_listener('response', _keep_navigation_response)

        self.headers = response.headers

@@ -9,6 +9,12 @@
 # exit when any command fails
 set -e

+# Failing fast is deliberate here (the first failure is usually the real problem, and it keeps
+# the run short) - but this script runs 8 independent pytest groups, so make it obvious that the
+# groups after the failure were SKIPPED rather than passed. Otherwise one failing test reads as
+# "the whole basic suite is broken".
+trap 'rc=$?; echo "::error::run_basic_tests.sh aborted at line $LINENO (exit $rc) - the test groups after this point were SKIPPED, not run"' ERR
+
 SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
 rm tests/logs/* -f