Skip to content

fix(api): classify client-disconnect body-read errors separately from auth failures (rebase of #3414) - #3723

Open
AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:fix/api-classify-client-disconnect-v2
Open

AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:fix/api-classify-client-disconnect-v2

Conversation

@AdaAibaby

Copy link
Copy Markdown
Contributor

Rebased follow-up to #3414, which @ryan-sana closed with: "A rebased PR on current main, with a test for the disconnect case, would be very welcome." This is that rebase, adapted to the refactored error helper and with the requested tests.

Problem

kin-openapi reads the full request body (io.ReadAll) before calling any auth function. When that read fails — the client dropped the connection, or the server read deadline fired while the goroutine was queued under high concurrency — kin-openapi returns a RequestError{Reason:"reading failed"} wrapping a net.Error, carried inside the SecurityRequirementsError (because the body is read before auth runs).

On current main that network failure flows through sharedauth.ProcessSecurityErrors and is reported as a SecurityRequirementsError:

  • logged WARN,
  • counted as a 400/401 auth error in metrics,
  • marked as an error on the OTel span.

All three are misleading — nothing was wrong with the caller's credentials; the connection just went away.

Fix

  • New clientDisconnectReason helper: a RequestError{reading failed} whose cause is a net.Error is detected in MultiErrorHandler and tagged with clientDisconnectPrefix instead of going to ProcessSecurityErrors.
  • ErrorHandler recognises the prefix, records an informational span event (telemetry.ReportEvent, not an error), returns 499 Client Closed Request, and skips c.Errors — so it never looks like an auth failure in logs or metrics.
  • A reading failure with a non-network cause, and genuine auth errors, are untouched.

Adapted to current main

#3414 edited processCustomErrors in error.go; that was refactored into sharedauth.ProcessSecurityErrors (commit f83a684). This PR hooks the classification into MultiErrorHandler's SecurityRequirementsError case instead, keeping the auth package unchanged.

Tests (as requested)

  • TestMultiErrorHandlerClassifiesClientDisconnect — a net.Error body-read failure gets the disconnect prefix; a non-network read failure and a real missing-credential error do not.
  • TestErrorHandlerClientDisconnectReturns499 — the prefix yields 499, no response body, and nothing on c.Errors.
  • TestMultiToErrorHandlerClientDisconnectEndToEnd — both stages together: network failure → 499, non-network read failure → auth 401.

go test ./packages/api/internal/utils/ passes; go build ./... and go vet are clean.

Closes #3413. Supersedes #3414.

… auth failures

kin-openapi reads the full request body (io.ReadAll) before calling any auth
function. When that read fails - the client dropped the connection, or the
server read deadline fired while the goroutine was queued under high
concurrency - kin-openapi returns a RequestError{Reason:"reading failed"}
wrapping a net.Error, and that is carried inside the SecurityRequirementsError
because the body is read before auth runs.

On current main such a network failure flows through ProcessSecurityErrors and
is reported as a SecurityRequirementsError: logged WARN, counted as a 400/401
auth error in metrics, and marked as an error on the OTel span. All three are
misleading - nothing was wrong with the caller credentials; the connection
just went away.

Detect this case in MultiErrorHandler (new clientDisconnectReason helper): a
RequestError{reading failed} whose cause is a net.Error is tagged with
clientDisconnectPrefix instead of going to ProcessSecurityErrors. ErrorHandler
then records it as an informational span event (not an error) and returns 499
Client Closed Request, skipping c.Errors so it never looks like an auth
failure. A reading failure with a non-network cause, and genuine auth errors,
are untouched.

Rebased onto current main at the maintainers invitation on e2b-dev#3414, adapted to
the refactored error helper (processCustomErrors -> sharedauth.
ProcessSecurityErrors). Adds tests for the disconnect case as requested:
classification in MultiErrorHandler, the 499/no-c.Errors behaviour in
ErrorHandler, and an end-to-end pass through both stages.

Closes e2b-dev#3413. Supersedes e2b-dev#3414.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

api: client-disconnect body-read failures are misclassified as auth errors (400 WARN noise)

2 participants