feat: skip presigned url step + analytics (#5542)

packages/services/cdn-worker/src/analytics.ts

@@ -57,12 +57,15 @@ type Event =
   | {
       type: 'r2';
       action:
-        | 'HEAD artifact'
+        | 'GET artifact'
         | 'GET cdn-legacy-keys'
         | 'GET cdn-access-token'
         | 'GET persistedOperation'
         | 'HEAD appDeploymentIsEnabled';
-      statusCode: number;
+      // Either 3 digit status code or error code e.g. timeout, http error etc.
+      statusCodeOrErrCode: number | string;
+      /** duration in milliseconds */
+      duration: number;
     }
   | {
       type: 'response';
@@ -97,7 +100,8 @@ export function createAnalytics(
           });
         case 'r2':
           return engines.r2.writeDataPoint({
-            blobs: [event.action, event.statusCode.toString(), targetId],
+            blobs: [event.action, event.statusCodeOrErrCode.toString(), targetId],
+            doubles: [event.duration],
             indexes: [targetId.substring(0, 32)],
           });
         case 'response':

packages/services/cdn-worker/src/artifact-handler.ts

@@ -129,7 +129,7 @@ export const createArtifactRequestHandler = (deps: ArtifactRequestHandler) => {
 
     const eTag = request.headers.get('if-none-match');
 
-    const result = await deps.artifactStorageReader.generateArtifactReadUrl(
+    const result = await deps.artifactStorageReader.readArtifact(
       params.targetId,
       params.contractName,
       params.artifactType,

packages/services/cdn-worker/src/artifact-storage-reader.ts

@@ -65,11 +65,16 @@ export class ArtifactStorageReader {
       endpoint: string;
       bucketName: string;
     },
+    // private s3Mirror: {
+    //   client: AwsClient;
+    //   endpoint: string;
+    //   bucketName: string;
+    // },
     private analytics: Analytics | null,
   ) {}
 
-  /** Generate a pre-signed url for reading an artifact from a bucket for a limited time period. */
-  async generateArtifactReadUrl(
+  /** Read an artifact from S3 */
+  async readArtifact(
     targetId: string,
     contractName: string | null,
     artifactType: ArtifactsType,
@@ -81,55 +86,57 @@ export class ArtifactStorageReader {
 
     const key = buildArtifactStorageKey(targetId, artifactType, contractName);
 
-    const headResponse = await this.s3.client.fetch(
+    const headers: HeadersInit = {};
+
+    if (etagValue) {
+      headers['if-none-match'] = etagValue;
+    }
+
+    const response = await this.s3.client.fetch(
       [this.s3.endpoint, this.s3.bucketName, key].join('/'),
       {
-        method: 'HEAD',
+        method: 'GET',
+        headers,
         aws: {
           signQuery: true,
         },
         timeout: READ_TIMEOUT_MS,
-      },
-    );
-    this.analytics?.track(
-      {
-        type: 'r2',
-        statusCode: headResponse.status,
-        action: 'HEAD artifact',
-      },
-      targetId,
-    );
-
-    if (headResponse.status === 200) {
-      if (etagValue && headResponse.headers.get('etag') === etagValue) {
-        return { type: 'notModified' } as const;
-      }
-
-      const getResponse = await this.s3.client.fetch(
-        [this.s3.endpoint, this.s3.bucketName, key].join('/'),
-        {
-          method: 'GET',
-          aws: {
-            signQuery: true,
-          },
-          timeout: READ_TIMEOUT_MS,
+        onAttempt: args => {
+          this.analytics?.track(
+            {
+              type: 'r2',
+              statusCodeOrErrCode:
+                args.result.type === 'error'
+                  ? String(args.result.error.name ?? 'unknown')
+                  : args.result.response.status,
+              action: 'GET artifact',
+              duration: args.duration,
+            },
+            targetId,
+          );
         },
-      );
+      },
+    );
 
-      if (getResponse.ok) {
-        return {
-          type: 'response',
-          response: getResponse,
-        } as const;
-      }
-
-      throw new Error(`GET request failed with status ${getResponse.status}`);
-    }
-    if (headResponse.status === 404) {
+    if (response.status === 404) {
       return { type: 'notFound' } as const;
     }
-    const body = await headResponse.text();
-    throw new Error(`HEAD request failed with status ${headResponse.status}: ${body}`);
+
+    if (response.status === 304) {
+      return {
+        type: 'notModified',
+      } as const;
+    }
+
+    if (response.status === 200) {
+      return {
+        type: 'response',
+        response,
+      } as const;
+    }
+
+    const body = await response.text();
+    throw new Error(`GET request failed with status ${response.status}: ${body}`);
   }
 
   async isAppDeploymentEnabled(targetId: string, appName: string, appVersion: string) {
@@ -143,16 +150,22 @@ export class ArtifactStorageReader {
           signQuery: true,
         },
         timeout: READ_TIMEOUT_MS,
+        onAttempt: args => {
+          this.analytics?.track(
+            {
+              type: 'r2',
+              statusCodeOrErrCode:
+                args.result.type === 'error'
+                  ? String(args.result.error.name ?? 'unknown')
+                  : args.result.response.status,
+              action: 'HEAD appDeploymentIsEnabled',
+              duration: args.duration,
+            },
+            targetId,
+          );
+        },
       },
     );
-    this.analytics?.track(
-      {
-        type: 'r2',
-        statusCode: response.status,
-        action: 'HEAD appDeploymentIsEnabled',
-      },
-      targetId,
-    );
 
     return response.status === 200;
   }
@@ -180,18 +193,23 @@ export class ArtifactStorageReader {
         },
         headers,
         timeout: READ_TIMEOUT_MS,
+        onAttempt: args => {
+          this.analytics?.track(
+            {
+              type: 'r2',
+              statusCodeOrErrCode:
+                args.result.type === 'error'
+                  ? String(args.result.error.name ?? 'unknown')
+                  : args.result.response.status,
+              action: 'GET persistedOperation',
+              duration: args.duration,
+            },
+            targetId,
+          );
+        },
       },
     );
 
-    this.analytics?.track(
-      {
-        type: 'r2',
-        statusCode: response.status,
-        action: 'GET persistedOperation',
-      },
-      targetId,
-    );
-
     if (etagValue && response.status === 304) {
       return { type: 'notModified' } as const;
     }
@@ -211,4 +229,62 @@ export class ArtifactStorageReader {
     const body = await response.text();
     throw new Error(`HEAD request failed with status ${response.status}: ${body}`);
   }
+
+  async readLegacyAccessKey(targetId: string) {
+    const response = await this.s3.client.fetch(
+      [this.s3.endpoint, this.s3.bucketName, 'cdn-legacy-keys', targetId].join('/'),
+      {
+        method: 'GET',
+        timeout: READ_TIMEOUT_MS,
+        onAttempt: args => {
+          this.analytics?.track(
+            {
+              type: 'r2',
+              statusCodeOrErrCode:
+                args.result.type === 'error'
+                  ? String(args.result.error.name ?? 'unknown')
+                  : args.result.response.status,
+              action: 'GET cdn-legacy-keys',
+              duration: args.duration,
+            },
+            targetId,
+          );
+        },
+      },
+    );
+
+    return response;
+  }
+
+  async readAccessKey(targetId: string, keyId: string) {
+    const s3KeyParts = ['cdn-keys', targetId, keyId];
+
+    const response = await this.s3.client.fetch(
+      [this.s3.endpoint, this.s3.bucketName, ...s3KeyParts].join('/'),
+      {
+        method: 'GET',
+        aws: {
+          // This boolean makes Google Cloud Storage & AWS happy.
+          signQuery: true,
+        },
+        timeout: READ_TIMEOUT_MS,
+        onAttempt: args => {
+          this.analytics?.track(
+            {
+              type: 'r2',
+              statusCodeOrErrCode:
+                args.result.type === 'error'
+                  ? String(args.result.error.name ?? 'unknown')
+                  : args.result.response.status,
+              action: 'GET cdn-access-token',
+              duration: args.duration,
+            },
+            targetId,
+          );
+        },
+      },
+    );
+
+    return response;
+  }
 }

packages/services/cdn-worker/src/aws.ts

@@ -49,6 +49,25 @@ type AwsRequestInit = RequestInit & {
    * Timeout in milliseconds for each fetch call.
    */
   timeout?: number;
+  /** Hook being invoked for each attempt for gathering analytics or similar. */
+  onAttempt?: (args: {
+    /** attempt number */
+    attempt: number;
+    /** attempt duration in ms */
+    duration: number;
+    /** result */
+    result:
+      | {
+          // HTTP or other unexpected error
+          type: 'error';
+          error: Error;
+        }
+      | {
+          // HTTP response sent by upstream server
+          type: 'success';
+          response: Response;
+        };
+  }) => void;
 };
 
 export type AWSClientConfig = {
@@ -82,7 +101,7 @@ export class AwsClient {
     this.service = args.service;
     this.region = args.region;
     this.cache = args.cache || new Map();
-    this.retries = args.retries != null ? args.retries : 10; // Up to 25.6 secs
+    this.retries = args.retries != null ? args.retries : 3;
     this.initRetryMs = args.initRetryMs || 50;
     this._fetch = args.fetch || fetch.bind(globalThis);
   }
@@ -129,18 +148,31 @@ export class AwsClient {
 
   async fetch(input: RequestInfo, init: AwsRequestInit): Promise<Response> {
     for (let i = 0; i <= this.retries; i++) {
-      const fetched = this._fetch(...(await this.sign(input, init)));
-      if (i === this.retries) {
-        return fetched; // No need to await if we're returning anyway
-      }
+      const attemptStart = performance.now();
       try {
-        const res = await fetched;
-        if (res.status < 500 && res.status !== 429 && res.status !== 499) {
-          return res;
+        const response = await this._fetch(...(await this.sign(input, init)));
+        const duration = performance.now() - attemptStart;
+        init.onAttempt?.({ attempt: i, duration, result: { type: 'success', response } });
+
+        if (
+          (response.status < 500 && response.status !== 429 && response.status !== 499) ||
+          i === this.retries
+        ) {
+          return response;
         }
       } catch (error) {
+        const duration = performance.now() - attemptStart;
         // Retry also when there's an exception
         console.error(error);
+        init.onAttempt?.({
+          attempt: i,
+          duration,
+          result: { type: 'error', error: error as Error },
+        });
+
+        if (i === this.retries) {
+          throw error;
+        }
       }
       await new Promise(resolve =>
         setTimeout(resolve, Math.random() * this.initRetryMs * Math.pow(2, i)),

packages/services/cdn-worker/src/dev.ts

@@ -26,14 +26,14 @@ const PORT = process.env.PORT ? parseInt(process.env.PORT, 10) : 4010;
 const artifactStorageReader = new ArtifactStorageReader(s3, null);
 
 const handleRequest = createRequestHandler({
-  isKeyValid: createIsKeyValid({ s3, getCache: null, waitUntil: null, analytics: null }),
+  isKeyValid: createIsKeyValid({
+    artifactStorageReader,
+    getCache: null,
+    waitUntil: null,
+    analytics: null,
+  }),
   async getArtifactAction(targetId, contractName, artifactType, eTag) {
-    return artifactStorageReader.generateArtifactReadUrl(
-      targetId,
-      contractName,
-      artifactType,
-      eTag,
-    );
+    return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag);
   },
   async fetchText(url) {
     const r = await fetch(url);
@@ -47,7 +47,12 @@ const handleRequest = createRequestHandler({
 });
 
 const handleArtifactRequest = createArtifactRequestHandler({
-  isKeyValid: createIsKeyValid({ s3, getCache: null, waitUntil: null, analytics: null }),
+  isKeyValid: createIsKeyValid({
+    artifactStorageReader,
+    getCache: null,
+    waitUntil: null,
+    analytics: null,
+  }),
   isAppDeploymentActive: createIsAppDeploymentActive({
     artifactStorageReader,
     getCache: null,

packages/services/cdn-worker/src/index.ts

@@ -61,18 +61,13 @@ const handler: ExportedHandler<Env> = {
     const isKeyValid = createIsKeyValid({
       waitUntil: p => ctx.waitUntil(p),
       getCache: () => caches.open('artifacts-auth'),
-      s3,
+      artifactStorageReader,
       analytics,
     });
 
     const handleRequest = createRequestHandler({
       async getArtifactAction(targetId, contractName, artifactType, eTag) {
-        return artifactStorageReader.generateArtifactReadUrl(
-          targetId,
-          contractName,
-          artifactType,
-          eTag,
-        );
+        return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag);
       },
       isKeyValid,
       analytics,

packages/services/cdn-worker/src/key-validation.ts

@@ -1,23 +1,17 @@
 import bcrypt from 'bcryptjs';
 import { Analytics } from './analytics';
-import { type AwsClient } from './aws';
+import { ArtifactStorageReader } from './artifact-storage-reader';
 import { decodeCdnAccessTokenSafe, isCDNAccessToken } from './cdn-token';
 
 export type KeyValidator = (targetId: string, headerKey: string) => Promise<boolean>;
 
 type WaitUntil = (promise: Promise<void>) => void;
 
-type S3Config = {
-  client: AwsClient;
-  bucketName: string;
-  endpoint: string;
-};
-
 type GetCache = () => Promise<Cache | null>;
 
 type CreateKeyValidatorDeps = {
   waitUntil: null | WaitUntil;
-  s3: S3Config;
+  artifactStorageReader: ArtifactStorageReader;
   getCache: null | GetCache;
   analytics: null | Analytics;
 };
@@ -39,7 +33,7 @@ export const createIsKeyValid =
 const handleLegacyCDNAccessToken = async (args: {
   targetId: string;
   accessToken: string;
-  s3: S3Config;
+  artifactStorageReader: ArtifactStorageReader;
   getCache: null | GetCache;
   waitUntil: null | WaitUntil;
   analytics: null | Analytics;
@@ -118,21 +112,7 @@ const handleLegacyCDNAccessToken = async (args: {
     }
   }
 
-  const key = await args.s3.client.fetch(
-    [args.s3.endpoint, args.s3.bucketName, 'cdn-legacy-keys', args.targetId].join('/'),
-    {
-      method: 'GET',
-    },
-  );
-
-  args.analytics?.track(
-    {
-      type: 'r2',
-      statusCode: key.status,
-      action: 'GET cdn-legacy-keys',
-    },
-    args.targetId,
-  );
+  const key = await args.artifactStorageReader.readLegacyAccessKey(args.targetId);
 
   if (key.status !== 200) {
     return withCache(false);
@@ -237,27 +217,7 @@ async function handleCDNAccessToken(
     return withCache(false);
   }
 
-  const s3KeyParts = ['cdn-keys', targetId, decodeResult.token.keyId];
-
-  const key = await deps.s3.client.fetch(
-    [deps.s3.endpoint, deps.s3.bucketName, ...s3KeyParts].join('/'),
-    {
-      method: 'GET',
-      aws: {
-        // This boolean makes Google Cloud Storage & AWS happy.
-        signQuery: true,
-      },
-    },
-  );
-
-  deps.analytics?.track(
-    {
-      type: 'r2',
-      statusCode: key.status,
-      action: 'GET cdn-access-token',
-    },
-    targetId,
-  );
+  const key = await deps.artifactStorageReader.readAccessKey(targetId, decodeResult.token.keyId);
 
   if (key.status !== 200) {
     return withCache(false);

packages/services/cdn-worker/tests/cdn.spec.ts

@@ -3,6 +3,7 @@ import * as bcrypt from 'bcryptjs';
 import '../src/dev-polyfill';
 // eslint-disable-next-line import/no-extraneous-dependencies
 import { describe, expect, test } from 'vitest';
+import { ArtifactStorageReader } from '../src/artifact-storage-reader';
 import {
   InvalidArtifactTypeResponse,
   InvalidAuthKeyResponse,
@@ -48,17 +49,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -124,17 +128,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -216,17 +223,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -292,17 +302,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -374,17 +387,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -454,17 +470,20 @@ describe('CDN Worker', () => {
         getCache: null,
         waitUntil: null,
         analytics: null,
-        s3: {
-          endpoint: 'http://localhost:1337',
-          bucketName: 'artifacts',
-          client: {
-            async fetch() {
-              return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                status: 200,
-              });
-            },
-          } as any,
-        },
+        artifactStorageReader: new ArtifactStorageReader(
+          {
+            endpoint: 'http://localhost:1337',
+            bucketName: 'artifacts',
+            client: {
+              async fetch() {
+                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                  status: 200,
+                });
+              },
+            } as any,
+          },
+          null,
+        ),
       }),
       async getArtifactAction(targetId, _, artifactType) {
         return map.has(`target:${targetId}:${artifactType}`)
@@ -619,17 +638,20 @@ describe('CDN Worker', () => {
           getCache: null,
           waitUntil: null,
           analytics: null,
-          s3: {
-            endpoint: 'http://localhost:1337',
-            bucketName: 'artifacts',
-            client: {
-              async fetch() {
-                return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
-                  status: 200,
-                });
-              },
-            } as any,
-          },
+          artifactStorageReader: new ArtifactStorageReader(
+            {
+              endpoint: 'http://localhost:1337',
+              bucketName: 'artifacts',
+              client: {
+                async fetch() {
+                  return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), {
+                    status: 200,
+                  });
+                },
+              } as any,
+            },
+            null,
+          ),
         }),
         async getArtifactAction(targetId, _, artifactType) {
           return map.has(`target:${targetId}:${artifactType}`)
@@ -667,17 +689,20 @@ describe('CDN Worker', () => {
           getCache: null,
           waitUntil: null,
           analytics: null,
-          s3: {
-            endpoint: 'http://localhost:1337',
-            bucketName: 'artifacts',
-            client: {
-              async fetch() {
-                return new Response(null, {
-                  status: 404,
-                });
-              },
-            } as any,
-          },
+          artifactStorageReader: new ArtifactStorageReader(
+            {
+              endpoint: 'http://localhost:1337',
+              bucketName: 'artifacts',
+              client: {
+                async fetch() {
+                  return new Response(null, {
+                    status: 404,
+                  });
+                },
+              } as any,
+            },
+            null,
+          ),
         }),
         async getArtifactAction(targetId, _, artifactType) {
           return map.has(`target:${targetId}:${artifactType}`)
@@ -713,17 +738,20 @@ describe('CDN Worker', () => {
           getCache: null,
           waitUntil: null,
           analytics: null,
-          s3: {
-            endpoint: 'http://localhost:1337',
-            bucketName: 'artifacts',
-            client: {
-              async fetch() {
-                return new Response(await bcrypt.hash('foobars', await bcrypt.genSalt()), {
-                  status: 200,
-                });
-              },
-            } as any,
-          },
+          artifactStorageReader: new ArtifactStorageReader(
+            {
+              endpoint: 'http://localhost:1337',
+              bucketName: 'artifacts',
+              client: {
+                async fetch() {
+                  return new Response(await bcrypt.hash('foobars', await bcrypt.genSalt()), {
+                    status: 200,
+                  });
+                },
+              } as any,
+            },
+            null,
+          ),
         }),
         async getArtifactAction() {
           return {

packages/services/server/src/index.ts

@@ -542,7 +542,12 @@ export async function main() {
       const artifactStorageReader = new ArtifactStorageReader(s3, null);
 
       const artifactHandler = createArtifactRequestHandler({
-        isKeyValid: createIsKeyValid({ s3, analytics: null, getCache: null, waitUntil: null }),
+        isKeyValid: createIsKeyValid({
+          artifactStorageReader,
+          analytics: null,
+          getCache: null,
+          waitUntil: null,
+        }),
         artifactStorageReader,
         isAppDeploymentActive: createIsAppDeploymentActive({
           artifactStorageReader,