New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[protocolv2] Handle invalid requests and internal server errors #203

Merged

masad-frost merged 1 commit into protocolv2 from fm-invalid-requests

Jun 20, 2024

Member

masad-frost commented Jun 15, 2024 •

edited

Loading

Why

Clients can send invalid requests for many reasons, most commonly due to backwards incompatible server changes, server should handle those and send back a stream abort with it.

What changed

Introduced INTERNAL_RIVER_ERROR code since some of the errors are invariant violations on the server
Split out stream request validation from stream handling
When we see a bad request, we send INVALID_REQUEST code with an abort bit
Made tracing createHandlerSpan accept tracing fields explicitly instead of a transport message

some more changes that I'll note inline

masad-frost requested a review from a team as a code owner

June 15, 2024 03:33

masad-frost requested review from jackyzha0 and removed request for a team

June 15, 2024 03:33

masad-frost commented

View reviewed changes

router/server.ts

@@ @@ -399,10 +395,6 @@ class RiverServer<Services extends AnyServiceSchemaMap> @@
                   const onHandlerError = (err: unknown, span: Span) => {
                     const errorMsg = coerceErrorString(err);
-                    this.log?.error(

Member Author

masad-frost Jun 15, 2024

stopped logging here, it's an application-level error, clients can choose to log this

router/server.ts

@@ @@ -558,11 +551,11 @@ class RiverServer<Services extends AnyServiceSchemaMap> @@
                       break;
                     default:
-                      this.log?.warn(
+                      this.log?.error(

Member Author

masad-frost Jun 15, 2024

promoted to error, this should never happen.

router/server.ts

+                    this.transport.sessionHandshakeMetadata.get(session);
+                  if (!sessionMetadata) {
+                    const errMessage = `session doesn't have handshake metadata`;
+                    this.log?.error(errMessage, {

Member Author

masad-frost Jun 15, 2024

promoted this from warn to error, as it's an internal error, we should never hit this in theory.

router/server.ts Show resolved Hide resolved

router/server.ts

+                  if (!initMessage.serviceName) {
+                    const errMessage = `missing service name in stream open message`;
+                    this.log?.warn(errMessage, {

Member Author

masad-frost Jun 15, 2024

demoted to warn, it's a client problem.

router/server.ts

+                  if (!Value.Check(procedure.init, initMessage.payload)) {
+                    const errMessage = `procedure init failed validation`;
+                    this.log?.warn(errMessage, {

Member Author

masad-frost Jun 15, 2024

demoted to warn, it's a client problem.

router/server.ts

+                        validationErrors.push(...Value.Errors(procedure.input, msg.payload));
+                      }
+                      this.log?.warn(errMessage, {

Member Author

masad-frost Jun 15, 2024

demoted to warn, it's a client problem.

router/server.ts

@@ @@ -315,23 +287,47 @@ class RiverServer<Services extends AnyServiceSchemaMap> @@
                     }
                     if (inputReader.isClosed()) {
-                      this.log?.error('Received message after input stream is closed', {
+                      this.log?.warn('Received message after input stream is closed', {

Member Author

masad-frost Jun 15, 2024

demoted to warn, it's a client problem.

Member

jackyzha0 Jun 17, 2024

is this an invariant violation / should we add that as a tag?

router/server.ts

@@ @@ -293,7 +264,8 @@ class RiverServer<Services extends AnyServiceSchemaMap> @@
                           code: ABORT_CODE,
                           message: 'Stream aborted, client sent invalid payload',
                         });
-                        this.log?.error('Got stream abort without a valid protocol error', {
+                        this.log?.warn('Got stream abort without a valid protocol error', {

Member Author

masad-frost Jun 15, 2024

demoted to warn, it's a client problem.

router/context.ts

                  */
-                session: Session<Connection>; // TODO: only expose a subset interface of session
+                from: TransportClientId;

Member Author

masad-frost Jun 15, 2024

this feels like the only thing a service implementer would might need from a session

Member

jackyzha0 Jun 17, 2024

lgtm

masad-frost force-pushed the fm-invalid-requests branch from 3b42ea6 to 3b43f71 Compare

June 15, 2024 03:41

masad-frost mentioned this pull request

Base branch for protocol v2 #127

Open

32 tasks

masad-frost force-pushed the fm-invalid-requests branch from 3b43f71 to 6618a10 Compare

June 15, 2024 19:40

jackyzha0 approved these changes

View reviewed changes

router/context.ts

                  */
-                session: Session<Connection>; // TODO: only expose a subset interface of session
+                from: TransportClientId;

Member

jackyzha0 Jun 17, 2024

lgtm

tracing/index.ts Show resolved Hide resolved

router/server.ts

@@ @@ -315,23 +287,47 @@ class RiverServer<Services extends AnyServiceSchemaMap> @@
                     }
                     if (inputReader.isClosed()) {
-                      this.log?.error('Received message after input stream is closed', {
+                      this.log?.warn('Received message after input stream is closed', {

Member

jackyzha0 Jun 17, 2024

is this an invariant violation / should we add that as a tag?

router/server.ts

+                        ...loggingMetadata,
+                        clientId: this.transport.clientId,
+                        transportMessage: msg,
+                        validationErrors,

Member

jackyzha0 Jun 17, 2024

same here: maybe we want to tag this with invariant violation?

router/server.ts Show resolved Hide resolved

router/server.ts Show resolved Hide resolved

masad-frost force-pushed the fm-add-cleanup branch from 938b625 to 5f339e3 Compare

June 18, 2024 00:46

Base automatically changed from fm-add-cleanup to protocolv2

June 18, 2024 00:48


          [protocolv2] Handle invalid requests and internal server errors

04eaac6

masad-frost force-pushed the fm-invalid-requests branch from 6618a10 to 04eaac6 Compare

June 18, 2024 00:49

masad-frost merged commit 791a3c4 into protocolv2

3 checks passed

masad-frost deleted the fm-invalid-requests branch

June 20, 2024 17:39

masad-frost mentioned this pull request

[protocolv2] Add invalid-request tag to logs #218

Merged

masad-frost added a commit that referenced this pull request


          [protocolv2] Add invalid-request tag to logs (#218)

eab068d

#203 (comment)

masad-frost added a commit that referenced this pull request


          [protocolv2] Handle invalid requests and internal server errors (#203)

f964d74

## Why

Clients can send invalid requests for many reasons, most commonly due to
backwards incompatible server changes, server should handle those and
send back a stream abort with it.

## What changed

- Introduced `INTERNAL_RIVER_ERROR` code since some of the errors are
invariant violations on the server
- Split out stream request validation from stream handling
- When we see a bad request, we send `INVALID_REQUEST` code with an
abort bit
- Made tracing `createHandlerSpan` accept tracing fields explicitly
instead of a transport message

some more changes that I'll note inline

masad-frost added a commit that referenced this pull request


          [protocolv2] Add invalid-request tag to logs (#218)

334056e

#203 (comment)

masad-frost added a commit that referenced this pull request


          [protocolv2] Handle invalid requests and internal server errors (#203)

298ada6

## Why

Clients can send invalid requests for many reasons, most commonly due to
backwards incompatible server changes, server should handle those and
send back a stream abort with it.

## What changed

- Introduced `INTERNAL_RIVER_ERROR` code since some of the errors are
invariant violations on the server
- Split out stream request validation from stream handling
- When we see a bad request, we send `INVALID_REQUEST` code with an
abort bit
- Made tracing `createHandlerSpan` accept tracing fields explicitly
instead of a transport message

some more changes that I'll note inline

masad-frost added a commit that referenced this pull request


          [protocolv2] Add invalid-request tag to logs (#218)

7453a3d

#203 (comment)

masad-frost added a commit that referenced this pull request


          [protocolv2] Handle invalid requests and internal server errors (#203)

2d08c4c

## Why

Clients can send invalid requests for many reasons, most commonly due to
backwards incompatible server changes, server should handle those and
send back a stream abort with it.

## What changed

- Introduced `INTERNAL_RIVER_ERROR` code since some of the errors are
invariant violations on the server
- Split out stream request validation from stream handling
- When we see a bad request, we send `INVALID_REQUEST` code with an
abort bit
- Made tracing `createHandlerSpan` accept tracing fields explicitly
instead of a transport message

some more changes that I'll note inline

masad-frost added a commit that referenced this pull request


          [protocolv2] Add invalid-request tag to logs (#218)

57a0656

#203 (comment)

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