Stop repeated probes when server-type detection fails; keep the HTTP response on errors - #97
Closed
VanDelinea wants to merge 1 commit into
Closed
Stop repeated probes when server-type detection fails; keep the HTTP response on errors#97VanDelinea wants to merge 1 commit into
VanDelinea wants to merge 1 commit into
Conversation
…P response on errors
Server-type detection probes /api/v1/healthcheck and /health without
authentication. Three gaps remained after the detection cache landed.
1. No braking while an endpoint is down or blocked.
Only successful detections were cached, so when probing failed nothing was
remembered and every new authorizer probed again at full rate - adding load
to an endpoint that was already refusing traffic. A failed detection now
pauses probing for that base_url: 5 seconds, doubling on repeated failures,
up to 60. Measured against an endpoint that rejects every probe, 20
authorizer constructions dropped from 20 probe rounds to 1.
The pause is deliberately short-lived: a successful detection clears it, it
always expires on its own, clear_server_type_cache() clears it, and passing
an explicit server_type skips it entirely. A permanent block would turn a
brief outage into a lasting one, which is worse than the extra probes.
2. Errors hid the HTTP response.
SecretServerError accepted a response and then discarded it, so callers had
no way to tell an expired token (401, worth one retry) from a denial or a
rate limit (403 or 429, where retrying only makes things worse). The
response is now kept on the exception.
3. Some error bodies crashed instead of raising a normal error.
A 4xx response whose body was JSON but not in the expected shape - for
example {"error": {...}} or a bare number - left an internal variable unset
and raised UnboundLocalError or TypeError from inside process(). That
bypassed every `except SecretServerError` handler in calling code, so users
saw a raw traceback instead of a clear failure. All paths now raise a proper
error with a usable message.
Tests: 13 new tests (26 total in the detection suite). Existing tests are
unchanged and still pass.
⛔ Snyk checks have failed. 4 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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
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.
What this changes
Server-type detection probes
/api/v1/healthcheckand/healthwithout authentication. Building on the detection cache infeature/server-detection, this closes three gaps.1. Nothing slowed the probes down while an endpoint was down or blocked
Only successful detections were cached. When probing failed, nothing was remembered, so every new authorizer probed again at full rate — adding load to an endpoint that was already refusing traffic. This is the situation reported in ADO 734475: an Ansible run against a Platform tenant whose edge starts returning
403to the probes.A failed detection now pauses probing for that
base_url: 5 seconds, doubling on repeated failures, capped at 60.Measured against a local endpoint that rejects every probe, 20 authorizer constructions:
The pause is deliberately short-lived — a successful detection clears it, it always expires on its own,
clear_server_type_cache()clears it, and passing an explicitserver_typeskips it entirely. A permanent block would turn a brief outage into a lasting one, which is worse than the extra probes. It shares the existing cache's lock and size limit, so it is not a new source of unbounded growth.2. Errors hid the HTTP response
SecretServerErroraccepted aresponseargument and then discarded it. Callers had no way to tell an expired token (401, worth one retry) from a denial or rate limit (403/429, where retrying makes things worse). The response is now kept on the exception, so callers can classify the failure instead of guessing.3. Some error bodies crashed instead of raising a normal error
A
4xxresponse whose body was JSON but not in the expected shape — for example{"error": {...}}, or a bare123— left an internal variable unset and raisedUnboundLocalErrororTypeErrorfrom insideprocess(). That bypassed everyexcept SecretServerErrorhandler in calling code, so users saw a raw traceback instead of a clear failure. All paths now raise a proper error with a usable message.Tests
13 new tests, 26 total in the detection suite. Existing tests are unchanged and still pass; the pre-existing live-credential tests in
tests/test_server.pystill require credentials, exactly as before.New coverage: probe throttling, pause expiry and recovery, backoff growth and cap, a success clearing the pause, size bounding, error wording,
server_typebypassing the pause, the response contract, and four malformed-body shapes.Notes for reviewers
feature/server-detection, notmain— this stacks on that work rather than competing with it.403/429handling cannot run, and it falls back to a more conservative rule.