Skip to content
This repository was archived by the owner on Aug 18, 2026. It is now read-only.

fix: race condition in GameServersQueue.PushToQueue causing nil pointer dereference - #506

Closed
Dimitris Gkanatsios (dgkanatsios) with Copilot wants to merge 4 commits into
mainfrom
copilot/load-testing-solution
Closed

Dimitris Gkanatsios (dgkanatsios) with Copilot wants to merge 4 commits into
mainfrom
copilot/load-testing-solution

Conversation

Copilot AI commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a TOCTOU (time-of-check-time-of-use) race condition in GameServersQueue.PushToQueue that could cause a nil pointer dereference under concurrent access.

The Bug

PushToQueue used a split RLock/Lock pattern:

  1. Check if a build entry exists under RLock
  2. Release RLock ← race window opens here
  3. Acquire Lock to use the entry

Between steps 2 and 3, a concurrent PopFromQueue could pop the last GameServer for a build and delete the build's entry from the map. When PushToQueue then proceeds to access gsq.queuesPerBuilds[gs.BuildID], it gets nil → panic.

The Fix

Hold a single write lock for the entire PushToQueue operation, eliminating the TOCTOU race window. Since PushToQueue always writes to the map (at minimum namespacedNameToBuildId), a write lock was needed anyway, so there's no real performance penalty.

Changes

File Change
gameserverqueue.go Replace split RLock+Lock with single Lock for PushToQueue
stress_test.go Add TestGameServerQueueConcurrentPushPopSameBuild — regression test with concurrent push/pop on the same build ID (10 runs × 1000 iterations each)
gameserverqueue_benchmark_test.go Update concurrent benchmark to use a shared build ID instead of per-goroutine IDs, proving the race is fixed

Testing

All tests pass with -race:

=== RUN   TestGameServerQueueConcurrentPushPopSameBuild
--- PASS: TestGameServerQueueConcurrentPushPopSameBuild (0.05s)
=== RUN   TestGameServerQueueConcurrentStress
--- PASS: TestGameServerQueueConcurrentStress (0.02s)
=== RUN   TestGameServerQueueConcurrentPushPopRemove
--- PASS: TestGameServerQueueConcurrentPushPopRemove (0.01s)

CodeQL: 0 security alerts.

Copilot AI and others added 3 commits March 24, 2026 03:07
…rtbeat, queue, and controllers

Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com>
Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/29971878-6148-47a4-a4e9-689bafb2ef6e
…chmark robustness

Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com>
Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/29971878-6148-47a4-a4e9-689bafb2ef6e
…er dereference

PushToQueue used a split RLock/Lock pattern: it checked if a build entry
existed under RLock, released it, then acquired Lock to use the entry.
Between the RUnlock and the Lock, a concurrent PopFromQueue could delete
the build entry (when the last GameServer was popped), causing a nil
pointer dereference on the next access.

Fix: hold a single write lock for the entire PushToQueue operation,
eliminating the TOCTOU race window.

Added a regression test (TestGameServerQueueConcurrentPushPopSameBuild)
that exercises concurrent push/pop on the same build ID, which would
consistently panic before this fix. Updated the benchmark to use a shared
build ID instead of per-goroutine IDs, proving the race is resolved.

Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com>
Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/9c73c26e-828a-4b5f-89db-5e79167f2115
@dgkanatsios
Dimitris Gkanatsios (dgkanatsios) marked this pull request as ready for review March 24, 2026 03:28
@dgkanatsios

Copy link
Copy Markdown
Collaborator

Copilot can you check if the fix is included in #505 and close this one if not needed anymore?

Copilot AI commented Mar 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Copilot can you check if the fix is included in #505 and close this one if not needed anymore?

Confirmed — PR #505 includes the exact same fix for PushToQueue (replacing the split RLock/Lock with a single Lock). The only additional content in this PR is the regression test TestGameServerQueueConcurrentPushPopSameBuild and the benchmark update to use a shared build ID. Since the core fix is already in #505 and the existing "should work with multiple simultaneous requests" test covers the concurrent scenario, this PR is not needed. Closing.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh pr close 506 --comment Closing as the fix is already included in #505. (http block)
    • Triggering command: /usr/bin/gh gh pr close 506 (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants