fix(mattermost): remove substring auth misclassification + escalate genuine WS auth failures - #85157
Merged
Merged
Conversation
The WS reconnect loop had a fallback check that looked for "401", "403",
or "unauthorized" as substrings anywhere in an exception's string form.
A transient error whose message happens to contain those digits (a proxy
body, a stack trace, anything) got treated as a permanent auth failure
and stopped reconnection for good.
I removed the substring fallback and kept only the structured check:
aiohttp.WSServerHandshakeError with status in {401, 403}. That's the only
signal that reliably means the server rejected our credentials.
Added two regression tests: one proving a transient error containing
"401" in its text still retries, and one confirming the existing
_closing early-return path is untouched by the removal.
…ify fix Independent-verifier boundary probes for commit fdd1a11ac5, covering cases the implementer's regression tests did not exercise: - WSServerHandshakeError(status=403) also stops the loop (only 401 tested) - WSServerHandshakeError(status=500) does NOT stop the loop (structured check must not over-match on type alone) - transient error containing the word 'unauthorized' (not digit substring) now retries correctly - 5 consecutive transient errors all retry, not just the first Verified these 2nd/4th tests fail against the pre-fix baseline commit (01a1037) and pass against the fix (fdd1a11ac5), confirming they have real signal.
…error hook Follow-up to the salvaged #80489 substring-fallback removal: the structured 401/403 branch still exited with a bare return, leaving _running True — dead listener, healthy-looking is_connected(), gateway never told (the zombie half of the bug, OOF-156 class). It now sets a non-retryable mattermost_auth_error with token guidance and notifies the gateway fatal handler. Also: pytest.importorskip for aiohttp in the verifier probe file (module-level import crashed collection in envs without the optional dep), and probe fixtures updated for the escalation attributes.
Contributor
૮ >ﻌ< ა ci reviewran on bd99426 — fix(mattermost): escalate genuine WS auth failures through t
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A transient error whose message merely contains "401" no longer permanently kills the Mattermost WebSocket listener — and a genuine auth rejection now escalates through the fatal-error hook instead of dying silently while
is_connected()keeps reporting healthy.Salvages #80489 by @steveonjava (authorship preserved via cherry-pick, both commits) onto current main, plus our follow-up closing the zombie half.
Changes
From #80489 (cherry-picked):
_ws_loop()that classified any exception mentioning "401"/"403"/"unauthorized" as a permanent failure (proxy error bodies, 502 pages quoting an upstream 401, etc.). The structuredWSServerHandshakeError+status in {401, 403}check — the only true auth rejection — is keptFollow-ups (ours):
return:_runningstayed True, the gateway was never told — the zombie-listener half of the bug (same class as fix(gateway): classify terminal adapter connect failures + escalate long-lived retry loops (OOF-156) #85049's connect-path fixes). It now sets a non-retryablemattermost_auth_errorwith token guidance and notifies the gateway fatal handlerpytest.importorskip("aiohttp")in the probe file — its module-levelimport aiohttpcrashed collection in envs without the optional depValidation
Closes #80489. Related: #35645 keeps its API-classification/lock/attachment scope; its substring-escalation half is superseded here.
Infographic