Carry the errno through the proxy server's error payload - #2249
Merged
Merged
Conversation
kmcginnes
added this pull request to stack #2253
September 23, 2026 21:40
Base automatically changed from
proxy-server-test-environment-factory
to
main
September 23, 2026 22:12
kmcginnes
force-pushed
the
proxy-server-errno-in-error-payload
branch
2 times, most recently
from
September 23, 2026 23:36
b5241db to
917c01e
Compare
kmcginnes
removed this pull request from stack #2253
September 23, 2026 23:36
extractErrorInfo dropped a plain Error's code and cause.code, so the client's createDisplayError could never match the ECONNREFUSED and ECONNRESET branches it already had. An unresolvable host and a refused port both surfaced as a generic "Network Response 500". Carry code and cause.code through the response, each guarded on typeof === "string" so no stack, path, or other cause property rides along, and give ENOTFOUND, ETIMEDOUT, and EAI_AGAIN a message of their own.
kmcginnes
force-pushed
the
proxy-server-errno-in-error-payload
branch
from
September 24, 2026 00:04
917c01e to
c071e6f
Compare
kmcginnes
added this pull request to stack #2256
September 24, 2026 00:08
error-handler.test.ts and logging.test.ts each built their own, with the same app.locals.logger shape. Each request now gets its own logger so a test can spy on it without leaking onto another test.
node-fetch assigns the errno to code and never sets cause, so the nested field was unreachable. Resolving code from the error or its cause covers the same ground with one field, and keeps the nested object out of the wire format.
ETIMEDOUT shared the DNS message, which told the user to check a hostname that had already resolved. The name resolved and nothing answered, so the remedy is a security group, a firewall, or the port.
Both titles are new, and the troubleshooting guide had nothing to find when a user searched for them. Each gets its cause and its fix, since a DNS failure and a connect timeout point at different things.
kmcginnes
marked this pull request as ready for review
September 24, 2026 18:06
This was referenced Sep 25, 2026
kmcginnes
added a commit
that referenced
this pull request
Sep 25, 2026
## 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 `DOMException` named `TimeoutError`. A database-side query timeout surfaced as a `NetworkError` whose body happened to carry `TimeLimitExceededException`. 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: - `FetchTimeoutError` carries `timeoutMs`. It's thrown when the fetch timeout signal fired and the caller's signal didn't, so a user cancel stays an `AbortError` even 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 raw `DOMException`. - `DatabaseTimeoutError` extends `NetworkError` and carries the database's `databaseCode`. Staying a `NetworkError` keeps the retry policy, the error details dialog, and the `requestId` in the details unchanged. It matches on the body, never on status alone, since Neptune also returns 500 for memory limits, throttling, and cancellation: - Neptune: `code: "TimeLimitExceededException"` - Gremlin Server: `"Exception-Class": "java.util.concurrent.TimeoutException"` `createDisplayError` renders 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. `cancelQueries` reverted 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 passes `revert: false`, so the cancel lands on the cancellation branch. `CONTEXT.md` names the two timeouts, and `docs/agents/connectors.md` records that `fetchDatabaseRequest` is the only place they are classified. `MemoryLimitExceededException` and the `ETIMEDOUT` errno that [#2249](#2249) now carries are not query timeouts, and stay plain `NetworkError`s. Not changed: timeouts are still retried 3 times by the query client. ## Validation - The Gremlin Server body is a real capture from `tinkerpop/gremlin-server:3.8`. A request-level `evaluationTimeout` field is ignored over HTTP, but `g.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. - The Neptune shape matches the existing `TimeLimitExceededException` handling. - `fetchDatabaseRequest` tests cover: - the timer firing - the caller aborting first, then the timeout, and the reverse, with both signals aborted before the catch runs - the timeout firing while an error body is read, which keeps the database's error - a timeout during `response.json()` - Neptune bodies with and without the proxy's `error` wrapper - `MemoryLimitExceededException` and `ETIMEDOUT` staying `NetworkError` - Each explorer has a test for both errors through `rawQuery`. - Live captures matched the classification on Neptune 1.2.1.0, 1.3.5.0, 1.4.5.1 and 1.4.7.0 (Gremlin), 1.4.5.1 and 1.4.7.0 (SPARQL), and 1.4.7.0 (openCypher), each returning HTTP 500 with `TimeLimitExceededException`, directly and through the proxy. Earlier versions reject a per-query openCypher timeout. - In the browser, both messages showed through the proxy on Gremlin Server and Neptune, and a cancel with a fetch timeout set stayed a cancel. - `pnpm checks` and `pnpm test` are clean: 227 files, 2779 tests. ## Related Issues None. [#2244](#2244) will adopt these so edge connection discovery can tell the user which timeout to raise. ### Check List - [x] I confirm that my contribution is made under the terms of the Apache 2.0 license. - [x] I have verified `pnpm checks` passes with no errors. - [x] I have verified `pnpm test` passes with no failures. - [x] I have covered new added functionality with unit tests if necessary. - [x] I have updated documentation if necessary.
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.
Description
extractErrorInfoinerror-handler.tsreturned only{ status, message }for a plainError, dropping the errno. The client'screateDisplayErroralready hadECONNREFUSEDandECONNRESETbranches keyed ondata.codeanddata.cause?.code, so for anything routed through the proxy server those branches were dead code. An unresolvable hostname and a refused port both surfaced as a generic "Network Response 500".Two changes:
extractErrorInfocarriescodeandcause.codethrough the response payload. Each is guarded ontypeof === "string"and nothing else is copied, so a stack trace, filesystem path, or any othercauseproperty cannot ride along.createDisplayErrorgains a branch forENOTFOUND,ETIMEDOUT, andEAI_AGAINunder one "Database unreachable" message. The wording deliberately does not name who made the failing request, because a non-proxy connection has the browser talking to the database directly.The existing
ECONNREFUSEDandECONNRESETmessages are unchanged.Validation
A new
error-handler.test.tspins the payload shape, including a case whosecausecarriesstack,hostname,path, andsyscalland asserts onlycodesurvives. The client-side tests are built from the exact payloadextractErrorInfonow sends, so they fail if the two sides drift.pnpm checksandpnpm testclean: 223 files, 2720 tests.Related Issues
None.
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.