fix(connections): give the key check a deadline and the executor's error mapping - #91
Merged
Conversation
…ror mapping Validating an api key was the one outbound call with no timeout and no rate-limit handling. A vendor that accepted the connection and never answered held an Express handler for undici's default, and a 429 came back as a 502 with no retry_after, so a client retried straight into the limit. The cause was a second error mapping living beside the executor's. The arms readResponse already had are now a shared throwIfFailed, and validateKey routes through it, so bodyFailure, the header rate-limit rule, 402, 429 with its retry hint and the vendor's own reason all arrive without being written twice. The two callers differ on one argument: a stored credential that stopped working is reauth_required, and a key that arrived in the request being served is invalid_arguments. The deadline is AbortSignal.timeout rather than the raced promise withTimeout uses, because this has to cancel the request instead of only stopping the wait, and followRedirects spreads init so the signal survives every hop. Ten seconds rather than the thirty a tool call gets: somebody is holding an HTTP request open on this one. Closes #90
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.
Validating an api key was the one outbound call with no timeout and no rate-limit handling. A vendor that accepted the connection and never answered held an Express handler for undici's default, and a 429 came back as a 502 with no
retry_after, so a client retried straight into the limit.The cause was a second error mapping living beside the executor's, so I removed the second one rather than adding two arms to it. The arms
readResponsealready had are now a sharedthrowIfFailed, andvalidateKeyroutes through it.bodyFailure, the header rate-limit rule, 402, 429 with its retry hint, and the vendor's own reason text all arrive without being written twice. The two callers differ on one argument: a stored credential that stopped working isreauth_required, a key that arrived in the request being served isinvalid_arguments.The deadline is
AbortSignal.timeoutrather than the raced promisewithTimeoutuses (call-tool.ts:160), because this has to cancel the request rather than only stop waiting on it, andfollowRedirectsspreadsinitso the signal survives every hop. Ten seconds rather than the thirty a tool call gets, since somebody is holding an HTTP request open on this one.Closes #90
Evidence
The extraction changed no behaviour: 451 passed with no test edited before anything new was added. Four tests then cover what was missing, and two of the existing ones already guard the shared path.
Broken deliberately, signal dropped and the credential code swapped to the executor's:
Restored, full suite:
What I did not do
The timeout is asserted by checking the signal reaches the vendor and that an abort maps to
upstream_timeout, not by letting a real ten second deadline elapse in the suite. No live vendor was called.