Skip to content

[.NET] Throw when AddWorkflow is given a duplicate workflow name - #66

Merged
Shyju Krishnankutty (kshyju) merged 4 commits into
mainfrom
fix/addworkflow-duplicate-name
Aug 24, 2026
Merged

[.NET] Throw when AddWorkflow is given a duplicate workflow name#66
Shyju Krishnankutty (kshyju) merged 4 commits into
mainfrom
fix/addworkflow-duplicate-name

Conversation

@kshyju

Copy link
Copy Markdown
Contributor

AddWorkflow used an indexer assignment, so registering a workflow under a name that was already taken silently replaced the existing one. Because ExecutorRegistry is first-writer-wins while the workflow dictionary was last-writer-wins, a collision left the two registries disagreeing.

AddWorkflow now throws when a name maps to a different workflow instance. Re-registering the same instance remains a no-op, which the recursive sub-workflow registration relies on.

Fixes #50

AddWorkflow assigned into its backing dictionary with an indexer, so registering
a second workflow under a name that was already taken replaced the first one
with no exception, log, or startup failure.

Because ExecutorRegistry.Register uses TryAdd (first writer wins) while the
workflow dictionary took the last writer, a collision left the two registries
disagreeing: the surviving workflow's topology was executed using the first
workflow's executor code for any executor name the two shared.

AddWorkflow now throws when a name maps to a different workflow instance, using
a message consistent with the existing AddAIAgent duplicate-name error.
Re-registering the same instance stays a no-op, which is required because
BuildWorkflowRegistrationRecursive re-adds a sub-workflow that was also
registered explicitly, and the registration walk can run more than once against
the shared DurableOptions.

Note that a sub-workflow cannot be shared by multiple parent workflows: the
framework's Workflow.TakeOwnership rejects that at bind time. The added test
therefore covers the reachable equivalent, a sub-workflow re-registered through
the recursive registration walk.

Fixes #50

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1feb866c-d58d-4b86-baf3-c15e18f51787
Copilot AI lite review requested due to automatic review settings August 10, 2026 22:12
@kshyju Shyju Krishnankutty (kshyju) changed the title Throw when AddWorkflow is given a duplicate workflow name [.NET] Throw when AddWorkflow is given a duplicate workflow name Aug 10, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1feb866c-d58d-4b86-baf3-c15e18f51787

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 hardens durable workflow registration by preventing silent overwrites when multiple workflows share the same name, which previously could leave the workflow registry and executor registry in a contradictory state. It aligns workflow registration behavior with the existing “duplicate name is an error” pattern used elsewhere (e.g., agent registration), while preserving idempotent re-registration of the same instance (needed for recursive sub-workflow discovery).

Changes:

  • Update DurableWorkflowOptions.AddWorkflow to throw when a different workflow instance is registered under an already-used name, while treating re-registering the same instance as a no-op.
  • Add unit tests covering duplicate-name throwing, case-insensitive name collision, idempotent same-instance registration, and explicit + recursive sub-workflow registration.
  • Add an Unreleased changelog entry documenting the behavior change.

Reviewed changes

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

File Description
dotnet/tests/Microsoft.Agents.AI.DurableTask.UnitTests/Workflows/DurableWorkflowOptionsTests.cs Adds unit coverage for duplicate workflow-name handling and idempotent/sub-workflow registration scenarios.
dotnet/src/Microsoft.Agents.AI.DurableTask/Workflows/DurableWorkflowOptions.cs Enforces “unique workflow name per instance” by throwing on collisions while allowing same-instance re-add.
dotnet/src/Microsoft.Agents.AI.DurableTask/CHANGELOG.md Documents the workflow registration behavior change under Unreleased.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dotnet/src/Microsoft.Agents.AI.DurableTask/CHANGELOG.md Outdated
Copilot AI review requested due to automatic review settings August 10, 2026 22:17
… state

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1feb866c-d58d-4b86-baf3-c15e18f51787

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

dotnet/src/Microsoft.Agents.AI.DurableTask/CHANGELOG.md:5

  • Changelog entries in this file consistently link to the PR (pull/NN) rather than only the issue. This new entry links to the issue (#50) but not the PR, which makes it harder to trace the change back to the implementation.
- Fixed `AddWorkflow` silently overwriting an existing workflow registered under the same name, which left the workflow and executor registries inconsistent. Registering a different workflow under a name that is already taken now throws, while re-registering the same workflow instance remains a no-op ([#66](https://github.com/microsoft/agent-framework-durable-extension/pull/66))

dotnet/src/Microsoft.Agents.AI.DurableTask/Workflows/DurableWorkflowOptions.cs:79

  • If RegisterWorkflowExecutors throws after the workflow is added to the dictionary, a retry with the same Workflow instance will now short-circuit here and never attempt to register executors/agents again, leaving DurableWorkflowOptions.Workflows and Executors potentially inconsistent. Consider re-running RegisterWorkflowExecutors when the same instance is re-added so retries can self-heal (Executors.Register is TryAdd-based and agent registration is guarded).

            // The same instance was already registered, so its executors are registered too.
            return this;
        }

Copilot AI review requested due to automatic review settings August 10, 2026 22:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 3 out of 3 changed files in this pull request and generated no new comments.

Comment thread dotnet/src/Microsoft.Agents.AI.DurableTask/CHANGELOG.md Outdated
…workflow

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1feb866c-d58d-4b86-baf3-c15e18f51787
@kshyju

Copy link
Copy Markdown
Contributor Author

Heads up Ahmed Muhsin (@ahmedmuhsin) — since you'd already approved, note that 1ed4d03 contains a small code change beyond the changelog tag, so it may be worth a second look.

It addresses a comment the Copilot reviewer suppressed on an earlier pass: because re-adding the same instance is now a no-op, a failure part-way through RegisterWorkflowExecutors would leave the workflow in _workflows with its executors only partially registered, and any retry with that same instance would short-circuit and never finish the job.

Fix is a one-line reorder — register executors first, then add to the dictionary:

\\csharp
// Register executors first so a failure part-way through leaves no entry behind and a retry re-runs registration.
this.RegisterWorkflowExecutors(workflow);
this._workflows[workflow.Name] = workflow;
\\

Safe because RegisterWorkflowExecutors never reads _workflows, and it's idempotent anyway (TryAdd plus the ContainsAgent guard), so a retry re-runs cleanly.

Being upfront: this is defensive rather than a bug I could reach from a test. Triggering it needs RegisterWorkflowExecutors to throw and the caller to catch and retry during DI configuration, and I couldn't construct that without adding a seam I didn't think was worth it. Happy to revert if you'd rather keep the diff strictly scoped to #50.

334 unit tests pass and dotnet format --verify-no-changes is clean.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 3 out of 3 changed files in this pull request and generated no new comments.

@kshyju
Shyju Krishnankutty (kshyju) merged commit 884137b into main Aug 24, 2026
9 checks passed
@kshyju
Shyju Krishnankutty (kshyju) deleted the fix/addworkflow-duplicate-name branch August 24, 2026 19:59
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.

AddWorkflow silently overwrites an existing workflow registered under the same name

3 participants