Name the class of a transport failure, not just "request failed" - #64
Merged
YellowSnnowmann merged 1 commit intoAug 19, 2026
Conversation
A remote store against Cognee's hosted endpoint reported:
memory API request to api.cognee.ai failed
which is true and useless. The endpoint accepted TCP and then aborted
the TLS handshake; the operator cannot tell that from a DNS typo, a
firewall, or a service that is simply down, and each sends them to a
different runbook. `reqwest`'s own Display is one clause -- "error
sending request" -- and the cause lives one or more `source()` hops
down, which a host logging only the top line never sees.
Failures now name their class and carry the chain:
memory API request to api.cognee.ai: TLS failed — the endpoint
answered on the port but could not establish a secure connection;
check that the URL is the engine's real API host (client error
(Connect): received fatal alert: InternalError)
memory API request to no-such-host.invalid: the host could not be
resolved — check the URL (dns error: failed to lookup address
information: nodename nor servname provided)
memory API request to 127.0.0.1: could not connect — check the URL
and that the service is reachable (tcp connect error: Connection
refused (os error 61))
Two things a live run taught that reasoning did not:
- `is_connect()` is ALSO true for DNS and TLS failures, so testing it
first collapses every class into "could not connect". That is what
the first version did. The specific classes are checked first now.
- rustls never says "TLS". A handshake abort reads "received fatal
alert: InternalError", so matching on the word would have missed the
exact case this was written for. The needles match rustls' wording.
Both traps are pinned by tests. The classification is a free function
purely so the ORDER can be tested; reintroducing the is_connect-first
bug fails `a_rustls_handshake_abort_is_named_tls_not_connect`, verified
by doing it.
The credential is never included -- only the host, the class, and the
transport chain.
cargo test -p tinymemory-remote: 24 passed (19 + 5 ordering tests)
cargo clippy --all-targets: clean
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Found by testing the demo UI against a real hosted engine. A store against Cognee's cloud endpoint reported:
True and useless. The endpoint accepted TCP then aborted the TLS handshake — indistinguishable, from that message, from a DNS typo, a firewall, or a dead service. Each sends an operator to a different runbook.
reqwest's own Display is one clause ("error sending request"); the cause lives one or moresource()hops down, where a host logging the top line never sees it.After
… api.cognee.ai: TLS failed — the endpoint answered on the port but could not establish a secure connection; check that the URL is the engine's real API host (client error (Connect): received fatal alert: InternalError)… : the host could not be resolved — check the URL (dns error: failed to lookup address information…)… : could not connect — check the URL and that the service is reachable (tcp connect error: Connection refused)… : timed outTwo things the live run taught that reasoning did not
is_connect()is also true for DNS and TLS failures. Testing it first collapses every class into "could not connect" — which is exactly what my first version did, and it looked correct until run against a real broken endpoint.received fatal alert: InternalError. Matching on the word "tls" would have missed the precise case this was written for; the needles match rustls' actual wording.Both traps are pinned. The classifier is a free function purely so the order is testable — I reintroduced the
is_connect-first bug to confirma_rustls_handshake_abort_is_named_tls_not_connectfails, then restored it.Credential is never included: host, class, and transport chain only.
Validation:
cargo test -p tinymemory-remote24 passed (19 existing + 5 ordering tests), clippy clean, fmt clean. Also verified end-to-end through the demo consumer against the live failing endpoint.