fix(engine): round-robin workers with equal label affinity - #4697
Merged
Conversation
Label ranking reset the assignment ring to offset 0, so equal-score workers packed onto the first UUID. Rotate only the highest-rank tied group so the fleet is cycled without skipping a better match.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
|
mrkaye97
reviewed
Aug 14, 2026
|
|
||
| regionLabel := &sqlcv1.ListManyWorkerLabelsRow{ | ||
| Key: "region", | ||
| StrValue: pgtype.Text{String: "us-east-1", Valid: true}, |
Contributor
There was a problem hiding this comment.
small nit, but I've been trying to use sqlchelpers.TextFromStr instead (when I have time to do stuff, I want to keep removing more uses of pgtype.Foobar from everywhere
Member
Author
There was a problem hiding this comment.
pre-existing condition on the other tests. this is a good candidate for a linter rule 😅 because afik no one else is reviewing for this.
Comment on lines
+527
to
+532
| first := assignOne(t, s, a, testQI(tenantId, "A", 1), desired, defaultRequest(), nil, nil) | ||
| require.True(t, first.succeeded) | ||
|
|
||
| second := assignOne(t, s, a, testQI(tenantId, "A", 2), desired, defaultRequest(), nil, nil) | ||
| require.True(t, second.succeeded) | ||
| require.NotEqual(t, first.workerId, second.workerId, "equal-affinity workers with free slots must not pack onto the same worker") |
Contributor
There was a problem hiding this comment.
I struggle with these types of tests a bit because there are actually more cases here. There's the (alleged) old case, which was us always scheduling on the first worker (although unclear to me if "first" is coming from db ordering, in-memory ordering, etc.). Then there's the new one which is where we try to round robin. But there's also a possibility where we would assign semi-randomly, in which case this test would sometimes pass if it just happens that we get round robin behavior because e.g. the ordering changes for some reason.
This is the same issue we have in e.g. the tests for dynamic label assign, and so I basically wrote those tests to be "probabilistic," meaning that if we run N tasks with label X provided on two workers with labels X and Y, the probability of all N running on a single worker by random chance (the second case) is (1/2)^N, so then we just make N sufficiently big to be confident the feature is working.
I think we should do that here too
Member
Author
There was a problem hiding this comment.
this path is not probabilistic and this covers the case on main (and fails). this wont cover reliably if we decide to make this path probabilistic in the future, but it should be fine for today
Contributor
There was a problem hiding this comment.
Sorry to be clear, the other path I was describing also isn't probabilistic. But the question is 1) if it becomes probabilistic, this test will be flaky instead of reliably failing and 2) how confident are you that it's not probabilistic?
Member
Author
There was a problem hiding this comment.
- yes, but do we expect to make this path probabilistic?
- 100%
Contributor
There was a problem hiding this comment.
maybe I'm misunderstanding 😅 these methods: 1) loop over things from the database and 2) loop over maps. At the very least, map iteration in Go is intentionally non-deterministic, so are we really 100% sure?
Contributor
There was a problem hiding this comment.
Ah, okay I see the thing I was missing - there's a byte sort of the worker ids on line 6something that gets rid of the risk from the maps here, so I think this is fine 👍
Member
Author
There was a problem hiding this comment.
just double checked to be extra certain.
the replenish step is where the nondeterminism is possible, but we sort there.
we're not looping over a map on this path
| candidates := a.workerIds | ||
| offset := a.ringOffset | ||
| a.ringOffset++ | ||
| tiedLen := len(candidates) |
Contributor
There was a problem hiding this comment.
nit: it took me like 10 mins of reading the code to understand that this was the number of workers that are tied for "first place" in the rank order - I feel like naming it something to do with that would be good?
Comment on lines
+1038
to
+1039
| for i := 0; i < tiedLen; i++ { | ||
| workerId := candidates[(offset+i)%tiedLen] |
Contributor
There was a problem hiding this comment.
I'm a little confused by the modulus math here - to me, this read like it'll still select the first candidate? maybe I'm misunderstanding why this works
Member
Author
There was a problem hiding this comment.
added some clarifying comments here
Contributor
Benchmark resultsCompared against |
mrkaye97
approved these changes
Aug 14, 2026
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
2 participants
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
desired_worker_labelsscore packed onto the first UUID until its slots filled.Test plan
go test ./pkg/scheduling/v1/TestScheduler_AssignSingleton_EqualLabelsRoundRobinfails against main packing behavior and passes on this branch