Skip to content

fix(shard-distributor): Handle ErrShardAlreadyAssigned as success in assignEphemeralShard#7739

Merged
jakobht merged 1 commit intocadence-workflow:masterfrom
jakobht:assignShardErrors
Feb 24, 2026
Merged

fix(shard-distributor): Handle ErrShardAlreadyAssigned as success in assignEphemeralShard#7739
jakobht merged 1 commit intocadence-workflow:masterfrom
jakobht:assignShardErrors

Conversation

@jakobht
Copy link
Member

@jakobht jakobht commented Feb 24, 2026

What changed?

Handle ErrShardAlreadyAssigned as a successful response in assignEphemeralShard instead of returning an InternalServiceError. Added Metadata field to ErrShardAlreadyAssigned to include executor metadata directly in the error.

Why?

When multiple concurrent requests try to assign the same ephemeral shard, the first succeeds and subsequent ones receive ErrShardAlreadyAssigned. Previously this bubbled up as an InternalServiceError, causing unnecessary errors for clients. Since shard assignment is idempotent, we should return the already-assigned owner as a successful response. The error now includes metadata to avoid an extra GetExecutor DB call.

How did you test it?

go test -v ./service/sharddistributor/handler/... -run TestGetShardOwner
go test -v ./service/sharddistributor/store/...

Potential risks

N/A - This is a purely additive change that treats an existing error condition as success.

Release notes

N/A - Internal implementation detail.

Documentation Changes

N/A

Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
@gitar-bot
Copy link

gitar-bot bot commented Feb 24, 2026

Code Review ✅ Approved

Clean, well-scoped fix that correctly treats ErrShardAlreadyAssigned as a successful idempotent response. The error-handling flow, metadata propagation, and test coverage are all solid.

Rules ✅ All requirements met

Repository Rules

PR Description Quality Standards: PR description includes Intent & Rationale, Implementation Analysis, Codebase Context, Risk Assessment, and clear testing details

1 rule not applicable. Show all rules by commenting gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@jakobht jakobht enabled auto-merge (squash) February 24, 2026 12:51
@jakobht jakobht merged commit cdeb383 into cadence-workflow:master Feb 24, 2026
42 checks passed
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