Merge pull request #1699 from Bilb/improve-502-handlin

Improve 502 handlin

ts/session/onions/onionPath.ts

@@ -156,9 +156,9 @@ export async function getOnionPath(toExclude?: Snode): Promise<Array<Snode>> {
 /**
  * If we don't know which nodes is causing trouble, increment the issue with this full path.
  */
-export async function incrementBadPathCountOrDrop(sndeEd25519: string) {
+export async function incrementBadPathCountOrDrop(snodeEd25519: string) {
   const pathWithSnodeIndex = onionPaths.findIndex(path =>
-    path.some(snode => snode.pubkey_ed25519 === sndeEd25519)
+    path.some(snode => snode.pubkey_ed25519 === snodeEd25519)
   );
 
   if (pathWithSnodeIndex === -1) {
@@ -187,7 +187,7 @@ export async function incrementBadPathCountOrDrop(sndeEd25519: string) {
   // a guard node is dropped when the path is dropped completely (in dropPathStartingWithGuardNode)
   for (let index = 1; index < pathWithIssues.length; index++) {
     const snode = pathWithIssues[index];
-    await incrementBadSnodeCountOrDrop({ snodeEd25519: snode.pubkey_ed25519 });
+    await incrementBadSnodeCountOrDrop({ snodeEd25519: snode.pubkey_ed25519, guardNodeEd25519 });
   }
 
   if (newPathFailureCount >= pathFailureThreshold) {

ts/session/onions/onionSend.ts

@@ -190,7 +190,7 @@ export const sendViaOnion = async (
     );
   } catch (e) {
     window?.log?.warn('sendViaOnionRetryable failed ', e);
-    console.warn('error to show to user', e);
+    // console.warn('error to show to user', e);
     return null;
   }
 

ts/session/snode_api/onions.ts

@@ -211,11 +211,17 @@ function processOxenServerError(statusCode: number, body?: string) {
 async function process421Error(
   statusCode: number,
   body: string,
+  guardNodeEd25519: string,
   associatedWith?: string,
   lsrpcEd25519Key?: string
 ) {
   if (statusCode === 421) {
-    await handle421InvalidSwarm({ snodeEd25519: lsrpcEd25519Key, body, associatedWith });
+    await handle421InvalidSwarm({
+      snodeEd25519: lsrpcEd25519Key,
+      guardNodeEd25519,
+      body,
+      associatedWith,
+    });
   }
 }
 
@@ -228,11 +234,13 @@ async function process421Error(
 async function processOnionRequestErrorAtDestination({
   statusCode,
   body,
+  guardNodeEd25519,
   destinationEd25519,
   associatedWith,
 }: {
   statusCode: number;
   body: string;
+  guardNodeEd25519: string;
   destinationEd25519?: string;
   associatedWith?: string;
 }) {
@@ -242,10 +250,16 @@ async function processOnionRequestErrorAtDestination({
   window?.log?.info('processOnionRequestErrorAtDestination. statusCode nok:', statusCode);
 
   process406Error(statusCode);
-  await process421Error(statusCode, body, associatedWith, destinationEd25519);
+  await process421Error(statusCode, body, guardNodeEd25519, associatedWith, destinationEd25519);
   processOxenServerError(statusCode, body);
   if (destinationEd25519) {
-    await processAnyOtherErrorAtDestination(statusCode, body, destinationEd25519, associatedWith);
+    await processAnyOtherErrorAtDestination(
+      statusCode,
+      body,
+      guardNodeEd25519,
+      destinationEd25519,
+      associatedWith
+    );
   }
 }
 
@@ -277,6 +291,7 @@ async function processAnyOtherErrorOnPath(
     if (nodeNotFound) {
       await exports.incrementBadSnodeCountOrDrop({
         snodeEd25519: nodeNotFound,
+        guardNodeEd25519,
         associatedWith,
       });
 
@@ -291,6 +306,7 @@ async function processAnyOtherErrorOnPath(
 async function processAnyOtherErrorAtDestination(
   status: number,
   body: string,
+  guardNodeEd25519: string,
   destinationEd25519: string,
   associatedWith?: string
 ) {
@@ -309,6 +325,7 @@ async function processAnyOtherErrorAtDestination(
       if (nodeNotFound) {
         await exports.incrementBadSnodeCountOrDrop({
           snodeEd25519: destinationEd25519,
+          guardNodeEd25519,
           associatedWith,
         });
         // if we get a nodeNotFound at the desitnation. it means the targetNode to which we made the request is not found.
@@ -325,6 +342,7 @@ async function processAnyOtherErrorAtDestination(
     // if (nodeNotFound) {
     await exports.incrementBadSnodeCountOrDrop({
       snodeEd25519: destinationEd25519,
+      guardNodeEd25519,
       associatedWith,
     });
 
@@ -343,7 +361,13 @@ async function processOnionRequestErrorOnPath(
     window?.log?.warn('errorONpath:', ciphertext);
   }
   process406Error(httpStatusCode);
-  await process421Error(httpStatusCode, ciphertext, associatedWith, lsrpcEd25519Key);
+  await process421Error(
+    httpStatusCode,
+    ciphertext,
+    guardNodeEd25519,
+    associatedWith,
+    lsrpcEd25519Key
+  );
   await processAnyOtherErrorOnPath(httpStatusCode, guardNodeEd25519, ciphertext, associatedWith);
 }
 
@@ -457,6 +481,7 @@ export async function processOnionResponse({
     await processOnionRequestErrorAtDestination({
       statusCode: status,
       body: jsonRes?.body, // this is really important. the `.body`. the .body should be a string. for isntance for nodeNotFound but is most likely a dict (Record<string,any>))
+      guardNodeEd25519: guardNode.pubkey_ed25519,
       destinationEd25519: lsrpcEd25519Key,
       associatedWith,
     });
@@ -503,9 +528,11 @@ function isSnodeResponse(arg: any): arg is SnodeResponse {
 async function handle421InvalidSwarm({
   body,
   snodeEd25519,
+  guardNodeEd25519,
   associatedWith,
 }: {
   body: string;
+  guardNodeEd25519: string;
   snodeEd25519?: string;
   associatedWith?: string;
 }) {
@@ -543,7 +570,7 @@ async function handle421InvalidSwarm({
       await dropSnodeFromSwarmIfNeeded(associatedWith, snodeEd25519);
     }
   }
-  await exports.incrementBadSnodeCountOrDrop({ snodeEd25519, associatedWith });
+  await exports.incrementBadSnodeCountOrDrop({ snodeEd25519, guardNodeEd25519, associatedWith });
 
   // this is important we throw so another retry is made and we exit the handling of that reponse
   throw new pRetry.AbortError(exceptionMessage);
@@ -556,17 +583,23 @@ async function handle421InvalidSwarm({
  *
  * So after this call, if the snode keeps getting errors, we won't contact it again
  *
- * @param snodeEd25519 the snode ed25519 which cause issues
+ * @param snodeEd25519 the snode ed25519 which cause issues (this might be a nodeNotFound)
+ * @param guardNodeEd25519 the guard node ed25519 of the current path in use. a nodeNoteFound ed25519 is not part of any path, so we fallback to this one if we need to increment the bad path count of the current path in use
  * @param associatedWith if set, we will drop this snode from the swarm of the pubkey too
  * @param isNodeNotFound if set, we will drop this snode right now as this is an invalid node for the network.
  */
 export async function incrementBadSnodeCountOrDrop({
   snodeEd25519,
+  guardNodeEd25519,
   associatedWith,
 }: {
   snodeEd25519: string;
+  guardNodeEd25519: string;
   associatedWith?: string;
 }) {
+  if (!guardNodeEd25519) {
+    console.warn('We need a guardNodeEd25519 at all times');
+  }
   const oldFailureCount = snodeFailureCount[snodeEd25519] || 0;
   const newFailureCount = oldFailureCount + 1;
   snodeFailureCount[snodeEd25519] = newFailureCount;
@@ -593,8 +626,10 @@ export async function incrementBadSnodeCountOrDrop({
         'dropSnodeFromPath, got error while patching up... incrementing the whole path as bad',
         e
       );
-      // if dropSnodeFromPath throws, it means there is an issue patching up the path, increment the whole path issues count
+      // If dropSnodeFromPath throws, it means there is an issue patching up the path, increment the whole path issues count
-      await OnionPaths.incrementBadPathCountOrDrop(snodeEd25519);
+      // but using the guardNode we got instead of the snodeEd25519.
+      //
+      await OnionPaths.incrementBadPathCountOrDrop(guardNodeEd25519);
     }
   } else {
     window?.log?.warn(
@@ -839,7 +874,7 @@ export async function lokiOnionFetch(
     return retriedResult;
   } catch (e) {
     window?.log?.warn('onionFetchRetryable failed ', e);
-    console.warn('error to show to user');
+    // console.warn('error to show to user');
     throw e;
   }
 }

ts/test/session/unit/onion/OnionErrors_test.ts

@@ -287,6 +287,7 @@ describe('OnionPathsErrors', () => {
           expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
           expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
             snodeEd25519: targetNode,
+            guardNodeEd25519: guardSnode1.pubkey_ed25519,
             associatedWith,
           });
         });
@@ -331,6 +332,7 @@ describe('OnionPathsErrors', () => {
           expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
           expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
             snodeEd25519: targetNode,
+            guardNodeEd25519: guardSnode1.pubkey_ed25519,
             associatedWith,
           });
         });
@@ -368,6 +370,7 @@ describe('OnionPathsErrors', () => {
           expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
           expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
             snodeEd25519: targetNode,
+            guardNodeEd25519: guardSnode1.pubkey_ed25519,
             associatedWith,
           });
         });
@@ -407,6 +410,7 @@ describe('OnionPathsErrors', () => {
           expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
           expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
             snodeEd25519: targetNode,
+            guardNodeEd25519: guardSnode1.pubkey_ed25519,
             associatedWith,
           });
         });
@@ -483,6 +487,7 @@ describe('OnionPathsErrors', () => {
       expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
       expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
         snodeEd25519: failingSnode.pubkey_ed25519,
+        guardNodeEd25519: guardSnode1.pubkey_ed25519,
         associatedWith,
       });
     });
@@ -518,6 +523,7 @@ describe('OnionPathsErrors', () => {
       expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(1);
       expect(incrementBadSnodeCountOrDropSpy.firstCall.args[0]).to.deep.eq({
         snodeEd25519: failingSnode.pubkey_ed25519,
+        guardNodeEd25519: guardSnode1.pubkey_ed25519,
         associatedWith,
       });
     });
@@ -560,14 +566,17 @@ describe('OnionPathsErrors', () => {
       expect(incrementBadSnodeCountOrDropSpy.callCount).to.eq(3);
       expect(incrementBadSnodeCountOrDropSpy.args[0][0]).to.deep.eq({
         snodeEd25519: failingSnode.pubkey_ed25519,
+        guardNodeEd25519: guardSnode1.pubkey_ed25519,
         associatedWith,
       });
       expect(incrementBadSnodeCountOrDropSpy.args[1][0]).to.deep.eq({
         snodeEd25519: failingSnode.pubkey_ed25519,
+        guardNodeEd25519: guardSnode1.pubkey_ed25519,
         associatedWith,
       });
       expect(incrementBadSnodeCountOrDropSpy.args[2][0]).to.deep.eq({
         snodeEd25519: failingSnode.pubkey_ed25519,
+        guardNodeEd25519: guardSnode1.pubkey_ed25519,
         associatedWith,
       });
       expect(incrementBadPathCountOrDropSpy.callCount).to.eq(0);
@@ -610,6 +619,7 @@ describe('OnionPathsErrors', () => {
     for (let index = 0; index < 6; index++) {
       expect(incrementBadSnodeCountOrDropSpy.args[index][0]).to.deep.eq({
         snodeEd25519: oldOnionPaths[0][(index % 2) + 1].pubkey_ed25519,
+        guardNodeEd25519: guardNode.pubkey_ed25519,
       });
     }