Skip to content

Stop the link sanitizer from removing commercial support URLs on transient failures - #1050

Open
Valyrian-Code wants to merge 1 commit into
qgis:mainfrom
Valyrian-Code:fix/sanitize-keep-on-transient
Open

Valyrian-Code wants to merge 1 commit into
qgis:mainfrom
Valyrian-Code:fix/sanitize-keep-on-transient

Conversation

@Valyrian-Code

Copy link
Copy Markdown
Contributor

Continues the script reliability and test-coverage work (#1015, #1020, #1042).

Problem

scripts/sanitize_commercial_supports.py runs every Sunday and empties a listing's URL in data/commercial_support when the link looks broken; the workflow then commits the result to main. But check_url_redirects treated two ambiguous situations as a dead link:

if response.status_code != 200:
    return True            # 403, 429, 503... all counted as dead
...
except RequestException as e:
    return True             # timeout, DNS hiccup, connection reset: also dead

Since the script only ever empties URLs and never restores them, every false positive permanently removes a company's link from the site.

Evidence this is happening

All five URLs emptied by the last three scheduled runs are alive today, and all five pass this script's own check when run locally:

Emptied in URL Status today
1cc0f36 (Jun 7) csgis.de 200
1cc0f36 (Jun 7) envirosolutions.pl 200
c8910b4 (May 31) gismexico.com 200
6b40174 (May 24) clearmapping.co.uk 200
6b40174 (May 24) thinkwhere.com alive (redirect)

That points at transient conditions on the CI runner: timeouts, rate limiting, or bot protection answering the datacenter IP with 403/429/5xx.

Fix

Keep the URL whenever the check cannot give a clear verdict, which is the choice the code already makes for SSLError:

  • network-level exceptions (timeout, DNS, connection reset) keep the URL and log why;
  • 5xx server errors and access-restricted or transient statuses (401, 403, 405, 408, 429) keep the URL;
  • genuinely broken links (404 and other 4xx) and redirects to a different domain still empty it, unchanged.

Tests

test/test_sanitize_commercial_supports.py (on the #1015 harness, all network mocked): transient exceptions and statuses keep the URL, 404 and cross-domain redirects still empty it, same-domain http to https redirects are fine, and process_file empties only flagged lines while preserving formatting. The transient cases fail on the previous code, so the suite has teeth. Full test suite green (60 tests).

Happy to follow up by restoring the five wrongly emptied URLs above in a small data PR if you'd like.

check_url_redirects treated any RequestException (timeout, DNS hiccup,
connection reset) and any non-200 status as a dead link, so the weekly
sanitize job emptied working URLs in data/commercial_support and committed
the result. The script only ever empties URLs, it never restores them, so
each false positive permanently removed a listing's link.

All five URLs emptied by the last three scheduled runs are reachable today
and pass this script's own check when run locally, which points at transient
conditions on the runner: timeouts, rate limiting or bot protection
answering the datacenter IP with 403/429/5xx.

Keep the URL when the check cannot give a clear verdict, matching the
existing SSLError behaviour: network-level exceptions, 5xx server errors
and access-restricted or transient statuses (401, 403, 405, 408, 429) no
longer empty a URL. Genuinely broken links (404 and other 4xx) and
redirects to a different domain still do.

Add tests covering both directions; the transient cases fail on the
previous code.
Copilot AI review requested due to automatic review settings July 10, 2026 11:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Xpirix
Xpirix requested a review from timlinux July 21, 2026 05:59
@Xpirix

Xpirix commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

It would be nice to have your thoughts on this @timlinux .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants