Fix: processor allocator handler concurrency and reconnect issues by lacroixthomas · Pull Request #4789 · agones-dev/agones

lacroixthomas · 2026-10-19T17:27:45Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:
/kind bug

What this PR does / Why we need it:

Fixes issues in the processor allocator code, found while integrating the batch allocator #4559:

Serialize Send on the batch stream with a mutex. Batch responses and pull requests are sent from different goroutines, and gRPC doesn't allow concurrent sends on same stream
Wait for in-flight batch sends before the stream handler returns
Only unregister a client if its stream is still the registered one, so a stale stream can't evict a client that reconnected with the same ID
Fixing config in the allocator and extensions main.go and the extensions Helm deployment

Which issue(s) this PR fixes:

Part of #4190

Did you use AI tools in preparing this PR?:

Y

Special notes for your reviewer:

This is a first part of the split of #4559

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

Copilot

🟡 Changes recommended

Unbounded in-flight batches can exhaust processor resources under sustained overload.

2 open issues

What changed in this PR

Prepares the processor allocator for batch integration by improving stream concurrency, reconnect handling, and configuration wiring.

Changes:

Serializes stream sends and waits for concurrent batch responses.
Prevents stale streams from unregistering reconnected clients, with regression coverage.
Binds processor environment settings and exposes them through the extensions deployment.

File	Description
pkg/gameserverallocations/processor/handler.go	Adds send locking, concurrent batches, and stream-aware cleanup.
pkg/gameserverallocations/processor/handler_test.go	Tests stale-stream removal after reconnect.
install/helm/agones/templates/extensions-deployment.yaml	Passes feature-gated processor settings.
cmd/extensions/main.go	Binds processor environment variables.
cmd/allocator/main.go	Binds processor environment variables.

_{Review effort: Balanced}

markmandel · 2026-10-22T03:59:57Z

+		batches.Go(func() {
+			response := h.submitBatch(stream.Context(), batchID, requestWrappers)


Each batch runs under stream.Context(), and that context has no deadline. It only ends when the stream closes.

Should this be something like:

Suggested change

batches.Go(func() {

response := h.submitBatch(stream.Context(), batchID, requestWrappers)

batches.Go(func() {

defer func() { <-locked.inFlight }()

response := h.submitBatch(context.WithTimeout(stream.Context(), maxTimeout), batchID, requestWrappers)

or similar?

agones-bot · 2026-10-19T18:24:25Z

Build Succeeded 🥳

Build Id: b7e7292a-90c3-4569-a3fc-46c376ec4276

The following development artifacts have been built, and will exist for the next 30 days:

image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.62.0-dev-e17e166
image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.62.0-dev-e17e166
image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.62.0-dev-e17e166-linux
image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.62.0-dev-e17e166
image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.62.0-dev-e17e166
image: us-docker.pkg.dev/agones-images/ci/agones-processor:1.62.0-dev-e17e166
Linux C++ SDK (build): agonessdk-1.62.0-dev-e17e166-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.62.0-dev-e17e166.zip

A preview of the website (the last 30 builds are retained):

https://e17e166-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4789/head:pr_4789 && git checkout pr_4789
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.62.0-dev-e17e166

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

agones-bot · 2026-10-19T18:38:30Z

Build Failed 😭

Build Id: 6967b58e-0726-4b34-a931-42245b5df9c1

Status: FAILURE

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

agones-bot · 2026-10-19T21:21:22Z

Build Succeeded 🥳

Build Id: be1b3d98-0314-45d0-9aa5-ed8e58760315

The following development artifacts have been built, and will exist for the next 30 days:

image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.62.0-dev-54132a3
image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.62.0-dev-54132a3
image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.62.0-dev-54132a3-linux
image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.62.0-dev-54132a3
image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.62.0-dev-54132a3
image: us-docker.pkg.dev/agones-images/ci/agones-processor:1.62.0-dev-54132a3
Linux C++ SDK (build): agonessdk-1.62.0-dev-54132a3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.62.0-dev-54132a3.zip

A preview of the website (the last 30 builds are retained):

https://54132a3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4789/head:pr_4789 && git checkout pr_4789
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.62.0-dev-54132a3

nrwiersma

I only have a cursory overview of how the allocator works, so take the review with a pinch of salt. That said, it looks good to me, with a single convention nitpick.

nrwiersma · 2026-10-19T19:30:09Z

+	inFlight chan struct{}
+	mu       sync.Mutex


Nit: conventionally, mu is above what it protects.

Copilot

🔵 Needs a closer look

Stream concurrency needs human validation because the regression test does not reliably exercise competing sends.

2 open issues

1 resolved since last review

Bound in-flight batches to prevent memory exhaustion

_{Review effort: Balanced}

+	done := runStreamBatches(h, stream)
+
+	// pull requests race with the batch responses
+	for range 50 {
+		h.sendPullRequestsToClients()


markmandel · 2026-10-21T16:52:49Z

I have some review comments pending - I just need to find some time to do it!

markmandel · 2026-10-22T03:51:39Z


+// maxInFlightBatches is the maximum number of batches being processed at once for a single client stream
+// Once reached, the handler stops receiving batches until one completes
+const maxInFlightBatches = 10


Where does this number come from? Why 10?

markmandel · 2026-10-22T03:59:57Z

+		batches.Go(func() {
+			response := h.submitBatch(stream.Context(), batchID, requestWrappers)


Each batch runs under stream.Context(), and that context has no deadline. It only ends when the stream closes.

Should this be something like:

Suggested change

batches.Go(func() {

response := h.submitBatch(stream.Context(), batchID, requestWrappers)

batches.Go(func() {

defer func() { <-locked.inFlight }()

response := h.submitBatch(context.WithTimeout(stream.Context(), maxTimeout), batchID, requestWrappers)

or similar?

markmandel · 2026-10-22T04:07:55Z

+		// Process the batch on its own, so that a slow request only delays the response of its own batch,
+		// and not the receipt and processing of the next ones.
+		batches.Go(func() {
+			defer func() { <-locked.inFlight }()


Could this lock if nothing is in this channel at the end of the batch processing?

markmandel · 2026-10-22T04:16:05Z

+// sendPull sends a pull request, unless the stream is at capacity or busy sending something else, as a
+// pull is not needed then and must not hold up the pulls to the other clients.
+func (s *lockedStream) sendPull(msg *allocationpb.ProcessorMessage) error {
+	if len(s.inFlight) == cap(s.inFlight) || !s.mu.TryLock() {


The client only sends batches after it gets a pull. If a tick lands while a batch response holds s.mu, the pull is skipped. Pending requests then wait at least one more pullInterval (200ms by default).

I assume this is by design to allow time for each batch to process?

markmandel · 2026-10-22T04:20:34Z

+
+// sendPull sends a pull request, unless the stream is at capacity or busy sending something else, as a
+// pull is not needed then and must not hold up the pulls to the other clients.
+func (s *lockedStream) sendPull(msg *allocationpb.ProcessorMessage) error {


A skipped pull returns nil, so removeClient at handler:253 is never reached for a stream that is at capacity or stuck in Send.

Scenario: the client stops reading, and a response Send blocks on HTTP/2 flow control while holding s.mu. Every later tick fails TryLock and returns nil, so the client stays registered forever. Before this change, the unconditional pull Send would fail and the client would be removed.

This feels like this goes back to the itmeout in previous review comments.

hmm true indeed, I'm gonna have a think about it, I might have an idea of how to catch that and also make things simple around this "lockedStream" part, might gonna create another file to handle all this part to keep things simpler on the handler.go, Will poke you back once I have something that fixes that and a bit cleaner - got something locally, but want to improve it 👌🏼
Also, pretty good catch !

markmandel · 2026-10-22T04:28:49Z

+
+	// sends must be over before the handler returns
+	var batches sync.WaitGroup
+	defer batches.Wait()


stream.Context() is only cancelled after the handler returns. So once the client half-closes (Recv returns io.EOF at handler.go:147-154, the deferred Wait blocks on batch goroutines whose context will never be cancelled.

May we worth the effort then to also make the context cancellable as well for this reason to ensure it finishes when the system shuts down.

markmandel · 2026-10-22T04:30:13Z

+		select {
+		case locked.inFlight <- struct{}{}:
+		case <-stream.Context().Done():
+			return stream.Context().Err()


grpc-go turns an error that isn't a status into codes.Unknown. So if a client cancels while 10 batches are in flight, it sees Unknown instead of Canceled or DeadlineExceeded, and retry or backoff logic that checks the code treats it as an unknown server failure.

Suggestion: return status.FromContextError(stream.Context().Err()).Err().

Oooh, that's a good catch

Also gonna add a unit test for that 👌🏼

markmandel · 2026-10-22T04:40:08Z

 		}
-		if err := stream.Send(pullMsg); err != nil {
+		send := stream.Send
+		if ls, ok := stream.(*lockedStream); ok {


Could this ever not be a lockedStream?

Should h.clients be map[string]*lockedStream ?

Gonna update that, it definitively should, way cleaner

markmandel · 2026-10-22T04:40:58Z

 	defer h.mu.Unlock()

-	delete(h.clients, clientID)
+	if h.clients[clientID] == stream {


Comparing interface values with == panics at runtime if the dynamic type can't be compared, for example a struct value holding a slice, map or func field. The panic would happen in deferred cleanup and crash the processor. The old delete(h.clients, id) could not panic. Item 6's typed map also fixes this, since *lockedStream pointers can always be compared.

fix: processor allocator handler concurrency and reconnect issues

d0af217

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

github-actions Bot added the kind/bug These are bugs. label Oct 19, 2026

lacroixthomas changed the title ~~fix: processor allocator handler concurrency and reconnect issues~~ Fix: processor allocator handler concurrency and reconnect issues Oct 19, 2026

lacroixthomas requested a balanced review from Copilot October 19, 2026 17:28

github-actions Bot added the size/M label Oct 19, 2026

lacroixthomas marked this pull request as ready for review October 19, 2026 17:28

Merge branch 'main' into fix/processor-allocator-handler-fixes

e17e166

Copilot started reviewing on behalf of lacroixthomas October 19, 2026 17:28

View session

lacroixthomas requested a review from a team October 19, 2026 17:29

Copilot AI reviewed Oct 19, 2026

View reviewed changes

fix: improve context / errors on streams

f5c93fa

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

fix: fix linter issues

54132a3

Signed-off-by: Thomas Lacroix <thomas.lacroix@epitech.eu>

nrwiersma approved these changes Oct 20, 2026

View reviewed changes

markmandel requested a balanced review from Copilot October 21, 2026 00:23

Copilot started reviewing on behalf of markmandel October 21, 2026 00:24

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Comment thread pkg/gameserverallocations/processor/handler_test.go

Comment on lines +861 to +865

done := runStreamBatches(h, stream)

// pull requests race with the batch responses

for range 50 {

h.sendPullRequestsToClients()

markmandel reviewed Oct 22, 2026

View reviewed changes

		batches.Go(func() {
		response := h.submitBatch(stream.Context(), batchID, requestWrappers)

Conversation

lacroixthomas commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agones-bot commented Oct 19, 2026

Uh oh!

agones-bot commented Oct 19, 2026

Uh oh!

agones-bot commented Oct 19, 2026

Uh oh!

nrwiersma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

markmandel commented Oct 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markmandel Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markmandel Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

markmandel Oct 22, 2026 •

edited

Loading

markmandel Oct 22, 2026 •

edited

Loading