Deduplicate repeated target indices within a SwitchBuilder case to avoid duplicate delivery - #706
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes duplicate message delivery in SwitchBuilder when the same ExecutorBinding is listed multiple times within a single switch case or default branch. It aligns the Go SDK’s switch routing behavior with the .NET implementation by ensuring each distinct target receives a matched message at most once per case/default.
Changes:
- Deduplicate per-case/per-default target indices in
SwitchBuilder.collectTargetsusing a per-callseenset. - Add black-box tests to verify deduplication for repeated targets in a case and in the default branch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| workflow/builder.go | Adds per-call deduplication in collectTargets to prevent duplicate deliveries when targets repeat within the same case/default. |
| workflow/builder_test.go | Adds behavioral tests ensuring repeated targets in a case/default only receive one delivery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
c5d3f7f to
e15da58
Compare
This comment has been minimized.
This comment has been minimized.
collectTargets appended a target index for every binding passed to AddCase/WithDefault without a membership check, so listing the same target twice (e.g. AddCase(pred, t1, t1)) yielded indices [0,0] and the message was enqueued into that executor twice. Track a per-call seen set and emit each index at most once, matching .NET's HashSet<int> target semantics so a message is delivered once per target.
e15da58 to
65a3c33
Compare
Parity Review — ✅ No Issues FoundScopeThis PR fixes a behavioral bug in the unexported Cross-SDK ParityThe fix directly mirrors the upstream .NET implementation:
No Python equivalent was found for LabelsNo exported Go API surface changed, so Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
SwitchBuilder.collectTargetsdeduplicates the aggregated fan-outtargetsslice viatargetIndexByID, but the per-calloutslice appended an index for every binding passed with no membership check. Listing the same target twice within a single case or default —AddCase(pred, t1, t1)orWithDefault(t1, t1)— therefore produced indices[0, 0].Nothing downstream deduplicates either: the assigner yields index 0 twice,
selectedTargetIDsappends the same sink ID twice,resolveTargetsreturns the same executor twice, the type filter keeps both, andMapIntoenqueues the envelope once per entry — so the matched message is delivered to that executor twice.This change tracks a per-call
seenset incollectTargetsand emits each index at most once. It applies identically toAddCaseandWithDefaultsince both route throughcollectTargets.Why
Mirrors the .NET implementation, where a switch case's targets are backed by
HashSet<int>so a repeated target collapses to a single delivery. This restores cross-SDK parity: a message routed through a switch should reach each distinct target exactly once regardless of how many times it was listed.Testing
Added two black-box behavioral tests in
workflow/builder_test.gousing the existingrecordingBindingharness:TestAddSwitch_DeduplicatesRepeatedTargetsWithinCase—AddCase(pred, target, target), asserts the target's handler fires exactly once.TestAddSwitch_DeduplicatesRepeatedDefaultTargets—WithDefault(def, def), asserts the default target fires exactly once.Both fail before the fix (delivered twice) and pass after.
go build ./...,go vet ./workflow/..., andgo test ./workflow/...are green.