Distinguish fetch timeouts from database query timeouts - #2268
Merged
Merged
Conversation
Adds FetchTimeoutError (client-side fetch timeout) and DatabaseTimeoutError (database-side query timeout) so all three connectors can tell users which side stopped the request instead of a generic "deadline exceeded" or "TimeoutError" message. fetchDatabaseRequest now keeps the fetch-timeout signal separate from the caller's abort signal and classifies a caught error by which signal actually fired, so a user cancellation is never reported as a timeout. Response body reads are covered by the same classification, since a timeout can also fire while streaming the body. databaseTimeoutCode() recognizes Neptune's TimeLimitExceededException and a Gremlin Server evaluation timeout, captured from a local tinkerpop/gremlin-server container.
Reclassify a caught error as a fetch timeout by comparing the combined signal's reason to the fetch-timeout signal's reason, rather than checking both signals' aborted flags after the fact, which cannot tell who fired first once both end up aborted. Skip a NetworkError/DatabaseTimeoutError already built from a received response body, even if the timeout also fired while that body was being read. Cancel a running query with revert: false so a cancelled query lands in the cancellation branch instead of reverting to an earlier failed query's error and re-showing it as current. Also: pin the FetchTimeoutError message format (comma-grouped ms, trailing period), extract the duplicated abort-aware fetch mock into utils/testing/abortableFetch, and update the CONTEXT.md glossary and connector/troubleshooting docs for Fetch Timeout vs Database Query Timeout.
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.
Description
A timeout reached the user as one of two things, and neither said who gave up. The connection's own fetch timeout surfaced as a bare
DOMExceptionnamedTimeoutError. A database-side query timeout surfaced as aNetworkErrorwhose body happened to carryTimeLimitExceededException. They have opposite fixes: one is a setting on the connection, the other is the database's configuration. Anything that wanted to give specific advice had to sniff error names and body codes on its own.This adds two errors, thrown from
fetchDatabaseRequest, which every connector (Gremlin, openCypher, SPARQL) already goes through:FetchTimeoutErrorcarriestimeoutMs. It's thrown when the fetch timeout signal fired and the caller's signal didn't, so a user cancel stays anAbortErroreven if both have fired by the time it's caught. It now also covers a timeout that fires while the body is being read, which used to escape as a rawDOMException.DatabaseTimeoutErrorextendsNetworkErrorand carries the database'sdatabaseCode. Staying aNetworkErrorkeeps the retry policy, the error details dialog, and therequestIdin the details unchanged. It matches on the body, never on status alone, since Neptune also returns 500 for memory limits, throttling, and cancellation:code: "TimeLimitExceededException""Exception-Class": "java.util.concurrent.TimeoutException"createDisplayErrorrenders each with its own advice and drops the old name and code sniffing. The cancellation message no longer claims it might have been a timeout, since a timeout can't reach that branch any more. The troubleshooting guide explains the two messages.Cancelling a Query tab query no longer restores the previous query's error.
cancelQueriesreverted to the last cached state by default, so a cancel right after a failure showed that failure again, which with this change could read "Database query timed out". Cancel now passesrevert: false, so the cancel lands on the cancellation branch.CONTEXT.mdnames the two timeouts, anddocs/agents/connectors.mdrecords thatfetchDatabaseRequestis the only place they are classified.MemoryLimitExceededExceptionand theETIMEDOUTerrno that #2249 now carries are not query timeouts, and stay plainNetworkErrors.Not changed: timeouts are still retried 3 times by the query client.
Validation
tinkerpop/gremlin-server:3.8. A request-levelevaluationTimeoutfield is ignored over HTTP, butg.with("evaluationTimeout", N)in the script works, and both the script limit and the server's 30s default produce the same body. Tests use that exact body.TimeLimitExceededExceptionhandling.fetchDatabaseRequesttests cover:response.json()errorwrapperMemoryLimitExceededExceptionandETIMEDOUTstayingNetworkErrorrawQuery.TimeLimitExceededException, directly and through the proxy. Earlier versions reject a per-query openCypher timeout.pnpm checksandpnpm testare clean: 227 files, 2779 tests.Related Issues
None. #2244 will adopt these so edge connection discovery can tell the user which timeout to raise.
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.