Page.stopLoading stops the network, not script execution. A page whose JavaScript has
pegged the renderer's main thread keeps that thread indefinitely, and every CDP call that
needs to run script then queues behind it and never returns - page.content, the xPath
scraper, the favicon fetcher. The fetch dies at PUPPETEER_MAX_PROCESSING_TIMEOUT_SECONDS
having extracted nothing, with a core spinning the entire time.
Seen in production on a watch that failed every check for days: the renderer sat at
1.04-1.07 cores for the full 60s budget (sampled every 2s, flat), 33s of which was a
single unanswered Runtime.evaluate, and the watch logged "xpath_data length returned
empty" every time.
Nothing else recovers this state. Runtime.evaluate's own `timeout` parameter bounds an
evaluation once it starts, not time spent queued behind the running task - measured, it
still hung past 15s. Wrapping the call in asyncio.wait_for is worse than useless:
cancelling a pyppeteer request mid-flight leaves the connection unusable, with
"Protocol error: Target closed" on everything after it.
Runtime.terminateExecution is what releases the thread. Against a page that fires load
and then spins forever, through the real fetcher:
before: 60.6s, BrowserFetchTimedOut, 0 bytes content, no xpath_data, no screenshot
after: 5.6s, no exception, content + payload, xpath_data present, screenshot 8415b
and the renderer drops from 1.00 to 0.08 cores.
Safe at this point in the fetch: stopLoading has already declared "give me what
rendered", and the content-ready wait above has already had its chance to let late
JS-rendered content appear.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several places call gc.collect() after a check to keep C-level memory
(pyppeteer buffers, libxml2 documents, PIL, brotli) from accumulating.
Individually each is reasonable; run concurrently by many fetch workers they
become a storm. Every gc.collect() is a full stop-the-world pass that walks the
whole heap holding the GIL, so at FETCH_WORKERS=50 the process spends most of
its time stopped in the collector - which also starves each worker's asyncio
loop, leaving its CDP websocket unread.
Measured on 153 real puppeteer checks of a live site at FETCH_WORKERS=50, with
an `//div` include filter so the lxml document tree is realistic:
collects objects freed gc time checks/sec CPU/check RSS
one per call site 790 2,594,098 91.3s 0.766 1.373s 279.6MB
debounced to 1s 48 2,219,010 7.3s 1.433 0.731s 275.3MB
none at all 0 0 0.0s 1.503 0.685s 287.7MB
Debouncing keeps 86% of the reclamation for 6% of the collections: 1.9x the
throughput, half the CPU per check, gc down from 30.8% to 6.4% of wall time,
and a lower resident plateau than collecting every time. It works because the
collector is process-wide - any worker's collection breaks every other worker's
cycles too, so with many workers the calls are overwhelmingly redundant
duplicates rather than independently necessary.
Removing them entirely is slightly faster still, but it was the only
configuration whose RSS had not plateaued by the end of the run, so it is not
the default. EXPLICIT_GC_MIN_INTERVAL=0 restores the previous behaviour.
Collecting a younger generation was measured and rejected: gen 0 freed 1,136
objects against the full pass's 2,594,098, because objects surviving a 10-30s
fetch have already been promoted out of gen 0.
Two call sites are additionally fixed because they could never reclaim anything:
- Watch.py brotli: brotli.Compressor is not gc-tracked, so the collector cannot
see it - `del` frees it by refcount. Over 60 x 2.2MB compressions, RSS growth
was +0.9MB with neither mechanism, +0.2MB with gc.collect() alone, and +0.0MB
with malloc_trim() alone or with both. malloc_trim is the load-bearing line
and is kept; the collect cost ~31ms of stop-the-world per snapshot save for no
reclamation.
- puppeteer quit(): runs twice per check (run()'s finally, then the worker's
safety net) and nulls self.page/self.browser in its own finally blocks, so the
second call closes nothing and breaks no cycles yet still paid for a full
collection - 88 calls across 51 checks. Now only collects when it actually
closed something.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `locales` package was installed "For presenting price amounts correctly in
the restock/price detection overview", but nothing ever ran locale-gen. The
package only ships /etc/locale.gen; it does not build any locale. So published
images had only C, C.utf8 and POSIX available, which made the existing
`ENV LC_ALL=en_US.UTF-8` unsatisfiable:
$ docker run --rm --entrypoint bash ghcr.io/dgtlmoon/changedetection.io:latest \
-c 'locale -a'
C
C.utf8
POSIX
locale.setlocale() in flask_app.py therefore raised, was caught by the existing
`except locale.Error` and logged "Unable to set locale ... is not installed
maybe?", and the process stayed on the C locale. The knock-on effect is that the
format_number_locale / format_int_locale Jinja filters lose their thousands
separators, contradicting format_number_locale's own docstring:
before: format_number_locale(1234567.89) -> '1234567.89'
after: format_number_locale(1234567.89) -> '1,234,567.89'
Those filters render prices in the watchlist overview
(blueprint/watchlist/templates/watch-overview-single-row.html), so every
published release has been showing unseparated price amounts.
How it got this way: LC_ALL arrived on master in d1b1dd70f (#3340) without a
locale-gen to satisfy it. The matching `RUN locale-gen en_US.UTF-8` was written
back in 3f73695e7 (2024-07-22), but that commit only ever existed on the
unmerged 2486-charset-encoding branch, so master has never had it.
Generate one glibc locale per UI translation in changedetectionio/translations
rather than only en_US, so that operators can override LC_ALL / LANG to any
language the UI actually offers and get correct local formatting (de_DE gives
1.234.567,89, fr_FR gives 1 234 567,89). Territories for bare language codes
come from CLDR likely-subtags, not from uppercasing the code, which would have
been wrong for cs, ja, ko, uk and zh.
Costs ~21MB and ~16s of build time.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pressing Enter in the search box closed the modal without running the search;
you had to click Search with the mouse.
Implicit form submission fires a click at the submit button, and a
keyboard-synthesised click carries detail 0 and coordinates of 0,0. The
backdrop-click handler only tested the coordinates against the dialog's
bounding box, so 0,0 read as "outside" - it closed the dialog and blanked the
input while the click was still bubbling. By the time the submit ran, `q` was
empty and `required` rejected it, so nothing was searched.
Ignore clicks with detail 0 - only a real pointer can hit the backdrop.
Verified with Chromium against a local instance: Enter now lands on
/?q=<term>, and mouse submit, backdrop click, Escape and Enter-on-empty all
still behave.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The modal submitted the active tag as `tags=<uuid>`, but the watchlist filters on
`tag` - nothing reads `tags`. So searching from inside a tag view silently
searched every watch, despite the label promising "URL or Title in '<tag>'".
The test pulls the hidden field straight out of the rendered modal and feeds it
back to the watchlist, so the field name can't drift from the arg again.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* UI - Search - Fix search modal navigating to the host root on sub-path deployments
base_path was referenced by search-modal.js but never defined, so searching
always jumped to the host root instead of the X-Forwarded-Prefix sub-path.
* UI - Search modal - submit natively instead of rebuilding the URL in JS
Alternative to defining a `base_path` JS global: give the search form a
server-rendered `action`, so url_for() supplies the reverse-proxy sub-path the
same way every other link on the page already does, and let the browser submit.
Drops the submit handler and the Enter handler from search-modal.js - Enter in
the input reaches the footer's submit button via implicit submission, which also
runs the `required` validation the synthetic `new Event('submit')` skipped.
The hidden tag field is only rendered when a tag is active, so a plain search no
longer carries an empty value.
Also drops the nginx-job grep for the rendered markup - test_search.py already
covers the sub-path case, and asserting on an exact HTML attribute string from a
shell grep breaks on any unrelated edit to that tag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: ponstream24 <87808547+ponstream24@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Browser fetchers - Judge a fetch on the document we end up extracting, not the first navigation
The goal is to compare the text of the page the browser lands on, even when the site navigates
again after the first response. Both fetchers were bound to the first navigation, which shows up
as two different bugs:
1. pyppeteer hangs until the hard processing timeout. Its navigation watcher is bound to the
loaderId of the navigation it started, so when the site replaces that document the 'load' it
waits for never arrives for that loaderId. With timeout=0 and setDefaultNavigationTimeout(0)
there is nothing to break the wait, so goto() blocks until
PUPPETEER_MAX_PROCESSING_TIMEOUT_SECONDS (180s) kills the fetch and the watch records an empty
xpath_data - while the browser is sitting on a fully loaded page. Traced on slated.com:
0.24s goto start
0.74s main frame networkIdle loaderId=A0E0E0B2 <- never gets 'load'
2.72s main frame init loaderId=56D2B5EF <- re-navigated to get.slated.com
3.49s main frame load loaderId=56D2B5EF <- fires for the new document
25.2s goto still hanging, frame._loaderId is now 56D2B5EF
Now the navigation races goto() against the main frame firing 'load', bounded by
BROWSER_NAVIGATION_TIMEOUT_SECONDS (default 30), and falls back to the document we can see.
slated.com / getastra.com / addupsolutions.com went from a 180s timeout with no content to
200 with full content in 5-35s.
2. Both fetchers reported the status of the interstitial. A site that gates unseen visitors with
an error status plus a client-side redirect (reported against fotokoch.de: 503 + meta refresh,
then a 200 with the real page) failed the watch even though the content was present, and the
only workaround was ignore_status_codes, which also hides genuine 404s and 500s forever.
The fetchers now keep the latest main-frame document response and judge on that - the refresh
lands during the existing extra_wait, so the 200 wins.
Playwright also waits for a settled load state before extracting, which is what produced
"Execution context was destroyed, most likely because of a navigation" when the refresh collided
with extraction.
The navigation-response tracker is installed once per page and shared between the fetcher and
action_goto_url() rather than each navigation adding its own listener - 'response' fires once per
HTTP response, hundreds of times on a heavy page, so the callbacks are worth not duplicating.
Verified one listener remains after an install plus four navigations.
Selenium is unaffected either way - it hardcodes status_code = 200 because WebDriver cannot see
the HTTP status.
Tested: new test_renavigation.py covers the interstitial case end to end and was checked to fail
without the fix and pass with it, on both fetchers. The test endpoint gates on last-seen time
rather than a hit count, because a counter lets the second check see a clean 200 and the test
then passes without the fix. Full browser suite 11 passed on playwright and on pyppeteer, 494
unit tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
In minimal sidebar mode, count badges (unread changes, queue size)
were hidden until hover. This makes them always visible so users can
see at a glance how many items need attention without expanding the
rail.
Closes#4413
* 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>
* Watchlist/Notifications - Resolve 'Link to Open' once per row, don't leak 'DISABLED' into notification tokens
Follow-up to #4290.
`watch.open_link` was read three times per watch list row (two hrefs plus the
new title attribute). Each read re-runs `_resolve_link()`, which for a Jinja2
templated URL builds a fresh ImmutableSandboxedEnvironment - the cost the
comment in validate_url.py already warns about. Hoisted to a single
`{% set open_link = watch.open_link %}` alongside the other per-row lookups.
`watch_open_url` was set from `watch.open_link`, which returns the string
'DISABLED' when the URL fails validation, so the default RSS body template
(`RSS_TEMPLATE_HTML_DEFAULT`) rendered `<a href="DISABLED">` for those watches.
Now falls back to the raw URL, matching the neighbouring `watch_url` token.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Rebuild template
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Compare against a resolved data_dir in the history path-traversal test
Watch.history resolves entries with os.path.realpath, so
test_normal_snapshot_entry_is_accepted compared a resolved path against an
unresolved data_dir. On macOS the datastore lives under /tmp, which is a
symlink to /private/tmp, so the assertion fails for a path that is in fact
inside the directory. The guard is correct; the test was not.
Resolve both sides, matching what the production code does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Add a test that actually exercises the history containment check
Disabling the containment check in Watch.history left every test in
TestHistoryPathTraversal passing. os.path.basename() reduces both traversal
fixtures ('/etc/passwd', '../../etc/passwd') to 'passwd', so neither reaches
the check — they stop at the os.path.exists() test below it.
A bare '..' survives basename() and resolves to the parent of data_dir, which
exists, so the containment check is what rejects it. With that check disabled
this new test is the only one in the class that fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: GG5533 <285285461+GG5533@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(conditions): support zero values in condition filtering and json logic conversion
- Fix filter_complete_rules dropping rules where value is 0/0.0 due to 0 == False in Python
- Fix convert_to_jsonlogic raising EmptyConditionRuleRowNotUsable on value=0 due to truthiness check
- Fix str != 'None' type comparison typo in convert_to_jsonlogic
- Add comprehensive unit tests covering zero value condition filtering, conversion, and execution
* chore: re-trigger CI checks
---------
Co-authored-by: Andrew Peabody <apeabody@users.noreply.github.com>
`tag` on POST /watch was documented as taking a tag UUID, but the value went to
add_tag(title): a UUID silently created a junk tag *titled* with that UUID and never
applied the tag the caller asked for. `tags` (UUIDs) was the only thing that worked.
- `tag=` now resolves an existing tag UUID to that tag, still falling back to title
matching/creation for names. A UUID-shaped value matching nothing is skipped with a
warning rather than becoming a group named after a UUID.
- Blank tokens ("One,,Two,") no longer store False in watch['tags'] - add_tag() returns
False for an empty title and it was appended unguarded. Consumers tolerate it
(get_all_tags_for_watch() dictfilt()s over known tags) but it is not valid data.
- add_tag()'s title search is extracted to tag_uuid_for_title(), so existence can be
tested without creating as a side effect. add_tag()'s contract is unchanged.
- api-spec: `tag` is marked `deprecated: true` (so Redoc renders the badge) and states
plainly that it takes names, not UUIDs. `tags` now says what it really does - applied
verbatim, never creates, unknown UUIDs stored as dangling refs. Rendered docs rebuilt.
Every claim in the new field docs is asserted in test_api_tags.py against the real API.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
