From 8b23a35d3f75b3419742ece8fbf25537bb6b4716 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Sat, 12 Sep 2026 12:46:16 +0200 Subject: [PATCH] Puppeteer fetcher - One content-ready deadline instead of a stopLoading watchdog per frame event Page.stopLoading is what stops a page that would otherwise load forever waiting on a subresource that never answers, so that we can still screenshot and scrape what rendered. That intent was right, but it was implemented as a fire-and-forget task armed by every frame event, which measured on a single fetch of an iframe-heavy page came to: 14 watchdog tasks spawned 11 page-wide Page.stopLoading calls 3 tasks outliving the fetch and firing against a closed page Page.stopLoading takes no frame or loader argument - it is the Stop button, and it stops the whole page. Verified directly: one call stopped a pending main frame and a pending iframe in the same instant. So the other 10 calls were redundant, and because they landed at arbitrary later times they could stop a *subsequent* navigation we actually wanted - which is the likeliest reason the same URL fetched in 8s on one run and 35s on the next. Replaced with a single deadline, awaited inline so nothing can outlive the fetch (there is no create_task left in this file at all): navigate (bounded) -> wait the configured delay -> Page.stopLoading -> extract The delay is measured from when navigation finished, not from when it started. Anchoring it to the start would quietly rob a slow-loading page of its settle time, and letting JS-rendered content appear after load is the whole point of the setting. Verified with a server that takes 5s to answer and renders via JS 2s after load: total 9.6s for a 4s delay, and the late content is captured. Because a page is never reliably "finished" - many sites navigate as part of their normal design - the delay restarts when the MAIN frame replaces its document, so a redirect or interstitial gets the same settle time the first document got. Iframes do not restart it, and it is capped by BROWSER_CONTENT_READY_MAX_RESETS (default 2). Only the existing "wait n seconds before extracting text" stays user-facing; BROWSER_NAVIGATION_TIMEOUT_SECONDS is a safety net with a sane default rather than a second knob for users to reason about. This matches what other scrapers do: bound the navigation, do not fail when it times out, settle, then extract. Timings are also more predictable now - the four reported URLs went from 5-35s of variance to 4.6-7.2s at a 3s delay, all with full content and a 200. Tested: 11 passed pyppeteer browser suite, 7 passed playwright, 494 unit + llm. Co-Authored-By: Claude Opus 5 (1M context) --- .../content_fetchers/puppeteer.py | 90 ++++++++++++------- 1 file changed, 56 insertions(+), 34 deletions(-) diff --git a/changedetectionio/content_fetchers/puppeteer.py b/changedetectionio/content_fetchers/puppeteer.py index 747ed08f3..13e9ee0d0 100644 --- a/changedetectionio/content_fetchers/puppeteer.py +++ b/changedetectionio/content_fetchers/puppeteer.py @@ -399,36 +399,59 @@ class fetcher(Fetcher): # Enable Network domain to detect when first bytes arrive await self.page._client.send('Network.enable') - # Now set up the frame navigation handlers - async def handle_frame_navigation(event=None): - # Wait n seconds after the frameStartedLoading, not from any frameStartedLoading/frameStartedNavigating - logger.debug(f"Frame navigated: {event}") - w = extra_wait - 2 if extra_wait > 4 else 2 - logger.debug(f"Waiting {w} seconds before calling Page.stopLoading...") - await asyncio.sleep(w) + # Navigate (bounded), then wait the configured "wait n seconds before extracting text" + # delay, then stop whatever is still loading, then extract. The delay is measured from + # when navigation finished, not from when it started, because the point of it is to let + # JS-rendered content appear *after* load - anchoring it to the start would quietly give a + # slow-loading page almost no settle time. + # + # Only that delay is a user-facing setting. The navigation bound above is a safety net with + # a sane default, not a tuning knob, so there is still one number for users to think about. + # + # There is no way to know a page is "finished" - plenty of sites navigate as part of their + # normal design, and some sit forever on a subresource that never answers. So the delay + # restarts if the MAIN frame replaces its document (a redirect or interstitial gets the + # same settle time the first document got), iframes do not restart it, and it is capped so + # a page that re-navigates in a loop cannot extend it indefinitely. + max_content_ready_resets = int(os.getenv("BROWSER_CONTENT_READY_MAX_RESETS", 2)) - # Check if page still exists (might have been closed due to error during sleep) - if not self.page or not hasattr(self.page, '_client'): - logger.debug("Page already closed, skipping stopLoading") - return + async def wait_for_content_ready_then_stop_loading(): + main_frame_id = self.page.mainFrame._id + renavigated = asyncio.Event() - logger.debug("Issuing stopLoading command...") - await self.page._client.send('Page.stopLoading') - logger.debug("stopLoading command sent!") + def _on_main_frame_navigation(event): + if event.get('frameId') == main_frame_id: + renavigated.set() - async def setup_frame_handlers_on_first_response(event): - # Only trigger for the main document response - if event.get('type') == 'Document': - logger.debug("First response received, setting up frame handlers for forced page stop load.") - self.page._client.on('Page.frameStartedNavigating', lambda e: asyncio.create_task(handle_frame_navigation(e))) - self.page._client.on('Page.frameStartedLoading', lambda e: asyncio.create_task(handle_frame_navigation(e))) - self.page._client.on('Page.frameStoppedLoading', lambda e: logger.debug(f"Frame stopped loading: {e}")) - logger.debug("First response received, setting up frame handlers for forced page stop load DONE SETUP") - # De-register this listener - we only need it once - self.page._client.remove_listener('Network.responseReceived', setup_frame_handlers_on_first_response) + self.page._client.on('Page.frameStartedLoading', _on_main_frame_navigation) + self.page._client.on('Page.frameStoppedLoading', lambda e: logger.debug(f"Frame stopped loading: {e}")) + try: + resets = 0 + while True: + renavigated.clear() + try: + await asyncio.wait_for(renavigated.wait(), timeout=extra_wait) + # Main frame started a new document + resets += 1 + if resets > max_content_ready_resets: + logger.debug(f"Main frame keeps re-navigating, not restarting the content-ready wait again") + break + logger.debug(f"Main frame started a new document, restarting the {extra_wait}s " + f"content-ready wait ({resets}/{max_content_ready_resets})") + except asyncio.TimeoutError: + # Quiet for the whole delay - the page is as ready as it is going to get + break + finally: + self.page._client.remove_listener('Page.frameStartedLoading', _on_main_frame_navigation) - # Listen for first response to trigger frame handler setup - self.page._client.on('Network.responseReceived', setup_frame_handlers_on_first_response) + # Stop whatever is still in flight so the DOM and screenshot come from what rendered, + # rather than waiting on a subresource that may never answer + try: + logger.debug(f"Content-ready wait of {extra_wait}s elapsed, issuing Page.stopLoading before extracting") + await self.page._client.send('Page.stopLoading') + logger.debug("stopLoading command sent!") + except Exception as e: + logger.debug(f"Page.stopLoading skipped, page is most likely already gone: {e}") # Track the LATEST main-frame document response for the whole fetch, not just the one that # goto() happens to return. This app compares the text of the page the browser ends up on, @@ -461,7 +484,6 @@ class fetcher(Fetcher): try: while not response: logger.debug(f"Attempting page fetch {url} attempt {attempt}") - asyncio.create_task(handle_frame_navigation()) # Race goto() against the main frame actually firing 'load'. In the re-navigation # case goto() can never resolve, but the replacement document does fire 'load' - # usually within a few seconds - so this returns then instead of sitting out the @@ -505,11 +527,6 @@ class fetcher(Fetcher): for t in (goto_task, load_task): if not t.done(): t.cancel() - 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: @@ -519,8 +536,13 @@ class fetcher(Fetcher): raise EmptyReply(url=url, status_code=None) attempt+=1 - # The sleep above is where a meta-refresh interstitial typically swaps in the real page, - # so re-check which document we are actually on before judging the status code. + # Navigation is done; now honour "wait n seconds before extracting text" and then + # force-stop whatever is still loading, so extraction always gets what rendered. + # Awaited inline rather than fired off as a task, so nothing can outlive the fetch. + await wait_for_content_ready_then_stop_loading() + + # That wait is where a meta-refresh interstitial typically swaps in the real page, so + # re-check which document we are actually on before judging the status code. latest = navigation_response.get('response') if latest is not None and latest is not response: logger.debug(f"Page navigated again while waiting, judging the fetch on {latest.url} "