Skip to content

Changing the default dedupe statuses behavior#622

Open
sophiatev wants to merge 15 commits intomainfrom
stevosyan/remove-default-dedupe-statuses
Open

Changing the default dedupe statuses behavior#622
sophiatev wants to merge 15 commits intomainfrom
stevosyan/remove-default-dedupe-statuses

Conversation

@sophiatev
Copy link
Contributor

Summary

Currently, only terminal statuses are reusable when creating a new orchestration with a given instance ID. This is enforced when creating the OrchestrationIdReusePolicy, which ensures that only terminal statuses are considered "reusable" (minus whatever statuses the user includes in the dedupe statuses passed to the creation call). This PR allows all statuses to be reusable, with the assumption that the server-side implementation will terminate an existing orchestration if it is running, pending, or suspended (in a non-terminal status) before creating a new one. This is a breaking change for which all server-side implementations will need to be updated accordingly:

  1. This PR addresses the WebJobs implementation for the DF use-case.
  2. DTS will need to be updated for the standalone SDK use-case to add the termination logic. However, the current backend implementation already does not allow creating a new orchestration if one exists in a non-terminal status, so changing the client-side implementation will not "break" anything until then.
  3. Dapr sidecar, which forks this repo so it should be fine?

Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to release_notes.md
  • Backport is not required
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • All required tests have been added/updated (unit tests, E2E tests)
  • Breaking change?
    • If yes:
      • Impact: See summary
      • Migration guidance:

AI-assisted code disclosure (required)

Was an AI tool used? (select one)

  • No
  • Yes, AI helped write parts of this PR (e.g., GitHub Copilot)
  • Yes, an AI agent generated most of this PR

If AI was used:

  • Tool(s):
  • AI-assisted areas/files:
  • What you changed after AI output:

AI verification (required if AI was used):

  • I understand the code and can explain it
  • I verified referenced APIs/types exist and are correct
  • I reviewed edge cases/failure paths (timeouts, retries, cancellation, exceptions)
  • I reviewed concurrency/async behavior
  • I checked for unintended breaking or behavior changes

Notes for reviewers

  • N/A

Copilot AI review requested due to automatic review settings January 21, 2026 01:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes the default behavior for orchestration ID reuse policies to allow all orchestration statuses to be reusable, rather than just terminal statuses. This is a breaking change that assumes server-side implementations will terminate existing orchestrations in non-terminal states before creating new ones with the same instance ID.

Changes:

  • Renamed GetTerminalStatuses() to GetAllStatuses() and expanded it to include non-terminal statuses (Pending, Running, Suspended)
  • Modified ConvertDedupeStatusesToReusePolicy() to return non-nullable policy and work with all statuses instead of just terminal ones
  • Updated all unit tests to reflect the new behavior and removed tests that validated non-terminal status filtering
  • Updated comments in server-side code to reflect the new semantics

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File Description
src/Client/Grpc/ProtoUtils.cs Renamed method to GetAllStatuses, added non-terminal statuses, changed return type to non-nullable, updated logic to handle all statuses
src/Client/Grpc/GrpcDurableTaskClient.cs Refactored dedupe status handling using collection expressions, removed conditional policy assignment logic
src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs Updated comment to reflect "all statuses" instead of "terminal statuses"
test/Client/Grpc.Tests/ProtoUtilsTests.cs Comprehensive test updates including renamed tests, updated assertions for all statuses, removed non-terminal filtering tests, added tests for new statuses

Copilot AI review requested due to automatic review settings January 26, 2026 20:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/InProcessTestHost/Sidecar/Grpc/TaskHubGrpcServer.cs:214

  • With the new semantics, making non-terminal statuses replaceable implies the server should terminate an existing non-terminal instance before creating the new one. This in-process gRPC server currently only converts the policy to dedupeStatuses and proceeds to create the orchestration, without any termination step, which can diverge from the intended behavior. Consider implementing the termination behavior here so the test host matches real backends.
        return Task.FromResult(new P.DeleteTaskHubResponse());
    }

    /// <summary>
    /// Starts a new orchestration instance.

Copilot AI review requested due to automatic review settings January 26, 2026 22:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

…upe statuses is null (never set by the client. this is different than explicitly making it an empty list)
Copilot AI review requested due to automatic review settings January 28, 2026 23:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

// Note: This requires the protobuf to support OrchestrationIdReusePolicy field
// If the protobuf doesn't support it yet, this will need to be updated when the protobuf is updated
if (options?.DedupeStatuses != null && options.DedupeStatuses.Count > 0)
if (options?.DedupeStatuses != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for removing the options.DedupeStatuses.Count check? Is that a behavior change?

Copy link
Contributor Author

@sophiatev sophiatev Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a behavior change. My thinking is:

  1. If the user didn't specify dedupe statuses at all, we should default to whatever the backend implementation does
  2. If the user specifically set the dedupe statuses to an empty array, we should take that to mean all statuses are reusable

Previously, I think both situations would default to whatever the backend does

Copilot AI review requested due to automatic review settings February 4, 2026 23:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings February 5, 2026 06:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants