Fixing issue #301

[aniket866]

Solved issue : the "Matchmaking Limbo" race condition, we need to change the logic so users are not removed from the pool until the room is successfully created in the database. Instead, we will mark them as IsMatching.

Fix Strategy: "Reserve" before "Remove"

• Add State: Add an IsMatching flag to the user struct.

1. Reserve: In findMatch, set IsMatching = true (locking the users) instead of deleting them.

2. Commit/Rollback: In createRoomForMatch, delete them on success, or reset IsMatching = false on failure.

Video or screenshot : N/A logical changes

@bhavik-mangla closes #301

Summary by CodeRabbit

• Bug Fixes
  • Improved matchmaking reliability by preventing users from being re-selected during the matching process.
  • Enhanced eligibility checks to ensure only appropriate candidates are matched.
  • Fixed room creation flow to properly handle matched player reservations.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces an IsMatching state flag to the MatchmakingPool struct to reserve users during matchmaking. Users are marked as matching instead of immediately removed from the pool, with deletion deferred until successful room creation. This prevents users from entering an unrecoverable state if room creation fails.

Changes

Cohort / File(s)	Summary
Matchmaking Pool State Management backend/services/matchmaking.go	Added IsMatching bool field to reserve users during matching. Updated matching eligibility checks to skip users already marked as matching. Modified room creation flow to defer deletion until after successful database commit. Added cleanup handling for test mode when database is nil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A match reserved, not yet undone,
Two hearts paused mid dance, waiting as one,
Until the room's ready, doors wide and true,
Then safely committed, their journey renewed. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fixing issue #301' is vague and generic, using non-descriptive phrasing that doesn't convey meaningful information about the actual changeset (race condition fix with IsMatching flag).	Revise the title to be more descriptive, such as 'Fix matchmaking race condition with IsMatching flag' to clearly summarize the main change.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The pull request successfully implements all core requirements from issue #301: adds IsMatching flag, reserves users instead of immediate deletion, and implements commit/rollback logic on room creation.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the race condition described in issue #301; minor formatting adjustments in room payload and data structures are incidental to the main fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

backend/services/matchmaking.go (1)

210-215: ⚠️ Potential issue | 🔴 Critical

Critical bug: rollback never resets IsMatching = false — users are stuck in limbo again.

When InsertOne fails, you re-assign the user pointers into the pool map (which is redundant since they were never removed), but you never reset IsMatching to false. Both users will remain permanently skipped by findMatch (line 133 and 145 guard against IsMatching == true). This reintroduces the exact "Matchmaking Limbo" the PR aims to fix.

🐛 Proposed fix

 	if err != nil {
 		ms.mutex.Lock()
-		ms.pool[user1.UserID] = user1
-		ms.pool[user2.UserID] = user2
+		user1.IsMatching = false
+		user2.IsMatching = false
 		ms.mutex.Unlock()
 		return
 	}

The users are already in the pool (they were never deleted), so re-assigning them is unnecessary. The only thing the rollback needs to do is clear the reservation flag.

🧹 Nitpick comments (3)

backend/services/matchmaking.go (3)

268-275: Minor: periodicMatchmaking fires goroutines for already-reserved users.

The StartedMatchmaking check on line 272 doesn't filter out users with IsMatching == true. Each tick will spawn a findMatch goroutine for reserved users that immediately returns at line 133. Not a bug, but wasteful under load.

♻️ Suggested filter

 		for userID, poolEntry := range ms.pool {
-			if poolEntry.StartedMatchmaking {
+			if poolEntry.StartedMatchmaking && !poolEntry.IsMatching {
 				usersToMatch = append(usersToMatch, userID)
 			}

---

228-244: Edge case: cleanupInactiveUsers can delete a reserved user mid-match.

If a user's LastActivity exceeds 5 minutes while IsMatching == true (e.g., slow DB), the cleanup goroutine will delete them from the pool. A subsequent successful RemoveFromPool would be a no-op, which is fine. However, if room creation fails, the rollback (once the critical bug above is fixed) would reset IsMatching on a user pointer that's no longer in the map — silently losing them.

Consider skipping IsMatching users during cleanup:

♻️ Suggested guard

 		for userID, poolEntry := range ms.pool {
-			if now.Sub(poolEntry.LastActivity) > 5*time.Minute {
+			if now.Sub(poolEntry.LastActivity) > 5*time.Minute && !poolEntry.IsMatching {
 				delete(ms.pool, userID)
 			}

---

220-226: Stray formatting: extra blank lines and closing brace alignment.

Lines 220 and 225 introduce extra blank lines inside the function. This is cosmetic but mildly inconsistent with the rest of the file. Also verify that line 226 is the function's closing brace and not a stray — the indentation in the diff looks off compared to other function closings.

[bhavik-mangla]

you need to include evidences of testing
logical changes or ui changes - testing is required