fix: Remove most instanceof checks from SDK (#1128)

The number one reported issue stems from instanceof checks and SDK
module level variables, which fail when there are multiple versions of
the SDK packages installed.

- Remove the instanceof checks in favor of coded `is` methods on the
critical classes.
- Install the activity context local storage globally.

Note that the `is` methods do no use the class name, which may be lost
when using minifiers.

Author: bergundy

packages/activity/src/index.ts

@@ -81,6 +81,8 @@ export {
   UntypedActivities,
 } from '@temporalio/common';
 
+const isCompleteAsyncError = Symbol.for('__temporal_isCompleteAsyncError');
+
 /**
  * Throw this error from an Activity in order to make the Worker forget about this Activity.
  *
@@ -104,10 +106,27 @@ export class CompleteAsyncError extends Error {
   constructor() {
     super();
   }
+
+  /**
+   * Marker to determine whether an error is an instance of CompleteAsyncError.
+   */
+  protected readonly [isCompleteAsyncError] = true;
+
+  /**
+   * Instanceof check that works when multiple versions of @temporalio/activity are installed.
+   */
+  static is(error: unknown): error is CompleteAsyncError {
+    return error instanceof CompleteAsyncError || (error instanceof Error && (error as any)[isCompleteAsyncError]);
+  }
+}
+
+// Make it safe to use @temporalio/activity with multiple versions installed.
+const asyncLocalStorageSymbol = Symbol.for('__temporal_activity_context_storage__');
+if (!(globalThis as any)[asyncLocalStorageSymbol]) {
+  (globalThis as any)[asyncLocalStorageSymbol] = new AsyncLocalStorage<Context>();
 }
 
-/** @ignore */
-export const asyncLocalStorage = new AsyncLocalStorage<Context>();
+export const asyncLocalStorage: AsyncLocalStorage<Context> = (globalThis as any)[asyncLocalStorageSymbol];
 
 /**
  * Holds information about the current Activity Execution. Retrieved inside an Activity with `Context.current().info`.

packages/client/src/async-completion-client.ts

@@ -207,33 +207,36 @@ export class AsyncCompletionClient extends BaseClient {
   heartbeat(fullActivityId: FullActivityId, details?: unknown): Promise<void>;
 
   async heartbeat(taskTokenOrFullActivityId: Uint8Array | FullActivityId, details?: unknown): Promise<void> {
+    let cancelRequested = false;
     try {
-      if (taskTokenOrFullActivityId instanceof Uint8Array) {
-        const { cancelRequested } = await this.workflowService.recordActivityTaskHeartbeat({
-          identity: this.options.identity,
-          namespace: this.options.namespace,
-          taskToken: taskTokenOrFullActivityId,
-          details: { payloads: await encodeToPayloads(this.dataConverter, details) },
-        });
-        if (cancelRequested) {
-          throw new ActivityCancelledError('cancelled');
-        }
-      } else {
-        const { cancelRequested } = await this.workflowService.recordActivityTaskHeartbeatById({
-          identity: this.options.identity,
-          namespace: this.options.namespace,
-          ...taskTokenOrFullActivityId,
-          details: { payloads: await encodeToPayloads(this.dataConverter, details) },
-        });
-        if (cancelRequested) {
-          throw new ActivityCancelledError('cancelled');
-        }
-      }
+      const response = await this._sendHeartbeat(taskTokenOrFullActivityId, details);
+      cancelRequested = response.cancelRequested;
     } catch (err) {
-      if (err instanceof ActivityCancelledError) {
-        throw err;
-      }
       this.handleError(err);
     }
+    if (cancelRequested) {
+      throw new ActivityCancelledError('cancelled');
+    }
+  }
+
+  private async _sendHeartbeat(
+    taskTokenOrFullActivityId: Uint8Array | FullActivityId,
+    details?: unknown
+  ): Promise<{ cancelRequested: boolean }> {
+    if (taskTokenOrFullActivityId instanceof Uint8Array) {
+      return await this.workflowService.recordActivityTaskHeartbeat({
+        identity: this.options.identity,
+        namespace: this.options.namespace,
+        taskToken: taskTokenOrFullActivityId,
+        details: { payloads: await encodeToPayloads(this.dataConverter, details) },
+      });
+    } else {
+      return await this.workflowService.recordActivityTaskHeartbeatById({
+        identity: this.options.identity,
+        namespace: this.options.namespace,
+        ...taskTokenOrFullActivityId,
+        details: { payloads: await encodeToPayloads(this.dataConverter, details) },
+      });
+    }
   }
 }

packages/common/src/converter/failure-converter.ts

@@ -57,8 +57,10 @@ export interface FailureConverter {
   errorToFailure(err: unknown, payloadConverter: PayloadConverter): ProtoFailure;
   /**
    * Converts a Failure proto message to a JS Error object.
+   *
+   * The returned error must be an instance of `TemporalFailure`.
    */
