fix: the party-split reconnect lockout, plus two guards that protected the wrong thing - #578
Merged
Merged
Conversation
… party Three coordinated changes, because the lockout had two acts and our own tolerance for it could never fire. 1. PartyPresenceList.Join treats a join whose USER is already present on a different session as a session REPLACEMENT, not a new member. Capacity is keyed on session ID, so a reconnecting player's fresh session counted against a limit their stale session was still occupying, and Join returned ErrPartyFull. PartyHandler.Join discards that error as "should not happen, this process is just a confirmation" -- so the player ended up tracked on the party stream but absent from ph.members, and the leader's matchmaking cohort silently excluded them. That is the split, and nothing surfaced it. The superseded session is now dropped as the replacement is added, so occupancy is unchanged and MaxSize still holds against genuine new members. 2. PartyHandler.JoinRequest checks identity BEFORE capacity. An existing member consumes no new slot, so telling them the party is full answers a question they did not ask. The already-member check sat below the p.Open branch, which made it unreachable for an open party -- and EVR creates only open parties. A closed party already behaved correctly; the two now agree. 3. lobbyGroup's JoinRequest caller treats already-being-a-member as success. It already meant to: it tolerated ErrPartyJoinRequestAlreadyMember, then fell through to `if !success` and returned "failed to join party" anyway, because JoinRequest returns false alongside that error. addedMember is now set only for a genuine join, so the Track rollback below stays precise. Together these are the reconnect-storm lockout in the ops case: 34 party-full and 14 lobby-full rejections over 72h for one player. The characterization tests added earlier, which pinned the defective behaviour so the diagnosis would stop being contested, failed on this change with the "this is fixed, update the assertion" message they were written to produce. They now assert the fixed behaviour, cover open and closed parties together since the asymmetry was the tell, and add a guard that the replacement path cannot be used to exceed MaxSize. Pre-existing; no party file changed in the v3.27.2-evr.320..main range. Co-authored-by: nakama-fixer <nakama-fixer@metis.agents>
Two independent fixes, both cases where a guard protected the wrong thing. PRUNE, POST-RESTART unavailableGuilds -- the record distinguishing a guild that is merely dark from one the bot was removed from -- lives only in memory. A restart erases it, and it is the only thing between a Discord read anomaly and an unrecoverable GroupDelete. Normally READY lists unavailable guilds as stubs and they are protected anyway, but a READY that omits a member guild is exactly the "Discord is lying" case this pass was hardened against, and it is most likely during the incident that caused the restart. Group DELETES are now suppressed for 30 minutes after boot, regardless of configuration: two prune intervals, enough for the gateway to settle and the first GUILD_CREATE burst to land. Leaves and the non-destructive repair pass are untouched -- re-inviting the bot undoes a leave, nothing undoes a delete. The rule is a function rather than an inline condition so it can be tested at all; the prune loop runs in a goroutine started by Start(). RESERVATION REFRESH The capacity guard ran before the upsert and could not tell a new booking from a refresh. upsertReservationByUserID deletes any existing reservation for the user before inserting, so a refresh consumes no additional slot -- but a follower in a full lobby was skipped every time their client re-sent LobbyFindSessionRequest, so their expiry was never extended. At the 5-minute mark rebuildCache dropped the reservation and a backfill player took the seat they had been holding: the party split this subsystem exists to prevent, produced by the guard meant to protect it. hasReservationForUserID distinguishes the two. Capacity still binds for genuinely new reservations, which is what the guard was added for, and a test pins that the refresh path cannot be used to overbook. Also: the signal logged "Created party reservations" at Info on every firing, including created: 0. createReservationForNewPartyMember signals on every follower re-send, so a full lobby with retrying followers produced a steady stream of Info lines announcing something that did not happen. It now reports refreshed as well as created, and drops to Debug when nothing changed. Both verified by mutation. Co-authored-by: nakama-fixer <nakama-fixer@metis.agents>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes three production-impacting edge cases in EchoVR’s Nakama fork: (1) party reconnects being incorrectly rejected/silently dropped due to session-vs-user identity handling, (2) Discord prune potentially deleting guild groups after restart when “unavailable guild” evidence is wiped, and (3) reservation refreshes being blocked in full lobbies, causing followers to lose held seats.
Changes:
- Treat party joins for an already-present user as session replacements and reorder JoinRequest identity checks ahead of capacity checks.
- Add a post-boot grace period that suppresses destructive Discord prune deletes even when configured.
- Allow reservation refreshes in full lobbies and reduce log noise by distinguishing “created” vs “refreshed” outcomes.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/party_presence.go | Adds session-replacement semantics when a reconnecting user re-joins a party. |
| server/party_handler.go | Reorders JoinRequest checks so existing members aren’t rejected by capacity first. |
| server/evr_reservation_capacity_test.go | Adds coverage for “refresh in full lobby” behavior. |
| server/evr_party_system_test.go | Updates/replaces characterization tests to assert the fixed reconnect behavior. |
| server/evr_match.go | Allows reservation refreshes in full lobbies; improves signal logging. |
| server/evr_match_label.go | Adds helper to detect whether a user already holds a reservation (refresh vs create). |
| server/evr_lobby_group.go | Correctly treats “already member” as success and fixes rollback gating. |
| server/evr_discord_integrator.go | Adds post-boot delete suppression and a testable policy helper. |
| server/evr_discord_integrator_guilddelete_test.go | Pins the post-restart destructive-prune grace policy with tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
118
to
121
| @@ -87,6 +120,23 @@ func (m *PartyPresenceList) Join(joins []*Presence) ([]*Presence, error) { | |||
| return nil, runtime.ErrPartyFull | |||
| } | |||
Comment on lines
+131
to
+136
| for _, member := range p.members.presences { | ||
| if member.Presence.UserID == presence.UserID { | ||
| p.Unlock() | ||
| return false, runtime.ErrPartyJoinRequestAlreadyMember | ||
| } | ||
| } |
Comment on lines
+2212
to
+2213
| refresh := state.hasReservationForUserID(member.GetUserId()) | ||
| if !refresh && state.OpenSlots() <= 0 { |
…itself - PartyPresenceList.Join compared the incoming batch against CURRENT occupancy while the stale sessions it is about to remove were still counted, so a batch containing both a replacement and a genuine newcomer could be refused even though it fits. Reachable when a user carries more than one stale session, which a reconnect storm could produce -- and since PartyHandler.Join discards ErrPartyFull, the refusal would have been the same silent roster drop this change exists to remove. The check now uses projected occupancy. - JoinRequest's new identity check read p.members.presences directly. PartyPresenceList has its own mutex and holding the handler lock does not confer it: JoinPartyGroup's rollback path calls members.Leave without the handler lock at all. It now reads the atomic snapshot via List(), which is what every other reader uses. (The pre-existing check at the bottom of the function had the same flaw; moving it was the opportunity to fix it.) - hasReservationForUserID returned false for a nil user ID, but the reservation system deliberately supports nil-ID members by keying on session ID -- upsertReservationByUserID says so. Their refresh was therefore classified as a new booking and skipped in a full lobby, which is the lost seat the refresh path exists to prevent. Now hasReservationFor, matching upsert's own fallback, with a test. Verified by mutation and by -race -count=2 over the party tests. Co-authored-by: nakama-fixer <nakama-fixer@metis.agents>
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
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.
The three items approved after the adversarial review. All mutation-verified;
just testgreen, party tests green under-race.1. The party-split reconnect lockout
A player who drops and reconnects gets separated from their own party. Diagnosed months ago, contested by two reviews that reached opposite conclusions, and now fixed — it had two acts, which is why each review was right about one half.
Act one, silent. Party capacity is keyed on session ID. A reconnecting player's fresh session counted against a limit their stale session was still occupying, so
PartyPresenceList.JoinreturnedErrPartyFull— andPartyHandler.Joindiscards that error as "should not happen, this process is just a confirmation." The player ended up tracked on the party stream but absent fromph.members, so the leader's matchmaking cohort silently excluded them. Nothing surfaced it;Joinreturns nothing and cannot report failure.A join whose user is already present on a different session is now a session replacement: the superseded entry is dropped as the new one is added, so occupancy is unchanged and
MaxSizestill binds against genuine new members.Act two, visible.
JoinRequestchecked capacity before identity, and the already-member check sat below thep.Openbranch — unreachable for an open party, and EVR creates only open parties. A member reconnecting into their own full party was told it was full, with no bypass for already being in it. Every retry hit the same wall, which is the reconnect-storm shape in the ops case (34 party-full + 14 lobby-full rejections over 72h for one player).Identity is now checked first, because an existing member consumes no new slot. A closed party already behaved this way — that asymmetry was the tell.
Act three, ours.
evr_lobby_group.gotoleratedErrPartyJoinRequestAlreadyMember, then fell through toif !successand returned "failed to join party" anyway, becauseJoinRequestreturnsfalsealongside that error. Someone wrote that bypass intending exactly this case and it never worked.addedMemberis now set only for a genuine join, so theTrackrollback stays precise.The characterization tests from #577 — added to stop the diagnosis being contested — failed on this change with the "this is fixed, update the assertion" message they were written to produce. They now assert the fixed behaviour, run against open and closed parties since the asymmetry was the evidence, and guard that the replacement path cannot exceed
MaxSize.2. Prune deletes after a restart
unavailableGuilds— the record distinguishing a guild that is merely dark from one the bot was removed from — lives only in memory. A restart erases it, and it is the only thing between a Discord read anomaly and an unrecoverableGroupDelete.Normally READY lists unavailable guilds as stubs and they're protected anyway. But a READY that omits a member guild is exactly the "Discord is lying" case this pass was hardened against — and it is most likely during the incident that caused the restart.
Group deletes are suppressed for 30 minutes after boot regardless of configuration (two prune intervals). Leaves and the non-destructive repair pass are untouched: re-inviting the bot undoes a leave, nothing undoes a delete.
3. Reservation refresh in a full lobby
The capacity guard ran before the upsert and couldn't tell a new booking from a refresh.
upsertReservationByUserIDdeletes any existing reservation before inserting, so a refresh is slot-neutral — but a follower in a full lobby was skipped every time their client re-sentLobbyFindSessionRequest. Their expiry was never extended, and at the 5-minute markrebuildCachedropped the reservation and a backfill player took the seat they were holding.The guard meant to prevent party splits was causing one.
hasReservationForUserIDdistinguishes the two; capacity still binds for new reservations, pinned by a test.Also quieted the signal's logging: it announced "Created party reservations" at Info on every firing including
created: 0, and that signal fires on every follower re-send. It now reportsrefreshedalongsidecreatedand drops to Debug when nothing changed.Verification
just testgreen; party tests green under-race -count=2Co-authored-by: nakama-fixer nakama-fixer@metis.agents