From f34c2936dc4e664c90b69172136eb748f9cd1c1c Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Fri, 27 Mar 2026 08:40:01 +0000 Subject: [PATCH 1/2] fix: raiseIfFailed() now checks runtimeStatus for failed orchestrations without failure details raiseIfFailed() only checked failureDetails presence, not runtimeStatus. When an orchestration had FAILED status but failure details were missing (e.g., empty error message/type from sidecar), the method silently returned without throwing, giving callers a false sense of success. Now also checks runtimeStatus === FAILED and throws with synthetic details when real failure details are absent. Adds 10 unit tests covering all status combinations. Fixes #204 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/orchestration/orchestration-state.ts | 15 +++ .../test/orchestration-state.spec.ts | 121 ++++++++++++++++++ 2 files changed, 136 insertions(+) create mode 100644 packages/durabletask-js/test/orchestration-state.spec.ts diff --git a/packages/durabletask-js/src/orchestration/orchestration-state.ts b/packages/durabletask-js/src/orchestration/orchestration-state.ts index 2456932..080759c 100644 --- a/packages/durabletask-js/src/orchestration/orchestration-state.ts +++ b/packages/durabletask-js/src/orchestration/orchestration-state.ts @@ -48,5 +48,20 @@ export class OrchestrationState { this.failureDetails, ); } + + // Also check the runtime status for cases where failure details were + // dropped (e.g., empty error message/type from the sidecar). + // Without this check, a FAILED orchestration with missing details would + // silently pass through raiseIfFailed(). + if (this.runtimeStatus === OrchestrationStatus.FAILED) { + const syntheticDetails = new FailureDetails( + "Unknown error", + "UnknownError", + ); + throw new OrchestrationFailedError( + `Orchestration '${this.instanceId}' failed`, + syntheticDetails, + ); + } } } diff --git a/packages/durabletask-js/test/orchestration-state.spec.ts b/packages/durabletask-js/test/orchestration-state.spec.ts new file mode 100644 index 0000000..48f953b --- /dev/null +++ b/packages/durabletask-js/test/orchestration-state.spec.ts @@ -0,0 +1,121 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { OrchestrationState, OrchestrationStatus, FailureDetails } from "../src"; +import { OrchestrationFailedError } from "../src/orchestration/exception/orchestration-failed-error"; + +describe("OrchestrationState", () => { + const instanceId = "test-instance-001"; + const name = "TestOrchestrator"; + const createdAt = new Date("2026-01-01T00:00:00Z"); + const lastUpdatedAt = new Date("2026-01-01T00:01:00Z"); + + function createState( + runtimeStatus: OrchestrationStatus, + failureDetails?: FailureDetails, + ): OrchestrationState { + return new OrchestrationState( + instanceId, + name, + runtimeStatus, + createdAt, + lastUpdatedAt, + undefined, + undefined, + undefined, + failureDetails, + ); + } + + describe("raiseIfFailed", () => { + it("should throw OrchestrationFailedError when failureDetails is present", () => { + const details = new FailureDetails("Something went wrong", "Error", "at foo.ts:1"); + const state = createState(OrchestrationStatus.FAILED, details); + + expect(() => state.raiseIfFailed()).toThrow(OrchestrationFailedError); + }); + + it("should include the instance ID and error message when failureDetails is present", () => { + const details = new FailureDetails("Something went wrong", "Error"); + const state = createState(OrchestrationStatus.FAILED, details); + + try { + state.raiseIfFailed(); + fail("Expected raiseIfFailed to throw"); + } catch (e: unknown) { + expect(e).toBeInstanceOf(OrchestrationFailedError); + const error = e as OrchestrationFailedError; + expect(error.message).toContain(instanceId); + expect(error.message).toContain("Something went wrong"); + expect(error.failureDetails).toBe(details); + } + }); + + it("should throw when runtimeStatus is FAILED but failureDetails is missing", () => { + const state = createState(OrchestrationStatus.FAILED); + + expect(() => state.raiseIfFailed()).toThrow(OrchestrationFailedError); + }); + + it("should include the instance ID when runtimeStatus is FAILED without details", () => { + const state = createState(OrchestrationStatus.FAILED); + + try { + state.raiseIfFailed(); + fail("Expected raiseIfFailed to throw"); + } catch (e: unknown) { + expect(e).toBeInstanceOf(OrchestrationFailedError); + const error = e as OrchestrationFailedError; + expect(error.message).toContain(instanceId); + expect(error.failureDetails).toBeDefined(); + expect(error.failureDetails.errorType).toEqual("UnknownError"); + } + }); + + it("should not throw when runtimeStatus is COMPLETED", () => { + const state = createState(OrchestrationStatus.COMPLETED); + + expect(() => state.raiseIfFailed()).not.toThrow(); + }); + + it("should not throw when runtimeStatus is RUNNING", () => { + const state = createState(OrchestrationStatus.RUNNING); + + expect(() => state.raiseIfFailed()).not.toThrow(); + }); + + it("should not throw when runtimeStatus is TERMINATED", () => { + const state = createState(OrchestrationStatus.TERMINATED); + + expect(() => state.raiseIfFailed()).not.toThrow(); + }); + + it("should not throw when runtimeStatus is PENDING", () => { + const state = createState(OrchestrationStatus.PENDING); + + expect(() => state.raiseIfFailed()).not.toThrow(); + }); + + it("should not throw when runtimeStatus is SUSPENDED", () => { + const state = createState(OrchestrationStatus.SUSPENDED); + + expect(() => state.raiseIfFailed()).not.toThrow(); + }); + + it("should prioritize failureDetails over synthetic details when both are available", () => { + const details = new FailureDetails("Specific error", "CustomError", "stack trace"); + const state = createState(OrchestrationStatus.FAILED, details); + + try { + state.raiseIfFailed(); + fail("Expected raiseIfFailed to throw"); + } catch (e: unknown) { + const error = e as OrchestrationFailedError; + // Should use the real failure details, not synthetic ones + expect(error.failureDetails.message).toEqual("Specific error"); + expect(error.failureDetails.errorType).toEqual("CustomError"); + expect(error.failureDetails.stackTrace).toEqual("stack trace"); + } + }); + }); +}); From 851cff94f3df95dc501f3ca553d5ce9eab76a74d Mon Sep 17 00:00:00 2001 From: wangbill Date: Fri, 17 Jul 2026 14:51:40 -0700 Subject: [PATCH 2/2] Address review comments: use throw over fail() and include synthetic message - Replace fail(...) with throw new Error(...) in orchestration-state.spec.ts; fail is not a guaranteed global under jest-circus (Jest 29 default runner), so a regression would surface as a clean assertion failure instead of ReferenceError. - Include the synthetic failure message in the OrchestrationFailedError thrown from the runtimeStatus===FAILED path, matching the failure-details path message format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d175a43f-c852-49a2-a0de-63db579a8e3a --- .../durabletask-js/src/orchestration/orchestration-state.ts | 2 +- packages/durabletask-js/test/orchestration-state.spec.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/durabletask-js/src/orchestration/orchestration-state.ts b/packages/durabletask-js/src/orchestration/orchestration-state.ts index 080759c..5ff2a6f 100644 --- a/packages/durabletask-js/src/orchestration/orchestration-state.ts +++ b/packages/durabletask-js/src/orchestration/orchestration-state.ts @@ -59,7 +59,7 @@ export class OrchestrationState { "UnknownError", ); throw new OrchestrationFailedError( - `Orchestration '${this.instanceId}' failed`, + `Orchestration '${this.instanceId}' failed: ${syntheticDetails.message}`, syntheticDetails, ); } diff --git a/packages/durabletask-js/test/orchestration-state.spec.ts b/packages/durabletask-js/test/orchestration-state.spec.ts index 48f953b..8d2cb4a 100644 --- a/packages/durabletask-js/test/orchestration-state.spec.ts +++ b/packages/durabletask-js/test/orchestration-state.spec.ts @@ -41,7 +41,7 @@ describe("OrchestrationState", () => { try { state.raiseIfFailed(); - fail("Expected raiseIfFailed to throw"); + throw new Error("Expected raiseIfFailed to throw"); } catch (e: unknown) { expect(e).toBeInstanceOf(OrchestrationFailedError); const error = e as OrchestrationFailedError; @@ -62,7 +62,7 @@ describe("OrchestrationState", () => { try { state.raiseIfFailed(); - fail("Expected raiseIfFailed to throw"); + throw new Error("Expected raiseIfFailed to throw"); } catch (e: unknown) { expect(e).toBeInstanceOf(OrchestrationFailedError); const error = e as OrchestrationFailedError; @@ -108,7 +108,7 @@ describe("OrchestrationState", () => { try { state.raiseIfFailed(); - fail("Expected raiseIfFailed to throw"); + throw new Error("Expected raiseIfFailed to throw"); } catch (e: unknown) { const error = e as OrchestrationFailedError; // Should use the real failure details, not synthetic ones