-  failureToError(err: ProtoFailure, payloadConverter: PayloadConverter): Error;
+  failureToError(err: ProtoFailure, payloadConverter: PayloadConverter): TemporalFailure;
 }
 
 /**
@@ -183,7 +185,7 @@ export class DefaultFailureConverter implements FailureConverter {
     );
   }
 
-  failureToError(failure: ProtoFailure, payloadConverter: PayloadConverter): Error {
+  failureToError(failure: ProtoFailure, payloadConverter: PayloadConverter): TemporalFailure {
     if (failure.encodedAttributes) {
       const attrs = payloadConverter.fromPayload<DefaultEncodedFailureAttributes>(failure.encodedAttributes);
       // Don't apply encodedAttributes unless they conform to an expected schema
@@ -217,7 +219,7 @@ export class DefaultFailureConverter implements FailureConverter {
   }
 
   errorToFailureInner(err: unknown, payloadConverter: PayloadConverter): ProtoFailure {
-    if (err instanceof TemporalFailure) {
+    if (TemporalFailure.is(err)) {
       if (err.failure) return err.failure;
       const base = {
         message: err.message,
@@ -226,7 +228,7 @@ export class DefaultFailureConverter implements FailureConverter {
         source: FAILURE_SOURCE,
       };
 
-      if (err instanceof ActivityFailure) {
+      if (ActivityFailure.is(err)) {
         return {
           ...base,
           activityFailureInfo: {
@@ -235,7 +237,7 @@ export class DefaultFailureConverter implements FailureConverter {
           },
         };
       }
-      if (err instanceof ChildWorkflowFailure) {
+      if (ChildWorkflowFailure.is(err)) {
         return {
           ...base,
           childWorkflowExecutionFailureInfo: {
@@ -245,7 +247,7 @@ export class DefaultFailureConverter implements FailureConverter {
           },
         };
       }
-      if (err instanceof ApplicationFailure) {
+      if (ApplicationFailure.is(err)) {
         return {
           ...base,
           applicationFailureInfo: {
@@ -258,7 +260,7 @@ export class DefaultFailureConverter implements FailureConverter {
           },
         };
       }
-      if (err instanceof CancelledFailure) {
+      if (CancelledFailure.is(err)) {
         return {
           ...base,
           canceledFailureInfo: {
@@ -269,7 +271,7 @@ export class DefaultFailureConverter implements FailureConverter {
           },
         };
       }
-      if (err instanceof TimeoutFailure) {
+      if (TimeoutFailure.is(err)) {
         return {
           ...base,
           timeoutFailureInfo: {
@@ -280,16 +282,16 @@ export class DefaultFailureConverter implements FailureConverter {
           },
         };
       }
-      if (err instanceof TerminatedFailure) {
+      if (ServerFailure.is(err)) {
         return {
           ...base,
-          terminatedFailureInfo: {},
+          serverFailureInfo: { nonRetryable: err.nonRetryable },
         };
       }
-      if (err instanceof ServerFailure) {
+      if (TerminatedFailure.is(err)) {
         return {
           ...base,
-          serverFailureInfo: { nonRetryable: err.nonRetryable },
+          terminatedFailureInfo: {},
         };
       }
       // Just a TemporalFailure
@@ -333,7 +335,7 @@ export class DefaultFailureConverter implements FailureConverter {
   optionalFailureToOptionalError(
     failure: ProtoFailure | undefined | null,
     payloadConverter: PayloadConverter
-  ): Error | undefined {
+  ): TemporalFailure | undefined {
     return failure ? this.failureToError(failure, payloadConverter) : undefined;
   }
 

packages/common/src/errors.ts

@@ -25,6 +25,8 @@ export class IllegalStateError extends Error {
   public readonly name: string = 'IllegalStateError';
 }
 
+const isWorkflowExecutionAlreadyStartedError = Symbol.for('__temporal_isWorkflowExecutionAlreadyStartedError');
+
 /**
  * This exception is thrown in the following cases:
  *  - Workflow with the same Workflow Id is currently running
@@ -39,6 +41,21 @@ export class WorkflowExecutionAlreadyStartedError extends TemporalFailure {
   constructor(message: string, public readonly workflowId: string, public readonly workflowType: string) {
     super(message);
   }
+
+  /**
+   * Marker to determine whether an error is an instance of WorkflowExecutionAlreadyStartedError.
+   */
+  protected readonly [isWorkflowExecutionAlreadyStartedError] = true;
+
+  /**
+   * Instanceof check that works when multiple versions of @temporalio/common are installed.
+   */
+  static is(error: unknown): error is WorkflowExecutionAlreadyStartedError {
+    return (
+      error instanceof WorkflowExecutionAlreadyStartedError ||
+      (error instanceof Error && (error as any)[isWorkflowExecutionAlreadyStartedError])
+    );
+  }
 }
 
 /**