fix(testing-sdk): fix duplicate start history event for retry updates (#356)

*Issue #, if available:*

*Description of changes:*

Populate execution history with correct event for step retries. There
should be a StepFailed event on each retry update if there is an `Error`
object, and `StepSucceeded` if it's not an `Error` object, not a
StepStarted event.


By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.

Author: anthonyting

packages/aws-durable-execution-sdk-js-testing/src/checkpoint-server/storage/__tests__/event-processor.test.ts

@@ -16,6 +16,9 @@ describe("EventProcessor", () => {
   const mockGetHistoryEventDetail = jest.mocked(
     historyEventDetails.getHistoryEventDetail,
   );
+  const mockGetRetryHistoryEventDetail = jest.mocked(
+    historyEventDetails.getRetryHistoryEventDetail,
+  );
 
   beforeEach(() => {
     eventProcessor = new EventProcessor();
@@ -345,6 +348,133 @@ describe("EventProcessor", () => {
         OperationType.EXECUTION,
       );
     });
+
+    it("should handle RETRY action with error using getRetryHistoryEventDetail", () => {
+      const mockRetryHistoryDetails = {
+        eventType: EventType.StepFailed,
+        detailPlace: "StepFailedDetails" as const,
+        getDetails: jest.fn().mockReturnValue({
+          Error: {
+            Payload: { ErrorType: "TestError", ErrorMessage: "Test error" },
+          },
+          RetryDetails: {
+            NextAttemptDelaySeconds: 5,
+            CurrentAttempt: 2,
+          },
+        }),
+      };
+
+      mockGetRetryHistoryEventDetail.mockReturnValue(mockRetryHistoryDetails);
+
+      const update: OperationUpdate = {
+        Id: "update-id",
+        Name: "update-name",
+        Action: OperationAction.RETRY,
+        SubType: "update-subtype",
+        Error: { ErrorType: "TestError", ErrorMessage: "Test error" },
+        Type: undefined,
+      };
+
+      const operation: Operation = {
+        Id: "operation-id",
+        Name: "operation-name",
+        Type: OperationType.STEP,
+        SubType: "operation-subtype",
+        StartTimestamp: undefined,
+        Status: undefined,
+      };
+
+      const result = eventProcessor.processUpdate(update, operation);
+
+      expect(mockGetRetryHistoryEventDetail).toHaveBeenCalledWith(true);
+      expect(mockGetHistoryEventDetail).not.toHaveBeenCalled();
+      expect(result.EventType).toBe(EventType.StepFailed);
+      expect(result.EventId).toBe(1);
+    });
+
+    it("should handle RETRY action without error using getRetryHistoryEventDetail", () => {
+      const mockSuccessRetryDetails = {
+        eventType: EventType.StepSucceeded,
+        detailPlace: "StepSucceededDetails" as const,
+        getDetails: jest.fn().mockReturnValue({
+          Result: { Payload: "test-result" },
+          RetryDetails: {
+            NextAttemptDelaySeconds: 5,
+            CurrentAttempt: 2,
+          },
+        }),
+      };
+
+      mockGetRetryHistoryEventDetail.mockReturnValue(mockSuccessRetryDetails);
+
+      const update: OperationUpdate = {
+        Id: "update-id",
+        Name: "update-name",
+        Action: OperationAction.RETRY,
+        SubType: "update-subtype",
+        Payload: "test-result",
+        Error: undefined,
+        Type: undefined,
+      };
+
+      const operation: Operation = {
+        Id: "operation-id",
+        Name: "operation-name",
+        Type: OperationType.STEP,
+        SubType: "operation-subtype",
+        StartTimestamp: undefined,
+        Status: undefined,
+      };
+
+      const result = eventProcessor.processUpdate(update, operation);
+
+      expect(mockGetRetryHistoryEventDetail).toHaveBeenCalledWith(false);
+      expect(mockGetHistoryEventDetail).not.toHaveBeenCalled();
+      expect(result.EventType).toBe(EventType.StepSucceeded);
+      expect(result.EventId).toBe(1);
+    });
+
+    it("should handle RETRY action without payload and without error using getRetryHistoryEventDetail", () => {
+      const mockSuccessRetryDetails = {
+        eventType: EventType.StepSucceeded,
+        detailPlace: "StepSucceededDetails" as const,
+        getDetails: jest.fn().mockReturnValue({
+          Result: { Payload: "test-result" },
+          RetryDetails: {
+            NextAttemptDelaySeconds: 5,
+            CurrentAttempt: 2,
+          },
+        }),
+      };
+
+      mockGetRetryHistoryEventDetail.mockReturnValue(mockSuccessRetryDetails);
+
+      const update: OperationUpdate = {
+        Id: "update-id",
+        Name: "update-name",
+        Action: OperationAction.RETRY,
+        SubType: "update-subtype",
+        Payload: undefined,
+        Error: undefined,
+        Type: undefined,
+      };
+
+      const operation: Operation = {
+        Id: "operation-id",
+        Name: "operation-name",
+        Type: OperationType.STEP,
+        SubType: "operation-subtype",
+        StartTimestamp: undefined,
+        Status: undefined,
+      };
+
+      const result = eventProcessor.processUpdate(update, operation);
+
+      expect(mockGetRetryHistoryEventDetail).toHaveBeenCalledWith(false);
+      expect(mockGetHistoryEventDetail).not.toHaveBeenCalled();
+      expect(result.EventType).toBe(EventType.StepSucceeded);
+      expect(result.EventId).toBe(1);
+    });
   });
 
   describe("getHistoryDetailsFromUpdate", () => {
@@ -407,18 +537,6 @@ describe("EventProcessor", () => {
         OperationType.CONTEXT,
       );
     });
-
-    it("should handle RETRY action with STEP type", () => {
-      EventProcessor.getHistoryDetailsFromUpdate(
-        OperationAction.RETRY,
-        OperationType.STEP,
-      );
-
-      expect(mockGetHistoryEventDetail).toHaveBeenCalledWith(
-        OperationAction.RETRY,
-        OperationType.STEP,
-      );
-    });
   });
 
   describe("event ID management", () => {

packages/aws-durable-execution-sdk-js-testing/src/checkpoint-server/storage/event-processor.ts

@@ -8,7 +8,9 @@ import {
 } from "@aws-sdk/client-lambda";
 import {
   getHistoryEventDetail,
+  getRetryHistoryEventDetail,
   HistoryEventDetail,
+  OperationActionWithoutRetry,
 } from "../utils/history-event-details";
 
 /**
@@ -102,10 +104,13 @@ export class EventProcessor {
       );
     }
 
-    const historyDetails = EventProcessor.getHistoryDetailsFromUpdate(
-      update.Action,
-      operation.Type,
-    );
+    const historyDetails =
+      update.Action === OperationAction.RETRY
+        ? getRetryHistoryEventDetail(!!update.Error)
+        : EventProcessor.getHistoryDetailsFromUpdate(
+            update.Action,
+            operation.Type,
+          );
 
     return {
       EventType: historyDetails.eventType,
@@ -131,7 +136,7 @@ export class EventProcessor {
    * This static method provides a way to validate that a specific operation action and type
    * combination is supported and to retrieve the corresponding event detail handler.
    *
-   * @template Action - The operation action (START, FAIL, SUCCEED, RETRY)
+   * @template Action - The operation action (START, FAIL, SUCCEED)
    * @template Type - The operation type (EXECUTION, CALLBACK, CONTEXT, INVOKE, STEP, WAIT)
    * @param action - The action being performed on the operation
    * @param type - The type of operation being performed
@@ -149,7 +154,7 @@ export class EventProcessor {
    * ```
    */
   static getHistoryDetailsFromUpdate<
-    Action extends OperationAction,
+    Action extends OperationActionWithoutRetry,
     Type extends OperationType,
   >(action: Action, type: Type): HistoryEventDetail<Action, Type> {
     const historyDetails = getHistoryEventDetail(action, type);

packages/aws-durable-execution-sdk-js-testing/src/checkpoint-server/utils/__tests__/history-event-details.test.ts

@@ -6,7 +6,11 @@ import {
   Operation,
   ErrorObject,
 } from "@aws-sdk/client-lambda";
-import { getHistoryEventDetail } from "../history-event-details";
+import {
+  getHistoryEventDetail,
+  getRetryHistoryEventDetail,
+  OperationActionWithoutRetry,
+} from "../history-event-details";
 
 describe("history-event-details", () => {
   const mockMetadata = {
@@ -63,7 +67,7 @@ describe("history-event-details", () => {
   describe("getHistoryEventDetail", () => {
     it("should return undefined for unknown action-type combinations", () => {
       const result = getHistoryEventDetail(
-        "UNKNOWN" as OperationAction,
+        "UNKNOWN" as OperationActionWithoutRetry,
         "UNKNOWN" as OperationType,
       );
       expect(result).toBeUndefined();
@@ -312,20 +316,12 @@ describe("history-event-details", () => {
         expect(details).toEqual({});
       });
 
-      it("should return correct details for RETRY-STEP", () => {
+      it("should return undefined for RETRY-STEP", () => {
         const detail = getHistoryEventDetail(
-          OperationAction.RETRY,
+          OperationAction.RETRY as OperationActionWithoutRetry,
           OperationType.STEP,
         );
-        expect(detail).toBeDefined();
-        expect(detail!.eventType).toBe(EventType.StepStarted);
-        expect(detail!.detailPlace).toBe("StepStartedDetails");
-
-        const update = createMockUpdate();
-        const operation = createMockOperation();
-        const details = detail!.getDetails(update, operation, mockMetadata);
-
-        expect(details).toEqual({});
+        expect(detail).toBeUndefined();
       });
 
       it("should return correct details for FAIL-STEP", () => {
@@ -367,7 +363,7 @@ describe("history-event-details", () => {
           },
           RetryDetails: {
             NextAttemptDelaySeconds: 5,
-            CurrentAttempt: undefined,
+            CurrentAttempt: 1,
           },
         });
       });
@@ -529,6 +525,28 @@ describe("history-event-details", () => {
     });
   });
 
+  describe("getRetryHistoryEventDetail", () => {
+    it("should return FAIL-STEP handler when isError is true", () => {
+      const retryFailDetail = getRetryHistoryEventDetail(true);
+      const directFailDetail = getHistoryEventDetail(
+        OperationAction.FAIL,
+        OperationType.STEP,
+      );
+
+      expect(retryFailDetail).toBe(directFailDetail);
+    });
+
+    it("should return SUCCEED-STEP handler when isError is false", () => {
+      const retrySucceedDetail = getRetryHistoryEventDetail(false);
+      const directSucceedDetail = getHistoryEventDetail(
+        OperationAction.SUCCEED,
+        OperationType.STEP,
+      );
+
+      expect(retrySucceedDetail).toBe(directSucceedDetail);
+    });
+  });
+
   describe("edge cases", () => {
     it("should handle empty metadata", () => {
       const detail = getHistoryEventDetail(

packages/aws-durable-execution-sdk-js-testing/src/checkpoint-server/utils/history-event-details.ts

@@ -7,6 +7,11 @@ import {
   Operation,
 } from "@aws-sdk/client-lambda";
 
+export type OperationActionWithoutRetry = Exclude<
+  OperationAction,
+  typeof OperationAction.RETRY
+>;
+
 /**
  * Metadata required for generating event details.
  */
@@ -57,11 +62,11 @@ function createEventDetails<T extends keyof Event>(
 /**
  * Type alias for retrieving the correct HistoryEventDetails type based on operation action and type.
  *
- * @template Action - The operation action (START, FAIL, SUCCEED, RETRY)
+ * @template Action - The operation action (START, FAIL, SUCCEED)
  * @template Type - The operation type (EXECUTION, CALLBACK, CONTEXT, INVOKE, STEP, WAIT)
  */
 export type HistoryEventDetail<
-  Action extends OperationAction,
+  Action extends OperationActionWithoutRetry,
   Type extends OperationType,
 > = (typeof historyEventDetailMap)[`${Action}-${Type}`];
 
@@ -76,7 +81,7 @@ export type HistoryEventDetail<
  * - Callback: START
  * - Context: START, FAIL, SUCCEED
  * - Invoke: START
- * - Step: START, RETRY, FAIL, SUCCEED
+ * - Step: START, FAIL, SUCCEED
  * - Wait: START
  */
 const historyEventDetailMap = {
@@ -170,11 +175,6 @@ const historyEventDetailMap = {
     "StepStartedDetails",
     () => ({}),
   ),
-  [`${OperationAction.RETRY}-${OperationType.STEP}`]: createEventDetails(
-    EventType.StepStarted,
-    "StepStartedDetails",
-    () => ({}),
-  ),
   [`${OperationAction.FAIL}-${OperationType.STEP}`]: createEventDetails(
     EventType.StepFailed,
     "StepFailedDetails",
@@ -184,7 +184,7 @@ const historyEventDetailMap = {
       },
       RetryDetails: {
         NextAttemptDelaySeconds: update.StepOptions?.NextAttemptDelaySeconds,
-        CurrentAttempt: operation.StepDetails?.Attempt,
+        CurrentAttempt: operation.StepDetails?.Attempt ?? 1,
       },
     }),
   ),
@@ -223,7 +223,7 @@ const historyEventDetailMap = {
 /**
  * Retrieves the appropriate history event detail handler for a given operation action and type.
  *
- * @template Action - The operation action (START, FAIL, SUCCEED, RETRY)
+ * @template Action - The operation action (START, FAIL, SUCCEED)
  * @template Type - The operation type (EXECUTION, CALLBACK, CONTEXT, INVOKE, STEP, WAIT)
  * @param action - The action being performed on the operation
  * @param type - The type of operation being performed
@@ -238,8 +238,27 @@ const historyEventDetailMap = {
  * ```
  */
 export function getHistoryEventDetail<
-  Action extends OperationAction,
+  Action extends OperationActionWithoutRetry,
   Type extends OperationType,
 >(action: Action, type: Type): HistoryEventDetail<Action, Type> | undefined {
   return historyEventDetailMap[`${action}-${type}`];
 }
+
+/**
+ * Retry updates can create either a FAIL or SUCCEED event depending on the outcome of the retry.
+ * This function returns the appropriate HistoryEventDetail handler based on the payload of the retry.
+ *
+ * @param isError
+ * @returns
+ */
+export function getRetryHistoryEventDetail(isError: boolean) {
+  if (isError) {
+    return historyEventDetailMap[
+      `${OperationAction.FAIL}-${OperationType.STEP}`
+    ];
+  }
+
+  return historyEventDetailMap[
+    `${OperationAction.SUCCEED}-${OperationType.STEP}`
+  ];
+}