fix (replicache): handle non 200 errors correctly (#1464)

rocicorp · Mar 5, 2024 · 39daea0 · 39daea0 · arv · Mar 5, 2024
1 parent 287b8f9
commit 39daea0
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 21 deletions.
diff --git a/packages/replicache/src/replicache-pull.test.ts b/packages/replicache/src/replicache-pull.test.ts
@@ -186,7 +186,7 @@ test('reauth pull', async () => {
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.lastCall,
-    `Got error response doing pull: 401: xxx`,
+    `Got a non 200 response doing pull: 401: xxx`,
     ['pull', requestIDLogContextRegex],
   );
   {
@@ -273,7 +273,7 @@ test('pull request is only sent when pullURL or non-default puller are set', asy
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.firstCall,
-    'Got error response doing pull: 500: Test failure',
+    'Got a non 200 response doing pull: 500: Test failure',
     ['pull', requestIDLogContextRegex],
   );
   consoleErrorStub.restore();

diff --git a/packages/replicache/src/replicache.test.ts b/packages/replicache/src/replicache.test.ts
@@ -442,7 +442,7 @@ test('reauth push', async () => {
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.firstCall,
-    'Got error response doing push: 401: xxx',
+    'Got a non 200 response doing push: 401: xxx',
     ['push', requestIDLogContextRegex],
   );
 
@@ -481,6 +481,10 @@ test('HTTP status pull', async () => {
         return {body: 'internal error', status: 500};
       case 1:
         return {body: 'not found', status: 404};
+      case 2:
+        return {body: 'created', status: 201};
+      case 3:
+        return {status: 204};
       default: {
         okCalled = true;
         return {body: makePullResponseV1(clientID, 0), status: 200};
@@ -492,19 +496,31 @@ test('HTTP status pull', async () => {
 
   rep.pullIgnorePromise({now: true});
 
-  await tickAFewTimes(20, 10);
+  await tickAFewTimes(60, 10);
 
-  expect(consoleErrorStub.callCount).to.equal(2);
+  expect(consoleErrorStub.callCount).to.equal(4);
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.firstCall,
-    'Got error response doing pull: 500: internal error',
+    'Got a non 200 response doing pull: 500: internal error',
+    ['pull', requestIDLogContextRegex],
+  );
+  expectConsoleLogContextStub(
+    rep.name,
+    consoleErrorStub.secondCall,
+    'Got a non 200 response doing pull: 404: not found',
+    ['pull', requestIDLogContextRegex],
+  );
+  expectConsoleLogContextStub(
+    rep.name,
+    consoleErrorStub.thirdCall,
+    'Got a non 200 response doing pull: 201: created',
     ['pull', requestIDLogContextRegex],
   );
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.lastCall,
-    'Got error response doing pull: 404: not found',
+    'Got a non 200 response doing pull: 204',
     ['pull', requestIDLogContextRegex],
   );
 
@@ -529,6 +545,10 @@ test('HTTP status push', async () => {
         return {body: 'internal error', status: 500};
       case 1:
         return {body: 'not found', status: 404};
+      case 2:
+        return {body: 'created', status: 201};
+      case 3:
+        return {status: 204};
       default:
         okCalled = true;
         return {body: {}, status: 200};
@@ -541,19 +561,31 @@ test('HTTP status push', async () => {
     a: 0,
   });
 
-  await tickAFewTimes(20, 10);
+  await tickAFewTimes(60, 10);
 
-  expect(consoleErrorStub.callCount).to.equal(2);
+  expect(consoleErrorStub.callCount).to.equal(4);
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.firstCall,
-    'Got error response doing push: 500: internal error',
+    'Got a non 200 response doing push: 500: internal error',
+    ['push', requestIDLogContextRegex],
+  );
+  expectConsoleLogContextStub(
+    rep.name,
+    consoleErrorStub.secondCall,
+    'Got a non 200 response doing push: 404: not found',
+    ['push', requestIDLogContextRegex],
+  );
+  expectConsoleLogContextStub(
+    rep.name,
+    consoleErrorStub.thirdCall,
+    'Got a non 200 response doing push: 201: created',
     ['push', requestIDLogContextRegex],
   );
   expectConsoleLogContextStub(
     rep.name,
     consoleErrorStub.lastCall,
-    'Got error response doing push: 404: not found',
+    'Got a non 200 response doing push: 204',
     ['push', requestIDLogContextRegex],
   );
 
@@ -1297,7 +1329,7 @@ test('onSync', async () => {
     expectConsoleLogContextStub(
       rep.name,
       consoleErrorStub.firstCall,
-      'Got error response doing push: 401: xxx',
+      'Got a non 200 response doing push: 401: xxx',
       ['push', requestIDLogContextRegex],
     );
 

diff --git a/packages/replicache/src/replicache.ts b/packages/replicache/src/replicache.ts
@@ -1124,11 +1124,11 @@ export class Replicache<MD extends MutatorDefs = {}> {
       }
       const {errorMessage, httpStatusCode} = httpRequestInfo;
 
-      if (errorMessage || httpStatusCode >= 400) {
+      if (errorMessage || httpStatusCode !== 200) {
         // TODO(arv): Maybe we should not log the server URL when the error comes
         // from a Pusher/Puller?
         requestLc.error?.(
-          `Got error response doing ${verb}: ${httpStatusCode}` +
+          `Got a non 200 response doing ${verb}: ${httpStatusCode}` +
             (errorMessage ? `: ${errorMessage}` : ''),
         );
       }
@@ -1774,7 +1774,7 @@ function reload(): void {
 /**
  * Wrapper error class that should be reported as error (logger.error)
  */
-class ReportError extends Error {}
+export class ReportError extends Error {}
 
 async function throwIfError(p: Promise<undefined | {error: unknown}>) {
   const res = await p;

diff --git a/packages/replicache/src/sync/pull.ts b/packages/replicache/src/sync/pull.ts
@@ -51,6 +51,7 @@ import type {ClientGroupID, ClientID} from './ids.js';
 import * as patch from './patch.js';
 import {PullError} from './pull-error.js';
 import {SYNC_HEAD_NAME} from './sync-head-name.js';
+import {ReportError} from '../replicache.js';
 
 export const PULL_VERSION_SDD = 0;
 export const PULL_VERSION_DD31 = 1;
@@ -270,23 +271,26 @@ async function callPuller(
 ): Promise<PullerResult> {
   lc.debug?.('Starting pull...');
   const pullStart = Date.now();
+  let pullerResult: PullerResult;
   try {
-    const pullerResult = await puller(pullReq, requestID);
+    pullerResult = await puller(pullReq, requestID);
     lc.debug?.(
       `...Pull ${pullerResult.response ? 'complete' : 'failed'} in `,
       Date.now() - pullStart,
       'ms',
     );
-
+  } catch (e) {
+    throw new PullError(toError(e));
+  }
+  try {
     if (isPullRequestV1(pullReq)) {
       assertPullerResultV1(pullerResult);
     } else {
       assertPullerResultV0(pullerResult);
     }
-
     return pullerResult;
   } catch (e) {
-    throw new PullError(toError(e));
+    throw new ReportError('Invalid puller result', toError(e));
   }
 }
 

diff --git a/packages/replicache/src/sync/push.ts b/packages/replicache/src/sync/push.ts
@@ -27,6 +27,7 @@ import {
   type ClientGroupID,
   type ClientID,
 } from './ids.js';
+import {ReportError} from '../replicache.js';
 
 export const PUSH_VERSION_SDD = 0;
 export const PUSH_VERSION_DD31 = 1;
@@ -245,11 +246,16 @@ async function callPusher(
   body: PushRequestV0 | PushRequestV1,
   requestID: string,
 ): Promise<PusherResult> {
+  let pusherResult: PusherResult;
+  try {
+    pusherResult = await pusher(body, requestID);
+  } catch (e) {
+    throw new PushError(toError(e));
+  }
   try {
-    const pusherResult = await pusher(body, requestID);
     assertPusherResult(pusherResult);
     return pusherResult;
   } catch (e) {
-    throw new PushError(toError(e));
+    throw new ReportError('Invalid pusher result', toError(e));
   }
 }