diff --git a/.github/workflows/test-stack-reusable-workflow.yml b/.github/workflows/test-stack-reusable-workflow.yml index 0fe623875..dae9e9dd7 100644 --- a/.github/workflows/test-stack-reusable-workflow.yml +++ b/.github/workflows/test-stack-reusable-workflow.yml @@ -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: | diff --git a/changedetectionio/browser_steps/browser_steps.py b/changedetectionio/browser_steps/browser_steps.py index 3aace66dc..15c925786 100644 --- a/changedetectionio/browser_steps/browser_steps.py +++ b/changedetectionio/browser_steps/browser_steps.py @@ -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': }, 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 diff --git a/changedetectionio/content_fetchers/puppeteer.py b/changedetectionio/content_fetchers/puppeteer.py index 362e0d167..6e9acbb1a 100644 --- a/changedetectionio/content_fetchers/puppeteer.py +++ b/changedetectionio/content_fetchers/puppeteer.py @@ -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 diff --git a/changedetectionio/run_basic_tests.sh b/changedetectionio/run_basic_tests.sh index f69ef75d2..f7e6f380d 100755 --- a/changedetectionio/run_basic_tests.sh +++ b/changedetectionio/run_basic_tests.sh @@ -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