Give up on a hanging vendor before the device gives up on us - #35
Merged
Merged
Conversation
The session forwarding to the vendor had no ClientTimeout, so it inherited aiohttp's five-minute default. That was tolerable while this only relayed -- the device gives up and retries on its own, and nothing here depended on answering quickly. It is no longer only that. Since the local token answer, a token request is relayed first and answered locally only if that relay fails, so a vendor that hangs rather than refuses would hold the minted answer far past the point where the device has stopped listening. The device abandons a request after about ten seconds -- device_got=499 at 10.003s in the proxy's access log -- so the answer whose entire value is arriving in time would not arrive at all. Four seconds, roughly three times the slowest healthy vendor response measured at the shim, leaving room for the existing retry without either attempt approaching the device's limit. A hang is deliberately not retried: the retry is for a vendor that answers nothing quickly, where replaying costs milliseconds, and replaying a hang only doubles the wait. The second half matters more than the timeout. A timeout is not a ClientError, so it escaped the relay's handlers entirely: the device got aiohttp's 504 rather than an honest 502, the message was never parsed -- losing a reading already in hand, which is the whole point of running this locally -- and no vendor failure was recorded, so the outage shape that most needs a locally minted token would never have got one.
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.
Closes #29.
upstream_session()created its session with noClientTimeout, so itinherited aiohttp's five-minute default. Fine while this only relayed. Not fine
since #21, where a token request is relayed first and answered locally only if
that relay fails — a vendor that hangs rather than refuses would hold the
minted answer far past the point where the device has stopped listening.
Two corrections to the issue, both from measurement
The device's patience is 10 seconds, not 11. The shim log shows
device_got=499at 10.003 s — the device closing the request. 11 s is itsretry interval; 10 s is where it stops listening.
The bigger problem was not the wait.
asyncio.TimeoutErroris not anaiohttp.ClientError, so a timeout escaped_relay's handlers entirely. Threeconsequences at once, and the middle one is the worst:
whole point of running this locally,
locally minted token would never have opened the gate to get one.
Adding the timeout without catching it would have made that reachable rather
than theoretical.
What shipped
VENDOR_TIMEOUT_SECONDS = 4on the session — ~3× the slowest healthy vendorresponse measured at the shim (0.1–1.3 s), and two attempts still fit inside
the device's 10 s.
_relaycatchesTimeoutErrorbesideClientError, with a written-outreason since a bare
TimeoutErrorstringifies to nothing.FIN-with-no-response, where replaying costs milliseconds; replaying a hang
doubles the only thing that matters. This falls out of catching
ServerDisconnectedErroralone, and is now pinned by a test rather than leftimplicit.
Tests
A
hanging_upstreamfixture — accepts the request, answers nothing, holds theconnection — since the existing
flaky_upstreammodels the hangup, which isa different failure. Four new tests, each verified against the unfixed code:
the two hang tests do not merely fail without the timeout, they sit for five
minutes, which is the defect stated as an experiment.
213 pass.
core/still imports nohomeassistant.