fix: add missing CANCELED orchestration status#307
Closed
YunchuWang wants to merge 2 commits into
Closed
Conversation
…us enum The client-facing OrchestrationStatus enum was missing the CANCELED member even though the protobuf definition includes ORCHESTRATION_STATUS_CANCELED (4). This caused fromProtobuf() to throw 'Unknown protobuf OrchestrationStatus value: 4' when encountering canceled orchestrations, crashing any client call that reads orchestration state (getOrchestration, waitForCompletion, getAllInstances). Changes: - Add CANCELED to OrchestrationStatus enum (auto-registers in conversion maps) - Add CANCELED case to InMemoryOrchestrationBackend.toClientStatus() - Add CANCELED to isTerminalStatus() in both InMemoryOrchestrationBackend and TestOrchestrationClient - Add comprehensive tests for OrchestrationStatus enum round-trip conversions - Add toClientStatus tests to in-memory backend spec Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve the in-memory backend test conflict while preserving current main coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d175a43f-c852-49a2-a0de-63db579a8e3a
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a client crash and incorrect in-memory behavior by exposing the protobuf CANCELED orchestration status through the public OrchestrationStatus enum, and ensuring canceled orchestrations are treated as terminal in the test/in-memory backend.
Changes:
- Add
CANCELEDto the publicOrchestrationStatusenum and rely on existing conversion-map initialization. - Update in-memory backend status mapping and terminal-status checks to include
CANCELED. - Add/extend Jest coverage for status conversion and in-memory backend status mapping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/durabletask-js/src/orchestration/enum/orchestration-status.enum.ts | Adds CANCELED to the public status enum to align with protobuf and prevent conversion crashes. |
| packages/durabletask-js/src/testing/in-memory-backend.ts | Maps protobuf CANCELED to client CANCELED and treats it as terminal in the in-memory backend. |
| packages/durabletask-js/src/testing/test-client.ts | Treats protobuf CANCELED as terminal so waitForCompletion can return for canceled instances. |
| packages/durabletask-js/test/orchestration-status.spec.ts | Adds conversion/round-trip/completeness tests for status enum mappings, including CANCELED. |
| packages/durabletask-js/test/in-memory-backend.spec.ts | Adds regression coverage for toClientStatus mapping of CANCELED. |
Comment on lines
+64
to
+66
| it("should cover all non-zero proto OrchestrationStatus values", () => { | ||
| // Get all numeric values from the proto enum (protobuf JS enums are plain objects) | ||
| const protoValues = new Set<number>(); |
Comment on lines
+606
to
+609
| it("should map CANCELED proto status to CANCELED client status", () => { | ||
| const { OrchestrationStatus: pbStatus } = require("../src/proto/orchestrator_service_pb"); | ||
| expect(backend.toClientStatus(pbStatus.ORCHESTRATION_STATUS_CANCELED)).toBe(OrchestrationStatus.CANCELED); | ||
| }); |
Comment on lines
+611
to
+622
| it("should map all known proto statuses without throwing", () => { | ||
| const { OrchestrationStatus: pbStatus } = require("../src/proto/orchestrator_service_pb"); | ||
| const protoStatuses = [ | ||
| pbStatus.ORCHESTRATION_STATUS_RUNNING, | ||
| pbStatus.ORCHESTRATION_STATUS_COMPLETED, | ||
| pbStatus.ORCHESTRATION_STATUS_CONTINUED_AS_NEW, | ||
| pbStatus.ORCHESTRATION_STATUS_FAILED, | ||
| pbStatus.ORCHESTRATION_STATUS_CANCELED, | ||
| pbStatus.ORCHESTRATION_STATUS_TERMINATED, | ||
| pbStatus.ORCHESTRATION_STATUS_PENDING, | ||
| pbStatus.ORCHESTRATION_STATUS_SUSPENDED, | ||
| ]; |
Member
Author
|
Closing as not an active bug. A cross-SDK/backend check shows nothing currently produces |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #203
Testing
npm test -w @microsoft/durabletask-js -- --runTestsByPath test/orchestration-status.spec.ts test/in-memory-backend.spec.tsnpm run build:core